linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/2] HID: i2c-hid: add reset gpio property
@ 2017-10-31  3:03 Lin Huang
  2017-10-31  3:03 ` [PATCH v2 2/2] devicetree: i2c-hid: Add reset property Lin Huang
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Lin Huang @ 2017-10-31  3:03 UTC (permalink / raw)
  To: dmitry.torokhov, jikos, robh
  Cc: benjamin.tissoires, jani.nikula, linux-input, linux-kernel,
	briannorris, devicetree, Lin Huang

some i2c hid devices have reset gpio, need to control
it in the driver.

Signed-off-by: Lin Huang <hl@rock-chips.com>
---
Changes in v2:
- Add 10us in usleep_range() upper range
- reuse post_power_delay_ms as deassert reset delay
- delete deassert_reset_us property

 drivers/hid/i2c-hid/i2c-hid.c         | 61 +++++++++++++++++++++++++++--------
 include/linux/platform_data/i2c-hid.h |  3 ++
 2 files changed, 50 insertions(+), 14 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 9145c21..9f06135 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -38,6 +38,7 @@
 #include <linux/mutex.h>
 #include <linux/acpi.h>
 #include <linux/of.h>
+#include <linux/gpio/consumer.h>
 #include <linux/regulator/consumer.h>
 
 #include <linux/platform_data/i2c-hid.h>
@@ -793,6 +794,35 @@ struct hid_ll_driver i2c_hid_ll_driver = {
 };
 EXPORT_SYMBOL_GPL(i2c_hid_ll_driver);
 
+static int i2c_hid_hw_power_on(struct i2c_hid *ihid)
+{
+	int ret;
+
+	ret = regulator_enable(ihid->pdata.supply);
+	if (ret < 0)
+		return ret;
+
+	gpiod_set_value_cansleep(ihid->pdata.reset_gpio, 1);
+	if (ihid->pdata.assert_reset_us)
+		usleep_range(ihid->pdata.assert_reset_us,
+			     ihid->pdata.assert_reset_us + 10);
+
+	gpiod_set_value_cansleep(ihid->pdata.reset_gpio, 0);
+
+	/* use for power supply delay or reset deassert delay */
+	if (ihid->pdata.post_power_delay_ms)
+		msleep(ihid->pdata.post_power_delay_ms);
+
+	return ret;
+}
+
+static int i2c_hid_hw_power_off(struct i2c_hid *ihid)
+{
+	gpiod_set_value_cansleep(ihid->pdata.reset_gpio, 1);
+
+	return regulator_disable(ihid->pdata.supply);
+}
+
 static int i2c_hid_init_irq(struct i2c_client *client)
 {
 	struct i2c_hid *ihid = i2c_get_clientdata(client);
@@ -934,6 +964,9 @@ static int i2c_hid_of_probe(struct i2c_client *client,
 	if (!ret)
 		pdata->post_power_delay_ms = val;
 
+	of_property_read_u32(dev->of_node, "assert-reset-us",
+			     &pdata->assert_reset_us);
+
 	return 0;
 }
 
@@ -1002,14 +1035,16 @@ static int i2c_hid_probe(struct i2c_client *client,
 		goto err;
 	}
 
-	ret = regulator_enable(ihid->pdata.supply);
+	ihid->pdata.reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
+							 GPIOD_OUT_HIGH);
+	if (IS_ERR(ihid->pdata.reset_gpio))
+		goto err;
+
+	ret = i2c_hid_hw_power_on(ihid);
 	if (ret < 0) {
-		dev_err(&client->dev, "Failed to enable regulator: %d\n",
-			ret);
+		dev_err(&client->dev, "Failed to power on: %d\n", ret);
 		goto err;
 	}
-	if (ihid->pdata.post_power_delay_ms)
-		msleep(ihid->pdata.post_power_delay_ms);
 
 	i2c_set_clientdata(client, ihid);
 
@@ -1086,7 +1121,7 @@ static int i2c_hid_probe(struct i2c_client *client,
 	pm_runtime_disable(&client->dev);
 
 err_regulator:
-	regulator_disable(ihid->pdata.supply);
+	i2c_hid_hw_power_off(ihid);
 
 err:
 	i2c_hid_free_buffers(ihid);
@@ -1112,8 +1147,7 @@ static int i2c_hid_remove(struct i2c_client *client)
 	if (ihid->bufsize)
 		i2c_hid_free_buffers(ihid);
 
-	regulator_disable(ihid->pdata.supply);
-
+	i2c_hid_hw_power_off(ihid);
 	kfree(ihid);
 
 	return 0;
@@ -1165,9 +1199,10 @@ static int i2c_hid_suspend(struct device *dev)
 			hid_warn(hid, "Failed to enable irq wake: %d\n",
 				wake_status);
 	} else {
-		ret = regulator_disable(ihid->pdata.supply);
+		ret = i2c_hid_hw_power_off(ihid);
 		if (ret < 0)
-			hid_warn(hid, "Failed to disable supply: %d\n", ret);
+			hid_warn(hid, "Failed to disable hw power: %d\n",
+				 ret);
 	}
 
 	return 0;
@@ -1182,11 +1217,9 @@ static int i2c_hid_resume(struct device *dev)
 	int wake_status;
 
 	if (!device_may_wakeup(&client->dev)) {
-		ret = regulator_enable(ihid->pdata.supply);
+		ret = i2c_hid_hw_power_on(ihid);
 		if (ret < 0)
-			hid_warn(hid, "Failed to enable supply: %d\n", ret);
-		if (ihid->pdata.post_power_delay_ms)
-			msleep(ihid->pdata.post_power_delay_ms);
+			hid_warn(hid, "Failed to enable hw power: %d\n", ret);
 	} else if (ihid->irq_wake_enabled) {
 		wake_status = disable_irq_wake(client->irq);
 		if (!wake_status)
diff --git a/include/linux/platform_data/i2c-hid.h b/include/linux/platform_data/i2c-hid.h
index 1fb0882..40f1840 100644
--- a/include/linux/platform_data/i2c-hid.h
+++ b/include/linux/platform_data/i2c-hid.h
@@ -14,6 +14,7 @@
 
 #include <linux/types.h>
 
+struct gpio_desc;
 struct regulator;
 
 /**
@@ -37,6 +38,8 @@ struct i2c_hid_platform_data {
 	u16 hid_descriptor_address;
 	struct regulator *supply;
 	int post_power_delay_ms;
+	struct gpio_desc *reset_gpio;
+	int assert_reset_us;
 };
 
 #endif /* __LINUX_I2C_HID_H */
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 2/2] devicetree: i2c-hid: Add reset property
  2017-10-31  3:03 [PATCH v2 1/2] HID: i2c-hid: add reset gpio property Lin Huang
@ 2017-10-31  3:03 ` Lin Huang
  2017-11-01 22:02   ` Rob Herring
  2017-11-04  4:35   ` Brian Norris
  2017-10-31  8:47 ` [PATCH v2 1/2] HID: i2c-hid: add reset gpio property Jani Nikula
  2017-11-04  2:29 ` Brian Norris
  2 siblings, 2 replies; 9+ messages in thread
