Re: [PATCH v3 1/3] gpio: pca953x: Rewrite ->get_multiple() function

[Paul Thomas <pthomas8589@xxxxxxxxx>]

On Mon, Apr 20, 2020 at 1:27 PM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> The commit 96d7c7b3e654 ("gpio: gpio-pca953x, Add get_multiple function")
> basically did everything wrong from style and code reuse perspective, i.e.
Hi Andy,

Well your version is certainly elegant and simple, and does better
with code reuse. However there are a couple of other goals I had in
mind.
First, the "lazy" approach of 96d7c7b3e654 is actually faster when
user space sets up a 8-bit linehandle[1]146us (single regmap_read())
vs 172us (pca953x_read_regs()) which incidentally is what we do in our
application. In lazily reading 1 byte at a time it is the fastest
access for that, if user space is always setting up the linehandle for
the whole chip pca953x_read_regs() would be faster. Seeing as
get_multiple has been unimplemented for this chip until now perhaps
our use case deserves some consideration? That being said, the
pca953x_read_regs() is still far better than calling regmap_read() 8
times.

Second, your version does not work with a 5.2 kernel, bitmap_replace
is not there yet and pca953x_read_regs() signature is different. So
perhaps we'll all move on and no one will care about 5.2, but as
that's what we are using that was the basis for the patch. Have you
tested this with actual hardware? I actually didn't do a proper test
just the timing of the pca953x_read_regs().

> - it didn't utilize existing PCA953x internal helpers
> - it didn't utilize bitmap API
> - it misses the point that ilog2(), besides that BANK_SFT is useless,
>   can be used in macros
Yes, I know ilog2() can be used in macros, I didn't think it was worth
including the .h file just to calculate 3. Putting the ilog2(x) in the
comments seemed to be common in other kernel sections, but maybe that
was historic before the macro version? Either way is fine. The shift
is not useless, without that you would go into the if statement for
every bit, but you only want to do a regmap_read() for every byte.

> - it has indentation issues.
It passed checkpatch.pl, and any indentation fixes are fine with me.

thanks,
Paul

[1] Tested using 16-bit max7312