linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] Add ADXL359 support
@ 2022-10-28 13:44 Ramona Bolboaca
  2022-10-28 13:44 ` [PATCH 1/2] dt-bindings:iio:accel: Add docs for ADXL359 Ramona Bolboaca
  2022-10-28 13:44 ` [PATCH 2/2] drivers:iio:accel: Add support for ADXL359 device Ramona Bolboaca
  0 siblings, 2 replies; 6+ messages in thread
From: Ramona Bolboaca @ 2022-10-28 13:44 UTC (permalink / raw)
  To: jic23, robh+dt, krzysztof.kozlowski+dt, linux-iio, devicetree,
	linux-kernel
  Cc: Ramona Bolboaca

Add support for ADXL359 device in existing ADXL355 driver. 

The digital output ADXL359 is a low noise density, low 0 g offset drift,
low power, 3-axis microelectromechanical system (MEMS) accelerometer with
selectable measurement ranges. The ADXL359 supports the ±10 g, ±20 g, 
and ±40 g ranges.

Ramona Bolboaca (2):
  dt-bindings:iio:accel: Add docs for ADXL359
  drivers:iio:accel: Add support for ADXL359 device

 .../bindings/iio/accel/adi,adxl355.yaml       |  8 ++-
 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 ++++--
 5 files changed, 106 insertions(+), 24 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/2] dt-bindings:iio:accel: Add docs for ADXL359
  2022-10-28 13:44 [PATCH 0/2] Add ADXL359 support Ramona Bolboaca
@ 2022-10-28 13:44 ` Ramona Bolboaca
  2022-10-28 14:45   ` Krzysztof Kozlowski
  2022-10-29 15:22   ` Jonathan Cameron
  2022-10-28 13:44 ` [PATCH 2/2] drivers:iio:accel: Add support for ADXL359 device Ramona Bolboaca
  1 sibling, 2 replies; 6+ messages in thread
From: Ramona Bolboaca @ 2022-10-28 13:44 UTC (permalink / raw)
  To: jic23, robh+dt, krzysztof.kozlowski+dt, linux-iio, devicetree,
	linux-kernel
  Cc: Ramona Bolboaca

Update ADXL355 existing documentation with documentation
for ADXL359 dedvice.

Signed-off-by: Ramona Bolboaca <ramona.bolboaca@analog.com>
---
 .../devicetree/bindings/iio/accel/adi,adxl355.yaml        | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adxl355.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adxl355.yaml
index 14b487088ab4..93ad7ff6b355 100644
--- a/Documentation/devicetree/bindings/iio/accel/adi,adxl355.yaml
+++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl355.yaml
@@ -4,20 +4,22 @@
 $id: http://devicetree.org/schemas/iio/accel/adi,adxl355.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: Analog Devices ADXL355 3-Axis, Low noise MEMS Accelerometer
+title: Analog Devices ADXL355 and ADXL359 3-Axis, Low noise MEMS Accelerometer
 
 maintainers:
   - Puranjay Mohan <puranjay12@gmail.com>
 
 description: |
-  Analog Devices ADXL355 3-Axis, Low noise MEMS Accelerometer that supports
-  both I2C & SPI interfaces
+  Analog Devices ADXL355 and ADXL359 3-Axis, Low noise MEMS Accelerometer that
+  supports both I2C & SPI interfaces
     https://www.analog.com/en/products/adxl355.html
+    https://www.analog.com/en/products/adxl359.html
 
 properties:
   compatible:
     enum:
       - adi,adxl355