From: Lin Huang @ 2017-10-31  3:03 UTC (permalink / raw)
  To: dmitry.torokhov, jikos, robh
  Cc: benjamin.tissoires, jani.nikula, linux-input, linux-kernel,
	briannorris, devicetree, Lin Huang

Document a "reset" and "assert-reset-us", it can be used for
driver control reset property. And reuse post-power-on-delay-ms
for deassert reset delay.

Signed-off-by: Lin Huang <hl@rock-chips.com>
---
 Documentation/devicetree/bindings/input/hid-over-i2c.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.txt b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
index 28e8bd8..6ab0eed 100644
--- a/Documentation/devicetree/bindings/input/hid-over-i2c.txt
+++ b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
@@ -31,7 +31,9 @@ device-specific compatible properties, which should be used in addition to the
 
 - vdd-supply: phandle of the regulator that provides the supply voltage.
 - post-power-on-delay-ms: time required by the device after enabling its regulators
-  before it is ready for communication. Must be used with 'vdd-supply'.
+  or deassert reset pin before it is ready for communication.
+- reset: phandle of the gpio that provides for hid reset pin.
+- assert-reset-us: the device require reset assert time.
 
 Example:
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/2] HID: i2c-hid: add reset gpio property
  2017-10-31  3:03 [PATCH v2 1/2] HID: i2c-hid: add reset gpio property Lin Huang
  2017-10-31  3:03 ` [PATCH v2 2/2] devicetree: i2c-hid: Add reset property Lin Huang
@ 2017-10-31  8:47 ` Jani Nikula
  2017-11-04  2:29 ` Brian Norris
  2 siblings, 0 replies; 9+ messages in thread
From: Jani Nikula @ 2017-10-31  8:47 UTC (permalink / raw)
  To: Lin Huang, dmitry.torokhov, jikos, robh
  Cc: benjamin.tissoires, linux-input, linux-kernel, briannorris,
	devicetree, Lin Huang

On Tue, 31 Oct 2017, Lin Huang <hl@rock-chips.com> wrote:
> some i2c hid devices have reset gpio, need to control
> it in the driver.

So I have Reviewed-by and Acked-by on *one* commit touching the files
being changed here. get_maintainer.pl is clueless, and you shouldn't use
its output verbatim.

BR,
Jani.

> Signed-off-by: Lin Huang <hl@rock-chips.com>
> ---
> Changes in v2:
> - Add 10us in usleep_range() upper range
> - reuse post_power_delay_ms as deassert reset delay
> - delete deassert_reset_us property
>
>  drivers/hid/i2c-hid/i2c-hid.c         | 61 +++++++++++++++++++++++++++--------
>  include/linux/platform_data/i2c-hid.h |  3 ++
>  2 files changed, 50 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 9145c21..9f06135 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -38,6 +38,7 @@
>  #include <linux/mutex.h>
>  #include <linux/acpi.h>
>  #include <linux/of.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/regulator/consumer.h>
>  
>  #include <linux/platform_data/i2c-hid.h>
> @@ -793,6 +794,35 @@ struct hid_ll_driver i2c_hid_ll_driver = {
>  };
>  EXPORT_SYMBOL_GPL(i2c_hid_ll_driver);
>  
> +static int i2c_hid_hw_power_on(struct i2c_hid *ihid)
> +{
> +	int ret;
> +
> +	ret = regulator_enable(ihid->pdata.supply);
> +	if (ret < 0)
> +		return ret;
> +
> +	gpiod_set_value_cansleep(ihid->pdata.reset_gpio, 1);
> +	if (ihid->pdata.assert_reset_us)
> +		usleep_range(ihid->pdata.assert_reset_us,
> +			     ihid->pdata.assert_reset_us + 10);
> +
> +	gpiod_set_value_cansleep(ihid->pdata.reset_gpio, 0);
> +
> +	/* use for power supply delay or reset deassert delay */
> +	if (ihid->pdata.post_power_delay_ms)
> +		msleep(ihid->pdata.post_power_delay_ms);
> +
> +	return ret;
> +}
> +
> +static int i2c_hid_hw_power_off(struct i2c_hid *ihid)
> +{
> +	gpiod_set_value_cansleep(ihid->pdata.reset_gpio, 1);
> +
> +	return regulator_disable(ihid->pdata.supply);
> +}
> +
>  static int i2c_hid_init_irq(struct i2c_client *client)
>  {
>  	struct i2c_hid *ihid = i2c_get_clientdata(client);
> @@ -934,6 +964,9 @@ static int i2c_hid_of_probe(struct i2c_client *client,
>  	if (!ret)
>  		pdata->post_power_delay_ms = val;
>  
> +	of_property_read_u32(dev->of_node, "assert-reset-us",
> +			     &pdata->assert_reset_us);
> +
>  	return 0;
>  }
>  
> @@ -1002,14 +1035,16 @@ static int i2c_hid_probe(struct i2c_client *client,
>  		goto err;
>  	}
>  
> -	ret = regulator_enable(ihid->pdata.supply);
> +	ihid->pdata.reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> +							 GPIOD_OUT_HIGH);
> +	if (IS_ERR(ihid->pdata.reset_gpio))
> +		goto err;
> +
> +	ret = i2c_hid_hw_power_on(ihid);
>  	if (ret < 0) {
> -		dev_err(&client->dev, "Failed to enable regulator: %d\n",
> -			ret);
> +		dev_err(&client->dev, "Failed to power on: %d\n", ret);
>  		goto err;
>  	}
> -	if (ihid->pdata.post_power_delay_ms)
> -		msleep(ihid->pdata.post_power_delay_ms);
>  
>  	i2c_set_clientdata(client, ihid);
>  
> @@ -1086,7 +1121,7 @@ static int i2c_hid_probe(struct i2c_client *client,
>  	pm_runtime_disable(&client->dev);
>  
>  err_regulator:
> -	regulator_disable(ihid->pdata.supply);
> +	i2c_hid_hw_power_off(ihid);
>  
>  err:
>  	i2c_hid_free_buffers(ihid);
> @@ -1112,8 +1147,7 @@ static int i2c_hid_remove(struct i2c_client *client)
>  	if (ihid->bufsize)
>  		i2c_hid_free_buffers(ihid);
>  
> -	regulator_disable(ihid->pdata.supply);
> -
> +	i2c_hid_hw_power_off(ihid);
>  	kfree(ihid);
>  
>  	return 0;
> @@ -1165,9 +1199,10 @@ static int i2c_hid_suspend(struct device *dev)
>  			hid_warn(hid, "Failed to enable irq wake: %d\n",
>  				wake_status);
>  	} else {
> -		ret = regulator_disable(ihid->pdata.supply);
> +		ret = i2c_hid_hw_power_off(ihid);
>  		if (ret < 0)
> -			hid_warn(hid, "Failed to disable supply: %d\n", ret);
> +			hid_warn(hid, "Failed to disable hw power: %d\n",
> +				 ret);
>  	}
>  
>  	return 0;
> @@ -1182,11 +1217,9 @@ static int i2c_hid_resume(struct device *dev)
>  	int wake_status;
>  
>  	if (!device_may_wakeup(&client->dev)) {
> -		ret = regulator_enable(ihid->pdata.supply);
> +		ret = i2c_hid_hw_power_on(ihid);
>  		if (ret < 0)
> -			hid_warn(hid, "Failed to enable supply: %d\n", ret);
> -		if (ihid->pdata.post_power_delay_ms)
> -			msleep(ihid->pdata.post_power_delay_ms);
> +			hid_warn(hid, "Failed to enable hw power: %d\n", ret);
>  	} else if (ihid->irq_wake_enabled) {
>  		wake_status = disable_irq_wake(client->irq);
>  		if (!wake_status)
> diff --git a/include/linux/platform_data/i2c-hid.h b/include/linux/platform_data/i2c-hid.h
> index 1fb0882..40f1840 100644
> --- a/include/linux/platform_data/i2c-hid.h
> +++ b/include/linux/platform_data/i2c-hid.h
> @@ -14,6 +14,7 @@
>  
>  #include <linux/types.h>
>  
> +struct gpio_desc;
>  struct regulator;
>  
>  /**
> @@ -37,6 +38,8 @@ struct i2c_hid_platform_data {
>  	u16 hid_descriptor_address;
>  	struct regulator *supply;
>  	int post_power_delay_ms;
> +	struct gpio_desc *reset_gpio;
> +	int assert_reset_us;
>  };
>  
>  #endif /* __LINUX_I2C_HID_H */

-- 
Jani Nikula, Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] devicetree: i2c-hid: Add reset property
  2017-10-31  3:03 ` [PATCH v2 2/2] devicetree: i2c-hid: Add reset property Lin Huang
