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

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

[Brian Norris]

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"
 - 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.
+
 Example:
 
 	i2c-hid-dev@2c {
-- 
2.8.0.rc3.226.g39d4020

[PATCH v2 2/2] HID: i2c-hid: support Wacom digitizer + regulator

[Brian Norris]

We need to power on the digitizer before using it, and it's also nice to
save power in suspend by disabling it. Support an optional "vdd-supply"
and wire it up for the new Wacom device.

Wacom recommended waiting up to 100ms after powering on before trying to
access this device.

Signed-off-by: Brian Norris <briannorris@chromium.org>
Signed-off-by: Caesar Wang <wxt@rock-chips.com>
Cc: Jiri Kosina <jikos@kernel.org>
Cc: linux-input@vger.kernel.org
---
v1 was a few months back. I finally got around to rewriting it based on
DT binding feedback.

v2:
 * support compatible property for wacom, with specific "vdd-supply" name
 * support the 100ms delay needed for this digitizer
 * target regulator support only at specific device

 Documentation/devicetree/bindings/input/hid-over-i2c.txt | 6 +++++-
 drivers/hid/i2c-hid/i2c-hid.c | 70 ++++++++++++++++++++++++++++++++++++++++++-
 include/linux/i2c/i2c-hid.h   |  6 ++++
 2 files changed, 75 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index b3ec4f2de875..1bc174f3a788 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -37,7 +37,9 @@
 #include <linux/mutex.h>
 #include <linux/acpi.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/gpio/consumer.h>
+#include <linux/regulator/consumer.h>
 
 #include <linux/i2c/i2c-hid.h>
 
@@ -918,10 +920,25 @@ static inline int i2c_hid_acpi_pdata(struct i2c_client *client,
 #endif
 
 #ifdef CONFIG_OF
+
+/* of_device_id match data */
+struct i2c_hid_of_data {
+	/* Name of supply regulator. */
+	const char *supply_name;
+	/* Delay required after powering on device before it is usable. */
+	int init_delay_ms;
+};
+
+static const struct i2c_hid_of_data wacom_w9013_data = {
+	.supply_name		= "vdd",
+	.init_delay_ms		= 100,
+};
+
 static int i2c_hid_of_probe(struct i2c_client *client,
 		struct i2c_hid_platform_data *pdata)
 {
 	struct device *dev = &client->dev;
+	const struct i2c_hid_of_data *data = of_device_get_match_data(dev);
 	u32 val;
 	int ret;
 
@@ -937,10 +954,33 @@ static int i2c_hid_of_probe(struct i2c_client *client,
 	}
 	pdata->hid_descriptor_address = val;
 
+	if (data) {
+		pdata->init_delay_ms = data->init_delay_ms;
+		if (data->supply_name) {
+			pdata->supply = devm_regulator_get_optional(&client->dev,
+								    data->supply_name);
+			if (IS_ERR(pdata->supply)) {
+				ret = PTR_ERR(pdata->supply);
+				pdata->supply = NULL;
+				if (ret == -EPROBE_DEFER)
+					return ret;
+				if (ret == -ENODEV)
+					return 0;
+				dev_err(dev, "Failed to get %s regulator: %d\n",
+					data->supply_name, ret);
+				return ret;
+			}
+		}
+	}
+
 	return 0;
 }
 
 static const struct of_device_id i2c_hid_of_match[] = {
+	{
+		.compatible = "wacom,w9013",
+		.data = &wacom_w9013_data,
+	},
 	{ .compatible = "hid-over-i2c" },
 	{},
 };
@@ -983,6 +1023,17 @@ static int i2c_hid_probe(struct i2c_client *client,
 		ihid->pdata = *platform_data;
 	}
 
+	if (ihid->pdata.supply) {
+		ret = regulator_enable(ihid->pdata.supply);
+		if (ret < 0) {
+			dev_err(&client->dev, "Failed to enable regulator: %d\n",
+				ret);
+			return ret;
+		}
+		if (ihid->pdata.init_delay_ms)
+			msleep(ihid->pdata.init_delay_ms);
+	}
+
 	if (client->irq > 0) {
 		ihid->irq = client->irq;
 	} else if (ACPI_COMPANION(&client->dev)) {
@@ -1100,6 +1151,9 @@ static int i2c_hid_remove(struct i2c_client *client)
 	if (ihid->desc)
 		gpiod_put(ihid->desc);
 
+	if (ihid->pdata.supply)
+		regulator_disable(ihid->pdata.supply);
+
 	kfree(ihid);
 
 	acpi_dev_remove_driver_gpios(ACPI_COMPANION(&client->dev));
@@ -1152,6 +1206,11 @@ static int i2c_hid_suspend(struct device *dev)
 		else
 			hid_warn(hid, "Failed to enable irq wake: %d\n",
 				wake_status);
+	} else if (ihid->pdata.supply) {
+		ret = regulator_disable(ihid->pdata.supply);
+		if (ret < 0)
+			hid_warn(hid, "Failed to disable supply: %d\n",
+				 ret);
 	}
 
 	return 0;
@@ -1165,7 +1224,16 @@ static int i2c_hid_resume(struct device *dev)
 	struct hid_device *hid = ihid->hid;
 	int wake_status;
 
-	if (device_may_wakeup(&client->dev) && ihid->irq_wake_enabled) {
+	if (!device_may_wakeup(&client->dev)) {
+		if (ihid->pdata.supply) {
+			ret = regulator_enable(ihid->pdata.supply);
+			if (ret < 0)
+				hid_warn(hid, "Failed to enable supply: %d\n",
+					 ret);
+			if (ihid->pdata.init_delay_ms)
+				msleep(ihid->pdata.init_delay_ms);
+		}
+	} else if (ihid->irq_wake_enabled) {
 		wake_status = disable_irq_wake(ihid->irq);
 		if (!wake_status)
 			ihid->irq_wake_enabled = false;
diff --git a/include/linux/i2c/i2c-hid.h b/include/linux/i2c/i2c-hid.h
index 7aa901d92058..97688cde4a91 100644
--- a/include/linux/i2c/i2c-hid.h
+++ b/include/linux/i2c/i2c-hid.h
@@ -14,9 +14,13 @@
 
 #include <linux/types.h>
 
+struct regulator;
+
 /**
  * struct i2chid_platform_data - used by hid over i2c implementation.
  * @hid_descriptor_address: i2c register where the HID descriptor is stored.
+ * @supply: regulator for powering on the device.
+ * @init_delay_ms: delay after powering on before device is usable.
  *
  * Note that it is the responsibility of the platform driver (or the acpi 5.0
  * driver, or the flattened device tree) to setup the irq related to the gpio in
@@ -31,6 +35,8 @@
  */
 struct i2c_hid_platform_data {
 	u16 hid_descriptor_address;
+	struct regulator *supply;
+	int init_delay_ms;
 };
 
 #endif /* __LINUX_I2C_HID_H */
-- 
2.8.0.rc3.226.g39d4020

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

[Benjamin Tissoires]

On Nov 30 2016 or thereabouts, Brian Norris wrote:
> We need to power on the digitizer before using it, and it's also nice to
> save power in suspend by disabling it. Support an optional "vdd-supply"
> and wire it up for the new Wacom device.
> 
> Wacom recommended waiting up to 100ms after powering on before trying to
> access this device.
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> Cc: Jiri Kosina <jikos@kernel.org>
> Cc: linux-input@vger.kernel.org
> ---
> v1 was a few months back. I finally got around to rewriting it based on
> DT binding feedback.
> 
> v2:
>  * support compatible property for wacom, with specific "vdd-supply" name
>  * support the 100ms delay needed for this digitizer
>  * target regulator support only at specific device
> 
>  Documentation/devicetree/bindings/input/hid-over-i2c.txt | 6 +++++-
>  drivers/hid/i2c-hid/i2c-hid.c | 70 ++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/i2c/i2c-hid.h   |  6 ++++
>  2 files changed, 75 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index b3ec4f2de875..1bc174f3a788 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -37,7 +37,9 @@
>  #include <linux/mutex.h>
>  #include <linux/acpi.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/gpio/consumer.h>
> +#include <linux/regulator/consumer.h>
>  
>  #include <linux/i2c/i2c-hid.h>
>  
> @@ -918,10 +920,25 @@ static inline int i2c_hid_acpi_pdata(struct i2c_client *client,
>  #endif
>  
>  #ifdef CONFIG_OF
> +
> +/* of_device_id match data */
> +struct i2c_hid_of_data {
> +	/* Name of supply regulator. */
> +	const char *supply_name;
> +	/* Delay required after powering on device before it is usable. */
> +	int init_delay_ms;
> +};
> +
> +static const struct i2c_hid_of_data wacom_w9013_data = {

Why is the struct "wacom" specific?
If the vdd line is required, I don't see why there is a need to specify
that this is a Wacom specifics. Then Elan, SYnaptics will want the same
and we won't be able to to follow.

> +	.supply_name		= "vdd",
> +	.init_delay_ms		= 100,

If the purpose of this declaration is to set the delay, why isn't this
something provided by the device tree?

> +};
> +
>  static int i2c_hid_of_probe(struct i2c_client *client,
>  		struct i2c_hid_platform_data *pdata)
>  {
>  	struct device *dev = &client->dev;
> +	const struct i2c_hid_of_data *data = of_device_get_match_data(dev);
>  	u32 val;
>  	int ret;
>  
> @@ -937,10 +954,33 @@ static int i2c_hid_of_probe(struct i2c_client *client,
>  	}
>  	pdata->hid_descriptor_address = val;
>  
> +	if (data) {
> +		pdata->init_delay_ms = data->init_delay_ms;
> +		if (data->supply_name) {
> +			pdata->supply = devm_regulator_get_optional(&client->dev,
> +								    data->supply_name);
> +			if (IS_ERR(pdata->supply)) {
> +				ret = PTR_ERR(pdata->supply);
> +				pdata->supply = NULL;
> +				if (ret == -EPROBE_DEFER)
> +					return ret;
> +				if (ret == -ENODEV)
> +					return 0;
> +				dev_err(dev, "Failed to get %s regulator: %d\n",
> +					data->supply_name, ret);
> +				return ret;
> +			}
> +		}
> +	}
> +
>  	return 0;
>  }
>  
>  static const struct of_device_id i2c_hid_of_match[] = {
> +	{
> +		.compatible = "wacom,w9013",
> +		.data = &wacom_w9013_data,
> +	},

NACK, see 1/2

I don't really like the v2. IMO, v1 was less intrusive (though it was
missing the init_delay_ms).
I believe it's possible to have a generic device tree description which
doesn't require us to adapt the driver for each and every device.

Cheers,
Benjamin

>  	{ .compatible = "hid-over-i2c" },
>  	{},
>  };
> @@ -983,6 +1023,17 @@ static int i2c_hid_probe(struct i2c_client *client,
>  		ihid->pdata = *platform_data;
>  	}
>  
> +	if (ihid->pdata.supply) {
> +		ret = regulator_enable(ihid->pdata.supply);
> +		if (ret < 0) {
> +			dev_err(&client->dev, "Failed to enable regulator: %d\n",
> +				ret);
> +			return ret;
> +		}
> +		if (ihid->pdata.init_delay_ms)
> +			msleep(ihid->pdata.init_delay_ms);
> +	}
> +
>  	if (client->irq > 0) {
>  		ihid->irq = client->irq;
>  	} else if (ACPI_COMPANION(&client->dev)) {
> @@ -1100,6 +1151,9 @@ static int i2c_hid_remove(struct i2c_client *client)
>  	if (ihid->desc)
>  		gpiod_put(ihid->desc);
>  
> +	if (ihid->pdata.supply)
> +		regulator_disable(ihid->pdata.supply);
> +
>  	kfree(ihid);
>  
>  	acpi_dev_remove_driver_gpios(ACPI_COMPANION(&client->dev));
> @@ -1152,6 +1206,11 @@ static int i2c_hid_suspend(struct device *dev)
>  		else
>  			hid_warn(hid, "Failed to enable irq wake: %d\n",
>  				wake_status);
> +	} else if (ihid->pdata.supply) {
> +		ret = regulator_disable(ihid->pdata.supply);
> +		if (ret < 0)
> +			hid_warn(hid, "Failed to disable supply: %d\n",
> +				 ret);
>  	}
>  
>  	return 0;
> @@ -1165,7 +1224,16 @@ static int i2c_hid_resume(struct device *dev)
>  	struct hid_device *hid = ihid->hid;
>  	int wake_status;
>  
> -	if (device_may_wakeup(&client->dev) && ihid->irq_wake_enabled) {
> +	if (!device_may_wakeup(&client->dev)) {
> +		if (ihid->pdata.supply) {
> +			ret = regulator_enable(ihid->pdata.supply);
> +			if (ret < 0)
> +				hid_warn(hid, "Failed to enable supply: %d\n",
> +					 ret);
> +			if (ihid->pdata.init_delay_ms)
> +				msleep(ihid->pdata.init_delay_ms);
> +		}
> +	} else if (ihid->irq_wake_enabled) {
>  		wake_status = disable_irq_wake(ihid->irq);
>  		if (!wake_status)
>  			ihid->irq_wake_enabled = false;
> diff --git a/include/linux/i2c/i2c-hid.h b/include/linux/i2c/i2c-hid.h
> index 7aa901d92058..97688cde4a91 100644
> --- a/include/linux/i2c/i2c-hid.h
> +++ b/include/linux/i2c/i2c-hid.h
> @@ -14,9 +14,13 @@
>  
>  #include <linux/types.h>
>  
> +struct regulator;
> +
>  /**
>   * struct i2chid_platform_data - used by hid over i2c implementation.
>   * @hid_descriptor_address: i2c register where the HID descriptor is stored.
> + * @supply: regulator for powering on the device.
> + * @init_delay_ms: delay after powering on before device is usable.
>   *
>   * Note that it is the responsibility of the platform driver (or the acpi 5.0
>   * driver, or the flattened device tree) to setup the irq related to the gpio in
> @@ -31,6 +35,8 @@
>   */
>  struct i2c_hid_platform_data {
>  	u16 hid_descriptor_address;
> +	struct regulator *supply;
> +	int init_delay_ms;
>  };
>  
>  #endif /* __LINUX_I2C_HID_H */
> -- 
> 2.8.0.rc3.226.g39d4020
> 

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

[Brian Norris]

Hi,

On Thu, Dec 01, 2016 at 03:41:26PM +0100, Benjamin Tissoires wrote:
> On Nov 30 2016 or thereabouts, Brian Norris wrote:
> > We need to power on the digitizer before using it, and it's also nice to
> > save power in suspend by disabling it. Support an optional "vdd-supply"
> > and wire it up for the new Wacom device.
> > 
> > Wacom recommended waiting up to 100ms after powering on before trying to
> > access this device.
> > 
> > Signed-off-by: Brian Norris <briannorris@chromium.org>
> > Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> > Cc: Jiri Kosina <jikos@kernel.org>
> > Cc: linux-input@vger.kernel.org
> > ---
> > v1 was a few months back. I finally got around to rewriting it based on
> > DT binding feedback.
> > 
> > v2:
> >  * support compatible property for wacom, with specific "vdd-supply" name
> >  * support the 100ms delay needed for this digitizer
> >  * target regulator support only at specific device
> > 
> >  Documentation/devicetree/bindings/input/hid-over-i2c.txt | 6 +++++-
> >  drivers/hid/i2c-hid/i2c-hid.c | 70 ++++++++++++++++++++++++++++++++++++++++++-
> >  include/linux/i2c/i2c-hid.h   |  6 ++++
> >  2 files changed, 75 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> > index b3ec4f2de875..1bc174f3a788 100644
> > --- a/drivers/hid/i2c-hid/i2c-hid.c
> > +++ b/drivers/hid/i2c-hid/i2c-hid.c
> > @@ -37,7 +37,9 @@
> >  #include <linux/mutex.h>
> >  #include <linux/acpi.h>
> >  #include <linux/of.h>
> > +#include <linux/of_device.h>
> >  #include <linux/gpio/consumer.h>
> > +#include <linux/regulator/consumer.h>
> >  
> >  #include <linux/i2c/i2c-hid.h>
> >  
> > @@ -918,10 +920,25 @@ static inline int i2c_hid_acpi_pdata(struct i2c_client *client,
> >  #endif
> >  
> >  #ifdef CONFIG_OF
> > +
> > +/* of_device_id match data */
> > +struct i2c_hid_of_data {
> > +	/* Name of supply regulator. */
> > +	const char *supply_name;
> > +	/* Delay required after powering on device before it is usable. */
> > +	int init_delay_ms;
> > +};
> > +
> > +static const struct i2c_hid_of_data wacom_w9013_data = {
> 
> Why is the struct "wacom" specific?
> If the vdd line is required, I don't see why there is a need to specify
> that this is a Wacom specifics. Then Elan, SYnaptics will want the same
> and we won't be able to to follow.
> 
> > +	.supply_name		= "vdd",
> > +	.init_delay_ms		= 100,
> 
> If the purpose of this declaration is to set the delay, why isn't this
> something provided by the device tree?

The alleged purpose wasn't just for the delay (it wasn't even mentioned
in v1), but in case there are ever devices with multiple regulators and
different power sequencing (?) I guess. I still don't see why Rob
thought it needed a separate compatible property.

About the delay: as noted in my reply to patch 1, this was actually a
property of the device/firmware -- it needs some time to initialize
before we can talk to it. I previously had included that delay in the
regulator ramp delay, but since we'd know the device info here, I found
it convenient to latch onto the same property.

IOW, typically, if you have a device-specific compatible property, it
makes sense to key device-specific info off of that property where
possible. But if (like you suggest) the compatible property is
counter-productive, then we would need a separate property for this
delay.

> > +};
> > +
> >  static int i2c_hid_of_probe(struct i2c_client *client,
> >  		struct i2c_hid_platform_data *pdata)
> >  {
> >  	struct device *dev = &client->dev;
> > +	const struct i2c_hid_of_data *data = of_device_get_match_data(dev);
> >  	u32 val;
> >  	int ret;
> >  
> > @@ -937,10 +954,33 @@ static int i2c_hid_of_probe(struct i2c_client *client,
> >  	}
> >  	pdata->hid_descriptor_address = val;
> >  
> > +	if (data) {
> > +		pdata->init_delay_ms = data->init_delay_ms;
> > +		if (data->supply_name) {
> > +			pdata->supply = devm_regulator_get_optional(&client->dev,
> > +								    data->supply_name);
> > +			if (IS_ERR(pdata->supply)) {
> > +				ret = PTR_ERR(pdata->supply);
> > +				pdata->supply = NULL;
> > +				if (ret == -EPROBE_DEFER)
> > +					return ret;
> > +				if (ret == -ENODEV)
> > +					return 0;
> > +				dev_err(dev, "Failed to get %s regulator: %d\n",
> > +					data->supply_name, ret);
> > +				return ret;
> > +			}
> > +		}
> > +	}
> > +
> >  	return 0;
> >  }
> >  
> >  static const struct of_device_id i2c_hid_of_match[] = {
> > +	{
> > +		.compatible = "wacom,w9013",
> > +		.data = &wacom_w9013_data,
> > +	},
> 
> NACK, see 1/2

Fine with me, if Rob is OK. I replid on 1/2.

> I don't really like the v2. IMO, v1 was less intrusive (though it was
> missing the init_delay_ms).
> I believe it's possible to have a generic device tree description which
> doesn't require us to adapt the driver for each and every device.

Brian

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

[Benjamin Tissoires]

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.

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

Cheers,
Benjamin

> +
>  Example:
>  
>  	i2c-hid-dev@2c {
> -- 
> 2.8.0.rc3.226.g39d4020
> 

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

[Brian Norris]

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.

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

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

[Rob Herring]

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.

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

Now if you want to make 'hid-over-i2c' a fallback to 'wacom,w9013', I'm 
fine with that.

Rob

[1] https://sietch-tagr.blogspot.de/2016/04/display-panels-are-not-special.html

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

[Dmitry Torokhov]

On Mon, Dec 05, 2016 at 05:59:08PM -0600, 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.
> 
> 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].
> 
> Now if you want to make 'hid-over-i2c' a fallback to 'wacom,w9013', I'm 
> fine with that.

So if I understand it correctly the only change is to have DTS specify

	compatible = "wacom,w9013", "hid-over-i2c";

and no actual changes to the driver itself with regard to the new
compatible string, correct?

I wonder what, besides breaking module autoload, this really buys us?

Thanks.

-- 
Dmitry

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

[Benjamin Tissoires]

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.

I have dealt with that for the wacom modules for years, and this is
definitively not a good solution.

And one additional caveat of this solution is the time between the
release of the new device and its readiness in the hands of the
consumer. You need to push a patch upstream, then backport it or wait
for it to come to your distribution. While if there is a device tree
specific quirk, you just read the spec of the device and applies it to
your device tree and you are good to go.

So no, I don't buy this. If hardware makers want to have fancy way of
initializing their devices, we can cope with those, but I don't want to
do the Device Tree job in a kernel module were you need to recompile it
each time a new device appears.

> 
> Now if you want to make 'hid-over-i2c' a fallback to 'wacom,w9013', I'm 
> fine with that.

I agree to have some sort of quirks in the i2c-hid module, but
definitively not a list of devices with a specific initialization
sequence. Device Tree has also been introduced to remove the specific
platform devices, and you are basically asking us to go back there,
which I don't want.

Cheers,
Benjamin

> 
> Rob
> 
> [1] https://sietch-tagr.blogspot.de/2016/04/display-panels-are-not-special.html

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

[Rob Herring]

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.

> I have dealt with that for the wacom modules for years, and this is
> definitively not a good solution.
>
> And one additional caveat of this solution is the time between the
> release of the new device and its readiness in the hands of the
> consumer. You need to push a patch upstream, then backport it or wait
> for it to come to your distribution. While if there is a device tree
> specific quirk, you just read the spec of the device and applies it to
> your device tree and you are good to go.
>
> So no, I don't buy this. If hardware makers want to have fancy way of
> initializing their devices, we can cope with those, but I don't want to
> do the Device Tree job in a kernel module were you need to recompile it
> each time a new device appears.
>
>>
>> Now if you want to make 'hid-over-i2c' a fallback to 'wacom,w9013', I'm
>> fine with that.
>
> I agree to have some sort of quirks in the i2c-hid module, but
> definitively not a list of devices with a specific initialization
> sequence. Device Tree has also been introduced to remove the specific
> platform devices, and you are basically asking us to go back there,
> which I don't want.

That is not correct. DT does not abstract the h/w to hide
implementation details like ACPI does with AML code. We still have
specific platform drivers and always will. DT is about removing the
board files and describing connections between devices.

Rob

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

[Doug Anderson]

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.

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.

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.

-Doug

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

[Benjamin Tissoires]

On Dec 06 2016 or thereabouts, Doug Anderson 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.
> 
> 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.
> 
> 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.
>

Thanks Doug for this. I had the feeling this wasn't right, but you
actually managed to put the words on it. If it's a board problem (if
you switch the wacom device with an other HID over I2C device and you
still need the same regulator/timing parameters), then this should
simply be mentioned on the patch.

So Brian, could you please respin the series and remve the Wacom
mentions and explain that it is required for the board itself?

Cheers,
Benjamin

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

[Rob Herring]

On Thu, Dec 8, 2016 at 9:41 AM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> On Dec 06 2016 or thereabouts, Doug Anderson 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.
>>
>> 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.
>>
>> 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.
>>
>
> Thanks Doug for this. I had the feeling this wasn't right, but you
> actually managed to put the words on it. If it's a board problem (if
> you switch the wacom device with an other HID over I2C device and you
> still need the same regulator/timing parameters), then this should
> simply be mentioned on the patch.
>
> So Brian, could you please respin the series and remve the Wacom
> mentions and explain that it is required for the board itself?

In advance, NAK.

This is not how DT works. Either this binding needs a Wacom compatible
or don't use DT.

Rob

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

[Dmitry Torokhov]

On December 8, 2016 8:03:06 AM PST, Rob Herring <robh@kernel.org> wrote:
>On Thu, Dec 8, 2016 at 9:41 AM, Benjamin Tissoires
><benjamin.tissoires@redhat.com> wrote:
>> On Dec 06 2016 or thereabouts, Doug Anderson 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.
>>>
>>> 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.
>>>
>>> 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.
>>>
>>
>> Thanks Doug for this. I had the feeling this wasn't right, but you
>> actually managed to put the words on it. If it's a board problem (if
>> you switch the wacom device with an other HID over I2C device and you
>> still need the same regulator/timing parameters), then this should
>> simply be mentioned on the patch.
>>
>> So Brian, could you please respin the series and remve the Wacom
>> mentions and explain that it is required for the board itself?
>
>In advance, NAK.
>
>This is not how DT works. Either this binding needs a Wacom compatible
>or don't use DT.
>

And if tomorrow there is Elan device that is drop-in compatible (same connector, etc) with Wacom i2c-hid, will you ask for Elan-specific binding? Atmel? Weida? They all need to be powered up ultimately.


Thanks.

-- 
Dmitry

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

[Rob Herring]

On Thu, Dec 8, 2016 at 10:13 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On December 8, 2016 8:03:06 AM PST, Rob Herring <robh@kernel.org> wrote:
>>On Thu, Dec 8, 2016 at 9:41 AM, Benjamin Tissoires
>><benjamin.tissoires@redhat.com> wrote:
>>> On Dec 06 2016 or thereabouts, Doug Anderson 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.
>>>>
>>>> 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.
>>>>
>>>> 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.
>>>>
>>>
>>> Thanks Doug for this. I had the feeling this wasn't right, but you
>>> actually managed to put the words on it. If it's a board problem (if
>>> you switch the wacom device with an other HID over I2C device and you
>>> still need the same regulator/timing parameters), then this should
>>> simply be mentioned on the patch.
>>>
>>> So Brian, could you please respin the series and remve the Wacom
>>> mentions and explain that it is required for the board itself?
>>
>>In advance, NAK.
>>
>>This is not how DT works. Either this binding needs a Wacom compatible
>>or don't use DT.
>>
>
> And if tomorrow there is Elan device that is drop-in compatible (same connector, etc) with Wacom i2c-hid, will you ask for Elan-specific binding? Atmel? Weida? They all need to be powered up ultimately.

Yes, I will. Anyone who's worked on drop-in or pin compatible parts
knows there's no such thing.

That in no way means the OS driver has to know about each and every
one. If they can all claim compatibility with Wacom (including power
control), then they can have a Wacom compatible string too. Or you can
just never tell me that there's a different manufacturer and I won't
care as long you don't need different control. But soon as a device
needs another power rail, GPIO or different timing, then you'd better
have a new compatible string.

Rob

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

[Dmitry Torokhov]

On Thu, Dec 08, 2016 at 10:26:41AM -0600, Rob Herring wrote:
> On Thu, Dec 8, 2016 at 10:13 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On December 8, 2016 8:03:06 AM PST, Rob Herring <robh@kernel.org> wrote:
> >>On Thu, Dec 8, 2016 at 9:41 AM, Benjamin Tissoires
> >><benjamin.tissoires@redhat.com> wrote:
> >>> On Dec 06 2016 or thereabouts, Doug Anderson 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.
> >>>>
> >>>> 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.
> >>>>
> >>>> 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.
> >>>>
> >>>
> >>> Thanks Doug for this. I had the feeling this wasn't right, but you
> >>> actually managed to put the words on it. If it's a board problem (if
> >>> you switch the wacom device with an other HID over I2C device and you
> >>> still need the same regulator/timing parameters), then this should
> >>> simply be mentioned on the patch.
> >>>
> >>> So Brian, could you please respin the series and remve the Wacom
> >>> mentions and explain that it is required for the board itself?
> >>
> >>In advance, NAK.
> >>
> >>This is not how DT works. Either this binding needs a Wacom compatible
> >>or don't use DT.
> >>
> >
> > And if tomorrow there is Elan device that is drop-in compatible
> > (same connector, etc) with Wacom i2c-hid, will you ask for
> > Elan-specific binding? Atmel? Weida? They all need to be powered up
> > ultimately.
> 
> Yes, I will. Anyone who's worked on drop-in or pin compatible parts
> knows there's no such thing.

And yet we are shipping quite a few of Chromebooks with touchpads that
are dual-sourced and can be exchanged at any time without any changes to
the software (be it kernel or firmware).

> 
> That in no way means the OS driver has to know about each and every
> one. If they can all claim compatibility with Wacom (including power
> control), then they can have a Wacom compatible string too. Or you can
> just never tell me that there's a different manufacturer and I won't
> care as long you don't need different control. But soon as a device
> needs another power rail, GPIO or different timing, then you'd better
> have a new compatible string.

That I simply do not understand. We routinely enhance bindings because
the devices get used on different boards that expose more or less
connections. I.e. quite often we start with a device whose rails are
controlled by the firmware, and so the binding only contains register
and interrupt data. Then we come across board that exposes reset GPIO
and we add that to the binding (bit we do not invent new compatible
string just because there is GPIO now). Then we get a board that
actually wants kernel to control power to the chip and we add a
regulator. Non-optional, mind you, because we rely on the regulator
system to give us a dummy one if it is not described by the
firmware/other means. And then we get another board that exposes another
power rail (let's say 3.3V to the panel whereas the previous one did not
use it). And we add another regulator binding. All this time we have
the same compatibility string.

So in this case we finally got to the point where we admit that devices
speaking HID over I2C do have power rails; we simply did not need to
control them before. The same Wacom digitizer, that you now demand to
add a compatible for, may have been used in other boards where power
rails were either turned on by the firmware at boot time and left on
until the board is shutdown, or ACPI was controlling them (via _ON/_OFF
methods), or there was some other magic. Having a supply and ability to
control the time it takes to bring the device into operating state is in
no way Wacom-specific, so why new compatibility instead of enhancing
the current binding?

And on top of that, currently multiple compatible strings are utterly
broken with regard to module loading (you only emit modalias for the
first component), so having DTS with multiples does not work well in
real life.

-- 
Dmitry

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

[Rob Herring]

On Thu, Dec 8, 2016 at 12:12 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Thu, Dec 08, 2016 at 10:26:41AM -0600, Rob Herring wrote:
>> On Thu, Dec 8, 2016 at 10:13 AM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>> > On December 8, 2016 8:03:06 AM PST, Rob Herring <robh@kernel.org> wrote:
>> >>On Thu, Dec 8, 2016 at 9:41 AM, Benjamin Tissoires
>> >><benjamin.tissoires@redhat.com> wrote:
>> >>> On Dec 06 2016 or thereabouts, Doug Anderson wrote:

[...]

>> >>>> 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.
>> >>>>
>> >>>> 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.
>> >>>>
>> >>>
>> >>> Thanks Doug for this. I had the feeling this wasn't right, but you
>> >>> actually managed to put the words on it. If it's a board problem (if
>> >>> you switch the wacom device with an other HID over I2C device and you
>> >>> still need the same regulator/timing parameters), then this should
>> >>> simply be mentioned on the patch.
>> >>>
>> >>> So Brian, could you please respin the series and remve the Wacom
>> >>> mentions and explain that it is required for the board itself?
>> >>
>> >>In advance, NAK.
>> >>
>> >>This is not how DT works. Either this binding needs a Wacom compatible
>> >>or don't use DT.
>> >>
>> >
>> > And if tomorrow there is Elan device that is drop-in compatible
>> > (same connector, etc) with Wacom i2c-hid, will you ask for
>> > Elan-specific binding? Atmel? Weida? They all need to be powered up
>> > ultimately.
>>
>> Yes, I will. Anyone who's worked on drop-in or pin compatible parts
>> knows there's no such thing.
>
> And yet we are shipping quite a few of Chromebooks with touchpads that
> are dual-sourced and can be exchanged at any time without any changes to
> the software (be it kernel or firmware).
>
>>
>> That in no way means the OS driver has to know about each and every
>> one. If they can all claim compatibility with Wacom (including power
>> control), then they can have a Wacom compatible string too. Or you can
>> just never tell me that there's a different manufacturer and I won't
>> care as long you don't need different control. But soon as a device
>> needs another power rail, GPIO or different timing, then you'd better
>> have a new compatible string.
>
> That I simply do not understand. We routinely enhance bindings because
> the devices get used on different boards that expose more or less
> connections. I.e. quite often we start with a device whose rails are
> controlled by the firmware, and so the binding only contains register
> and interrupt data. Then we come across board that exposes reset GPIO
> and we add that to the binding (bit we do not invent new compatible
> string just because there is GPIO now). Then we get a board that
> actually wants kernel to control power to the chip and we add a
> regulator. Non-optional, mind you, because we rely on the regulator
> system to give us a dummy one if it is not described by the
> firmware/other means. And then we get another board that exposes another
> power rail (let's say 3.3V to the panel whereas the previous one did not
> use it). And we add another regulator binding. All this time we have
> the same compatibility string.

