Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1742385 > unrolled thread

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

Started byRajat Jain <rajatja@google.com>
First post2017-09-30 00:50 +0200
Last post2017-10-01 18:20 +0200
Articles 3 — 3 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] HID: i2c-hid: Use device properties (instead of device tree) Rajat Jain <rajatja@google.com> - 2017-09-30 00:50 +0200
    Re: [PATCH] HID: i2c-hid: Use device properties (instead of device  tree) Brian Norris <briannorris@chromium.org> - 2017-09-30 02:10 +0200
    Re: [PATCH] HID: i2c-hid: Use device properties (instead of device  tree) Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-10-01 18:20 +0200

#1742385 — [PATCH] HID: i2c-hid: Use device properties (instead of device tree)

FromRajat Jain <rajatja@google.com>
Date2017-09-30 00:50 +0200
Subject[PATCH] HID: i2c-hid: Use device properties (instead of device tree)
Message-ID<uvcgx-3Rx-11@gated-at.bofh.it>
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 <rajatja@google.com>
---
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(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 77396145d2d0..718afceb2395 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -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");
-- 
2.14.2.822.g60be5d43e6-goog

[toc] | [next] | [standalone]

#1742414 — Re: [PATCH] HID: i2c-hid: Use device properties (instead of device tree)

FromBrian Norris <briannorris@chromium.org>
Date2017-09-30 02:10 +0200
SubjectRe: [PATCH] HID: i2c-hid: Use device properties (instead of device tree)
Message-ID<uvdvX-4Sy-5@gated-at.bofh.it>
In reply to#1742385
Hi Rajat,

On Fri, Sep 29, 2017 at 03:44:41PM -0700, Rajat Jain wrote:
> 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 <rajatja@google.com>
> ---
> 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(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 77396145d2d0..718afceb2395 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -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) {

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.

> +		/* 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.

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

This should propagate the error code from i2c_hid_acpi_pdata().

> +		}
>  	}
>  	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).

>  
> -	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;
>  	}

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

>  
>  	ihid->pdata.supply = devm_regulator_get(&client->dev, "vdd");
> -- 
> 2.14.2.822.g60be5d43e6-goog
>

[toc] | [prev] | [next] | [standalone]

#1742842 — Re: [PATCH] HID: i2c-hid: Use device properties (instead of device tree)

FromAndy Shevchenko <andriy.shevchenko@linux.intel.com>
Date2017-10-01 18:20 +0200
SubjectRe: [PATCH] HID: i2c-hid: Use device properties (instead of device tree)
Message-ID<uvP8d-4kl-3@gated-at.bofh.it>
In reply to#1742385
On Fri, 2017-09-29 at 15:44 -0700, Rajat Jain wrote:
> 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. 

> +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;
> +		}

Why not just replace of_ calls by device_ ones?

>  	}
>  	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;
>  }
> 

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

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web