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) {
+ /* 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");
--
2.14.2.822.g60be5d43e6-goog
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
>