[RFT,v2,7/8] gpio: exar: switch to using regmap

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

We can simplify the code in gpio-exar by using regmap. This allows us to
drop the mutex (regmap provides its own locking) and we can also reuse
regmap's bit operations instead of implementing our own update function.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/Kconfig     |  1 +
 drivers/gpio/gpio-exar.c | 87 ++++++++++++++++------------------------
 2 files changed, 35 insertions(+), 53 deletions(-)

On Wed, Nov 4, 2020 at 9:34 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

...

> +static const struct regmap_config exar_regmap_config = {

> +       .name           = "exar-gpio",

> +       .reg_bits       = 8,

> +       .val_bits       = 8,

> +};


Looking at gpio-pca953xx regmap conversion I'm wondering shouldn't you
provide a callback to define volatile registers (such as GPIO input
bits)?

-- 
With Best Regards,
Andy Shevchenko

On Wed, Nov 4, 2020 at 9:35 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Nov 4, 2020 at 9:34 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> ...
>
> > +static const struct regmap_config exar_regmap_config = {
> > +       .name           = "exar-gpio",
> > +       .reg_bits       = 8,
> > +       .val_bits       = 8,
> > +};
>
> Looking at gpio-pca953xx regmap conversion I'm wondering shouldn't you
> provide a callback to define volatile registers (such as GPIO input
> bits)?
>
> --
> With Best Regards,
> Andy Shevchenko

I think this was done in pca953x due to weird calculations of banks
and registers. For a rather simple driver like this one I don't think
this is needed.

Bartosz

On Wed, Nov 04, 2020 at 08:30:50PM +0100, Bartosz Golaszewski wrote:

> @@ -119,21 +81,39 @@ static void exar_set_value(struct gpio_chip *chip, unsigned int offset,

>  	unsigned int addr = exar_offset_to_lvl_addr(exar_gpio, offset);

>  	unsigned int bit = exar_offset_to_bit(exar_gpio, offset);

>  

> -	exar_update(chip, addr, value, bit);

> +	regmap_assign_bits(exar_gpio->regs, addr, BIT(bit), value);

>  }


This appears to be the use of _assign_bits() and TBH I'm still both
having a hard time understanding the motivation for it and liking the
name, especially since AFAICT it's only setting a single bit here.  The
above is just

	regmap_update_bits(exar_gpio->regs, addr, 1 << bit, value << bit);

AFAICT (and indeed now I dig around assign_bit() only works on a single
bit and does both shifts which makes the correspondance with that
interface super unclear, we're not mirroring that interface here).  If
you're trying to clone the bitops function it should probably be an
actual clone of the bitops function not something different, that would
be clearer and it'd be easier to understand why someone would want the
API in the first place.  But perhaps I'm missing something here?

On Thu, Nov 5, 2020 at 6:41 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Nov 04, 2020 at 08:30:50PM +0100, Bartosz Golaszewski wrote:
>
> > @@ -119,21 +81,39 @@ static void exar_set_value(struct gpio_chip *chip, unsigned int offset,
> >       unsigned int addr = exar_offset_to_lvl_addr(exar_gpio, offset);
> >       unsigned int bit = exar_offset_to_bit(exar_gpio, offset);
> >
> > -     exar_update(chip, addr, value, bit);
> > +     regmap_assign_bits(exar_gpio->regs, addr, BIT(bit), value);
> >  }
>
> This appears to be the use of _assign_bits() and TBH I'm still both
> having a hard time understanding the motivation for it and liking the
> name, especially since AFAICT it's only setting a single bit here.  The
> above is just
>
>         regmap_update_bits(exar_gpio->regs, addr, 1 << bit, value << bit);
>
> AFAICT (and indeed now I dig around assign_bit() only works on a single
> bit and does both shifts which makes the correspondance with that
> interface super unclear, we're not mirroring that interface here).  If
> you're trying to clone the bitops function it should probably be an
> actual clone of the bitops function not something different, that would
> be clearer and it'd be easier to understand why someone would want the
> API in the first place.  But perhaps I'm missing something here?

It's true that bitops set/clear/assign bit macros work on single bits
and take their offsets as arguments. However all regmap helpers
operate on masks. Two release cycles back we added two helpers
regmap_set_bits() and regmap_clear_bits() which are just wrappers
around regmap_update_bits(). The naming was inspired by bitops
(because how would one name these operations differently anyway?) but
it was supposed to be able to clear/set multiple bits at once - at
least this was my use-case in mtk-star-emac driver I was writing at
the time and for which I wrote these helpers.

Now the regmap_assign_bits() helper is just an extension to these two
which allows users to use one line instead of four. I'm not trying to
clone bitops - it's just that I don't have a better idea for the
naming.

Bartosz

On Fri, Nov 06, 2020 at 12:13:55PM +0100, Bartosz Golaszewski wrote:
> On Thu, Nov 5, 2020 at 6:41 PM Mark Brown <broonie@kernel.org> wrote:

> > AFAICT (and indeed now I dig around assign_bit() only works on a single
> > bit and does both shifts which makes the correspondance with that
> > interface super unclear, we're not mirroring that interface here).  If
> > you're trying to clone the bitops function it should probably be an
> > actual clone of the bitops function not something different, that would
> > be clearer and it'd be easier to understand why someone would want the
> > API in the first place.  But perhaps I'm missing something here?

> It's true that bitops set/clear/assign bit macros work on single bits
> and take their offsets as arguments. However all regmap helpers
> operate on masks. Two release cycles back we added two helpers
> regmap_set_bits() and regmap_clear_bits() which are just wrappers
> around regmap_update_bits(). The naming was inspired by bitops
> (because how would one name these operations differently anyway?) but
> it was supposed to be able to clear/set multiple bits at once - at
> least this was my use-case in mtk-star-emac driver I was writing at
> the time and for which I wrote these helpers.

Which is fine and not at all unclear since there's no separate value
argument, the value comes along with the name.  

> Now the regmap_assign_bits() helper is just an extension to these two
> which allows users to use one line instead of four. I'm not trying to
> clone bitops - it's just that I don't have a better idea for the
> naming.

I really don't see the benefit to the helper, it makes sense in the
context of bitops where the operation does all the shifting and it's
only a single bit but for regmap where it's dealing with bitmasks as
well and the naming doesn't make it crystal clear I can only see this
being confusing to people.  Had the set and clear helpers for regmap
been done as single bits it'd be a lot easier but that's not the case
and it'd also be odd to have just this one helper that took a shift
rather than a bitmask.