[PATCH 3/8] iio: accel: bmc150: Move check for second ACPI device into a separate function

[Jonathan Cameron]

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