Re: [PATCH v2 2/6] iio: imu: bmi270: Provide chip info as configuration structure

[Justin Weiss]

Jonathan Cameron <jic23@xxxxxxxxxx> writes:

> On Fri, 18 Oct 2024 16:36:08 -0700
> Justin Weiss <justin@xxxxxxxxxxxxxxx> wrote:
>
>> Prepare the bmi270 driver to support similar devices like the bmi260.
>> 
>> Signed-off-by: Justin Weiss <justin@xxxxxxxxxxxxxxx>
> One thing in here.  The enum ID thing tends to end up costing more than
> the benefit it brings, so for newer drivers preferred option is separate
> structure instances rather than an array.

That makes sense to me, even considering your comments on patch #4. I'll
switch to separate structures here and keep the if / else in that later
patch.

Justin

>> ---
>>  drivers/iio/imu/bmi270/bmi270.h      | 15 ++++++++++++++-
>>  drivers/iio/imu/bmi270/bmi270_core.c | 18 +++++++++++++++---
>>  drivers/iio/imu/bmi270/bmi270_i2c.c  | 11 ++++++++---
>>  drivers/iio/imu/bmi270/bmi270_spi.c  | 11 ++++++++---
>>  4 files changed, 45 insertions(+), 10 deletions(-)
>> 
>> diff --git a/drivers/iio/imu/bmi270/bmi270.h b/drivers/iio/imu/bmi270/bmi270.h
>> index 8ac20ad7ee94..2e8d85a4e419 100644
>> --- a/drivers/iio/imu/bmi270/bmi270.h
>> +++ b/drivers/iio/imu/bmi270/bmi270.h
>> @@ -10,10 +10,23 @@ struct device;
>>  struct bmi270_data {
>>  	struct device *dev;
>>  	struct regmap *regmap;
>> +	const struct bmi270_chip_info *chip_info;
>> +};
>> +
>> +enum bmi270_device_type {
>> +	BMI270,
>
> Whilst quite a few drivers do it this way, over time we've found that it's
> much easier to just skip the array of structures and have independent ones.
> Increase the extern lines to one per supported device, but removes
> need for an enum here and generally gives slightly more readable code.
>
>
>> +};
>
>>  };
>>  
>>  static const struct of_device_id bmi270_of_match[] = {
>> -	{ .compatible = "bosch,bmi270" },
>> +	{ .compatible = "bosch,bmi270", .data = &bmi270_chip_info[BMI270] },
>
> After dropping the enum this just becomes &bmi270_chip_info
> and later you'll add bmi260_chip_info etc.
>
>>  	{ }
>>  };
>>