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

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

[LI Qingwu]

The current driver support sca3300 only.
Modifed for support SCL3300.
Verifed with SCL3300 on IMX8MM.
Splited the change for review.

Same as sca3300, scl3300 have 3-axis acceleration,and temperature.
Different with sca3300, it can output inclination data directly.
The change add the support with scl3300, support inclination data output.

Change in V2:
Drop the extra interface for set/get opration mode.
Drop the interface for enalbe/disable inclination output,
set inclination output is alwasy on.
Fix the findings in V1.

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                   | 271 ++++++++++++++----
 2 files changed, 222 insertions(+), 50 deletions(-)

-- 
2.25.1

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

[LI Qingwu]

Add DT bindings for Murata scl3300 inclinometer.

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 V2 2/5] iio: accel: sca3300: add define for temp channel for reuse.

[LI Qingwu]

Add define of SCA3300_TEMP_CHANNEL for reuse.

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 V2 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.
modified the driver to read the device id,
add the tables for the corresponding id to support multiple chips.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
---
 drivers/iio/accel/sca3300.c | 156 +++++++++++++++++++++++++++---------
 1 file changed, 118 insertions(+), 38 deletions(-)

diff --git a/drivers/iio/accel/sca3300.c b/drivers/iio/accel/sca3300.c
index ff16d2cc8c70..b1748874c02d 100644
--- a/drivers/iio/accel/sca3300.c
+++ b/drivers/iio/accel/sca3300.c
@@ -37,12 +37,24 @@
 
 /* Device ID */
 #define SCA3300_REG_WHOAMI	0x10
-#define SCA3300_WHOAMI_ID	0x51
 
 /* Device return status and mask */
 #define SCA3300_VALUE_RS_ERROR	0x3
 #define SCA3300_MASK_RS_STATUS	GENMASK(1, 0)
 
+enum sca3300_op_mode_indexes {
+	OP_MOD_1 = 0,
+	OP_MOD_2,
+	OP_MOD_3,
+	OP_MOD_4,
+	OP_MOD_CNT,
+};
+
+enum sca3300_chip_type {
+	CHIP_SCA3300 = 0,
+	CHIP_CNT
+};
+
 enum sca3300_scan_indexes {
 	SCA3300_ACC_X = 0,
 	SCA3300_ACC_Y,
@@ -91,15 +103,29 @@ static const struct iio_chan_spec sca3300_channels[] = {
 	SCA3300_ACCEL_CHANNEL(SCA3300_ACC_Z, 0x3, Z),
 	SCA3300_TEMP_CHANNEL(SCA3300_TEMP, 0x05),
 	IIO_CHAN_SOFT_TIMESTAMP(4),
+	IIO_CHAN_SOFT_TIMESTAMP(SCA3300_TIMESTAMP),
 };
 
-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_lp_freq_table[] = {70, 70, 70, 10};
+static const int sca3300_accel_scale_table[][2] = {{0, 370}, {0, 741}, {0, 185}, {0, 185}};
 
 static const unsigned long sca3300_scan_masks[] = {
 	BIT(SCA3300_ACC_X) | BIT(SCA3300_ACC_Y) | BIT(SCA3300_ACC_Z) |
 	BIT(SCA3300_TEMP),
-	0
+	0,
+};
+
+struct sca3300_chip_info {
+	const struct iio_chan_spec *channels;
+	enum sca3300_chip_type chip_type;
+	const int (*accel_scale_table)[2];
+	unsigned int num_accel_scales;
+	unsigned long scan_masks;
+	unsigned int num_freqs;
+	const int *freq_table;
+	const char *name;
+	int num_channels;
+	u8 chip_id;
 };
 
 /**
@@ -114,13 +140,29 @@ struct sca3300_data {
 	struct spi_device *spi;
 	struct mutex lock;
 	struct {
-		s16 channels[4];
+		s16 channels[SCA3300_TIMESTAMP - 1];
 		s64 ts __aligned(sizeof(s64));
 	} scan;
+	const struct sca3300_chip_info *chip_info;
 	u8 txbuf[4] ____cacheline_aligned;
 	u8 rxbuf[4];
 };
 
+static const struct sca3300_chip_info sca3300_chip_info_tbl[] = {
+	[CHIP_SCA3300] = {
+		.num_accel_scales = ARRAY_SIZE(sca3300_accel_scale_table)*2-1,
+		.accel_scale_table = sca3300_accel_scale_table,
+		.num_channels = ARRAY_SIZE(sca3300_channels),
+		.freq_table = &sca3300_lp_freq_table[2],
+		.scan_masks = sca3300_scan_masks,
+		.channels = sca3300_channels,
+		.chip_type = CHIP_SCA3300,
+		.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 +269,57 @@ 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, unsigned int mode)
+{
+	if ((mode < OP_MOD_1) || (mode >= OP_MOD_CNT))
+		return -EINVAL;
+
+	return sca3300_write_reg(sca_data, SCA3300_REG_MODE, mode);
+}
+
 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 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 < OP_MOD_CNT; i++) {
+			if ((val == data->chip_info->accel_scale_table[i][0]) &&
+			(val2 == data->chip_info->accel_scale_table[i][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);
 		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;
+		for (i = 0; i < OP_MOD_CNT; i++) {
+			if (val == data->chip_info->freq_table[i]) {
+				mode = i;
+				break;
+			}
+		}
+		switch (data->chip_info->chip_type) {
+		case CHIP_SCA3300:
+			/* SCA330 freq. change is possible only for mode 3 and 4 */
+			if (reg_val == OP_MOD_3 && mode == OP_MOD_4)
+				return sca3300_set_op_mode(data, mode);
+			if (reg_val == OP_MOD_4 && mode == OP_MOD_3)
+				return sca3300_set_op_mode(data, mode);
+			return -EINVAL;
+		default:
+			return -EINVAL;
+		}
 	default:
 		return -EINVAL;
 	}
