Re: [PATCH v2] Input: cyttsp5 - improve error handling and remove regmap

[Dmitry Torokhov]

On Sat, Oct 28, 2023 at 03:31:00AM -0600, James Hilliard wrote:
> On Fri, Oct 27, 2023 at 1:59 PM Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> wrote:
> 
> > Hi James,
> >
> > On Tue, Oct 24, 2023 at 07:39:38PM -0600, James Hilliard wrote:
> > > The vendor cyttsp5 driver does not use regmap for i2c support, it
> > > would appear this is due to regmap not providing sufficient levels
> > > of control to handle various error conditions that may be present
> > > under some configuration/firmware variants.
> > >
> > > To improve reliability lets refactor the cyttsp5 i2c interface to
> > > function more like the vendor driver and implement some of the error
> > > handling retry/recovery techniques present there.
> >
> > Sorry but you need to elaborate more on what is missing in regmap and
> > how vendor code is better. In my experience vendors rarely follow kernel
> > development and either are not aware of the latest kernel APIs, or they
> > simply have the driver written to what we had in 3.x kernels and have
> > not really updated it since then.
> >
> 
> I'm unaware of a way to do essentially raw reads when using regmap, for
> example I don't know of a way to implement the cyttsp5_deassert_read
> function using regmap, maybe there's a way I'm not aware of however?

What is wrong with current way of reading from the input register? It
should clear the interrupt line.

> 
> In general the issue with regmap seems to be that regmap always does
> operations against specific registers and prevents doing raw i2c operations
> needed to handle some hardware/firmware issues for some variants.

What are those issues and why do they need raw access.

> 
> Note that I'm not exactly doing things the same way the vendor driver does,
> I have simplified the error recovery/retry code paths in the startup
> function.
> 
> 
> >
> > >
> > > As part of this rather than assuming the device is in bootloader mode
> > > we should first check that the device is in bootloader and only
> > > attempt to launch the app if it actually is in the bootloader.
> >
> > I would prefer if this was split into a separate patch.
> >
> 
> I think this change is somewhat intertwined with the probe retry/recovery
> logic
> changes and is a bit tricky to split out without breaking the startup
> sequence
> from my testing at least.

I understand that it might be tricky but each logical change should
stand on its own.

Thanks.

-- 
Dmitry