Re: [PATCH v6 0/2] Input: Add Cypress Gen5 Touchscreen driver

[MylÃne Josserand]

Hello Dmitry,

On Mon, 13 Aug 2018 08:36:32 -0700
Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> wrote:

> Hi MylÃne,
> 
> On Mon, Aug 13, 2018 at 8:24 AM MylÃne Josserand
> <mylene.josserand@xxxxxxxxxxx> wrote:
> >
> > Hi Dmitry,
> >
> > On Tue, 24 Jul 2018 10:40:53 -0700
> > Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> wrote:
> >  
> > > Hi MylÃne,
> > >
> > > On Tue, Jul 24, 2018 at 03:00:46PM +0200, MylÃne Josserand wrote:  
> > > > Hello Dmitry,
> > > >
> > > > On Wed, 4 Jul 2018 16:21:58 +0000
> > > > Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> wrote:
> > > >  
> > > > > Hi MylÃne,
> > > > >
> > > > > On Tue, Jul 03, 2018 at 11:43:07AM +0200, MylÃne Josserand wrote:  
> > > > > > Hello,
> > > > > >
> > > > > > Here is a V6 series to add the driver of the touchscreen Cypress,
> > > > > > TrueTouch Generation 5.
> > > > > > Based on v4.18-rc3.
> > > > > >
> > > > > > This patch series has already been posted in several iterations:
> > > > > >     - v1: Sent on 2017/05/29
> > > > > >     - v2: Sent on 2017/08/18
> > > > > >     - v3: Sent on 2017/09/27
> > > > > >     - v4: Sent on 2017/12/01
> > > > > >     - v5: Sent on 2017/12/20
> > > > > >
> > > > > > I did not have any comments the last 4 versions.
> > > > > > And no reviews on my v5 during 6 months. Could I have any updates
> > > > > > or feedback on my series to know why it is not merged (to be able to
> > > > > > correct what is wrong)?  
> > > > >
> > > > > Sorry, I must have missed the v5, sorry about that.
> > > > >
> > > > > I probably asked this question before, but just to make sure - I see
> > > > > references to HID in the patch - the device is really not HID
> > > > > compatible? Is there any hope it could be made work with i2c-hid +
> > > > > hid-multitouch?
> > > > >
> > > > > Thanks.
> > > > >  
> > > >
> > > > I have checked and, for what I have seen, all the HID descriptor stuff
> > > > is HID compliant. We could definitely use i2c-hid and hid-multitouch
> > > > (there is the "hid-cypress" driver that exists also).
> > > >
> > > > The only problem is that this touchscreen has two modes: a bootloader
> > > > mode and an application mode (which is the one where we can send
> > > > HID commands). After a power-on-reset, it is always in "bootloader"
> > > > mode so we need to send some commands (called "bootloader commands") to
> > > > switch to application mode.  
> > >
> > > Is this a documented or observed behavior? In my practice devices (I am
> > > talking in general, not about Cypress) that have proper configuration
> > > loaded and that were brought up with appropriate power up sequence and
> > > timings automatically switch to application mode. They only end up in
> > > bootloader mode when proper power up sequence is not respected and they
> > > are unhappy.  
> >
> > I have checked and indeed, if everything is correctly performed, the
> > bootloader has a timeout to switch to application mode.
> > The datasheet says that this timeout can be configured and the "0" value
> > means that the bootloader will never automatically switch to application
> > unless a bootloader command is sent.
> >
> > In our case, you were right, after a timeout, the touchscreen is
> > correctly switching to Application mode. Great news!
> >  
> > >  
> > > > These commands are not HID-compliant as the
> > > > datasheet indicates:
> > > >
> > > > "Bootloader commands are not HID-over-I2C compliant."  
> > >
> > > Any chance you could share the datasheet?  
> >
> > Sorry, it is not possible, the datasheet is under NDA :(
> >  
> > >  
> > > >
> > > > I think that if the touchscreen would start directly in "application"
> > > > mode, we could directly use i2c-hid and hid-cypress drivers.
> > > > Unfortunately, this is not the case.
> > > >
> > > > In bootloader mode, the ProductID is 0xc101 and in application mode, it
> > > > is 0xc001 (already available in hid-ids.h:
> > > > USB_DEVICE_ID_CYPRESS_TRUETOUCH but not handled)
> > > >
> > > > What would be the better approach here?
> > > > Should I add a new product ID to detect the bootloader mode in
> > > > hid-cypress driver and send non-HID commands to switch to
> > > > "application" mode in this driver?
> > > > Anyway, I guess that I will drop this cyttsp5 driver and update the
> > > > existing one, right?  
> > >
> > > So it still accessible through HID, even when in bootloader mode? OK,
> > > then I guess there are 2 options:
> > >
> > > - if device is documented to always start in bootloader mode, you could
> > >   have a small stub driver that switches it into application mode in its
> > >   probe() code. The "bootloader" device will disappear and
> > >   "application" device will appear, and standard driver (hid-multitouch)
> > >   will bind to it.  
> >
> > Okay, I see. In our case, we do not have the timeout to 0 as after a
> > moment, the application mode is automatically switched.
> >  
> > >
> > > - if device supposed to come up in application mode unless configuration
> > >   is damaged: I'd recommend doing what we do on Chrome OS and have some
> > >   userspace software that runs on boot (or whenever device is
> > >   discovered) and check if it has the latest firmware/configuration, and
> > >   repair device if needed.  
> >
> > I see.
> >
> > I tried to make the i2d-hid & hid-cypress working. This is not
> > successful for the moment because I can not retrieve the correct
> > bcdVersion. While debugging, I have noticed that the HID descriptors
> > don't seem to be exactly the same:
> >
> > under i2c-hid.c:
> >
> > struct i2c_hid_desc {
> >         __le16 wHIDDescLength;
> >         __le16 bcdVersion;
> >         __le16 wReportDescLength;
> >         __le16 wReportDescRegister;
> >         __le16 wInputRegister;
> >         __le16 wMaxInputLength;
> >         __le16 wOutputRegister;
> >         __le16 wMaxOutputLength;
> >         __le16 wCommandRegister;
> >         __le16 wDataRegister;
> >         __le16 wVendorID;
> >         __le16 wProductID;
> >         __le16 wVersionID;
> >         __le32 reserved;
> > } __packed;
> >
> > whereas in my driver, I have:
> >
> > struct cyttsp5_hid_desc {
> >         __le16 hid_desc_len;
> >         u8 packet_id;       <-- Different
> >         u8 reserved_byte;   <-- Different
> >         __le16 bcd_version;
> >         __le16 report_desc_len;
> >         __le16 report_desc_register;
> >         __le16 input_register;
> >         __le16 max_input_len;
> >         __le16 output_register;
> >         __le16 max_output_len;
> >         __le16 command_register;
> >         __le16 data_register;
> >         __le16 vendor_id;
> >         __le16 product_id;
> >         __le16 version_id;
> >         u8 reserved[4];
> > } __packed;
> >
> > Have you already seen devices that are "HID compatible" with a different
> > HID descriptor's content like this?  
> 
> That seems like a violation of Microsoft I2C HID protocol:
> https://docs.microsoft.com/en-us/previous-versions/windows/hardware/design/dn642101(v=vs.85)
> How do Cypress devices work in Windows? Might they have a compatible
> firmware?

I do not know how it works on Windows, actually.
The datasheet indicates that it is based on HID specification. I guess
it is not "HID compliant" as I was thinking while reading it.

"Packet Interface Protocol (PIP) is a command and response-based
communication protocol used to communicate with the
TrueTouch device over the physical communication interface. PIP is
modeled after Microsoftâs HID over I2C protocol specification, version
1.00. However, PIP extends the functionality of HID over I2C protocol
to support both I2C and SPI physical communication interfaces, raw
data extraction, self-tests, bootloading, and configuration data
programming."

> 
> In any case, for all I2C HID things Benjamin (CCed) is your guy.

Okay, thanks.
I am not so sure it is possible to use HID's drivers, now.

Best regards,

-- 
MylÃne Josserand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com