@ 2017-11-01 22:02   ` Rob Herring
  2017-11-06  9:06     ` Benjamin Tissoires
  2017-11-04  4:35   ` Brian Norris
  1 sibling, 1 reply; 9+ messages in thread
From: Rob Herring @ 2017-11-01 22:02 UTC (permalink / raw)
  To: Lin Huang
  Cc: dmitry.torokhov, jikos, benjamin.tissoires, jani.nikula,
	linux-input, linux-kernel, briannorris, devicetree

On Tue, Oct 31, 2017 at 11:03:16AM +0800, Lin Huang wrote:
> Document a "reset" and "assert-reset-us", it can be used for
> driver control reset property. And reuse post-power-on-delay-ms
> for deassert reset delay.

"dt-bindings: input: " for the subject please.

> 
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> ---
>  Documentation/devicetree/bindings/input/hid-over-i2c.txt | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.txt b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> index 28e8bd8..6ab0eed 100644
> --- a/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> +++ b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> @@ -31,7 +31,9 @@ device-specific compatible properties, which should be used in addition to the
>  
>  - vdd-supply: phandle of the regulator that provides the supply voltage.
>  - post-power-on-delay-ms: time required by the device after enabling its regulators
> -  before it is ready for communication. Must be used with 'vdd-supply'.
> +  or deassert reset pin before it is ready for communication.
> +- reset: phandle of the gpio that provides for hid reset pin.

The kernel api takes "reset", but the property is "reset-gpios".

And it's not just a phandle typically.

> +- assert-reset-us: the device require reset assert time.
>  
>  Example:
>  
> -- 
> 2.7.4
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/2] HID: i2c-hid: add reset gpio property
  2017-10-31  3:03 [PATCH v2 1/2] HID: i2c-hid: add reset gpio property Lin Huang
  2017-10-31  3:03 ` [PATCH v2 2/2] devicetree: i2c-hid: Add reset property Lin Huang
  2017-10-31  8:47 ` [PATCH v2 1/2] HID: i2c-hid: add reset gpio property Jani Nikula
@ 2017-11-04  2:29 ` Brian Norris
  2 siblings, 0 replies; 9+ messages in thread
