Re: [PATCH v2 1/2] devicetree: i2c-hid: Add Wacom digitizer + regulator support

[Rob Herring]

On Tue, Dec 6, 2016 at 10:18 AM, Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
> Hi,
>
> On Tue, Dec 6, 2016 at 6:56 AM, Rob Herring <robh@xxxxxxxxxx> wrote:
>> On Tue, Dec 6, 2016 at 2:48 AM, Benjamin Tissoires
>> <benjamin.tissoires@xxxxxxxxxx> wrote:
>>> On Dec 05 2016 or thereabouts, Rob Herring wrote:
>>>> On Thu, Dec 01, 2016 at 09:24:50AM -0800, Brian Norris wrote:
>>>> > Hi Benjamin and Rob,
>>>> >
>>>> > On Thu, Dec 01, 2016 at 03:34:34PM +0100, Benjamin Tissoires wrote:
>>>> > > On Nov 30 2016 or thereabouts, Brian Norris wrote:
>>>> > > > From: Caesar Wang <wxt@xxxxxxxxxxxxxx>
>>>> > > >
>>>> > > > Add a compatible string and regulator property for Wacom W9103
>>>> > > > digitizer. Its VDD supply may need to be enabled before using it.
>>>> > > >
>>>> > > > Signed-off-by: Caesar Wang <wxt@xxxxxxxxxxxxxx>
>>>> > > > Cc: Rob Herring <robh+dt@xxxxxxxxxx>
>>>> > > > Cc: Jiri Kosina <jikos@xxxxxxxxxx>
>>>> > > > Cc: linux-input@xxxxxxxxxxxxxxx
>>>> > > > Signed-off-by: Brian Norris <briannorris@xxxxxxxxxxxx>
>>>> > > > ---
>>>> > > > v1 was a few months back. I finally got around to rewriting it based on
>>>> > > > DT binding feedback.
>>>> > > >
>>>> > > > v2:
>>>> > > >  * add compatible property for wacom
>>>> > > >  * name the regulator property specifically (VDD)
>>>> > > >
>>>> > > >  Documentation/devicetree/bindings/input/hid-over-i2c.txt | 6 +++++-
>>>> > > >  1 file changed, 5 insertions(+), 1 deletion(-)
>>>> > > >
>>>> > > > diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.txt b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
>>>> > > > index 488edcb264c4..eb98054e60c9 100644
>>>> > > > --- a/Documentation/devicetree/bindings/input/hid-over-i2c.txt
>>>> > > > +++ b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
>>>> > > > @@ -11,12 +11,16 @@ If this binding is used, the kernel module i2c-hid will handle the communication
>>>> > > >  with the device and the generic hid core layer will handle the protocol.
>>>> > > >
>>>> > > >  Required properties:
>>>> > > > -- compatible: must be "hid-over-i2c"
>>>> > > > +- compatible: must be "hid-over-i2c", or a device-specific string like:
>>>> > > > +    * "wacom,w9013"
>>>> > >
>>>> > > NACK on this one.
>>>> > >
>>>> > > After re-reading the v1 submission I realized Rob asked for this change,
>>>> > > but I strongly disagree.
>>>> > >
>>>> > > HID over I2C is a generic protocol, in the same way HID over USB is. We
>>>> > > can not start adding device specifics here, this is opening the can of
>>>> > > worms. If the device is a HID one, nothing else should matter. The rest
>>>> > > (description of the device, name, etc...) is all provided by the
>>>> > > protocol.
>>>> >
>>>> > I should have spoken up when Rob made the suggestion, because I more or
>>>> > less agree with Benjamin here. I don't really see why this needs to have
>>>> > a specialized compatible string, as the property is still fairly
>>>> > generic, and the entire device handling is via a generic protocol. The
>>>> > fact that we manage its power via a regulator is not very
>>>> > device-specific.
>>>>
>>>> It doesn't matter that the protocol is generic. The device attached and
>>>> the implementation is not. Implementations have been known to have
>>>> bugs/quirks (generally speaking, not HID over I2C in particular). There
>>>> are also things outside the scope of what is 'hid-over-i2c' like what's
>>>> needed to power-on the device which this patch clearly show.
>>>
>>> Yes, there are bugs, quirks, even with HID. But the HID declares within
>>> the protocol the Vendor ID and the Product ID, which means once we pass
>>> the initial "device is ready" step and can do a single i2c write/read,
>>> we don't give a crap about device tree anymore.
>>>
>>> This is just about setting the device in shape so that it can answer a
>>> single write/read.
>>>
>>>>
>>>> This is no different than a panel attached via LVDS, eDP, etc., or
>>>> USB/PCIe device hard-wired on a board. They all use standard protocols
>>>> and all need additional data to describe them. Of course, adding a
>>>> single property for a delay would not be a big deal, but it's never
>>>> ending. Next you need multiple supplies, GPIO controls, mutiple
>>>> delays... This has been discussed to death already. As Thierry Reding
>>>> said, you're not special[1].
>>>
>>> I can somewhat understand what you mean. The official specification is
>>> for ACPI. And ACPI allows to calls various settings while querying the
>>> _STA method for instance. So in the ACPI world, we don't need to care
>>> about regulators or GPIOs because the OEM deals with this in its own
>>> blob.
>>>
>>> Now, coming back to our issue. We are not special, maybe, if he says so.
>>> But this really feels like a design choice between putting the burden on
>>> device tree and OEMs or in the module maintainers. And I'd rather have
>>> the OEM deal with their device than me having to update the module for
>>> each generations of hardware. Indeed, this looks like an "endless"
>>> amount of quirks, but I'd rather have this endless amount of quirks than
>>> having to maintain an endless amount of list of new devices that behaves
>>> the same way. We are talking here about "wacom,w9013", but then comes
>>> "wacom,w9014" and we need to upgrade the kernel.
>>
>> No. If the w9014 can claim compatibility with then w9013, then you
>> don't need a kernel change. The DT should list the w9014 AND w9013,
>> but the kernel only needs to know about the w9013. That is until there
>> is some difference which is why the DT should list w9014 to start
>> with.
>>
>> If you don't have any power control to deal with, then the kernel can
>> always just match on "hid-over-i2c" compatible.
>
> Just my $0.02.  Feel free to ignore.
>
> One thought is that I would say that the need to power on the device
> explicitly seems more like a board level difference and less like a
> difference associated with a particular digitizer.  Said another way,
> it seems likely there will be boards with a w9013 without explicit
> control of the regulator in software and it seems like there will be
> boards with other digitizers where suddenly a new board will come out
> that needs explicit control of the regulator.

Then either the regulator is optional or you don't say it is a w9013
for that board. But if you do need to initialize the device and
therefore know what type of device it is, then you need a compatible
for the device. It's when things really vary by board that we add DT
properties.

> In this particular case I feel like we can draw a lot of parallels to
> an SDIO bus.
>
> When you specify an SDIO bus you don't specify what kind of card will
> be present, you just say "I've got an SDIO bus" and then the specific
> device underneath is probed.  Here we've say "I've got an i2c
> connection intended for HID" and then you probe for the HID device
> that's on the connection.

No, the soldered down devices require all sorts of extra non-SDIO
connections and we do specify the device in those cases.

> Also for an SDIO bus, you've possibly got a regulators / GPIOs /
> resets that need to be controlled, but the specific details of these
> regulator / GPIOs / resets are specific to a given board and not
> necessarily a given SDIO device.

It's both. The device defines what is needed and the specs to control
them (active states of GPIOs, de/assertion times of resets, supply
voltages, etc.). The board only determines what the connections are
and if you can control them.

Rob