@@ -267,8 +330,8 @@ 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 ret;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
@@ -280,14 +343,18 @@ static int sca3300_read_raw(struct iio_dev *indio_dev,
 		ret = sca3300_read_reg(data, SCA3300_REG_MODE, &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:
+			*val =  data->chip_info->accel_scale_table[reg_val][0];
+			*val2 = data->chip_info->accel_scale_table[reg_val][1];
+			return IIO_VAL_INT_PLUS_MICRO;
+		}
+		return -EINVAL;
 	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
 		ret = sca3300_read_reg(data, SCA3300_REG_MODE, &reg_val);
 		if (ret)
 			return ret;
-		*val = sca3300_lp_freq[reg_val];
+		*val = data->chip_info->freq_table[reg_val];
 		return IIO_VAL_INT;
 	default:
 		return -EINVAL;
@@ -330,6 +397,7 @@ static int sca3300_init(struct sca3300_data *sca_data,
 			struct iio_dev *indio_dev)
 {
 	int value = 0;
+	int i = 0;
 	int ret;
 
 	ret = sca3300_write_reg(sca_data, SCA3300_REG_MODE,
@@ -347,12 +415,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_info_tbl); i++) {
+		if (sca3300_chip_info_tbl[i].chip_id == value)
+			break;
+	}
+	if (i == ARRAY_SIZE(sca3300_chip_info_tbl)) {
+		dev_err(&sca_data->spi->dev, "Invalid chip %x\n", value);
 		return -ENODEV;
 	}
+
+	indio_dev->available_scan_masks = sca3300_chip_info_tbl[i].scan_masks;
+	indio_dev->num_channels = sca3300_chip_info_tbl[i].num_channels;
+	indio_dev->channels = sca3300_chip_info_tbl[i].channels;
+	sca_data->chip_info = &sca3300_chip_info_tbl[i];
+	indio_dev->name = sca3300_chip_info_tbl[i].name;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
 	return 0;
 }
 
@@ -384,15 +462,22 @@ 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_info->accel_scale_table;
+			*length = data->chip_info->num_accel_scales;
+			*type = IIO_VAL_INT_PLUS_MICRO;
+			return IIO_AVAIL_LIST;
+		default:
+			return -EINVAL;
+		}
+		return -EINVAL;
 	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
-		*vals = &sca3300_lp_freq[2];
-		*length = 2;
+		*vals = (const int *)data->chip_info->freq_table;
+		*length = data->chip_info->num_freqs;
 		*type = IIO_VAL_INT;
 		return IIO_AVAIL_LIST;
 	default:
@@ -424,11 +509,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

[PATCH V2 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 accelerometer and temperature output.
Datasheet:
www.murata.com/en-us/products/sensor/inclinometer/overview/lineup/scl3300

Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
---
 drivers/iio/accel/sca3300.c | 45 +++++++++++++++++++++++++++++++++++--
 1 file changed, 43 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/accel/sca3300.c b/drivers/iio/accel/sca3300.c
index b1748874c02d..123ab9a784d2 100644
--- a/drivers/iio/accel/sca3300.c
+++ b/drivers/iio/accel/sca3300.c
@@ -42,6 +42,9 @@
 #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_op_mode_indexes {
 	OP_MOD_1 = 0,
 	OP_MOD_2,
@@ -52,6 +55,7 @@ enum sca3300_op_mode_indexes {
 
 enum sca3300_chip_type {
 	CHIP_SCA3300 = 0,
+	CHIP_SCL3300,
 	CHIP_CNT
 };
 
@@ -106,8 +110,19 @@ static const struct iio_chan_spec sca3300_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(SCA3300_TIMESTAMP),
 };
 
+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),
+};
+
 static const int sca3300_lp_freq_table[] = {70, 70, 70, 10};
+static const int scl3300_lp_freq_table[] = {40, 70, 10, 10};
+
 static const int sca3300_accel_scale_table[][2] = {{0, 370}, {0, 741}, {0, 185}, {0, 185}};
+static const int scl3300_accel_scale_table[][2] = {{0, 167}, {0, 333}, {0, 83}, {0, 83}};
 
 static const unsigned long sca3300_scan_masks[] = {
 	BIT(SCA3300_ACC_X) | BIT(SCA3300_ACC_Y) | BIT(SCA3300_ACC_Z) |
@@ -161,6 +176,18 @@ static const struct sca3300_chip_info sca3300_chip_info_tbl[] = {
 		.chip_id = 0x51,
 		.num_freqs = 2,
 	},
+	[CHIP_SCL3300] = {
+		.num_accel_scales = ARRAY_SIZE(scl3300_accel_scale_table)*2-1,
+		.num_channels = ARRAY_SIZE(scl3300_channels),
+		.accel_scale_table = scl3300_accel_scale_table,
+		.freq_table = scl3300_lp_freq_table,
+		.scan_masks = sca3300_scan_masks,
+		.channels = scl3300_channels,
+		.chip_type = CHIP_SCL3300,
+		.name = "scl3300",
+		.chip_id = 0xC1,
+		.num_freqs = 3,
+	},
 };
 
 DECLARE_CRC8_TABLE(sca3300_crc_table);
