On Mon, Dec 12, 2016 at 4:01 AM, Benjamin Tissoires
[off-list ref] wrote:

On Dec 12 2016 or thereabouts, Jiri Kosina wrote:

quoted

Given the timing (merge window being open) and given then NACK given by
Rob, I've now unapplied the patches (the for-4.10/i2c-hid branch is now
obsolete, and has been superseded by for-4.10/i2c-hid-nopower).

However, this is mostly done in order to provide more time for discussion;
I still disagree with the reasoning behind the NACK.

To hopefully make things going forward a little bit, I was wondering
over the week-end if we should not solve this particular issue by adding
an intermediate platform DT node:

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

we would have:
---
        platform-i2c-hid@01 {
                compatible = "very-special-board-that-needs-firmware-quirks-and-delay-of-100ms";
                vdd-supply = <sth>;
                i2c-hid-dev@2c {
                        compatible = "hid-over-i2c";
                        reg = <0x2c>;
                        hid-descr-addr = <0x0020>;
                        interrupt-parent = <&gpx3>;
                        interrupts = <3 2>;
                };
        };
---

If I am not wrong, the platform device should be initialized before
i2c-hid get called, which allows to setup properly the vdd supply.
On resume/suspend, the tree should be respected and we should be able to
enable/disable power in the same fashion this patch provides.

We could then extend this platform device at will without tinkering in
i2c-hid and we could also handle the GPIOs, reset or whatever is
required in the future through compatibles.

Thoughts? yes? no? bullshit?

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

Anyway, we're only debating this:

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

vs.

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

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

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html