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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Apr 22, 2020 at 12:33:34AM -0400, Paul Thomas wrote:
> On Tue, Apr 21, 2020 at 10:21 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Tue, Apr 21, 2020 at 04:06:24PM +0200, Uwe Kleine-König wrote:
> > > On Tue, Apr 21, 2020 at 03:55:53PM +0300, Andy Shevchenko wrote:
> > > > 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@linux.intel.com> wrote:

...

> > > > 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
> Sure, it's not a HUGE deal, but this will get worse for 5 bank chips.
> Also, 26us is not insignificant, with the preempt-rt kernel we're
> using that can be more than the max interrupt latency.

Using slow buses, no, not just using, but relying on them, in RT environment
seems pretty much bad hardware design. If you have time critical solution,
it should try its best by excluding points of possible failures.

> > > I didn't do any timing analysis and while I understand your
> > > argumentation, I'm not sure to agree. I noticed while debugging the
> > > problem that then resulted in my fix that gpioctl uses the .set_multiple
> > > callback. I told my customer to use gpioctl instead of /sys/class/gpio
> > > because it performs better just to notice that when using gpioctl to set
> > > a single GPIO this transfers five bytes instead of only two.
> > >
> > > Having said that I think the sane approach is to optimize
> > > .{g,s}et_multiple to reduce the read/write size to the smallest bulk
> > > size possible that covers all bits that have their corresponding bit set
> > > in mask.
> > >
> > > > - the code maintenance has a priority over micro-optimization (this driver
> > > >   suffered many times of breakages because of some optimizations done)
> Yeah, I appreciate that maintainability needs to be a big priority,
> I'm wondering if comments & syntax could be improved so that the
> general idea of 96d7c7b3e654 is clear and maintainable. It is just
> walking through mask, and whenever it gets to a new byte it reads it
> from the hardware.

I think the idea per se is worth to consider, though it should not be limited
to the ->get_multiple(), for example reading IRQ status can advantage of this
as well. So, it should be rather some common helper which takes mask as input
parameter and converts it to the set of registers to read. regmap API, IIRC,
has support of sparse reading. I dunno, though, if it is supported by regmap
I²C case.

> > > ack here. Some parts of the driver were harder to grasp than necessary.
> > >
> > > > - it breaks Uwe's approach to fix AI chips, after my patch Uwe's ones are
> > > >   applied cleanly
> > >
> > > I didn't check, is 96d7c7b3e654 broken for some chips?
> >
> > I was referring to another call to recalc address with additional parameters,
> > which your second patch actually gets rid of.
> 
> If it's just the call to pca953x_recalc_addr() that caused the
> conflict with Uwe's work with 96d7c7b3e654, we can just remove the
> last two arguments so it matches what pca953x_gpio_get_value() is
> doing.

Let's rethink entire approach. See above.

-- 
With Best Regards,
Andy Shevchenko