[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAMRc=Mffr4pn+mnuO6WVP9p3JT-G_t8buJBZMBBRFjQDsfLeuw@mail.gmail.com>
Date: Fri, 6 Nov 2020 12:13:55 +0100
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Mark Brown <broonie@...nel.org>
Cc: Linus Walleij <linus.walleij@...aro.org>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Jan Kiszka <jan.kiszka@...mens.com>,
David Laight <David.Laight@...lab.com>,
"open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Bartosz Golaszewski <bgolaszewski@...libre.com>
Subject: Re: [RFT PATCH v2 7/8] gpio: exar: switch to using regmap
On Thu, Nov 5, 2020 at 6:41 PM Mark Brown <broonie@...nel.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
Powered by blists - more mailing lists