Linux Input Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/2] dt-bindings: HID: i2c-hid: Add the ability to control a reset GPIO
@ 2020-10-14 23:42 Douglas Anderson
  2020-10-14 23:42 ` [PATCH 2/2] " Douglas Anderson
  2020-10-19 21:10 ` [PATCH 1/2] dt-bindings: " Rob Herring
  0 siblings, 2 replies; 3+ messages in thread
From: Douglas Anderson @ 2020-10-14 23:42 UTC (permalink / raw)
  To: jkosina, benjamin.tissoires, gregkh
  Cc: kai.heng.feng, linux-input, hdegoede, andrea, swboyd,
	Douglas Anderson, Dmitry Torokhov, Rob Herring, devicetree,
	linux-kernel

Apparently some devices connected via i2c-hid have timing requirements
around when a reset GPIO should be asserted to them.  The diagram I
have seen, which I believe is from a Goodix device, looked like this:

         +----------------------------------
         |
AVDD ----+
               +------------------------------
         | (a) |
RESET ---------+
                     +-------------
               | (b) |
I2C comm OK ---------+

Where (a) is 10 ms and (b) is 120 ms.

Let's add the ability to specify these timings to the devicetree
bindings.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
I notice this bindings file is still a ".txt" file.  Some searching on
The Internets(TM) shows that Rob has maybe started a conversion 5
years ago [1], but that looks ancient.  I can try to put something
together if need be, or we can just land this fix.  ;-)

Note that the .txt version of the bindings seems to indicate that
anyone using one of the optional properties is supposed to declare
their special compatible string.  I'm not sure if that's still
considered important or not?  Once you manage to get these devices
powered on and talking i2c they self-describe themselves...

[1] https://kernel.googlesource.com/pub/scm/linux/kernel/git/robh/linux/+/refs/heads/dt-yaml/Documentation/devicetree/bindings/hid/hid-over-i2c.yaml

 Documentation/devicetree/bindings/input/hid-over-i2c.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.txt b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
index c76bafaf98d2..6fca39aa8cc6 100644
--- a/Documentation/devicetree/bindings/input/hid-over-i2c.txt
+++ b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
@@ -32,6 +32,11 @@ 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
   or powering it on, before it is ready for communication.
+- reset-gpios: GPIOs to assert to reset the device. This GPIO is asserted when
+  the device is powered off and released post-power-on-delay-ms after
+  enabling the regulators.
+- post-gpio-reset-delay-ms: After deasserting reset we'll delay for this many
+  more milliseconds.
 
 Example:
 
-- 
2.28.0.1011.ga647a8990f-goog


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

* [PATCH 2/2] HID: i2c-hid: Add the ability to control a reset GPIO
  2020-10-14 23:42 [PATCH 1/2] dt-bindings: HID: i2c-hid: Add the ability to control a reset GPIO Douglas Anderson
@ 2020-10-14 23:42 ` Douglas Anderson
  2020-10-19 21:10 ` [PATCH 1/2] dt-bindings: " Rob Herring
  1 sibling, 0 replies; 3+ messages in thread
From: Douglas Anderson @ 2020-10-14 23:42 UTC (permalink / raw)
  To: jkosina, benjamin.tissoires, gregkh
  Cc: kai.heng.feng, linux-input, hdegoede, andrea, swboyd,
	Douglas Anderson, Aaron Ma, Jiri Kosina, Pavel Balan,
	You-Sheng Yang, linux-kernel

Apparently some devices connected via i2c-hid have timing requirements
around when a reset GPIO should be asserted to them.  The diagram I
have seen, which I believe is from a Goodix device, looked like this:

         +----------------------------------
         |
AVDD ----+
               +------------------------------
         | (a) |
RESET ---------+
                     +-------------
               | (b) |
I2C comm OK ---------+

Where (a) is 10 ms and (b) is 120 ms.

