From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932655AbcLARY4 (ORCPT ); Thu, 1 Dec 2016 12:24:56 -0500 Received: from mail-pg0-f50.google.com ([74.125.83.50]:36259 "EHLO mail-pg0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754342AbcLARYx (ORCPT ); Thu, 1 Dec 2016 12:24:53 -0500 Date: Thu, 1 Dec 2016 09:24:50 -0800 From: Brian Norris To: Benjamin Tissoires , Rob Herring Cc: Jiri Kosina , Caesar Wang , linux-rockchip@lists.infradead.org, Rob Herring , linux-input@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Dmitry Torokhov , Mark Rutland , Doug Anderson Subject: Re: [PATCH v2 1/2] devicetree: i2c-hid: Add Wacom digitizer + regulator support Message-ID: <20161201172448.GA46688@google.com> References: <1480555288-142791-1-git-send-email-briannorris@chromium.org> <20161201143434.GD1280@mail.corp.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161201143434.GD1280@mail.corp.redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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-input@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