| Message ID | 20210521171418.393871-5-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iio: accel: bmc150: Add support for yoga's with dual accelerometers with an ACPI HID of DUAL250E | expand |
On Fri, 21 May 2021 19:14:14 +0200 Hans de Goede <hdegoede@redhat.com> wrote: > The Lenovo Yoga 300-11IBR has a ACPI fwnode with a HID of DUAL250E > which contains I2C and IRQ resources for 2 accelerometers, 1 in the > display and one in the base of the device. Add support for this. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/iio/accel/bmc150-accel-i2c.c | 19 ++++++++++++------- > 1 file changed, 12 insertions(+), 7 deletions(-) > > diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c > index e24ce28a4660..b81e4005788e 100644 > --- a/drivers/iio/accel/bmc150-accel-i2c.c > +++ b/drivers/iio/accel/bmc150-accel-i2c.c > @@ -24,6 +24,7 @@ > #ifdef CONFIG_ACPI > static const struct acpi_device_id bmc150_acpi_dual_accel_ids[] = { > {"BOSC0200"}, > + {"DUAL250E"}, > { }, > }; > > @@ -35,21 +36,24 @@ static void bmc150_acpi_dual_accel_probe(struct i2c_client *client) > { > struct acpi_device *adev = ACPI_COMPANION(&client->dev); > struct i2c_client *second_dev; > + char dev_name[16]; I'm a bit in two minds about having a fixed length array for this. Obviously this is always big enough (I think a bit too big), but it might be a place where a future bug is introduced. Perhaps it's worth the dance of a kasprintf and kfree, to avoid that possibility? > struct i2c_board_info board_info = { > .type = "bmc150_accel", > - /* > - * The 2nd accel sits in the base of 2-in-1s. Note this > - * name is static, as there should never be more then 1 > - * BOSC0200 ACPI node with 2 accelerometers in it. > - */ > - .dev_name = "BOSC0200:base", > + .dev_name = dev_name, > .fwnode = client->dev.fwnode, > - .irq = -ENOENT, > }; > > if (acpi_match_device_ids(adev, bmc150_acpi_dual_accel_ids)) > return; > > + /* > + * The 2nd accel sits in the base of 2-in-1s. The suffix is static, as > + * there should never be more then 1 ACPI node with 2 accelerometers in it. > + */ > + snprintf(dev_name, sizeof(dev_name), "%s:base", acpi_device_hid(adev)); > + > + board_info.irq = acpi_dev_gpio_irq_get_by(adev, NULL, 1); > + > second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info); > if (!IS_ERR(second_dev)) > bmc150_set_second_device(client, second_dev); > @@ -114,6 +118,7 @@ static const struct acpi_device_id bmc150_accel_acpi_match[] = { > {"BMA222E", bma222e}, > {"BMA0280", bma280}, > {"BOSC0200"}, > + {"DUAL250E"}, > { }, > }; > MODULE_DEVICE_TABLE(acpi, bmc150_accel_acpi_match);
Hi, On 5/22/21 7:43 PM, Jonathan Cameron wrote: > On Fri, 21 May 2021 19:14:14 +0200 > Hans de Goede <hdegoede@redhat.com> wrote: > >> The Lenovo Yoga 300-11IBR has a ACPI fwnode with a HID of DUAL250E >> which contains I2C and IRQ resources for 2 accelerometers, 1 in the >> display and one in the base of the device. Add support for this. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/iio/accel/bmc150-accel-i2c.c | 19 ++++++++++++------- >> 1 file changed, 12 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c >> index e24ce28a4660..b81e4005788e 100644 >> --- a/drivers/iio/accel/bmc150-accel-i2c.c >> +++ b/drivers/iio/accel/bmc150-accel-i2c.c >> @@ -24,6 +24,7 @@ >> #ifdef CONFIG_ACPI >> static const struct acpi_device_id bmc150_acpi_dual_accel_ids[] = { >> {"BOSC0200"}, >> + {"DUAL250E"}, >> { }, >> }; >> >> @@ -35,21 +36,24 @@ static void bmc150_acpi_dual_accel_probe(struct i2c_client *client) >> { >> struct acpi_device *adev = ACPI_COMPANION(&client->dev); >> struct i2c_client *second_dev; >> + char dev_name[16]; > > I'm a bit in two minds about having a fixed length array for this. > Obviously this is always big enough (I think a bit too big), but it > might be a place where a future bug is introduced. Perhaps it's worth the dance > of a kasprintf and kfree, to avoid that possibility? I would prefer to keep this as is, using malloc + free always leads to problems if an error-exit path shows up between the 2. But if you've a strong preference for switching to kasprintf + kfree I can do that for v2. Regards, Hans > >> struct i2c_board_info board_info = { >> .type = "bmc150_accel", >> - /* >> - * The 2nd accel sits in the base of 2-in-1s. Note this >> - * name is static, as there should never be more then 1 >> - * BOSC0200 ACPI node with 2 accelerometers in it. >> - */ >> - .dev_name = "BOSC0200:base", >> + .dev_name = dev_name, >> .fwnode = client->dev.fwnode, >> - .irq = -ENOENT, >> }; >> >> if (acpi_match_device_ids(adev, bmc150_acpi_dual_accel_ids)) >> return; >> >> + /* >> + * The 2nd accel sits in the base of 2-in-1s. The suffix is static, as >> + * there should never be more then 1 ACPI node with 2 accelerometers in it. >> + */ >> + snprintf(dev_name, sizeof(dev_name), "%s:base", acpi_device_hid(adev)); >> + >> + board_info.irq = acpi_dev_gpio_irq_get_by(adev, NULL, 1); >> + >> second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info); >> if (!IS_ERR(second_dev)) >> bmc150_set_second_device(client, second_dev); >> @@ -114,6 +118,7 @@ static const struct acpi_device_id bmc150_accel_acpi_match[] = { >> {"BMA222E", bma222e}, >> {"BMA0280", bma280}, >> {"BOSC0200"}, >> + {"DUAL250E"}, >> { }, >> }; >> MODULE_DEVICE_TABLE(acpi, bmc150_accel_acpi_match); >
On Sat, 22 May 2021 19:44:55 +0200 Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 5/22/21 7:43 PM, Jonathan Cameron wrote: > > On Fri, 21 May 2021 19:14:14 +0200 > > Hans de Goede <hdegoede@redhat.com> wrote: > > > >> The Lenovo Yoga 300-11IBR has a ACPI fwnode with a HID of DUAL250E > >> which contains I2C and IRQ resources for 2 accelerometers, 1 in the > >> display and one in the base of the device. Add support for this. > >> > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >> --- > >> drivers/iio/accel/bmc150-accel-i2c.c | 19 ++++++++++++------- > >> 1 file changed, 12 insertions(+), 7 deletions(-) > >> > >> diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c > >> index e24ce28a4660..b81e4005788e 100644 > >> --- a/drivers/iio/accel/bmc150-accel-i2c.c > >> +++ b/drivers/iio/accel/bmc150-accel-i2c.c > >> @@ -24,6 +24,7 @@ > >> #ifdef CONFIG_ACPI > >> static const struct acpi_device_id bmc150_acpi_dual_accel_ids[] = { > >> {"BOSC0200"}, > >> + {"DUAL250E"}, > >> { }, > >> }; > >> > >> @@ -35,21 +36,24 @@ static void bmc150_acpi_dual_accel_probe(struct i2c_client *client) > >> { > >> struct acpi_device *adev = ACPI_COMPANION(&client->dev); > >> struct i2c_client *second_dev; > >> + char dev_name[16]; > > > > I'm a bit in two minds about having a fixed length array for this. > > Obviously this is always big enough (I think a bit too big), but it > > might be a place where a future bug is introduced. Perhaps it's worth the dance > > of a kasprintf and kfree, to avoid that possibility? > > I would prefer to keep this as is, using malloc + free always leads > to problems if an error-exit path shows up between the 2. > > But if you've a strong preference for switching to > kasprintf + kfree I can do that for v2. Lets leave it as is and I get to be smug if we ever get a bug as a result (given that way the one you suggest can't happen, so I can't be proved wrong :) J > > Regards, > > Hans > > > > > > >> struct i2c_board_info board_info = { > >> .type = "bmc150_accel", > >> - /* > >> - * The 2nd accel sits in the base of 2-in-1s. Note this > >> - * name is static, as there should never be more then 1 > >> - * BOSC0200 ACPI node with 2 accelerometers in it. > >> - */ > >> - .dev_name = "BOSC0200:base", > >> + .dev_name = dev_name, > >> .fwnode = client->dev.fwnode, > >> - .irq = -ENOENT, > >> }; > >> > >> if (acpi_match_device_ids(adev, bmc150_acpi_dual_accel_ids)) > >> return; > >> > >> + /* > >> + * The 2nd accel sits in the base of 2-in-1s. The suffix is static, as > >> + * there should never be more then 1 ACPI node with 2 accelerometers in it. > >> + */ > >> + snprintf(dev_name, sizeof(dev_name), "%s:base", acpi_device_hid(adev)); > >> + > >> + board_info.irq = acpi_dev_gpio_irq_get_by(adev, NULL, 1); > >> + > >> second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info); > >> if (!IS_ERR(second_dev)) > >> bmc150_set_second_device(client, second_dev); > >> @@ -114,6 +118,7 @@ static const struct acpi_device_id bmc150_accel_acpi_match[] = { > >> {"BMA222E", bma222e}, > >> {"BMA0280", bma280}, > >> {"BOSC0200"}, > >> + {"DUAL250E"}, > >> { }, > >> }; > >> MODULE_DEVICE_TABLE(acpi, bmc150_accel_acpi_match); > > >
On Fri, May 21, 2021 at 11:23 PM Hans de Goede <hdegoede@redhat.com> wrote: > > The Lenovo Yoga 300-11IBR has a ACPI fwnode with a HID of DUAL250E > which contains I2C and IRQ resources for 2 accelerometers, 1 in the > display and one in the base of the device. Add support for this. ... > + board_info.irq = acpi_dev_gpio_irq_get_by(adev, NULL, 1); If NULL will be there after the series, why not to use acpi_dev_gpio_irq_get() directly?
Hi, On 5/22/21 8:11 PM, Andy Shevchenko wrote: > On Fri, May 21, 2021 at 11:23 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> The Lenovo Yoga 300-11IBR has a ACPI fwnode with a HID of DUAL250E >> which contains I2C and IRQ resources for 2 accelerometers, 1 in the >> display and one in the base of the device. Add support for this. > > ... > >> + board_info.irq = acpi_dev_gpio_irq_get_by(adev, NULL, 1); > > If NULL will be there after the series, why not to use > acpi_dev_gpio_irq_get() directly? I looked in drivers/gpio/gpiolib-acpi.c to see what is available and that is an inline helper in include/linux/acpi.h, so I missed it. I'll switch over in v2. Regards, Hans
diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c index e24ce28a4660..b81e4005788e 100644 --- a/drivers/iio/accel/bmc150-accel-i2c.c +++ b/drivers/iio/accel/bmc150-accel-i2c.c @@ -24,6 +24,7 @@ #ifdef CONFIG_ACPI static const struct acpi_device_id bmc150_acpi_dual_accel_ids[] = { {"BOSC0200"}, + {"DUAL250E"}, { }, }; @@ -35,21 +36,24 @@ static void bmc150_acpi_dual_accel_probe(struct i2c_client *client) { struct acpi_device *adev = ACPI_COMPANION(&client->dev); struct i2c_client *second_dev; + char dev_name[16]; struct i2c_board_info board_info = { .type = "bmc150_accel", - /* - * The 2nd accel sits in the base of 2-in-1s. Note this - * name is static, as there should never be more then 1 - * BOSC0200 ACPI node with 2 accelerometers in it. - */ - .dev_name = "BOSC0200:base", + .dev_name = dev_name, .fwnode = client->dev.fwnode, - .irq = -ENOENT, }; if (acpi_match_device_ids(adev, bmc150_acpi_dual_accel_ids)) return; + /* + * The 2nd accel sits in the base of 2-in-1s. The suffix is static, as + * there should never be more then 1 ACPI node with 2 accelerometers in it. + */ + snprintf(dev_name, sizeof(dev_name), "%s:base", acpi_device_hid(adev)); + + board_info.irq = acpi_dev_gpio_irq_get_by(adev, NULL, 1); + second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info); if (!IS_ERR(second_dev)) bmc150_set_second_device(client, second_dev); @@ -114,6 +118,7 @@ static const struct acpi_device_id bmc150_accel_acpi_match[] = { {"BMA222E", bma222e}, {"BMA0280", bma280}, {"BOSC0200"}, + {"DUAL250E"}, { }, }; MODULE_DEVICE_TABLE(acpi, bmc150_accel_acpi_match);
The Lenovo Yoga 300-11IBR has a ACPI fwnode with a HID of DUAL250E which contains I2C and IRQ resources for 2 accelerometers, 1 in the display and one in the base of the device. Add support for this. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/iio/accel/bmc150-accel-i2c.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)