Let's add the ability to support this into the i2c-hid driver.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/hid/i2c-hid/i2c-hid-core.c    | 18 ++++++++++++++++++
 include/linux/platform_data/i2c-hid.h |  5 +++++
 2 files changed, 23 insertions(+)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 786e3e9af1c9..807b344b697b 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -1005,6 +1005,9 @@ static void i2c_hid_fwnode_probe(struct i2c_client *client,
 	if (!device_property_read_u32(&client->dev, "post-power-on-delay-ms",
 				      &val))
 		pdata->post_power_delay_ms = val;
+	if (!device_property_read_u32(&client->dev, "post-gpio-reset-delay-ms",
+				      &val))
+		pdata->post_gpio_reset_delay_ms = val;
 }
 
 static int i2c_hid_probe(struct i2c_client *client,
@@ -1053,6 +1056,12 @@ static int i2c_hid_probe(struct i2c_client *client,
 	ihid->pdata.supplies[0].supply = "vdd";
 	ihid->pdata.supplies[1].supply = "vddl";
 
+	/* Start out with reset asserted */
+	ihid->pdata.reset_gpio =
+		devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(ihid->pdata.reset_gpio))
+		return PTR_ERR(ihid->pdata.reset_gpio);
+
 	ret = devm_regulator_bulk_get(&client->dev,
 				      ARRAY_SIZE(ihid->pdata.supplies),
 				      ihid->pdata.supplies);
@@ -1067,6 +1076,10 @@ static int i2c_hid_probe(struct i2c_client *client,
 	if (ihid->pdata.post_power_delay_ms)
 		msleep(ihid->pdata.post_power_delay_ms);
 
+	gpiod_set_value_cansleep(ihid->pdata.reset_gpio, 0);
+	if (ihid->pdata.post_gpio_reset_delay_ms)
+		msleep(ihid->pdata.post_gpio_reset_delay_ms);
+
 	i2c_set_clientdata(client, ihid);
 
 	ihid->client = client;
@@ -1163,6 +1176,7 @@ static int i2c_hid_remove(struct i2c_client *client)
 	if (ihid->bufsize)
 		i2c_hid_free_buffers(ihid);
 
+	gpiod_set_value_cansleep(ihid->pdata.reset_gpio, 1);
 	regulator_bulk_disable(ARRAY_SIZE(ihid->pdata.supplies),
 			       ihid->pdata.supplies);
 
@@ -1228,6 +1242,10 @@ static int i2c_hid_resume(struct device *dev)
 
 		if (ihid->pdata.post_power_delay_ms)
 			msleep(ihid->pdata.post_power_delay_ms);
+
+		gpiod_set_value_cansleep(ihid->pdata.reset_gpio, 0);
+		if (ihid->pdata.post_gpio_reset_delay_ms)
+			msleep(ihid->pdata.post_gpio_reset_delay_ms);
 	} 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 c628bb5e1061..b2150223ffa6 100644
--- a/include/linux/platform_data/i2c-hid.h
+++ b/include/linux/platform_data/i2c-hid.h
@@ -12,6 +12,7 @@
 #ifndef __LINUX_I2C_HID_H
 #define __LINUX_I2C_HID_H
 
+#include <linux/gpio/consumer.h>
 #include <linux/regulator/consumer.h>
 #include <linux/types.h>
 
@@ -20,6 +21,8 @@
  * @hid_descriptor_address: i2c register where the HID descriptor is stored.
  * @supplies: regulators for powering on the device.
  * @post_power_delay_ms: delay after powering on before device is usable.
+ * @post_gpio_reset_delay_ms: delay after reset via GPIO.
+ * @reset_gpio: optional gpio to de-assert after post_power_delay_ms.
  *
  * 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
@@ -36,6 +39,8 @@ struct i2c_hid_platform_data {
 	u16 hid_descriptor_address;
 	struct regulator_bulk_data supplies[2];
 	int post_power_delay_ms;
+	int post_gpio_reset_delay_ms;
+	struct gpio_desc *reset_gpio;
 };
 
 #endif /* __LINUX_I2C_HID_H */
-- 
2.28.0.1011.ga647a8990f-goog


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

* Re: [PATCH 1/2] dt-bindings: HID: i2c-hid: Add the ability to control a reset GPIO
  2020-10-14 23:42 [PATCH 1/2] dt-bindings: HID: i2c-hid: Add the ability to control a reset GPIO Douglas Anderson
  2020-10-14 23:42 ` [PATCH 2/2] " Douglas Anderson
@ 2020-10-19 21:10 ` Rob Herring
  1 sibling, 0 replies; 3+ messages in thread
From: Rob Herring @ 2020-10-19 21:10 UTC (permalink / raw)
  To: Douglas Anderson
  Cc: jkosina, benjamin.tissoires, gregkh, kai.heng.feng, linux-input,
	hdegoede, andrea, swboyd, Dmitry Torokhov, devicetree,
	linux-kernel

On Wed, Oct 14, 2020 at 04:42:20PM -0700, Douglas Anderson wrote:
> Apparently some devices connected via i2c-hid have timing requirements
> around when a reset GPIO should be asserted to them.  The diagram I
> have seen, which I believe is from a Goodix device, looked like this:
> 
>          +----------------------------------
>          |
> AVDD ----+
>                +------------------------------
>          | (a) |
> RESET ---------+
>                      +-------------
>                | (b) |
> I2C comm OK ---------+
> 
> Where (a) is 10 ms and (b) is 120 ms.
> 
> Let's add the ability to specify these timings to the devicetree
> bindings.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> I notice this bindings file is still a ".txt" file.  Some searching on
> The Internets(TM) shows that Rob has maybe started a conversion 5
> years ago [1], but that looks ancient.  I can try to put something
> together if need be, or we can just land this fix.  ;-)

That was attempt number 1 using YAML...

> Note that the .txt version of the bindings seems to indicate that
> anyone using one of the optional properties is supposed to declare
> their special compatible string.  I'm not sure if that's still
> considered important or not?  Once you manage to get these devices
> powered on and talking i2c they self-describe themselves...

This change is exactly why devices should have specific compatible 
strings.

> 
> [1] https://kernel.googlesource.com/pub/scm/linux/kernel/git/robh/linux/+/refs/heads/dt-yaml/Documentation/devicetree/bindings/hid/hid-over-i2c.yaml
> 
>  Documentation/devicetree/bindings/input/hid-over-i2c.txt | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.txt b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> index c76bafaf98d2..6fca39aa8cc6 100644
> --- a/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> +++ b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> @@ -32,6 +32,11 @@ 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
>    or powering it on, before it is ready for communication.
> +- reset-gpios: GPIOs to assert to reset the device. This GPIO is asserted when
> +  the device is powered off and released post-power-on-delay-ms after
> +  enabling the regulators.
> +- post-gpio-reset-delay-ms: After deasserting reset we'll delay for this many
> +  more milliseconds.

IMO, you should imply this from the compatible string.

Rob

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

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-14 23:42 [PATCH 1/2] dt-bindings: HID: i2c-hid: Add the ability to control a reset GPIO Douglas Anderson
2020-10-14 23:42 ` [PATCH 2/2] " Douglas Anderson
2020-10-19 21:10 ` [PATCH 1/2] dt-bindings: " Rob Herring

Linux Input Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-input/0 linux-input/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-input linux-input/ https://lore.kernel.org/linux-input \
		linux-input@vger.kernel.org
	public-inbox-index linux-input

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-input


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git