Date:   Mon, 20 Sep 2021 21:35:30 -0700
From:   Dmitry Torokhov <dmitry.torokhov@...il.com>
To:     Ping Cheng <pinglinux@...il.com>
Cc:     Alistair Francis <alistair@...stair23.me>,
        "Tobita, Tatsunosuke" <tatsunosuke.tobita@...om.com>,
        linux-input <linux-input@...r.kernel.org>, linux-imx@....com,
        kernel@...gutronix.de, Tatsunosuke Tobita <junkpainting@...il.com>,
        linux-kernel@...r.kernel.org, alistair23@...il.com,
        robh+dt@...nel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v10 05/12] Input: wacom_i2c - Read the descriptor values

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@...il.com> 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.

Thanks.

-- 
Dmitry

Powered by blists - more mailing lists