@@ -274,6 +301,11 @@ static int sca3300_set_op_mode(struct sca3300_data *sca_data, unsigned int mode)
 	if ((mode < OP_MOD_1) || (mode >= OP_MOD_CNT))
 		return -EINVAL;
 
+	/* SCL3300, mode 3 and 4 are same, use mode 4 only*/
+	if (sca_data->chip_info->chip_type == CHIP_SCL3300) {
+		if (mode == OP_MOD_3)
+			mode = OP_MOD_4;
+	}
 	return sca3300_write_reg(sca_data, SCA3300_REG_MODE, mode);
 }
 
@@ -317,6 +349,8 @@ static int sca3300_write_raw(struct iio_dev *indio_dev,
 			if (reg_val == OP_MOD_4 && mode == OP_MOD_3)
 				return sca3300_set_op_mode(data, mode);
 			return -EINVAL;
+		case CHIP_SCL3300:
+			return sca3300_set_op_mode(data, mode);
 		default:
 			return -EINVAL;
 		}
@@ -348,8 +382,9 @@ static int sca3300_read_raw(struct iio_dev *indio_dev,
 			*val =  data->chip_info->accel_scale_table[reg_val][0];
 			*val2 = data->chip_info->accel_scale_table[reg_val][1];
 			return IIO_VAL_INT_PLUS_MICRO;
+		default:
+			return -EINVAL;
 		}
-		return -EINVAL;
 	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
 		ret = sca3300_read_reg(data, SCA3300_REG_MODE, &reg_val);
 		if (ret)
