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
<benjamin.tissoires@xxxxxxxxxx> wrote:
> On Dec 12 2016 or thereabouts, Jiri Kosina wrote:
>> Given the timing (merge window being open) and given then NACK given by
>> Rob, I've now unapplied the patches (the for-4.10/i2c-hid branch is now
>> obsolete, and has been superseded by for-4.10/i2c-hid-nopower).
>>
>> However, this is mostly done in order to provide more time for discussion;
>> I still disagree with the reasoning behind the NACK.
>>
>
> To hopefully make things going forward a little bit, I was wondering
> over the week-end if we should not solve this particular issue by adding
> an intermediate platform DT node:
>
> instead of having:
> ---
>         i2c-hid-dev@2c {
>                 compatible = "hid-over-i2c";
>                 reg = <0x2c>;
>                 hid-descr-addr = <0x0020>;
>                 interrupt-parent = <&gpx3>;
>                 interrupts = <3 2>;
>                 vdd-supply = <sth>;
>                 init-delay-ms = <100>;
>         };
> ---
>
> we would have:
> ---
>         platform-i2c-hid@01 {
>                 compatible = "very-special-board-that-needs-firmware-quirks-and-delay-of-100ms";
>                 vdd-supply = <sth>;
>                 i2c-hid-dev@2c {
>                         compatible = "hid-over-i2c";
>                         reg = <0x2c>;
>                         hid-descr-addr = <0x0020>;
>                         interrupt-parent = <&gpx3>;
>                         interrupts = <3 2>;
>                 };
>         };
> ---
>
> If I am not wrong, the platform device should be initialized before
> i2c-hid get called, which allows to setup properly the vdd supply.
> On resume/suspend, the tree should be respected and we should be able to
> enable/disable power in the same fashion this patch provides.
>
> We could then extend this platform device at will without tinkering in
> i2c-hid and we could also handle the GPIOs, reset or whatever is
> required in the future through compatibles.
>
> Thoughts? yes? no? bullshit?

That is structuring the DT match your driver structure, not what the
h/w looks like. This would make sense if the device was multi-function
of which HID-over-I2C was one function. You could do what you describe
with a single node and the platform driver creates the hid-over-i2c
device. DT is not the only way to create devices.

Anyway, we're only debating this:

         i2c-hid-dev@2c {
                 compatible = "wacom,w9013", "hid-over-i2c";
                 reg = <0x2c>;
                 hid-descr-addr = <0x0020>;
                 interrupt-parent = <&gpx3>;
                 interrupts = <3 2>;
                 vdd-supply = <sth>;
                 init-delay-ms = <100>;
         };

vs.

         i2c-hid-dev@2c {
                 compatible = "hid-over-i2c";
                 reg = <0x2c>;
                 hid-descr-addr = <0x0020>;
                 interrupt-parent = <&gpx3>;
                 interrupts = <3 2>;
                 vdd-supply = <sth>;
                 init-delay-ms = <100>;
         };

My only other nit is use "post-power-on-delay-ms" which is already a
defined property name rather than "init-delay-ms".

Rob