Message-ID: <CADvTj4o8jSnQewB8bhCxGkOQrRPN9+EsqPg7D5+s7O2rkBMK8Q@mail.gmail.com>
Date:   Sun, 29 Oct 2023 03:05:59 -0600
From:   James Hilliard <james.hilliard1@...il.com>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:     linux-input@...r.kernel.org,
        Linus Walleij <linus.walleij@...aro.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] Input: cyttsp5 - improve error handling and remove regmap

On Sat, Oct 28, 2023 at 9:35 PM Dmitry Torokhov
<dmitry.torokhov@...il.com> wrote:
>
> 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@...il.com>
> > 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.

It doesn't seem to work reliably, for example I would often see this error:
[    2.234089] cyttsp5 2-0024: HID output cmd execution timed out
[    2.239957] cyttsp5 2-0024: Error on launch app r=-110
[    2.245150] cyttsp5 2-0024: Fail initial startup r=-110
[    2.257502] cyttsp5: probe of 2-0024 failed with error -110

Note that cyttsp5_hid_output_bl_launch_app is called immediately after
cyttsp5_deassert_int with the existing driver so presumably it doesn't actually
clear the interrupt line correctly.

Some more details:
https://lore.kernel.org/all/CADvTj4pdSkg5RWGThmj8Z_gOL5g2Ovhvfc-XtYTU88_0ve4NPw@mail.gmail.com/

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

Mostly startup issues with cyttsp5_deassert_read and retry logic from what I
can tell. It appears that in some cases(such as when the system is rebooted
without fully powering off) multiple i2c reads are often required to flush some
sort of firmware side message queue so that the firmware is in a state in which
it will respond to commands normally.

I don't understand the reason this driver was written using regmap in the first
place, from my understanding of the protocol using regmap is not appropriate
as you can't correctly model an i2c-hid like protocol(like cyttsp5 uses) using
regmap.

Note that upstream drivers like i2c-hid don't use regmap either.

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

Ok, I'll see what I can do there.

>
> Thanks.
>
> --
> Dmitry

Powered by blists - more mailing lists