On Fri, 21 May 2021 19:14:13 +0200
Hans de Goede <hdegoede@xxxxxxxxxx> 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@xxxxxxxxxx>
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);
> }
