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

[Hans de Goede]

Hi,

On 5/22/21 7:37 PM, Jonathan Cameron wrote:
> 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.

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);
>>  }
>