@@ -431,6 +466,12 @@ static int sca3300_init(struct sca3300_data *sca_data,
 	indio_dev->name = sca3300_chip_info_tbl[i].name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
+	if (sca_data->chip_info->chip_type == CHIP_SCL3300) {
+		ret = sca3300_write_reg(sca_data, SCA3300_REG_ANG_CTRL, SCA3300_ANG_ENABLE);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
@@ -474,7 +515,6 @@ static int sca3300_read_avail(struct iio_dev *indio_dev,
 		default:
 			return -EINVAL;
 		}
-		return -EINVAL;
 	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
 		*vals = (const int *)data->chip_info->freq_table;
 		*length = data->chip_info->num_freqs;
@@ -536,6 +576,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 V2 5/5] iio: accel: sca3300: Add inclination channels.

[LI Qingwu]

Different with 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 add 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

Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
---
 drivers/iio/accel/sca3300.c | 48 +++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/iio/accel/sca3300.c b/drivers/iio/accel/sca3300.c
index 123ab9a784d2..5c86b143d17a 100644
--- a/drivers/iio/accel/sca3300.c
+++ b/drivers/iio/accel/sca3300.c
@@ -64,6 +64,9 @@ enum sca3300_scan_indexes {
 	SCA3300_ACC_Y,
 	SCA3300_ACC_Z,
 	SCA3300_TEMP,
+	SCA3300_INCLI_X,
+	SCA3300_INCLI_Y,
+	SCA3300_INCLI_Z,
 	SCA3300_TIMESTAMP,
 };
 
@@ -88,6 +91,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,						\
@@ -115,6 +136,9 @@ static const struct iio_chan_spec scl3300_channels[] = {
 	SCA3300_ACCEL_CHANNEL(SCA3300_ACC_Y, 0x2, Y),
 	SCA3300_ACCEL_CHANNEL(SCA3300_ACC_Z, 0x3, Z),
 	SCA3300_TEMP_CHANNEL(SCA3300_TEMP, 0x05),
+	SCA3300_INCLI_CHANNEL(SCA3300_INCLI_X, 0x09, X),
+	SCA3300_INCLI_CHANNEL(SCA3300_INCLI_Y, 0x0A, Y),
+	SCA3300_INCLI_CHANNEL(SCA3300_INCLI_Z, 0x0B, Z),
 	IIO_CHAN_SOFT_TIMESTAMP(SCA3300_TIMESTAMP),
 };
 
@@ -124,17 +148,28 @@ static const int scl3300_lp_freq_table[] = {40, 70, 10, 10};
 static const int sca3300_accel_scale_table[][2] = {{0, 370}, {0, 741}, {0, 185}, {0, 185}};
 static const int scl3300_accel_scale_table[][2] = {{0, 167}, {0, 333}, {0, 83}, {0, 83}};
 
+static const int scl3300_incli_scale_table[][2] = {{0, 5495}, {0, 5495}, {0, 5495}, {0, 5495}};
+
 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;
 	enum sca3300_chip_type chip_type;
 	const int (*accel_scale_table)[2];
+	const int (*incli_scale_table)[2];
 	unsigned int num_accel_scales;
+	unsigned int num_incli_scales;
 	unsigned long scan_masks;
 	unsigned int num_freqs;
 	const int *freq_table;
@@ -172,14 +207,18 @@ static const struct sca3300_chip_info sca3300_chip_info_tbl[] = {
 		.scan_masks = sca3300_scan_masks,
 		.channels = sca3300_channels,
 		.chip_type = CHIP_SCA3300,
+		.incli_scale_table = NULL,
+		.num_incli_scales = 0,
 		.name = "sca3300",
 		.chip_id = 0x51,
 		.num_freqs = 2,
 	},
 	[CHIP_SCL3300] = {
 		.num_accel_scales = ARRAY_SIZE(scl3300_accel_scale_table)*2-1,
+		.num_incli_scales = ARRAY_SIZE(scl3300_lp_freq_table)-1,
 		.num_channels = ARRAY_SIZE(scl3300_channels),
 		.accel_scale_table = scl3300_accel_scale_table,
+		.incli_scale_table = scl3300_incli_scale_table,
 		.freq_table = scl3300_lp_freq_table,
 		.scan_masks = sca3300_scan_masks,
 		.channels = scl3300_channels,
@@ -378,6 +417,10 @@ static int sca3300_read_raw(struct iio_dev *indio_dev,
 		if (ret)
 			return ret;
 		switch (chan->type) {
+		case IIO_INCLI:
+			*val =  data->chip_info->incli_scale_table[reg_val][0];
+			*val2 = data->chip_info->incli_scale_table[reg_val][1];
+			return IIO_VAL_INT_PLUS_MICRO;
 		case IIO_ACCEL:
 			*val =  data->chip_info->accel_scale_table[reg_val][0];
 			*val2 = data->chip_info->accel_scale_table[reg_val][1];
@@ -507,6 +550,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_info->incli_scale_table;
+			*length = data->chip_info->num_incli_scales;
+			*type = IIO_VAL_INT_PLUS_MICRO;
+			return IIO_AVAIL_LIST;
 		case IIO_ACCEL:
 			*vals = (const int *)data->chip_info->accel_scale_table;
 			*length = data->chip_info->num_accel_scales;
-- 
2.25.1

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

[Rob Herring]

On Thu, 17 Feb 2022 06:27:01 +0000, LI Qingwu wrote:
> Add DT bindings for Murata scl3300 inclinometer.
> 
> 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(+)
> 


Please add Acked-by/Reviewed-by tags when posting new versions. However,
there's no need to repost patches *only* to add the tags. The upstream
maintainer will do that for acks received on the version they apply.

If a tag was not added on purpose, please state why and what changed.

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

[LI Qingwu]

Hi Rob Herring




> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Friday, February 18, 2022 7:04 AM
> To: LI Qingwu <qing-wu.li@leica-geosystems.com.cn>
> Cc: robh+dt@kernel.org; devicetree@vger.kernel.org; linux-iio@vger.kernel.org;
> jic23@kernel.org; tomas.melin@vaisala.com; andy.shevchenko@gmail.com;
> lars@metafoo.de; GEO-CHHER-bsp-development
> <bsp-development.geo@leica-geosystems.com>; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH V2 1/5] dt-bindings: iio: accel: sca3300: Document
> murata,scl3300
> 
> [Some people who received this message don't often get email from
> robh@kernel.org. Learn why this is important at
> http://aka.ms/LearnAboutSenderIdentification.]
> 
> This email is not from Hexagon's Office 365 instance. Please be careful while
> clicking links, opening attachments, or replying to this email.
> 
> 
> On Thu, 17 Feb 2022 06:27:01 +0000, LI Qingwu wrote:
> > Add DT bindings for Murata scl3300 inclinometer.
> >
> > 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(+)
> >
> 
> 
> Please add Acked-by/Reviewed-by tags when posting new versions. However,.
> there's no need to repost patches *only* to add the tags. The upstream
> maintainer will do that for acks received on the version they apply.
> 
> If a tag was not added on purpose, please state why and what changed.

Thanks, I will add in v3.

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

[Andy Shevchenko]

On Thu, Feb 17, 2022 at 7:27 AM LI Qingwu
<Qing-wu.Li@leica-geosystems.com.cn> wrote:
>
> The drive support sca3300 only,there are some other similar chips,
> for instance, SCL3300.
> modified the driver to read the device id,
> add the tables for the corresponding id to support multiple chips.

> Reported-by: kernel test robot <lkp@intel.com>

New features may not be reported. What does the above mean?

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

On Thu, 17 Feb 2022 06:27:00 +0000
LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn> wrote:

> The current driver support sca3300 only.
> Modifed for support SCL3300.
> Verifed with SCL3300 on IMX8MM.
> Splited the change for review.
> 
> Same as sca3300, scl3300 have 3-axis acceleration,and temperature.
> Different with sca3300, it can output inclination data directly.
> The change add the support with scl3300, support inclination data output.
> 
> Change in V2:
> Drop the extra interface for set/get opration mode.
> Drop the interface for enalbe/disable inclination output,
> set inclination output is alwasy on.
> Fix the findings in V1.

Please run a spell checker on cover letters / patch descriptions.

Though as people who follow IIO patches will probably note, I
often forget myself and have sent some completely unreadable
messages as a result!  One of those do as I say, not as I do
requests :)

Jonathan

> 
> 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                   | 271 ++++++++++++++----
>  2 files changed, 222 insertions(+), 50 deletions(-)
> 

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

[Jonathan Cameron]

On Thu, 17 Feb 2022 06:27:03 +0000
LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn> wrote:

Hi,

A few comments inline.

Thanks,
Jonathan


> The drive support sca3300 only,there are some other similar chips,

space after only,

> for instance, SCL3300.
> modified the driver to read the device id,
> add the tables for the corresponding id to support multiple chips.
> 
> Reported-by: kernel test robot <lkp@intel.com>

I guess something was reported in v1.  When you have fixed a patch
make sure you say what was reported. It is also common to credit
fixes before merging just in the change log rather than with an explicit
entry like tis.

> Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
> ---
>  drivers/iio/accel/sca3300.c | 156 +++++++++++++++++++++++++++---------
>  1 file changed, 118 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/iio/accel/sca3300.c b/drivers/iio/accel/sca3300.c
> index ff16d2cc8c70..b1748874c02d 100644
> --- a/drivers/iio/accel/sca3300.c
> +++ b/drivers/iio/accel/sca3300.c
> @@ -37,12 +37,24 @@
>  
>  /* Device ID */
>  #define SCA3300_REG_WHOAMI	0x10
> -#define SCA3300_WHOAMI_ID	0x51
>  
>  /* Device return status and mask */
>  #define SCA3300_VALUE_RS_ERROR	0x3
>  #define SCA3300_MASK_RS_STATUS	GENMASK(1, 0)
>  
> +enum sca3300_op_mode_indexes {
> +	OP_MOD_1 = 0,
> +	OP_MOD_2,
> +	OP_MOD_3,
> +	OP_MOD_4,
> +	OP_MOD_CNT,
> +};
> +
> +enum sca3300_chip_type {
> +	CHIP_SCA3300 = 0,
> +	CHIP_CNT
> +};
> +
>  enum sca3300_scan_indexes {
>  	SCA3300_ACC_X = 0,
>  	SCA3300_ACC_Y,
> @@ -91,15 +103,29 @@ static const struct iio_chan_spec sca3300_channels[] = {
>  	SCA3300_ACCEL_CHANNEL(SCA3300_ACC_Z, 0x3, Z),
>  	SCA3300_TEMP_CHANNEL(SCA3300_TEMP, 0x05),
>  	IIO_CHAN_SOFT_TIMESTAMP(4),
> +	IIO_CHAN_SOFT_TIMESTAMP(SCA3300_TIMESTAMP),
>  };
>  
> -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_lp_freq_table[] = {70, 70, 70, 10};
> +static const int sca3300_accel_scale_table[][2] = {{0, 370}, {0, 741}, {0, 185}, {0, 185}};
>  
>  static const unsigned long sca3300_scan_masks[] = {
>  	BIT(SCA3300_ACC_X) | BIT(SCA3300_ACC_Y) | BIT(SCA3300_ACC_Z) |
>  	BIT(SCA3300_TEMP),
> -	0
> +	0,

Unrelated change. Also last value can't be less bits set than 0 so this
is effectively a terminator and doesn't need the , (though I don't mind if there
is one as that reasoning is rather obscure!)

> +};
> +
> +struct sca3300_chip_info {
> +	const struct iio_chan_spec *channels;
> +	enum sca3300_chip_type chip_type;
> +	const int (*accel_scale_table)[2];
> +	unsigned int num_accel_scales;
> +	unsigned long scan_masks;
> +	unsigned int num_freqs;
> +	const int *freq_table;
> +	const char *name;
> +	int num_channels;
> +	u8 chip_id;
>  };
>  
>  /**
> @@ -114,13 +140,29 @@ struct sca3300_data {
>  	struct spi_device *spi;
>  	struct mutex lock;
>  	struct {
> -		s16 channels[4];
> +		s16 channels[SCA3300_TIMESTAMP - 1];

Why reduction is size?  The use of the enum -1 is
also confusing as it's not clear why that would be the
case so just use a number and add a comment if useful
on which channels it is.  

>  		s64 ts __aligned(sizeof(s64));
>  	} scan;
> +	const struct sca3300_chip_info *chip_info;
>  	u8 txbuf[4] ____cacheline_aligned;
>  	u8 rxbuf[4];
>  };
>  
> +static const struct sca3300_chip_info sca3300_chip_info_tbl[] = {
> +	[CHIP_SCA3300] = {
> +		.num_accel_scales = ARRAY_SIZE(sca3300_accel_scale_table)*2-1,
> +		.accel_scale_table = sca3300_accel_scale_table,
> +		.num_channels = ARRAY_SIZE(sca3300_channels),
> +		.freq_table = &sca3300_lp_freq_table[2],
> +		.scan_masks = sca3300_scan_masks,
> +		.channels = sca3300_channels,
> +		.chip_type = CHIP_SCA3300,
> +		.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 +269,57 @@ 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, unsigned int mode)
> +{
> +	if ((mode < OP_MOD_1) || (mode >= OP_MOD_CNT))
> +		return -EINVAL;
> +
> +	return sca3300_write_reg(sca_data, SCA3300_REG_MODE, mode);
> +}
> +
>  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 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 < OP_MOD_CNT; i++) {
> +			if ((val == data->chip_info->accel_scale_table[i][0]) &&
> +			(val2 == data->chip_info->accel_scale_table[i][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);
>  		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;
> +		for (i = 0; i < OP_MOD_CNT; i++) {
> +			if (val == data->chip_info->freq_table[i]) {
> +				mode = i;
> +				break;
> +			}
> +		}
> +		switch (data->chip_info->chip_type) {
> +		case CHIP_SCA3300:

I would prefer if chip_info included a field that expressed this. 
Ideally we want to just 'pick' static data from the chip_info structure
rather than having code depending on the chip type.

Perhaps chip_info->freq_modes_bitmap (with a better name).


> +			/* SCA330 freq. change is possible only for mode 3 and 4 */
> +			if (reg_val == OP_MOD_3 && mode == OP_MOD_4)
> +				return sca3300_set_op_mode(data, mode);
> +			if (reg_val == OP_MOD_4 && mode == OP_MOD_3)
> +				return sca3300_set_op_mode(data, mode);
> +			return -EINVAL;
> +		default:
> +			return -EINVAL;
> +		}
>  	default:
>  		return -EINVAL;
>  	}
> @@ -267,8 +330,8 @@ 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 ret;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> @@ -280,14 +343,18 @@ static int sca3300_read_raw(struct iio_dev *indio_dev,
>  		ret = sca3300_read_reg(data, SCA3300_REG_MODE, &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:
> +			*val =  data->chip_info->accel_scale_table[reg_val][0];
> +			*val2 = data->chip_info->accel_scale_table[reg_val][1];
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		}
Prefer
		default:
			return -EINVAL;
in the case statement as done in other similar paths.

However, why is the channel type check needed? (see below for similar)
 
> +		return -EINVAL;
>  	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
>  		ret = sca3300_read_reg(data, SCA3300_REG_MODE, &reg_val);
>  		if (ret)
>  			return ret;
> -		*val = sca3300_lp_freq[reg_val];
> +		*val = data->chip_info->freq_table[reg_val];
>  		return IIO_VAL_INT;
>  	default:
>  		return -EINVAL;
> @@ -330,6 +397,7 @@ static int sca3300_init(struct sca3300_data *sca_data,
>  			struct iio_dev *indio_dev)
>  {
>  	int value = 0;
> +	int i = 0;

No need to initialize as will always be set by the for loop.

>  	int ret;
>  
>  	ret = sca3300_write_reg(sca_data, SCA3300_REG_MODE,
> @@ -347,12 +415,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_info_tbl); i++) {
> +		if (sca3300_chip_info_tbl[i].chip_id == value)
> +			break;
> +	}
> +	if (i == ARRAY_SIZE(sca3300_chip_info_tbl)) {
> +		dev_err(&sca_data->spi->dev, "Invalid chip %x\n", value);
>  		return -ENODEV;
>  	}
> +
> +	indio_dev->available_scan_masks = sca3300_chip_info_tbl[i].scan_masks;
> +	indio_dev->num_channels = sca3300_chip_info_tbl[i].num_channels;
> +	indio_dev->channels = sca3300_chip_info_tbl[i].channels;
> +	sca_data->chip_info = &sca3300_chip_info_tbl[i];
> +	indio_dev->name = sca3300_chip_info_tbl[i].name;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
>  	return 0;
>  }
>  
> @@ -384,15 +462,22 @@ 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:

Why the need for a check on the channel type?  I don't particularly
mind having one but I think it will always be IIO_ACCEL and that hasn't
changed as a result of this patch.  So to keep the patch minimal I would
prefer not adding the check unless it is necessary.


> +			*vals = (const int *)data->chip_info->accel_scale_table;
> +			*length = data->chip_info->num_accel_scales;
> +			*type = IIO_VAL_INT_PLUS_MICRO;
> +			return IIO_AVAIL_LIST;
> +		default:
> +			return -EINVAL;
> +		}
> +		return -EINVAL;
>  	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> -		*vals = &sca3300_lp_freq[2];
> -		*length = 2;
> +		*vals = (const int *)data->chip_info->freq_table;
> +		*length = data->chip_info->num_freqs;
>  		*type = IIO_VAL_INT;
>  		return IIO_AVAIL_LIST;
>  	default:
> @@ -424,11 +509,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 V2 4/5] iio: accel: sca3300: Add support for SCL3300

