lkml.org 
[lkml]   [2019]   [Apr]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] pinctrl: intel: save HOSTSW_OWN register over suspend/resume
On Thu, Apr 04, 2019 at 09:06:04PM +0800, Chris Chiu wrote:
> On Wed, Apr 3, 2019 at 9:06 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Wed, Apr 03, 2019 at 03:06:43PM +0800, Chris Chiu wrote:
> > > On Tue, Apr 2, 2019 at 7:58 PM Andy Shevchenko
> > > <andriy.shevchenko@intel.com> wrote:

> > This better to make as a separate helper function
> >
> > static u32 intel_gpio_is_requested(chip, base, size)
> > {
> > u32 requested = 0;
> > unsigned int i;
> >
> > for () {
> > if ()
> > requested |= BIT();
> > }
> > return requested;
> > }
> >
> > (Note u32 as a type)
> >
>
> Thanks. I made a minor modification for the check function. I think to
> pass a padgroup
> as the argument would be better instead of base, size which I may need
> to check if
> the size > 32 (of course it shouldn't happen) or not.

Group size is never bigger than 32 pins.

The helper should be pure GPIO, that's why I still would like to see base
there. Otherwise it would be layering violation.

> +intel_padgroup_has_gpio_requested(struct gpio_chip *chip, const
> struct intel_padgroup *gpp)

Namespace is intel_gpio_ here.

> +{
> + u32 requested = 0;
> + int i;
> +
> + if (gpp == NULL)
> + return 0;
> +
> + if (gpp->gpio_base < 0)
> + return 0;
> +
> + for (i = 0; i < gpp->size; i++)
> + if (gpiochip_is_requested(chip, gpp->gpio_base + i))
> + requested |= BIT(i);
> +
> + return requested;
> +}

> > > + if (requested) {
> > > + if (communities[i].hostown[gpp] !=
> > > readl(base + gpp * 4)) {
> > > +
> > > writel(communities[i].hostown[gpp], base + gpp * 4);
> >
> > The idea here not to check this at all, but rather apply a mask.
> >
> > u32 value;
> >
> > ...
> > value = readl();
> > value = (value & ~requested) | (hostown[gpp] & requested);
> > writel(value);
> >
>
> I made the following per your suggestion. So basically I don't need to show a
> warning for the abnormal HOSTSW_OWN value change? I will submit a formal
> patch for review if there's no big problem for these code logic. Please advise
> if any. Thanks.

You still have all data to produce a warning if it's needed.

((value ^ hostown[gpp]) & requested) will return the changed bits.


> + base = community->regs + community->hostown_offset;
> + for (gpp = 0; gpp < community->ngpps; gpp++) {
> + const struct intel_padgroup *padgrp =
> &community->gpps[i];
> + u32 requested =
> intel_padgroup_has_gpio_requested(&pctrl->chip, padgrp);
> +

> + if (requested) {

You may not need this check at all.

> + u32 value = readl(base + gpp * 4);
> + u32 saved = communities[i].hostown[gpp];
> +
> + value = (value & ~requested) | (saved
> & requested);
> + writel(value, base + gpp * 4);

It's possible to split this as well to another helper function.

static void intel_gpio_update_pad_mode(void __iomem *hostown, u32 mask, u32 value)
{
...
}

> + dev_dbg(dev, "restored hostown %d/%u
> %#08x\n", i, gpp,
> + readl(base + gpp * 4));
> + }
> + }

--
With Best Regards,
Andy Shevchenko


\
 
 \ /
  Last update: 2019-04-04 16:00    [W:0.077 / U:5.328 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site