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

[Rob Herring]

On Mon, Dec 12, 2016 at 12:34 PM, Brian Norris  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 = <>;
>>  interrupts = <3 2>;
>>  vdd-supply = ;
>
> 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

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

[Rob Herring]

On Mon, Dec 12, 2016 at 12:34 PM, Brian Norris  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 = <>;
>>  interrupts = <3 2>;
>>  vdd-supply = ;
>
> 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

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

[Brian Norris]

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 = <>;
>  interrupts = <3 2>;
>  vdd-supply = ;

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 = <>;
>  interrupts = <3 2>;
>  vdd-supply = ;
>  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.

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

[Brian Norris]

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 = <>;
>  interrupts = <3 2>;
>  vdd-supply = ;

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 = <>;
>  interrupts = <3 2>;
>  vdd-supply = ;
>  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.

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

[Rob Herring]

On Mon, Dec 12, 2016 at 4:01 AM, Benjamin Tissoires
 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 = <>;
> interrupts = <3 2>;
> vdd-supply = ;
> 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 = ;
> i2c-hid-dev@2c {
> compatible = "hid-over-i2c";
> reg = <0x2c>;
> hid-descr-addr = <0x0020>;
> interrupt-parent = <>;
> 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 = <>;
 interrupts = <3 2>;
 vdd-supply = ;
 init-delay-ms = <100>;
 };