+      - adi,adxl359
 
   reg:
     maxItems: 1
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/2] drivers:iio:accel: Add support for ADXL359 device
  2022-10-28 13:44 [PATCH 0/2] Add ADXL359 support Ramona Bolboaca
  2022-10-28 13:44 ` [PATCH 1/2] dt-bindings:iio:accel: Add docs for ADXL359 Ramona Bolboaca
@ 2022-10-28 13:44 ` Ramona Bolboaca
  2022-10-29 15:38   ` Jonathan Cameron
  1 sibling, 1 reply; 6+ messages in thread
From: Ramona Bolboaca @ 2022-10-28 13:44 UTC (permalink / raw)
  To: jic23, robh+dt, krzysztof.kozlowski+dt, linux-iio, devicetree,
	linux-kernel
  Cc: Ramona Bolboaca

Add support for ADXL359 device in already existing ADXL355 driver.

Signed-off-by: Ramona Bolboaca <ramona.bolboaca@analog.com>
---
 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,
+	},
+	[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) {
 		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
+		 * 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) {
+		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)
 }
 
 int adxl355_core_probe(struct device *dev, struct regmap *regmap,
-		       const char *name)
+		       const struct adxl355_chip_info *chip_info)
 {
 	struct adxl355_data *data;
 	struct iio_dev *indio_dev;
@@ -722,9 +762,10 @@ int adxl355_core_probe(struct device *dev, struct regmap *regmap,
 	data->regmap = regmap;
 	data->dev = dev;
 	data->op_mode = ADXL355_STANDBY;
+	data->chip_info = chip_info;
 	mutex_init(&data->lock);
 
-	indio_dev->name = name;
+	indio_dev->name = chip_info->name;
 	indio_dev->info = &adxl355_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	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;
+	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);
 }
 
 static const struct i2c_device_id adxl355_i2c_id[] = {
-	{ "adxl355", 0 },
+	{ "adxl355", (kernel_ulong_t)&adxl35x_chip_info[ADXL355] },
+	{ "adxl359", (kernel_ulong_t)&adxl35x_chip_info[ADXL359] },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, adxl355_i2c_id);
 
 static const struct of_device_id adxl355_of_match[] = {
-	{ .compatible = "adi,adxl355" },
+	{ .compatible = "adi,adxl355", .data = &adxl35x_chip_info[ADXL355] },
+	{ .compatible = "adi,adxl359", .data = &adxl35x_chip_info[ADXL359] },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, adxl355_of_match);
diff --git a/drivers/iio/accel/adxl355_spi.c b/drivers/iio/accel/adxl355_spi.c
index 5fe986ae03f6..fc99534d91ff 100644
--- a/drivers/iio/accel/adxl355_spi.c
+++ b/drivers/iio/accel/adxl355_spi.c
@@ -9,6 +9,7 @@
 #include <linux/mod_devicetable.h>
 #include <linux/regmap.h>
 #include <linux/spi/spi.h>
+#include <linux/property.h>
 
 #include "adxl355.h"
 
@@ -24,9 +25,17 @@ static const struct regmap_config adxl355_spi_regmap_config = {
 
 static int adxl355_spi_probe(struct spi_device *spi)
 {
-	const struct spi_device_id *id = spi_get_device_id(spi);
+	const struct adxl355_chip_info *chip_data;
 	struct regmap *regmap;
 
+	chip_data = device_get_match_data(&spi->dev);
+	if (!chip_data) {
+		chip_data = (void *)spi_get_device_id(spi)->driver_data;
+
+		if (!chip_data)
+			return -EINVAL;
+	}
+
 	regmap = devm_regmap_init_spi(spi, &adxl355_spi_regmap_config);
 	if (IS_ERR(regmap)) {
 		dev_err(&spi->dev, "Error initializing spi regmap: %ld\n",
@@ -35,17 +44,19 @@ static int adxl355_spi_probe(struct spi_device *spi)
 		return PTR_ERR(regmap);
 	}
 
-	return adxl355_core_probe(&spi->dev, regmap, id->name);
+	return adxl355_core_probe(&spi->dev, regmap, chip_data);
 }
 
 static const struct spi_device_id adxl355_spi_id[] = {
-	{ "adxl355", 0 },
+	{ "adxl355", (kernel_ulong_t)&adxl35x_chip_info[ADXL355] },
+	{ "adxl359", (kernel_ulong_t)&adxl35x_chip_info[ADXL359] },
 	{ }
 };
 MODULE_DEVICE_TABLE(spi, adxl355_spi_id);
 
 static const struct of_device_id adxl355_of_match[] = {
-	{ .compatible = "adi,adxl355" },
+	{ .compatible = "adi,adxl355", .data = &adxl35x_chip_info[ADXL355] },
+	{ .compatible = "adi,adxl359", .data = &adxl35x_chip_info[ADXL359] },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, adxl355_of_match);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] dt-bindings:iio:accel: Add docs for ADXL359
  2022-10-28 13:44 ` [PATCH 1/2] dt-bindings:iio:accel: Add docs for ADXL359 Ramona Bolboaca
@ 2022-10-28 14:45   ` Krzysztof Kozlowski
  2022-10-29 15:22   ` Jonathan Cameron
  1 sibling, 0 replies; 6+ messages in thread
