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

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

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

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

[Brian Norris]

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
> 

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

[Rajat Jain]

On Fri, Sep 29, 2017 at 5:08 PM, Brian Norris <briannorris@chromium.org> wrote:
> 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.


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

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

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

[Andy Shevchenko]

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

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

[Rajat Jain]

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.

On Sun, Oct 1, 2017 at 9:18 AM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> 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.

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.

>
>> +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?

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?

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

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


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

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

[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.

Signed-off-by: Rajat Jain <rajatja@google.com>
---
 drivers/hid/i2c-hid/i2c-hid.c | 5 +++++
 1 file changed, 5 insertions(+)

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

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

[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.

On Mon, 2017-10-02 at 14:32 -0700, Rajat Jain wrote:
> 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.

> +	if (!device_property_read_u32(&client->dev, "post-power-on-
> delay-ms",
> +				      &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).

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

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

[Rajat Jain]

On Tue, Oct 3, 2017 at 2:28 AM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> + 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.

>
> On Mon, 2017-10-02 at 14:32 -0700, Rajat Jain wrote:
>> 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.
>
>> +     if (!device_property_read_u32(&client->dev, "post-power-on-
>> delay-ms",
>> +                                   &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.

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.

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

>
> 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 <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy

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

[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 <rajatja@google.com>
---
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(-)

diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.txt b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
index 28e8bd8b7d64..4d3da9d91de4 100644
--- a/Documentation/devicetree/bindings/input/hid-over-i2c.txt
+++ b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
@@ -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:
 
diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 77396145d2d0..741a457dfe61 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -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);
-- 
2.14.2.822.g60be5d43e6-goog

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

[Jiri Kosina]

On Tue, 3 Oct 2017, Rajat Jain wrote:

> 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 <rajatja@google.com>
> ---
> 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.

-- 
Jiri Kosina
SUSE Labs