[PATCH v2 0/3] HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms

[PATCH v2 0/3] HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms

[Hans de Goede]

Hi Benjamin,

Here is a v2 of my series to allow using i2c-hid-of on non OF platforms
to allow I2C-HID devices which are not enumerated by ACPI to work on ACPI
platforms (by manual i2c_client instantiation using i2c_client_id matching).

Changes in v2:
- As discussed Drop the patches to consolidate all the i2c-hid-of*
  drivers into one
- Add a comment to the "post-reset-deassert-delay-ms" property reading,
  that it is a kernel internal (non public) property used between x86
  platform code and the i2c-hid driver.

Regards,

Hans


Hans de Goede (3):
  HID: i2c-hid-of: Consistenly use dev local variable in probe()
  HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms
  HID: i2c-hid-of: Add reset GPIO support to i2c-hid-of

 drivers/hid/i2c-hid/Kconfig      |  6 +++--
 drivers/hid/i2c-hid/i2c-hid-of.c | 38 ++++++++++++++++++++++++--------
 2 files changed, 33 insertions(+), 11 deletions(-)

-- 
2.39.1

[PATCH v2 1/3] HID: i2c-hid-of: Consistenly use dev local variable in probe()

[Hans de Goede]

i2c_hid_of_probe() has a dev local variable pointing to &i2c_client->dev,
consistently use this everywhere in i2c_hid_of_probe().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/i2c-hid/i2c-hid-of.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
index 10176568133a..c82a5a54c3e6 100644
--- a/drivers/hid/i2c-hid/i2c-hid-of.c
+++ b/drivers/hid/i2c-hid/i2c-hid-of.c
@@ -75,7 +75,7 @@ static int i2c_hid_of_probe(struct i2c_client *client)
 	int ret;
 	u32 val;
 
-	ihid_of = devm_kzalloc(&client->dev, sizeof(*ihid_of), GFP_KERNEL);
+	ihid_of = devm_kzalloc(dev, sizeof(*ihid_of), GFP_KERNEL);
 	if (!ihid_of)
 		return -ENOMEM;
 
@@ -84,24 +84,21 @@ static int i2c_hid_of_probe(struct i2c_client *client)
 
 	ret = of_property_read_u32(dev->of_node, "hid-descr-addr", &val);
 	if (ret) {
-		dev_err(&client->dev, "HID register address not provided\n");
+		dev_err(dev, "HID register address not provided\n");
 		return -ENODEV;
 	}
 	if (val >> 16) {
-		dev_err(&client->dev, "Bad HID register address: 0x%08x\n",
-			val);
+		dev_err(dev, "Bad HID register address: 0x%08x\n", val);
 		return -EINVAL;
 	}
 	hid_descriptor_address = val;
 
-	if (!device_property_read_u32(&client->dev, "post-power-on-delay-ms",
-				      &val))
+	if (!device_property_read_u32(dev, "post-power-on-delay-ms", &val))
 		ihid_of->post_power_delay_ms = val;
 
 	ihid_of->supplies[0].supply = "vdd";
 	ihid_of->supplies[1].supply = "vddl";
-	ret = devm_regulator_bulk_get(&client->dev,
-				      ARRAY_SIZE(ihid_of->supplies),
+	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ihid_of->supplies),
 				      ihid_of->supplies);
 	if (ret)
 		return ret;
-- 
2.39.1

[PATCH v2 2/3] HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms

[Hans de Goede]

There are some x86 tablets / 2-in-1s which ship with Android as their
factory OS image. These have pretty broken ACPI tables, relying on
everything being hardcoded in the factory kernel image.

platform/x86/x86-android-tablets.c manually instantiates i2c-clients for
i2c devices on these tablets to make them work with the mainline kernel.

The Lenovo Yoga Book 1 (yb1-x90f/l) is such a 2-in-1. It has 2 I2C-HID
devices its main touchscreen and a Wacom digitizer. Its main touchscreen
can alternatively also be used in HiDeep's native protocol mode but
for the Wacom digitizer we really need I2C-HID.

This patch allows using i2c-hid-of on non OF platforms so that it can
bind to a non ACPI instantiated i2c_client on x86 for the Wacom digitizer.
Note the driver already has an "i2c-over-hid" i2c_device_id (rather then
an of_device_id).