All this I agree with, but it almost always starts with a compatible
that matches the device, not a compatible for the protocol. When
adding control for different devices diverge, then they need different
compatibles. Ideally, we'd start with all the supplies and control
lines defined for a binding, so we don't have so much evolving of
bindings. But obviously we can't always get things 100% complete to
start with, so we have to let them evolve. And I can't go read every
datasheet, but I do go read them sometimes for reviews.

> So in this case we finally got to the point where we admit that devices
> speaking HID over I2C do have power rails; we simply did not need to
> control them before. The same Wacom digitizer, that you now demand to
> add a compatible for, may have been used in other boards where power
> rails were either turned on by the firmware at boot time and left on
> until the board is shutdown, or ACPI was controlling them (via _ON/_OFF
> methods), or there was some other magic. Having a supply and ability to
> control the time it takes to bring the device into operating state is in
> no way Wacom-specific, so why new compatibility instead of enhancing
> the current binding?

A new compatible is enhancing the binding IMO. I'm just saying you
need both the compatible and the added properties. Whether the power
on delay should be a property or implied by the compatible is somewhat
debatable, but I'm okay with having it because we already do on other
bindings. What I'm not okay with is "simple" or "generic" bindings
that continually evolve with additional properties to handle more and
more complex cases. DT is not a scripting language. This has been
discussed countless times before...

