On Thu, Dec 8, 2016 at 10:13 AM, Dmitry Torokhov
[off-list ref] wrote:

On December 8, 2016 8:03:06 AM PST, Rob Herring [off-list ref] wrote:

quoted

On Thu, Dec 8, 2016 at 9:41 AM, Benjamin Tissoires
[off-list ref] wrote:

quoted

On Dec 06 2016 or thereabouts, Doug Anderson wrote:

quoted

Hi,

On Tue, Dec 6, 2016 at 6:56 AM, Rob Herring [off-list ref] wrote:

quoted

On Tue, Dec 6, 2016 at 2:48 AM, Benjamin Tissoires
[off-list ref] wrote:

quoted

On Dec 05 2016 or thereabouts, Rob Herring wrote:

quoted

On Thu, Dec 01, 2016 at 09:24:50AM -0800, Brian Norris wrote:

quoted

Hi Benjamin and Rob,

On Thu, Dec 01, 2016 at 03:34:34PM +0100, Benjamin Tissoires

wrote:

quoted

quoted

quoted

quoted

quoted

quoted

quoted

On Nov 30 2016 or thereabouts, Brian Norris wrote:

quoted

From: Caesar Wang <redacted>

Add a compatible string and regulator property for Wacom

W9103

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

digitizer. Its VDD supply may need to be enabled before

using it.

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

Signed-off-by: Caesar Wang <redacted>
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

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

DT binding feedback.

v2:
 * add compatible property for wacom
 * name the regulator property specifically (VDD)

 Documentation/devicetree/bindings/input/hid-over-i2c.txt

| 6 +++++-

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

 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

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

index 488edcb264c4..eb98054e60c9 100644
---

a/Documentation/devicetree/bindings/input/hid-over-i2c.txt

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

+++

b/Documentation/devicetree/bindings/input/hid-over-i2c.txt

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

@@ -11,12 +11,16 @@ If this binding is used, the kernel

module i2c-hid will handle the communication

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

 with the device and the generic hid core layer will

handle the protocol.

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

 Required properties:
-- compatible: must be "hid-over-i2c"
+- compatible: must be "hid-over-i2c", or a

device-specific string like:

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

+    * "wacom,w9013"

NACK on this one.

After re-reading the v1 submission I realized Rob asked for

this change,

quoted

quoted

quoted

quoted

quoted

quoted

quoted

but I strongly disagree.

HID over I2C is a generic protocol, in the same way HID over

USB is. We

quoted

quoted

quoted

quoted

quoted

quoted

quoted

can not start adding device specifics here, this is opening

the can of

quoted

quoted

quoted

quoted

quoted

quoted

quoted

worms. If the device is a HID one, nothing else should

matter. The rest

quoted

quoted

quoted

quoted

quoted

quoted

quoted

(description of the device, name, etc...) is all provided by

the

quoted

quoted

quoted

quoted

quoted

quoted

quoted

protocol.

I should have spoken up when Rob made the suggestion, because

I more or

quoted

quoted

quoted

quoted

quoted

quoted

less agree with Benjamin here. I don't really see why this

needs to have

quoted

quoted

quoted

quoted

quoted

quoted

a specialized compatible string, as the property is still

fairly

quoted

quoted

quoted

quoted

quoted

quoted

generic, and the entire device handling is via a generic

protocol. The

quoted

quoted

quoted

quoted

quoted

quoted

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

quoted

quoted

quoted

quoted

quoted

the implementation is not. Implementations have been known to

have

quoted

quoted

quoted

quoted

quoted

bugs/quirks (generally speaking, not HID over I2C in

particular). There

quoted

quoted

quoted

quoted

quoted

are also things outside the scope of what is 'hid-over-i2c' like

what's

quoted

quoted

quoted

quoted

quoted

needed to power-on the device which this patch clearly show.

Yes, there are bugs, quirks, even with HID. But the HID declares

within

quoted

quoted

quoted

quoted

the protocol the Vendor ID and the Product ID, which means once

we pass

quoted

quoted

quoted

quoted

the initial "device is ready" step and can do a single i2c

write/read,

quoted

quoted

quoted

quoted

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

quoted

quoted

quoted

quoted

single write/read.

quoted

This is no different than a panel attached via LVDS, eDP, etc.,

or

quoted

quoted

quoted

quoted

quoted

USB/PCIe device hard-wired on a board. They all use standard

protocols

quoted

quoted

quoted

quoted

quoted

and all need additional data to describe them. Of course, adding

a

quoted

quoted

quoted

quoted

quoted

single property for a delay would not be a big deal, but it's

never

quoted

quoted

quoted

quoted

quoted

ending. Next you need multiple supplies, GPIO controls, mutiple
delays... This has been discussed to death already. As Thierry

Reding

quoted

quoted

quoted

quoted

quoted

said, you're not special[1].

I can somewhat understand what you mean. The official

specification is

quoted

quoted

quoted

quoted

for ACPI. And ACPI allows to calls various settings while

querying the

quoted

quoted

quoted

quoted

_STA method for instance. So in the ACPI world, we don't need to

care

quoted

quoted

quoted

quoted

about regulators or GPIOs because the OEM deals with this in its

own

quoted

quoted

quoted

quoted

blob.

Now, coming back to our issue. We are not special, maybe, if he

says so.

quoted

quoted

quoted

quoted

But this really feels like a design choice between putting the

burden on

quoted

quoted

quoted

quoted

device tree and OEMs or in the module maintainers. And I'd rather

have

quoted

quoted

quoted

quoted

the OEM deal with their device than me having to update the

module for

quoted

quoted

quoted

quoted

each generations of hardware. Indeed, this looks like an

"endless"

quoted

quoted

quoted

quoted

amount of quirks, but I'd rather have this endless amount of

quirks than

quoted

quoted

quoted

quoted

having to maintain an endless amount of list of new devices that

behaves

quoted

quoted

quoted

quoted

the same way. We are talking here about "wacom,w9013", but then

comes

quoted

quoted

quoted

quoted

"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,

quoted

quoted

quoted

but the kernel only needs to know about the w9013. That is until

there

quoted

quoted

quoted

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

quoted

quoted

quoted

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,

quoted

quoted

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

quoted

quoted

that needs explicit control of the regulator.

In this particular case I feel like we can draw a lot of parallels

to

quoted

quoted

an SDIO bus.

When you specify an SDIO bus you don't specify what kind of card

will

quoted

quoted

be present, you just say "I've got an SDIO bus" and then the

specific

quoted

quoted

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