[Jonathan Cameron]

On Thu, 17 Feb 2022 06:27:04 +0000
LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn> wrote:

> Add support for Murata SCL3300, a 3-axis MEMS accelerometer.
> Same as SCA3300, it has accelerometer and temperature output.
> Datasheet:
> www.murata.com/en-us/products/sensor/inclinometer/overview/lineup/scl3300
> 
> Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>

Hi

Most comments are follow on from previous patch review.

Thanks,

Jonathan

> ---
>  drivers/iio/accel/sca3300.c | 45 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/accel/sca3300.c b/drivers/iio/accel/sca3300.c
> index b1748874c02d..123ab9a784d2 100644
> --- a/drivers/iio/accel/sca3300.c
> +++ b/drivers/iio/accel/sca3300.c
> @@ -42,6 +42,9 @@
>  #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_op_mode_indexes {
>  	OP_MOD_1 = 0,
>  	OP_MOD_2,
> @@ -52,6 +55,7 @@ enum sca3300_op_mode_indexes {
>  
>  enum sca3300_chip_type {
>  	CHIP_SCA3300 = 0,
> +	CHIP_SCL3300,
>  	CHIP_CNT
>  };
>  
> @@ -106,8 +110,19 @@ static const struct iio_chan_spec sca3300_channels[] = {
>  	IIO_CHAN_SOFT_TIMESTAMP(SCA3300_TIMESTAMP),
>  };
>  
> +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),
> +};
> +
>  static const int sca3300_lp_freq_table[] = {70, 70, 70, 10};
> +static const int scl3300_lp_freq_table[] = {40, 70, 10, 10};
> +
>  static const int sca3300_accel_scale_table[][2] = {{0, 370}, {0, 741}, {0, 185}, {0, 185}};
> +static const int scl3300_accel_scale_table[][2] = {{0, 167}, {0, 333}, {0, 83}, {0, 83}};

