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 <[email protected]>
---
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(-)
--
2.25.1
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Add spi-3wire because the driver optionally supports spi-3wire.
Signed-off-by: Lothar Rubusch <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Best regards,
Krzysztof
On Sat, 23 Mar 2024 12:20:27 +0000
Lothar Rubusch <[email protected]> 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 <[email protected]>
> ---
> 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 = {
On Sat, 23 Mar 2024 12:20:28 +0000
Lothar Rubusch <[email protected]> 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 <[email protected]>
> ---
> 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 Sat, 23 Mar 2024 12:20:29 +0000
Lothar Rubusch <[email protected]> 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 <[email protected]>
> ---
> 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)
On Sat, 23 Mar 2024 12:20:24 +0000
Lothar Rubusch <[email protected]> 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 <[email protected]>
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(-)
>
On Sun, Mar 24, 2024 at 2:32 PM Jonathan Cameron <[email protected]> wrote:
>
> On Sat, 23 Mar 2024 12:20:27 +0000
> Lothar Rubusch <[email protected]> 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 <[email protected]>
> > ---
> > 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.
(...)
On Sun, Mar 24, 2024 at 2:35 PM Jonathan Cameron <[email protected]> wrote:
>
> On Sat, 23 Mar 2024 12:20:28 +0000
> Lothar Rubusch <[email protected]> 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 <[email protected]>
> > ---
> > 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, Mar 24, 2024 at 2:39 PM Jonathan Cameron <[email protected]> wrote:
>
> On Sat, 23 Mar 2024 12:20:24 +0000
> Lothar Rubusch <[email protected]> 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 <[email protected]>
> 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?
On Sun, 24 Mar 2024 19:59:39 +0100
Lothar Rubusch <[email protected]> wrote:
> On Sun, Mar 24, 2024 at 2:32 PM Jonathan Cameron <[email protected]> wrote:
> >
> > On Sat, 23 Mar 2024 12:20:27 +0000
> > Lothar Rubusch <[email protected]> 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 <[email protected]>
> > > ---
> > > 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
>
> (...)
>
On Sun, 24 Mar 2024 20:20:21 +0100
Lothar Rubusch <[email protected]> wrote:
> On Sun, Mar 24, 2024 at 2:39 PM Jonathan Cameron <[email protected]> wrote:
> >
> > On Sat, 23 Mar 2024 12:20:24 +0000
> > Lothar Rubusch <[email protected]> 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 <[email protected]>
> > 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
>
On Sun, 24 Mar 2024 20:06:51 +0100
Lothar Rubusch <[email protected]> wrote:
> On Sun, Mar 24, 2024 at 2:35 PM Jonathan Cameron <[email protected]> wrote:
> >
> > On Sat, 23 Mar 2024 12:20:28 +0000
> > Lothar Rubusch <[email protected]> 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 <[email protected]>
> > > ---
> > > 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
>