* [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 related [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