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

Groups > linux.kernel > #1533729 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 1/2] devicetree: i2c-hid: Add Wacom digitizer + regulator support Brian Norris <briannorris@chromium.org> - 2016-12-01 02:30 +0100
    [PATCH v2 2/2] HID: i2c-hid: support Wacom digitizer + regulator Brian Norris <briannorris@chromium.org> - 2016-12-01 02:30 +0100
      Re: [PATCH v2 2/2] HID: i2c-hid: support Wacom digitizer + regulator Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-12-01 15:50 +0100
        Re: [PATCH v2 2/2] HID: i2c-hid: support Wacom digitizer + regulator Brian Norris <briannorris@chromium.org> - 2016-12-01 18:40 +0100
    Re: [PATCH v2 1/2] devicetree: i2c-hid: Add Wacom digitizer +  regulator support Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-12-01 15:40 +0100
      Re: [PATCH v2 1/2] devicetree: i2c-hid: Add Wacom digitizer +  regulator support Brian Norris <briannorris@chromium.org> - 2016-12-01 18:30 +0100
        Re: [PATCH v2 1/2] devicetree: i2c-hid: Add Wacom digitizer +  regulator support Rob Herring <robh@kernel.org> - 2016-12-06 01:00 +0100
          Re: [PATCH v2 1/2] devicetree: i2c-hid: Add Wacom digitizer +  regulator support Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2016-12-06 01:40 +0100
          Re: [PATCH v2 1/2] devicetree: i2c-hid: Add Wacom digitizer +  regulator support Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-12-06 09:50 +0100
            Re: [PATCH v2 1/2] devicetree: i2c-hid: Add Wacom digitizer +  regulator support Rob Herring <robh@kernel.org> - 2016-12-06 16:00 +0100
              Re: [PATCH v2 1/2] devicetree: i2c-hid: Add Wacom digitizer +  regulator support Doug Anderson <dianders@chromium.org> - 2016-12-06 17:20 +0100
                Re: [PATCH v2 1/2] devicetree: i2c-hid: Add Wacom digitizer +  regulator support Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-12-08 16:50 +0100
                  Re: [PATCH v2 1/2] devicetree: i2c-hid: Add Wacom digitizer +  regulator support Rob Herring <robh@kernel.org> - 2016-12-08 17:10 +0100
                    Re: [PATCH v2 1/2] devicetree: i2c-hid: Add Wacom digitizer + regulator support Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2016-12-08 17:20 +0100
                      Re: [PATCH v2 1/2] devicetree: i2c-hid: Add Wacom digitizer +  regulator support Rob Herring <robh@kernel.org> - 2016-12-08 17:30 +0100
                        Re: [PATCH v2 1/2] devicetree: i2c-hid: Add Wacom digitizer +  regulator support Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2016-12-08 19:20 +0100
                          Re: [PATCH v2 1/2] devicetree: i2c-hid: Add Wacom digitizer +  regulator support Rob Herring <robh@kernel.org> - 2016-12-09 15:40 +0100
                        Re: [PATCH v2 1/2] devicetree: i2c-hid: Add Wacom digitizer +  regulator support Jiri Kosina <jikos@kernel.org> - 2016-12-09 15:40 +0100
                          Re: [PATCH v2 1/2] devicetree: i2c-hid: Add Wacom digitizer +  regulator support Rob Herring <robh@kernel.org> - 2016-12-09 16:10 +0100
                            Re: [PATCH v2 1/2] devicetree: i2c-hid: Add Wacom digitizer +  regulator support Doug Anderson <dianders@chromium.org> - 2016-12-09 17:20 +0100
                Re: [PATCH v2 1/2] devicetree: i2c-hid: Add Wacom digitizer +  regulator support Rob Herring <robh@kernel.org> - 2016-12-08 17:10 +0100
                  Re: [PATCH v2 1/2] devicetree: i2c-hid: Add Wacom digitizer +  regulator support Doug Anderson <dianders@chromium.org> - 2016-12-09 17:10 +0100
                    Re: [PATCH v2 1/2] devicetree: i2c-hid: Add Wacom digitizer +  regulator support Rob Herring <robh@kernel.org> - 2016-12-09 18:50 +0100
    Re: [PATCH v2 1/2] devicetree: i2c-hid: Add Wacom digitizer +  regulator support Rob Herring <robh@kernel.org> - 2016-12-06 00:50 +0100
      Re: [PATCH v2 1/2] devicetree: i2c-hid: Add Wacom digitizer +  regulator support Brian Norris <briannorris@chromium.org> - 2016-12-06 01:00 +0100

Page 2 of 2 — ← Prev page 1 [2]

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

On Tue, Dec 6, 2016 at 10:18 AM, Doug Anderson <dianders@chromium.org> wrote:
> Hi,
>
> On Tue, Dec 6, 2016 at 6:56 AM, Rob Herring <robh@kernel.org> wrote:
>> On Tue, Dec 6, 2016 at 2:48 AM, Benjamin Tissoires
>> <benjamin.tissoires@redhat.com> 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 <wxt@rock-chips.com>
>>>> > > >
>>>> > > > 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 <wxt@rock-chips.com>
>>>> > > > Cc: Rob Herring <robh+dt@kernel.org>
>>>> > > > Cc: Jiri Kosina <jikos@kernel.org>
>>>> > > > Cc: linux-input@vger.kernel.org
>>>> > > > Signed-off-by: Brian Norris <briannorris@chromium.org>
>>>> > > > ---
>>>> > > > v1 was a few months back. I finally got around to rewriting it based on
>>>> > > > 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 AND w9013,
>> but the kernel only needs to know about the w9013. That is until there
>> is some difference which is why the DT should list w9014 to start
>> with.
>>
>> If you don't have any power control to deal with, then the kernel can
>> always just match on "hid-over-i2c" compatible.
>
> Just my $0.02.  Feel free to ignore.
>
> One thought is that I would say that the need to power on the device
> explicitly seems more like a board level difference and less like a
> difference associated with a particular digitizer.  Said another way,
> 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.

> 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.

Rob

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

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

Hi,

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

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

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

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

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

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

On Wed, Nov 30, 2016 at 05:21:27PM -0800, Brian Norris wrote:
> From: Caesar Wang <wxt@rock-chips.com>
> 
> 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 <wxt@rock-chips.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Jiri Kosina <jikos@kernel.org>
> Cc: linux-input@vger.kernel.org
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
> v1 was a few months back. I finally got around to rewriting it based on
> 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 <robh@kernel.org>

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

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

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 <wxt@rock-chips.com>
> > 
> > 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 <wxt@rock-chips.com>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Jiri Kosina <jikos@kernel.org>
> > Cc: linux-input@vger.kernel.org
> > Signed-off-by: Brian Norris <briannorris@chromium.org>
> > ---
> > v1 was a few months back. I finally got around to rewriting it based on
> > 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 <robh@kernel.org>

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

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web