[PATCH V3 0/5] iio: accel: sca3300: add compitible for scl3300

[PATCH V3 0/5] iio: accel: sca3300: add compitible for scl3300

[LI Qingwu]

The current driver support sca3300 only.
Modifed for support SCL3300 as well.
Verified with SCL3300 on IMX8MM.

SCL3300 is a three-axis accelerometer sensor with angle output.
The change adds the support of scl3300 and inclination data output.

Change in V3:
Add the accelerometer scale, frequency, and inclination scale mode map.
Add modes available for each device.
Drop "chip_type" and add all the chip info in the chip_info structure.
Fixed other findings in V2.


LI Qingwu (5):
  dt-bindings: iio: accel: sca3300: Document murata,scl3300
  iio: accel: sca3300: add define for temp channel for reuse.
  iio: accel: sca3300: modified to support multi chips
  iio: accel: sca3300: Add support for SCL3300
  iio: accel: sca3300: Add inclination channels

 .../bindings/iio/accel/murata,sca3300.yaml    |   1 +
 drivers/iio/accel/sca3300.c                   | 301 +++++++++++++++---
 2 files changed, 251 insertions(+), 51 deletions(-)

-- 
2.25.1

[PATCH V3 1/5] dt-bindings: iio: accel: sca3300: Document murata,scl3300

[LI Qingwu]

Add DT bindings for Murata scl3300 inclinometer.

Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
---
 Documentation/devicetree/bindings/iio/accel/murata,sca3300.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/iio/accel/murata,sca3300.yaml b/Documentation/devicetree/bindings/iio/accel/murata,sca3300.yaml
index 55fd3548e3b6..f6e2a16a710b 100644
--- a/Documentation/devicetree/bindings/iio/accel/murata,sca3300.yaml
+++ b/Documentation/devicetree/bindings/iio/accel/murata,sca3300.yaml
@@ -17,6 +17,7 @@ properties:
   compatible:
     enum:
       - murata,sca3300
+      - murata,scl3300
 
   reg:
     maxItems: 1
-- 
2.25.1

[PATCH V3 2/5] iio: accel: sca3300: add define for temp channel for reuse.

[LI Qingwu]

Add define of SCA3300_TEMP_CHANNEL for reuse.

Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
---
 drivers/iio/accel/sca3300.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/accel/sca3300.c b/drivers/iio/accel/sca3300.c
index f7ef8ecfd34a..ff16d2cc8c70 100644
--- a/drivers/iio/accel/sca3300.c
+++ b/drivers/iio/accel/sca3300.c
@@ -72,22 +72,24 @@ enum sca3300_scan_indexes {
 	},								\
 }
 
+#define SCA3300_TEMP_CHANNEL(index, reg) {				\
+		.type = IIO_TEMP,					\
+		.address = reg,						\
+		.scan_index = index,					\
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+		.scan_type = {						\
+			.sign = 's',					\
+			.realbits = 16,					\
+			.storagebits = 16,				\
+			.endianness = IIO_CPU,				\
+		},							\
+}
+
 static const struct iio_chan_spec sca3300_channels[] = {
 	SCA3300_ACCEL_CHANNEL(SCA3300_ACC_X, 0x1, X),
 	SCA3300_ACCEL_CHANNEL(SCA3300_ACC_Y, 0x2, Y),
 	SCA3300_ACCEL_CHANNEL(SCA3300_ACC_Z, 0x3, Z),
-	{
-		.type = IIO_TEMP,
-		.address = 0x5,
-		.scan_index = SCA3300_TEMP,
-		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
-		.scan_type = {
-			.sign = 's',
-			.realbits = 16,
-			.storagebits = 16,
-			.endianness = IIO_CPU,
-		},
-	},
+	SCA3300_TEMP_CHANNEL(SCA3300_TEMP, 0x05),
 	IIO_CHAN_SOFT_TIMESTAMP(4),
 };
 
-- 
2.25.1

[PATCH V3 3/5] iio: accel: sca3300: modified to support multi chips

[LI Qingwu]

The drive support sca3300 only.
There are some other similar chips, for instance, SCL3300.
Prepare the way for multiple chips and additional channels.
Modify the driver to read the device id.
Add the tables for the corresponding id to support multiple chips.
Add prepares for the addition of extra channels.
Add prepares for handling the operation modes for multiple chips.

Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
---
 drivers/iio/accel/sca3300.c | 180 ++++++++++++++++++++++++++++--------
 1 file changed, 141 insertions(+), 39 deletions(-)

diff --git a/drivers/iio/accel/sca3300.c b/drivers/iio/accel/sca3300.c
index ff16d2cc8c70..1e0e6a2f7a63 100644
--- a/drivers/iio/accel/sca3300.c
+++ b/drivers/iio/accel/sca3300.c
@@ -37,7 +37,6 @@
 
 /* Device ID */
 #define SCA3300_REG_WHOAMI	0x10
-#define SCA3300_WHOAMI_ID	0x51
 
 /* Device return status and mask */
 #define SCA3300_VALUE_RS_ERROR	0x3
@@ -93,15 +92,35 @@ static const struct iio_chan_spec sca3300_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(4),
 };
 
-static const int sca3300_lp_freq[] = {70, 70, 70, 10};
-static const int sca3300_accel_scale[][2] = {{0, 370}, {0, 741}, {0, 185}, {0, 185}};
+static const int sca3300_freq_tbl[] = {70, 10};
+static const int sca3300_freq_modes_map[] = {0, 0, 0, 1};
 
+static const int sca3300_accel_scale_tbl[][2] = {{0, 370}, {0, 741}, {0, 185}};
+static const int sca3300_accel_scale_modes_map[] = {0, 1, 2, 2};
+
+static const int sca3300_avail_modes_map[] = {0, 1, 2, 3};
 static const unsigned long sca3300_scan_masks[] = {
 	BIT(SCA3300_ACC_X) | BIT(SCA3300_ACC_Y) | BIT(SCA3300_ACC_Z) |
 	BIT(SCA3300_TEMP),
 	0
 };
 
+struct sca3300_chip_info {
+	const struct iio_chan_spec *channels;
+	const int (*accel_scale_table)[2];
+	const int *accel_scale_modes_map;
+	const unsigned long *scan_masks;
+	const int *avail_modes_table;
+	const int *freq_modes_map;
+	const int *freq_table;
+	const u8 num_accel_scales;
+	const u8 num_avail_modes;
+	const u8 num_channels;
+	const u8 num_freqs;
+	const u8 chip_id;
+	const char *name;
+};
+
 /**
  * struct sca3300_data - device data
  * @spi: SPI device structure
@@ -117,10 +136,28 @@ struct sca3300_data {
 		s16 channels[4];
 		s64 ts __aligned(sizeof(s64));
 	} scan;
+	const struct sca3300_chip_info *chip;
 	u8 txbuf[4] ____cacheline_aligned;
 	u8 rxbuf[4];
 };
 
+static const struct sca3300_chip_info sca3300_chip_tbl[] = {
+	{	.num_accel_scales = ARRAY_SIZE(sca3300_accel_scale_tbl)*2,
+		.accel_scale_modes_map = sca3300_accel_scale_modes_map,
+		.accel_scale_table = sca3300_accel_scale_tbl,
+		.num_channels = ARRAY_SIZE(sca3300_channels),
+		.freq_modes_map = sca3300_freq_modes_map,
+		.avail_modes_table = sca3300_avail_modes_map,
+		.freq_table = sca3300_freq_tbl,
+		.scan_masks = sca3300_scan_masks,
+		.channels = sca3300_channels,
+		.num_avail_modes = 4,
+		.name = "sca3300",
+		.chip_id = 0x51,
+		.num_freqs = 2,
+	},
+};
+
 DECLARE_CRC8_TABLE(sca3300_crc_table);
 
 static int sca3300_transfer(struct sca3300_data *sca_data, int *val)
@@ -227,36 +264,80 @@ static int sca3300_write_reg(struct sca3300_data *sca_data, u8 reg, int val)
 	return sca3300_error_handler(sca_data);
 }
 
+static int sca3300_set_op_mode(struct sca3300_data *sca_data, int index)
+{
+	int mode;
+
+	if ((index < 0) || (index >= sca_data->chip->num_avail_modes))
+		return -EINVAL;
+
+	mode = sca_data->chip->avail_modes_table[index];
+
+	return sca3300_write_reg(sca_data, SCA3300_REG_MODE, mode);
+}
+
+static int sca3300_get_op_mode(struct sca3300_data *sca_data, int *index)
+{
+	int reg_val;
+	int ret;
+	int i;
+
+	ret = sca3300_read_reg(sca_data, SCA3300_REG_MODE, &reg_val);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < sca_data->chip->num_avail_modes; i++) {
+		if (sca_data->chip->avail_modes_table[i] == reg_val) {
+			*index = i;
+			break;
+		}
+	}
+
+	return ret;
+}
+
 static int sca3300_write_raw(struct iio_dev *indio_dev,
 			     struct iio_chan_spec const *chan,
 			     int val, int val2, long mask)
 {
 	struct sca3300_data *data = iio_priv(indio_dev);
+	int mode = -1;
 	int reg_val;
+	int index;
 	int ret;
 	int i;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_SCALE:
-		if (val)
+		if (chan->type != IIO_ACCEL)
 			return -EINVAL;
-
-		for (i = 0; i < ARRAY_SIZE(sca3300_accel_scale); i++) {
-			if (val2 == sca3300_accel_scale[i][1])
-				return sca3300_write_reg(data, SCA3300_REG_MODE, i);
+		for (i = 0; i < data->chip->num_avail_modes; i++) {
+			index = data->chip->accel_scale_modes_map[i];
+			if ((val == data->chip->accel_scale_table[index][0]) &&
+			(val2 == data->chip->accel_scale_table[index][1])) {
+				mode = i;
+				break;
+			}
 		}
-		return -EINVAL;
-
+		return sca3300_set_op_mode(data, mode);
 	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
-		ret = sca3300_read_reg(data, SCA3300_REG_MODE, &reg_val);
+		ret = sca3300_get_op_mode(data, &reg_val);
 		if (ret)
 			return ret;
-		/* freq. change is possible only for mode 3 and 4 */
-		if (reg_val == 2 && val == sca3300_lp_freq[3])
-			return sca3300_write_reg(data, SCA3300_REG_MODE, 3);
-		if (reg_val == 3 && val == sca3300_lp_freq[2])
-			return sca3300_write_reg(data, SCA3300_REG_MODE, 2);
-		return -EINVAL;
+		index = data->chip->accel_scale_modes_map[reg_val];
+		for (i = 0; i < data->chip->num_avail_modes; i++) {
+			int index_new = data->chip->accel_scale_modes_map[i];
+			int index_freq = data->chip->freq_modes_map[i];
+
+			if (val == data->chip->freq_table[index_freq]) {
+				if (data->chip->accel_scale_table[index] ==
+				data->chip->accel_scale_table[index_new]) {
+					mode = i;
+					break;
+				}
+			}
+		}
+		return sca3300_set_op_mode(data, mode);
 	default:
 		return -EINVAL;
 	}