> And on top of that, currently multiple compatible strings are utterly
> broken with regard to module loading (you only emit modalias for the
> first component), so having DTS with multiples does not work well in
> real life.

Sounds like this should be a fixable problem.

Rob

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

[Jiri Kosina]

On Thu, 8 Dec 2016, Rob Herring wrote:

> > And if tomorrow there is Elan device that is drop-in compatible (same 
> > connector, etc) with Wacom i2c-hid, will you ask for Elan-specific 
> > binding? Atmel? Weida? They all need to be powered up ultimately.
> 
> Yes, I will. 

What advantage does that bring?

> That in no way means the OS driver has to know about each and every one. 
> If they can all claim compatibility with Wacom (including power 
> control), then they can have a Wacom compatible string too. Or you can 
> just never tell me that there's a different manufacturer and I won't 
> care as long you don't need different control. But soon as a device 
> needs another power rail, GPIO or different timing, then you'd better 
> have a new compatible string.

Again, I simply don't understand what advantage does the aproach you are 
trying to use bring.

HID over I2C is a generic protocol. Sure, we need to have quirks for 
device-specific bugs, and in such cases enumerate particular devices. But 
we don't need DT for that at all.

-- 
Jiri Kosina
SUSE Labs

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

[Rob Herring]

On Fri, Dec 9, 2016 at 8:36 AM, Jiri Kosina <jikos@kernel.org> wrote:
> On Thu, 8 Dec 2016, Rob Herring wrote:
>
>> > And if tomorrow there is Elan device that is drop-in compatible (same
>> > connector, etc) with Wacom i2c-hid, will you ask for Elan-specific
>> > binding? Atmel? Weida? They all need to be powered up ultimately.
>>
>> Yes, I will.
>
> What advantage does that bring?
>
>> That in no way means the OS driver has to know about each and every one.
>> If they can all claim compatibility with Wacom (including power
>> control), then they can have a Wacom compatible string too. Or you can
>> just never tell me that there's a different manufacturer and I won't
>> care as long you don't need different control. But soon as a device
>> needs another power rail, GPIO or different timing, then you'd better
>> have a new compatible string.
>
> Again, I simply don't understand what advantage does the aproach you are
> trying to use bring.