From: Brian Norris @ 2017-11-04  2:29 UTC (permalink / raw)
  To: Lin Huang
  Cc: dmitry.torokhov, jikos, robh, benjamin.tissoires, linux-input,
	linux-kernel, devicetree

On Tue, Oct 31, 2017 at 11:03:15AM +0800, Lin Huang wrote:
> some i2c hid devices have reset gpio, need to control
> it in the driver.
> 
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> ---
> Changes in v2:
> - Add 10us in usleep_range() upper range
> - reuse post_power_delay_ms as deassert reset delay
> - delete deassert_reset_us property
> 
>  drivers/hid/i2c-hid/i2c-hid.c         | 61 +++++++++++++++++++++++++++--------
>  include/linux/platform_data/i2c-hid.h |  3 ++
>  2 files changed, 50 insertions(+), 14 deletions(-)

Reviewed-by: Brian Norris <briannorris@chromium.org>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] devicetree: i2c-hid: Add reset property
  2017-10-31  3:03 ` [PATCH v2 2/2] devicetree: i2c-hid: Add reset property Lin Huang
  2017-11-01 22:02   ` Rob Herring
@ 2017-11-04  4:35   ` Brian Norris
  2017-11-06  1:00     ` hl
  1 sibling, 1 reply; 9+ messages in thread
