| Message ID | 20170929224441.98176-1-rajatja@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 >> -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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");
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(-)