* [PATCH v3 1/6] iio: accel: adxl345: Pass function pointer to core
2024-03-23 12:20 [PATCH v3 0/6] iio: accel: adxl345: Add spi-3wire feature Lothar Rubusch
@ 2024-03-23 12:20 ` Lothar Rubusch
2024-03-23 12:20 ` [PATCH v3 2/6] iio: accel: adxl345: Make data_range obsolete Lothar Rubusch
` (5 subsequent siblings)
6 siblings, 0 replies; 18+ messages in thread
From: Lothar Rubusch @ 2024-03-23 12:20 UTC (permalink / raw
To: lars, Michael.Hennerich, jic23, robh+dt, krzysztof.kozlowski+dt,
conor+dt
Cc: linux-iio, devicetree, linux-kernel, eraretuya, l.rubusch
Add a function pointer argument to the probe function in
the core module to provide a way to pre-configure the bus.
The passed setup function can be prepared in the bus
specific spi or the i2c module, or NULL otherwise. It shall
then be executed in the bus independent core module.
Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
drivers/iio/accel/adxl345.h | 3 ++-
drivers/iio/accel/adxl345_core.c | 10 +++++++++-
drivers/iio/accel/adxl345_i2c.c | 2 +-
drivers/iio/accel/adxl345_spi.c | 2 +-
4 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
index 284bd387c..3c1ded0c2 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -28,6 +28,7 @@ struct adxl345_chip_info {
int uscale;
};
-int adxl345_core_probe(struct device *dev, struct regmap *regmap);
+int adxl345_core_probe(struct device *dev, struct regmap *regmap,
+ int (*setup)(struct device*, struct regmap*));
#endif /* _ADXL345_H_ */
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 8bd30a23e..ae12836b5 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -197,13 +197,21 @@ static void adxl345_powerdown(void *regmap)
regmap_write(regmap, ADXL345_REG_POWER_CTL, ADXL345_POWER_CTL_STANDBY);
}
-int adxl345_core_probe(struct device *dev, struct regmap *regmap)
+int adxl345_core_probe(struct device *dev, struct regmap *regmap,
+ int (*setup)(struct device*, struct regmap*))
{
struct adxl345_data *data;
struct iio_dev *indio_dev;
u32 regval;
int ret;
+ /* Perform optional initial bus specific configuration */
+ if (setup) {
+ ret = setup(dev, regmap);
+ if (ret)
+ return ret;
+ }
+
ret = regmap_read(regmap, ADXL345_REG_DEVID, ®val);
if (ret < 0)
return dev_err_probe(dev, ret, "Error reading device ID\n");
diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c
index a3084b0a8..4065b8f7c 100644
--- a/drivers/iio/accel/adxl345_i2c.c
+++ b/drivers/iio/accel/adxl345_i2c.c
@@ -27,7 +27,7 @@ static int adxl345_i2c_probe(struct i2c_client *client)
if (IS_ERR(regmap))
return dev_err_probe(&client->dev, PTR_ERR(regmap), "Error initializing regmap\n");
- return adxl345_core_probe(&client->dev, regmap);
+ return adxl345_core_probe(&client->dev, regmap, NULL);
}
static const struct adxl345_chip_info adxl345_i2c_info = {
diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
index 93ca349f1..1c0513bd3 100644
--- a/drivers/iio/accel/adxl345_spi.c
+++ b/drivers/iio/accel/adxl345_spi.c
@@ -33,7 +33,7 @@ static int adxl345_spi_probe(struct spi_device *spi)
if (IS_ERR(regmap))
return dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing regmap\n");
- return adxl345_core_probe(&spi->dev, regmap);
+ return adxl345_core_probe(&spi->dev, regmap, NULL);
}
static const struct adxl345_chip_info adxl345_spi_info = {
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v3 2/6] iio: accel: adxl345: Make data_range obsolete
2024-03-23 12:20 [PATCH v3 0/6] iio: accel: adxl345: Add spi-3wire feature Lothar Rubusch
2024-03-23 12:20 ` [PATCH v3 1/6] iio: accel: adxl345: Pass function pointer to core Lothar Rubusch
@ 2024-03-23 12:20 ` Lothar Rubusch
2024-03-23 12:20 ` [PATCH v3 3/6] iio: accel: adxl345: Add spi-3wire option Lothar Rubusch
` (4 subsequent siblings)
6 siblings, 0 replies; 18+ messages in thread
From: Lothar Rubusch @ 2024-03-23 12:20 UTC (permalink / raw
To: lars, Michael.Hennerich, jic23, robh+dt, krzysztof.kozlowski+dt,
conor+dt
Cc: linux-iio, devicetree, linux-kernel, eraretuya, l.rubusch
Replace write() data_format by regmap_update_bits(), because
bus specific pre-configuration may have happened before on
the same register. Changes then need to be masked.
Remove the data_range field from the struct adxl345_data,
because it is not used anymore.
Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
drivers/iio/accel/adxl345_core.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index ae12836b5..33424edca 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -42,13 +42,13 @@
#define ADXL345_DATA_FORMAT_4G 1
#define ADXL345_DATA_FORMAT_8G 2
#define ADXL345_DATA_FORMAT_16G 3
+#define ADXL345_DATA_FORMAT_MSK ~((u8) BIT(6)) /* ignore spi-3wire */
#define ADXL345_DEVID 0xE5
struct adxl345_data {
const struct adxl345_chip_info *info;
struct regmap *regmap;
- u8 data_range;
};
#define ADXL345_CHANNEL(index, axis) { \
@@ -227,14 +227,13 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
data = iio_priv(indio_dev);
data->regmap = regmap;
/* Enable full-resolution mode */
- data->data_range = ADXL345_DATA_FORMAT_FULL_RES;
data->info = device_get_match_data(dev);
if (!data->info)
return -ENODEV;
- ret = regmap_write(data->regmap, ADXL345_REG_DATA_FORMAT,
- data->data_range);
- if (ret < 0)
+ ret = regmap_update_bits(regmap, ADXL345_REG_DATA_FORMAT,
+ ADXL345_DATA_FORMAT_MSK, ADXL345_DATA_FORMAT_FULL_RES);
+ if (ret)
return dev_err_probe(dev, ret, "Failed to set data range\n");
indio_dev->name = data->info->name;
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v3 3/6] iio: accel: adxl345: Add spi-3wire option
2024-03-23 12:20 [PATCH v3 0/6] iio: accel: adxl345: Add spi-3wire feature Lothar Rubusch
2024-03-23 12:20 ` [PATCH v3 1/6] iio: accel: adxl345: Pass function pointer to core Lothar Rubusch
2024-03-23 12:20 ` [PATCH v3 2/6] iio: accel: adxl345: Make data_range obsolete Lothar Rubusch
@ 2024-03-23 12:20 ` Lothar Rubusch
2024-03-24 13:32 ` Jonathan Cameron
2024-03-23 12:20 ` [PATCH v3 4/6] iio: accel: adxl345: Remove single info instances Lothar Rubusch
` (3 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: Lothar Rubusch @ 2024-03-23 12:20 UTC (permalink / raw
To: lars, Michael.Hennerich, jic23, robh+dt, krzysztof.kozlowski+dt,
conor+dt
Cc: linux-iio, devicetree, linux-kernel, eraretuya, l.rubusch
Add a setup function implementation to the spi module to enable
spi-3wire as option when specified in the device-tree.
Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
drivers/iio/accel/adxl345.h | 3 +++
drivers/iio/accel/adxl345_spi.c | 12 +++++++++++-
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
index 3c1ded0c2..6b84a2cee 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -8,6 +8,9 @@
#ifndef _ADXL345_H_
#define _ADXL345_H_
+#define ADXL345_REG_DATA_FORMAT 0x31
+#define ADXL345_DATA_FORMAT_SPI BIT(6) /* spi-3wire */
+
/*
* In full-resolution mode, scale factor is maintained at ~4 mg/LSB
* in all g ranges.
diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
index 1c0513bd3..1094396ac 100644
--- a/drivers/iio/accel/adxl345_spi.c
+++ b/drivers/iio/accel/adxl345_spi.c
@@ -20,6 +20,16 @@ static const struct regmap_config adxl345_spi_regmap_config = {
.read_flag_mask = BIT(7) | BIT(6),
};
+static int adxl345_spi_setup(struct device *dev, struct regmap *regmap)
+{
+ struct spi_device *spi = container_of(dev, struct spi_device, dev);
+
+ if (spi->mode & SPI_3WIRE)
+ return regmap_write(regmap, ADXL345_REG_DATA_FORMAT,
+ ADXL345_DATA_FORMAT_SPI);
+ return 0;
+}
+
static int adxl345_spi_probe(struct spi_device *spi)
{
struct regmap *regmap;
@@ -33,7 +43,7 @@ static int adxl345_spi_probe(struct spi_device *spi)
if (IS_ERR(regmap))
return dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing regmap\n");
- return adxl345_core_probe(&spi->dev, regmap, NULL);
+ return adxl345_core_probe(&spi->dev, regmap, &adxl345_spi_setup);
}
static const struct adxl345_chip_info adxl345_spi_info = {
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v3 3/6] iio: accel: adxl345: Add spi-3wire option
2024-03-23 12:20 ` [PATCH v3 3/6] iio: accel: adxl345: Add spi-3wire option Lothar Rubusch
@ 2024-03-24 13:32 ` Jonathan Cameron
2024-03-24 18:59 ` Lothar Rubusch
0 siblings, 1 reply; 18+ messages in thread
From: Jonathan Cameron @ 2024-03-24 13:32 UTC (permalink / raw
To: Lothar Rubusch
Cc: lars, Michael.Hennerich, robh+dt, krzysztof.kozlowski+dt,
conor+dt, linux-iio, devicetree, linux-kernel, eraretuya
On Sat, 23 Mar 2024 12:20:27 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:
> Add a setup function implementation to the spi module to enable
> spi-3wire as option when specified in the device-tree.
>
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
> drivers/iio/accel/adxl345.h | 3 +++
> drivers/iio/accel/adxl345_spi.c | 12 +++++++++++-
> 2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> index 3c1ded0c2..6b84a2cee 100644
> --- a/drivers/iio/accel/adxl345.h
> +++ b/drivers/iio/accel/adxl345.h
> @@ -8,6 +8,9 @@
> #ifndef _ADXL345_H_
> #define _ADXL345_H_
>
> +#define ADXL345_REG_DATA_FORMAT 0x31
> +#define ADXL345_DATA_FORMAT_SPI BIT(6) /* spi-3wire */
Name it that, rather than using a comment. No need to precisely
match datasheet naming - you are naming the value not the field here.
ADXL345_DATA_FORMAT_SPI_3_WRITE perhaps?
> +
> /*
> * In full-resolution mode, scale factor is maintained at ~4 mg/LSB
> * in all g ranges.
> diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
> index 1c0513bd3..1094396ac 100644
> --- a/drivers/iio/accel/adxl345_spi.c
> +++ b/drivers/iio/accel/adxl345_spi.c
> @@ -20,6 +20,16 @@ static const struct regmap_config adxl345_spi_regmap_config = {
> .read_flag_mask = BIT(7) | BIT(6),
> };
>
> +static int adxl345_spi_setup(struct device *dev, struct regmap *regmap)
> +{
> + struct spi_device *spi = container_of(dev, struct spi_device, dev);
> +
> + if (spi->mode & SPI_3WIRE)
> + return regmap_write(regmap, ADXL345_REG_DATA_FORMAT,
> + ADXL345_DATA_FORMAT_SPI);
> + return 0;
> +}
> +
> static int adxl345_spi_probe(struct spi_device *spi)
> {
> struct regmap *regmap;
> @@ -33,7 +43,7 @@ static int adxl345_spi_probe(struct spi_device *spi)
> if (IS_ERR(regmap))
> return dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing regmap\n");
>
> - return adxl345_core_probe(&spi->dev, regmap, NULL);
> + return adxl345_core_probe(&spi->dev, regmap, &adxl345_spi_setup);
> }
>
> static const struct adxl345_chip_info adxl345_spi_info = {
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3 3/6] iio: accel: adxl345: Add spi-3wire option
2024-03-24 13:32 ` Jonathan Cameron
@ 2024-03-24 18:59 ` Lothar Rubusch
2024-03-25 14:46 ` Jonathan Cameron
0 siblings, 1 reply; 18+ messages in thread
From: Lothar Rubusch @ 2024-03-24 18:59 UTC (permalink / raw
To: Jonathan Cameron
Cc: lars, Michael.Hennerich, robh+dt, krzysztof.kozlowski+dt,
conor+dt, linux-iio, devicetree, linux-kernel, eraretuya
On Sun, Mar 24, 2024 at 2:32 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sat, 23 Mar 2024 12:20:27 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Add a setup function implementation to the spi module to enable
> > spi-3wire as option when specified in the device-tree.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > ---
> > drivers/iio/accel/adxl345.h | 3 +++
> > drivers/iio/accel/adxl345_spi.c | 12 +++++++++++-
> > 2 files changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> > index 3c1ded0c2..6b84a2cee 100644
> > --- a/drivers/iio/accel/adxl345.h
> > +++ b/drivers/iio/accel/adxl345.h
> > @@ -8,6 +8,9 @@
> > #ifndef _ADXL345_H_
> > #define _ADXL345_H_
> >
> > +#define ADXL345_REG_DATA_FORMAT 0x31
> > +#define ADXL345_DATA_FORMAT_SPI BIT(6) /* spi-3wire */
> Name it that, rather than using a comment. No need to precisely
> match datasheet naming - you are naming the value not the field here.
>
> ADXL345_DATA_FORMAT_SPI_3_WRITE perhaps?
>
Would you accept the following instead?
ADXL345_DATA_FORMAT_SPI_3WIRE
In the OF binding it is SPI_3WIRE. So, from this perspective I think it would
be consistent naming.
(...)
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 3/6] iio: accel: adxl345: Add spi-3wire option
2024-03-24 18:59 ` Lothar Rubusch
@ 2024-03-25 14:46 ` Jonathan Cameron
0 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2024-03-25 14:46 UTC (permalink / raw
To: Lothar Rubusch
Cc: Jonathan Cameron, lars, Michael.Hennerich, robh+dt,
krzysztof.kozlowski+dt, conor+dt, linux-iio, devicetree,
linux-kernel, eraretuya
On Sun, 24 Mar 2024 19:59:39 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:
> On Sun, Mar 24, 2024 at 2:32 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Sat, 23 Mar 2024 12:20:27 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >
> > > Add a setup function implementation to the spi module to enable
> > > spi-3wire as option when specified in the device-tree.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > ---
> > > drivers/iio/accel/adxl345.h | 3 +++
> > > drivers/iio/accel/adxl345_spi.c | 12 +++++++++++-
> > > 2 files changed, 14 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> > > index 3c1ded0c2..6b84a2cee 100644
> > > --- a/drivers/iio/accel/adxl345.h
> > > +++ b/drivers/iio/accel/adxl345.h
> > > @@ -8,6 +8,9 @@
> > > #ifndef _ADXL345_H_
> > > #define _ADXL345_H_
> > >
> > > +#define ADXL345_REG_DATA_FORMAT 0x31
> > > +#define ADXL345_DATA_FORMAT_SPI BIT(6) /* spi-3wire */
> > Name it that, rather than using a comment. No need to precisely
> > match datasheet naming - you are naming the value not the field here.
> >
> > ADXL345_DATA_FORMAT_SPI_3_WRITE perhaps?
> >
>
> Would you accept the following instead?
> ADXL345_DATA_FORMAT_SPI_3WIRE
>
> In the OF binding it is SPI_3WIRE. So, from this perspective I think it would
> be consistent naming.
That's fine. Precise naming up to you.
Jonathan
>
> (...)
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 4/6] iio: accel: adxl345: Remove single info instances
2024-03-23 12:20 [PATCH v3 0/6] iio: accel: adxl345: Add spi-3wire feature Lothar Rubusch
` (2 preceding siblings ...)
2024-03-23 12:20 ` [PATCH v3 3/6] iio: accel: adxl345: Add spi-3wire option Lothar Rubusch
@ 2024-03-23 12:20 ` Lothar Rubusch
2024-03-24 13:35 ` Jonathan Cameron
2024-03-23 12:20 ` [PATCH v3 5/6] iio: accel: adxl345: Group bus configuration Lothar Rubusch
` (2 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: Lothar Rubusch @ 2024-03-23 12:20 UTC (permalink / raw
To: lars, Michael.Hennerich, jic23, robh+dt, krzysztof.kozlowski+dt,
conor+dt
Cc: linux-iio, devicetree, linux-kernel, eraretuya, l.rubusch
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);
+
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);
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v3 4/6] iio: accel: adxl345: Remove single info instances
2024-03-23 12:20 ` [PATCH v3 4/6] iio: accel: adxl345: Remove single info instances Lothar Rubusch
@ 2024-03-24 13:35 ` Jonathan Cameron
2024-03-24 19:06 ` Lothar Rubusch
0 siblings, 1 reply; 18+ messages in thread
From: Jonathan Cameron @ 2024-03-24 13:35 UTC (permalink / raw
To: Lothar Rubusch
Cc: lars, Michael.Hennerich, robh+dt, krzysztof.kozlowski+dt,
conor+dt, linux-iio, devicetree, linux-kernel, eraretuya
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);
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3 4/6] iio: accel: adxl345: Remove single info instances
2024-03-24 13:35 ` Jonathan Cameron
@ 2024-03-24 19:06 ` Lothar Rubusch
2024-03-25 14:48 ` Jonathan Cameron
0 siblings, 1 reply; 18+ messages in thread
From: Lothar Rubusch @ 2024-03-24 19:06 UTC (permalink / raw
To: Jonathan Cameron
Cc: lars, Michael.Hennerich, robh+dt, krzysztof.kozlowski+dt,
conor+dt, linux-iio, devicetree, linux-kernel, eraretuya
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?
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3 4/6] iio: accel: adxl345: Remove single info instances
2024-03-24 19:06 ` Lothar Rubusch
@ 2024-03-25 14:48 ` Jonathan Cameron
0 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2024-03-25 14:48 UTC (permalink / raw
To: Lothar Rubusch
Cc: Jonathan Cameron, lars, Michael.Hennerich, robh+dt,
krzysztof.kozlowski+dt, conor+dt, linux-iio, devicetree,
linux-kernel, eraretuya
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
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 5/6] iio: accel: adxl345: Group bus configuration
2024-03-23 12:20 [PATCH v3 0/6] iio: accel: adxl345: Add spi-3wire feature Lothar Rubusch
` (3 preceding siblings ...)
2024-03-23 12:20 ` [PATCH v3 4/6] iio: accel: adxl345: Remove single info instances Lothar Rubusch
@ 2024-03-23 12:20 ` Lothar Rubusch
2024-03-24 13:37 ` Jonathan Cameron
2024-03-23 12:20 ` [PATCH v3 6/6] dt-bindings: iio: accel: adxl345: Add spi-3wire Lothar Rubusch
2024-03-24 13:39 ` [PATCH v3 0/6] iio: accel: adxl345: Add spi-3wire feature Jonathan Cameron
6 siblings, 1 reply; 18+ messages in thread
From: Lothar Rubusch @ 2024-03-23 12:20 UTC (permalink / raw
To: lars, Michael.Hennerich, jic23, robh+dt, krzysztof.kozlowski+dt,
conor+dt
Cc: linux-iio, devicetree, linux-kernel, eraretuya, l.rubusch
In the probe function group bus configuration and the
indio_dev initialization to improve readability. Add a
comment to the probe function to explain function arguments.
Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
drivers/iio/accel/adxl345_core.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index e3718d0dd..662628cdf 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -209,6 +209,16 @@ static void adxl345_powerdown(void *regmap)
regmap_write(regmap, ADXL345_REG_POWER_CTL, ADXL345_POWER_CTL_STANDBY);
}
+/**
+ * adxl345_core_probe() - probe and setup for the adxl345 accelerometer,
+ * also covers the adlx375 accelerometer
+ * @dev: Driver model representation of the device
+ * @regmap: Regmap instance for the device
+ * @setup: Setup routine to be executed right before the standard device
+ * setup, can also be set to NULL if not required
+ *
+ * Return: 0 on success, negative errno on error
+ */
int adxl345_core_probe(struct device *dev, struct regmap *regmap,
int (*setup)(struct device*, struct regmap*))
{
@@ -238,22 +248,23 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
data = iio_priv(indio_dev);
data->regmap = regmap;
- /* Enable full-resolution mode */
+
data->info = device_get_match_data(dev);
if (!data->info)
return -ENODEV;
- ret = regmap_update_bits(regmap, ADXL345_REG_DATA_FORMAT,
- ADXL345_DATA_FORMAT_MSK, ADXL345_DATA_FORMAT_FULL_RES);
- if (ret)
- return dev_err_probe(dev, ret, "Failed to set data range\n");
-
indio_dev->name = data->info->name;
indio_dev->info = &adxl345_info;
indio_dev->modes = INDIO_DIRECT_MODE;
indio_dev->channels = adxl345_channels;
indio_dev->num_channels = ARRAY_SIZE(adxl345_channels);
+ /* Enable full-resolution mode */
+ ret = regmap_update_bits(regmap, ADXL345_REG_DATA_FORMAT,
+ ADXL345_DATA_FORMAT_MSK, ADXL345_DATA_FORMAT_FULL_RES);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to set data range\n");
+
/* Enable measurement mode */
ret = adxl345_powerup(data->regmap);
if (ret < 0)
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v3 5/6] iio: accel: adxl345: Group bus configuration
2024-03-23 12:20 ` [PATCH v3 5/6] iio: accel: adxl345: Group bus configuration Lothar Rubusch
@ 2024-03-24 13:37 ` Jonathan Cameron
0 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2024-03-24 13:37 UTC (permalink / raw
To: Lothar Rubusch
Cc: lars, Michael.Hennerich, robh+dt, krzysztof.kozlowski+dt,
conor+dt, linux-iio, devicetree, linux-kernel, eraretuya
On Sat, 23 Mar 2024 12:20:29 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:
> In the probe function group bus configuration and the
> indio_dev initialization to improve readability. Add a
> comment to the probe function to explain function arguments.
Doing 2 unrelated things. 2 Patches.
>
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
> drivers/iio/accel/adxl345_core.c | 23 +++++++++++++++++------
> 1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index e3718d0dd..662628cdf 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -209,6 +209,16 @@ static void adxl345_powerdown(void *regmap)
> regmap_write(regmap, ADXL345_REG_POWER_CTL, ADXL345_POWER_CTL_STANDBY);
> }
>
> +/**
> + * adxl345_core_probe() - probe and setup for the adxl345 accelerometer,
> + * also covers the adlx375 accelerometer
> + * @dev: Driver model representation of the device
> + * @regmap: Regmap instance for the device
> + * @setup: Setup routine to be executed right before the standard device
> + * setup, can also be set to NULL if not required
* @setup: Optional setup routine to be executed right before standard
* device setup.
For a function pointer, optional implies NULL if you don't want it.
> + *
> + * Return: 0 on success, negative errno on error
> + */
> int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> int (*setup)(struct device*, struct regmap*))
> {
> @@ -238,22 +248,23 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>
> data = iio_priv(indio_dev);
> data->regmap = regmap;
> - /* Enable full-resolution mode */
> +
> data->info = device_get_match_data(dev);
> if (!data->info)
> return -ENODEV;
>
> - ret = regmap_update_bits(regmap, ADXL345_REG_DATA_FORMAT,
> - ADXL345_DATA_FORMAT_MSK, ADXL345_DATA_FORMAT_FULL_RES);
> - if (ret)
> - return dev_err_probe(dev, ret, "Failed to set data range\n");
> -
> indio_dev->name = data->info->name;
> indio_dev->info = &adxl345_info;
> indio_dev->modes = INDIO_DIRECT_MODE;
> indio_dev->channels = adxl345_channels;
> indio_dev->num_channels = ARRAY_SIZE(adxl345_channels);
>
> + /* Enable full-resolution mode */
> + ret = regmap_update_bits(regmap, ADXL345_REG_DATA_FORMAT,
> + ADXL345_DATA_FORMAT_MSK, ADXL345_DATA_FORMAT_FULL_RES);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to set data range\n");
> +
> /* Enable measurement mode */
> ret = adxl345_powerup(data->regmap);
> if (ret < 0)
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 6/6] dt-bindings: iio: accel: adxl345: Add spi-3wire
2024-03-23 12:20 [PATCH v3 0/6] iio: accel: adxl345: Add spi-3wire feature Lothar Rubusch
` (4 preceding siblings ...)
2024-03-23 12:20 ` [PATCH v3 5/6] iio: accel: adxl345: Group bus configuration Lothar Rubusch
@ 2024-03-23 12:20 ` Lothar Rubusch
2024-03-23 14:28 ` Krzysztof Kozlowski
2024-03-24 13:39 ` [PATCH v3 0/6] iio: accel: adxl345: Add spi-3wire feature Jonathan Cameron
6 siblings, 1 reply; 18+ messages in thread
From: Lothar Rubusch @ 2024-03-23 12:20 UTC (permalink / raw
To: lars, Michael.Hennerich, jic23, robh+dt, krzysztof.kozlowski+dt,
conor+dt
Cc: linux-iio, devicetree, linux-kernel, eraretuya, l.rubusch
Add spi-3wire because the driver optionally supports spi-3wire.
Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
index 07cacc3f6..280ed479e 100644
--- a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
+++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
@@ -32,6 +32,8 @@ properties:
spi-cpol: true
+ spi-3wire: true
+
interrupts:
maxItems: 1
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v3 6/6] dt-bindings: iio: accel: adxl345: Add spi-3wire
2024-03-23 12:20 ` [PATCH v3 6/6] dt-bindings: iio: accel: adxl345: Add spi-3wire Lothar Rubusch
@ 2024-03-23 14:28 ` Krzysztof Kozlowski
0 siblings, 0 replies; 18+ messages in thread
From: Krzysztof Kozlowski @ 2024-03-23 14:28 UTC (permalink / raw
To: Lothar Rubusch, lars, Michael.Hennerich, jic23, robh+dt,
krzysztof.kozlowski+dt, conor+dt
Cc: linux-iio, devicetree, linux-kernel, eraretuya
On 23/03/2024 13:20, Lothar Rubusch wrote:
> Add spi-3wire because the driver optionally supports spi-3wire.
"the driver and the device"
Bindings should describe hardware, not drivers.
No need to resend just for this.
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 0/6] iio: accel: adxl345: Add spi-3wire feature
2024-03-23 12:20 [PATCH v3 0/6] iio: accel: adxl345: Add spi-3wire feature Lothar Rubusch
` (5 preceding siblings ...)
2024-03-23 12:20 ` [PATCH v3 6/6] dt-bindings: iio: accel: adxl345: Add spi-3wire Lothar Rubusch
@ 2024-03-24 13:39 ` Jonathan Cameron
2024-03-24 19:20 ` Lothar Rubusch
6 siblings, 1 reply; 18+ messages in thread
From: Jonathan Cameron @ 2024-03-24 13:39 UTC (permalink / raw
To: Lothar Rubusch
Cc: lars, Michael.Hennerich, robh+dt, krzysztof.kozlowski+dt,
conor+dt, linux-iio, devicetree, linux-kernel, eraretuya
On Sat, 23 Mar 2024 12:20:24 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:
> Pass a function setup() as pointer from SPI/I2C specific modules
> to the core module. Implement setup() to pass the spi-3wire bus
> option, if declared in the device-tree.
>
> In the core module, then update data_format register
> configuration bits instead of overwriting it. The changes allow
> to remove a data_range field, remove I2C and SPI redundant info
> instances and replace them by a common info array instance.
>
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
That patch break up seems reasonable (one minor request for a split
in the relevant patch), but normal convention would be do do
refactoring first, then functionality at the end. Also removal stuff
and group, before adding things.
So roughly speaking reorder as
> iio: accel: adxl345: Make data_format obsolete
> iio: accel: adxl345: Remove single info instances
> iio: accel: adxl345: Group bus configuration
> dt-bindings: iio: accel: adxl345: Add spi-3wire
> iio: accel: adxl345: Pass function pointer to core
> iio: accel: adxl345: Add the spi-3wire
Thanks,
Jonathan
> ---
> V1 -> V2: split into spi-3wire and refactoring
> V2 -> V3: split further, focus on needed changesets
>
> Lothar Rubusch (6):
> iio: accel: adxl345: Pass function pointer to core
> iio: accel: adxl345: Make data_format obsolete
> iio: accel: adxl345: Add the spi-3wire
> iio: accel: adxl345: Remove single info instances
> iio: accel: adxl345: Group bus configuration
> dt-bindings: iio: accel: adxl345: Add spi-3wire
>
> .../bindings/iio/accel/adi,adxl345.yaml | 2 +
> drivers/iio/accel/adxl345.h | 13 ++++-
> drivers/iio/accel/adxl345_core.c | 48 +++++++++++++++----
> drivers/iio/accel/adxl345_i2c.c | 22 +++------
> drivers/iio/accel/adxl345_spi.c | 32 ++++++-------
> 5 files changed, 75 insertions(+), 42 deletions(-)
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3 0/6] iio: accel: adxl345: Add spi-3wire feature
2024-03-24 13:39 ` [PATCH v3 0/6] iio: accel: adxl345: Add spi-3wire feature Jonathan Cameron
@ 2024-03-24 19:20 ` Lothar Rubusch
2024-03-25 14:51 ` Jonathan Cameron
0 siblings, 1 reply; 18+ messages in thread
From: Lothar Rubusch @ 2024-03-24 19:20 UTC (permalink / raw
To: Jonathan Cameron
Cc: lars, Michael.Hennerich, robh+dt, krzysztof.kozlowski+dt,
conor+dt, linux-iio, devicetree, linux-kernel, eraretuya
On Sun, Mar 24, 2024 at 2:39 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sat, 23 Mar 2024 12:20:24 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Pass a function setup() as pointer from SPI/I2C specific modules
> > to the core module. Implement setup() to pass the spi-3wire bus
> > option, if declared in the device-tree.
> >
> > In the core module, then update data_format register
> > configuration bits instead of overwriting it. The changes allow
> > to remove a data_range field, remove I2C and SPI redundant info
> > instances and replace them by a common info array instance.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> That patch break up seems reasonable (one minor request for a split
> in the relevant patch), but normal convention would be do do
> refactoring first, then functionality at the end. Also removal stuff
> and group, before adding things.
>
> So roughly speaking reorder as
>
> > iio: accel: adxl345: Make data_format obsolete
> > iio: accel: adxl345: Remove single info instances
> > iio: accel: adxl345: Group bus configuration
> > dt-bindings: iio: accel: adxl345: Add spi-3wire
> > iio: accel: adxl345: Pass function pointer to core
> > iio: accel: adxl345: Add the spi-3wire
>
Ok. If I split "Group bus configuration" into the grouping of the
indio_dev in the probe() and adding a comment to the core's probe(), I
will end up with something like this:
$ git log --oneline --reverse
iio: accel: adxl345: Make data_range obsolete
iio: accel: adxl345: Group bus configuration
iio: accel: adxl345: Move defines to header <--- new
dt-bindings: iio: accel: adxl345: Add spi-3wire
iio: accel: adxl345: Pass function pointer to core
iio: accel: adxl345: Add comment to probe <--- new after split
iio: accel: adxl345: Add spi-3wire option
I feel I have to add the comment after adding the passed function
pointer. Bascially I liked to add a comment mentioning especially the
new function pointer there. So, although being a comment, the commit
will be in this "high" position. Is this ok, or am I doing something
wrong? Should I split into setting the comment first, then inside
"Pass function pointer.." also update the comment?
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 0/6] iio: accel: adxl345: Add spi-3wire feature
2024-03-24 19:20 ` Lothar Rubusch
@ 2024-03-25 14:51 ` Jonathan Cameron
0 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2024-03-25 14:51 UTC (permalink / raw
To: Lothar Rubusch
Cc: Jonathan Cameron, lars, Michael.Hennerich, robh+dt,
krzysztof.kozlowski+dt, conor+dt, linux-iio, devicetree,
linux-kernel, eraretuya
On Sun, 24 Mar 2024 20:20:21 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:
> On Sun, Mar 24, 2024 at 2:39 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Sat, 23 Mar 2024 12:20:24 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >
> > > Pass a function setup() as pointer from SPI/I2C specific modules
> > > to the core module. Implement setup() to pass the spi-3wire bus
> > > option, if declared in the device-tree.
> > >
> > > In the core module, then update data_format register
> > > configuration bits instead of overwriting it. The changes allow
> > > to remove a data_range field, remove I2C and SPI redundant info
> > > instances and replace them by a common info array instance.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > That patch break up seems reasonable (one minor request for a split
> > in the relevant patch), but normal convention would be do do
> > refactoring first, then functionality at the end. Also removal stuff
> > and group, before adding things.
> >
> > So roughly speaking reorder as
> >
> > > iio: accel: adxl345: Make data_format obsolete
> > > iio: accel: adxl345: Remove single info instances
> > > iio: accel: adxl345: Group bus configuration
> > > dt-bindings: iio: accel: adxl345: Add spi-3wire
> > > iio: accel: adxl345: Pass function pointer to core
> > > iio: accel: adxl345: Add the spi-3wire
> >
>
> Ok. If I split "Group bus configuration" into the grouping of the
> indio_dev in the probe() and adding a comment to the core's probe(), I
> will end up with something like this:
>
> $ git log --oneline --reverse
> iio: accel: adxl345: Make data_range obsolete
> iio: accel: adxl345: Group bus configuration
> iio: accel: adxl345: Move defines to header <--- new
> dt-bindings: iio: accel: adxl345: Add spi-3wire
> iio: accel: adxl345: Pass function pointer to core
> iio: accel: adxl345: Add comment to probe <--- new after split
> iio: accel: adxl345: Add spi-3wire option
>
> I feel I have to add the comment after adding the passed function
> pointer. Bascially I liked to add a comment mentioning especially the
> new function pointer there. So, although being a comment, the commit
> will be in this "high" position. Is this ok, or am I doing something
> wrong? Should I split into setting the comment first, then inside
> "Pass function pointer.." also update the comment?
This is fine. Either of the options you suggest would be fine, but
given you've done the above already let's go with that.
Jonathan
>
^ permalink raw reply [flat|nested] 18+ messages in thread