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