From: Rajat Jain <hidden> Date: 2017-09-29 22:45:03
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 <redacted>
---
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(-)
From: Brian Norris <briannorris@chromium.org> Date: 2017-09-30 00:08:49
Hi Rajat,
On Fri, Sep 29, 2017 at 03:44:41PM -0700, Rajat Jain wrote:
quoted hunk
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 <redacted>
---
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(-)
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).
@@ -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
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: 2017-10-01 16:18:56
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 [off-list ref]
Intel Finland Oy
From: Rajat Jain <hidden> Date: 2017-10-02 19:24:09
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
[off-list ref] wrote:
On Fri, 2017-09-29 at 15:44 -0700, Rajat Jain wrote:
quoted
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.
quoted
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.
quoted
+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?
quoted
}
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
From: Rajat Jain <hidden> Date: 2017-10-02 19:28:04
On Fri, Sep 29, 2017 at 5:08 PM, Brian Norris [off-list ref] wrote:
Hi Rajat,
On Fri, Sep 29, 2017 at 03:44:41PM -0700, Rajat Jain wrote:
quoted
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 <redacted>
---
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(-)
This should propagate the error code from i2c_hid_acpi_pdata().
quoted
+ }
}
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).
@@ -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
From: Rajat Jain <hidden> Date: 2017-10-02 21:32:28
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 <redacted>
---
drivers/hid/i2c-hid/i2c-hid.c | 5 +++++
1 file changed, 5 insertions(+)
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: 2017-10-03 09:29:01
+ 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 [off-list ref]
Intel Finland Oy
From: Rajat Jain <hidden> Date: 2017-10-03 18:19:37
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 <redacted>
---
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(-)
@@ -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:
@@ -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;-return0;}
@@ -949,6 +944,16 @@ static inline int i2c_hid_of_probe(struct i2c_client *client,}#endif+staticvoidi2c_hid_fwnode_probe(structi2c_client*client,+structi2c_hid_platform_data*pdata)+{+u32val;++if(!device_property_read_u32(&client->dev,"post-power-on-delay-ms",+&val))+pdata->post_power_delay_ms=val;+}+staticinti2c_hid_probe(structi2c_client*client,conststructi2c_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);
From: Rajat Jain <hidden> Date: 2017-10-03 18:24:31
On Tue, Oct 3, 2017 at 2:28 AM, Andy Shevchenko
[off-list ref] 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:
quoted
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
+ 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 [off-list ref]
Intel Finland Oy
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 <redacted>
---
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