Re: [PATCH v3 4/6] iio: accel: adxl345: Remove single info instances

[Jonathan Cameron]

On Sat, 23 Mar 2024 12:20:28 +0000
Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote:

> Add a common array adxl3x5_chip_info and an enum for
> indexing. This allows to remove local redundantly
> initialized code in the bus specific modules.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@xxxxxxxxx>
> ---
>  drivers/iio/accel/adxl345.h      |  7 +++++++
>  drivers/iio/accel/adxl345_core.c | 12 ++++++++++++
>  drivers/iio/accel/adxl345_i2c.c  | 20 +++++---------------
>  drivers/iio/accel/adxl345_spi.c  | 20 +++++---------------
>  4 files changed, 29 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> index 6b84a2cee..de6b1767d 100644
> --- a/drivers/iio/accel/adxl345.h
> +++ b/drivers/iio/accel/adxl345.h
> @@ -26,11 +26,18 @@
>   */
>  #define ADXL375_USCALE	480000
>  
> +enum adxl345_device_type {
> +	ADXL345,
> +	ADXL375,
> +};
> +
>  struct adxl345_chip_info {
>  	const char *name;
>  	int uscale;
>  };
>  
> +extern const struct adxl345_chip_info adxl3x5_chip_info[];
> +
>  int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  		       int (*setup)(struct device*, struct regmap*));
>  
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 33424edca..e3718d0dd 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -62,6 +62,18 @@ struct adxl345_data {
>  		BIT(IIO_CHAN_INFO_SAMP_FREQ),				\
>  }
>  
> +const struct adxl345_chip_info adxl3x5_chip_info[] = {
> +	[ADXL345] = {
> +		.name = "adxl345",
> +		.uscale = ADXL345_USCALE,
> +	},
> +	[ADXL375] = {
> +		.name = "adxl375",
> +		.uscale = ADXL375_USCALE,
> +	},
> +};
> +EXPORT_SYMBOL_NS_GPL(adxl3x5_chip_info, IIO_ADXL345);

There is little advantage here form using an array.  I'd just have
two exported structures.   Then the name alone is enough in the
id tables.  And probably no need for the enum definition.

This use of arrays is an old pattern that makes little sense if the
IDs have no actual meaning and you aren't supporting lots of different
parts.  For 2 parts I'd argue definitely not worth it.

> +
>  static const struct iio_chan_spec adxl345_channels[] = {
>  	ADXL345_CHANNEL(0, X),
>  	ADXL345_CHANNEL(1, Y),
> diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c
> index 4065b8f7c..afb2d0b79 100644
> --- a/drivers/iio/accel/adxl345_i2c.c
> +++ b/drivers/iio/accel/adxl345_i2c.c
> @@ -30,32 +30,22 @@ static int adxl345_i2c_probe(struct i2c_client *client)
>  	return adxl345_core_probe(&client->dev, regmap, NULL);
>  }
>  
> -static const struct adxl345_chip_info adxl345_i2c_info = {
> -	.name = "adxl345",
> -	.uscale = ADXL345_USCALE,
> -};
> -
> -static const struct adxl345_chip_info adxl375_i2c_info = {
> -	.name = "adxl375",
> -	.uscale = ADXL375_USCALE,
> -};
> -
>  static const struct i2c_device_id adxl345_i2c_id[] = {
> -	{ "adxl345", (kernel_ulong_t)&adxl345_i2c_info },
> -	{ "adxl375", (kernel_ulong_t)&adxl375_i2c_info },
> +	{ "adxl345", (kernel_ulong_t)&adxl3x5_chip_info[ADXL345] },
> +	{ "adxl375", (kernel_ulong_t)&adxl3x5_chip_info[ADXL375] },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, adxl345_i2c_id);
>  
>  static const struct of_device_id adxl345_of_match[] = {
> -	{ .compatible = "adi,adxl345", .data = &adxl345_i2c_info },
> -	{ .compatible = "adi,adxl375", .data = &adxl375_i2c_info },
> +	{ .compatible = "adi,adxl345", .data = &adxl3x5_chip_info[ADXL345] },
> +	{ .compatible = "adi,adxl375", .data = &adxl3x5_chip_info[ADXL375] },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, adxl345_of_match);
>  
>  static const struct acpi_device_id adxl345_acpi_match[] = {
> -	{ "ADS0345", (kernel_ulong_t)&adxl345_i2c_info },
> +	{ "ADS0345", (kernel_ulong_t)&adxl3x5_chip_info[ADXL345] },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(acpi, adxl345_acpi_match);
> diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
> index 1094396ac..5c1109136 100644
> --- a/drivers/iio/accel/adxl345_spi.c
> +++ b/drivers/iio/accel/adxl345_spi.c
> @@ -46,32 +46,22 @@ static int adxl345_spi_probe(struct spi_device *spi)
>  	return adxl345_core_probe(&spi->dev, regmap, &adxl345_spi_setup);
>  }
>  
> -static const struct adxl345_chip_info adxl345_spi_info = {
> -	.name = "adxl345",
> -	.uscale = ADXL345_USCALE,
> -};
> -
> -static const struct adxl345_chip_info adxl375_spi_info = {
> -	.name = "adxl375",
> -	.uscale = ADXL375_USCALE,
> -};
> -
>  static const struct spi_device_id adxl345_spi_id[] = {
> -	{ "adxl345", (kernel_ulong_t)&adxl345_spi_info },
> -	{ "adxl375", (kernel_ulong_t)&adxl375_spi_info },
> +	{ "adxl345", (kernel_ulong_t)&adxl3x5_chip_info[ADXL345] },
> +	{ "adxl375", (kernel_ulong_t)&adxl3x5_chip_info[ADXL375] },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(spi, adxl345_spi_id);
>  
>  static const struct of_device_id adxl345_of_match[] = {
> -	{ .compatible = "adi,adxl345", .data = &adxl345_spi_info },
> -	{ .compatible = "adi,adxl375", .data = &adxl375_spi_info },
> +	{ .compatible = "adi,adxl345", .data = &adxl3x5_chip_info[ADXL345] },
> +	{ .compatible = "adi,adxl375", .data = &adxl3x5_chip_info[ADXL375] },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, adxl345_of_match);
>  
>  static const struct acpi_device_id adxl345_acpi_match[] = {
> -	{ "ADS0345", (kernel_ulong_t)&adxl345_spi_info },
> +	{ "ADS0345", (kernel_ulong_t)&adxl3x5_chip_info[ADXL345] },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(acpi, adxl345_acpi_match);