Re: [PATCH v10 05/12] Input: wacom_i2c - Read the descriptor values

[Alistair Francis]

On Tue, Sep 21, 2021 at 2:35 PM Dmitry Torokhov
<dmitry.torokhov@xxxxxxxxx> wrote:
>
> Hi Ping,
>
> On Fri, Sep 17, 2021 at 01:43:18PM -0700, Ping Cheng wrote:
> > Hi Dmitry,
> >
> > On Tue, Sep 7, 2021 at 10:55 PM Dmitry Torokhov
> > <dmitry.torokhov@xxxxxxxxx> wrote:
> > >
> > > > > >
> > > > > > Yes, our firmware supports HID over I2C.  However, some of our
> > > > > > customers often do not want to use HID to handle our hardware; even
> > > > > > they don't install the generic HID driver neither.  In such case, we
> > > > > > need to distinguish what generation of our device customer's has. And
> > > > > > to do so, we check I2C HID descriptor even though the driver is not
> > > > > > working with HID driver components, but this one.  That is why I2C HID
> > > > > > descriptor is used there. It is called, but the situation with this
> > > > > > driver is not supposed to work as a HID device.
> > > > >
> > > > > I would like to understand better why the customers do not want to use
> > > > > HID.
> > > >
> > > >
> > > > Those customers normally run embedded Linux. Their hardwares have very
> > > > specific use cases. They don't need to support any other HID devices except
> > > > the Wacom i2c device.
> > > >
> > > > >
> > > > There needs to be a _very_ strong reason to essentially duplicate
> > > > > HID layer in a vendor driver and I inclined to say that such customers
> > > >
> > > > would need to patch their kernels themselves.
> > > >
> > > > They most likely don't want to duplicate HID layer. They just don't need
> > > > most of the HID layer code.
> > >
> > > They just need touchscreen support. Plus stylus support. And maybe
> > > battery support. And maybe something else down the road... And they need
> > > to introduce DT and ACPI descriptors to be able to mould the behavior to
> > > platform needs. Which is pretty much the purpose of HID layer.
> >
> > I see your point.
> >
> > > > wacom_i2c simplifies their deployment and
> > > > testing process. Most of those customers are very small companies...
> > >
> > > And now please continue this train of thoughts and consider every touch
> > > vendor. Wacom is not unique. We have Elan, Cypress, Weida, Goodix, etc.
> > > etc. Vendor drivers were acceptable before we had I2C standard, but now
> > > it is much better for everyone to share the efforts and use HID instead
> > > of replicating it for every vendor.
> >
> > And I agree with you that we should share our efforts on the main tasks.
> >
> > However, with the same token of sharing efforts, I see the benefit of
> > merging this set of patches upstream. From the version number we can
> > tell the patchset has gone through at least 10 rounds of review and
> > update. Alistair has put a lot of effort to get this far (Thank you
> > Alistair for your time and effort!).
>
> I am sorry, but the fact that a developer spent a lot of time writing
> code can not be used as a justification for merging said code.
>
> > A few community developers have
> > also reviewed the patches. This set of patches thoroughly touched all
> > parts of the components that related to an input i2c driver, which is
> > much better than the original version. This patchset would be a great
> > starting point for vendors to create their out of tree drivers, when
> > necessary. It would also offer vendors a clear picture of what
> > components they need to change/update to make their i2c input device
> > work under kernel input subsystem.
>
> And they can do that by looking at the mailing list archive and they can
> also follow this discussion and see why what they are doing is quite
> wrong. I mean, why would they stop at dropping HID only? Why not bypass
> I2C system as well and poke at the I2C controller directly. Skip PCI as
> well?
>
> >
> > So, merging the patchset will benefit more people and preserve the
> > effort that went into the patchset so far. If you like, you can add a
> > comment in the patch mentioning that future effort should be directed
> > to the i2c-hid subsystem, etc...
>
> No, we should not merge the patch set that we agreed is wrong in its
> approach. Maybe if I (and other community reviewers) realized that the
> device was HID compliant we could stop this effort earlier, but
> unfortunately it did not happen, so effort was wasted, but this happens
> sometimes.

Should I prepare a patch to remove the wacom I2C driver entirely then?

Alistair

>
> Thanks.
>
> --
> Dmitry