From: Brian Norris @ 2017-11-04  4:35 UTC (permalink / raw)
  To: Lin Huang
  Cc: Dmitry Torokhov, jikos, robh, benjamin.tissoires, linux-input,
	linux-kernel, devicetree

On Mon, Oct 30, 2017 at 8:03 PM, Lin Huang <hl@rock-chips.com> wrote:
> Document a "reset" and "assert-reset-us", it can be used for
> driver control reset property. And reuse post-power-on-delay-ms
> for deassert reset delay.
>
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> ---
>  Documentation/devicetree/bindings/input/hid-over-i2c.txt | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.txt b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> index 28e8bd8..6ab0eed 100644
> --- a/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> +++ b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> @@ -31,7 +31,9 @@ device-specific compatible properties, which should be used in addition to the
>
>  - vdd-supply: phandle of the regulator that provides the supply voltage.
>  - post-power-on-delay-ms: time required by the device after enabling its regulators
> -  before it is ready for communication. Must be used with 'vdd-supply'.
> +  or deassert reset pin before it is ready for communication.
> +- reset: phandle of the gpio that provides for hid reset pin.
> +- assert-reset-us: the device require reset assert time.

If there was any point in adding the device-specific description
around "wacom,w9013"...then you should probably mention these
properties there too. The idea was to document possible properties
here (where you're adding them already), and to note the property
names under the devices (or so far, just 1 device) that support them.
Or IOW, you need an addition like this:

 - compatible:
   * "wacom,w9013" (Wacom W9013 digitizer). Supports:
     - vdd-supply
     - post-power-on-delay-ms
+    - reset-gpios
+    - assert-reset-us

Brian

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] devicetree: i2c-hid: Add reset property
  2017-11-04  4:35   ` Brian Norris
@ 2017-11-06  1:00     ` hl
  0 siblings, 0 replies; 9+ messages in thread