Besides enabling building on non-OF platforms this also replaces
the only of_property_read_u32() call with device_property_read_u32() note
that other properties where already read using device_property_read_...().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/i2c-hid/Kconfig      | 6 ++++--
 drivers/hid/i2c-hid/i2c-hid-of.c | 4 +++-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/i2c-hid/Kconfig b/drivers/hid/i2c-hid/Kconfig
index 4439be7fa74d..3be17109301a 100644
--- a/drivers/hid/i2c-hid/Kconfig
+++ b/drivers/hid/i2c-hid/Kconfig
@@ -23,12 +23,14 @@ config I2C_HID_ACPI
 
 config I2C_HID_OF
 	tristate "HID over I2C transport layer Open Firmware driver"
-	depends on OF
+	# No "depends on OF" because this can also be used for manually
+	# (board-file) instantiated "hid-over-i2c" type i2c-clients.
 	select I2C_HID_CORE
 	help
 	  Say Y here if you use a keyboard, a touchpad, a touchscreen, or any
 	  other HID based devices which is connected to your computer via I2C.
-	  This driver supports Open Firmware (Device Tree)-based systems.
+	  This driver supports Open Firmware (Device Tree)-based systems as
+	  well as binding to manually (board-file) instantiated i2c-hid-clients.
 
 	  If unsure, say N.
 
diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
index c82a5a54c3e6..385f7460e03c 100644
--- a/drivers/hid/i2c-hid/i2c-hid-of.c
+++ b/drivers/hid/i2c-hid/i2c-hid-of.c
@@ -82,7 +82,7 @@ static int i2c_hid_of_probe(struct i2c_client *client)
 	ihid_of->ops.power_up = i2c_hid_of_power_up;
 	ihid_of->ops.power_down = i2c_hid_of_power_down;
 
-	ret = of_property_read_u32(dev->of_node, "hid-descr-addr", &val);
+	ret = device_property_read_u32(dev, "hid-descr-addr", &val);
 	if (ret) {
 		dev_err(dev, "HID register address not provided\n");
 		return -ENODEV;
@@ -113,11 +113,13 @@ static int i2c_hid_of_probe(struct i2c_client *client)
 				  hid_descriptor_address, quirks);
 }
 
+#ifdef CONFIG_OF
 static const struct of_device_id i2c_hid_of_match[] = {
 	{ .compatible = "hid-over-i2c" },
 	{},
 };
 MODULE_DEVICE_TABLE(of, i2c_hid_of_match);
+#endif
 
 static const struct i2c_device_id i2c_hid_of_id_table[] = {
 	{ "hid", 0 },
-- 
2.39.1

[PATCH v2 3/3] HID: i2c-hid-of: Add reset GPIO support to i2c-hid-of

[Hans de Goede]

Add reset GPIO support to the generic i2c-hid-of driver

This is necessary to make the Wacom digitizer on the Lenovo Yoga Book 1
(yb1-x90f/l) work and this will also allow consolidating the 2 specialized
i2c-hid-of-elan.c and i2c-hid-of-goodix.c drivers into the generic
i2c-hid-of driver.

For now the new "post-reset-deassert-delay-ms" property is only used on
x86/ACPI (non devicetree) devs. IOW it is not used in actual devicetree
files and the same goes for the reset GPIO. The devicetree-bindings
maintainers have requested properties like these to not be added to
the devicetree-bindings, so the new property + GPIO are deliberately
not added to the existing devicetree-bindings.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Chanhes in v2:
- Add a comment to the "post-reset-deassert-delay-ms" property reading,
  that it is a kernel internal (non public) property used between x86
  platform code and the i2c-hid driver.
---
 drivers/hid/i2c-hid/i2c-hid-of.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
index 385f7460e03c..855f53092f4e 100644
--- a/drivers/hid/i2c-hid/i2c-hid-of.c
+++ b/drivers/hid/i2c-hid/i2c-hid-of.c
@@ -21,6 +21,7 @@
 
 #include <linux/delay.h>
 #include <linux/device.h>
+#include <linux/gpio/consumer.h>
 #include <linux/hid.h>
 #include <linux/i2c.h>
 #include <linux/kernel.h>
@@ -35,8 +36,10 @@ struct i2c_hid_of {
 	struct i2chid_ops ops;
 
 	struct i2c_client *client;
+	struct gpio_desc *reset_gpio;
 	struct regulator_bulk_data supplies[2];
 	int post_power_delay_ms;
+	int post_reset_delay_ms;
 };
 
 static int i2c_hid_of_power_up(struct i2chid_ops *ops)
@@ -55,6 +58,10 @@ static int i2c_hid_of_power_up(struct i2chid_ops *ops)
 	if (ihid_of->post_power_delay_ms)
 		msleep(ihid_of->post_power_delay_ms);
 
+	gpiod_set_value_cansleep(ihid_of->reset_gpio, 0);
+	if (ihid_of->post_reset_delay_ms)
+		msleep(ihid_of->post_reset_delay_ms);
+
 	return 0;
 }
 
