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

From: Nuno Sá
Date: Fri Mar 22 2024 - 03:16:35 EST

Next message: Nuno Sá: "Re: [PATCH v2 1/3] iio: accel: adxl345: Update adxl345"
Previous message: Wolfram Sang: "[PULL REQUEST] i2c-for-6.9-rc1-part2"
In reply to: Lothar Rubusch: "Re: [PATCH v2 1/3] iio: accel: adxl345: Update adxl345"
Next in thread: Lothar Rubusch: "[PATCH v2 2/3] iio: accel: adxl345: Add spi-3wire feature"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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 <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
> +
> /*
> * 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, &regval);
> + /* 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, &regval);
> 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á

Next message: Nuno Sá: "Re: [PATCH v2 1/3] iio: accel: adxl345: Update adxl345"
Previous message: Wolfram Sang: "[PULL REQUEST] i2c-for-6.9-rc1-part2"
In reply to: Lothar Rubusch: "Re: [PATCH v2 1/3] iio: accel: adxl345: Update adxl345"
Next in thread: Lothar Rubusch: "[PATCH v2 2/3] iio: accel: adxl345: Add spi-3wire feature"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]