HID: i2c-hid: Use device properties (instead of device tree)

PATCH In review Rajat Jain 5 participants 10 messages Sep 29, 2017 – Nov 21, 2017

Subsystems (3)

Device tree bindings HID Input drivers

lore

Share & tools

lore

Rajat Jain

author

2017-09-29 22:44 UTC #0 lore raw

PATCH 1/1 HID: i2c-hid: Use device properties (instead of device tree)

Use the device properties (that can be provided by ACPI systems as well as non ACPI systems) instead of device tree properties (that are not provided ACPI systems). This required some minor code restructuring.

Signed-off-by Rajat Jain <[email protected]>

---
I don't think its a big deal, but just FYI, this changes the order in which we look for HID register address from
(device tree -> platform_data -> ACPI) to
(platform data -> device tree -> ACPI)

 drivers/hid/i2c-hid/i2c-hid.c | 44 ++++++++++++++-----------------------------
 1 file changed, 14 insertions(+), 30 deletions(-)

drivers/hid/i2c-hid/i2c-hid.c +14 ~~-30~~

index 77396145d2d0..718afceb2395 100644
@@ -908,45 +908,36 @@ static inline int i2c_hid_acpi_pdata(struct i2c_client *client,
 static inline void i2c_hid_acpi_fix_up_power(struct device *dev) {}
 #endif
 
-#ifdef CONFIG_OF
-static int i2c_hid_of_probe(struct i2c_client *client,
+static int i2c_hid_fwnode_probe(struct i2c_client *client,
 		struct i2c_hid_platform_data *pdata)
 {
 	struct device *dev = &client->dev;
 	u32 val;
 	int ret;
 
-	ret = of_property_read_u32(dev->of_node, "hid-descr-addr", &val);
-	if (ret) {
-		dev_err(&client->dev, "HID register address not provided\n");
-		return -ENODEV;
-	}
-	if (val >> 16) {
-		dev_err(&client->dev, "Bad HID register address: 0x%08x\n",
-			val);
-		return -EINVAL;
+	ret = device_property_read_u32(dev, "hid-descr-addr", &val);
+	if (ret || val >> 16) {
+		/* Couldn't read using fwnode, try ACPI next */
+		if (!i2c_hid_acpi_pdata(client, pdata)) {
+			dev_err(dev, "Bad/Not provided HID register address\n");
+			return -ENODEV;
+		}
 	}
 	pdata->hid_descriptor_address = val;
 
-	ret = of_property_read_u32(dev->of_node, "post-power-on-delay-ms",
-				   &val);
+	ret = device_property_read_u32(dev, "post-power-on-delay-ms", &val);
 	if (!ret)
 		pdata->post_power_delay_ms = val;
 
 	return 0;
 }
 
+#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);
-#else
-static inline int i2c_hid_of_probe(struct i2c_client *client,
-		struct i2c_hid_platform_data *pdata)
-{
-	return -ENODEV;
-}
 #endif
 
 static int i2c_hid_probe(struct i2c_client *client,

@@ -977,19 +968,12 @@ static int i2c_hid_probe(struct i2c_client *client,
 	if (!ihid)
 		return -ENOMEM;
 
-	if (client->dev.of_node) {
-		ret = i2c_hid_of_probe(client, &ihid->pdata);
+	if (platform_data) {
+		ihid->pdata = *platform_data;
+	} else if (dev_fwnode(&client->dev)) {
+		ret = i2c_hid_fwnode_probe(client, &ihid->pdata);
 		if (ret)
 			goto err;
-	} else if (!platform_data) {
-		ret = i2c_hid_acpi_pdata(client, &ihid->pdata);
-		if (ret) {
-			dev_err(&client->dev,
-				"HID register address not provided\n");
-			goto err;
-		}
-	} else {
-		ihid->pdata = *platform_data;
 	}
 
 	ihid->pdata.supply = devm_regulator_get(&client->dev, "vdd");

Signature · 1 line

2.14.2.822.g60be5d43e6-goog

Brian Norris

2017-09-30 00:08 UTC #1 lore raw

Hi Rajat,

Quoted 33 lines · Rajat Jain

Use the device properties (that can be provided by ACPI systems as well as non ACPI systems) instead of device tree properties (that are not provided ACPI systems). This required some minor code restructuring.
Signed-off-by Rajat Jain <[email protected]>
---
I don't think its a big deal, but just FYI, this changes the order in which we look for HID register address from
(device tree -> platform_data -> ACPI) to
(platform data -> device tree -> ACPI)
 drivers/hid/i2c-hid/i2c-hid.c | 44 ++++++++++++++-----------------------------
 1 file changed, 14 insertions(+), 30 deletions(-)
drivers/hid/i2c-hid/i2c-hid.c +1 -3
index 77396145d2d0..718afceb2395 100644
@@ -908,45 +908,36 @@ static inline int i2c_hid_acpi_pdata(struct i2c_client *client,
 static inline void i2c_hid_acpi_fix_up_power(struct device *dev) {}
 #endif
 
-#ifdef CONFIG_OF
-static int i2c_hid_of_probe(struct i2c_client *client,
+static int i2c_hid_fwnode_probe(struct i2c_client *client,
 		struct i2c_hid_platform_data *pdata)
 {
 	struct device *dev = &client->dev;
 	u32 val;
 	int ret;
 
-	ret = of_property_read_u32(dev->of_node, "hid-descr-addr", &val);

drivers/hid/i2c-hid/i2c-hid.c static inline int i2c_hid_acpi_pdata(struct i2c_client *client, · line 919

Quoted 10 lines

drivers/hid/i2c-hid/i2c-hid.c +2 -8

-	if (ret) {
-		dev_err(&client->dev, "HID register address not provided\n");
-		return -ENODEV;
-	}
-	if (val >> 16) {
-		dev_err(&client->dev, "Bad HID register address: 0x%08x\n",
-			val);
-		return -EINVAL;
+	ret = device_property_read_u32(dev, "hid-descr-addr", &val);
+	if (ret || val >> 16) {

We used to reject a bad addr with -EINVAL. Now we retry with ACPI. Is that reasonable? I'd think you should just reject a bad value.

drivers/hid/i2c-hid/i2c-hid.c static inline int i2c_hid_acpi_pdata(struct i2c_client *client, · line 921

Quoted 2 lines

drivers/hid/i2c-hid/i2c-hid.c +2 -0

+		/* Couldn't read using fwnode, try ACPI next */
+		if (!i2c_hid_acpi_pdata(client, pdata)) {

I think the '!' negation is wrong. Returning 0 is success.

drivers/hid/i2c-hid/i2c-hid.c static inline int i2c_hid_acpi_pdata(struct i2c_client *client, · line 923

Quoted 2 lines

drivers/hid/i2c-hid/i2c-hid.c +2 -0

+			dev_err(dev, "Bad/Not provided HID register address\n");
+			return -ENODEV;

This should propagate the error code from i2c_hid_acpi_pdata().

drivers/hid/i2c-hid/i2c-hid.c static inline int i2c_hid_acpi_pdata(struct i2c_client *client, · line 926

Quoted 3 lines

drivers/hid/i2c-hid/i2c-hid.c +1 -0
+		}
 	}
 	pdata->hid_descriptor_address = val;

This will break ACPI (with no device property) now; i2c_hid_acpi_pdata() can parse one value, but then you'll clobber it here with some junk ('val' is potentially uninitialized in the ACPI case).

Quoted 37 lines

drivers/hid/i2c-hid/i2c-hid.c +6 ~~-10~~

-	ret = of_property_read_u32(dev->of_node, "post-power-on-delay-ms",
-				   &val);
+	ret = device_property_read_u32(dev, "post-power-on-delay-ms", &val);
 	if (!ret)
 		pdata->post_power_delay_ms = val;
 
 	return 0;
 }
 
+#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);
-#else
-static inline int i2c_hid_of_probe(struct i2c_client *client,
-		struct i2c_hid_platform_data *pdata)
-{
-	return -ENODEV;
-}
 #endif
 
 static int i2c_hid_probe(struct i2c_client *client,

@@ -977,19 +968,12 @@ static int i2c_hid_probe(struct i2c_client *client,
 	if (!ihid)
 		return -ENOMEM;
 
-	if (client->dev.of_node) {
-		ret = i2c_hid_of_probe(client, &ihid->pdata);
+	if (platform_data) {
+		ihid->pdata = *platform_data;
+	} else if (dev_fwnode(&client->dev)) {
+		ret = i2c_hid_fwnode_probe(client, &ihid->pdata);
 		if (ret)
 			goto err;

drivers/hid/i2c-hid/i2c-hid.c static int i2c_hid_probe(struct i2c_client *client, · line 977

Quoted 10 lines

drivers/hid/i2c-hid/i2c-hid.c +0 -9

-	} else if (!platform_data) {
-		ret = i2c_hid_acpi_pdata(client, &ihid->pdata);
-		if (ret) {
-			dev_err(&client->dev,
-				"HID register address not provided\n");
-			goto err;
-		}
-	} else {
-		ihid->pdata = *platform_data;
 	}

Where's the 'else' case now? Presumably there's some case where you have neither platform_data nor dev_fwnode() (I actually don't know much about non-device tree fwnodes -- do all ACPI systems have them now?)

Anyway, I'd think you should have at least an error in the 'else' case now.

Brian

Quoted 5 lines

drivers/hid/i2c-hid/i2c-hid.c +0 -0
 	ihid->pdata.supply = devm_regulator_get(&client->dev, "vdd");
Signature · 1 line
2.14.2.822.g60be5d43e6-goog

Rajat Jain

author

2017-10-02 19:27 UTC #2 lore raw

Quoted 120 lines · Brian Norris

Hi Rajat,
Quoted 33 lines · Rajat Jain
Use the device properties (that can be provided by ACPI systems as well as non ACPI systems) instead of device tree properties (that are not provided ACPI systems). This required some minor code restructuring.
Signed-off-by Rajat Jain <[email protected]>
---
I don't think its a big deal, but just FYI, this changes the order in which we look for HID register address from
(device tree -> platform_data -> ACPI) to
(platform data -> device tree -> ACPI)
 drivers/hid/i2c-hid/i2c-hid.c | 44 ++++++++++++++-----------------------------
 1 file changed, 14 insertions(+), 30 deletions(-)
drivers/hid/i2c-hid/i2c-hid.c +1 -3
index 77396145d2d0..718afceb2395 100644
@@ -908,45 +908,36 @@ static inline int i2c_hid_acpi_pdata(struct i2c_client *client,
 static inline void i2c_hid_acpi_fix_up_power(struct device *dev) {}
 #endif
 
-#ifdef CONFIG_OF
-static int i2c_hid_of_probe(struct i2c_client *client,
+static int i2c_hid_fwnode_probe(struct i2c_client *client,
              struct i2c_hid_platform_data *pdata)
 {
      struct device *dev = &client->dev;
      u32 val;
      int ret;
 
-     ret = of_property_read_u32(dev->of_node, "hid-descr-addr", &val);
drivers/hid/i2c-hid/i2c-hid.c static inline int i2c_hid_acpi_pdata(struct i2c_client *client, · line 919
Quoted 10 lines
drivers/hid/i2c-hid/i2c-hid.c +2 -8
-     if (ret) {
-             dev_err(&client->dev, "HID register address not provided\n");
-             return -ENODEV;
-     }
-     if (val >> 16) {
-             dev_err(&client->dev, "Bad HID register address: 0x%08x\n",
-                     val);
-             return -EINVAL;
+     ret = device_property_read_u32(dev, "hid-descr-addr", &val);
+     if (ret || val >> 16) {
We used to reject a bad addr with -EINVAL. Now we retry with ACPI. Is that reasonable? I'd think you should just reject a bad value.
drivers/hid/i2c-hid/i2c-hid.c static inline int i2c_hid_acpi_pdata(struct i2c_client *client, · line 921
Quoted 2 lines
drivers/hid/i2c-hid/i2c-hid.c +2 -0
+             /* Couldn't read using fwnode, try ACPI next */
+             if (!i2c_hid_acpi_pdata(client, pdata)) {
I think the '!' negation is wrong. Returning 0 is success.
drivers/hid/i2c-hid/i2c-hid.c static inline int i2c_hid_acpi_pdata(struct i2c_client *client, · line 923
Quoted 2 lines
drivers/hid/i2c-hid/i2c-hid.c +2 -0
+                     dev_err(dev, "Bad/Not provided HID register address\n");
+                     return -ENODEV;
This should propagate the error code from i2c_hid_acpi_pdata().
drivers/hid/i2c-hid/i2c-hid.c static inline int i2c_hid_acpi_pdata(struct i2c_client *client, · line 926
Quoted 3 lines
drivers/hid/i2c-hid/i2c-hid.c +1 -0
+             }
      }
      pdata->hid_descriptor_address = val;
This will break ACPI (with no device property) now; i2c_hid_acpi_pdata() can parse one value, but then you'll clobber it here with some junk ('val' is potentially uninitialized in the ACPI case).
Quoted 37 lines
drivers/hid/i2c-hid/i2c-hid.c +6 ~~-10~~
-     ret = of_property_read_u32(dev->of_node, "post-power-on-delay-ms",
-                                &val);
+     ret = device_property_read_u32(dev, "post-power-on-delay-ms", &val);
      if (!ret)
              pdata->post_power_delay_ms = val;
 
      return 0;
 }
 
+#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);
-#else
-static inline int i2c_hid_of_probe(struct i2c_client *client,
-             struct i2c_hid_platform_data *pdata)
-{
-     return -ENODEV;
-}
 #endif
 
 static int i2c_hid_probe(struct i2c_client *client,
@@ -977,19 +968,12 @@ static int i2c_hid_probe(struct i2c_client *client,
      if (!ihid)
              return -ENOMEM;
 
-     if (client->dev.of_node) {
-             ret = i2c_hid_of_probe(client, &ihid->pdata);
+     if (platform_data) {
+             ihid->pdata = *platform_data;
+     } else if (dev_fwnode(&client->dev)) {
+             ret = i2c_hid_fwnode_probe(client, &ihid->pdata);
              if (ret)
                      goto err;
drivers/hid/i2c-hid/i2c-hid.c static int i2c_hid_probe(struct i2c_client *client, · line 977
Quoted 10 lines
drivers/hid/i2c-hid/i2c-hid.c +0 -9
-     } else if (!platform_data) {
-             ret = i2c_hid_acpi_pdata(client, &ihid->pdata);
-             if (ret) {
-                     dev_err(&client->dev,
-                             "HID register address not provided\n");
-                     goto err;
-             }
-     } else {
-             ihid->pdata = *platform_data;
      }
Where's the 'else' case now? Presumably there's some case where you have neither platform_data nor dev_fwnode() (I actually don't know much about non-device tree fwnodes -- do all ACPI systems have them now?)

Quoted 2 lines

Anyway, I'd think you should have at least an error in the 'else' case now.

Thanks Brian for the review. Based on Andy's review, I think it might be more acceptable to just read the property post-power-on-delay property in the i2c_hid_acpi_pdata(), and thus not change anything else. So please allow me to send another patch, that should hopefully not raise the concerns that you raised.

Thanks,
Rajat

Quoted 8 lines

Brian
Quoted 5 lines
drivers/hid/i2c-hid/i2c-hid.c +0 -1
      ihid->pdata.supply = devm_regulator_get(&client->dev, "vdd");
--
2.14.2.822.g60be5d43e6-goog

Andy Shevchenko

2017-10-01 16:18 UTC #3 lore raw

In reply to Rajat Jain

Quoted 11 lines · Rajat Jain

Use the device properties (that can be provided by ACPI systems as well as non ACPI systems) instead of device tree properties (that are not provided ACPI systems). This required some minor code restructuring.
I don't think its a big deal, but just FYI, this changes the order in which we
look for HID register address from
(device tree -> platform_data -> ACPI) to
(platform data -> device tree -> ACPI)

I do.

We would like to discourage use of legacy platform data in favour of Device Tree / ACPI.

Quoted 25 lines

drivers/hid/i2c-hid/i2c-hid.c +1 -1

+static int i2c_hid_fwnode_probe(struct i2c_client *client,
 		struct i2c_hid_platform_data *pdata)
 {
 	struct device *dev = &client->dev;
 	u32 val;
 	int ret;
 
-	ret = of_property_read_u32(dev->of_node, "hid-descr-addr",

&val);

(quoted diff) +0 -2

-	if (ret) {
-		dev_err(&client->dev, "HID register address not

provided\n");

drivers/hid/i2c-hid/i2c-hid.c +0 -4

-		return -ENODEV;
-	}
-	if (val >> 16) {
-		dev_err(&client->dev, "Bad HID register address:

0x%08x\n",

drivers/hid/i2c-hid/i2c-hid.c +5 -2

-			val);
-		return -EINVAL;
+	ret = device_property_read_u32(dev, "hid-descr-addr", &val);
+	if (ret || val >> 16) {
+		/* Couldn't read using fwnode, try ACPI next */
+		if (!i2c_hid_acpi_pdata(client, pdata)) {
+			dev_err(dev, "Bad/Not provided HID register

address\n");

drivers/hid/i2c-hid/i2c-hid.c static inline int i2c_hid_acpi_pdata(struct i2c_client *client, · line 924

Quoted 2 lines

drivers/hid/i2c-hid/i2c-hid.c +2 -0
+			return -ENODEV;
+		}

Why not just replace of_ calls by device_ ones?

Quoted 9 lines

drivers/hid/i2c-hid/i2c-hid.c +0 -1

 	}
 	pdata->hid_descriptor_address = val;
 
-	ret = of_property_read_u32(dev->of_node, "post-power-on-

delay-ms",

(quoted diff) +1 -1

-				   &val);
+	ret = device_property_read_u32(dev, "post-power-on-delay-ms",

&val);

drivers/hid/i2c-hid/i2c-hid.c static inline int i2c_hid_acpi_pdata(struct i2c_client *client, · line 933

Quoted 5 lines

drivers/hid/i2c-hid/i2c-hid.c +0 -0

 	if (!ret)
 		pdata->post_power_delay_ms = val;
 
 	return 0;
 }

Looking how ACPI support is established in the driver, I would rather NAK this change. Is there any _actual_ hardware on the wild with such properties?

HID protocol for ACPI is described in [1] where nothing is about _DSD.

[1]: https://docs.microsoft.com/en-us/windows-hardware/drivers/hid/plug-
and-play-support-and-power-management

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Rajat Jain

author

2017-10-02 19:24 UTC #4 lore raw

OK, I think my patch changes more than what is needed (to solve my problem), and in the process creates concerns. Please allow me to step back and elaborate on the problem I'm trying to solve.

The hardware I am working on (Wacom touchscreen) does require a good 100ms delay after the reset, which I wanted to be able to specify via the ACPI (this is an x86 platform). I only wanted to address the problem of ACPI not being able to specify "post-power-on-delay-ms", and I did not have the intention of changing any thing other than that. So I'd gladly welcome any suggestions that I may get.

Quoted 13 lines · Andy Shevchenko

Quoted 11 lines · Rajat Jain
Use the device properties (that can be provided by ACPI systems as well as non ACPI systems) instead of device tree properties (that are not provided ACPI systems). This required some minor code restructuring.
I don't think its a big deal, but just FYI, this changes the order in which we
look for HID register address from
(device tree -> platform_data -> ACPI) to
(platform data -> device tree -> ACPI)

Quoted 4 lines

I do.
We would like to discourage use of legacy platform data in favour of Device Tree / ACPI.

Sure, I can prioritize both ACPI/device tree data over platform_data if that is more desirable. I was just trying to retain the past behavior where it falls back to ACPI only if there is no platform_data, and (wrongly) assumed that we might want the same for device tree data.

Quoted 29 lines

Quoted 26 lines

drivers/hid/i2c-hid/i2c-hid.c +1 -1

+static int i2c_hid_fwnode_probe(struct i2c_client *client,
              struct i2c_hid_platform_data *pdata)
 {
      struct device *dev = &client->dev;
      u32 val;
      int ret;
 
-     ret = of_property_read_u32(dev->of_node, "hid-descr-addr",

&val);

(quoted diff) +0 -2

-     if (ret) {
-             dev_err(&client->dev, "HID register address not

provided\n");

drivers/hid/i2c-hid/i2c-hid.c +0 -4

-             return -ENODEV;
-     }
-     if (val >> 16) {
-             dev_err(&client->dev, "Bad HID register address:

0x%08x\n",

drivers/hid/i2c-hid/i2c-hid.c +5 -2

-                     val);
-             return -EINVAL;
+     ret = device_property_read_u32(dev, "hid-descr-addr", &val);
+     if (ret || val >> 16) {
+             /* Couldn't read using fwnode, try ACPI next */
+             if (!i2c_hid_acpi_pdata(client, pdata)) {
+                     dev_err(dev, "Bad/Not provided HID register

address\n");

drivers/hid/i2c-hid/i2c-hid.c static inline int i2c_hid_acpi_pdata(struct i2c_client *client, · line 924

Quoted 2 lines

drivers/hid/i2c-hid/i2c-hid.c +2 -0

+                     return -ENODEV;
+             }

Quoted 1 line

Why not just replace of_ calls by device_ ones?

I assume you mean why not just replace

ret = of_property_read_u32(dev->of_node, "post-power-on-delay-ms", &val); with
ret = device_property_read_u32(dev, "post-power-on-delay-ms", &val);

in the function i2c_hid_of_probe()

Sure, that was the original idea that I started with. But I also needed to make sure that the i2c_hid_of_probe() function does get called for ACPI case. Then it made sense to rename it to "fwnode", and then to do things more generically in that function. So I ended up changing the i2c_hid_of_probe() to more generic i2c_hid_fwnode_probe() to end up with what I have. I think I acknowledge that this is changing more than what is needed, so I think yes, dumping this plan makes sense.

The other plan that I had pondered over was to simply read this property from the ACPI using device_property_read_u32 in i2c_hid_acpi_pdata() without changing anything else. Note that this would mean that we'd retain the of_property_read_*() call for the device tree case. Now that I think about it again, this looks like a more simpler, easier, and acceptable to do? Please let me know if this sounds reasonable to do, or if there are any other suggestions?

Quoted 11 lines

      }
      pdata->hid_descriptor_address = val;

- ret = of_property_read_u32(dev->of_node, "post-power-on-
delay-ms",

(quoted diff) +1 -1

-                                &val);
+     ret = device_property_read_u32(dev, "post-power-on-delay-ms",

&val);
      if (!ret)
              pdata->post_power_delay_ms = val;

Quoted 7 lines

Quoted 3 lines
      return 0;
}
Looking how ACPI support is established in the driver, I would rather NAK this change. Is there any _actual_ hardware on the wild with such properties?

As I mentioned above, I think my patch changed the _DSD and the HID register address asignment, which I did not want to do roiginally. Yes, I do have (fairly new) wacom touchscreen hardware, that wants a delay after the reset, that I want to be able to specify using the "post-power-on-delay-ms".

Thanks & Best Regards,

Rajat

Quoted 9 lines

HID protocol for ACPI is described in [1] where nothing is about _DSD.
[1]: https://docs.microsoft.com/en-us/windows-hardware/drivers/hid/plug-
and-play-support-and-power-management
--
Andy Shevchenko <[email protected]> Intel Finland Oy

Rajat Jain

author

2017-10-02 21:32 UTC #5 lore raw

PATCH 1/1 HID: i2c-hid: Allow ACPI systems to specify "post-power-on-delay-ms"

In reply to Andy Shevchenko

The property "post-power-on-delay-ms"" allows a platform to specify the delay needed after power-on, but only via device trees. Thus allow ACPI systems to also provide the same information.

Signed-off-by Rajat Jain <[email protected]>

---
 drivers/hid/i2c-hid/i2c-hid.c | 5 +++++
 1 file changed, 5 insertions(+)

drivers/hid/i2c-hid/i2c-hid.c +5 -0

index 77396145d2d0..97405156315a 100644
@@ -865,6 +865,7 @@ static int i2c_hid_acpi_pdata(struct i2c_client *client,
 	union acpi_object *obj;
 	struct acpi_device *adev;
 	acpi_handle handle;
+	u32 val;
 
 	handle = ACPI_HANDLE(&client->dev);
 	if (!handle || acpi_bus_get_device(handle, &adev))

@@ -880,6 +881,10 @@ static int i2c_hid_acpi_pdata(struct i2c_client *client,
 	pdata->hid_descriptor_address = obj->integer.value;
 	ACPI_FREE(obj);
 
+	if (!device_property_read_u32(&client->dev, "post-power-on-delay-ms",
+				      &val))
+		pdata->post_power_delay_ms = val;
+
 	return 0;
 }

Signature · 1 line

2.14.2.822.g60be5d43e6-goog

Andy Shevchenko

2017-10-03 09:28 UTC #6 lore raw

Re: [PATCH] HID: i2c-hid: Allow ACPI systems to specify "post-power-on-delay-ms"

+ Cc: Jarkko, he spent a lot of nice hours to debug i2c HID touchscreen devices on x86 ACPI enabled platforms, so, he might have a better idea or some comments.

Quoted 3 lines · Rajat Jain

The property "post-power-on-delay-ms"" allows a platform to specify the delay needed after power-on, but only via device trees. Thus allow ACPI systems to also provide the same information.

This one is even less acceptable (in given form), see below why.

drivers/hid/i2c-hid/i2c-hid.c static int i2c_hid_acpi_pdata(struct i2c_client *client, · line 885

Quoted 3 lines

(quoted diff) +1 -0
+	if (!device_property_read_u32(&client->dev, "post-power-on-
delay-ms",
drivers/hid/i2c-hid/i2c-hid.c +1 -0
+				      &val))

The main idea behind unified device properties API is to provide a way which will be resource provider agnostic, i.e. callers will get data in some kind of generic way independently on the source of it.

Since I2C HID protocol is well defined by Microsoft and it doesn't involve _DSD, you make here even more gnostic solution.

Besides the fact, each property must be registered in Device Tree bindings (yes, even if it's going to be used for ACPI enabled platforms initially).

Thus, _if_ (and only if) we have no other solution, you need to clean up your first version and send it as v3.

Don't forget to add a version to the patch (git-format-patch has a command line option to make this simpler).

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Rajat Jain

2017-10-03 18:24 UTC #7 lore raw

Re: [PATCH] HID: i2c-hid: Allow ACPI systems to specify "post-power-on-delay-ms"

Quoted 3 lines · Andy Shevchenko

+ Cc: Jarkko, he spent a lot of nice hours to debug i2c HID touchscreen devices on x86 ACPI enabled platforms, so, he might have a better idea or some comments.

Thanks Andy, I'll look out for any suggestions on inputs from Jarkko.

Quoted 12 lines · Rajat Jain

Quoted 3 lines
The property "post-power-on-delay-ms"" allows a platform to specify the delay needed after power-on, but only via device trees. Thus allow ACPI systems to also provide the same information.
This one is even less acceptable (in given form), see below why.
Quoted 3 lines
+ if (!device_property_read_u32(&client->dev, "post-power-on-
delay-ms",
+ &val))

Quoted 6 lines

The main idea behind unified device properties API is to provide a way which will be resource provider agnostic, i.e. callers will get data in some kind of generic way independently on the source of it.
Since I2C HID protocol is well defined by Microsoft and it doesn't involve _DSD, you make here even more gnostic solution.

Agree. Since I don't understand the HID protocol (or _DSD for that matter) that well, and hence I want to solve my problem without disturbing any other code. I've sent yet another iteration (v3) that introduces a new function to parse the common properties, please let me know if this is any good.

Quoted 4 lines

Besides the fact, each property must be registered in Device Tree bindings (yes, even if it's going to be used for ACPI enabled platforms initially).

The property was already documented, however, thanks for reminding, I cleaned up the doc to indicate that regulator property is not a requirement for the delay property.

Thanks & Best Regards,

Rajat

Quoted 10 lines

Thus, _if_ (and only if) we have no other solution, you need to clean up your first version and send it as v3.
Don't forget to add a version to the patch (git-format-patch has a command line option to make this simpler).
--
Andy Shevchenko <[email protected]> Intel Finland Oy

Rajat Jain

author

2017-10-03 18:19 UTC #8 lore raw

PATCH v3 1/1 HID: i2c-hid: Allow ACPI systems to specify "post-power-on-delay-ms"

In reply to Rajat Jain

The property "post-power-on-delay-ms" allows a platform to specify the delay needed after power-on, but only via device trees currently. Use device_property_* instead of of_* reads to allow ACPI systems to also provide the same information. This is useful for Wacom hardware on ACPI systems.

Signed-off-by Rajat Jain <[email protected]>

---
v3: introduce i2c_hid_fwnode_probe() to parse common device_properties.
Also fixup the binding doc to indicate that regulator isn't a requirement for "post-power-on-delay-ms"
v2: Don't change any other existing logic, only read "post-power-on-delay-ms"
in i2c_hid_acpi_pdata() also.
v1: Try to unify everything using device properties, convert i2c_hid_of_probe()
to i2c_hid_fwnode_probe() and call i2c_hid_acpi_pdata() if can't get HID register address using device_property.

 .../devicetree/bindings/input/hid-over-i2c.txt         |  2 +-
 drivers/hid/i2c-hid/i2c-hid.c                          | 18 +++++++++++++-----
 2 files changed, 14 insertions(+), 6 deletions(-)

Documentation/devicetree/bindings/input/hid-over-i2c.txt +1 -1

index 28e8bd8b7d64..4d3da9d91de4 100644
@@ -31,7 +31,7 @@ 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 powering it on, before it is ready for communication.
 
 Example:

drivers/hid/i2c-hid/i2c-hid.c +13 -5

index 77396145d2d0..741a457dfe61 100644
@@ -928,11 +928,6 @@ static int i2c_hid_of_probe(struct i2c_client *client,
 	}
 	pdata->hid_descriptor_address = val;
 
-	ret = of_property_read_u32(dev->of_node, "post-power-on-delay-ms",
-				   &val);
-	if (!ret)
-		pdata->post_power_delay_ms = val;
-
 	return 0;
 }

@@ -949,6 +944,16 @@ static inline int i2c_hid_of_probe(struct i2c_client *client,
 }
 #endif
 
+static void i2c_hid_fwnode_probe(struct i2c_client *client,
+				 struct i2c_hid_platform_data *pdata)
+{
+	u32 val;
+
+	if (!device_property_read_u32(&client->dev, "post-power-on-delay-ms",
+				      &val))
+		pdata->post_power_delay_ms = val;
+}
+
 static int i2c_hid_probe(struct i2c_client *client,
 			 const struct i2c_device_id *dev_id)
 {

@@ -992,6 +997,9 @@ static int i2c_hid_probe(struct i2c_client *client,
 		ihid->pdata = *platform_data;
 	}
 
+	/* Parse platform agnostic common properties from ACPI / device tree */
+	i2c_hid_fwnode_probe(client, &ihid->pdata);
+
 	ihid->pdata.supply = devm_regulator_get(&client->dev, "vdd");
 	if (IS_ERR(ihid->pdata.supply)) {
 		ret = PTR_ERR(ihid->pdata.supply);

Signature · 1 line

2.14.2.822.g60be5d43e6-goog

Jiri Kosina

maintainer

2017-11-21 12:30 UTC #9 lore raw

Re: [PATCH v3] HID: i2c-hid: Allow ACPI systems to specify "post-power-on-delay-ms"

Quoted 7 lines · Rajat Jain

The property "post-power-on-delay-ms" allows a platform to specify the delay needed after power-on, but only via device trees currently. Use device_property_* instead of of_* reads to allow ACPI systems to also provide the same information. This is useful for Wacom hardware on ACPI systems.
Signed-off-by Rajat Jain <[email protected]>

Quoted 9 lines

---
v3: introduce i2c_hid_fwnode_probe() to parse common device_properties.
Also fixup the binding doc to indicate that regulator isn't a requirement for "post-power-on-delay-ms"
v2: Don't change any other existing logic, only read "post-power-on-delay-ms"
in i2c_hid_acpi_pdata() also.
v1: Try to unify everything using device properties, convert i2c_hid_of_probe()
to i2c_hid_fwnode_probe() and call i2c_hid_acpi_pdata() if can't get HID register address using device_property.

Queued now for 4.16 merge window.

Signature · 2 lines

Jiri Kosina
SUSE Labs