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 <[email protected]>
> ---
> 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
>
