Re: [PATCH] pinctrl: intel: save HOSTSW_OWN register over suspend/resume

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>]

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@xxxxxxxxx> 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@xxxxxxxxx> 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