| Message ID | 20240323122030.21800-5-l.rubusch@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: accel: adxl345: Add spi-3wire feature | expand |
On Sat, 23 Mar 2024 12:20:28 +0000 Lothar Rubusch <l.rubusch@gmail.com> 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@gmail.com> > --- > 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);
On Sun, Mar 24, 2024 at 2:35 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Sat, 23 Mar 2024 12:20:28 +0000 > Lothar Rubusch <l.rubusch@gmail.com> 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@gmail.com> > > --- > > 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. > Agree. I see your point. I drop the info array enum patch. (...) Btw. may I ask another question: The adxl345/75 driver is doing the configuration inside the probe(). Other Analog drivers moved that out into a xxx_setup() and call this function in the probe(). In general, is it better to keep all inside the probe() or separate? I mean, the probe is still quite short, and reading through severl call hierarchies feels a bit "sparghetti". On the other side I can see a certain idea of separation of functionality: dedicated chip configuration. Would you mind to give me a small statement/opinion on this please?
On Sun, 24 Mar 2024 20:06:51 +0100 Lothar Rubusch <l.rubusch@gmail.com> wrote: > On Sun, Mar 24, 2024 at 2:35 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Sat, 23 Mar 2024 12:20:28 +0000 > > Lothar Rubusch <l.rubusch@gmail.com> 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@gmail.com> > > > --- > > > 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. > > > > Agree. I see your point. I drop the info array enum patch. > > (...) > > Btw. may I ask another question: The adxl345/75 driver is doing the > configuration > inside the probe(). Other Analog drivers moved that out into a > xxx_setup() and call > this function in the probe(). In general, is it better to keep all > inside the probe() or > separate? I mean, the probe is still quite short, and reading through > severl call > hierarchies feels a bit "sparghetti". On the other side I can see a > certain idea of > separation of functionality: dedicated chip configuration. Would you > mind to give > me a small statement/opinion on this please? I'd based it on code complexity. If it's one call (and error handling) to do it then inline makes sense. If it's lots of lines, a separate function make sense. Where the boundary between the two lies is subjective so I tend to just go with whatever an author prefers. Note that I'm not keen to see the noise of refactors if the code lies in this gray area? Jonathan >
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); + 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);
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@gmail.com> --- 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(-)