On December 8, 2016 8:03:06 AM PST, Rob Herring <robh@xxxxxxxxxx> wrote: >On Thu, Dec 8, 2016 at 9:41 AM, Benjamin Tissoires ><benjamin.tissoires@xxxxxxxxxx> wrote: >> On Dec 06 2016 or thereabouts, Doug Anderson 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. >>> >>> 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. >>> >>> 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. >>> >> >> Thanks Doug for this. I had the feeling this wasn't right, but you >> actually managed to put the words on it. If it's a board problem (if >> you switch the wacom device with an other HID over I2C device and you >> still need the same regulator/timing parameters), then this should >> simply be mentioned on the patch. >> >> So Brian, could you please respin the series and remve the Wacom >> mentions and explain that it is required for the board itself? > >In advance, NAK. > >This is not how DT works. Either this binding needs a Wacom compatible >or don't use DT. > And if tomorrow there is Elan device that is drop-in compatible (same connector, etc) with Wacom i2c-hid, will you ask for Elan-specific binding? Atmel? Weida? They all need to be powered up ultimately. Thanks. -- Dmitry -- 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