@@ -62,6 +69,7 @@ static void i2c_hid_of_power_down(struct i2chid_ops *ops)
 {
 	struct i2c_hid_of *ihid_of = container_of(ops, struct i2c_hid_of, ops);
 
+	gpiod_set_value_cansleep(ihid_of->reset_gpio, 1);
 	regulator_bulk_disable(ARRAY_SIZE(ihid_of->supplies),
 			       ihid_of->supplies);
 }
@@ -96,6 +104,19 @@ static int i2c_hid_of_probe(struct i2c_client *client)
 	if (!device_property_read_u32(dev, "post-power-on-delay-ms", &val))
 		ihid_of->post_power_delay_ms = val;
 
+	/*
+	 * Note this is a kernel internal device-property set by x86 platform code,
+	 * this MUST not be used in devicetree files without first adding it to
+	 * the DT bindings.
+	 */
+	if (!device_property_read_u32(dev, "post-reset-deassert-delay-ms", &val))
+		ihid_of->post_reset_delay_ms = val;
+
+	/* Start out with reset asserted */
+	ihid_of->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(ihid_of->reset_gpio))
+		return PTR_ERR(ihid_of->reset_gpio);
+
 	ihid_of->supplies[0].supply = "vdd";
 	ihid_of->supplies[1].supply = "vddl";
 	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ihid_of->supplies),
-- 
2.39.1

Re: [PATCH v2 0/3] HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms

[Benjamin Tissoires]

On Apr 13 2023, Hans de Goede wrote:
> Hi Benjamin,
> 
> Here is a v2 of my series to allow using i2c-hid-of on non OF platforms
> to allow I2C-HID devices which are not enumerated by ACPI to work on ACPI
> platforms (by manual i2c_client instantiation using i2c_client_id matching).
> 
> Changes in v2:
> - As discussed Drop the patches to consolidate all the i2c-hid-of*
>   drivers into one
> - Add a comment to the "post-reset-deassert-delay-ms" property reading,
>   that it is a kernel internal (non public) property used between x86
>   platform code and the i2c-hid driver.

The series is:
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

I'd rather have at least Doug test this one before we merge it, but I'll
be out next week, so you'll have to rely on Jiri to merge it (or wait
for one week).

Cheers,
Benjamin

> 
> Regards,
> 
> Hans
> 
> 
> Hans de Goede (3):
>   HID: i2c-hid-of: Consistenly use dev local variable in probe()
>   HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms
>   HID: i2c-hid-of: Add reset GPIO support to i2c-hid-of
> 
>  drivers/hid/i2c-hid/Kconfig      |  6 +++--
>  drivers/hid/i2c-hid/i2c-hid-of.c | 38 ++++++++++++++++++++++++--------
>  2 files changed, 33 insertions(+), 11 deletions(-)
> 
> -- 
> 2.39.1
> 

Re: [PATCH v2 0/3] HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms

[Hans de Goede]

Hi,

On 4/13/23 16:01, Benjamin Tissoires wrote:
> On Apr 13 2023, Hans de Goede wrote:
>> Hi Benjamin,
>>
>> Here is a v2 of my series to allow using i2c-hid-of on non OF platforms
>> to allow I2C-HID devices which are not enumerated by ACPI to work on ACPI
>> platforms (by manual i2c_client instantiation using i2c_client_id matching).
>>
>> Changes in v2:
>> - As discussed Drop the patches to consolidate all the i2c-hid-of*
>>   drivers into one
>> - Add a comment to the "post-reset-deassert-delay-ms" property reading,
>>   that it is a kernel internal (non public) property used between x86
>>   platform code and the i2c-hid driver.
> 
> The series is:
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Thank you.

> I'd rather have at least Doug test this one before we merge it, but I'll
> be out next week, so you'll have to rely on Jiri to merge it (or wait
> for one week).