From: Krzysztof Kozlowski @ 2022-10-28 14:45 UTC (permalink / raw)
  To: Ramona Bolboaca, jic23, robh+dt, krzysztof.kozlowski+dt,
	linux-iio, devicetree, linux-kernel

On 28/10/2022 09:44, Ramona Bolboaca wrote:
> Update ADXL355 existing documentation with documentation
> for ADXL359 dedvice.

Missing spaces in subject. Look at Git how subject should be formatted.

> 
> Signed-off-by: Ramona Bolboaca <ramona.bolboaca@analog.com>
> ---
>  .../devicetree/bindings/iio/accel/adi,adxl355.yaml        | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 

With subject fixed:

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] dt-bindings:iio:accel: Add docs for ADXL359
  2022-10-28 13:44 ` [PATCH 1/2] dt-bindings:iio:accel: Add docs for ADXL359 Ramona Bolboaca
  2022-10-28 14:45   ` Krzysztof Kozlowski
@ 2022-10-29 15:22   ` Jonathan Cameron
  1 sibling, 0 replies; 6+ messages in thread
From: Jonathan Cameron @ 2022-10-29 15:22 UTC (permalink / raw)
  To: Ramona Bolboaca
  Cc: robh+dt, krzysztof.kozlowski+dt, linux-iio, devicetree, linux-kernel

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

> Update ADXL355 existing documentation with documentation
> for ADXL359 dedvice.
> 
> Signed-off-by: Ramona Bolboaca <ramona.bolboaca@analog.com>
> ---
>  .../devicetree/bindings/iio/accel/adi,adxl355.yaml        | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adxl355.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adxl355.yaml
> index 14b487088ab4..93ad7ff6b355 100644
> --- a/Documentation/devicetree/bindings/iio/accel/adi,adxl355.yaml
> +++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl355.yaml
> @@ -4,20 +4,22 @@
>  $id: http://devicetree.org/schemas/iio/accel/adi,adxl355.yaml#
>  $schema: http://devicetree.org/meta-schemas/core.yaml#
>  
> -title: Analog Devices ADXL355 3-Axis, Low noise MEMS Accelerometer
> +title: Analog Devices ADXL355 and ADXL359 3-Axis, Low noise MEMS Accelerometer
trivial: Accelerometers
>  
>  maintainers:
>    - Puranjay Mohan <puranjay12@gmail.com>
>  
>  description: |
> -  Analog Devices ADXL355 3-Axis, Low noise MEMS Accelerometer that supports
> -  both I2C & SPI interfaces
> +  Analog Devices ADXL355 and ADXL359 3-Axis, Low noise MEMS Accelerometer that
trivial: Accelerometers

Fix it if we need a v2 for other reasons. If not I can do it whilst applying.

Jonathan

> +  supports both I2C & SPI interfaces
>      https://www.analog.com/en/products/adxl355.html
> +    https://www.analog.com/en/products/adxl359.html
>  
>  properties:
>    compatible:
>      enum:
>        - adi,adxl355
> +      - adi,adxl359
>  
>    reg:
>      maxItems: 1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] drivers:iio:accel: Add support for ADXL359 device
  2022-10-28 13:44 ` [PATCH 2/2] drivers:iio:accel: Add support for ADXL359 device Ramona Bolboaca
@ 2022-10-29 15:38   ` Jonathan Cameron
  0 siblings, 0 replies; 6+ messages in thread
From: Jonathan Cameron @ 2022-10-29 15:38 UTC (permalink / raw)
  To: Ramona Bolboaca
  Cc: robh+dt, krzysztof.kozlowski+dt, linux-iio, devicetree, linux-kernel

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

> Add support for ADXL359 device in already existing ADXL355 driver.
> 
> Signed-off-by: Ramona Bolboaca <ramona.bolboaca@analog.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);
>  }
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-10-29 15:26 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-28 13:44 [PATCH 0/2] Add ADXL359 support Ramona Bolboaca
2022-10-28 13:44 ` [PATCH 1/2] dt-bindings:iio:accel: Add docs for ADXL359 Ramona Bolboaca
2022-10-28 14:45   ` Krzysztof Kozlowski
2022-10-29 15:22   ` Jonathan Cameron
2022-10-28 13:44 ` [PATCH 2/2] drivers:iio:accel: Add support for ADXL359 device Ramona Bolboaca
2022-10-29 15:38   ` Jonathan Cameron

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).