This is simply how DT works. HID-over-I2C devices are no different
than any other I2C device or any other component. You are not special.

> HID over I2C is a generic protocol.

DT describes h/w, not protocols.

> Sure, we need to have quirks for
> device-specific bugs, and in such cases enumerate particular devices. But
> we don't need DT for that at all.

When it is related to powering on the device you may need to know the
specific device in DT. Compatibles are like VID/PID for devices, a
unique identifier for the specific device. Having that does not to
prevent generic/common drivers. The same rules apply. You wouldn't
want different devices having the same VID/PID (surely that has never
happened) or only change a VID/PID when you find a bug. The same rules
apply for compatible strings. You should be able to apply quirks
without changing/adding compatible strings (or more generally, without
changing the dtb). Just like you wouldn't change a VID/PID for a
device only when you find a bug, you can't add/change a DT compatible.

BTW, As you do have a VID/PID, you could define a compatible string
syntax using VID/PID like is done for PCI and USB.

Rob

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

[Doug Anderson]

Hi,

On Fri, Dec 9, 2016 at 7:01 AM, Rob Herring <robh@kernel.org> wrote:
> On Fri, Dec 9, 2016 at 8:36 AM, Jiri Kosina <jikos@kernel.org> wrote:
>> On Thu, 8 Dec 2016, Rob Herring wrote:
>>
>>> > And if tomorrow there is Elan device that is drop-in compatible (same
>>> > connector, etc) with Wacom i2c-hid, will you ask for Elan-specific
>>> > binding? Atmel? Weida? They all need to be powered up ultimately.
>>>
>>> Yes, I will.
>>
>> What advantage does that bring?
>>
>>> That in no way means the OS driver has to know about each and every one.
>>> If they can all claim compatibility with Wacom (including power
>>> control), then they can have a Wacom compatible string too. Or you can
>>> just never tell me that there's a different manufacturer and I won't
>>> care as long you don't need different control. But soon as a device
>>> needs another power rail, GPIO or different timing, then you'd better
>>> have a new compatible string.
>>
>> Again, I simply don't understand what advantage does the aproach you are
>> trying to use bring.
>
> This is simply how DT works. HID-over-I2C devices are no different
> than any other I2C device or any other component. You are not special.