@@ -267,8 +348,9 @@ static int sca3300_read_raw(struct iio_dev *indio_dev,
 			    int *val, int *val2, long mask)
 {
 	struct sca3300_data *data = iio_priv(indio_dev);
-	int ret;
 	int reg_val;
+	int index;
+	int ret;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
@@ -277,17 +359,25 @@ static int sca3300_read_raw(struct iio_dev *indio_dev,
 			return ret;
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
-		ret = sca3300_read_reg(data, SCA3300_REG_MODE, &reg_val);
+		ret = sca3300_get_op_mode(data, &reg_val);
 		if (ret)
 			return ret;
-		*val = 0;
-		*val2 = sca3300_accel_scale[reg_val][1];
-		return IIO_VAL_INT_PLUS_MICRO;
+		switch (chan->type) {
+		case IIO_ACCEL:
+			index = data->chip->accel_scale_modes_map[reg_val];
+			*val = data->chip->accel_scale_table[index][0];
+			*val2 = data->chip->accel_scale_table[index][1];
+			return IIO_VAL_INT_PLUS_MICRO;
+		default:
+			return -EINVAL;
+		}
 	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
-		ret = sca3300_read_reg(data, SCA3300_REG_MODE, &reg_val);
+
+		ret = sca3300_get_op_mode(data, &reg_val);
 		if (ret)
 			return ret;
-		*val = sca3300_lp_freq[reg_val];
+		index = data->chip->freq_modes_map[reg_val];
+		*val = data->chip->freq_table[index];
 		return IIO_VAL_INT;
 	default:
 		return -EINVAL;
@@ -331,6 +421,7 @@ static int sca3300_init(struct sca3300_data *sca_data,
 {
 	int value = 0;
 	int ret;
+	int i;
 
 	ret = sca3300_write_reg(sca_data, SCA3300_REG_MODE,
 				SCA3300_MODE_SW_RESET);
@@ -347,12 +438,22 @@ static int sca3300_init(struct sca3300_data *sca_data,
 	if (ret)
 		return ret;
 
-	if (value != SCA3300_WHOAMI_ID) {
-		dev_err(&sca_data->spi->dev,
-			"device id not expected value, %d != %u\n",
-			value, SCA3300_WHOAMI_ID);
+	for (i = 0; i < ARRAY_SIZE(sca3300_chip_tbl); i++) {
+		if (sca3300_chip_tbl[i].chip_id == value)
+			break;
+	}
+	if (i == ARRAY_SIZE(sca3300_chip_tbl)) {
+		dev_err(&sca_data->spi->dev, "Invalid chip %x\n", value);
 		return -ENODEV;
 	}
+
+	indio_dev->available_scan_masks = sca3300_chip_tbl[i].scan_masks;
+	indio_dev->num_channels = sca3300_chip_tbl[i].num_channels;
+	indio_dev->channels = sca3300_chip_tbl[i].channels;
+	sca_data->chip = &sca3300_chip_tbl[i];
+	indio_dev->name = sca3300_chip_tbl[i].name;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
 	return 0;
 }
 
@@ -384,15 +485,21 @@ static int sca3300_read_avail(struct iio_dev *indio_dev,
 			      const int **vals, int *type, int *length,
 			      long mask)
 {
+	struct sca3300_data *data = iio_priv(indio_dev);
 	switch (mask) {
 	case IIO_CHAN_INFO_SCALE:
-		*vals = (const int *)sca3300_accel_scale;
-		*length = ARRAY_SIZE(sca3300_accel_scale) * 2 - 2;
-		*type = IIO_VAL_INT_PLUS_MICRO;
-		return IIO_AVAIL_LIST;
+		switch (chan->type) {
+		case IIO_ACCEL:
+			*vals = (const int *)data->chip->accel_scale_table;
+			*length = data->chip->num_accel_scales;
+			*type = IIO_VAL_INT_PLUS_MICRO;
+			return IIO_AVAIL_LIST;
+		default:
+			return -EINVAL;
+		}
 	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
-		*vals = &sca3300_lp_freq[2];
-		*length = 2;
+		*vals = (const int *)data->chip->freq_table;
+		*length = data->chip->num_freqs;
 		*type = IIO_VAL_INT;
 		return IIO_AVAIL_LIST;
 	default:
@@ -424,11 +531,6 @@ static int sca3300_probe(struct spi_device *spi)
 	crc8_populate_msb(sca3300_crc_table, SCA3300_CRC8_POLYNOMIAL);
 
 	indio_dev->info = &sca3300_info;
-	indio_dev->name = SCA3300_ALIAS;
-	indio_dev->modes = INDIO_DIRECT_MODE;
-	indio_dev->channels = sca3300_channels;
-	indio_dev->num_channels = ARRAY_SIZE(sca3300_channels);
-	indio_dev->available_scan_masks = sca3300_scan_masks;
 
 	ret = sca3300_init(sca_data, indio_dev);
 	if (ret) {
-- 
2.25.1

Re: [PATCH V3 3/5] iio: accel: sca3300: modified to support multi chips

[Andy Shevchenko]

On Wed, May 4, 2022 at 3:36 PM LI Qingwu
<Qing-wu.Li@leica-geosystems.com.cn> wrote:
>
> The drive support sca3300 only.

driver supports

> There are some other similar chips, for instance, SCL3300.
> Prepare the way for multiple chips and additional channels.
> Modify the driver to read the device id.
> Add the tables for the corresponding id to support multiple chips.
> Add prepares for the addition of extra channels.
> Add prepares for handling the operation modes for multiple chips.

It seems you need to work more on the commit messages in all patches.

...

> +struct sca3300_chip_info {
> +       const struct iio_chan_spec *channels;
> +       const int (*accel_scale_table)[2];
> +       const int *accel_scale_modes_map;
> +       const unsigned long *scan_masks;
> +       const int *avail_modes_table;
> +       const int *freq_modes_map;
> +       const int *freq_table;
> +       const u8 num_accel_scales;
> +       const u8 num_avail_modes;
> +       const u8 num_channels;
> +       const u8 num_freqs;
> +       const u8 chip_id;

Why do you have const qualifier on all members?  The last one is
understandable, but the rest, esp. pointers should be justified.

> +       const char *name;
> +};

...

> +static int sca3300_set_op_mode(struct sca3300_data *sca_data, int index)
> +{
> +       int mode;
> +
> +       if ((index < 0) || (index >= sca_data->chip->num_avail_modes))

Too many parentheses.

> +               return -EINVAL;
> +
> +       mode = sca_data->chip->avail_modes_table[index];
> +
> +       return sca3300_write_reg(sca_data, SCA3300_REG_MODE, mode);
> +}

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH V3 3/5] iio: accel: sca3300: modified to support multi chips

[LI Qingwu]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Wednesday, May 4, 2022 10:20 PM
> To: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
> Cc: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen
> <lars@metafoo.de>; Rob Herring <robh+dt@kernel.org>; Tomas Melin
> <tomas.melin@vaisala.com>; devicetree <devicetree@vger.kernel.org>; Linux
> Kernel Mailing List <linux-kernel@vger.kernel.org>; linux-iio
> <linux-iio@vger.kernel.org>; Rob Herring <robh@kernel.org>
> Subject: Re: [PATCH V3 3/5] iio: accel: sca3300: modified to support multi chips
> 
> This email is not from Hexagon’s Office 365 instance. Please be careful while
> clicking links, opening attachments, or replying to this email.
> 
> 
> On Wed, May 4, 2022 at 3:36 PM LI Qingwu
> <Qing-wu.Li@leica-geosystems.com.cn> wrote:
> >
> > The drive support sca3300 only.
> 
> driver supports
> 
> > There are some other similar chips, for instance, SCL3300.
> > Prepare the way for multiple chips and additional channels.
> > Modify the driver to read the device id.
> > Add the tables for the corresponding id to support multiple chips.
> > Add prepares for the addition of extra channels.
> > Add prepares for handling the operation modes for multiple chips.
> 
> It seems you need to work more on the commit messages in all patches.
Thanks, Andy. I rely on grammar checking but it didn't point out. 
Let me fix it in the next version.
> 
> ...
> 
> > +struct sca3300_chip_info {
> > +       const struct iio_chan_spec *channels;
> > +       const int (*accel_scale_table)[2];
> > +       const int *accel_scale_modes_map;
> > +       const unsigned long *scan_masks;
> > +       const int *avail_modes_table;
> > +       const int *freq_modes_map;
> > +       const int *freq_table;
> > +       const u8 num_accel_scales;
> > +       const u8 num_avail_modes;
> > +       const u8 num_channels;
> > +       const u8 num_freqs;
> > +       const u8 chip_id;
> 
> Why do you have const qualifier on all members?  The last one is
> understandable, but the rest, esp. pointers should be justified.
Because I thought it was static and has fix value for each chip, unacceptable for you?
> 
> > +       const char *name;
> > +};
> 
> ...
> 
> > +static int sca3300_set_op_mode(struct sca3300_data *sca_data, int
> > +index) {
> > +       int mode;
> > +
> > +       if ((index < 0) || (index >= sca_data->chip->num_avail_modes))
> 
> Too many parentheses.
> 
> > +               return -EINVAL;
> > +
> > +       mode = sca_data->chip->avail_modes_table[index];
> > +
> > +       return sca3300_write_reg(sca_data, SCA3300_REG_MODE,
> mode); }
> 
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH V3 3/5] iio: accel: sca3300: modified to support multi chips

[Andy Shevchenko]

On Wed, May 4, 2022 at 4:35 PM LI Qingwu
<qing-wu.li@leica-geosystems.com.cn> wrote:
> > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Sent: Wednesday, May 4, 2022 10:20 PM
> > On Wed, May 4, 2022 at 3:36 PM LI Qingwu
> > <Qing-wu.Li@leica-geosystems.com.cn> wrote:

...

> > > +struct sca3300_chip_info {
> > > +       const struct iio_chan_spec *channels;
> > > +       const int (*accel_scale_table)[2];
> > > +       const int *accel_scale_modes_map;
> > > +       const unsigned long *scan_masks;
> > > +       const int *avail_modes_table;
> > > +       const int *freq_modes_map;
> > > +       const int *freq_table;
> > > +       const u8 num_accel_scales;
> > > +       const u8 num_avail_modes;
> > > +       const u8 num_channels;
> > > +       const u8 num_freqs;
> > > +       const u8 chip_id;
> >
> > Why do you have const qualifier on all members?  The last one is
> > understandable, but the rest, esp. pointers should be justified.
> Because I thought it was static and has fix value for each chip, unacceptable for you?

But why const qualifier? What is the point of it for example for u8
members if the entire object is qualified as const below in the same
patch?

On top of that, please explain what in your opinion the "const ...
*foo" gives us, and what we will lose if we remove the "const" part
out of them.

> > > +       const char *name;
> > > +};

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH V3 3/5] iio: accel: sca3300: modified to support multi chips

[LI Qingwu]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Wednesday, May 4, 2022 10:39 PM
> To: LI Qingwu <qing-wu.li@leica-geosystems.com.cn>
> Cc: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen
> <lars@metafoo.de>; Rob Herring <robh+dt@kernel.org>; Tomas Melin
> <tomas.melin@vaisala.com>; devicetree <devicetree@vger.kernel.org>; Linux
> Kernel Mailing List <linux-kernel@vger.kernel.org>; linux-iio
> <linux-iio@vger.kernel.org>; Rob Herring <robh@kernel.org>
> Subject: Re: [PATCH V3 3/5] iio: accel: sca3300: modified to support multi chips
> 
> This email is not from Hexagon’s Office 365 instance. Please be careful while
> clicking links, opening attachments, or replying to this email.
> 
> 
> On Wed, May 4, 2022 at 4:35 PM LI Qingwu
> <qing-wu.li@leica-geosystems.com.cn> wrote:
> > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > Sent: Wednesday, May 4, 2022 10:20 PM On Wed, May 4, 2022 at 3:36 PM
> > > LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn> wrote:
> 
> ...
> 
> > > > +struct sca3300_chip_info {
> > > > +       const struct iio_chan_spec *channels;
> > > > +       const int (*accel_scale_table)[2];
> > > > +       const int *accel_scale_modes_map;
> > > > +       const unsigned long *scan_masks;
> > > > +       const int *avail_modes_table;
> > > > +       const int *freq_modes_map;
> > > > +       const int *freq_table;
> > > > +       const u8 num_accel_scales;
> > > > +       const u8 num_avail_modes;
> > > > +       const u8 num_channels;
> > > > +       const u8 num_freqs;
> > > > +       const u8 chip_id;
> > >
> > > Why do you have const qualifier on all members?  The last one is
> > > understandable, but the rest, esp. pointers should be justified.
> > Because I thought it was static and has fix value for each chip, unacceptable
> for you?
> 
> But why const qualifier? What is the point of it for example for u8 members if
> the entire object is qualified as const below in the same patch?
> 
> On top of that, please explain what in your opinion the "const ...
> *foo" gives us, and what we will lose if we remove the "const" part out of them.

Ah, you are right, those const are unnecessary for nonpointer members.
for the pointers, the contexts that the pointer points to are still writable.
what about if I remove all the const from nonpointer and keep it for the pointers?
Like：
const struct iio_chan_spec *channels;
const int (*accel_scale_table)[2];
const int (*incli_scale_table)[2];
const int *accel_scale_modes_map;
const int *incli_scale_modes_map;
const unsigned long *scan_masks;
const int *avail_modes_table;
const int *freq_modes_map;
const int *freq_table;
const char *name;
u8 num_accel_scales;
u8 num_incli_scales;
u8 num_avail_modes;
u8 num_channels;
u8 num_freqs;
u8 chip_id;
bool angle;



> 
> > > > +       const char *name;
> > > > +};
> 
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH V3 3/5] iio: accel: sca3300: modified to support multi chips

[Andy Shevchenko]

On Thu, May 5, 2022 at 4:12 PM LI Qingwu
<qing-wu.li@leica-geosystems.com.cn> wrote:
> > -----Original Message-----
> > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Sent: Wednesday, May 4, 2022 10:39 PM
> > On Wed, May 4, 2022 at 4:35 PM LI Qingwu
> > <qing-wu.li@leica-geosystems.com.cn> wrote:
> > > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > > Sent: Wednesday, May 4, 2022 10:20 PM On Wed, May 4, 2022 at 3:36 PM
> > > > LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn> wrote:

...

> > > > > +struct sca3300_chip_info {
> > > > > +       const struct iio_chan_spec *channels;
> > > > > +       const int (*accel_scale_table)[2];
> > > > > +       const int *accel_scale_modes_map;
> > > > > +       const unsigned long *scan_masks;
> > > > > +       const int *avail_modes_table;
> > > > > +       const int *freq_modes_map;
> > > > > +       const int *freq_table;
> > > > > +       const u8 num_accel_scales;
> > > > > +       const u8 num_avail_modes;
> > > > > +       const u8 num_channels;
> > > > > +       const u8 num_freqs;
> > > > > +       const u8 chip_id;
> > > >
> > > > Why do you have const qualifier on all members?  The last one is
> > > > understandable, but the rest, esp. pointers should be justified.
> > > Because I thought it was static and has fix value for each chip, unacceptable
> > for you?
> >
> > But why const qualifier? What is the point of it for example for u8 members if
> > the entire object is qualified as const below in the same patch?
> >
> > On top of that, please explain what in your opinion the "const ...
> > *foo" gives us, and what we will lose if we remove the "const" part out of them.
>
> Ah, you are right, those const are unnecessary for nonpointer members.
> for the pointers, the contexts that the pointer points to are still writable.
> what about if I remove all the const from nonpointer and keep it for the pointers?
> Like：
> const struct iio_chan_spec *channels;
> const int (*accel_scale_table)[2];
> const int (*incli_scale_table)[2];
> const int *accel_scale_modes_map;
> const int *incli_scale_modes_map;
> const unsigned long *scan_masks;
> const int *avail_modes_table;
> const int *freq_modes_map;
> const int *freq_table;
> const char *name;
> u8 num_accel_scales;
> u8 num_incli_scales;
> u8 num_avail_modes;
> u8 num_channels;
> u8 num_freqs;
> u8 chip_id;
> bool angle;

It's better, but you still need to justify the rest with explanation
in the commit message.
And I leave this to maintainers to say if the const:s are needed or not.

> > > > > +       const char *name;
> > > > > +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V3 3/5] iio: accel: sca3300: modified to support multi chips

[Jonathan Cameron]

On Thu, 5 May 2022 20:01:22 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Thu, May 5, 2022 at 4:12 PM LI Qingwu
> <qing-wu.li@leica-geosystems.com.cn> wrote:
> > > -----Original Message-----
> > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > Sent: Wednesday, May 4, 2022 10:39 PM
> > > On Wed, May 4, 2022 at 4:35 PM LI Qingwu
> > > <qing-wu.li@leica-geosystems.com.cn> wrote:  
> > > > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > > > Sent: Wednesday, May 4, 2022 10:20 PM On Wed, May 4, 2022 at 3:36 PM
> > > > > LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn> wrote:  
> 
> ...
> 
> > > > > > +struct sca3300_chip_info {
> > > > > > +       const struct iio_chan_spec *channels;
> > > > > > +       const int (*accel_scale_table)[2];
> > > > > > +       const int *accel_scale_modes_map;
> > > > > > +       const unsigned long *scan_masks;
> > > > > > +       const int *avail_modes_table;
> > > > > > +       const int *freq_modes_map;
> > > > > > +       const int *freq_table;
> > > > > > +       const u8 num_accel_scales;
> > > > > > +       const u8 num_avail_modes;
> > > > > > +       const u8 num_channels;
> > > > > > +       const u8 num_freqs;
> > > > > > +       const u8 chip_id;  
> > > > >
> > > > > Why do you have const qualifier on all members?  The last one is
> > > > > understandable, but the rest, esp. pointers should be justified.  
> > > > Because I thought it was static and has fix value for each chip, unacceptable  
> > > for you?
> > >
> > > But why const qualifier? What is the point of it for example for u8 members if
> > > the entire object is qualified as const below in the same patch?
> > >
> > > On top of that, please explain what in your opinion the "const ...
> > > *foo" gives us, and what we will lose if we remove the "const" part out of them.  
> >
> > Ah, you are right, those const are unnecessary for nonpointer members.
> > for the pointers, the contexts that the pointer points to are still writable.
> > what about if I remove all the const from nonpointer and keep it for the pointers?
> > Like：
> > const struct iio_chan_spec *channels;
> > const int (*accel_scale_table)[2];
> > const int (*incli_scale_table)[2];
> > const int *accel_scale_modes_map;
> > const int *incli_scale_modes_map;
> > const unsigned long *scan_masks;
> > const int *avail_modes_table;
> > const int *freq_modes_map;
> > const int *freq_table;
> > const char *name;
> > u8 num_accel_scales;
> > u8 num_incli_scales;
> > u8 num_avail_modes;
> > u8 num_channels;
> > u8 num_freqs;
> > u8 chip_id;
> > bool angle;  
> 
> It's better, but you still need to justify the rest with explanation
> in the commit message.
> And I leave this to maintainers to say if the const:s are needed or not.
Where they are being set to point to
static const int array[] 
then to my mind it makes sense to have them as const as makes it nice and obvious
what is going on.

Jonathan

> 
> > > > > > +       const char *name;
> > > > > > +};  
> 

Re: [PATCH V3 3/5] iio: accel: sca3300: modified to support multi chips

[Melin Tomas]

Hi,

Some comments below.

On 04/05/2022 16:36, LI Qingwu wrote:
> The drive support sca3300 only.
> There are some other similar chips, for instance, SCL3300.
> Prepare the way for multiple chips and additional channels.
> Modify the driver to read the device id.
> Add the tables for the corresponding id to support multiple chips.
> Add prepares for the addition of extra channels.
> Add prepares for handling the operation modes for multiple chips.
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Reviewed-by: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
> ---
>  drivers/iio/accel/sca3300.c | 180 ++++++++++++++++++++++++++++--------
>  1 file changed, 141 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/iio/accel/sca3300.c b/drivers/iio/accel/sca3300.c
> index ff16d2cc8c70..1e0e6a2f7a63 100644
> --- a/drivers/iio/accel/sca3300.c
> +++ b/drivers/iio/accel/sca3300.c
> @@ -37,7 +37,6 @@
>  
>  /* Device ID */
>  #define SCA3300_REG_WHOAMI	0x10
> -#define SCA3300_WHOAMI_ID	0x51
I suggest leaving this here, define another value for scl3000 if needed.
>  
>  /* Device return status and mask */
>  #define SCA3300_VALUE_RS_ERROR	0x3
> @@ -93,15 +92,35 @@ static const struct iio_chan_spec sca3300_channels[] = {
>  	IIO_CHAN_SOFT_TIMESTAMP(4),
>  };
>  
> -static const int sca3300_lp_freq[] = {70, 70, 70, 10};
> -static const int sca3300_accel_scale[][2] = {{0, 370}, {0, 741}, {0, 185}, {0, 185}};
> +static const int sca3300_freq_tbl[] = {70, 10};
> +static const int sca3300_freq_modes_map[] = {0, 0, 0, 1};
>  
> +static const int sca3300_accel_scale_tbl[][2] = {{0, 370}, {0, 741}, {0, 185}};
> +static const int sca3300_accel_scale_modes_map[] = {0, 1, 2, 2};
Suggest keeping name of this intact, and keep name of the map variable
similar.
static const int sca3300_accel_scale[][2] = {{0, 370}, {0, 741}, {0, 185}};
static const int sca3300_accel_scale_map[] = {0, 1, 2, 2};

That keeps it easy to remember that these are related to each other.

> +
> +static const int sca3300_avail_modes_map[] = {0, 1, 2, 3};
>  static const unsigned long sca3300_scan_masks[] = {
>  	BIT(SCA3300_ACC_X) | BIT(SCA3300_ACC_Y) | BIT(SCA3300_ACC_Z) |
>  	BIT(SCA3300_TEMP),
>  	0
>  };
>  
> +struct sca3300_chip_info {
> +	const struct iio_chan_spec *channels;
> +	const int (*accel_scale_table)[2];> +	const int *accel_scale_modes_map;
> +	const unsigned long *scan_masks;
> +	const int *avail_modes_table;
> +	const int *freq_modes_map;
> +	const int *freq_table;> +	const u8 num_accel_scales;
> +	const u8 num_avail_modes;
> +	const u8 num_channels;
> +	const u8 num_freqs;
> +	const u8 chip_id;
> +	const char *name;
Suggest keeping related variables logically sorted in declaration
together instead of sorting according to length.
> +};
> +
>  /**
>   * struct sca3300_data - device data
>   * @spi: SPI device structure
> @@ -117,10 +136,28 @@ struct sca3300_data {
>  		s16 channels[4];
>  		s64 ts __aligned(sizeof(s64));
>  	} scan;
> +	const struct sca3300_chip_info *chip;
>  	u8 txbuf[4] ____cacheline_aligned;
>  	u8 rxbuf[4];
>  };
>  
> +static const struct sca3300_chip_info sca3300_chip_tbl[] = {
> +	{	.num_accel_scales = ARRAY_SIZE(sca3300_accel_scale_tbl)*2,> +		.accel_scale_modes_map = sca3300_accel_scale_modes_map,
> +		.accel_scale_table = sca3300_accel_scale_tbl,
> +		.num_channels = ARRAY_SIZE(sca3300_channels),
> +		.freq_modes_map = sca3300_freq_modes_map,
> +		.avail_modes_table = sca3300_avail_modes_map,
> +		.freq_table = sca3300_freq_tbl,
> +		.scan_masks = sca3300_scan_masks,
> +		.channels = sca3300_channels,
> +		.num_avail_modes = 4,
> +		.name = "sca3300",
> +		.chip_id = 0x51,
> +		.num_freqs = 2,
> +	},
> +};
> +
>  DECLARE_CRC8_TABLE(sca3300_crc_table);
>  
>  static int sca3300_transfer(struct sca3300_data *sca_data, int *val)
> @@ -227,36 +264,80 @@ static int sca3300_write_reg(struct sca3300_data *sca_data, u8 reg, int val)
>  	return sca3300_error_handler(sca_data);
>  }
>  
> +static int sca3300_set_op_mode(struct sca3300_data *sca_data, int index)
> +{
> +	int mode;
> +
> +	if ((index < 0) || (index >= sca_data->chip->num_avail_modes))
> +		return -EINVAL;
> +
> +	mode = sca_data->chip->avail_modes_table[index];
> +
> +	return sca3300_write_reg(sca_data, SCA3300_REG_MODE, mode);
> +}
> +
> +static int sca3300_get_op_mode(struct sca3300_data *sca_data, int *index)
> +{
> +	int reg_val;
> +	int ret;
> +	int i;
> +
> +	ret = sca3300_read_reg(sca_data, SCA3300_REG_MODE, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < sca_data->chip->num_avail_modes; i++) {
> +		if (sca_data->chip->avail_modes_table[i] == reg_val) {
> +			*index = i;
> +			break;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
>  static int sca3300_write_raw(struct iio_dev *indio_dev,
>  			     struct iio_chan_spec const *chan,
>  			     int val, int val2, long mask)
>  {
>  	struct sca3300_data *data = iio_priv(indio_dev);
> +	int mode = -1;
Suggest not setting to invalid default value, instead avoid calling
function if values are not found.

>  	int reg_val;
> +	int index;
>  	int ret;
>  	int i;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SCALE:
> -		if (val)
> +		if (chan->type != IIO_ACCEL)
>  			return -EINVAL;
Was there some problem with if (val) here, or why changed?
> -
> -		for (i = 0; i < ARRAY_SIZE(sca3300_accel_scale); i++) {
> -			if (val2 == sca3300_accel_scale[i][1])
> -				return sca3300_write_reg(data, SCA3300_REG_MODE, i);
> +		for (i = 0; i < data->chip->num_avail_modes; i++) { 4
> +			index = data->chip->accel_scale_modes_map[i];
> +			if ((val == data->chip->accel_scale_table[index][0]) &&
> +			(val2 == data->chip->accel_scale_table[index][1])) {
> +				mode = i;
> +				break;
> +			}
>  		}
> -		return -EINVAL;
> -
> +		return sca3300_set_op_mode(data, mode);
>  	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> -		ret = sca3300_read_reg(data, SCA3300_REG_MODE, &reg_val);
> +		ret = sca3300_get_op_mode(data, &reg_val);
>  		if (ret)
>  			return ret;
> -		/* freq. change is possible only for mode 3 and 4 */
> -		if (reg_val == 2 && val == sca3300_lp_freq[3])
> -			return sca3300_write_reg(data, SCA3300_REG_MODE, 3);
> -		if (reg_val == 3 && val == sca3300_lp_freq[2])
> -			return sca3300_write_reg(data, SCA3300_REG_MODE, 2);
> -		return -EINVAL;
> +		index = data->chip->accel_scale_modes_map[reg_val];
> +		for (i = 0; i < data->chip->num_avail_modes; i++) {
> +			int index_new = data->chip->accel_scale_modes_map[i]; 0122
> +			int index_freq = data->chip->freq_modes_map[i];
Declare variables at top of function.
> +
> +			if (val == data->chip->freq_table[index_freq]) {
> +				if (data->chip->accel_scale_table[index] ==
> +				data->chip->accel_scale_table[index_new]) {
To be compatible with existing code, this should check that mode is 2 or
3. I don't see how this does that, please explain.

> +					mode = i;
> +					break;
> +				}
> +			}
> +		}
> +		return sca3300_set_op_mode(data, mode);
>  	default:
>  		return -EINVAL;
>  	}
> @@ -267,8 +348,9 @@ static int sca3300_read_raw(struct iio_dev *indio_dev,
>  			    int *val, int *val2, long mask)
>  {
>  	struct sca3300_data *data = iio_priv(indio_dev);
> -	int ret;
>  	int reg_val;
> +	int index;
> +	int ret;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> @@ -277,17 +359,25 @@ static int sca3300_read_raw(struct iio_dev *indio_dev,
>  			return ret;
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_SCALE:
> -		ret = sca3300_read_reg(data, SCA3300_REG_MODE, &reg_val);
> +		ret = sca3300_get_op_mode(data, &reg_val);
>  		if (ret)
>  			return ret;
> -		*val = 0;
> -		*val2 = sca3300_accel_scale[reg_val][1];
> -		return IIO_VAL_INT_PLUS_MICRO;
> +		switch (chan->type) {
> +		case IIO_ACCEL:
> +			index = data->chip->accel_scale_modes_map[reg_val];
> +			*val = data->chip->accel_scale_table[index][0];
> +			*val2 = data->chip->accel_scale_table[index][1];
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		default:
> +			return -EINVAL;
> +		}
>  	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> -		ret = sca3300_read_reg(data, SCA3300_REG_MODE, &reg_val);
> +
> +		ret = sca3300_get_op_mode(data, &reg_val);
>  		if (ret)
>  			return ret;
> -		*val = sca3300_lp_freq[reg_val];
> +		index = data->chip->freq_modes_map[reg_val];
> +		*val = data->chip->freq_table[index];
>  		return IIO_VAL_INT;
>  	default:
>  		return -EINVAL;
> @@ -331,6 +421,7 @@ static int sca3300_init(struct sca3300_data *sca_data,
>  {
>  	int value = 0;
>  	int ret;
> +	int i;
>  
>  	ret = sca3300_write_reg(sca_data, SCA3300_REG_MODE,
>  				SCA3300_MODE_SW_RESET);
> @@ -347,12 +438,22 @@ static int sca3300_init(struct sca3300_data *sca_data,
>  	if (ret)
>  		return ret;
>  
> -	if (value != SCA3300_WHOAMI_ID) {
> -		dev_err(&sca_data->spi->dev,
> -			"device id not expected value, %d != %u\n",
> -			value, SCA3300_WHOAMI_ID);
> +	for (i = 0; i < ARRAY_SIZE(sca3300_chip_tbl); i++) {
> +		if (sca3300_chip_tbl[i].chip_id == value)
> +			break;
> +	}> +	if (i == ARRAY_SIZE(sca3300_chip_tbl)) {
> +		dev_err(&sca_data->spi->dev, "Invalid chip %x\n", value);
Please start this with small letter, also perhaps resort to "unknown
chip id %x\n" or so.
>  		return -ENODEV;
>  	}
> +
> +	indio_dev->available_scan_masks = sca3300_chip_tbl[i].scan_masks;
> +	indio_dev->num_channels = sca3300_chip_tbl[i].num_channels;
> +	indio_dev->channels = sca3300_chip_tbl[i].channels;
> +	sca_data->chip = &sca3300_chip_tbl[i];
> +	indio_dev->name = sca3300_chip_tbl[i].name;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
I would prefer to keep the above indio_dev additions in the probe
function. That way we keep the init function clean(er).

Thanks,
Tomas

>  	return 0;
>  }
>  
> @@ -384,15 +485,21 @@ static int sca3300_read_avail(struct iio_dev *indio_dev,
>  			      const int **vals, int *type, int *length,
>  			      long mask)
>  {
> +	struct sca3300_data *data = iio_priv(indio_dev);
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SCALE:
> -		*vals = (const int *)sca3300_accel_scale;
> -		*length = ARRAY_SIZE(sca3300_accel_scale) * 2 - 2;
> -		*type = IIO_VAL_INT_PLUS_MICRO;
> -		return IIO_AVAIL_LIST;
> +		switch (chan->type) {
> +		case IIO_ACCEL:
> +			*vals = (const int *)data->chip->accel_scale_table;
> +			*length = data->chip->num_accel_scales;
> +			*type = IIO_VAL_INT_PLUS_MICRO;
> +			return IIO_AVAIL_LIST;
> +		default:
> +			return -EINVAL;
> +		}
>  	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> -		*vals = &sca3300_lp_freq[2];
> -		*length = 2;
> +		*vals = (const int *)data->chip->freq_table;
> +		*length = data->chip->num_freqs;
>  		*type = IIO_VAL_INT;
>  		return IIO_AVAIL_LIST;
>  	default:
> @@ -424,11 +531,6 @@ static int sca3300_probe(struct spi_device *spi)
>  	crc8_populate_msb(sca3300_crc_table, SCA3300_CRC8_POLYNOMIAL);
>  
>  	indio_dev->info = &sca3300_info;
> -	indio_dev->name = SCA3300_ALIAS;
> -	indio_dev->modes = INDIO_DIRECT_MODE;
> -	indio_dev->channels = sca3300_channels;
> -	indio_dev->num_channels = ARRAY_SIZE(sca3300_channels);
> -	indio_dev->available_scan_masks = sca3300_scan_masks;
>  
>  	ret = sca3300_init(sca_data, indio_dev);
>  	if (ret) {

RE: [PATCH V3 3/5] iio: accel: sca3300: modified to support multi chips

[LI Qingwu]

Hi Tomas,

Thanks a lot for your input, I will fix in the version 4.
Two explains below.

> -----Original Message-----
> From: Melin Tomas <tomas.melin@vaisala.com>
> Sent: Saturday, May 7, 2022 4:12 AM
> To: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>; jic23@kernel.org;
> lars@metafoo.de; robh+dt@kernel.org; andy.shevchenko@gmail.com;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: linux-iio@vger.kernel.org; Rob Herring <robh@kernel.org>
> Subject: Re: [PATCH V3 3/5] iio: accel: sca3300: modified to support multi chips
> 
> This email is not from Hexagon’s Office 365 instance. Please be careful while
> clicking links, opening attachments, or replying to this email.
> 
> 
> Hi,
> 
> Some comments below.
> 
> On 04/05/2022 16:36, LI Qingwu wrote:
> > The drive support sca3300 only.
> > There are some other similar chips, for instance, SCL3300.
> > Prepare the way for multiple chips and additional channels.
> > Modify the driver to read the device id.
> > Add the tables for the corresponding id to support multiple chips.
> > Add prepares for the addition of extra channels.
> > Add prepares for handling the operation modes for multiple chips.
> >
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Reviewed-by: Jonathan Cameron <jic23@kernel.org>
> > Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
> > ---
> >  drivers/iio/accel/sca3300.c | 180
> > ++++++++++++++++++++++++++++--------
> >  1 file changed, 141 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/iio/accel/sca3300.c b/drivers/iio/accel/sca3300.c
> > index ff16d2cc8c70..1e0e6a2f7a63 100644
> > --- a/drivers/iio/accel/sca3300.c
> > +++ b/drivers/iio/accel/sca3300.c
> > @@ -37,7 +37,6 @@
> >
> >  /* Device ID */
> >  #define SCA3300_REG_WHOAMI   0x10
> > -#define SCA3300_WHOAMI_ID    0x51
> I suggest leaving this here, define another value for scl3000 if needed.
> >
> >  /* Device return status and mask */
> >  #define SCA3300_VALUE_RS_ERROR       0x3
> > @@ -93,15 +92,35 @@ static const struct iio_chan_spec sca3300_channels[]
> = {
> >       IIO_CHAN_SOFT_TIMESTAMP(4),
> >  };
> >
> > -static const int sca3300_lp_freq[] = {70, 70, 70, 10}; -static const
> > int sca3300_accel_scale[][2] = {{0, 370}, {0, 741}, {0, 185}, {0,
> > 185}};
> > +static const int sca3300_freq_tbl[] = {70, 10}; static const int
> > +sca3300_freq_modes_map[] = {0, 0, 0, 1};
> >
> > +static const int sca3300_accel_scale_tbl[][2] = {{0, 370}, {0, 741},
> > +{0, 185}}; static const int sca3300_accel_scale_modes_map[] = {0, 1,
> > +2, 2};
> Suggest keeping name of this intact, and keep name of the map variable similar.
> static const int sca3300_accel_scale[][2] = {{0, 370}, {0, 741}, {0, 185}}; static
> const int sca3300_accel_scale_map[] = {0, 1, 2, 2};
> 
> That keeps it easy to remember that these are related to each other.
> 
> > +
> > +static const int sca3300_avail_modes_map[] = {0, 1, 2, 3};
> >  static const unsigned long sca3300_scan_masks[] = {
> >       BIT(SCA3300_ACC_X) | BIT(SCA3300_ACC_Y) | BIT(SCA3300_ACC_Z) |
> >       BIT(SCA3300_TEMP),
> >       0
> >  };
> >
> > +struct sca3300_chip_info {
> > +     const struct iio_chan_spec *channels;
> > +     const int (*accel_scale_table)[2];> +   const int
> *accel_scale_modes_map;
> > +     const unsigned long *scan_masks;
> > +     const int *avail_modes_table;
> > +     const int *freq_modes_map;
> > +     const int *freq_table;> +       const u8 num_accel_scales;
> > +     const u8 num_avail_modes;
> > +     const u8 num_channels;
> > +     const u8 num_freqs;
> > +     const u8 chip_id;
> > +     const char *name;
> Suggest keeping related variables logically sorted in declaration together
> instead of sorting according to length.
> > +};
> > +
> >  /**
> >   * struct sca3300_data - device data
> >   * @spi: SPI device structure
> > @@ -117,10 +136,28 @@ struct sca3300_data {
> >               s16 channels[4];
> >               s64 ts __aligned(sizeof(s64));
> >       } scan;
> > +     const struct sca3300_chip_info *chip;
> >       u8 txbuf[4] ____cacheline_aligned;
> >       u8 rxbuf[4];
> >  };
> >
> > +static const struct sca3300_chip_info sca3300_chip_tbl[] = {
> > +     {       .num_accel_scales =
> ARRAY_SIZE(sca3300_accel_scale_tbl)*2,>
> +           .accel_scale_modes_map = sca3300_accel_scale_modes_map,
> > +             .accel_scale_table = sca3300_accel_scale_tbl,
> > +             .num_channels = ARRAY_SIZE(sca3300_channels),
> > +             .freq_modes_map = sca3300_freq_modes_map,
> > +             .avail_modes_table = sca3300_avail_modes_map,
> > +             .freq_table = sca3300_freq_tbl,
> > +             .scan_masks = sca3300_scan_masks,
> > +             .channels = sca3300_channels,
> > +             .num_avail_modes = 4,
> > +             .name = "sca3300",
> > +             .chip_id = 0x51,
> > +             .num_freqs = 2,
> > +     },
> > +};
> > +
> >  DECLARE_CRC8_TABLE(sca3300_crc_table);
> >
> >  static int sca3300_transfer(struct sca3300_data *sca_data, int *val)
> > @@ -227,36 +264,80 @@ static int sca3300_write_reg(struct sca3300_data
> *sca_data, u8 reg, int val)
> >       return sca3300_error_handler(sca_data);  }
> >
> > +static int sca3300_set_op_mode(struct sca3300_data *sca_data, int
> > +index) {
> > +     int mode;
> > +
> > +     if ((index < 0) || (index >= sca_data->chip->num_avail_modes))
> > +             return -EINVAL;
> > +
> > +     mode = sca_data->chip->avail_modes_table[index];
> > +
> > +     return sca3300_write_reg(sca_data, SCA3300_REG_MODE, mode); }
> > +
> > +static int sca3300_get_op_mode(struct sca3300_data *sca_data, int
> > +*index) {
> > +     int reg_val;
> > +     int ret;
> > +     int i;
> > +
> > +     ret = sca3300_read_reg(sca_data, SCA3300_REG_MODE, &reg_val);
> > +     if (ret)
> > +             return ret;
> > +
> > +     for (i = 0; i < sca_data->chip->num_avail_modes; i++) {
> > +             if (sca_data->chip->avail_modes_table[i] == reg_val) {
> > +                     *index = i;
> > +                     break;
> > +             }
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> >  static int sca3300_write_raw(struct iio_dev *indio_dev,
> >                            struct iio_chan_spec const *chan,
> >                            int val, int val2, long mask)  {
> >       struct sca3300_data *data = iio_priv(indio_dev);
> > +     int mode = -1;
> Suggest not setting to invalid default value, instead avoid calling function if
> values are not found.
> 
> >       int reg_val;
> > +     int index;
> >       int ret;
> >       int i;
> >
> >       switch (mask) {
> >       case IIO_CHAN_INFO_SCALE:
> > -             if (val)
> > +             if (chan->type != IIO_ACCEL)
> >                       return -EINVAL;
> Was there some problem with if (val) here, or why changed?

The existing code check if val is 0 and val2 in the accel_scale list,
It's working because val are all 0 in the list.
Since accel_scale include both val and val2, 
then it's better to check both val and val2 with the table instead of hard code 0,
the following code did the same thing.
if ((val == data->chip->accel_scale_table[index][0])  &&
(val2 == data->chip->accel_scale_table[index][1]))

> > -
> > -             for (i = 0; i < ARRAY_SIZE(sca3300_accel_scale); i++) {
> > -                     if (val2 == sca3300_accel_scale[i][1])
> > -                             return sca3300_write_reg(data,
> SCA3300_REG_MODE, i);
> > +             for (i = 0; i < data->chip->num_avail_modes; i++) { 4
> > +                     index = data->chip->accel_scale_modes_map[i];
> > +                     if ((val == data->chip->accel_scale_table[index][0])
> &&
> > +                     (val2 == data->chip->accel_scale_table[index][1]))
> {
> > +                             mode = i;
> > +                             break;
> > +                     }
> >               }
> > -             return -EINVAL;
> > -
> > +             return sca3300_set_op_mode(data, mode);
> >       case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> > -             ret = sca3300_read_reg(data, SCA3300_REG_MODE,
> &reg_val);
> > +             ret = sca3300_get_op_mode(data, &reg_val);
> >               if (ret)
> >                       return ret;
> > -             /* freq. change is possible only for mode 3 and 4 */
> > -             if (reg_val == 2 && val == sca3300_lp_freq[3])
> > -                     return sca3300_write_reg(data,
> SCA3300_REG_MODE, 3);
> > -             if (reg_val == 3 && val == sca3300_lp_freq[2])
> > -                     return sca3300_write_reg(data,
> SCA3300_REG_MODE, 2);
> > -             return -EINVAL;
> > +             index = data->chip->accel_scale_modes_map[reg_val];
> > +             for (i = 0; i < data->chip->num_avail_modes; i++) {
> > +                     int index_new =
> data->chip->accel_scale_modes_map[i]; 0122
> > +                     int index_freq = data->chip->freq_modes_map[i];
> Declare variables at top of function.
> > +
> > +                     if (val == data->chip->freq_table[index_freq]) {
> > +                             if (data->chip->accel_scale_table[index]
> ==
> > +
> > + data->chip->accel_scale_table[index_new]) {
> To be compatible with existing code, this should check that mode is 2 or 3. I
> don't see how this does that, please explain.

The purpose of existing code check mode 2 or 3 is to make sure set frequency did not impact the accel scale.
Since only for sca3300, mode 2 and 3 have same accel scale but different frequency.
In the new code,
	if (data->chip->accel_scale_table[index] ==
		data->chip->accel_scale_table[index_new])
also make sure the frequency did not impact the accel scale.

> 
> > +                                     mode = i;
> > +                                     break;
> > +                             }
> > +                     }
> > +             }
> > +             return sca3300_set_op_mode(data, mode);
> >       default:
> >               return -EINVAL;
> >       }
> > @@ -267,8 +348,9 @@ static int sca3300_read_raw(struct iio_dev
> *indio_dev,
> >                           int *val, int *val2, long mask)  {
> >       struct sca3300_data *data = iio_priv(indio_dev);
> > -     int ret;
> >       int reg_val;
> > +     int index;
> > +     int ret;
> >
> >       switch (mask) {
> >       case IIO_CHAN_INFO_RAW:
> > @@ -277,17 +359,25 @@ static int sca3300_read_raw(struct iio_dev
> *indio_dev,
> >                       return ret;
> >               return IIO_VAL_INT;
> >       case IIO_CHAN_INFO_SCALE:
> > -             ret = sca3300_read_reg(data, SCA3300_REG_MODE,
> &reg_val);
> > +             ret = sca3300_get_op_mode(data, &reg_val);
> >               if (ret)
> >                       return ret;
> > -             *val = 0;
> > -             *val2 = sca3300_accel_scale[reg_val][1];
> > -             return IIO_VAL_INT_PLUS_MICRO;
> > +             switch (chan->type) {
> > +             case IIO_ACCEL:
> > +                     index =
> data->chip->accel_scale_modes_map[reg_val];
> > +                     *val = data->chip->accel_scale_table[index][0];
> > +                     *val2 = data->chip->accel_scale_table[index][1];
> > +                     return IIO_VAL_INT_PLUS_MICRO;
> > +             default:
> > +                     return -EINVAL;
> > +             }
> >       case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> > -             ret = sca3300_read_reg(data, SCA3300_REG_MODE,
> &reg_val);
> > +
> > +             ret = sca3300_get_op_mode(data, &reg_val);
> >               if (ret)
> >                       return ret;
> > -             *val = sca3300_lp_freq[reg_val];
> > +             index = data->chip->freq_modes_map[reg_val];
> > +             *val = data->chip->freq_table[index];
> >               return IIO_VAL_INT;
> >       default:
> >               return -EINVAL;
> > @@ -331,6 +421,7 @@ static int sca3300_init(struct sca3300_data
> > *sca_data,  {
> >       int value = 0;
> >       int ret;
> > +     int i;
> >
> >       ret = sca3300_write_reg(sca_data, SCA3300_REG_MODE,
> >                               SCA3300_MODE_SW_RESET); @@
> -347,12
> > +438,22 @@ static int sca3300_init(struct sca3300_data *sca_data,
> >       if (ret)
> >               return ret;
> >
> > -     if (value != SCA3300_WHOAMI_ID) {
> > -             dev_err(&sca_data->spi->dev,
> > -                     "device id not expected value, %d != %u\n",
> > -                     value, SCA3300_WHOAMI_ID);
> > +     for (i = 0; i < ARRAY_SIZE(sca3300_chip_tbl); i++) {
> > +             if (sca3300_chip_tbl[i].chip_id == value)
> > +                     break;
> > +     }> +    if (i == ARRAY_SIZE(sca3300_chip_tbl)) {
> > +             dev_err(&sca_data->spi->dev, "Invalid chip %x\n",
> > + value);
> Please start this with small letter, also perhaps resort to "unknown chip id %x\n"
> or so.
> >               return -ENODEV;
> >       }
> > +
> > +     indio_dev->available_scan_masks = sca3300_chip_tbl[i].scan_masks;
> > +     indio_dev->num_channels = sca3300_chip_tbl[i].num_channels;
> > +     indio_dev->channels = sca3300_chip_tbl[i].channels;
> > +     sca_data->chip = &sca3300_chip_tbl[i];
> > +     indio_dev->name = sca3300_chip_tbl[i].name;
> > +     indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> I would prefer to keep the above indio_dev additions in the probe function. That
> way we keep the init function clean(er).
> 
> Thanks,
> Tomas
> 
> >       return 0;
> >  }
> >
> > @@ -384,15 +485,21 @@ static int sca3300_read_avail(struct iio_dev
> *indio_dev,
> >                             const int **vals, int *type, int *length,
> >                             long mask)  {
> > +     struct sca3300_data *data = iio_priv(indio_dev);
> >       switch (mask) {
> >       case IIO_CHAN_INFO_SCALE:
> > -             *vals = (const int *)sca3300_accel_scale;
> > -             *length = ARRAY_SIZE(sca3300_accel_scale) * 2 - 2;
> > -             *type = IIO_VAL_INT_PLUS_MICRO;
> > -             return IIO_AVAIL_LIST;
> > +             switch (chan->type) {
> > +             case IIO_ACCEL:
> > +                     *vals = (const int *)data->chip->accel_scale_table;
> > +                     *length = data->chip->num_accel_scales;
> > +                     *type = IIO_VAL_INT_PLUS_MICRO;
> > +                     return IIO_AVAIL_LIST;
> > +             default:
> > +                     return -EINVAL;
> > +             }
> >       case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> > -             *vals = &sca3300_lp_freq[2];
> > -             *length = 2;
> > +             *vals = (const int *)data->chip->freq_table;
> > +             *length = data->chip->num_freqs;
> >               *type = IIO_VAL_INT;
> >               return IIO_AVAIL_LIST;
> >       default:
> > @@ -424,11 +531,6 @@ static int sca3300_probe(struct spi_device *spi)
> >       crc8_populate_msb(sca3300_crc_table,
> SCA3300_CRC8_POLYNOMIAL);
> >
> >       indio_dev->info = &sca3300_info;
> > -     indio_dev->name = SCA3300_ALIAS;
> > -     indio_dev->modes = INDIO_DIRECT_MODE;
> > -     indio_dev->channels = sca3300_channels;
> > -     indio_dev->num_channels = ARRAY_SIZE(sca3300_channels);
> > -     indio_dev->available_scan_masks = sca3300_scan_masks;
> >
> >       ret = sca3300_init(sca_data, indio_dev);
> >       if (ret) {

Re: [PATCH V3 3/5] iio: accel: sca3300: modified to support multi chips

[Melin Tomas]

Hi,

Some comments below.

On 04/05/2022 16:36, LI Qingwu wrote:
> The drive support sca3300 only.
> There are some other similar chips, for instance, SCL3300.
> Prepare the way for multiple chips and additional channels.
> Modify the driver to read the device id.
> Add the tables for the corresponding id to support multiple chips.
> Add prepares for the addition of extra channels.
> Add prepares for handling the operation modes for multiple chips.
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Reviewed-by: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
> ---
>  drivers/iio/accel/sca3300.c | 180 ++++++++++++++++++++++++++++--------
>  1 file changed, 141 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/iio/accel/sca3300.c b/drivers/iio/accel/sca3300.c
> index ff16d2cc8c70..1e0e6a2f7a63 100644
> --- a/drivers/iio/accel/sca3300.c
> +++ b/drivers/iio/accel/sca3300.c
> @@ -37,7 +37,6 @@
>  
>  /* Device ID */
>  #define SCA3300_REG_WHOAMI	0x10
> -#define SCA3300_WHOAMI_ID	0x51
I suggest leaving this here, define another value for scl3000 if needed.
>  
>  /* Device return status and mask */
>  #define SCA3300_VALUE_RS_ERROR	0x3
> @@ -93,15 +92,35 @@ static const struct iio_chan_spec sca3300_channels[] = {
>  	IIO_CHAN_SOFT_TIMESTAMP(4),
>  };
>  
> -static const int sca3300_lp_freq[] = {70, 70, 70, 10};
> -static const int sca3300_accel_scale[][2] = {{0, 370}, {0, 741}, {0, 185}, {0, 185}};
> +static const int sca3300_freq_tbl[] = {70, 10};
> +static const int sca3300_freq_modes_map[] = {0, 0, 0, 1};
>  
> +static const int sca3300_accel_scale_tbl[][2] = {{0, 370}, {0, 741}, {0, 185}};
> +static const int sca3300_accel_scale_modes_map[] = {0, 1, 2, 2};
Suggest keeping name of this intact, and keep name of the map variable
similar.
static const int sca3300_accel_scale[][2] = {{0, 370}, {0, 741}, {0, 185}};
static const int sca3300_accel_scale_map[] = {0, 1, 2, 2};

That keeps it easy to remember that these are related to each other.

> +
> +static const int sca3300_avail_modes_map[] = {0, 1, 2, 3};
>  static const unsigned long sca3300_scan_masks[] = {
>  	BIT(SCA3300_ACC_X) | BIT(SCA3300_ACC_Y) | BIT(SCA3300_ACC_Z) |
>  	BIT(SCA3300_TEMP),
>  	0
>  };
>  
> +struct sca3300_chip_info {
> +	const struct iio_chan_spec *channels;
> +	const int (*accel_scale_table)[2];> +	const int *accel_scale_modes_map;
> +	const unsigned long *scan_masks;
> +	const int *avail_modes_table;
> +	const int *freq_modes_map;
> +	const int *freq_table;> +	const u8 num_accel_scales;
> +	const u8 num_avail_modes;
> +	const u8 num_channels;
> +	const u8 num_freqs;
> +	const u8 chip_id;
> +	const char *name;
Suggest keeping related variables logically sorted in declaration
together instead of sorting according to length.
> +};
> +
>  /**
>   * struct sca3300_data - device data
>   * @spi: SPI device structure
> @@ -117,10 +136,28 @@ struct sca3300_data {
>  		s16 channels[4];
>  		s64 ts __aligned(sizeof(s64));
>  	} scan;
> +	const struct sca3300_chip_info *chip;
>  	u8 txbuf[4] ____cacheline_aligned;
>  	u8 rxbuf[4];
>  };
>  
> +static const struct sca3300_chip_info sca3300_chip_tbl[] = {
> +	{	.num_accel_scales = ARRAY_SIZE(sca3300_accel_scale_tbl)*2,> +		.accel_scale_modes_map = sca3300_accel_scale_modes_map,
> +		.accel_scale_table = sca3300_accel_scale_tbl,
> +		.num_channels = ARRAY_SIZE(sca3300_channels),
> +		.freq_modes_map = sca3300_freq_modes_map,
> +		.avail_modes_table = sca3300_avail_modes_map,
> +		.freq_table = sca3300_freq_tbl,
> +		.scan_masks = sca3300_scan_masks,
> +		.channels = sca3300_channels,
> +		.num_avail_modes = 4,
> +		.name = "sca3300",
> +		.chip_id = 0x51,
> +		.num_freqs = 2,
> +	},
> +};
> +
>  DECLARE_CRC8_TABLE(sca3300_crc_table);
>  
>  static int sca3300_transfer(struct sca3300_data *sca_data, int *val)
> @@ -227,36 +264,80 @@ static int sca3300_write_reg(struct sca3300_data *sca_data, u8 reg, int val)
>  	return sca3300_error_handler(sca_data);
>  }
>  
> +static int sca3300_set_op_mode(struct sca3300_data *sca_data, int index)
> +{
> +	int mode;
> +
> +	if ((index < 0) || (index >= sca_data->chip->num_avail_modes))
> +		return -EINVAL;
> +
> +	mode = sca_data->chip->avail_modes_table[index];
> +
> +	return sca3300_write_reg(sca_data, SCA3300_REG_MODE, mode);
> +}
> +
> +static int sca3300_get_op_mode(struct sca3300_data *sca_data, int *index)
> +{
> +	int reg_val;
> +	int ret;
> +	int i;
> +
> +	ret = sca3300_read_reg(sca_data, SCA3300_REG_MODE, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < sca_data->chip->num_avail_modes; i++) {
> +		if (sca_data->chip->avail_modes_table[i] == reg_val) {
> +			*index = i;
> +			break;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
>  static int sca3300_write_raw(struct iio_dev *indio_dev,
>  			     struct iio_chan_spec const *chan,
>  			     int val, int val2, long mask)
>  {
>  	struct sca3300_data *data = iio_priv(indio_dev);
> +	int mode = -1;
Suggest not setting to invalid default value, instead avoid calling
function if values are not found.

>  	int reg_val;
> +	int index;
>  	int ret;
>  	int i;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SCALE:
> -		if (val)
> +		if (chan->type != IIO_ACCEL)
>  			return -EINVAL;
Was there some problem with if (val) here, or why changed?
> -
> -		for (i = 0; i < ARRAY_SIZE(sca3300_accel_scale); i++) {
> -			if (val2 == sca3300_accel_scale[i][1])
> -				return sca3300_write_reg(data, SCA3300_REG_MODE, i);
> +		for (i = 0; i < data->chip->num_avail_modes; i++) { 4
> +			index = data->chip->accel_scale_modes_map[i];
> +			if ((val == data->chip->accel_scale_table[index][0]) &&
> +			(val2 == data->chip->accel_scale_table[index][1])) {
> +				mode = i;
> +				break;
> +			}
>  		}
> -		return -EINVAL;
> -
> +		return sca3300_set_op_mode(data, mode);
>  	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> -		ret = sca3300_read_reg(data, SCA3300_REG_MODE, &reg_val);
> +		ret = sca3300_get_op_mode(data, &reg_val);
>  		if (ret)
>  			return ret;
> -		/* freq. change is possible only for mode 3 and 4 */
> -		if (reg_val == 2 && val == sca3300_lp_freq[3])
> -			return sca3300_write_reg(data, SCA3300_REG_MODE, 3);
> -		if (reg_val == 3 && val == sca3300_lp_freq[2])
> -			return sca3300_write_reg(data, SCA3300_REG_MODE, 2);
> -		return -EINVAL;
> +		index = data->chip->accel_scale_modes_map[reg_val];
> +		for (i = 0; i < data->chip->num_avail_modes; i++) {
> +			int index_new = data->chip->accel_scale_modes_map[i]; 0122
> +			int index_freq = data->chip->freq_modes_map[i];
Declare variables at top of function.
> +
> +			if (val == data->chip->freq_table[index_freq]) {
> +				if (data->chip->accel_scale_table[index] ==
> +				data->chip->accel_scale_table[index_new]) {
To be compatible with existing code, this should check that mode is 2 or
3. I don't see how this does that, please explain.

> +					mode = i;
> +					break;
> +				}
> +			}
> +		}
> +		return sca3300_set_op_mode(data, mode);
>  	default:
>  		return -EINVAL;
>  	}
> @@ -267,8 +348,9 @@ static int sca3300_read_raw(struct iio_dev *indio_dev,
>  			    int *val, int *val2, long mask)
>  {
>  	struct sca3300_data *data = iio_priv(indio_dev);
> -	int ret;
>  	int reg_val;
> +	int index;
> +	int ret;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> @@ -277,17 +359,25 @@ static int sca3300_read_raw(struct iio_dev *indio_dev,
>  			return ret;
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_SCALE:
> -		ret = sca3300_read_reg(data, SCA3300_REG_MODE, &reg_val);
> +		ret = sca3300_get_op_mode(data, &reg_val);
>  		if (ret)
>  			return ret;
> -		*val = 0;
> -		*val2 = sca3300_accel_scale[reg_val][1];
> -		return IIO_VAL_INT_PLUS_MICRO;
> +		switch (chan->type) {
> +		case IIO_ACCEL:
> +			index = data->chip->accel_scale_modes_map[reg_val];
> +			*val = data->chip->accel_scale_table[index][0];
> +			*val2 = data->chip->accel_scale_table[index][1];
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		default:
> +			return -EINVAL;
> +		}
>  	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> -		ret = sca3300_read_reg(data, SCA3300_REG_MODE, &reg_val);
> +
> +		ret = sca3300_get_op_mode(data, &reg_val);
>  		if (ret)
>  			return ret;
> -		*val = sca3300_lp_freq[reg_val];
> +		index = data->chip->freq_modes_map[reg_val];
> +		*val = data->chip->freq_table[index];
>  		return IIO_VAL_INT;
>  	default:
>  		return -EINVAL;
> @@ -331,6 +421,7 @@ static int sca3300_init(struct sca3300_data *sca_data,
>  {
>  	int value = 0;
>  	int ret;
> +	int i;
>  
>  	ret = sca3300_write_reg(sca_data, SCA3300_REG_MODE,
>  				SCA3300_MODE_SW_RESET);
> @@ -347,12 +438,22 @@ static int sca3300_init(struct sca3300_data *sca_data,
>  	if (ret)
>  		return ret;
>  
> -	if (value != SCA3300_WHOAMI_ID) {
> -		dev_err(&sca_data->spi->dev,
> -			"device id not expected value, %d != %u\n",
> -			value, SCA3300_WHOAMI_ID);
> +	for (i = 0; i < ARRAY_SIZE(sca3300_chip_tbl); i++) {
> +		if (sca3300_chip_tbl[i].chip_id == value)
> +			break;
> +	}> +	if (i == ARRAY_SIZE(sca3300_chip_tbl)) {
> +		dev_err(&sca_data->spi->dev, "Invalid chip %x\n", value);
Please start this with small letter, also perhaps "unknown chip id %x\n"
>  		return -ENODEV;
>  	}
> +
> +	indio_dev->available_scan_masks = sca3300_chip_tbl[i].scan_masks;
> +	indio_dev->num_channels = sca3300_chip_tbl[i].num_channels;
> +	indio_dev->channels = sca3300_chip_tbl[i].channels;
> +	sca_data->chip = &sca3300_chip_tbl[i];
> +	indio_dev->name = sca3300_chip_tbl[i].name;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
I would prefer to keep the above indio_dev additions in the probe
function. That way we keep the init function clean(er).
>  	return 0;
>  }
>  
> @@ -384,15 +485,21 @@ static int sca3300_read_avail(struct iio_dev *indio_dev,
>  			      const int **vals, int *type, int *length,
>  			      long mask)
>  {
> +	struct sca3300_data *data = iio_priv(indio_dev);
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SCALE:
> -		*vals = (const int *)sca3300_accel_scale;
> -		*length = ARRAY_SIZE(sca3300_accel_scale) * 2 - 2;
> -		*type = IIO_VAL_INT_PLUS_MICRO;
> -		return IIO_AVAIL_LIST;
> +		switch (chan->type) {
> +		case IIO_ACCEL:
> +			*vals = (const int *)data->chip->accel_scale_table;
> +			*length = data->chip->num_accel_scales;
> +			*type = IIO_VAL_INT_PLUS_MICRO;
> +			return IIO_AVAIL_LIST;
> +		default:
> +			return -EINVAL;
> +		}
>  	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> -		*vals = &sca3300_lp_freq[2];
> -		*length = 2;
> +		*vals = (const int *)data->chip->freq_table;
> +		*length = data->chip->num_freqs;
>  		*type = IIO_VAL_INT;
>  		return IIO_AVAIL_LIST;
>  	default:
> @@ -424,11 +531,6 @@ static int sca3300_probe(struct spi_device *spi)
>  	crc8_populate_msb(sca3300_crc_table, SCA3300_CRC8_POLYNOMIAL);
>  
>  	indio_dev->info = &sca3300_info;
> -	indio_dev->name = SCA3300_ALIAS;
> -	indio_dev->modes = INDIO_DIRECT_MODE;
> -	indio_dev->channels = sca3300_channels;
> -	indio_dev->num_channels = ARRAY_SIZE(sca3300_channels);
> -	indio_dev->available_scan_masks = sca3300_scan_masks;
>  
>  	ret = sca3300_init(sca_data, indio_dev);
>  	if (ret) {

Re: [PATCH V3 3/5] iio: accel: sca3300: modified to support multi chips

[Jonathan Cameron]

On Wed,  4 May 2022 13:36:10 +0000
LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn> wrote:

> The drive support sca3300 only.
> There are some other similar chips, for instance, SCL3300.
> Prepare the way for multiple chips and additional channels.
> Modify the driver to read the device id.
> Add the tables for the corresponding id to support multiple chips.
> Add prepares for the addition of extra channels.
> Add prepares for handling the operation modes for multiple chips.
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Reviewed-by: Jonathan Cameron <jic23@kernel.org>

These tags were not give, so don't add them...

Various other comment inline, many of which probably overlap with other reviews

Thanks,

Jonathan

> Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
> ---
>  drivers/iio/accel/sca3300.c | 180 ++++++++++++++++++++++++++++--------
>  1 file changed, 141 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/iio/accel/sca3300.c b/drivers/iio/accel/sca3300.c
> index ff16d2cc8c70..1e0e6a2f7a63 100644
> --- a/drivers/iio/accel/sca3300.c
> +++ b/drivers/iio/accel/sca3300.c
> @@ -37,7 +37,6 @@
>  
>  /* Device ID */
>  #define SCA3300_REG_WHOAMI	0x10
> -#define SCA3300_WHOAMI_ID	0x51
>  
>  /* Device return status and mask */
>  #define SCA3300_VALUE_RS_ERROR	0x3
> @@ -93,15 +92,35 @@ static const struct iio_chan_spec sca3300_channels[] = {
>  	IIO_CHAN_SOFT_TIMESTAMP(4),
>  };
>  
> -static const int sca3300_lp_freq[] = {70, 70, 70, 10};
> -static const int sca3300_accel_scale[][2] = {{0, 370}, {0, 741}, {0, 185}, {0, 185}};
> +static const int sca3300_freq_tbl[] = {70, 10};
> +static const int sca3300_freq_modes_map[] = {0, 0, 0, 1};
>  
> +static const int sca3300_accel_scale_tbl[][2] = {{0, 370}, {0, 741}, {0, 185}};
> +static const int sca3300_accel_scale_modes_map[] = {0, 1, 2, 2};
> +
> +static const int sca3300_avail_modes_map[] = {0, 1, 2, 3};
>  static const unsigned long sca3300_scan_masks[] = {
>  	BIT(SCA3300_ACC_X) | BIT(SCA3300_ACC_Y) | BIT(SCA3300_ACC_Z) |
>  	BIT(SCA3300_TEMP),
>  	0
>  };
>  
> +struct sca3300_chip_info {

See below, though others already mentioned it.

> +	const struct iio_chan_spec *channels;
> +	const int (*accel_scale_table)[2];
> +	const int *accel_scale_modes_map;
> +	const unsigned long *scan_masks;
> +	const int *avail_modes_table;
> +	const int *freq_modes_map;
> +	const int *freq_table;
> +	const u8 num_accel_scales;
> +	const u8 num_avail_modes;
> +	const u8 num_channels;
> +	const u8 num_freqs;
> +	const u8 chip_id;
> +	const char *name;
> +};
> +
>  /**
>   * struct sca3300_data - device data
>   * @spi: SPI device structure
> @@ -117,10 +136,28 @@ struct sca3300_data {
>  		s16 channels[4];
>  		s64 ts __aligned(sizeof(s64));
>  	} scan;
> +	const struct sca3300_chip_info *chip;
>  	u8 txbuf[4] ____cacheline_aligned;
>  	u8 rxbuf[4];
>  };
>  
> +static const struct sca3300_chip_info sca3300_chip_tbl[] = {

Others commented on this but definitely don't do this.  Keep the order
the same as that of the structure definition and for that
order them so that related items are next to each other.

> +	{	.num_accel_scales = ARRAY_SIZE(sca3300_accel_scale_tbl)*2,
> +		.accel_scale_modes_map = sca3300_accel_scale_modes_map,
> +		.accel_scale_table = sca3300_accel_scale_tbl,
> +		.num_channels = ARRAY_SIZE(sca3300_channels),
> +		.freq_modes_map = sca3300_freq_modes_map,
> +		.avail_modes_table = sca3300_avail_modes_map,
> +		.freq_table = sca3300_freq_tbl,
> +		.scan_masks = sca3300_scan_masks,
> +		.channels = sca3300_channels,
> +		.num_avail_modes = 4,
> +		.name = "sca3300",
> +		.chip_id = 0x51,
> +		.num_freqs = 2,
> +	},
> +};
> +
>  DECLARE_CRC8_TABLE(sca3300_crc_table);
>  
>  static int sca3300_transfer(struct sca3300_data *sca_data, int *val)
> @@ -227,36 +264,80 @@ static int sca3300_write_reg(struct sca3300_data *sca_data, u8 reg, int val)
>  	return sca3300_error_handler(sca_data);
>  }
>  
> +static int sca3300_set_op_mode(struct sca3300_data *sca_data, int index)
> +{
> +	int mode;
> +
> +	if ((index < 0) || (index >= sca_data->chip->num_avail_modes))
> +		return -EINVAL;
> +
> +	mode = sca_data->chip->avail_modes_table[index];
> +
> +	return sca3300_write_reg(sca_data, SCA3300_REG_MODE, mode);

	return sca3300_write_reg(sca_data, SCA3300_REG_MODE,
				 sca_data->chip->avail_modes_table[index]);
doesn't hurt reability or lead to particularly long line length so
better to save us a few lines of code and the local variable.

> +}
> +
> +static int sca3300_get_op_mode(struct sca3300_data *sca_data, int *index)
> +{
> +	int reg_val;
> +	int ret;
> +	int i;
> +
> +	ret = sca3300_read_reg(sca_data, SCA3300_REG_MODE, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < sca_data->chip->num_avail_modes; i++) {
> +		if (sca_data->chip->avail_modes_table[i] == reg_val) {
> +			*index = i;
> +			break;
			return 0;
> +		}
> +	}
> +
> +	return ret;

Not an error if we get here without finding a match?

> +}
> +
>  static int sca3300_write_raw(struct iio_dev *indio_dev,
>  			     struct iio_chan_spec const *chan,
>  			     int val, int val2, long mask)
>  {
>  	struct sca3300_data *data = iio_priv(indio_dev);
> +	int mode = -1;
>  	int reg_val;
> +	int index;
>  	int ret;
>  	int i;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SCALE:
> -		if (val)
> +		if (chan->type != IIO_ACCEL)
>  			return -EINVAL;
> -
> -		for (i = 0; i < ARRAY_SIZE(sca3300_accel_scale); i++) {
> -			if (val2 == sca3300_accel_scale[i][1])
> -				return sca3300_write_reg(data, SCA3300_REG_MODE, i);
> +		for (i = 0; i < data->chip->num_avail_modes; i++) {
> +			index = data->chip->accel_scale_modes_map[i];
> +			if ((val == data->chip->accel_scale_table[index][0]) &&
> +			(val2 == data->chip->accel_scale_table[index][1])) {

Align this so it is clear that the second line is part of the ocnditional.

			if ((val...
			    (val2 ==

> +				mode = i;
> +				break;
> +			}

What if there isn't a match?

>  		}
> -		return -EINVAL;
> -
> +		return sca3300_set_op_mode(data, mode);
>  	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> -		ret = sca3300_read_reg(data, SCA3300_REG_MODE, &reg_val);
> +		ret = sca3300_get_op_mode(data, &reg_val);
>  		if (ret)
>  			return ret;
> -		/* freq. change is possible only for mode 3 and 4 */
> -		if (reg_val == 2 && val == sca3300_lp_freq[3])
> -			return sca3300_write_reg(data, SCA3300_REG_MODE, 3);
> -		if (reg_val == 3 && val == sca3300_lp_freq[2])
> -			return sca3300_write_reg(data, SCA3300_REG_MODE, 2);
> -		return -EINVAL;
> +		index = data->chip->accel_scale_modes_map[reg_val];
> +		for (i = 0; i < data->chip->num_avail_modes; i++) {
> +			int index_new = data->chip->accel_scale_modes_map[i];
> +			int index_freq = data->chip->freq_modes_map[i];
> +
> +			if (val == data->chip->freq_table[index_freq]) {
> +				if (data->chip->accel_scale_table[index] ==
> +				data->chip->accel_scale_table[index_new]) {

as before, fix alignment and handle case where there isn't a match.


> +					mode = i;
> +					break;
> +				}
> +			}
> +		}
> +		return sca3300_set_op_mode(data, mode);
>  	default:
>  		return -EINVAL;
>  	}
> @@ -267,8 +348,9 @@ static int sca3300_read_raw(struct iio_dev *indio_dev,
>  			    int *val, int *val2, long mask)
>  {
>  	struct sca3300_data *data = iio_priv(indio_dev);
> -	int ret;
>  	int reg_val;
> +	int index;
> +	int ret;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> @@ -277,17 +359,25 @@ static int sca3300_read_raw(struct iio_dev *indio_dev,
>  			return ret;
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_SCALE:
> -		ret = sca3300_read_reg(data, SCA3300_REG_MODE, &reg_val);
> +		ret = sca3300_get_op_mode(data, &reg_val);
>  		if (ret)
>  			return ret;
> -		*val = 0;
> -		*val2 = sca3300_accel_scale[reg_val][1];
> -		return IIO_VAL_INT_PLUS_MICRO;
> +		switch (chan->type) {
> +		case IIO_ACCEL:
> +			index = data->chip->accel_scale_modes_map[reg_val];
> +			*val = data->chip->accel_scale_table[index][0];
> +			*val2 = data->chip->accel_scale_table[index][1];
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		default:
> +			return -EINVAL;
> +		}
>  	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> -		ret = sca3300_read_reg(data, SCA3300_REG_MODE, &reg_val);
> +
> +		ret = sca3300_get_op_mode(data, &reg_val);
>  		if (ret)
>  			return ret;
> -		*val = sca3300_lp_freq[reg_val];
> +		index = data->chip->freq_modes_map[reg_val];
> +		*val = data->chip->freq_table[index];
>  		return IIO_VAL_INT;
>  	default:
>  		return -EINVAL;
> @@ -331,6 +421,7 @@ static int sca3300_init(struct sca3300_data *sca_data,
>  {
>  	int value = 0;
>  	int ret;
> +	int i;
>  
>  	ret = sca3300_write_reg(sca_data, SCA3300_REG_MODE,
>  				SCA3300_MODE_SW_RESET);
> @@ -347,12 +438,22 @@ static int sca3300_init(struct sca3300_data *sca_data,
>  	if (ret)
>  		return ret;
>  
> -	if (value != SCA3300_WHOAMI_ID) {
> -		dev_err(&sca_data->spi->dev,
> -			"device id not expected value, %d != %u\n",
> -			value, SCA3300_WHOAMI_ID);
> +	for (i = 0; i < ARRAY_SIZE(sca3300_chip_tbl); i++) {
> +		if (sca3300_chip_tbl[i].chip_id == value)
> +			break;
> +	}
> +	if (i == ARRAY_SIZE(sca3300_chip_tbl)) {
> +		dev_err(&sca_data->spi->dev, "Invalid chip %x\n", value);
>  		return -ENODEV;
>  	}
> +
> +	indio_dev->available_scan_masks = sca3300_chip_tbl[i].scan_masks;
> +	indio_dev->num_channels = sca3300_chip_tbl[i].num_channels;
> +	indio_dev->channels = sca3300_chip_tbl[i].channels;
> +	sca_data->chip = &sca3300_chip_tbl[i];
> +	indio_dev->name = sca3300_chip_tbl[i].name;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
>  	return 0;
>  }
>  
> @@ -384,15 +485,21 @@ static int sca3300_read_avail(struct iio_dev *indio_dev,
>  			      const int **vals, int *type, int *length,
>  			      long mask)
>  {
> +	struct sca3300_data *data = iio_priv(indio_dev);
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SCALE:
> -		*vals = (const int *)sca3300_accel_scale;
> -		*length = ARRAY_SIZE(sca3300_accel_scale) * 2 - 2;
> -		*type = IIO_VAL_INT_PLUS_MICRO;
> -		return IIO_AVAIL_LIST;
> +		switch (chan->type) {
> +		case IIO_ACCEL:
> +			*vals = (const int *)data->chip->accel_scale_table;
> +			*length = data->chip->num_accel_scales;
> +			*type = IIO_VAL_INT_PLUS_MICRO;
> +			return IIO_AVAIL_LIST;
> +		default:
> +			return -EINVAL;
> +		}
>  	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> -		*vals = &sca3300_lp_freq[2];
> -		*length = 2;
> +		*vals = (const int *)data->chip->freq_table;
> +		*length = data->chip->num_freqs;
>  		*type = IIO_VAL_INT;
>  		return IIO_AVAIL_LIST;
>  	default:
> @@ -424,11 +531,6 @@ static int sca3300_probe(struct spi_device *spi)
>  	crc8_populate_msb(sca3300_crc_table, SCA3300_CRC8_POLYNOMIAL);
>  
>  	indio_dev->info = &sca3300_info;
> -	indio_dev->name = SCA3300_ALIAS;
> -	indio_dev->modes = INDIO_DIRECT_MODE;
> -	indio_dev->channels = sca3300_channels;
> -	indio_dev->num_channels = ARRAY_SIZE(sca3300_channels);
> -	indio_dev->available_scan_masks = sca3300_scan_masks;
>  
>  	ret = sca3300_init(sca_data, indio_dev);
>  	if (ret) {

[PATCH V3 4/5] iio: accel: sca3300: Add support for SCL3300

[LI Qingwu]

Add support for Murata SCL3300, a 3-axis MEMS accelerometer.
Same as SCA3300, it has the accelerometer and temperature output.
Datasheet:
www.murata.com/en-us/products/sensor/inclinometer/overview/lineup/scl3300

Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
---
 drivers/iio/accel/sca3300.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/iio/accel/sca3300.c b/drivers/iio/accel/sca3300.c
index 1e0e6a2f7a63..040f8e1a1327 100644
--- a/drivers/iio/accel/sca3300.c
+++ b/drivers/iio/accel/sca3300.c
@@ -93,12 +93,17 @@ static const struct iio_chan_spec sca3300_channels[] = {
 };
 
 static const int sca3300_freq_tbl[] = {70, 10};
+static const int scl3300_freq_tbl[] = {40, 70, 10};
 static const int sca3300_freq_modes_map[] = {0, 0, 0, 1};
+static const int scl3300_freq_modes_map[] = {0, 1, 2};
 
 static const int sca3300_accel_scale_tbl[][2] = {{0, 370}, {0, 741}, {0, 185}};
+static const int scl3300_accel_scale_tbl[][2] = {{0, 167}, {0, 333}, {0, 83}};
 static const int sca3300_accel_scale_modes_map[] = {0, 1, 2, 2};
+static const int scl3300_accel_scale_modes_map[] = {0, 1, 2};
 
 static const int sca3300_avail_modes_map[] = {0, 1, 2, 3};
+static const int scl3300_avail_modes_map[] = {0, 1, 3};
 static const unsigned long sca3300_scan_masks[] = {
 	BIT(SCA3300_ACC_X) | BIT(SCA3300_ACC_Y) | BIT(SCA3300_ACC_Z) |
 	BIT(SCA3300_TEMP),
@@ -156,6 +161,20 @@ static const struct sca3300_chip_info sca3300_chip_tbl[] = {
 		.chip_id = 0x51,
 		.num_freqs = 2,
 	},
+	{	.num_accel_scales = ARRAY_SIZE(scl3300_accel_scale_tbl)*2,
+		.accel_scale_modes_map = scl3300_accel_scale_modes_map,
+		.accel_scale_table = scl3300_accel_scale_tbl,
+		.num_channels = ARRAY_SIZE(sca3300_channels),
+		.avail_modes_table = scl3300_avail_modes_map,
+		.freq_modes_map = scl3300_freq_modes_map,
+		.scan_masks = sca3300_scan_masks,
+		.freq_table = scl3300_freq_tbl,
+		.channels = sca3300_channels,
+		.num_avail_modes = 3,
+		.name = "scl3300",
+		.chip_id = 0xC1,
+		.num_freqs = 3,
+	},
 };
 
 DECLARE_CRC8_TABLE(sca3300_crc_table);
@@ -558,6 +577,7 @@ static int sca3300_probe(struct spi_device *spi)
 
 static const struct of_device_id sca3300_dt_ids[] = {
 	{ .compatible = "murata,sca3300"},
+	{ .compatible = "murata,scl3300"},
 	{}
 };
 MODULE_DEVICE_TABLE(of, sca3300_dt_ids);
-- 
2.25.1

[PATCH V3 5/5] iio: accel: sca3300: Add inclination channels

[LI Qingwu]

Different from SCA3300, SCL3300 can output inclination angles.
Angles are formed from acceleration with following equations:
ANG_X = atan2(accx / √(accy^2 + accz^2)),
ANG_Y = atan2(accy / √(accx^2 + accz^2)),
ANG_Z = atan2(accz / √(accx^2 + accy^2)),

The commit adds the output of the raw value, scale
and scale_available of angles.

New interfaces:
  in_incli_scale
  in_incli_scale_available
  in_incli_x_raw
  in_incli_y_raw
  in_incli_z_raw
Data converted by application of scale to degrees.

Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
---
 drivers/iio/accel/sca3300.c | 79 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 77 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/accel/sca3300.c b/drivers/iio/accel/sca3300.c
index 040f8e1a1327..cf588f014407 100644
--- a/drivers/iio/accel/sca3300.c
+++ b/drivers/iio/accel/sca3300.c
@@ -42,12 +42,18 @@
 #define SCA3300_VALUE_RS_ERROR	0x3
 #define SCA3300_MASK_RS_STATUS	GENMASK(1, 0)
 
+#define SCA3300_REG_ANG_CTRL 0x0C
+#define SCA3300_ANG_ENABLE   0x1F
+
 enum sca3300_scan_indexes {
 	SCA3300_ACC_X = 0,
 	SCA3300_ACC_Y,
 	SCA3300_ACC_Z,
 	SCA3300_TEMP,
 	SCA3300_TIMESTAMP,
+	SCA3300_INCLI_X,
+	SCA3300_INCLI_Y,
+	SCA3300_INCLI_Z,
 };
 
 #define SCA3300_ACCEL_CHANNEL(index, reg, axis) {			\
@@ -71,6 +77,24 @@ enum sca3300_scan_indexes {
 	},								\
 }
 
+#define SCA3300_INCLI_CHANNEL(index, reg, axis) {			\
+	.type = IIO_INCLI,						\
+	.address = reg,							\
+	.modified = 1,							\
+	.channel2 = IIO_MOD_##axis,					\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
+	.info_mask_shared_by_type_available =				\
+	BIT(IIO_CHAN_INFO_SCALE),					\
+	.scan_index = index,						\
+	.scan_type = {							\
+		.sign = 's',						\
+		.realbits = 16,						\
+		.storagebits = 16,					\
+		.endianness = IIO_CPU,					\
+	},								\
+}
+
 #define SCA3300_TEMP_CHANNEL(index, reg) {				\
 		.type = IIO_TEMP,					\
 		.address = reg,						\
@@ -102,28 +126,54 @@ static const int scl3300_accel_scale_tbl[][2] = {{0, 167}, {0, 333}, {0, 83}};
 static const int sca3300_accel_scale_modes_map[] = {0, 1, 2, 2};
 static const int scl3300_accel_scale_modes_map[] = {0, 1, 2};
 
+static const int scl3300_incli_scale_tbl[][2] = {{0, 5495}};
+static const int scl3300_incli_scale_modes_map[] = {0, 0, 0};
+
 static const int sca3300_avail_modes_map[] = {0, 1, 2, 3};
 static const int scl3300_avail_modes_map[] = {0, 1, 3};
+
+static const struct iio_chan_spec scl3300_channels[] = {
+	SCA3300_ACCEL_CHANNEL(SCA3300_ACC_X, 0x1, X),
+	SCA3300_ACCEL_CHANNEL(SCA3300_ACC_Y, 0x2, Y),
+	SCA3300_ACCEL_CHANNEL(SCA3300_ACC_Z, 0x3, Z),
+	SCA3300_TEMP_CHANNEL(SCA3300_TEMP, 0x05),
+	IIO_CHAN_SOFT_TIMESTAMP(SCA3300_TIMESTAMP),
+	SCA3300_INCLI_CHANNEL(SCA3300_INCLI_X, 0x09, X),
+	SCA3300_INCLI_CHANNEL(SCA3300_INCLI_Y, 0x0A, Y),
+	SCA3300_INCLI_CHANNEL(SCA3300_INCLI_Z, 0x0B, Z),
+};
+
 static const unsigned long sca3300_scan_masks[] = {
 	BIT(SCA3300_ACC_X) | BIT(SCA3300_ACC_Y) | BIT(SCA3300_ACC_Z) |
 	BIT(SCA3300_TEMP),
 	0
 };
 
+static const unsigned long scl3300_scan_masks[] = {
+	BIT(SCA3300_ACC_X) | BIT(SCA3300_ACC_Y) | BIT(SCA3300_ACC_Z) |
+	BIT(SCA3300_TEMP) |
+	BIT(SCA3300_INCLI_X) | BIT(SCA3300_INCLI_Y) | BIT(SCA3300_INCLI_Z),
+	0
+};
+
 struct sca3300_chip_info {
 	const struct iio_chan_spec *channels;
 	const int (*accel_scale_table)[2];
+	const int (*incli_scale_table)[2];
 	const int *accel_scale_modes_map;
+	const int *incli_scale_modes_map;
 	const unsigned long *scan_masks;
 	const int *avail_modes_table;
 	const int *freq_modes_map;
 	const int *freq_table;
 	const u8 num_accel_scales;
+	const u8 num_incli_scales;
 	const u8 num_avail_modes;
 	const u8 num_channels;
 	const u8 num_freqs;
 	const u8 chip_id;
 	const char *name;
+	const bool angle;
 };
 
 /**
@@ -156,24 +206,32 @@ static const struct sca3300_chip_info sca3300_chip_tbl[] = {
 		.freq_table = sca3300_freq_tbl,
 		.scan_masks = sca3300_scan_masks,
 		.channels = sca3300_channels,
+		.incli_scale_modes_map = NULL,
+		.incli_scale_table = NULL,
+		.num_incli_scales = 0,
 		.num_avail_modes = 4,
 		.name = "sca3300",
 		.chip_id = 0x51,
 		.num_freqs = 2,
+		.angle = false,
 	},
 	{	.num_accel_scales = ARRAY_SIZE(scl3300_accel_scale_tbl)*2,
+		.num_incli_scales = ARRAY_SIZE(scl3300_incli_scale_tbl)*2,
 		.accel_scale_modes_map = scl3300_accel_scale_modes_map,
+		.incli_scale_modes_map = scl3300_incli_scale_modes_map,
 		.accel_scale_table = scl3300_accel_scale_tbl,
-		.num_channels = ARRAY_SIZE(sca3300_channels),
+		.incli_scale_table = scl3300_incli_scale_tbl,
+		.num_channels = ARRAY_SIZE(scl3300_channels),
 		.avail_modes_table = scl3300_avail_modes_map,
 		.freq_modes_map = scl3300_freq_modes_map,
 		.scan_masks = sca3300_scan_masks,
 		.freq_table = scl3300_freq_tbl,
-		.channels = sca3300_channels,
+		.channels = scl3300_channels,
 		.num_avail_modes = 3,
 		.name = "scl3300",
 		.chip_id = 0xC1,
 		.num_freqs = 3,
+		.angle = true,
 	},
 };
 
@@ -382,6 +440,11 @@ static int sca3300_read_raw(struct iio_dev *indio_dev,
 		if (ret)
 			return ret;
 		switch (chan->type) {
+		case IIO_INCLI:
+			index = data->chip->incli_scale_modes_map[reg_val];
+			*val = data->chip->incli_scale_table[index][0];
+			*val2 = data->chip->incli_scale_table[index][1];
+			return IIO_VAL_INT_PLUS_MICRO;
 		case IIO_ACCEL:
 			index = data->chip->accel_scale_modes_map[reg_val];
 			*val = data->chip->accel_scale_table[index][0];
@@ -473,6 +536,13 @@ static int sca3300_init(struct sca3300_data *sca_data,
 	indio_dev->name = sca3300_chip_tbl[i].name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
+	if (sca_data->chip->angle) {
+		ret = sca3300_write_reg(sca_data, SCA3300_REG_ANG_CTRL,
+					SCA3300_ANG_ENABLE);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
@@ -508,6 +578,11 @@ static int sca3300_read_avail(struct iio_dev *indio_dev,
 	switch (mask) {
 	case IIO_CHAN_INFO_SCALE:
 		switch (chan->type) {
+		case IIO_INCLI:
+			*vals = (const int *)data->chip->incli_scale_table;
+			*length = data->chip->num_incli_scales;
+			*type = IIO_VAL_INT_PLUS_MICRO;
+			return IIO_AVAIL_LIST;
 		case IIO_ACCEL:
 			*vals = (const int *)data->chip->accel_scale_table;
 			*length = data->chip->num_accel_scales;
-- 
2.25.1

Re: [PATCH V3 5/5] iio: accel: sca3300: Add inclination channels

[kernel test robot]

Hi LI,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on v5.18-rc5 next-20220504]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/LI-Qingwu/iio-accel-sca3300-add-compitible-for-scl3300/20220504-213941
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: x86_64-randconfig-a016 (https://download.01.org/0day-ci/archive/20220505/202205050139.AiGvfpnI-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 363b3a645a1e30011cc8da624f13dac5fd915628)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/7d890b741d98d981843f66de413c1420e3fa27d3
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review LI-Qingwu/iio-accel-sca3300-add-compitible-for-scl3300/20220504-213941
        git checkout 7d890b741d98d981843f66de413c1420e3fa27d3
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/iio/accel/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/iio/accel/sca3300.c:152:28: warning: unused variable 'scl3300_scan_masks' [-Wunused-const-variable]
   static const unsigned long scl3300_scan_masks[] = {
                              ^
   1 warning generated.


vim +/scl3300_scan_masks +152 drivers/iio/accel/sca3300.c

   151	
 > 152	static const unsigned long scl3300_scan_masks[] = {
   153		BIT(SCA3300_ACC_X) | BIT(SCA3300_ACC_Y) | BIT(SCA3300_ACC_Z) |
   154		BIT(SCA3300_TEMP) |
   155		BIT(SCA3300_INCLI_X) | BIT(SCA3300_INCLI_Y) | BIT(SCA3300_INCLI_Z),
   156		0
   157	};
   158	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH V3 5/5] iio: accel: sca3300: Add inclination channels

[Jonathan Cameron]

On Wed,  4 May 2022 13:36:12 +0000
LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn> wrote:

> Different from SCA3300, SCL3300 can output inclination angles.
> Angles are formed from acceleration with following equations:
> ANG_X = atan2(accx / √(accy^2 + accz^2)),
> ANG_Y = atan2(accy / √(accx^2 + accz^2)),
> ANG_Z = atan2(accz / √(accx^2 + accy^2)),
> 
> The commit adds the output of the raw value, scale
> and scale_available of angles.
> 
> New interfaces:
>   in_incli_scale
>   in_incli_scale_available
>   in_incli_x_raw
>   in_incli_y_raw
>   in_incli_z_raw
> Data converted by application of scale to degrees.
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Reviewed-by: Jonathan Cameron <jic23@kernel.org>

Don't add tags for other people.  They have to be explicitly given by the person
in question.

This isn't reflecting that people reviewed the code, but that they
reviewed the code and are happy with it.  That's not yet the case
and even if they are happy, it is up to individual reviewers to give
a tag or not as they wish.

Easy to tell in my case as I don't use that email address for tags ;)

Jonathan



> Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
> ---
>  drivers/iio/accel/sca3300.c | 79 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 77 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/accel/sca3300.c b/drivers/iio/accel/sca3300.c
> index 040f8e1a1327..cf588f014407 100644
> --- a/drivers/iio/accel/sca3300.c
> +++ b/drivers/iio/accel/sca3300.c
> @@ -42,12 +42,18 @@
>  #define SCA3300_VALUE_RS_ERROR	0x3
>  #define SCA3300_MASK_RS_STATUS	GENMASK(1, 0)
>  
> +#define SCA3300_REG_ANG_CTRL 0x0C
> +#define SCA3300_ANG_ENABLE   0x1F
> +
>  enum sca3300_scan_indexes {
>  	SCA3300_ACC_X = 0,
>  	SCA3300_ACC_Y,
>  	SCA3300_ACC_Z,
>  	SCA3300_TEMP,
>  	SCA3300_TIMESTAMP,
> +	SCA3300_INCLI_X,
> +	SCA3300_INCLI_Y,
> +	SCA3300_INCLI_Z,
>  };
>  
>  #define SCA3300_ACCEL_CHANNEL(index, reg, axis) {			\
> @@ -71,6 +77,24 @@ enum sca3300_scan_indexes {
>  	},								\
>  }
>  
> +#define SCA3300_INCLI_CHANNEL(index, reg, axis) {			\
> +	.type = IIO_INCLI,						\
> +	.address = reg,							\
> +	.modified = 1,							\
> +	.channel2 = IIO_MOD_##axis,					\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
> +	.info_mask_shared_by_type_available =				\
> +	BIT(IIO_CHAN_INFO_SCALE),					\
> +	.scan_index = index,						\
> +	.scan_type = {							\
> +		.sign = 's',						\
> +		.realbits = 16,						\
> +		.storagebits = 16,					\
> +		.endianness = IIO_CPU,					\
> +	},								\
> +}
> +
>  #define SCA3300_TEMP_CHANNEL(index, reg) {				\
>  		.type = IIO_TEMP,					\
>  		.address = reg,						\
> @@ -102,28 +126,54 @@ static const int scl3300_accel_scale_tbl[][2] = {{0, 167}, {0, 333}, {0, 83}};
>  static const int sca3300_accel_scale_modes_map[] = {0, 1, 2, 2};
>  static const int scl3300_accel_scale_modes_map[] = {0, 1, 2};
>  
> +static const int scl3300_incli_scale_tbl[][2] = {{0, 5495}};
> +static const int scl3300_incli_scale_modes_map[] = {0, 0, 0};
> +
>  static const int sca3300_avail_modes_map[] = {0, 1, 2, 3};
>  static const int scl3300_avail_modes_map[] = {0, 1, 3};
> +
> +static const struct iio_chan_spec scl3300_channels[] = {
> +	SCA3300_ACCEL_CHANNEL(SCA3300_ACC_X, 0x1, X),
> +	SCA3300_ACCEL_CHANNEL(SCA3300_ACC_Y, 0x2, Y),
> +	SCA3300_ACCEL_CHANNEL(SCA3300_ACC_Z, 0x3, Z),
> +	SCA3300_TEMP_CHANNEL(SCA3300_TEMP, 0x05),
> +	IIO_CHAN_SOFT_TIMESTAMP(SCA3300_TIMESTAMP),
> +	SCA3300_INCLI_CHANNEL(SCA3300_INCLI_X, 0x09, X),
> +	SCA3300_INCLI_CHANNEL(SCA3300_INCLI_Y, 0x0A, Y),
> +	SCA3300_INCLI_CHANNEL(SCA3300_INCLI_Z, 0x0B, Z),
> +};
> +
>  static const unsigned long sca3300_scan_masks[] = {
>  	BIT(SCA3300_ACC_X) | BIT(SCA3300_ACC_Y) | BIT(SCA3300_ACC_Z) |
>  	BIT(SCA3300_TEMP),
>  	0
>  };
>  
> +static const unsigned long scl3300_scan_masks[] = {
> +	BIT(SCA3300_ACC_X) | BIT(SCA3300_ACC_Y) | BIT(SCA3300_ACC_Z) |
> +	BIT(SCA3300_TEMP) |
> +	BIT(SCA3300_INCLI_X) | BIT(SCA3300_INCLI_Y) | BIT(SCA3300_INCLI_Z),
> +	0
> +};
> +
>  struct sca3300_chip_info {
>  	const struct iio_chan_spec *channels;
>  	const int (*accel_scale_table)[2];
> +	const int (*incli_scale_table)[2];
>  	const int *accel_scale_modes_map;
> +	const int *incli_scale_modes_map;
>  	const unsigned long *scan_masks;
>  	const int *avail_modes_table;
>  	const int *freq_modes_map;
>  	const int *freq_table;
>  	const u8 num_accel_scales;
> +	const u8 num_incli_scales;
>  	const u8 num_avail_modes;
>  	const u8 num_channels;
>  	const u8 num_freqs;
>  	const u8 chip_id;
>  	const char *name;
> +	const bool angle;
>  };
>  
>  /**
> @@ -156,24 +206,32 @@ static const struct sca3300_chip_info sca3300_chip_tbl[] = {
>  		.freq_table = sca3300_freq_tbl,
>  		.scan_masks = sca3300_scan_masks,
>  		.channels = sca3300_channels,
> +		.incli_scale_modes_map = NULL,
> +		.incli_scale_table = NULL,
> +		.num_incli_scales = 0,
>  		.num_avail_modes = 4,
>  		.name = "sca3300",
>  		.chip_id = 0x51,
>  		.num_freqs = 2,
> +		.angle = false,
>  	},
>  	{	.num_accel_scales = ARRAY_SIZE(scl3300_accel_scale_tbl)*2,
> +		.num_incli_scales = ARRAY_SIZE(scl3300_incli_scale_tbl)*2,
>  		.accel_scale_modes_map = scl3300_accel_scale_modes_map,
> +		.incli_scale_modes_map = scl3300_incli_scale_modes_map,
>  		.accel_scale_table = scl3300_accel_scale_tbl,
> -		.num_channels = ARRAY_SIZE(sca3300_channels),
> +		.incli_scale_table = scl3300_incli_scale_tbl,
> +		.num_channels = ARRAY_SIZE(scl3300_channels),
>  		.avail_modes_table = scl3300_avail_modes_map,
>  		.freq_modes_map = scl3300_freq_modes_map,
>  		.scan_masks = sca3300_scan_masks,
>  		.freq_table = scl3300_freq_tbl,
> -		.channels = sca3300_channels,
> +		.channels = scl3300_channels,
>  		.num_avail_modes = 3,
>  		.name = "scl3300",
>  		.chip_id = 0xC1,
>  		.num_freqs = 3,
> +		.angle = true,
>  	},
>  };
>  
> @@ -382,6 +440,11 @@ static int sca3300_read_raw(struct iio_dev *indio_dev,
>  		if (ret)
>  			return ret;
>  		switch (chan->type) {
> +		case IIO_INCLI:
> +			index = data->chip->incli_scale_modes_map[reg_val];
> +			*val = data->chip->incli_scale_table[index][0];
> +			*val2 = data->chip->incli_scale_table[index][1];
> +			return IIO_VAL_INT_PLUS_MICRO;
>  		case IIO_ACCEL:
>  			index = data->chip->accel_scale_modes_map[reg_val];
>  			*val = data->chip->accel_scale_table[index][0];
> @@ -473,6 +536,13 @@ static int sca3300_init(struct sca3300_data *sca_data,
>  	indio_dev->name = sca3300_chip_tbl[i].name;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
> +	if (sca_data->chip->angle) {
> +		ret = sca3300_write_reg(sca_data, SCA3300_REG_ANG_CTRL,
> +					SCA3300_ANG_ENABLE);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -508,6 +578,11 @@ static int sca3300_read_avail(struct iio_dev *indio_dev,
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SCALE:
>  		switch (chan->type) {
> +		case IIO_INCLI:
> +			*vals = (const int *)data->chip->incli_scale_table;
> +			*length = data->chip->num_incli_scales;
> +			*type = IIO_VAL_INT_PLUS_MICRO;
> +			return IIO_AVAIL_LIST;
>  		case IIO_ACCEL:
>  			*vals = (const int *)data->chip->accel_scale_table;
>  			*length = data->chip->num_accel_scales;