Date:   Sat, 29 Oct 2022 16:38:09 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Ramona Bolboaca <ramona.bolboaca@...log.com>
Cc:     <robh+dt@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>,
        <linux-iio@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] drivers:iio:accel: Add support for ADXL359 device

On Fri, 28 Oct 2022 16:44:54 +0300
Ramona Bolboaca <ramona.bolboaca@...log.com> wrote:

> Add support for ADXL359 device in already existing ADXL355 driver.
> 
> Signed-off-by: Ramona Bolboaca <ramona.bolboaca@...log.com>

Hi Ramona,

Various comments inline. Most are around moving more from 'code' to
'data' by adding more elements to your chip_info structure.
Experience tells us that almost always ends up cleaner and more maintainable
in the long run.

Thanks,

Jonathan

> ---
>  drivers/iio/accel/adxl355.h      | 14 ++++++-
>  drivers/iio/accel/adxl355_core.c | 67 +++++++++++++++++++++++++-------
>  drivers/iio/accel/adxl355_i2c.c  | 22 +++++++++--
>  drivers/iio/accel/adxl355_spi.c  | 19 +++++++--
>  4 files changed, 101 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl355.h b/drivers/iio/accel/adxl355.h
> index 6dd49b13e4fd..c02106cbd745 100644
> --- a/drivers/iio/accel/adxl355.h
> +++ b/drivers/iio/accel/adxl355.h
> @@ -10,12 +10,24 @@
>  
>  #include <linux/regmap.h>
>  
> +enum adxl355_device_type {
> +	ADXL355,
> +	ADXL359,
> +};
> +
>  struct device;
>  
> +struct adxl355_chip_info {
> +	const char			*name;
> +	u8				part_id;
> +	enum adxl355_device_type	type;
> +};
> +
>  extern const struct regmap_access_table adxl355_readable_regs_tbl;
>  extern const struct regmap_access_table adxl355_writeable_regs_tbl;
> +extern const struct adxl355_chip_info adxl35x_chip_info[];
>  
>  int adxl355_core_probe(struct device *dev, struct regmap *regmap,
> -		       const char *name);
> +		       const struct adxl355_chip_info *chip_info);
>  
>  #endif /* _ADXL355_H_ */
> diff --git a/drivers/iio/accel/adxl355_core.c b/drivers/iio/accel/adxl355_core.c
> index 4bc648eac8b2..069c945aebde 100644
> --- a/drivers/iio/accel/adxl355_core.c
> +++ b/drivers/iio/accel/adxl355_core.c
> @@ -60,6 +60,7 @@
>  #define ADXL355_DEVID_AD_VAL		0xAD
>  #define ADXL355_DEVID_MST_VAL		0x1D
>  #define ADXL355_PARTID_VAL		0xED
> +#define ADXL359_PARTID_VAL		0xE9
>  #define ADXL355_RESET_CODE		0x52
>  
>  static const struct regmap_range adxl355_read_reg_range[] = {
> @@ -83,6 +84,20 @@ const struct regmap_access_table adxl355_writeable_regs_tbl = {
>  };
>  EXPORT_SYMBOL_NS_GPL(adxl355_writeable_regs_tbl, IIO_ADXL355);
>  
> +const struct adxl355_chip_info adxl35x_chip_info[] = {
> +	[ADXL355] = {
> +		.name = "adxl355",
> +		.part_id = ADXL355_PARTID_VAL,
> +		.type = ADXL355,

A type field always makes me look closely.  Generally we are better off not having
type specific switches in the code, but instead pulling the constant data / callbacks
in here.  That always ends up cleaner as we add more parts to a driver (otherwise
those switch statements keep getting bigger and bigger).

> +	},
> +	[ADXL359] = {
> +		.name = "adxl359",
> +		.part_id = ADXL359_PARTID_VAL,
> +		.type = ADXL359,
> +	},
> +};
> +EXPORT_SYMBOL_NS_GPL(adxl35x_chip_info, IIO_ADXL355);
> +
>  enum adxl355_op_mode {
>  	ADXL355_MEASUREMENT,
>  	ADXL355_STANDBY,
> @@ -162,6 +177,7 @@ static const struct adxl355_chan_info adxl355_chans[] = {
>  };
>  
>  struct adxl355_data {
> +	const struct adxl355_chip_info *chip_info;
>  	struct regmap *regmap;
>  	struct device *dev;
>  	struct mutex lock; /* lock to protect op_mode */
> @@ -262,7 +278,7 @@ static int adxl355_setup(struct adxl355_data *data)
>  	if (ret)
>  		return ret;
>  
> -	if (regval != ADXL355_PARTID_VAL) {
> +	if (regval != data->chip_info->part_id) {

Ah.  Whilst we are here, please relax this constraint to warning only (precusor patch ideally)
If a new device is released that has some minor changes but is compatible with a previous
device, it will be normal to use a fallback compatible so that it works with older kernels.
As things stand today this driver will reject such a situation.

It is fine to warn if it is wrong, just not fail driver probe.

The more complex solution is to check if we know about the device and if so, switch
to using the right values (whilst warning the firmware is wrong).
Doing that is entirely optional though as firmware shouldn't be broken!

>  		dev_err(data->dev, "Invalid DEV ID 0x%02x\n", regval);
>  		return -ENODEV;
>  	}
> @@ -459,31 +475,55 @@ static int adxl355_read_raw(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_SCALE:
>  		switch (chan->type) {
>  		/*
> -		 * The datasheet defines an intercept of 1885 LSB at 25 degC
> -		 * and a slope of -9.05 LSB/C. The following formula can be used
> +		 * The datasheet defines an intercept of 1885 LSB for ADXL355 and of 1852 for ADXL359
> +		 * at 25 degC and a slope of -9.05 LSB/C. The following formula can be used
>  		 * to find the temperature:
> -		 * Temp = ((RAW - 1885)/(-9.05)) + 25 but this doesn't follow
> +		 * Temp = ((RAW - 1885)/(-9.05)) + 25 for ADXL355
> +		 * Temp = ((RAW - 1852)/(-9.05)) + 25 for ADXL359
Once you have a field in chip_info for val2 here (see below), move the documentation of the values
at least alongside the values being specified in those structures.  You can have one detailed
explanation and a follow up one that just says "see ADXL355 description" + provides this
line of the formula. 

> +		 * but this doesn't follow
>  		 * the format of the IIO which is Temp = (RAW + OFFSET) * SCALE.
>  		 * Hence using some rearranging we get the scale as -110.497238
> -		 * and offset as -2111.25.
> +		 * and offset as -2111.25 for ADXL355 and -2079.25 for ADXL359
>  		 */
>  		case IIO_TEMP:
>  			*val = -110;
>  			*val2 = 497238;
>  			return IIO_VAL_INT_PLUS_MICRO;
> -		/*
> -		 * At +/- 2g with 20-bit resolution, scale is given in datasheet
> -		 * as 3.9ug/LSB = 0.0000039 * 9.80665 = 0.00003824593 m/s^2.
> -		 */
>  		case IIO_ACCEL:
> +			switch (data->chip_info->type) {
> +			case ADXL355:
> +				/*
> +				 * At +/- 2g with 20-bit resolution, scale is given in datasheet
> +				 * as 3.9ug/LSB = 0.0000039 * 9.80665 = 0.00003824593 m/s^2.
> +				 */
> +				*val2 = 38245;
> +				break;
> +			case ADXL359:
> +				/*
> +				 * At +/- 10g with 20-bit resolution, scale is given in datasheet
> +				 * as 19.5ug/LSB = 0.0000195 * 9.80665 = 0.0.00019122967 m/s^2.
> +				 */
> +				*val2 = 191229;
> +				break;
> +			default:
> +				return -EINVAL;
> +			}
>  			*val = 0;
> -			*val2 = 38245;
>  			return IIO_VAL_INT_PLUS_NANO;
>  		default:
>  			return -EINVAL;
>  		}
>  	case IIO_CHAN_INFO_OFFSET:
> -		*val = -2111;
> +		switch (data->chip_info->type) {

As a general rule, it is more extensible to put these sorts of values in the
chip_info structure than it is to introduce switch statements that will continue growing
as more parts are added to the driver.  So please add fields for offset and temp_scale

> +		case ADXL355:
> +			*val = -2111;
> +			break;
> +		case ADXL359:
> +			*val = -2079;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
>  		*val2 = 250000;
>  		return IIO_VAL_INT_PLUS_MICRO;
>  	case IIO_CHAN_INFO_CALIBBIAS:
> @@ -707,7 +747,7 @@ static int adxl355_probe_trigger(struct iio_dev *indio_dev, int irq)
>  }
>  

>  	indio_dev->channels = adxl355_channels;
> diff --git a/drivers/iio/accel/adxl355_i2c.c b/drivers/iio/accel/adxl355_i2c.c
> index f67d57921c81..4cf38625fc27 100644
> --- a/drivers/iio/accel/adxl355_i2c.c
> +++ b/drivers/iio/accel/adxl355_i2c.c
> @@ -23,6 +23,20 @@ static const struct regmap_config adxl355_i2c_regmap_config = {
>  static int adxl355_i2c_probe(struct i2c_client *client)
>  {
>  	struct regmap *regmap;
> +	const struct adxl355_chip_info *chip_data;
> +	const struct i2c_device_id *adxl355;
> +
> +	adxl355 = to_i2c_driver(client->dev.driver)->id_table;

There is a discussion ongoing about having something like
spi_device_get_id() for I2C.  This is another good place
to use that.  In the meantime we only use the id_table in the
case that the firmware query fails.  So move it inside the 
if (!chip_data) {} block. There are other reasons that it should exist
but we don't need to care about them here (autoprobing etc)

> +	if (!adxl355)
> +		return -EINVAL;
> +
> +	chip_data = device_get_match_data(&client->dev);
> +	if (!chip_data) {
> +		chip_data = (void *)i2c_match_id(adxl355, client)->driver_data;
> +
> +		if (!chip_data)
> +			return -EINVAL;
> +	}
>  
>  	regmap = devm_regmap_init_i2c(client, &adxl355_i2c_regmap_config);
>  	if (IS_ERR(regmap)) {
> @@ -32,17 +46,19 @@ static int adxl355_i2c_probe(struct i2c_client *client)
>  		return PTR_ERR(regmap);
>  	}
>  
> -	return adxl355_core_probe(&client->dev, regmap, client->name);
> +	return adxl355_core_probe(&client->dev, regmap, chip_data);
>  }
>

Powered by blists - more mailing lists