Move driver wide constants and fields into the header. Reduce
redundant info struct definitions. Allow to pass a function
pointer from SPI/I2C specific probe, and smaller refactorings.
Apply regmap_update_bits() in the core file replaces the
regmap_write() to format_data.
Signed-off-by: Lothar Rubusch <[email protected]>
---
V1 -> V2: split into spi-3wire and refactoring
Lothar Rubusch (3):
iio: accel: adxl345: Update adxl345
iio: accel: adxl345: Add spi-3wire feature
dt-bindings: iio: accel: adxl345: Add spi-3wire
.../bindings/iio/accel/adi,adxl345.yaml | 2 +
drivers/iio/accel/adxl345.h | 44 ++++++-
drivers/iio/accel/adxl345_core.c | 117 ++++++++++--------
drivers/iio/accel/adxl345_i2c.c | 30 ++---
drivers/iio/accel/adxl345_spi.c | 38 +++---
5 files changed, 146 insertions(+), 85 deletions(-)
--
2.25.1
Move driver wide constants and fields into the header.
Let probe call a separate setup function. Provide
possibility for an SPI/I2C specific setup to be passed
as function pointer to core.
Signed-off-by: Lothar Rubusch <[email protected]>
---
drivers/iio/accel/adxl345.h | 44 +++++++++++-
drivers/iio/accel/adxl345_core.c | 117 +++++++++++++++++--------------
drivers/iio/accel/adxl345_i2c.c | 30 ++++----
drivers/iio/accel/adxl345_spi.c | 28 ++++----
4 files changed, 134 insertions(+), 85 deletions(-)
diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
index 284bd387c..01493c999 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -8,6 +8,39 @@
#ifndef _ADXL345_H_
#define _ADXL345_H_
+#include <linux/iio/iio.h>
+
+/* ADXL345 register definitions */
+#define ADXL345_REG_DEVID 0x00
+#define ADXL345_REG_OFSX 0x1E
+#define ADXL345_REG_OFSY 0x1F
+#define ADXL345_REG_OFSZ 0x20
+#define ADXL345_REG_OFS_AXIS(index) (ADXL345_REG_OFSX + (index))
+#define ADXL345_REG_BW_RATE 0x2C
+#define ADXL345_REG_POWER_CTL 0x2D
+#define ADXL345_REG_DATA_FORMAT 0x31
+#define ADXL345_REG_DATAX0 0x32
+#define ADXL345_REG_DATAY0 0x34
+#define ADXL345_REG_DATAZ0 0x36
+#define ADXL345_REG_DATA_AXIS(index) \
+ (ADXL345_REG_DATAX0 + (index) * sizeof(__le16))
+
+#define ADXL345_BW_RATE GENMASK(3, 0)
+#define ADXL345_BASE_RATE_NANO_HZ 97656250LL
+
+#define ADXL345_POWER_CTL_MEASURE BIT(3)
+#define ADXL345_POWER_CTL_STANDBY 0x00
+
+#define ADXL345_DATA_FORMAT_FULL_RES BIT(3) /* Up to 13-bits resolution */
+#define ADXL345_DATA_FORMAT_SPI BIT(6) /* spi-3wire */
+#define ADXL345_DATA_FORMAT_2G 0
+#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
+
/*
* In full-resolution mode, scale factor is maintained at ~4 mg/LSB
* in all g ranges.
@@ -23,11 +56,20 @@
*/
#define ADXL375_USCALE 480000
+enum adxl345_device_type {
+ ADXL345,
+ ADXL375,
+};
+
struct adxl345_chip_info {
const char *name;
int uscale;
};
-int adxl345_core_probe(struct device *dev, struct regmap *regmap);
+extern const struct adxl345_chip_info adxl3x5_chip_info[];
+
+int adxl345_core_probe(struct device *dev, struct regmap *regmap,
+ const struct adxl345_chip_info *chip_info,
+ 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..040c3f05a 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -17,38 +17,9 @@
#include "adxl345.h"
-#define ADXL345_REG_DEVID 0x00
-#define ADXL345_REG_OFSX 0x1e
-#define ADXL345_REG_OFSY 0x1f
-#define ADXL345_REG_OFSZ 0x20
-#define ADXL345_REG_OFS_AXIS(index) (ADXL345_REG_OFSX + (index))
-#define ADXL345_REG_BW_RATE 0x2C
-#define ADXL345_REG_POWER_CTL 0x2D
-#define ADXL345_REG_DATA_FORMAT 0x31
-#define ADXL345_REG_DATAX0 0x32
-#define ADXL345_REG_DATAY0 0x34
-#define ADXL345_REG_DATAZ0 0x36
-#define ADXL345_REG_DATA_AXIS(index) \
- (ADXL345_REG_DATAX0 + (index) * sizeof(__le16))
-
-#define ADXL345_BW_RATE GENMASK(3, 0)
-#define ADXL345_BASE_RATE_NANO_HZ 97656250LL
-
-#define ADXL345_POWER_CTL_MEASURE BIT(3)
-#define ADXL345_POWER_CTL_STANDBY 0x00
-
-#define ADXL345_DATA_FORMAT_FULL_RES BIT(3) /* Up to 13-bits resolution */
-#define ADXL345_DATA_FORMAT_2G 0
-#define ADXL345_DATA_FORMAT_4G 1
-#define ADXL345_DATA_FORMAT_8G 2
-#define ADXL345_DATA_FORMAT_16G 3
-
-#define ADXL345_DEVID 0xE5
-
struct adxl345_data {
const struct adxl345_chip_info *info;
struct regmap *regmap;
- u8 data_range;
};
#define ADXL345_CHANNEL(index, axis) { \
@@ -62,6 +33,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),
@@ -197,14 +180,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)
+static int adxl345_setup(struct device *dev, struct adxl345_data *data,
+ int (*setup)(struct device*, struct regmap*))
{
- struct adxl345_data *data;
- struct iio_dev *indio_dev;
u32 regval;
int ret;
- ret = regmap_read(regmap, ADXL345_REG_DEVID, ®val);
+ /* Perform bus specific settings if available */
+ if (setup) {
+ ret = setup(dev, data->regmap);
+ if (ret)
+ return ret;
+ }
+
+ /* Read out DEVID */
+ ret = regmap_read(data->regmap, ADXL345_REG_DEVID, ®val);
if (ret < 0)
return dev_err_probe(dev, ret, "Error reading device ID\n");
@@ -212,37 +202,62 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap)
return dev_err_probe(dev, -ENODEV, "Invalid device ID: %x, expected %x\n",
regval, ADXL345_DEVID);
+ /* Update data_format to full-resolution mode */
+ ret = regmap_update_bits(data->regmap, ADXL345_REG_DATA_FORMAT,
+ ADXL345_DATA_FORMAT_MSK, ADXL345_DATA_FORMAT_FULL_RES);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to update data_format register\n");
+
+ /* Enable measurement mode */
+ ret = adxl345_powerup(data->regmap);
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "Failed to enable measurement mode\n");
+
+ ret = devm_add_action_or_reset(dev, adxl345_powerdown, data->regmap);
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+/**
+ * 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
+ * @chip_info: Structure containing device specific data
+ * @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,
+ const struct adxl345_chip_info *chip_info,
+ int (*setup)(struct device*, struct regmap*))
+{
+ struct adxl345_data *data;
+ struct iio_dev *indio_dev;
+ int ret;
+
indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
if (!indio_dev)
return -ENOMEM;
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)
- return dev_err_probe(dev, ret, "Failed to set data range\n");
+ data->info = chip_info;
- indio_dev->name = data->info->name;
+ indio_dev->name = chip_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 measurement mode */
- ret = adxl345_powerup(data->regmap);
- if (ret < 0)
- return dev_err_probe(dev, ret, "Failed to enable measurement mode\n");
-
- ret = devm_add_action_or_reset(dev, adxl345_powerdown, data->regmap);
- if (ret < 0)
+ ret = adxl345_setup(dev, data, setup);
+ if (ret) {
+ dev_err(dev, "ADXL345 setup failed\n");
return ret;
+ }
return devm_iio_device_register(dev, indio_dev);
}
diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c
index a3084b0a8..3f882e2e0 100644
--- a/drivers/iio/accel/adxl345_i2c.c
+++ b/drivers/iio/accel/adxl345_i2c.c
@@ -9,6 +9,7 @@
*/
#include <linux/i2c.h>
+#include <linux/mod_devicetable.h>
#include <linux/module.h>
#include <linux/regmap.h>
@@ -21,41 +22,36 @@ static const struct regmap_config adxl345_i2c_regmap_config = {
static int adxl345_i2c_probe(struct i2c_client *client)
{
+ const struct adxl345_chip_info *chip_data;
struct regmap *regmap;
+ /* Retrieve device data, i.e. the name, to pass it to the core */
+ chip_data = i2c_get_match_data(client);
+
regmap = devm_regmap_init_i2c(client, &adxl345_i2c_regmap_config);
if (IS_ERR(regmap))
- return dev_err_probe(&client->dev, PTR_ERR(regmap), "Error initializing regmap\n");
+ 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, chip_data, 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 93ca349f1..c26bac462 100644
--- a/drivers/iio/accel/adxl345_spi.c
+++ b/drivers/iio/accel/adxl345_spi.c
@@ -22,8 +22,14 @@ static const struct regmap_config adxl345_spi_regmap_config = {
static int adxl345_spi_probe(struct spi_device *spi)
{
+ const struct adxl345_chip_info *chip_data;
struct regmap *regmap;
+ /* Retrieve device name to pass it as driver specific data */
+ chip_data = device_get_match_data(&spi->dev);
+ if (!chip_data)
+ chip_data = spi_get_device_match_data(spi);
+
/* Bail out if max_speed_hz exceeds 5 MHz */
if (spi->max_speed_hz > ADXL345_MAX_SPI_FREQ_HZ)
return dev_err_probe(&spi->dev, -EINVAL, "SPI CLK, %d Hz exceeds 5 MHz\n",
@@ -33,35 +39,25 @@ 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, chip_data, NULL);
}
-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
Provide the optional spi-3wire in the example.
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
Add the spi-3wire feature to the iio driver. Pass a
function pointer to configure the device if 3-wire
was configured in the device-tree.
Signed-off-by: Lothar Rubusch <[email protected]>
---
drivers/iio/accel/adxl345_spi.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
index c26bac462..aea368895 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)
{
const struct adxl345_chip_info *chip_data;
@@ -39,7 +49,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, chip_data, NULL);
+ return adxl345_core_probe(&spi->dev, regmap, chip_data, &adxl345_spi_setup);
}
static const struct spi_device_id adxl345_spi_id[] = {
--
2.25.1
On Fri, Mar 22, 2024 at 12:37:13AM +0000, Lothar Rubusch wrote:
> Provide the optional spi-3wire in the example.
That doesn't match the diff as you don't touch the example. But really,
this should say why you need 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
>
On 22/03/2024 01:37, Lothar Rubusch wrote:
> Move driver wide constants and fields into the header.
> Let probe call a separate setup function. Provide
> possibility for an SPI/I2C specific setup to be passed
> as function pointer to core.
Subject: you received feedback already of not calling things "update".
Everything is update.
No, write descriptive text.
If you cannot, means you are doing way too many things in one patch.
Please read submitting-patches document.
>
> Signed-off-by: Lothar Rubusch <[email protected]>
> ---
> drivers/iio/accel/adxl345.h | 44 +++++++++++-
> drivers/iio/accel/adxl345_core.c | 117 +++++++++++++++++--------------
> drivers/iio/accel/adxl345_i2c.c | 30 ++++----
> drivers/iio/accel/adxl345_spi.c | 28 ++++----
> 4 files changed, 134 insertions(+), 85 deletions(-)
>
> diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> index 284bd387c..01493c999 100644
> --- a/drivers/iio/accel/adxl345.h
> +++ b/drivers/iio/accel/adxl345.h
> @@ -8,6 +8,39 @@
> #ifndef _ADXL345_H_
> #define _ADXL345_H_
>
> +#include <linux/iio/iio.h>
> +
> +/* ADXL345 register definitions */
> +#define ADXL345_REG_DEVID 0x00
> +#define ADXL345_REG_OFSX 0x1E
> +#define ADXL345_REG_OFSY 0x1F
> +#define ADXL345_REG_OFSZ 0x20
> +#define ADXL345_REG_OFS_AXIS(index) (ADXL345_REG_OFSX + (index))
> +#define ADXL345_REG_BW_RATE 0x2C
> +#define ADXL345_REG_POWER_CTL 0x2D
> +#define ADXL345_REG_DATA_FORMAT 0x31
> +#define ADXL345_REG_DATAX0 0x32
> +#define ADXL345_REG_DATAY0 0x34
> +#define ADXL345_REG_DATAZ0 0x36
> +#define ADXL345_REG_DATA_AXIS(index) \
> + (ADXL345_REG_DATAX0 + (index) * sizeof(__le16))
> +
> +#define ADXL345_BW_RATE GENMASK(3, 0)
> +#define ADXL345_BASE_RATE_NANO_HZ 97656250LL
> +
> +#define ADXL345_POWER_CTL_MEASURE BIT(3)
> +#define ADXL345_POWER_CTL_STANDBY 0x00
> +
> +#define ADXL345_DATA_FORMAT_FULL_RES BIT(3) /* Up to 13-bits resolution */
> +#define ADXL345_DATA_FORMAT_SPI BIT(6) /* spi-3wire */
> +#define ADXL345_DATA_FORMAT_2G 0
> +#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
How is all this related to the patch? I don't understand. Several parts
of this patch are not explained / obvious.
> +
> /*
> * In full-resolution mode, scale factor is maintained at ~4 mg/LSB
> * in all g ranges.
> @@ -23,11 +56,20 @@
> */
> #define ADXL375_USCALE 480000
>
> +enum adxl345_device_type {
> + ADXL345,
> + ADXL375,
> +};
> +
> struct adxl345_chip_info {
> const char *name;
> int uscale;
> };
>
> -int adxl345_core_probe(struct device *dev, struct regmap *regmap);
> +extern const struct adxl345_chip_info adxl3x5_chip_info[];
> +
> +int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> + const struct adxl345_chip_info *chip_info,
> + 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..040c3f05a 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -17,38 +17,9 @@
>
> #include "adxl345.h"
>
> -#define ADXL345_REG_DEVID 0x00
> -#define ADXL345_REG_OFSX 0x1e
> -#define ADXL345_REG_OFSY 0x1f
> -#define ADXL345_REG_OFSZ 0x20
> -#define ADXL345_REG_OFS_AXIS(index) (ADXL345_REG_OFSX + (index))
> -#define ADXL345_REG_BW_RATE 0x2C
> -#define ADXL345_REG_POWER_CTL 0x2D
> -#define ADXL345_REG_DATA_FORMAT 0x31
> -#define ADXL345_REG_DATAX0 0x32
> -#define ADXL345_REG_DATAY0 0x34
> -#define ADXL345_REG_DATAZ0 0x36
> -#define ADXL345_REG_DATA_AXIS(index) \
> - (ADXL345_REG_DATAX0 + (index) * sizeof(__le16))
> -
> -#define ADXL345_BW_RATE GENMASK(3, 0)
> -#define ADXL345_BASE_RATE_NANO_HZ 97656250LL
> -
> -#define ADXL345_POWER_CTL_MEASURE BIT(3)
> -#define ADXL345_POWER_CTL_STANDBY 0x00
> -
> -#define ADXL345_DATA_FORMAT_FULL_RES BIT(3) /* Up to 13-bits resolution */
> -#define ADXL345_DATA_FORMAT_2G 0
> -#define ADXL345_DATA_FORMAT_4G 1
> -#define ADXL345_DATA_FORMAT_8G 2
> -#define ADXL345_DATA_FORMAT_16G 3
Why?
..
>
> return devm_iio_device_register(dev, indio_dev);
> }
> diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c
> index a3084b0a8..3f882e2e0 100644
> --- a/drivers/iio/accel/adxl345_i2c.c
> +++ b/drivers/iio/accel/adxl345_i2c.c
> @@ -9,6 +9,7 @@
> */
>
> #include <linux/i2c.h>
> +#include <linux/mod_devicetable.h>
One more... how is this related?
> #include <linux/module.h>
> #include <linux/regmap.h>
>
> @@ -21,41 +22,36 @@ static const struct regmap_config adxl345_i2c_regmap_config = {
>
> static int adxl345_i2c_probe(struct i2c_client *client)
> {
> + const struct adxl345_chip_info *chip_data;
> struct regmap *regmap;
>
> + /* Retrieve device data, i.e. the name, to pass it to the core */
> + chip_data = i2c_get_match_data(client);
> +
> regmap = devm_regmap_init_i2c(client, &adxl345_i2c_regmap_config);
> if (IS_ERR(regmap))
> - return dev_err_probe(&client->dev, PTR_ERR(regmap), "Error initializing regmap\n");
> + return dev_err_probe(&client->dev, PTR_ERR(regmap),
> + "Error initializing regmap\n");
How is this change related to your commit?
Stop doing unrelated changes.
>
> - return adxl345_core_probe(&client->dev, regmap);
> + return adxl345_core_probe(&client->dev, regmap, chip_data, NULL);
> }
>
> -static const struct adxl345_chip_info adxl345_i2c_info = {
> - .name = "adxl345",
> - .uscale = ADXL345_USCALE,
> -};
..
> MODULE_DEVICE_TABLE(acpi, adxl345_acpi_match);
> diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
> index 93ca349f1..c26bac462 100644
> --- a/drivers/iio/accel/adxl345_spi.c
> +++ b/drivers/iio/accel/adxl345_spi.c
> @@ -22,8 +22,14 @@ static const struct regmap_config adxl345_spi_regmap_config = {
>
> static int adxl345_spi_probe(struct spi_device *spi)
> {
> + const struct adxl345_chip_info *chip_data;
> struct regmap *regmap;
>
> + /* Retrieve device name to pass it as driver specific data */
> + chip_data = device_get_match_data(&spi->dev);
> + if (!chip_data)
> + chip_data = spi_get_device_match_data(spi);
That's not how you use it spi_get_device_match_data(). Open the function
and read it... it should be obvious that you now duplicate code.
Best regards,
Krzysztof
On 22/03/2024 01:37, Lothar Rubusch wrote:
> Move driver wide constants and fields into the header.
Why?
> Let probe call a separate setup function. Provide
Why?
> possibility for an SPI/I2C specific setup to be passed
> as function pointer to core.
Why?
Your commit message *MUST* explain why you are doing things.
>
> Signed-off-by: Lothar Rubusch <[email protected]>
> ---
> drivers/iio/accel/adxl345.h | 44 +++++++++++-
> drivers/iio/accel/adxl345_core.c | 117 +++++++++++++++++--------------
> drivers/iio/accel/adxl345_i2c.c | 30 ++++----
> drivers/iio/accel/adxl345_spi.c | 28 ++++----
> 4 files changed, 134 insertions(+), 85 deletions(-)
>
> diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> index 284bd387c..01493c999 100644
> --- a/drivers/iio/accel/adxl345.h
> +++ b/drivers/iio/accel/adxl345.h
> @@ -8,6 +8,39 @@
> #ifndef _ADXL345_H_
> #define _ADXL345_H_
>
> +#include <linux/iio/iio.h>
> +
> +/* ADXL345 register definitions */
> +#define ADXL345_REG_DEVID 0x00
> +#define ADXL345_REG_OFSX 0x1E
> +#define ADXL345_REG_OFSY 0x1F
> +#define ADXL345_REG_OFSZ 0x20
> +#define ADXL345_REG_OFS_AXIS(index) (ADXL345_REG_OFSX + (index))
> +#define ADXL345_REG_BW_RATE 0x2C
> +#define ADXL345_REG_POWER_CTL 0x2D
> +#define ADXL345_REG_DATA_FORMAT 0x31
> +#define ADXL345_REG_DATAX0 0x32
> +#define ADXL345_REG_DATAY0 0x34
> +#define ADXL345_REG_DATAZ0 0x36
> +#define ADXL345_REG_DATA_AXIS(index) \
> + (ADXL345_REG_DATAX0 + (index) * sizeof(__le16))
> +
> +#define ADXL345_BW_RATE GENMASK(3, 0)
> +#define ADXL345_BASE_RATE_NANO_HZ 97656250LL
> +
> +#define ADXL345_POWER_CTL_MEASURE BIT(3)
> +#define ADXL345_POWER_CTL_STANDBY 0x00
> +
> +#define ADXL345_DATA_FORMAT_FULL_RES BIT(3) /* Up to 13-bits resolution */
> +#define ADXL345_DATA_FORMAT_SPI BIT(6) /* spi-3wire */
> +#define ADXL345_DATA_FORMAT_2G 0
> +#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
> +
> /*
> * In full-resolution mode, scale factor is maintained at ~4 mg/LSB
> * in all g ranges.
> @@ -23,11 +56,20 @@
> */
> #define ADXL375_USCALE 480000
>
> +enum adxl345_device_type {
> + ADXL345,
> + ADXL375,
> +};
> +
> struct adxl345_chip_info {
> const char *name;
> int uscale;
> };
>
> -int adxl345_core_probe(struct device *dev, struct regmap *regmap);
> +extern const struct adxl345_chip_info adxl3x5_chip_info[];
> +
> +int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> + const struct adxl345_chip_info *chip_info,
> + int (*setup)(struct device*, struct regmap*));
Last setup argument is entirely unused. Drop this change, it's not
related to this patchset. Neither explained.
Best regards,
Krzysztof
On Fri, 2024-03-22 at 00:37 +0000, Lothar Rubusch wrote:
> Move driver wide constants and fields into the header.
> Let probe call a separate setup function. Provide
> possibility for an SPI/I2C specific setup to be passed
> as function pointer to core.
>
> Signed-off-by: Lothar Rubusch <[email protected]>
> ---
> drivers/iio/accel/adxl345.h | 44 +++++++++++-
> drivers/iio/accel/adxl345_core.c | 117 +++++++++++++++++--------------
> drivers/iio/accel/adxl345_i2c.c | 30 ++++----
> drivers/iio/accel/adxl345_spi.c | 28 ++++----
> 4 files changed, 134 insertions(+), 85 deletions(-)
>
> diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> index 284bd387c..01493c999 100644
> --- a/drivers/iio/accel/adxl345.h
> +++ b/drivers/iio/accel/adxl345.h
> @@ -8,6 +8,39 @@
> #ifndef _ADXL345_H_
> #define _ADXL345_H_
>
> +#include <linux/iio/iio.h>
> +
> +/* ADXL345 register definitions */
> +#define ADXL345_REG_DEVID 0x00
> +#define ADXL345_REG_OFSX 0x1E
> +#define ADXL345_REG_OFSY 0x1F
> +#define ADXL345_REG_OFSZ 0x20
> +#define ADXL345_REG_OFS_AXIS(index) (ADXL345_REG_OFSX + (index))
> +#define ADXL345_REG_BW_RATE 0x2C
> +#define ADXL345_REG_POWER_CTL 0x2D
> +#define ADXL345_REG_DATA_FORMAT 0x31
> +#define ADXL345_REG_DATAX0 0x32
> +#define ADXL345_REG_DATAY0 0x34
> +#define ADXL345_REG_DATAZ0 0x36
> +#define ADXL345_REG_DATA_AXIS(index) \
> + (ADXL345_REG_DATAX0 + (index) * sizeof(__le16))
> +
> +#define ADXL345_BW_RATE GENMASK(3, 0)
> +#define ADXL345_BASE_RATE_NANO_HZ 97656250LL
> +
> +#define ADXL345_POWER_CTL_MEASURE BIT(3)
> +#define ADXL345_POWER_CTL_STANDBY 0x00
> +
> +#define ADXL345_DATA_FORMAT_FULL_RES BIT(3) /* Up to 13-bits resolution */
> +#define ADXL345_DATA_FORMAT_SPI BIT(6) /* spi-3wire */
> +#define ADXL345_DATA_FORMAT_2G 0
> +#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
> +
> /*
> * In full-resolution mode, scale factor is maintained at ~4 mg/LSB
> * in all g ranges.
> @@ -23,11 +56,20 @@
> */
> #define ADXL375_USCALE 480000
>
> +enum adxl345_device_type {
> + ADXL345,
> + ADXL375,
> +};
> +
> struct adxl345_chip_info {
> const char *name;
> int uscale;
> };
>
> -int adxl345_core_probe(struct device *dev, struct regmap *regmap);
> +extern const struct adxl345_chip_info adxl3x5_chip_info[];
> +
> +int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> + const struct adxl345_chip_info *chip_info,
> + 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..040c3f05a 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -17,38 +17,9 @@
>
> #include "adxl345.h"
>
> -#define ADXL345_REG_DEVID 0x00
> -#define ADXL345_REG_OFSX 0x1e
> -#define ADXL345_REG_OFSY 0x1f
> -#define ADXL345_REG_OFSZ 0x20
> -#define ADXL345_REG_OFS_AXIS(index) (ADXL345_REG_OFSX + (index))
> -#define ADXL345_REG_BW_RATE 0x2C
> -#define ADXL345_REG_POWER_CTL 0x2D
> -#define ADXL345_REG_DATA_FORMAT 0x31
> -#define ADXL345_REG_DATAX0 0x32
> -#define ADXL345_REG_DATAY0 0x34
> -#define ADXL345_REG_DATAZ0 0x36
> -#define ADXL345_REG_DATA_AXIS(index) \
> - (ADXL345_REG_DATAX0 + (index) * sizeof(__le16))
> -
> -#define ADXL345_BW_RATE GENMASK(3, 0)
> -#define ADXL345_BASE_RATE_NANO_HZ 97656250LL
> -
> -#define ADXL345_POWER_CTL_MEASURE BIT(3)
> -#define ADXL345_POWER_CTL_STANDBY 0x00
> -
> -#define ADXL345_DATA_FORMAT_FULL_RES BIT(3) /* Up to 13-bits resolution */
> -#define ADXL345_DATA_FORMAT_2G 0
> -#define ADXL345_DATA_FORMAT_4G 1
> -#define ADXL345_DATA_FORMAT_8G 2
> -#define ADXL345_DATA_FORMAT_16G 3
> -
> -#define ADXL345_DEVID 0xE5
> -
> struct adxl345_data {
> const struct adxl345_chip_info *info;
> struct regmap *regmap;
> - u8 data_range;
> };
>
> #define ADXL345_CHANNEL(index, axis) { \
> @@ -62,6 +33,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),
> @@ -197,14 +180,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)
> +static int adxl345_setup(struct device *dev, struct adxl345_data *data,
> + int (*setup)(struct device*, struct regmap*))
> {
> - struct adxl345_data *data;
> - struct iio_dev *indio_dev;
> u32 regval;
> int ret;
>
> - ret = regmap_read(regmap, ADXL345_REG_DEVID, ®val);
> + /* Perform bus specific settings if available */
> + if (setup) {
> + ret = setup(dev, data->regmap);
> + if (ret)
> + return ret;
> + }
nit: likely a better name would be bus_setup(). Then you could drop the comment as it
becomes useless...
> +
> + /* Read out DEVID */
> + ret = regmap_read(data->regmap, ADXL345_REG_DEVID, ®val);
> if (ret < 0)
> return dev_err_probe(dev, ret, "Error reading device ID\n");
>
> @@ -212,37 +202,62 @@ int adxl345_core_probe(struct device *dev, struct regmap
> *regmap)
> return dev_err_probe(dev, -ENODEV, "Invalid device ID: %x,
> expected %x\n",
> regval, ADXL345_DEVID);
>
> + /* Update data_format to full-resolution mode */
> + ret = regmap_update_bits(data->regmap, ADXL345_REG_DATA_FORMAT,
> + ADXL345_DATA_FORMAT_MSK,
> ADXL345_DATA_FORMAT_FULL_RES);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to update data_format
> register\n");
> +
> + /* Enable measurement mode */
> + ret = adxl345_powerup(data->regmap);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "Failed to enable measurement
> mode\n");
> +
> + ret = devm_add_action_or_reset(dev, adxl345_powerdown, data->regmap);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +/**
> + * 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
> + * @chip_info: Structure containing device specific data
> + * @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,
> + const struct adxl345_chip_info *chip_info,
> + int (*setup)(struct device*, struct regmap*))
> +{
> + struct adxl345_data *data;
> + struct iio_dev *indio_dev;
> + int ret;
> +
> indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> if (!indio_dev)
> return -ENOMEM;
>
> 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)
> - return dev_err_probe(dev, ret, "Failed to set data range\n");
> + data->info = chip_info;
>
> - indio_dev->name = data->info->name;
> + indio_dev->name = chip_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 measurement mode */
> - ret = adxl345_powerup(data->regmap);
> - if (ret < 0)
> - return dev_err_probe(dev, ret, "Failed to enable measurement
> mode\n");
> -
> - ret = devm_add_action_or_reset(dev, adxl345_powerdown, data->regmap);
> - if (ret < 0)
> + ret = adxl345_setup(dev, data, setup);
> + if (ret) {
> + dev_err(dev, "ADXL345 setup failed\n");
> return ret;
> + }
>
> return devm_iio_device_register(dev, indio_dev);
> }
> diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c
> index a3084b0a8..3f882e2e0 100644
> --- a/drivers/iio/accel/adxl345_i2c.c
> +++ b/drivers/iio/accel/adxl345_i2c.c
> @@ -9,6 +9,7 @@
> */
>
> #include <linux/i2c.h>
> +#include <linux/mod_devicetable.h>
> #include <linux/module.h>
> #include <linux/regmap.h>
>
> @@ -21,41 +22,36 @@ static const struct regmap_config adxl345_i2c_regmap_config = {
>
> static int adxl345_i2c_probe(struct i2c_client *client)
> {
> + const struct adxl345_chip_info *chip_data;
> struct regmap *regmap;
>
> + /* Retrieve device data, i.e. the name, to pass it to the core */
> + chip_data = i2c_get_match_data(client);
> +
While unlikely use a proper pattern. Meaning, check for NULL pointers and bail out in
that case... Also, you need to justify why are you moving these calls to the bus in
your commit message.
It seems to me that you're trying to refactor too much in a single patch. Maybe step
back and try to separate changes in different patches. Like this one (passing
chip_info) from the bus file could be in it's own patch. Will also (or should at
least :)) "force" you to have a more dedicated commit message explaining why you're
introducing the change.
- Nuno Sá
On Fri, 2024-03-22 at 06:53 +0100, Krzysztof Kozlowski wrote:
> On 22/03/2024 01:37, Lothar Rubusch wrote:
> > Move driver wide constants and fields into the header.
>
> Why?
>
> > Let probe call a separate setup function. Provide
>
> Why?
>
> > possibility for an SPI/I2C specific setup to be passed
> > as function pointer to core.
>
> Why?
>
> Your commit message *MUST* explain why you are doing things.
>
> >
> > Signed-off-by: Lothar Rubusch <[email protected]>
> > ---
> > drivers/iio/accel/adxl345.h | 44 +++++++++++-
> > drivers/iio/accel/adxl345_core.c | 117 +++++++++++++++++--------------
> > drivers/iio/accel/adxl345_i2c.c | 30 ++++----
> > drivers/iio/accel/adxl345_spi.c | 28 ++++----
> > 4 files changed, 134 insertions(+), 85 deletions(-)
> >
> > diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> > index 284bd387c..01493c999 100644
> > --- a/drivers/iio/accel/adxl345.h
> > +++ b/drivers/iio/accel/adxl345.h
> > @@ -8,6 +8,39 @@
> > #ifndef _ADXL345_H_
> > #define _ADXL345_H_
> >
> > +#include <linux/iio/iio.h>
> > +
> > +/* ADXL345 register definitions */
> > +#define ADXL345_REG_DEVID 0x00
> > +#define ADXL345_REG_OFSX 0x1E
> > +#define ADXL345_REG_OFSY 0x1F
> > +#define ADXL345_REG_OFSZ 0x20
> > +#define ADXL345_REG_OFS_AXIS(index) (ADXL345_REG_OFSX + (index))
> > +#define ADXL345_REG_BW_RATE 0x2C
> > +#define ADXL345_REG_POWER_CTL 0x2D
> > +#define ADXL345_REG_DATA_FORMAT 0x31
> > +#define ADXL345_REG_DATAX0 0x32
> > +#define ADXL345_REG_DATAY0 0x34
> > +#define ADXL345_REG_DATAZ0 0x36
> > +#define ADXL345_REG_DATA_AXIS(index) \
> > + (ADXL345_REG_DATAX0 + (index) * sizeof(__le16))
> > +
> > +#define ADXL345_BW_RATE GENMASK(3, 0)
> > +#define ADXL345_BASE_RATE_NANO_HZ 97656250LL
> > +
> > +#define ADXL345_POWER_CTL_MEASURE BIT(3)
> > +#define ADXL345_POWER_CTL_STANDBY 0x00
> > +
> > +#define ADXL345_DATA_FORMAT_FULL_RES BIT(3) /* Up to 13-bits resolution */
> > +#define ADXL345_DATA_FORMAT_SPI BIT(6) /* spi-3wire */
> > +#define ADXL345_DATA_FORMAT_2G 0
> > +#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
> > +
> > /*
> > * In full-resolution mode, scale factor is maintained at ~4 mg/LSB
> > * in all g ranges.
> > @@ -23,11 +56,20 @@
> > */
> > #define ADXL375_USCALE 480000
> >
> > +enum adxl345_device_type {
> > + ADXL345,
> > + ADXL375,
> > +};
> > +
> > struct adxl345_chip_info {
> > const char *name;
> > int uscale;
> > };
> >
> > -int adxl345_core_probe(struct device *dev, struct regmap *regmap);
> > +extern const struct adxl345_chip_info adxl3x5_chip_info[];
> > +
> > +int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> > + const struct adxl345_chip_info *chip_info,
> > + int (*setup)(struct device*, struct regmap*));
>
> Last setup argument is entirely unused. Drop this change, it's not
> related to this patchset. Neither explained.
>
Yeah, you need to make it explicit in the message that this change is in preparation
for a future change (adding the 3-wire spi mode). Otherwise it's natural for
reviewers to make questions about it... Maybe another one that could live in it#s own
patch.
- Nuno Sá
>
On Fri, Mar 22, 2024 at 3:17 AM Rob Herring <[email protected]> wrote:
>
> On Fri, Mar 22, 2024 at 12:37:13AM +0000, Lothar Rubusch wrote:
> > Provide the optional spi-3wire in the example.
>
> That doesn't match the diff as you don't touch the example. But really,
> this should say why you need spi-3wire.
I understand. The change does not add anything to the example. which
is definitely wrong.
Anyway I'm unsure about this change in particular. I know the spi-3wire
binding exists and can be implemented. Not all spi devices offer it. Not all
drivers implement it. My patch set tries to implement spi-3wire for the
particular accelerometer.
Do I need to add something here to dt-bindings documentation of the
adxl345? Or, as an optional spi feature, is it covered anyway by
documentation of optional spi bindings? So, should I refrase this particular
patch or may I drop it entirely? Could you please clarify.
>
> >
> > 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
> >
(...)
> > Signed-off-by: Lothar Rubusch <[email protected]>
> > ---
> > drivers/iio/accel/adxl345.h | 44 +++++++++++-
> > drivers/iio/accel/adxl345_core.c | 117 +++++++++++++++++--------------
> > drivers/iio/accel/adxl345_i2c.c | 30 ++++----
> > drivers/iio/accel/adxl345_spi.c | 28 ++++----
> > 4 files changed, 134 insertions(+), 85 deletions(-)
> >
> > diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> > index 284bd387c..01493c999 100644
> > --- a/drivers/iio/accel/adxl345.h
> > +++ b/drivers/iio/accel/adxl345.h
> > @@ -8,6 +8,39 @@
> > #ifndef _ADXL345_H_
> > #define _ADXL345_H_
> >
> > +#include <linux/iio/iio.h>
> > +
> > +/* ADXL345 register definitions */
> > +#define ADXL345_REG_DEVID 0x00
> > +#define ADXL345_REG_OFSX 0x1E
> > +#define ADXL345_REG_OFSY 0x1F
> > +#define ADXL345_REG_OFSZ 0x20
> > +#define ADXL345_REG_OFS_AXIS(index) (ADXL345_REG_OFSX + (index))
> > +#define ADXL345_REG_BW_RATE 0x2C
> > +#define ADXL345_REG_POWER_CTL 0x2D
> > +#define ADXL345_REG_DATA_FORMAT 0x31
> > +#define ADXL345_REG_DATAX0 0x32
> > +#define ADXL345_REG_DATAY0 0x34
> > +#define ADXL345_REG_DATAZ0 0x36
> > +#define ADXL345_REG_DATA_AXIS(index) \
> > + (ADXL345_REG_DATAX0 + (index) * sizeof(__le16))
> > +
> > +#define ADXL345_BW_RATE GENMASK(3, 0)
> > +#define ADXL345_BASE_RATE_NANO_HZ 97656250LL
> > +
> > +#define ADXL345_POWER_CTL_MEASURE BIT(3)
> > +#define ADXL345_POWER_CTL_STANDBY 0x00
> > +
> > +#define ADXL345_DATA_FORMAT_FULL_RES BIT(3) /* Up to 13-bits resolution */
> > +#define ADXL345_DATA_FORMAT_SPI BIT(6) /* spi-3wire */
> > +#define ADXL345_DATA_FORMAT_2G 0
> > +#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
> > +
(...)
I think I see your point. My patch has more noise and lacks a logic
structure in proceding.
I will resubmit, but may I ask one question in particular. I moved the
entire list of register
defines from the adxl345_core.c to the common adxl345.h.
For setting spi-3wire with my approach, only two of those defines are
needed. I think it is
nicer for readability to keep the defines together, though, in a
commonly shared header.
Nevertheless most of the defines are just used locally in the .._core.c
Should I move them for refactory?
I feel there is no reason to move them. On the other hand I see many
drivers keep them in a common header. Hence, is there a best practice
which justifies moving them to a header?
On 23/03/2024 13:04, Lothar Rubusch wrote:
> On Fri, Mar 22, 2024 at 3:17 AM Rob Herring <[email protected]> wrote:
>>
>> On Fri, Mar 22, 2024 at 12:37:13AM +0000, Lothar Rubusch wrote:
>>> Provide the optional spi-3wire in the example.
>>
>> That doesn't match the diff as you don't touch the example. But really,
>> this should say why you need spi-3wire.
>
> I understand. The change does not add anything to the example. which
> is definitely wrong.
> Anyway I'm unsure about this change in particular. I know the spi-3wire
> binding exists and can be implemented. Not all spi devices offer it. Not all
> drivers implement it. My patch set tries to implement spi-3wire for the
> particular accelerometer.
> Do I need to add something here to dt-bindings documentation of the
> adxl345? Or, as an optional spi feature, is it covered anyway by
> documentation of optional spi bindings? So, should I refrase this particular
> patch or may I drop it entirely? Could you please clarify.
Whether you need to change bindings or not, dtbs_check will tell you.
Just run dtbs_check on your DTS.
It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).
AFAIR, spi-3wire requires being explicitly mentioned in the device bindings.
Best regards,
Krzysztof
On Sat, Mar 23, 2024 at 3:27 PM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 23/03/2024 13:04, Lothar Rubusch wrote:
> > On Fri, Mar 22, 2024 at 3:17 AM Rob Herring <[email protected]> wrote:
> >>
> >> On Fri, Mar 22, 2024 at 12:37:13AM +0000, Lothar Rubusch wrote:
> >>> Provide the optional spi-3wire in the example.
> >>
> >> That doesn't match the diff as you don't touch the example. But really,
> >> this should say why you need spi-3wire.
> >
> > I understand. The change does not add anything to the example. which
> > is definitely wrong.
> > Anyway I'm unsure about this change in particular. I know the spi-3wire
> > binding exists and can be implemented. Not all spi devices offer it. Not all
> > drivers implement it. My patch set tries to implement spi-3wire for the
> > particular accelerometer.
> > Do I need to add something here to dt-bindings documentation of the
> > adxl345? Or, as an optional spi feature, is it covered anyway by
> > documentation of optional spi bindings? So, should I refrase this particular
> > patch or may I drop it entirely? Could you please clarify.
>
> Whether you need to change bindings or not, dtbs_check will tell you.
> Just run dtbs_check on your DTS.
>
I'm not changing upstream DTS. At most, the documentation should
mention something.
> It does not look like you tested the DTS against bindings. Please run
> `make dtbs_check W=1` (see
> Documentation/devicetree/bindings/writing-schema.rst or
> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> for instructions).
>
No, I didn't. dtbs_check did not work right out of the box, but it
sounds great and I will figure out. Currently my setup is a bit
customized. I compile the modules out of tree, dockerized with several
DTBOs. I use an automized setup to verify spi, spi-3wire and i2c
probing still works on the hardware. It is tested at least somehow.
> AFAIR, spi-3wire requires being explicitly mentioned in the device bindings.
>
>
> Best regards,
> Krzysztof
>
On Sat, 23 Mar 2024 13:16:56 +0100
Lothar Rubusch <[email protected]> wrote:
> (...)
> > > Signed-off-by: Lothar Rubusch <[email protected]>
> > > ---
> > > drivers/iio/accel/adxl345.h | 44 +++++++++++-
> > > drivers/iio/accel/adxl345_core.c | 117 +++++++++++++++++--------------
> > > drivers/iio/accel/adxl345_i2c.c | 30 ++++----
> > > drivers/iio/accel/adxl345_spi.c | 28 ++++----
> > > 4 files changed, 134 insertions(+), 85 deletions(-)
> > >
> > > diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> > > index 284bd387c..01493c999 100644
> > > --- a/drivers/iio/accel/adxl345.h
> > > +++ b/drivers/iio/accel/adxl345.h
> > > @@ -8,6 +8,39 @@
> > > #ifndef _ADXL345_H_
> > > #define _ADXL345_H_
> > >
> > > +#include <linux/iio/iio.h>
> > > +
> > > +/* ADXL345 register definitions */
> > > +#define ADXL345_REG_DEVID 0x00
> > > +#define ADXL345_REG_OFSX 0x1E
> > > +#define ADXL345_REG_OFSY 0x1F
> > > +#define ADXL345_REG_OFSZ 0x20
> > > +#define ADXL345_REG_OFS_AXIS(index) (ADXL345_REG_OFSX + (index))
> > > +#define ADXL345_REG_BW_RATE 0x2C
> > > +#define ADXL345_REG_POWER_CTL 0x2D
> > > +#define ADXL345_REG_DATA_FORMAT 0x31
> > > +#define ADXL345_REG_DATAX0 0x32
> > > +#define ADXL345_REG_DATAY0 0x34
> > > +#define ADXL345_REG_DATAZ0 0x36
> > > +#define ADXL345_REG_DATA_AXIS(index) \
> > > + (ADXL345_REG_DATAX0 + (index) * sizeof(__le16))
> > > +
> > > +#define ADXL345_BW_RATE GENMASK(3, 0)
> > > +#define ADXL345_BASE_RATE_NANO_HZ 97656250LL
> > > +
> > > +#define ADXL345_POWER_CTL_MEASURE BIT(3)
> > > +#define ADXL345_POWER_CTL_STANDBY 0x00
> > > +
> > > +#define ADXL345_DATA_FORMAT_FULL_RES BIT(3) /* Up to 13-bits resolution */
> > > +#define ADXL345_DATA_FORMAT_SPI BIT(6) /* spi-3wire */
> > > +#define ADXL345_DATA_FORMAT_2G 0
> > > +#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
> > > +
> (...)
>
> I think I see your point. My patch has more noise and lacks a logic
> structure in proceding.
> I will resubmit, but may I ask one question in particular. I moved the
> entire list of register
> defines from the adxl345_core.c to the common adxl345.h.
> For setting spi-3wire with my approach, only two of those defines are
> needed. I think it is
> nicer for readability to keep the defines together, though, in a
> commonly shared header.
> Nevertheless most of the defines are just used locally in the .._core.c
> Should I move them for refactory?
Move them as a block (which you did). It's confusing to have only a subset of
defines in one place.
> I feel there is no reason to move them. On the other hand I see many
> drivers keep them in a common header. Hence, is there a best practice
> which justifies moving them to a header?
On 23/03/2024 18:44, Lothar Rubusch wrote:
> On Sat, Mar 23, 2024 at 3:27 PM Krzysztof Kozlowski
> <[email protected]> wrote:
>>
>> On 23/03/2024 13:04, Lothar Rubusch wrote:
>>> On Fri, Mar 22, 2024 at 3:17 AM Rob Herring <[email protected]> wrote:
>>>>
>>>> On Fri, Mar 22, 2024 at 12:37:13AM +0000, Lothar Rubusch wrote:
>>>>> Provide the optional spi-3wire in the example.
>>>>
>>>> That doesn't match the diff as you don't touch the example. But really,
>>>> this should say why you need spi-3wire.
>>>
>>> I understand. The change does not add anything to the example. which
>>> is definitely wrong.
>>> Anyway I'm unsure about this change in particular. I know the spi-3wire
>>> binding exists and can be implemented. Not all spi devices offer it. Not all
>>> drivers implement it. My patch set tries to implement spi-3wire for the
>>> particular accelerometer.
>>> Do I need to add something here to dt-bindings documentation of the
>>> adxl345? Or, as an optional spi feature, is it covered anyway by
>>> documentation of optional spi bindings? So, should I refrase this particular
>>> patch or may I drop it entirely? Could you please clarify.
>>
>> Whether you need to change bindings or not, dtbs_check will tell you.
>> Just run dtbs_check on your DTS.
>>
>
> I'm not changing upstream DTS. At most, the documentation should
> mention something.
Nothing should stop you testing from downstream DTS...
Best regards,
Krzysztof