| Message ID | 1480555288-142791-1-git-send-email-briannorris@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Nov 30 2016 or thereabouts, Brian Norris wrote: > From: Caesar Wang <wxt@rock-chips.com> > > 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@rock-chips.com> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Jiri Kosina <jikos@kernel.org> > Cc: linux-input@vger.kernel.org > Signed-off-by: Brian Norris <briannorris@chromium.org> > --- > 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. > - reg: i2c slave address > - hid-descr-addr: HID descriptor address > - interrupt-parent: the phandle for the interrupt controller > - interrupts: interrupt line > > +Optional properties: > +- vdd-supply: phandle of the regulator that provides the supply voltage. Agree on this one however. Cheers, Benjamin > + > Example: > > i2c-hid-dev@2c { > -- > 2.8.0.rc3.226.g39d4020 > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@rock-chips.com> > > > > 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@rock-chips.com> > > Cc: Rob Herring <robh+dt@kernel.org> > > Cc: Jiri Kosina <jikos@kernel.org> > > Cc: linux-input@vger.kernel.org > > Signed-off-by: Brian Norris <briannorris@chromium.org> > > --- > > 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. > > - reg: i2c slave address > > - hid-descr-addr: HID descriptor address > > - interrupt-parent: the phandle for the interrupt controller > > - interrupts: interrupt line > > > > +Optional properties: > > +- vdd-supply: phandle of the regulator that provides the supply voltage. > > Agree on this one however. Thanks. As Benjamin noticed on patch 2, I added a delay property; I realized I had been hacking that delay in to the regulator framework as a "ramp delay" property, when in fact it was actually a property of *this* device -- the 100 ms wait is a suggested wait for the HID firmware to boot, not for the regulator to stabilize. So, what do you two think about the following two properties? - vdd-supply, as in the quoted patch - init-delay-ms: time required by the device after power-on before it is ready for communication And I'd drop the extra compatible property. Brian -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 30, 2016 at 05:21:27PM -0800, Brian Norris wrote: > From: Caesar Wang <wxt@rock-chips.com> > > 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@rock-chips.com> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Jiri Kosina <jikos@kernel.org> > Cc: linux-input@vger.kernel.org > Signed-off-by: Brian Norris <briannorris@chromium.org> > --- > 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(-) Acked-by: Rob Herring <robh@kernel.org> -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Rob, On Mon, Dec 05, 2016 at 05:42:48PM -0600, Rob Herring wrote: > On Wed, Nov 30, 2016 at 05:21:27PM -0800, Brian Norris wrote: > > From: Caesar Wang <wxt@rock-chips.com> > > > > 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@rock-chips.com> > > Cc: Rob Herring <robh+dt@kernel.org> > > Cc: Jiri Kosina <jikos@kernel.org> > > Cc: linux-input@vger.kernel.org > > Signed-off-by: Brian Norris <briannorris@chromium.org> > > --- > > 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(-) > > Acked-by: Rob Herring <robh@kernel.org> Thanks but (unfortunately) there've been 2 new versions since then. Specifically, Benjamin NACK'ed this patch and requested we NOT include device/manufacturer-specific compatible properties here. In fact, the binding is still rather generic and IMO (and in Benjamin's opinion) doesn't really need to be restricted to a specific device. Please consider reviewing Benjamin's requests and my recent changes. Benjamin has one small remaining comment on v4, and I plan to send the 5th (and final?) version once I'm confident you and Benjamin agree :) Thanks, Brian -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@rock-chips.com> > > > > > > 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@rock-chips.com> > > > Cc: Rob Herring <robh+dt@kernel.org> > > > Cc: Jiri Kosina <jikos@kernel.org> > > > Cc: linux-input@vger.kernel.org > > > Signed-off-by: Brian Norris <briannorris@chromium.org> > > > --- > > > 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. 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]. Now if you want to make 'hid-over-i2c' a fallback to 'wacom,w9013', I'm fine with that. Rob [1] https://sietch-tagr.blogspot.de/2016/04/display-panels-are-not-special.html -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 05, 2016 at 05:59:08PM -0600, 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@rock-chips.com> > > > > > > > > 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@rock-chips.com> > > > > Cc: Rob Herring <robh+dt@kernel.org> > > > > Cc: Jiri Kosina <jikos@kernel.org> > > > > Cc: linux-input@vger.kernel.org > > > > Signed-off-by: Brian Norris <briannorris@chromium.org> > > > > --- > > > > 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. > > 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]. > > Now if you want to make 'hid-over-i2c' a fallback to 'wacom,w9013', I'm > fine with that. So if I understand it correctly the only change is to have DTS specify compatible = "wacom,w9013", "hid-over-i2c"; and no actual changes to the driver itself with regard to the new compatible string, correct? I wonder what, besides breaking module autoload, this really buys us? Thanks.
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@rock-chips.com> > > > > > > > > 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@rock-chips.com> > > > > Cc: Rob Herring <robh+dt@kernel.org> > > > > Cc: Jiri Kosina <jikos@kernel.org> > > > > Cc: linux-input@vger.kernel.org > > > > Signed-off-by: Brian Norris <briannorris@chromium.org> > > > > --- > > > > 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. I have dealt with that for the wacom modules for years, and this is definitively not a good solution. And one additional caveat of this solution is the time between the release of the new device and its readiness in the hands of the consumer. You need to push a patch upstream, then backport it or wait for it to come to your distribution. While if there is a device tree specific quirk, you just read the spec of the device and applies it to your device tree and you are good to go. So no, I don't buy this. If hardware makers want to have fancy way of initializing their devices, we can cope with those, but I don't want to do the Device Tree job in a kernel module were you need to recompile it each time a new device appears. > > Now if you want to make 'hid-over-i2c' a fallback to 'wacom,w9013', I'm > fine with that. I agree to have some sort of quirks in the i2c-hid module, but definitively not a list of devices with a specific initialization sequence. Device Tree has also been introduced to remove the specific platform devices, and you are basically asking us to go back there, which I don't want. Cheers, Benjamin > > Rob > > [1] https://sietch-tagr.blogspot.de/2016/04/display-panels-are-not-special.html -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 6, 2016 at 2:48 AM, Benjamin Tissoires <benjamin.tissoires@redhat.com> 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@rock-chips.com> >> > > > >> > > > 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@rock-chips.com> >> > > > Cc: Rob Herring <robh+dt@kernel.org> >> > > > Cc: Jiri Kosina <jikos@kernel.org> >> > > > Cc: linux-input@vger.kernel.org >> > > > Signed-off-by: Brian Norris <briannorris@chromium.org> >> > > > --- >> > > > 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. > I have dealt with that for the wacom modules for years, and this is > definitively not a good solution. > > And one additional caveat of this solution is the time between the > release of the new device and its readiness in the hands of the > consumer. You need to push a patch upstream, then backport it or wait > for it to come to your distribution. While if there is a device tree > specific quirk, you just read the spec of the device and applies it to > your device tree and you are good to go. > > So no, I don't buy this. If hardware makers want to have fancy way of > initializing their devices, we can cope with those, but I don't want to > do the Device Tree job in a kernel module were you need to recompile it > each time a new device appears. > >> >> Now if you want to make 'hid-over-i2c' a fallback to 'wacom,w9013', I'm >> fine with that. > > I agree to have some sort of quirks in the i2c-hid module, but > definitively not a list of devices with a specific initialization > sequence. Device Tree has also been introduced to remove the specific > platform devices, and you are basically asking us to go back there, > which I don't want. That is not correct. DT does not abstract the h/w to hide implementation details like ACPI does with AML code. We still have specific platform drivers and always will. DT is about removing the board files and describing connections between devices. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Tue, Dec 6, 2016 at 6:56 AM, Rob Herring <robh@kernel.org> wrote: > On Tue, Dec 6, 2016 at 2:48 AM, Benjamin Tissoires > <benjamin.tissoires@redhat.com> 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@rock-chips.com> >>> > > > >>> > > > 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@rock-chips.com> >>> > > > Cc: Rob Herring <robh+dt@kernel.org> >>> > > > Cc: Jiri Kosina <jikos@kernel.org> >>> > > > Cc: linux-input@vger.kernel.org >>> > > > Signed-off-by: Brian Norris <briannorris@chromium.org> >>> > > > --- >>> > > > 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. -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Dec 06 2016 or thereabouts, Doug Anderson wrote: > Hi, > > On Tue, Dec 6, 2016 at 6:56 AM, Rob Herring <robh@kernel.org> wrote: > > On Tue, Dec 6, 2016 at 2:48 AM, Benjamin Tissoires > > <benjamin.tissoires@redhat.com> 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@rock-chips.com> > >>> > > > > >>> > > > 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@rock-chips.com> > >>> > > > Cc: Rob Herring <robh+dt@kernel.org> > >>> > > > Cc: Jiri Kosina <jikos@kernel.org> > >>> > > > Cc: linux-input@vger.kernel.org > >>> > > > Signed-off-by: Brian Norris <briannorris@chromium.org> > >>> > > > --- > >>> > > > 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? Cheers, Benjamin -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 6, 2016 at 10:18 AM, Doug Anderson <dianders@chromium.org> wrote: > Hi, > > On Tue, Dec 6, 2016 at 6:56 AM, Rob Herring <robh@kernel.org> wrote: >> On Tue, Dec 6, 2016 at 2:48 AM, Benjamin Tissoires >> <benjamin.tissoires@redhat.com> 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@rock-chips.com> >>>> > > > >>>> > > > 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@rock-chips.com> >>>> > > > Cc: Rob Herring <robh+dt@kernel.org> >>>> > > > Cc: Jiri Kosina <jikos@kernel.org> >>>> > > > Cc: linux-input@vger.kernel.org >>>> > > > Signed-off-by: Brian Norris <briannorris@chromium.org> >>>> > > > --- >>>> > > > 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 linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 8, 2016 at 9:41 AM, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > On Dec 06 2016 or thereabouts, Doug Anderson wrote: >> Hi, >> >> On Tue, Dec 6, 2016 at 6:56 AM, Rob Herring <robh@kernel.org> wrote: >> > On Tue, Dec 6, 2016 at 2:48 AM, Benjamin Tissoires >> > <benjamin.tissoires@redhat.com> 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@rock-chips.com> >> >>> > > > >> >>> > > > 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@rock-chips.com> >> >>> > > > Cc: Rob Herring <robh+dt@kernel.org> >> >>> > > > Cc: Jiri Kosina <jikos@kernel.org> >> >>> > > > Cc: linux-input@vger.kernel.org >> >>> > > > Signed-off-by: Brian Norris <briannorris@chromium.org> >> >>> > > > --- >> >>> > > > 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. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On December 8, 2016 8:03:06 AM PST, Rob Herring <robh@kernel.org> wrote: >On Thu, Dec 8, 2016 at 9:41 AM, Benjamin Tissoires ><benjamin.tissoires@redhat.com> wrote: >> On Dec 06 2016 or thereabouts, Doug Anderson wrote: >>> Hi, >>> >>> On Tue, Dec 6, 2016 at 6:56 AM, Rob Herring <robh@kernel.org> wrote: >>> > On Tue, Dec 6, 2016 at 2:48 AM, Benjamin Tissoires >>> > <benjamin.tissoires@redhat.com> 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@rock-chips.com> >>> >>> > > > >>> >>> > > > 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@rock-chips.com> >>> >>> > > > Cc: Rob Herring <robh+dt@kernel.org> >>> >>> > > > Cc: Jiri Kosina <jikos@kernel.org> >>> >>> > > > Cc: linux-input@vger.kernel.org >>> >>> > > > Signed-off-by: Brian Norris <briannorris@chromium.org> >>> >>> > > > --- >>> >>> > > > 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.
On Thu, Dec 8, 2016 at 10:13 AM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On December 8, 2016 8:03:06 AM PST, Rob Herring <robh@kernel.org> wrote: >>On Thu, Dec 8, 2016 at 9:41 AM, Benjamin Tissoires >><benjamin.tissoires@redhat.com> wrote: >>> On Dec 06 2016 or thereabouts, Doug Anderson wrote: >>>> Hi, >>>> >>>> On Tue, Dec 6, 2016 at 6:56 AM, Rob Herring <robh@kernel.org> wrote: >>>> > On Tue, Dec 6, 2016 at 2:48 AM, Benjamin Tissoires >>>> > <benjamin.tissoires@redhat.com> 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@rock-chips.com> >>>> >>> > > > >>>> >>> > > > 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@rock-chips.com> >>>> >>> > > > Cc: Rob Herring <robh+dt@kernel.org> >>>> >>> > > > Cc: Jiri Kosina <jikos@kernel.org> >>>> >>> > > > Cc: linux-input@vger.kernel.org >>>> >>> > > > Signed-off-by: Brian Norris <briannorris@chromium.org> >>>> >>> > > > --- >>>> >>> > > > 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. Yes, I will. Anyone who's worked on drop-in or pin compatible parts knows there's no such thing. That in no way means the OS driver has to know about each and every one. If they can all claim compatibility with Wacom (including power control), then they can have a Wacom compatible string too. Or you can just never tell me that there's a different manufacturer and I won't care as long you don't need different control. But soon as a device needs another power rail, GPIO or different timing, then you'd better have a new compatible string. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 08, 2016 at 10:26:41AM -0600, Rob Herring wrote: > On Thu, Dec 8, 2016 at 10:13 AM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > On December 8, 2016 8:03:06 AM PST, Rob Herring <robh@kernel.org> wrote: > >>On Thu, Dec 8, 2016 at 9:41 AM, Benjamin Tissoires > >><benjamin.tissoires@redhat.com> wrote: > >>> On Dec 06 2016 or thereabouts, Doug Anderson wrote: > >>>> Hi, > >>>> > >>>> On Tue, Dec 6, 2016 at 6:56 AM, Rob Herring <robh@kernel.org> wrote: > >>>> > On Tue, Dec 6, 2016 at 2:48 AM, Benjamin Tissoires > >>>> > <benjamin.tissoires@redhat.com> 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@rock-chips.com> > >>>> >>> > > > > >>>> >>> > > > 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@rock-chips.com> > >>>> >>> > > > Cc: Rob Herring <robh+dt@kernel.org> > >>>> >>> > > > Cc: Jiri Kosina <jikos@kernel.org> > >>>> >>> > > > Cc: linux-input@vger.kernel.org > >>>> >>> > > > Signed-off-by: Brian Norris <briannorris@chromium.org> > >>>> >>> > > > --- > >>>> >>> > > > 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. > > Yes, I will. Anyone who's worked on drop-in or pin compatible parts > knows there's no such thing. And yet we are shipping quite a few of Chromebooks with touchpads that are dual-sourced and can be exchanged at any time without any changes to the software (be it kernel or firmware). > > That in no way means the OS driver has to know about each and every > one. If they can all claim compatibility with Wacom (including power > control), then they can have a Wacom compatible string too. Or you can > just never tell me that there's a different manufacturer and I won't > care as long you don't need different control. But soon as a device > needs another power rail, GPIO or different timing, then you'd better > have a new compatible string. That I simply do not understand. We routinely enhance bindings because the devices get used on different boards that expose more or less connections. I.e. quite often we start with a device whose rails are controlled by the firmware, and so the binding only contains register and interrupt data. Then we come across board that exposes reset GPIO and we add that to the binding (bit we do not invent new compatible string just because there is GPIO now). Then we get a board that actually wants kernel to control power to the chip and we add a regulator. Non-optional, mind you, because we rely on the regulator system to give us a dummy one if it is not described by the firmware/other means. And then we get another board that exposes another power rail (let's say 3.3V to the panel whereas the previous one did not use it). And we add another regulator binding. All this time we have the same compatibility string. So in this case we finally got to the point where we admit that devices speaking HID over I2C do have power rails; we simply did not need to control them before. The same Wacom digitizer, that you now demand to add a compatible for, may have been used in other boards where power rails were either turned on by the firmware at boot time and left on until the board is shutdown, or ACPI was controlling them (via _ON/_OFF methods), or there was some other magic. Having a supply and ability to control the time it takes to bring the device into operating state is in no way Wacom-specific, so why new compatibility instead of enhancing the current binding? And on top of that, currently multiple compatible strings are utterly broken with regard to module loading (you only emit modalias for the first component), so having DTS with multiples does not work well in real life.
On Thu, 8 Dec 2016, Rob Herring wrote: > > 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. > > Yes, I will. What advantage does that bring? > That in no way means the OS driver has to know about each and every one. > If they can all claim compatibility with Wacom (including power > control), then they can have a Wacom compatible string too. Or you can > just never tell me that there's a different manufacturer and I won't > care as long you don't need different control. But soon as a device > needs another power rail, GPIO or different timing, then you'd better > have a new compatible string. Again, I simply don't understand what advantage does the aproach you are trying to use bring. HID over I2C is a generic protocol. Sure, we need to have quirks for device-specific bugs, and in such cases enumerate particular devices. But we don't need DT for that at all.
On Thu, Dec 8, 2016 at 12:12 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Thu, Dec 08, 2016 at 10:26:41AM -0600, Rob Herring wrote: >> On Thu, Dec 8, 2016 at 10:13 AM, Dmitry Torokhov >> <dmitry.torokhov@gmail.com> wrote: >> > On December 8, 2016 8:03:06 AM PST, Rob Herring <robh@kernel.org> wrote: >> >>On Thu, Dec 8, 2016 at 9:41 AM, Benjamin Tissoires >> >><benjamin.tissoires@redhat.com> wrote: >> >>> On Dec 06 2016 or thereabouts, Doug Anderson wrote: [...] >> >>>> 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. >> >> Yes, I will. Anyone who's worked on drop-in or pin compatible parts >> knows there's no such thing. > > And yet we are shipping quite a few of Chromebooks with touchpads that > are dual-sourced and can be exchanged at any time without any changes to > the software (be it kernel or firmware). > >> >> That in no way means the OS driver has to know about each and every >> one. If they can all claim compatibility with Wacom (including power >> control), then they can have a Wacom compatible string too. Or you can >> just never tell me that there's a different manufacturer and I won't >> care as long you don't need different control. But soon as a device >> needs another power rail, GPIO or different timing, then you'd better >> have a new compatible string. > > That I simply do not understand. We routinely enhance bindings because > the devices get used on different boards that expose more or less > connections. I.e. quite often we start with a device whose rails are > controlled by the firmware, and so the binding only contains register > and interrupt data. Then we come across board that exposes reset GPIO > and we add that to the binding (bit we do not invent new compatible > string just because there is GPIO now). Then we get a board that > actually wants kernel to control power to the chip and we add a > regulator. Non-optional, mind you, because we rely on the regulator > system to give us a dummy one if it is not described by the > firmware/other means. And then we get another board that exposes another > power rail (let's say 3.3V to the panel whereas the previous one did not > use it). And we add another regulator binding. All this time we have > the same compatibility string. All this I agree with, but it almost always starts with a compatible that matches the device, not a compatible for the protocol. When adding control for different devices diverge, then they need different compatibles. Ideally, we'd start with all the supplies and control lines defined for a binding, so we don't have so much evolving of bindings. But obviously we can't always get things 100% complete to start with, so we have to let them evolve. And I can't go read every datasheet, but I do go read them sometimes for reviews. > So in this case we finally got to the point where we admit that devices > speaking HID over I2C do have power rails; we simply did not need to > control them before. The same Wacom digitizer, that you now demand to > add a compatible for, may have been used in other boards where power > rails were either turned on by the firmware at boot time and left on > until the board is shutdown, or ACPI was controlling them (via _ON/_OFF > methods), or there was some other magic. Having a supply and ability to > control the time it takes to bring the device into operating state is in > no way Wacom-specific, so why new compatibility instead of enhancing > the current binding? A new compatible is enhancing the binding IMO. I'm just saying you need both the compatible and the added properties. Whether the power on delay should be a property or implied by the compatible is somewhat debatable, but I'm okay with having it because we already do on other bindings. What I'm not okay with is "simple" or "generic" bindings that continually evolve with additional properties to handle more and more complex cases. DT is not a scripting language. This has been discussed countless times before... > And on top of that, currently multiple compatible strings are utterly > broken with regard to module loading (you only emit modalias for the > first component), so having DTS with multiples does not work well in > real life. Sounds like this should be a fixable problem. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Dec 9, 2016 at 8:36 AM, Jiri Kosina <jikos@kernel.org> wrote: > On Thu, 8 Dec 2016, Rob Herring wrote: > >> > 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. >> >> Yes, I will. > > What advantage does that bring? > >> That in no way means the OS driver has to know about each and every one. >> If they can all claim compatibility with Wacom (including power >> control), then they can have a Wacom compatible string too. Or you can >> just never tell me that there's a different manufacturer and I won't >> care as long you don't need different control. But soon as a device >> needs another power rail, GPIO or different timing, then you'd better >> have a new compatible string. > > Again, I simply don't understand what advantage does the aproach you are > trying to use bring. This is simply how DT works. HID-over-I2C devices are no different than any other I2C device or any other component. You are not special. > HID over I2C is a generic protocol. DT describes h/w, not protocols. > Sure, we need to have quirks for > device-specific bugs, and in such cases enumerate particular devices. But > we don't need DT for that at all. When it is related to powering on the device you may need to know the specific device in DT. Compatibles are like VID/PID for devices, a unique identifier for the specific device. Having that does not to prevent generic/common drivers. The same rules apply. You wouldn't want different devices having the same VID/PID (surely that has never happened) or only change a VID/PID when you find a bug. The same rules apply for compatible strings. You should be able to apply quirks without changing/adding compatible strings (or more generally, without changing the dtb). Just like you wouldn't change a VID/PID for a device only when you find a bug, you can't add/change a DT compatible. BTW, As you do have a VID/PID, you could define a compatible string syntax using VID/PID like is done for PCI and USB. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Thu, Dec 8, 2016 at 8:01 AM, Rob Herring <robh@kernel.org> wrote: >> 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. We have never specified the device on boards I've worked with. On rk3288-veyron, for instance, we might have stuffed a Broadcom 4354 WiFi chip or a Marvell 8897 WiFi chip. Some veyron boards have one chip and some have the other. ...and during bringup we even had some of the exact same boards where half were stuffed with one chip and half with the other. Nothing in the device tree says which chip is stuffed. In both cases the board uses the same power on sequence for the WiFi chip. Once that is done, we dynamically probe which actual WiFi part is stuffed. Certainly not all users that have these WiFi chips use the same power on sequence. I have certainly seen development boards for these chips where you just insert them into a regular SD card slot. This is a more expensive solution because you need more logic on the board, but it shows that the power on sequence is not associated with these chips. >> 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. It's not always that simple. The device says that it needs power and resets to happen. How power is provided and how resets happen is awfully board specific. As per above it is possible that the board wouldn't need to be involved above is you want to spend more money / power. -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Fri, Dec 9, 2016 at 7:01 AM, Rob Herring <robh@kernel.org> wrote: > On Fri, Dec 9, 2016 at 8:36 AM, Jiri Kosina <jikos@kernel.org> wrote: >> On Thu, 8 Dec 2016, Rob Herring wrote: >> >>> > 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. >>> >>> Yes, I will. >> >> What advantage does that bring? >> >>> That in no way means the OS driver has to know about each and every one. >>> If they can all claim compatibility with Wacom (including power >>> control), then they can have a Wacom compatible string too. Or you can >>> just never tell me that there's a different manufacturer and I won't >>> care as long you don't need different control. But soon as a device >>> needs another power rail, GPIO or different timing, then you'd better >>> have a new compatible string. >> >> Again, I simply don't understand what advantage does the aproach you are >> trying to use bring. > > This is simply how DT works. HID-over-I2C devices are no different > than any other I2C device or any other component. You are not special. ...but once you say that it's HID over I2C then it becomes probe-able, right? Said another way: we need to specify just enough to power the device up properly and then we can ask it what kind of device it is and then we can make quirk decisions based on that. So specifying what kind of device it is in the device tree is somewhat redundant and it also means that you make it needlessly difficult to build a system with dual-sourced components. One of the major points of probe-able connections is that you could put anything you want there and the kernel _doesn't_ need to describe it. ...and the whole point of device tree (I thought) was to specifically handle connections that _aren't_ probe-able. For instance, if a board has a USB bus on it but you need to assert a special reset or turn on a special regulator (besides vbus) before you can probe the USB bus, you wouldn't think that the board should specify exactly what device was stuffed in the connection, would you? >> HID over I2C is a generic protocol. > > DT describes h/w, not protocols. Isn't it a little of each, though? When you say that there's a USB port or an SDIO port or a serial port on the board, you're saying more than just that there are 2, 4, or 8 wires coming out of the board. You're saying that you're expecting to talk a certain protocol over those wires. -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Dec 9, 2016 at 10:05 AM, Doug Anderson <dianders@chromium.org> wrote: > Hi, > > On Thu, Dec 8, 2016 at 8:01 AM, Rob Herring <robh@kernel.org> wrote: >>> 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. > > We have never specified the device on boards I've worked with. > > On rk3288-veyron, for instance, we might have stuffed a Broadcom 4354 > WiFi chip or a Marvell 8897 WiFi chip. Some veyron boards have one > chip and some have the other. ...and during bringup we even had some > of the exact same boards where half were stuffed with one chip and > half with the other. > > Nothing in the device tree says which chip is stuffed. In both cases > the board uses the same power on sequence for the WiFi chip. Once > that is done, we dynamically probe which actual WiFi part is stuffed. That's good and I'm happy when that works, but it doesn't work in the general case. I'm not saying you can't do exactly the same thing here. All I'm asking for is add the properties to the binding AND a compatible. The kernel can ignore the added compatible. The key point is if you have additional properties outside of what it means to be HID-over-I2C, then you must have a compatible for that device. Whether you enforce that in the driver, I don't give a shit. I do care how it is documented though and will care when or if we start validating bindings (I don't think that's the kernel's job). This is not just a problem with probe-able buses. This dual sourcing happens with non-probe-able devices too. Look at Hans' series for Allwinner tablet touchscreens. > Certainly not all users that have these WiFi chips use the same power > on sequence. I have certainly seen development boards for these chips > where you just insert them into a regular SD card slot. This is a > more expensive solution because you need more logic on the board, but > it shows that the power on sequence is not associated with these > chips. If SD slots were the primary target for SDIO cards, we'd be in much better shape without all the misc side band signals. Many devices I don't think could be made to work as a card. >>> 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. > > It's not always that simple. The device says that it needs power and > resets to happen. How power is provided and how resets happen is > awfully board specific. As per above it is possible that the board > wouldn't need to be involved above is you want to spend more money / > power. We have ways to deal with board specifics. If you want something completely generic to handle any possible power sequence of any board and any device, then propose something that does that. That's not what we have here with 2 properties. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Given the timing (merge window being open) and given then NACK given by Rob, I've now unapplied the patches (the for-4.10/i2c-hid branch is now obsolete, and has been superseded by for-4.10/i2c-hid-nopower). However, this is mostly done in order to provide more time for discussion; I still disagree with the reasoning behind the NACK.
On Dec 12 2016 or thereabouts, Jiri Kosina wrote: > Given the timing (merge window being open) and given then NACK given by > Rob, I've now unapplied the patches (the for-4.10/i2c-hid branch is now > obsolete, and has been superseded by for-4.10/i2c-hid-nopower). > > However, this is mostly done in order to provide more time for discussion; > I still disagree with the reasoning behind the NACK. > To hopefully make things going forward a little bit, I was wondering over the week-end if we should not solve this particular issue by adding an intermediate platform DT node: instead of having: --- i2c-hid-dev@2c { compatible = "hid-over-i2c"; reg = <0x2c>; hid-descr-addr = <0x0020>; interrupt-parent = <&gpx3>; interrupts = <3 2>; vdd-supply = <sth>; init-delay-ms = <100>; }; --- we would have: --- platform-i2c-hid@01 { compatible = "very-special-board-that-needs-firmware-quirks-and-delay-of-100ms"; vdd-supply = <sth>; i2c-hid-dev@2c { compatible = "hid-over-i2c"; reg = <0x2c>; hid-descr-addr = <0x0020>; interrupt-parent = <&gpx3>; interrupts = <3 2>; }; }; --- If I am not wrong, the platform device should be initialized before i2c-hid get called, which allows to setup properly the vdd supply. On resume/suspend, the tree should be respected and we should be able to enable/disable power in the same fashion this patch provides. We could then extend this platform device at will without tinkering in i2c-hid and we could also handle the GPIOs, reset or whatever is required in the future through compatibles. Thoughts? yes? no? bullshit? Cheers, Benjamin -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 12, 2016 at 4:01 AM, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > On Dec 12 2016 or thereabouts, Jiri Kosina wrote: >> Given the timing (merge window being open) and given then NACK given by >> Rob, I've now unapplied the patches (the for-4.10/i2c-hid branch is now >> obsolete, and has been superseded by for-4.10/i2c-hid-nopower). >> >> However, this is mostly done in order to provide more time for discussion; >> I still disagree with the reasoning behind the NACK. >> > > To hopefully make things going forward a little bit, I was wondering > over the week-end if we should not solve this particular issue by adding > an intermediate platform DT node: > > instead of having: > --- > i2c-hid-dev@2c { > compatible = "hid-over-i2c"; > reg = <0x2c>; > hid-descr-addr = <0x0020>; > interrupt-parent = <&gpx3>; > interrupts = <3 2>; > vdd-supply = <sth>; > init-delay-ms = <100>; > }; > --- > > we would have: > --- > platform-i2c-hid@01 { > compatible = "very-special-board-that-needs-firmware-quirks-and-delay-of-100ms"; > vdd-supply = <sth>; > i2c-hid-dev@2c { > compatible = "hid-over-i2c"; > reg = <0x2c>; > hid-descr-addr = <0x0020>; > interrupt-parent = <&gpx3>; > interrupts = <3 2>; > }; > }; > --- > > If I am not wrong, the platform device should be initialized before > i2c-hid get called, which allows to setup properly the vdd supply. > On resume/suspend, the tree should be respected and we should be able to > enable/disable power in the same fashion this patch provides. > > We could then extend this platform device at will without tinkering in > i2c-hid and we could also handle the GPIOs, reset or whatever is > required in the future through compatibles. > > Thoughts? yes? no? bullshit? That is structuring the DT match your driver structure, not what the h/w looks like. This would make sense if the device was multi-function of which HID-over-I2C was one function. You could do what you describe with a single node and the platform driver creates the hid-over-i2c device. DT is not the only way to create devices. Anyway, we're only debating this: i2c-hid-dev@2c { compatible = "wacom,w9013", "hid-over-i2c"; reg = <0x2c>; hid-descr-addr = <0x0020>; interrupt-parent = <&gpx3>; interrupts = <3 2>; vdd-supply = <sth>; init-delay-ms = <100>; }; vs. i2c-hid-dev@2c { compatible = "hid-over-i2c"; reg = <0x2c>; hid-descr-addr = <0x0020>; interrupt-parent = <&gpx3>; interrupts = <3 2>; vdd-supply = <sth>; init-delay-ms = <100>; }; My only other nit is use "post-power-on-delay-ms" which is already a defined property name rather than "init-delay-ms". Rob -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi all, On Mon, Dec 12, 2016 at 08:47:06AM -0600, Rob Herring wrote: [Snip Benjamin's proposal; I agree we don't really want a multi-level DT layout purely for making the driver look a little nicer (I'm not sure this would really be nicer anyway). And I think, as Rob notes here, our disagreement is smaller than appears. But I might be wrong.] > Anyway, we're only debating this: OK, so I think we might have a consensus of sorts? I'll describe it here, in case I'm wrong. Otherwise, I'll send another rev. > i2c-hid-dev@2c { > compatible = "wacom,w9013", "hid-over-i2c"; I plan to document the above, but not treat "wacom,w9013" specially in the driver, apart from possibly listing it in the driver of_match_table. This was mentioned by Dmitry earlier, and I didn't see any objection. (Note that there are problems with module autoload when using multiple compatible strings like above. May not be supremely relevant to the documentation, but it *is* practically important.) > reg = <0x2c>; > hid-descr-addr = <0x0020>; > interrupt-parent = <&gpx3>; > interrupts = <3 2>; > vdd-supply = <sth>; Document and support 'vdd-supply', optionally. > init-delay-ms = <100>; Per Rob's mention below, support this as 'post-power-on-delay-ms', optionally. We can use either of these properties on any device, with the intention that if there are future needs for divergent bindings, the aforementioned compatible property can help us differentiate. > }; > > vs. > > i2c-hid-dev@2c { > compatible = "hid-over-i2c"; > reg = <0x2c>; > hid-descr-addr = <0x0020>; > interrupt-parent = <&gpx3>; > interrupts = <3 2>; > vdd-supply = <sth>; > init-delay-ms = <100>; > }; > > My only other nit is use "post-power-on-delay-ms" which is already a > defined property name rather than "init-delay-ms". Any objections? Speak now or forever [1] hold your peace. Brian [1] Who am I kidding? There's always room for more paint on the bikeshed. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 12, 2016 at 12:34 PM, Brian Norris <briannorris@chromium.org> wrote: > Hi all, > > On Mon, Dec 12, 2016 at 08:47:06AM -0600, Rob Herring wrote: > > [Snip Benjamin's proposal; I agree we don't really want a multi-level DT > layout purely for making the driver look a little nicer (I'm not sure > this would really be nicer anyway). And I think, as Rob notes here, our > disagreement is smaller than appears. But I might be wrong.] > >> Anyway, we're only debating this: > > OK, so I think we might have a consensus of sorts? I'll describe it > here, in case I'm wrong. Otherwise, I'll send another rev. > >> i2c-hid-dev@2c { >> compatible = "wacom,w9013", "hid-over-i2c"; > > I plan to document the above, but not treat "wacom,w9013" specially in > the driver, apart from possibly listing it in the driver of_match_table. > This was mentioned by Dmitry earlier, and I didn't see any objection. > > (Note that there are problems with module autoload when using multiple > compatible strings like above. May not be supremely relevant to the > documentation, but it *is* practically important.) I'm not sure what is not working here exactly. We emit all the compatible strings in the uevent. However, it looks like this is only called for platform devices. In the case of i2c, I don't think any compatible string is emitted. It looks to me like i2c_device_uevent just needs a call to of_device_get_modalias to fix this. There's some issues in the I2C core in how it does matching and maybe this is related? I would guess if it was that easy, then it would be fixed already. Maybe not. >> reg = <0x2c>; >> hid-descr-addr = <0x0020>; >> interrupt-parent = <&gpx3>; >> interrupts = <3 2>; >> vdd-supply = <sth>; > > Document and support 'vdd-supply', optionally. > >> init-delay-ms = <100>; > > Per Rob's mention below, support this as 'post-power-on-delay-ms', > optionally. > > We can use either of these properties on any device, with the > intention that if there are future needs for divergent bindings, the > aforementioned compatible property can help us differentiate. TBC, from a DT perspective (and what the binding should say), is the properties are only valid with a wacom compatible present (or any others you want to add). If the driver doesn't enforce that and supports having those properties with just "hid-over-i2c", then downstream dts's can use that for all I care. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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" - reg: i2c slave address - hid-descr-addr: HID descriptor address - interrupt-parent: the phandle for the interrupt controller - interrupts: interrupt line +Optional properties: +- vdd-supply: phandle of the regulator that provides the supply voltage. + Example: i2c-hid-dev@2c {