| Message ID | 20210521171418.393871-4-hdegoede@redhat.com |
|---|---|
| State | Accepted |
| Commit | 42d1c1dc0f91cae51781b4c5ccdb26989d7521dc |
| 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:13 +0200 Hans de Goede <hdegoede@redhat.com> wrote: > Move the check for a second ACPI device for BOSC0200 ACPI fwnodes into > a new bmc150_acpi_dual_accel_probe() helper function. > > This is a preparation patch for adding support for a new "DUAL250E" ACPI > Hardware-ID (HID) used on some devices. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> A few places I'd like comments rewrapped on basis of still having a minor preference for a 80 chars limit unless there is a reason to do otherwise. If this is all that turns up in the series, I can do that whilst applying. Thanks, Jonathan > --- > drivers/iio/accel/bmc150-accel-i2c.c | 80 +++++++++++++++++----------- > 1 file changed, 49 insertions(+), 31 deletions(-) > > diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c > index 2afaae0294ee..e24ce28a4660 100644 > --- a/drivers/iio/accel/bmc150-accel-i2c.c > +++ b/drivers/iio/accel/bmc150-accel-i2c.c > @@ -21,6 +21,51 @@ > > #include "bmc150-accel.h" > > +#ifdef CONFIG_ACPI > +static const struct acpi_device_id bmc150_acpi_dual_accel_ids[] = { > + {"BOSC0200"}, > + { }, > +}; > + > +/* > + * Some acpi_devices describe 2 accelerometers in a single ACPI device, try instantiating 80 char wrap still preferred when it doesn't otherwise hurt readability. > + * a second i2c_client for an I2cSerialBusV2 ACPI resource with index 1. > + */ > +static void bmc150_acpi_dual_accel_probe(struct i2c_client *client) > +{ > + struct acpi_device *adev = ACPI_COMPANION(&client->dev); > + struct i2c_client *second_dev; > + 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. Given the lesser indent after pulling this out into a new function, it would be good to rewrap this text as nearer to 80 chars. > + */ > + .dev_name = "BOSC0200:base", > + .fwnode = client->dev.fwnode, > + .irq = -ENOENT, > + }; > + > + if (acpi_match_device_ids(adev, bmc150_acpi_dual_accel_ids)) > + return; > + > + second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info); > + if (!IS_ERR(second_dev)) > + bmc150_set_second_device(client, second_dev); > +} > + > +static void bmc150_acpi_dual_accel_remove(struct i2c_client *client) > +{ > + struct i2c_client *second_dev = bmc150_get_second_device(client); > + > + i2c_unregister_device(second_dev); > +} > +#else > +static void bmc150_acpi_dual_accel_probe(struct i2c_client *client) {} > +static void bmc150_acpi_dual_accel_remove(struct i2c_client *client) {} > +#endif > + > static int bmc150_accel_probe(struct i2c_client *client, > const struct i2c_device_id *id) > { > @@ -30,7 +75,6 @@ static int bmc150_accel_probe(struct i2c_client *client, > i2c_check_functionality(client->adapter, I2C_FUNC_I2C) || > i2c_check_functionality(client->adapter, > I2C_FUNC_SMBUS_READ_I2C_BLOCK); > - struct acpi_device __maybe_unused *adev; > int ret; > > regmap = devm_regmap_init_i2c(client, &bmc150_regmap_conf); > @@ -46,42 +90,16 @@ static int bmc150_accel_probe(struct i2c_client *client, > if (ret) > return ret; > > - /* > - * Some BOSC0200 acpi_devices describe 2 accelerometers in a single ACPI > - * device, try instantiating a second i2c_client for an I2cSerialBusV2 > - * ACPI resource with index 1. The !id check avoids recursion when > - * bmc150_accel_probe() gets called for the second client. > - */ > -#ifdef CONFIG_ACPI > - adev = ACPI_COMPANION(&client->dev); > - if (!id && adev && strcmp(acpi_device_hid(adev), "BOSC0200") == 0) { > - 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", > - .fwnode = client->dev.fwnode, > - .irq = -ENOENT, > - }; > - struct i2c_client *second_dev; > - > - second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info); > - if (!IS_ERR(second_dev)) > - bmc150_set_second_device(client, second_dev); > - } > -#endif > + /* The !id check avoids recursion when probe() gets called for the second client. */ Won't hurt readability to wrap this to 80 chars as a multiline comment. > + if (!id && has_acpi_companion(&client->dev)) > + bmc150_acpi_dual_accel_probe(client); > > return 0; > } > > static int bmc150_accel_remove(struct i2c_client *client) > { > - struct i2c_client *second_dev = bmc150_get_second_device(client); > - > - i2c_unregister_device(second_dev); > + bmc150_acpi_dual_accel_remove(client); > > return bmc150_accel_core_remove(&client->dev); > }
Hi, On 5/22/21 7:37 PM, Jonathan Cameron wrote: > On Fri, 21 May 2021 19:14:13 +0200 > Hans de Goede <hdegoede@redhat.com> wrote: > >> Move the check for a second ACPI device for BOSC0200 ACPI fwnodes into >> a new bmc150_acpi_dual_accel_probe() helper function. >> >> This is a preparation patch for adding support for a new "DUAL250E" ACPI >> Hardware-ID (HID) used on some devices. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > A few places I'd like comments rewrapped on basis of still having > a minor preference for a 80 chars limit unless there is a reason to > do otherwise. > > If this is all that turns up in the series, I can do that whilst > applying. Thank for the review. If you can do the comment rewrapping while applying that would be great, thanks. If a v2 is necessary I will take care of the rewrapping myself. Regards, Hans > > Thanks, > > Jonathan > > >> --- >> drivers/iio/accel/bmc150-accel-i2c.c | 80 +++++++++++++++++----------- >> 1 file changed, 49 insertions(+), 31 deletions(-) >> >> diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c >> index 2afaae0294ee..e24ce28a4660 100644 >> --- a/drivers/iio/accel/bmc150-accel-i2c.c >> +++ b/drivers/iio/accel/bmc150-accel-i2c.c >> @@ -21,6 +21,51 @@ >> >> #include "bmc150-accel.h" >> >> +#ifdef CONFIG_ACPI >> +static const struct acpi_device_id bmc150_acpi_dual_accel_ids[] = { >> + {"BOSC0200"}, >> + { }, >> +}; >> + >> +/* >> + * Some acpi_devices describe 2 accelerometers in a single ACPI device, try instantiating > > 80 char wrap still preferred when it doesn't otherwise hurt readability. > >> + * a second i2c_client for an I2cSerialBusV2 ACPI resource with index 1. >> + */ >> +static void bmc150_acpi_dual_accel_probe(struct i2c_client *client) >> +{ >> + struct acpi_device *adev = ACPI_COMPANION(&client->dev); >> + struct i2c_client *second_dev; >> + 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. > > Given the lesser indent after pulling this out into a new function, it would > be good to rewrap this text as nearer to 80 chars. > >> + */ >> + .dev_name = "BOSC0200:base", >> + .fwnode = client->dev.fwnode, >> + .irq = -ENOENT, >> + }; >> + >> + if (acpi_match_device_ids(adev, bmc150_acpi_dual_accel_ids)) >> + return; >> + >> + second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info); >> + if (!IS_ERR(second_dev)) >> + bmc150_set_second_device(client, second_dev); >> +} >> + >> +static void bmc150_acpi_dual_accel_remove(struct i2c_client *client) >> +{ >> + struct i2c_client *second_dev = bmc150_get_second_device(client); >> + >> + i2c_unregister_device(second_dev); >> +} >> +#else >> +static void bmc150_acpi_dual_accel_probe(struct i2c_client *client) {} >> +static void bmc150_acpi_dual_accel_remove(struct i2c_client *client) {} >> +#endif >> + >> static int bmc150_accel_probe(struct i2c_client *client, >> const struct i2c_device_id *id) >> { >> @@ -30,7 +75,6 @@ static int bmc150_accel_probe(struct i2c_client *client, >> i2c_check_functionality(client->adapter, I2C_FUNC_I2C) || >> i2c_check_functionality(client->adapter, >> I2C_FUNC_SMBUS_READ_I2C_BLOCK); >> - struct acpi_device __maybe_unused *adev; >> int ret; >> >> regmap = devm_regmap_init_i2c(client, &bmc150_regmap_conf); >> @@ -46,42 +90,16 @@ static int bmc150_accel_probe(struct i2c_client *client, >> if (ret) >> return ret; >> >> - /* >> - * Some BOSC0200 acpi_devices describe 2 accelerometers in a single ACPI >> - * device, try instantiating a second i2c_client for an I2cSerialBusV2 >> - * ACPI resource with index 1. The !id check avoids recursion when >> - * bmc150_accel_probe() gets called for the second client. >> - */ >> -#ifdef CONFIG_ACPI >> - adev = ACPI_COMPANION(&client->dev); >> - if (!id && adev && strcmp(acpi_device_hid(adev), "BOSC0200") == 0) { >> - 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", >> - .fwnode = client->dev.fwnode, >> - .irq = -ENOENT, >> - }; >> - struct i2c_client *second_dev; >> - >> - second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info); >> - if (!IS_ERR(second_dev)) >> - bmc150_set_second_device(client, second_dev); >> - } >> -#endif >> + /* The !id check avoids recursion when probe() gets called for the second client. */ > > Won't hurt readability to wrap this to 80 chars as a multiline comment. > >> + if (!id && has_acpi_companion(&client->dev)) >> + bmc150_acpi_dual_accel_probe(client); >> >> return 0; >> } >> >> static int bmc150_accel_remove(struct i2c_client *client) >> { >> - struct i2c_client *second_dev = bmc150_get_second_device(client); >> - >> - i2c_unregister_device(second_dev); >> + bmc150_acpi_dual_accel_remove(client); >> >> return bmc150_accel_core_remove(&client->dev); >> } >
On Fri, May 21, 2021 at 11:23 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Move the check for a second ACPI device for BOSC0200 ACPI fwnodes into > a new bmc150_acpi_dual_accel_probe() helper function. > > This is a preparation patch for adding support for a new "DUAL250E" ACPI > Hardware-ID (HID) used on some devices. ... > +#ifdef CONFIG_ACPI > +static const struct acpi_device_id bmc150_acpi_dual_accel_ids[] = { > + {"BOSC0200"}, > + { }, I guess it is a good chance to drop a comma. > +}; ... > + if (!IS_ERR(second_dev)) > + bmc150_set_second_device(client, second_dev); I would comment on the pattern here, but I have noticed that this code is changed in the further patches anyway.
Hi, On 5/22/21 8:09 PM, Andy Shevchenko wrote: > On Fri, May 21, 2021 at 11:23 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Move the check for a second ACPI device for BOSC0200 ACPI fwnodes into >> a new bmc150_acpi_dual_accel_probe() helper function. >> >> This is a preparation patch for adding support for a new "DUAL250E" ACPI >> Hardware-ID (HID) used on some devices. > > ... > >> +#ifdef CONFIG_ACPI >> +static const struct acpi_device_id bmc150_acpi_dual_accel_ids[] = { >> + {"BOSC0200"}, > >> + { }, > > I guess it is a good chance to drop a comma. Ack, will drop for v2. Regards, Hans > >> +}; > > ... > >> + if (!IS_ERR(second_dev)) >> + bmc150_set_second_device(client, second_dev); > > I would comment on the pattern here, but I have noticed that this code > is changed in the further patches anyway. >
diff --git a/drivers/iio/accel/bmc150-accel-i2c.c b/drivers/iio/accel/bmc150-accel-i2c.c index 2afaae0294ee..e24ce28a4660 100644 --- a/drivers/iio/accel/bmc150-accel-i2c.c +++ b/drivers/iio/accel/bmc150-accel-i2c.c @@ -21,6 +21,51 @@ #include "bmc150-accel.h" +#ifdef CONFIG_ACPI +static const struct acpi_device_id bmc150_acpi_dual_accel_ids[] = { + {"BOSC0200"}, + { }, +}; + +/* + * Some acpi_devices describe 2 accelerometers in a single ACPI device, try instantiating + * a second i2c_client for an I2cSerialBusV2 ACPI resource with index 1. + */ +static void bmc150_acpi_dual_accel_probe(struct i2c_client *client) +{ + struct acpi_device *adev = ACPI_COMPANION(&client->dev); + struct i2c_client *second_dev; + 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", + .fwnode = client->dev.fwnode, + .irq = -ENOENT, + }; + + if (acpi_match_device_ids(adev, bmc150_acpi_dual_accel_ids)) + return; + + second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info); + if (!IS_ERR(second_dev)) + bmc150_set_second_device(client, second_dev); +} + +static void bmc150_acpi_dual_accel_remove(struct i2c_client *client) +{ + struct i2c_client *second_dev = bmc150_get_second_device(client); + + i2c_unregister_device(second_dev); +} +#else +static void bmc150_acpi_dual_accel_probe(struct i2c_client *client) {} +static void bmc150_acpi_dual_accel_remove(struct i2c_client *client) {} +#endif + static int bmc150_accel_probe(struct i2c_client *client, const struct i2c_device_id *id) { @@ -30,7 +75,6 @@ static int bmc150_accel_probe(struct i2c_client *client, i2c_check_functionality(client->adapter, I2C_FUNC_I2C) || i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_READ_I2C_BLOCK); - struct acpi_device __maybe_unused *adev; int ret; regmap = devm_regmap_init_i2c(client, &bmc150_regmap_conf); @@ -46,42 +90,16 @@ static int bmc150_accel_probe(struct i2c_client *client, if (ret) return ret; - /* - * Some BOSC0200 acpi_devices describe 2 accelerometers in a single ACPI - * device, try instantiating a second i2c_client for an I2cSerialBusV2 - * ACPI resource with index 1. The !id check avoids recursion when - * bmc150_accel_probe() gets called for the second client. - */ -#ifdef CONFIG_ACPI - adev = ACPI_COMPANION(&client->dev); - if (!id && adev && strcmp(acpi_device_hid(adev), "BOSC0200") == 0) { - 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", - .fwnode = client->dev.fwnode, - .irq = -ENOENT, - }; - struct i2c_client *second_dev; - - second_dev = i2c_acpi_new_device(&client->dev, 1, &board_info); - if (!IS_ERR(second_dev)) - bmc150_set_second_device(client, second_dev); - } -#endif + /* The !id check avoids recursion when probe() gets called for the second client. */ + if (!id && has_acpi_companion(&client->dev)) + bmc150_acpi_dual_accel_probe(client); return 0; } static int bmc150_accel_remove(struct i2c_client *client) { - struct i2c_client *second_dev = bmc150_get_second_device(client); - - i2c_unregister_device(second_dev); + bmc150_acpi_dual_accel_remove(client); return bmc150_accel_core_remove(&client->dev); }
Move the check for a second ACPI device for BOSC0200 ACPI fwnodes into a new bmc150_acpi_dual_accel_probe() helper function. This is a preparation patch for adding support for a new "DUAL250E" ACPI Hardware-ID (HID) used on some devices. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/iio/accel/bmc150-accel-i2c.c | 80 +++++++++++++++++----------- 1 file changed, 49 insertions(+), 31 deletions(-)