Messages in this thread | | | Date | Fri, 29 Sep 2017 17:08:43 -0700 | From | Brian Norris <> | Subject | Re: [PATCH] HID: i2c-hid: Use device properties (instead of device tree) |
| |
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 >
| |