From: hl @ 2017-11-06  1:00 UTC (permalink / raw)
  To: Brian Norris
  Cc: Dmitry Torokhov, jikos, robh, benjamin.tissoires, linux-input,
	linux-kernel, devicetree



On Saturday, November 04, 2017 12:35 PM, Brian Norris wrote:
> On Mon, Oct 30, 2017 at 8:03 PM, Lin Huang <hl@rock-chips.com> wrote:
>> Document a "reset" and "assert-reset-us", it can be used for
>> driver control reset property. And reuse post-power-on-delay-ms
>> for deassert reset delay.
>>
>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>> ---
>>   Documentation/devicetree/bindings/input/hid-over-i2c.txt | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.txt b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
>> index 28e8bd8..6ab0eed 100644
>> --- a/Documentation/devicetree/bindings/input/hid-over-i2c.txt
>> +++ b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
>> @@ -31,7 +31,9 @@ device-specific compatible properties, which should be used in addition to the
>>
>>   - vdd-supply: phandle of the regulator that provides the supply voltage.
>>   - post-power-on-delay-ms: time required by the device after enabling its regulators
>> -  before it is ready for communication. Must be used with 'vdd-supply'.
>> +  or deassert reset pin before it is ready for communication.
>> +- reset: phandle of the gpio that provides for hid reset pin.
>> +- assert-reset-us: the device require reset assert time.
> If there was any point in adding the device-specific description
> around "wacom,w9013"...then you should probably mention these
> properties there too. The idea was to document possible properties
> here (where you're adding them already), and to note the property
> names under the devices (or so far, just 1 device) that support them.
> Or IOW, you need an addition like this:
>
>   - compatible:
>     * "wacom,w9013" (Wacom W9013 digitizer). Supports:
>       - vdd-supply
>       - post-power-on-delay-ms
> +    - reset-gpios
> +    - assert-reset-us
  Okay, got it, will fix it next version.
>
> Brian
>
>
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] devicetree: i2c-hid: Add reset property
  2017-11-01 22:02   ` Rob Herring
@ 2017-11-06  9:06     ` Benjamin Tissoires
  2017-11-06 16:10       ` Dmitry Torokhov
  0 siblings, 1 reply; 9+ messages in thread
From: Benjamin Tissoires @ 2017-11-06  9:06 UTC (permalink / raw)
  To: Rob Herring
  Cc: Lin Huang, dmitry.torokhov, jikos, jani.nikula, linux-input,
	linux-kernel, briannorris, devicetree

Hi Rob,

On Nov 01 2017 or thereabouts, Rob Herring wrote:
> On Tue, Oct 31, 2017 at 11:03:16AM +0800, Lin Huang wrote:
> > Document a "reset" and "assert-reset-us", it can be used for
> > driver control reset property. And reuse post-power-on-delay-ms
> > for deassert reset delay.
> 
> "dt-bindings: input: " for the subject please.
> 
> > 
> > Signed-off-by: Lin Huang <hl@rock-chips.com>
> > ---
> >  Documentation/devicetree/bindings/input/hid-over-i2c.txt | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.txt b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> > index 28e8bd8..6ab0eed 100644
> > --- a/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> > +++ b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> > @@ -31,7 +31,9 @@ device-specific compatible properties, which should be used in addition to the
> >  
> >  - vdd-supply: phandle of the regulator that provides the supply voltage.
> >  - post-power-on-delay-ms: time required by the device after enabling its regulators
> > -  before it is ready for communication. Must be used with 'vdd-supply'.
> > +  or deassert reset pin before it is ready for communication.
> > +- reset: phandle of the gpio that provides for hid reset pin.
> 
> The kernel api takes "reset", but the property is "reset-gpios".

In the same way the generic regulator interface handles vdd-supply, is
there any generic OF handling of reset lines? As far as I can tell,
there are 97 references of reset-gpios in my tree, and I wonder if all
the matching drivers need to rewrite the same code to set the reset line
on power on/off.

I am worried because here, 1/2 writes "arbitrary" 0 and 1 to the gpios,
and nothing prevent a manufacturer to invert the required voltages on
the reset lines, meaning there will be a need for a new OF property.

Cheers,
Benjamin

> 
> And it's not just a phandle typically.
> 
> > +- assert-reset-us: the device require reset assert time.
> >  
> >  Example:
> >  
> > -- 
> > 2.7.4
> > 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] devicetree: i2c-hid: Add reset property
  2017-11-06  9:06     ` Benjamin Tissoires
