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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
