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

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Mon, Apr 20, 2020 at 11:23:57PM -0400, Paul Thomas wrote:
> 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?

I understand completely your goal, but
- for I²C expanders timings is the last thing to look for (they are quite slow
  by nature), so, I really don't care about 16% speed up for one call; don't
  forget that we are in multi-task OS, where this can be easily interrupted and
  user will see the result quite after expected quick result
- the code maintenance has a priority over micro-optimization (this driver
  suffered many times of breakages because of some optimizations done)
- it breaks Uwe's approach to fix AI chips, after my patch Uwe's ones are
  applied cleanly

>That being said, the
> pca953x_read_regs() is still far better than calling regmap_read() 8
> times.

Yes, it's a best compromise I can come with.

> 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.

That's not an argument at all. This is feature request, not the fix.
Uwe's ones, though, are fixes, and thus much more important.

> Have you
> tested this with actual hardware?

Do you see any issues with it?

For the record, yes, on Intel Edison/Arduino where 4 PCA9555 are present.

% gpiodetect
gpiochip0 [0000:00:0c.0] (192 lines)
gpiochip1 [i2c-INT3491:00] (16 lines)
gpiochip2 [spi-PRP0001:00] (4 lines)
gpiochip3 [i2c-INT3491:01] (16 lines)
gpiochip4 [i2c-INT3491:02] (16 lines)
gpiochip5 [i2c-INT3491:03] (16 lines)

### Consider gpiochip1: 8 and 10 are PUed

% gpioget gpiochip1 0 1 2 3 6 7 8 9 10 11 14 15
[  201.416858] pca953x i2c-INT3491:00: pca953x_gpio_get_multiple <<< 00,0000b58f
1 1 1 1 0 1 1 0 1 0 0 1

### only 8

% gpioget gpiochip1 0 1 2 3 6 7 8 9 10 11 14 15
[  354.153921] pca953x i2c-INT3491:00: pca953x_gpio_get_multiple <<< 00,0000b18f
1 1 1 1 0 1 1 0 0 0 0 1

### only 10

% gpioget gpiochip1 0 1 2 3 6 7 8 9 10 11 14 15
[  362.723867] pca953x i2c-INT3491:00: pca953x_gpio_get_multiple <<< 00,0000b48f
1 1 1 1 0 1 0 0 1 0 0 1

### None of them

% gpioget gpiochip1 0 1 2 3 6 7 8 9 10 11 14 15
[  371.294910] pca953x i2c-INT3491:00: pca953x_gpio_get_multiple <<< 00,0000b08f
1 1 1 1 0 1 0 0 0 0 0 1

Yes, I specifically added a debug print since GPIO trace does not distinguish
between plain get and complex one.

  dev_info(&chip->client->dev, "%s <<< %*pb\n", __func__, MAX_LINE, reg_val);

> 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.

> [1] Tested using 16-bit max7312

-- 
With Best Regards,
Andy Shevchenko