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

Groups > linux.kernel > #1533729 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

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

Page 1 of 2  [1] 2  Next page →

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

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

[toc] | [next] | [standalone]

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

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

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

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

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
> 

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

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

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

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

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

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
> 

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

#1538671

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web