Since Doug's devices use the separate specialized drivers and this series
now only touches the generic i2c-hid-of driver I don't think Doug can test
this, or am I missing something ?

Regards,

Hans





>> Hans de Goede (3):
>>   HID: i2c-hid-of: Consistenly use dev local variable in probe()
>>   HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms
>>   HID: i2c-hid-of: Add reset GPIO support to i2c-hid-of
>>
>>  drivers/hid/i2c-hid/Kconfig      |  6 +++--
>>  drivers/hid/i2c-hid/i2c-hid-of.c | 38 ++++++++++++++++++++++++--------
>>  2 files changed, 33 insertions(+), 11 deletions(-)
>>
>> -- 
>> 2.39.1
>>
> 

Re: [PATCH v2 0/3] HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms

[Benjamin Tissoires]

On Thu, Apr 13, 2023 at 4:03 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 4/13/23 16:01, Benjamin Tissoires wrote:
> > On Apr 13 2023, Hans de Goede wrote:
> >> Hi Benjamin,
> >>
> >> Here is a v2 of my series to allow using i2c-hid-of on non OF platforms
> >> to allow I2C-HID devices which are not enumerated by ACPI to work on ACPI
> >> platforms (by manual i2c_client instantiation using i2c_client_id matching).
> >>
> >> Changes in v2:
> >> - As discussed Drop the patches to consolidate all the i2c-hid-of*
> >>   drivers into one
> >> - Add a comment to the "post-reset-deassert-delay-ms" property reading,
> >>   that it is a kernel internal (non public) property used between x86
> >>   platform code and the i2c-hid driver.
> >
> > The series is:
> > Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
> Thank you.
>
> > I'd rather have at least Doug test this one before we merge it, but I'll
> > be out next week, so you'll have to rely on Jiri to merge it (or wait
> > for one week).
>
> Since Doug's devices use the separate specialized drivers and this series
> now only touches the generic i2c-hid-of driver I don't think Doug can test
> this, or am I missing something ?

Ooh, sorry for the confusion. I thought we were having:
i2c-hid-of-elan -> i2c-hid-of -> i2c-hid-core
When actually, i2c-hid-of-elan is only talking to i2c-hid-core.

This is why I wanted i2c-hid-of-elan/goodix to override the property
in their probe and then forward the payload to i2c-hid-of...

I guess we can take this series right away then

Cheers,
Benjamin

>
> Regards,
>
> Hans
>
>
>
>
>
> >> Hans de Goede (3):
> >>   HID: i2c-hid-of: Consistenly use dev local variable in probe()
> >>   HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms
> >>   HID: i2c-hid-of: Add reset GPIO support to i2c-hid-of
> >>
> >>  drivers/hid/i2c-hid/Kconfig      |  6 +++--
> >>  drivers/hid/i2c-hid/i2c-hid-of.c | 38 ++++++++++++++++++++++++--------
> >>  2 files changed, 33 insertions(+), 11 deletions(-)
> >>
> >> --
> >> 2.39.1
> >>
> >
>

Re: [PATCH v2 0/3] HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms

[Benjamin Tissoires]

On Thu, 13 Apr 2023 11:36:22 +0200, Hans de Goede wrote:
> Here is a v2 of my series to allow using i2c-hid-of on non OF platforms
> to allow I2C-HID devices which are not enumerated by ACPI to work on ACPI
> platforms (by manual i2c_client instantiation using i2c_client_id matching).
> 
> Changes in v2:
> - As discussed Drop the patches to consolidate all the i2c-hid-of*
>   drivers into one
> - Add a comment to the "post-reset-deassert-delay-ms" property reading,
>   that it is a kernel internal (non public) property used between x86
>   platform code and the i2c-hid driver.
> 
> [...]

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git (for-6.4/i2c-hid), thanks!

[1/3] HID: i2c-hid-of: Consistenly use dev local variable in probe()
      https://git.kernel.org/hid/hid/c/9d793e7c1f88
[2/3] HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms
      https://git.kernel.org/hid/hid/c/728ec8b6eda8
[3/3] HID: i2c-hid-of: Add reset GPIO support to i2c-hid-of
      https://git.kernel.org/hid/hid/c/2be404486c05

Cheers,
-- 
Benjamin Tissoires <benjamin.tissoires@redhat.com>