...but once you say that it's HID over I2C then it becomes probe-able,
right?  Said another way: we need to specify just enough to power the
device up properly and then we can ask it what kind of device it is
and then we can make quirk decisions based on that.

So specifying what kind of device it is in the device tree is somewhat
redundant and it also means that you make it needlessly difficult to
build a system with dual-sourced components.

One of the major points of probe-able connections is that you could
put anything you want there and the kernel _doesn't_ need to describe
it.  ...and the whole point of device tree (I thought) was to
specifically handle connections that _aren't_ probe-able.

For instance, if a board has a USB bus on it but you need to assert a
special reset or turn on a special regulator (besides vbus) before you
can probe the USB bus, you wouldn't think that the board should
specify exactly what device was stuffed in the connection, would you?


>> HID over I2C is a generic protocol.
>
> DT describes h/w, not protocols.

Isn't it a little of each, though?  When you say that there's a USB
port or an SDIO port or a serial port on the board, you're saying more
than just that there are 2, 4, or 8 wires coming out of the board.
You're saying that you're expecting to talk a certain protocol over
those wires.

-Doug

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

[Jiri Kosina]

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

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

[Benjamin Tissoires]

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

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

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

[Brian Norris]

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.

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

[Rob Herring]

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

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

[Rob Herring]

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

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

[Doug Anderson]

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

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

[Rob Herring]

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

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

[Rob Herring]

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>

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

[Brian Norris]

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