@ 2017-11-06 16:10       ` Dmitry Torokhov
  0 siblings, 0 replies; 9+ messages in thread
From: Dmitry Torokhov @ 2017-11-06 16:10 UTC (permalink / raw)
  To: Benjamin Tissoires, Rob Herring
  Cc: Lin Huang, jikos, jani.nikula, linux-input, linux-kernel,
	briannorris, devicetree

On November 6, 2017 1:06:30 AM PST, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
>Hi Rob,
>
>On Nov 01 2017 or thereabouts, Rob Herring wrote:
>> On Tue, Oct 31, 2017 at 11:03:16AM +0800, Lin Huang wrote:
>> > Document a "reset" and "assert-reset-us", it can be used for
>> > driver control reset property. And reuse post-power-on-delay-ms
>> > for deassert reset delay.
>> 
>> "dt-bindings: input: " for the subject please.
>> 
>> > 
>> > Signed-off-by: Lin Huang <hl@rock-chips.com>
>> > ---
>> >  Documentation/devicetree/bindings/input/hid-over-i2c.txt | 4 +++-
>> >  1 file changed, 3 insertions(+), 1 deletion(-)
>> > 
>> > diff --git
>a/Documentation/devicetree/bindings/input/hid-over-i2c.txt
>b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
>> > index 28e8bd8..6ab0eed 100644
>> > --- a/Documentation/devicetree/bindings/input/hid-over-i2c.txt
>> > +++ b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
>> > @@ -31,7 +31,9 @@ device-specific compatible properties, which
>should be used in addition to the
>> >  
>> >  - vdd-supply: phandle of the regulator that provides the supply
>voltage.
>> >  - post-power-on-delay-ms: time required by the device after
>enabling its regulators
>> > -  before it is ready for communication. Must be used with
>'vdd-supply'.
>> > +  or deassert reset pin before it is ready for communication.
>> > +- reset: phandle of the gpio that provides for hid reset pin.
>> 
>> The kernel api takes "reset", but the property is "reset-gpios".
>
>In the same way the generic regulator interface handles vdd-supply, is
>there any generic OF handling of reset lines? As far as I can tell,
>there are 97 references of reset-gpios in my tree, and I wonder if all
>the matching drivers need to rewrite the same code to set the reset
>line
>on power on/off.
>
>I am worried because here, 1/2 writes "arbitrary" 0 and 1 to the gpios,
>and nothing prevent a manufacturer to invert the required voltages on
>the reset lines, meaning there will be a need for a new OF property.

1and 0 are logical high and low here. Gpiod API takes care of converting to the proper polarity.


Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2017-11-06 16:11 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-31  3:03 [PATCH v2 1/2] HID: i2c-hid: add reset gpio property Lin Huang
2017-10-31  3:03 ` [PATCH v2 2/2] devicetree: i2c-hid: Add reset property Lin Huang
2017-11-01 22:02   ` Rob Herring
2017-11-06  9:06     ` Benjamin Tissoires
2017-11-06 16:10       ` Dmitry Torokhov
2017-11-04  4:35   ` Brian Norris
2017-11-06  1:00     ` hl
2017-10-31  8:47 ` [PATCH v2 1/2] HID: i2c-hid: add reset gpio property Jani Nikula
2017-11-04  2:29 ` Brian Norris

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).