Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1540188 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 1/2] devicetree: i2c-hid: Add Wacom digitizer +  regulator support Jiri Kosina <jikos@kernel.org> - 2016-12-12 10:00 +0100
    Re: [PATCH v2 1/2] devicetree: i2c-hid: Add Wacom digitizer +  regulator support Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-12-12 11:10 +0100
      Re: [PATCH v2 1/2] devicetree: i2c-hid: Add Wacom digitizer +  regulator support Rob Herring <robh@kernel.org> - 2016-12-12 15:50 +0100
        Re: [PATCH v2 1/2] devicetree: i2c-hid: Add Wacom digitizer +  regulator support Brian Norris <briannorris@chromium.org> - 2016-12-12 19:40 +0100
          Re: [PATCH v2 1/2] devicetree: i2c-hid: Add Wacom digitizer +  regulator support Rob Herring <robh@kernel.org> - 2016-12-13 23:20 +0100

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

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

[toc] | [next] | [standalone]

#1540234

On Dec 12 2016 or thereabouts, Jiri Kosina wrote:
> Given the timing (merge window being open) and given then NACK given by 
> Rob, I've now unapplied the patches (the for-4.10/i2c-hid branch is now 
> obsolete, and has been superseded by for-4.10/i2c-hid-nopower).
> 
> However, this is mostly done in order to provide more time for discussion; 
> I still disagree with the reasoning behind the NACK.
> 

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

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

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

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

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

Thoughts? yes? no? bullshit?

Cheers,
Benjamin

[toc] | [prev] | [next] | [standalone]

#1540358

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

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

Anyway, we're only debating this:

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

vs.

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

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

Rob

[toc] | [prev] | [next] | [standalone]

#1540519

Hi all,

On Mon, Dec 12, 2016 at 08:47:06AM -0600, Rob Herring wrote:

[Snip Benjamin's proposal; I agree we don't really want a multi-level DT
layout purely for making the driver look a little nicer (I'm not sure
this would really be nicer anyway). And I think, as Rob notes here, our
disagreement is smaller than appears. But I might be wrong.]

> Anyway, we're only debating this:

OK, so I think we might have a consensus of sorts? I'll describe it
here, in case I'm wrong. Otherwise, I'll send another rev.

>          i2c-hid-dev@2c {
>                  compatible = "wacom,w9013", "hid-over-i2c";

I plan to document the above, but not treat "wacom,w9013" specially in
the driver, apart from possibly listing it in the driver of_match_table.
This was mentioned by Dmitry earlier, and I didn't see any objection.

(Note that there are problems with module autoload when using multiple
compatible strings like above. May not be supremely relevant to the
documentation, but it *is* practically important.)

>                  reg = <0x2c>;
>                  hid-descr-addr = <0x0020>;
>                  interrupt-parent = <&gpx3>;
>                  interrupts = <3 2>;
>                  vdd-supply = <sth>;

Document and support 'vdd-supply', optionally.

>                  init-delay-ms = <100>;

Per Rob's mention below, support this as 'post-power-on-delay-ms',
optionally.

We can use either of these properties on any device, with the
intention that if there are future needs for divergent bindings, the
aforementioned compatible property can help us differentiate.

>          };
> 
> vs.
> 
>          i2c-hid-dev@2c {
>                  compatible = "hid-over-i2c";
>                  reg = <0x2c>;
>                  hid-descr-addr = <0x0020>;
>                  interrupt-parent = <&gpx3>;
>                  interrupts = <3 2>;
>                  vdd-supply = <sth>;
>                  init-delay-ms = <100>;
>          };
> 
> My only other nit is use "post-power-on-delay-ms" which is already a
> defined property name rather than "init-delay-ms".

Any objections? Speak now or forever [1] hold your peace.

Brian

[1] Who am I kidding? There's always room for more paint on the
bikeshed.

[toc] | [prev] | [next] | [standalone]

#1541540

On Mon, Dec 12, 2016 at 12:34 PM, Brian Norris <briannorris@chromium.org> wrote:
> Hi all,
>
> On Mon, Dec 12, 2016 at 08:47:06AM -0600, Rob Herring wrote:
>
> [Snip Benjamin's proposal; I agree we don't really want a multi-level DT
> layout purely for making the driver look a little nicer (I'm not sure
> this would really be nicer anyway). And I think, as Rob notes here, our
> disagreement is smaller than appears. But I might be wrong.]
>
>> Anyway, we're only debating this:
>
> OK, so I think we might have a consensus of sorts? I'll describe it
> here, in case I'm wrong. Otherwise, I'll send another rev.
>
>>          i2c-hid-dev@2c {
>>                  compatible = "wacom,w9013", "hid-over-i2c";
>
> I plan to document the above, but not treat "wacom,w9013" specially in
> the driver, apart from possibly listing it in the driver of_match_table.
> This was mentioned by Dmitry earlier, and I didn't see any objection.
>
> (Note that there are problems with module autoload when using multiple
> compatible strings like above. May not be supremely relevant to the
> documentation, but it *is* practically important.)

I'm not sure what is not working here exactly. We emit all the
compatible strings in the uevent. However, it looks like this is only
called for platform devices. In the case of i2c, I don't think any
compatible string is emitted. It looks to me like i2c_device_uevent
just needs a call to of_device_get_modalias to fix this. There's some
issues in the I2C core in how it does matching and maybe this is
related? I would guess if it was that easy, then it would be fixed
already. Maybe not.

>>                  reg = <0x2c>;
>>                  hid-descr-addr = <0x0020>;
>>                  interrupt-parent = <&gpx3>;
>>                  interrupts = <3 2>;
>>                  vdd-supply = <sth>;
>
> Document and support 'vdd-supply', optionally.
>
>>                  init-delay-ms = <100>;
>
> Per Rob's mention below, support this as 'post-power-on-delay-ms',
> optionally.
>
> We can use either of these properties on any device, with the
> intention that if there are future needs for divergent bindings, the
> aforementioned compatible property can help us differentiate.

TBC, from a DT perspective (and what the binding should say), is the
properties are only valid with a wacom compatible present (or any
others you want to add). If the driver doesn't enforce that and
supports having those properties with just "hid-over-i2c", then
downstream dts's can use that for all I care.

Rob

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web