As below, given there is no obvious reason to support both mode 3 and 4 I would only
have values for one of them and have a different number of modes available for this device.

>  
>  static const unsigned long sca3300_scan_masks[] = {
>  	BIT(SCA3300_ACC_X) | BIT(SCA3300_ACC_Y) | BIT(SCA3300_ACC_Z) |
> @@ -161,6 +176,18 @@ static const struct sca3300_chip_info sca3300_chip_info_tbl[] = {
>  		.chip_id = 0x51,
>  		.num_freqs = 2,
>  	},
> +	[CHIP_SCL3300] = {
> +		.num_accel_scales = ARRAY_SIZE(scl3300_accel_scale_table)*2-1,
> +		.num_channels = ARRAY_SIZE(scl3300_channels),
> +		.accel_scale_table = scl3300_accel_scale_table,
> +		.freq_table = scl3300_lp_freq_table,
> +		.scan_masks = sca3300_scan_masks,
> +		.channels = scl3300_channels,
> +		.chip_type = CHIP_SCL3300,
> +		.name = "scl3300",
> +		.chip_id = 0xC1,
> +		.num_freqs = 3,
> +	},
>  };
>  
>  DECLARE_CRC8_TABLE(sca3300_crc_table);
> @@ -274,6 +301,11 @@ static int sca3300_set_op_mode(struct sca3300_data *sca_data, unsigned int mode)
>  	if ((mode < OP_MOD_1) || (mode >= OP_MOD_CNT))
>  		return -EINVAL;
>  
> +	/* SCL3300, mode 3 and 4 are same, use mode 4 only*/
> +	if (sca_data->chip_info->chip_type == CHIP_SCL3300) {

For this, the interface would be simpler if we just pretend mode 3 doesn't
exist and have only 3 modes (though obvious we'll still need to adjust it ehre).

> +		if (mode == OP_MOD_3)
> +			mode = OP_MOD_4;
> +	}
>  	return sca3300_write_reg(sca_data, SCA3300_REG_MODE, mode);
>  }
>  
> @@ -317,6 +349,8 @@ static int sca3300_write_raw(struct iio_dev *indio_dev,
>  			if (reg_val == OP_MOD_4 && mode == OP_MOD_3)
>  				return sca3300_set_op_mode(data, mode);
>  			return -EINVAL;
> +		case CHIP_SCL3300:
> +			return sca3300_set_op_mode(data, mode);

As mentioned in previous patch I'd like these choices to be expressed as
'features' or similar in the chip_info so that we don't end up with
the part differences exposed only via a specific 'type' field.

>  		default:
>  			return -EINVAL;
>  		}
> @@ -348,8 +382,9 @@ static int sca3300_read_raw(struct iio_dev *indio_dev,
>  			*val =  data->chip_info->accel_scale_table[reg_val][0];
>  			*val2 = data->chip_info->accel_scale_table[reg_val][1];
>  			return IIO_VAL_INT_PLUS_MICRO;
> +		default:
> +			return -EINVAL;
>  		}
> -		return -EINVAL;

