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@xxxxxxxxxx> > --- > 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html