vs.

 i2c-hid-dev@2c {
 compatible = "hid-over-i2c";
 reg = <0x2c>;
 hid-descr-addr = <0x0020>;
 interrupt-parent = <>;
 interrupts = <3 2>;
 vdd-supply = ;
 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

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

[Rob Herring]

On Mon, Dec 12, 2016 at 4:01 AM, Benjamin Tissoires
 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 = <>;
> interrupts = <3 2>;
> vdd-supply = ;
> 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 = ;
> i2c-hid-dev@2c {
> compatible = "hid-over-i2c";
> reg = <0x2c>;
> hid-descr-addr = <0x0020>;
> interrupt-parent = <>;
> 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 = <>;
 interrupts = <3 2>;
 vdd-supply = ;
 init-delay-ms = <100>;
 };

vs.

 i2c-hid-dev@2c {
 compatible = "hid-over-i2c";
 reg = <0x2c>;
 hid-descr-addr = <0x0020>;
 interrupt-parent = <>;
 interrupts = <3 2>;
 vdd-supply = ;
 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

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

[Benjamin Tissoires]

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 = <>;
interrupts = <3 2>;
vdd-supply = ;
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 = ;
i2c-hid-dev@2c {
compatible = "hid-over-i2c";
reg = <0x2c>;
hid-descr-addr = <0x0020>;
interrupt-parent = <>;
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

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

[Benjamin Tissoires]

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 = <>;
interrupts = <3 2>;
vdd-supply = ;
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 = ;
i2c-hid-dev@2c {
compatible = "hid-over-i2c";
reg = <0x2c>;
hid-descr-addr = <0x0020>;
interrupt-parent = <>;
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

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

[Jiri Kosina]

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.

-- 
Jiri Kosina
SUSE Labs

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

[Jiri Kosina]

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.

-- 
Jiri Kosina
SUSE Labs

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

[Rob Herring]

On Fri, Dec 9, 2016 at 10:05 AM, Doug Anderson  wrote:
> Hi,
>
> On Thu, Dec 8, 2016 at 8:01 AM, Rob Herring  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

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

[Rob Herring]

On Fri, Dec 9, 2016 at 10:05 AM, Doug Anderson  wrote:
> Hi,
>
> On Thu, Dec 8, 2016 at 8:01 AM, Rob Herring  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

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

[Doug Anderson]

Hi,

On Fri, Dec 9, 2016 at 7:01 AM, Rob Herring  wrote:
> On Fri, Dec 9, 2016 at 8:36 AM, Jiri Kosina  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

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

[Doug Anderson]

Hi,

On Fri, Dec 9, 2016 at 7:01 AM, Rob Herring  wrote:
> On Fri, Dec 9, 2016 at 8:36 AM, Jiri Kosina  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

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

[Doug Anderson]

Hi,

On Thu, Dec 8, 2016 at 8:01 AM, Rob Herring  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

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

[Doug Anderson]

Hi,

On Thu, Dec 8, 2016 at 8:01 AM, Rob Herring  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

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

[Rob Herring]

On Fri, Dec 9, 2016 at 8:36 AM, Jiri Kosina  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

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

[Rob Herring]

On Fri, Dec 9, 2016 at 8:36 AM, Jiri Kosina  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

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

[Rob Herring]

On Thu, Dec 8, 2016 at 12:12 PM, Dmitry Torokhov
 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
>>  wrote:
>> > On December 8, 2016 8:03:06 AM PST, Rob Herring  wrote:
>> >>On Thu, Dec 8, 2016 at 9:41 AM, Benjamin Tissoires
>> >> 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 

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

[Rob Herring]

On Thu, Dec 8, 2016 at 12:12 PM, Dmitry Torokhov
 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
>>  wrote:
>> > On December 8, 2016 8:03:06 AM PST, Rob Herring  wrote:
>> >>On Thu, Dec 8, 2016 at 9:41 AM, Benjamin Tissoires
>> >> 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 

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

[Jiri Kosina]

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.

-- 
Jiri Kosina
SUSE Labs

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

[Jiri Kosina]

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.

-- 
Jiri Kosina
SUSE Labs

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

[Dmitry Torokhov]

On Thu, Dec 08, 2016 at 10:26:41AM -0600, Rob Herring wrote:
> On Thu, Dec 8, 2016 at 10:13 AM, Dmitry Torokhov
>  wrote:
> > On December 8, 2016 8:03:06 AM PST, Rob Herring  wrote:
> >>On Thu, Dec 8, 2016 at 9:41 AM, Benjamin Tissoires
> >> wrote:
> >>> On Dec 06 2016 or thereabouts, Doug Anderson wrote:
>  Hi,
> 
>  On Tue, Dec 6, 2016 at 6:56 AM, Rob Herring  wrote:
>  > On Tue, Dec 6, 2016 at 2:48 AM, Benjamin Tissoires
>  >  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 
>  >>> > > >
>  >>> > > > 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 
>  >>> > > > Cc: Rob Herring 
>  >>> > > > Cc: Jiri Kosina 
>  >>> > > > Cc: linux-in...@vger.kernel.org
>  >>> > > > Signed-off-by: Brian Norris 
>  >>> > > > ---
>  >>> > > > 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, 

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

[Dmitry Torokhov]

On Thu, Dec 08, 2016 at 10:26:41AM -0600, Rob Herring wrote:
> On Thu, Dec 8, 2016 at 10:13 AM, Dmitry Torokhov
>  wrote:
> > On December 8, 2016 8:03:06 AM PST, Rob Herring  wrote:
> >>On Thu, Dec 8, 2016 at 9:41 AM, Benjamin Tissoires
> >> wrote:
> >>> On Dec 06 2016 or thereabouts, Doug Anderson wrote:
>  Hi,
> 
>  On Tue, Dec 6, 2016 at 6:56 AM, Rob Herring  wrote:
>  > On Tue, Dec 6, 2016 at 2:48 AM, Benjamin Tissoires
>  >  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 
>  >>> > > >
>  >>> > > > 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 
>  >>> > > > Cc: Rob Herring 
>  >>> > > > Cc: Jiri Kosina 
>  >>> > > > Cc: linux-in...@vger.kernel.org
>  >>> > > > Signed-off-by: Brian Norris 
>  >>> > > > ---
>  >>> > > > 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 

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

[Rob Herring]

On Thu, Dec 8, 2016 at 10:13 AM, Dmitry Torokhov
 wrote:
> On December 8, 2016 8:03:06 AM PST, Rob Herring  wrote:
>>On Thu, Dec 8, 2016 at 9:41 AM, Benjamin Tissoires
>> wrote:
>>> On Dec 06 2016 or thereabouts, Doug Anderson wrote:
 Hi,

 On Tue, Dec 6, 2016 at 6:56 AM, Rob Herring  wrote:
 > On Tue, Dec 6, 2016 at 2:48 AM, Benjamin Tissoires
 >  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 
 >>> > > >
 >>> > > > 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 
 >>> > > > Cc: Rob Herring 
 >>> > > > Cc: Jiri Kosina 
 >>> > > > Cc: linux-in...@vger.kernel.org
 >>> > > > Signed-off-by: Brian Norris 
 >>> > > > ---
 >>> > > > 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].
 >>
 >> 

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

[Rob Herring]

On Thu, Dec 8, 2016 at 10:13 AM, Dmitry Torokhov
 wrote:
> On December 8, 2016 8:03:06 AM PST, Rob Herring  wrote:
>>On Thu, Dec 8, 2016 at 9:41 AM, Benjamin Tissoires
>> wrote:
>>> On Dec 06 2016 or thereabouts, Doug Anderson wrote:
 Hi,

 On Tue, Dec 6, 2016 at 6:56 AM, Rob Herring  wrote:
 > On Tue, Dec 6, 2016 at 2:48 AM, Benjamin Tissoires
 >  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 
 >>> > > >
 >>> > > > 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 
 >>> > > > Cc: Rob Herring 
 >>> > > > Cc: Jiri Kosina 
 >>> > > > Cc: linux-in...@vger.kernel.org
 >>> > > > Signed-off-by: Brian Norris 
 >>> > > > ---
 >>> > > > 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

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

[Dmitry Torokhov]

On December 8, 2016 8:03:06 AM PST, Rob Herring  wrote:
>On Thu, Dec 8, 2016 at 9:41 AM, Benjamin Tissoires
> wrote:
>> On Dec 06 2016 or thereabouts, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Tue, Dec 6, 2016 at 6:56 AM, Rob Herring  wrote:
>>> > On Tue, Dec 6, 2016 at 2:48 AM, Benjamin Tissoires
>>> >  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 
>>> >>> > > >
>>> >>> > > > 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 
>>> >>> > > > Cc: Rob Herring 
>>> >>> > > > Cc: Jiri Kosina 
>>> >>> > > > Cc: linux-in...@vger.kernel.org
>>> >>> > > > Signed-off-by: Brian Norris 
>>> >>> > > > ---
>>> >>> > > > 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 

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

[Dmitry Torokhov]

On December 8, 2016 8:03:06 AM PST, Rob Herring  wrote:
>On Thu, Dec 8, 2016 at 9:41 AM, Benjamin Tissoires
> wrote:
>> On Dec 06 2016 or thereabouts, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Tue, Dec 6, 2016 at 6:56 AM, Rob Herring  wrote:
>>> > On Tue, Dec 6, 2016 at 2:48 AM, Benjamin Tissoires
>>> >  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 
>>> >>> > > >
>>> >>> > > > 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 
>>> >>> > > > Cc: Rob Herring 
>>> >>> > > > Cc: Jiri Kosina 
>>> >>> > > > Cc: linux-in...@vger.kernel.org
>>> >>> > > > Signed-off-by: Brian Norris 
>>> >>> > > > ---
>>> >>> > > > 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.
>>> 

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

[Rob Herring]

On Thu, Dec 8, 2016 at 9:41 AM, Benjamin Tissoires
 wrote:
> On Dec 06 2016 or thereabouts, Doug Anderson wrote:
>> Hi,
>>
>> On Tue, Dec 6, 2016 at 6:56 AM, Rob Herring  wrote:
>> > On Tue, Dec 6, 2016 at 2:48 AM, Benjamin Tissoires
>> >  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 
>> >>> > > >
>> >>> > > > 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 
>> >>> > > > Cc: Rob Herring 
>> >>> > > > Cc: Jiri Kosina 
>> >>> > > > Cc: linux-in...@vger.kernel.org
>> >>> > > > Signed-off-by: Brian Norris 
>> >>> > > > ---
>> >>> > > > 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 

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

[Rob Herring]

On Thu, Dec 8, 2016 at 9:41 AM, Benjamin Tissoires
 wrote:
> On Dec 06 2016 or thereabouts, Doug Anderson wrote:
>> Hi,
>>
>> On Tue, Dec 6, 2016 at 6:56 AM, Rob Herring  wrote:
>> > On Tue, Dec 6, 2016 at 2:48 AM, Benjamin Tissoires
>> >  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 
>> >>> > > >
>> >>> > > > 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 
>> >>> > > > Cc: Rob Herring 
>> >>> > > > Cc: Jiri Kosina 
>> >>> > > > Cc: linux-in...@vger.kernel.org
>> >>> > > > Signed-off-by: Brian Norris 
>> >>> > > > ---
>> >>> > > > 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 

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

[Rob Herring]

On Tue, Dec 6, 2016 at 10:18 AM, Doug Anderson  wrote:
> Hi,
>
> On Tue, Dec 6, 2016 at 6:56 AM, Rob Herring  wrote:
>> On Tue, Dec 6, 2016 at 2:48 AM, Benjamin Tissoires
>>  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 
 > > >
 > > > 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 
 > > > Cc: Rob Herring 
 > > > Cc: Jiri Kosina 
 > > > Cc: linux-in...@vger.kernel.org
 > > > Signed-off-by: Brian Norris 
 > > > ---
 > > > 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 

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

[Rob Herring]

On Tue, Dec 6, 2016 at 10:18 AM, Doug Anderson  wrote:
> Hi,
>
> On Tue, Dec 6, 2016 at 6:56 AM, Rob Herring  wrote:
>> On Tue, Dec 6, 2016 at 2:48 AM, Benjamin Tissoires
>>  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 
 > > >
 > > > 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 
 > > > Cc: Rob Herring 
 > > > Cc: Jiri Kosina 
 > > > Cc: linux-in...@vger.kernel.org
 > > > Signed-off-by: Brian Norris 
 > > > ---
 > > > 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 

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

[Benjamin Tissoires]

On Dec 06 2016 or thereabouts, Doug Anderson wrote:
> Hi,
> 
> On Tue, Dec 6, 2016 at 6:56 AM, Rob Herring  wrote:
> > On Tue, Dec 6, 2016 at 2:48 AM, Benjamin Tissoires
> >  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 
> >>> > > >
> >>> > > > 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 
> >>> > > > Cc: Rob Herring 
> >>> > > > Cc: Jiri Kosina 
> >>> > > > Cc: linux-in...@vger.kernel.org
> >>> > > > Signed-off-by: Brian Norris 
> >>> > > > ---
> >>> > > > 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
> 

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

[Benjamin Tissoires]

On Dec 06 2016 or thereabouts, Doug Anderson wrote:
> Hi,
> 
> On Tue, Dec 6, 2016 at 6:56 AM, Rob Herring  wrote:
> > On Tue, Dec 6, 2016 at 2:48 AM, Benjamin Tissoires
> >  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 
> >>> > > >
> >>> > > > 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 
> >>> > > > Cc: Rob Herring 
> >>> > > > Cc: Jiri Kosina 
> >>> > > > Cc: linux-in...@vger.kernel.org
> >>> > > > Signed-off-by: Brian Norris 
> >>> > > > ---
> >>> > > > 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
> >> 

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

[Doug Anderson]

Hi,

On Tue, Dec 6, 2016 at 6:56 AM, Rob Herring  wrote:
> On Tue, Dec 6, 2016 at 2:48 AM, Benjamin Tissoires
>  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 
>>> > > >
>>> > > > 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 
>>> > > > Cc: Rob Herring 
>>> > > > Cc: Jiri Kosina 
>>> > > > Cc: linux-in...@vger.kernel.org
>>> > > > Signed-off-by: Brian Norris 
>>> > > > ---
>>> > > > 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
>> 

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

[Doug Anderson]

Hi,

On Tue, Dec 6, 2016 at 6:56 AM, Rob Herring  wrote:
> On Tue, Dec 6, 2016 at 2:48 AM, Benjamin Tissoires
>  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 
>>> > > >
>>> > > > 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 
>>> > > > Cc: Rob Herring 
>>> > > > Cc: Jiri Kosina 
>>> > > > Cc: linux-in...@vger.kernel.org
>>> > > > Signed-off-by: Brian Norris 
>>> > > > ---
>>> > > > 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
>> 

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

[Rob Herring]

On Tue, Dec 6, 2016 at 2:48 AM, Benjamin Tissoires
 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 
>> > > >
>> > > > 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 
>> > > > Cc: Rob Herring 
>> > > > Cc: Jiri Kosina 
>> > > > Cc: linux-in...@vger.kernel.org
>> > > > Signed-off-by: Brian Norris 
>> > > > ---
>> > > > 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 

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

[Rob Herring]

On Tue, Dec 6, 2016 at 2:48 AM, Benjamin Tissoires
 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 
>> > > >
>> > > > 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 
>> > > > Cc: Rob Herring 
>> > > > Cc: Jiri Kosina 
>> > > > Cc: linux-in...@vger.kernel.org
>> > > > Signed-off-by: Brian Norris 
>> > > > ---
>> > > > 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 

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

[Benjamin Tissoires]

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 
> > > > 
> > > > 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 
> > > > Cc: Rob Herring 
> > > > Cc: Jiri Kosina 
> > > > Cc: linux-in...@vger.kernel.org
> > > > Signed-off-by: Brian Norris 
> > > > ---
> > > > 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 

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

[Benjamin Tissoires]

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 
> > > > 
> > > > 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 
> > > > Cc: Rob Herring 
> > > > Cc: Jiri Kosina 
> > > > Cc: linux-in...@vger.kernel.org
> > > > Signed-off-by: Brian Norris 
> > > > ---
> > > > 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 

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

[Dmitry Torokhov]

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 
> > > > 
> > > > 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 
> > > > Cc: Rob Herring 
> > > > Cc: Jiri Kosina 
> > > > Cc: linux-in...@vger.kernel.org
> > > > Signed-off-by: Brian Norris 
> > > > ---
> > > > 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.

-- 
Dmitry

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

[Dmitry Torokhov]

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 
> > > > 
> > > > 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 
> > > > Cc: Rob Herring 
> > > > Cc: Jiri Kosina 
> > > > Cc: linux-in...@vger.kernel.org
> > > > Signed-off-by: Brian Norris 
> > > > ---
> > > > 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.

-- 
Dmitry

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

[Rob Herring]

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 
> > > 
> > > 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 
> > > Cc: Rob Herring 
> > > Cc: Jiri Kosina 
> > > Cc: linux-in...@vger.kernel.org
> > > Signed-off-by: Brian Norris 
> > > ---
> > > 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

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

[Rob Herring]

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 
> > > 
> > > 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 
> > > Cc: Rob Herring 
> > > Cc: Jiri Kosina 
> > > Cc: linux-in...@vger.kernel.org
> > > Signed-off-by: Brian Norris 
> > > ---
> > > 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

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

[Brian Norris]

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 
> > 
> > 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 
> > Cc: Rob Herring 
> > Cc: Jiri Kosina 
> > Cc: linux-in...@vger.kernel.org
> > Signed-off-by: Brian Norris 
> > ---
> > 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 

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

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

[Brian Norris]

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 
> > 
> > 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 
> > Cc: Rob Herring 
> > Cc: Jiri Kosina 
> > Cc: linux-in...@vger.kernel.org
> > Signed-off-by: Brian Norris 
> > ---
> > 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 

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

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

[Rob Herring]

On Wed, Nov 30, 2016 at 05:21:27PM -0800, Brian Norris wrote:
> From: Caesar Wang 
> 
> 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 
> Cc: Rob Herring 
> Cc: Jiri Kosina 
> Cc: linux-in...@vger.kernel.org
> Signed-off-by: Brian Norris 
> ---
> 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 

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

[Rob Herring]

On Wed, Nov 30, 2016 at 05:21:27PM -0800, Brian Norris wrote:
> From: Caesar Wang 
> 
> 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 
> Cc: Rob Herring 
> Cc: Jiri Kosina 
> Cc: linux-in...@vger.kernel.org
> Signed-off-by: Brian Norris 
> ---
> 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 

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

[Brian Norris]

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 
> > 
> > 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 
> > Cc: Rob Herring 
> > Cc: Jiri Kosina 
> > Cc: linux-in...@vger.kernel.org
> > Signed-off-by: Brian Norris 
> > ---
> > 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

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

[Brian Norris]

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 
> > 
> > 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 
> > Cc: Rob Herring 
> > Cc: Jiri Kosina 
> > Cc: linux-in...@vger.kernel.org
> > Signed-off-by: Brian Norris 
> > ---
> > 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

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

[Benjamin Tissoires]

On Nov 30 2016 or thereabouts, Brian Norris wrote:
> From: Caesar Wang 
> 
> 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 
> Cc: Rob Herring 
> Cc: Jiri Kosina 
> Cc: linux-in...@vger.kernel.org
> Signed-off-by: Brian Norris 
> ---
> 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
> 

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

[Benjamin Tissoires]

On Nov 30 2016 or thereabouts, Brian Norris wrote:
> From: Caesar Wang 
> 
> 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 
> Cc: Rob Herring 
> Cc: Jiri Kosina 
> Cc: linux-in...@vger.kernel.org
> Signed-off-by: Brian Norris 
> ---
> 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
>