Re: [PATCH v2 1/2] Input: mms114 - use smbus functions whenever possible

[Stephan Gerhold <stephan@gerhold.net>]

Hi Dmitry,

On Tue, Oct 22, 2019 at 02:18:39PM +0300, Andi Shyti wrote:
> Hi Dmitry,
> 
> > > > > > On Sun, Oct 20, 2019 at 11:28:55PM +0300, Andi Shyti wrote:
> > > > > > > The exchange of data to and from the mms114 touchscreen never
> > > > > > > exceeds 256 bytes. In the worst case it goes up to 80 bytes in
> > > > > > > the interrupt handler while reading the events.
> > > > > > > 
> > > > > > 
> > > > > > i2c_smbus_read_i2c_block_data() is actually limited to
> > > > > > I2C_SMBUS_BLOCK_MAX = 32.
> > > > > 
> > > > > oh sorry, I don't know how I slipped on this.
> > > > > 
> > > > > But this means that the i2c in the kernel is wrong (or outdated),
> > > > > smbus specifies 256 bytes of data[*]. I might have relied on the
> > > > > specification more than the code.
> > > > > 
> > > > > I guess SMBUS needs some update.
> > > > 
> > > > You are right. It seems like that part of the specification was changed
> > > > with SMBus version 3.0 [1]:
> > > > 
> > > >   D.6 255 Bytes in Process Call:
> > > >     The maximum number of bytes allowed in the Block Write-Block Read
> > > >     Process Call (Section 6.5.8) was increased from 32 to 255.
> > > > 
> > > > [1]: http://www.smbus.org/specs/SMBus_3_0_20141220.pdf
> > > 
> > > yes :)
> > > 
> > > OK, then I would ask Dmitry to hold on this patch I will try to
> > > update the smbus properly.
> > 
> > 3.0 is from 2014, so we can't simply update the limits. And we need to
> > handle the case where device connected to a controller that does not
> > implement 3.0 standard.
> 
> actually I don't see why, given that devices that were sending
> 32bytes will keep sending 32bytes and in any case I still haven't
> seen a controller that is strictly compliant to SMBUS 2. The
> mms114 device is a good example (and I think most of the
> touchscreens don't really care of the 32byte limit).
> 
> In any case, I agree that I can't simply update the
> I2C_SMBUS_BLOCK_MAX because for sure I might have forgotten some
> cases and I'm currently looking how to do it. I have a few ideas
> but no one is good. I planned to send an RFC sometimes soon in
> order to kickstart some discussion.
> 
> > If regmap is too much work then as a stop gap we could maybe only
> > convert write functions and mention why read needs to be custom.
> 
> regmap is not too much work but I don't have the device with me,
> I might get one at some point, but can't do anything right now.
> 
> Perhaps, for now you can take Stephan's patches and I would
> update everything once I get the device. They are not mutually
> exclusive, anyway.

What do you think about this?

My patches for MMS345L keep behavior for MMS114 and MMS152 as-is,
so we can be sure that there are no regressions for them.

Cleaning up the driver to use SMBUS and/or regmap instead of the custom
read/write methods is definitely something we should attempt to do
at some point, but only when we can properly test them on MMS114.

Until we have a MMS114 test device available, I would say that
applying my patches is the least intrusive way to make MMS345L work.

Thanks,
Stephan