Fix in earlier patch.

>  	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
>  		ret = sca3300_read_reg(data, SCA3300_REG_MODE, &reg_val);
>  		if (ret)
> @@ -431,6 +466,12 @@ static int sca3300_init(struct sca3300_data *sca_data,
>  	indio_dev->name = sca3300_chip_info_tbl[i].name;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
> +	if (sca_data->chip_info->chip_type == CHIP_SCL3300) {
As previously I'd ideally prefer to have the chip_info contain flags on which we make
a decision on a particular 'feature' rather than using a generic chip_type and
mixing where we have the information on the difference between devices.  I want
all that info in the chip_info structure.

	if (sca_data->chip_info->ang) {
		ret = sca3000_write_reg(sca_data, SCA330...
		...
	}
> +		ret = sca3300_write_reg(sca_data, SCA3300_REG_ANG_CTRL, SCA3300_ANG_ENABLE);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -474,7 +515,6 @@ static int sca3300_read_avail(struct iio_dev *indio_dev,
>  		default:
>  			return -EINVAL;
>  		}
> -		return -EINVAL;

This was introduced in previous patch. Please fix it there.


>  	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
>  		*vals = (const int *)data->chip_info->freq_table;
>  		*length = data->chip_info->num_freqs;
> @@ -536,6 +576,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);

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

[Jonathan Cameron]

On Thu, 17 Feb 2022 06:27:05 +0000
LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn> wrote:

> Different with 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 add output of the raw value,scale
> and scale_available of angles.

Hi,

As it came up in another review today, could you confirm
the units of incli after application of scale?  The units
of angles have unfortunately become a bit of a mess over time :(
See the BNO055 review for more discussion.

incli is documented as being in degrees so that should be the case here
as well.

Otherwise, just one comment inline which is really a request to add a little
more to the description of patch 3 to say it is preparing the way for additional
channels.

Looks good to me.
Thanks,

Jonathan


> 
> New interfaces:
>   in_incli_scale
>   in_incli_scale_available
>   in_incli_x_raw
>   in_incli_y_raw
>   in_incli_z_raw
> 
> Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
> ---
>  drivers/iio/accel/sca3300.c | 48 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/drivers/iio/accel/sca3300.c b/drivers/iio/accel/sca3300.c
> index 123ab9a784d2..5c86b143d17a 100644
> --- a/drivers/iio/accel/sca3300.c
> +++ b/drivers/iio/accel/sca3300.c
> @@ -64,6 +64,9 @@ enum sca3300_scan_indexes {
>  	SCA3300_ACC_Y,
>  	SCA3300_ACC_Z,
>  	SCA3300_TEMP,
> +	SCA3300_INCLI_X,
> +	SCA3300_INCLI_Y,
> +	SCA3300_INCLI_Z,
>  	SCA3300_TIMESTAMP,
>  };
>  
> @@ -88,6 +91,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,						\
> @@ -115,6 +136,9 @@ static const struct iio_chan_spec scl3300_channels[] = {
>  	SCA3300_ACCEL_CHANNEL(SCA3300_ACC_Y, 0x2, Y),
>  	SCA3300_ACCEL_CHANNEL(SCA3300_ACC_Z, 0x3, Z),
>  	SCA3300_TEMP_CHANNEL(SCA3300_TEMP, 0x05),
> +	SCA3300_INCLI_CHANNEL(SCA3300_INCLI_X, 0x09, X),
> +	SCA3300_INCLI_CHANNEL(SCA3300_INCLI_Y, 0x0A, Y),
> +	SCA3300_INCLI_CHANNEL(SCA3300_INCLI_Z, 0x0B, Z),
>  	IIO_CHAN_SOFT_TIMESTAMP(SCA3300_TIMESTAMP),
>  };
>  
> @@ -124,17 +148,28 @@ static const int scl3300_lp_freq_table[] = {40, 70, 10, 10};
>  static const int sca3300_accel_scale_table[][2] = {{0, 370}, {0, 741}, {0, 185}, {0, 185}};
>  static const int scl3300_accel_scale_table[][2] = {{0, 167}, {0, 333}, {0, 83}, {0, 83}};
>  
> +static const int scl3300_incli_scale_table[][2] = {{0, 5495}, {0, 5495}, {0, 5495}, {0, 5495}};
> +
>  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;
>  	enum sca3300_chip_type chip_type;
>  	const int (*accel_scale_table)[2];
> +	const int (*incli_scale_table)[2];
>  	unsigned int num_accel_scales;
> +	unsigned int num_incli_scales;
>  	unsigned long scan_masks;
>  	unsigned int num_freqs;
>  	const int *freq_table;
> @@ -172,14 +207,18 @@ static const struct sca3300_chip_info sca3300_chip_info_tbl[] = {
>  		.scan_masks = sca3300_scan_masks,
>  		.channels = sca3300_channels,
>  		.chip_type = CHIP_SCA3300,
> +		.incli_scale_table = NULL,
> +		.num_incli_scales = 0,
>  		.name = "sca3300",
>  		.chip_id = 0x51,
>  		.num_freqs = 2,
>  	},
>  	[CHIP_SCL3300] = {
>  		.num_accel_scales = ARRAY_SIZE(scl3300_accel_scale_table)*2-1,
> +		.num_incli_scales = ARRAY_SIZE(scl3300_lp_freq_table)-1,
>  		.num_channels = ARRAY_SIZE(scl3300_channels),
>  		.accel_scale_table = scl3300_accel_scale_table,
> +		.incli_scale_table = scl3300_incli_scale_table,
>  		.freq_table = scl3300_lp_freq_table,
>  		.scan_masks = sca3300_scan_masks,
>  		.channels = scl3300_channels,
> @@ -378,6 +417,10 @@ static int sca3300_read_raw(struct iio_dev *indio_dev,
>  		if (ret)
>  			return ret;
>  		switch (chan->type) {
> +		case IIO_INCLI:

Ah. This is why you had the switch in the earlier patch. Makes sense now!
Please add to the description of patch 3 that it also prepares for the addition of
extra channels.

> +			*val =  data->chip_info->incli_scale_table[reg_val][0];
> +			*val2 = data->chip_info->incli_scale_table[reg_val][1];
> +			return IIO_VAL_INT_PLUS_MICRO;
>  		case IIO_ACCEL:
>  			*val =  data->chip_info->accel_scale_table[reg_val][0];
>  			*val2 = data->chip_info->accel_scale_table[reg_val][1];
> @@ -507,6 +550,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_info->incli_scale_table;
> +			*length = data->chip_info->num_incli_scales;
> +			*type = IIO_VAL_INT_PLUS_MICRO;
> +			return IIO_AVAIL_LIST;
>  		case IIO_ACCEL:
>  			*vals = (const int *)data->chip_info->accel_scale_table;
>  			*length = data->chip_info->num_accel_scales;