Re: [PATCH v2 1/3] iio: accel: adxl345: Update adxl345

From: Krzysztof Kozlowski
Date: Fri Mar 22 2024 - 01:52:14 EST

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 <l.rubusch@xxxxxxxxx>
> ---
> 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