2022-05-09 10:23:54

by LI Qingwu

[permalink] [raw]
Subject: [PATCH V4 0/5] iio: accel: sca3300: add compatible for scl3300

The current driver support sca3300 only.
Modified 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 V4:
Remove Reviewed-by tags, that were not explicitly given.
Fix all the review comments from V3.
Sort variables logically instead of length in the struct
sca3300_chip_info declaration and assignment.
Fixed the warning "unused variable scl3300_scan_masks".
Reported-by: kernel test robot <[email protected]>.

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 | 307 +++++++++++++++---
2 files changed, 255 insertions(+), 53 deletions(-)

--
2.25.1



2022-05-09 10:23:55

by LI Qingwu

[permalink] [raw]
Subject: [PATCH V4 3/5] iio: accel: sca3300: modified to support multi chips

The driver supports 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.

Signed-off-by: LI Qingwu <[email protected]>
---
drivers/iio/accel/sca3300.c | 182 ++++++++++++++++++++++++++++--------
1 file changed, 141 insertions(+), 41 deletions(-)

diff --git a/drivers/iio/accel/sca3300.c b/drivers/iio/accel/sca3300.c
index ff16d2cc8c70..39e6f9f162f2 100644
--- a/drivers/iio/accel/sca3300.c
+++ b/drivers/iio/accel/sca3300.c
@@ -93,15 +93,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_lp_freq[] = {70, 10};
+static const int sca3300_lp_freq_map[] = {0, 0, 0, 1};

+static const int sca3300_accel_scale[][2] = {{0, 370}, {0, 741}, {0, 185}};
+static const int sca3300_accel_scale_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;
+ u8 num_channels;
+ const unsigned long *scan_masks;
+ const int (*accel_scale)[2];
+ const int *accel_scale_map;
+ u8 num_accel_scales;
+ const int *freq_table;
+ const int *freq_map;
+ u8 num_freqs;
+ const int *avail_modes_table;
+ u8 num_avail_modes;
+ const char *name;
+ u8 chip_id;
+};
+
/**
* struct sca3300_data - device data
* @spi: SPI device structure
@@ -117,10 +137,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[] = {
+ { .channels = sca3300_channels,
+ .num_channels = ARRAY_SIZE(sca3300_channels),
+ .scan_masks = sca3300_scan_masks,
+ .accel_scale = sca3300_accel_scale,
+ .accel_scale_map = sca3300_accel_scale_map,
+ .num_accel_scales = ARRAY_SIZE(sca3300_accel_scale)*2,
+ .freq_table = sca3300_lp_freq,
+ .freq_map = sca3300_lp_freq_map,
+ .num_freqs = ARRAY_SIZE(sca3300_lp_freq),
+ .avail_modes_table = sca3300_avail_modes_map,
+ .num_avail_modes = 4,
+ .name = "sca3300",
+ .chip_id = SCA3300_WHOAMI_ID,
+ },
+};
+
DECLARE_CRC8_TABLE(sca3300_crc_table);

static int sca3300_transfer(struct sca3300_data *sca_data, int *val)
@@ -227,36 +265,77 @@ 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)
+{
+ if ((index < 0) || (index >= sca_data->chip->num_avail_modes))
+ return -EINVAL;
+
+ return sca3300_write_reg(sca_data, SCA3300_REG_MODE,
+ sca_data->chip->avail_modes_table[index]);
+}
+
+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;
+ return 0;
+ }
+ }
+
+ return -EINVAL;
+}
+
+static int sca3300_set_frequency(struct sca3300_data *data, int val)
+{
+ const struct sca3300_chip_info *chip = data->chip;
+ int index;
+ int i;
+
+ if (sca3300_get_op_mode(data, &index))
+ return -EINVAL;
+
+ for (i = 0; i < chip->num_avail_modes; i++) {
+ if (val == chip->freq_table[chip->freq_map[i]]) {
+ if (chip->accel_scale[chip->accel_scale_map[index]] ==
+ chip->accel_scale[chip->accel_scale_map[i]])
+ return sca3300_set_op_mode(data, i);
+ }
+ }
+
+ return -EINVAL;
+}
+
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 reg_val;
- int ret;
+ int index;
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_map[i];
+ if ((val == data->chip->accel_scale[index][0]) &&
+ (val2 == data->chip->accel_scale[index][1])) {
+ return sca3300_set_op_mode(data, i);
+ }
}
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;
- /* 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;
+ return sca3300_set_frequency(data, val);
default:
return -EINVAL;
}
@@ -267,8 +346,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 index;
int ret;
- int reg_val;

switch (mask) {
case IIO_CHAN_INFO_RAW:
@@ -277,17 +356,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, &index);
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_map[index];
+ *val = data->chip->accel_scale[index][0];
+ *val2 = data->chip->accel_scale[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, &index);
if (ret)
return ret;
- *val = sca3300_lp_freq[reg_val];
+ index = data->chip->freq_map[index];
+ *val = data->chip->freq_table[index];
return IIO_VAL_INT;
default:
return -EINVAL;
@@ -331,6 +418,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 +435,17 @@ 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, "unknown chip id %x\n", value);
return -ENODEV;
}
+
+ sca_data->chip = &sca3300_chip_tbl[i];
+
return 0;
}

@@ -384,15 +477,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;
+ *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 +523,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) {
@@ -436,6 +530,12 @@ static int sca3300_probe(struct spi_device *spi)
return ret;
}

+ indio_dev->name = sca_data->chip->name;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+ indio_dev->channels = sca_data->chip->channels;
+ indio_dev->num_channels = sca_data->chip->num_channels;
+ indio_dev->available_scan_masks = sca_data->chip->scan_masks;
+
ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
iio_pollfunc_store_time,
sca3300_trigger_handler, NULL);
--
2.25.1


2022-05-09 10:49:25

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH V4 3/5] iio: accel: sca3300: modified to support multi chips

On Mon, May 9, 2022 at 8:49 AM LI Qingwu
<[email protected]> wrote:
>
> The driver supports 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.

Fancy style. Utilize the entire width of the line, i.e. ~72 characters.

...

> +struct sca3300_chip_info {
> + const struct iio_chan_spec *channels;
> + u8 num_channels;
> + const unsigned long *scan_masks;
> + const int (*accel_scale)[2];
> + const int *accel_scale_map;
> + u8 num_accel_scales;
> + const int *freq_table;
> + const int *freq_map;
> + u8 num_freqs;
> + const int *avail_modes_table;
> + u8 num_avail_modes;
> + const char *name;
> + u8 chip_id;

I recommend to shuffle it in a way that u8 members go closer to each
other if possible.

Like:

const unsigned long *scan_masks;
const struct iio_chan_spec *channels;
u8 num_channels;
u8 num_accel_scales;
const int (*accel_scale)[2];
const int *accel_scale_map;

It will save a few bytes per object instance due to padding reduction.

The idea about grouping all of them also can be done despite the
comment given previously by someone.

> +};

...

> + for (i = 0; i < sca_data->chip->num_avail_modes; i++) {
> + if (sca_data->chip->avail_modes_table[i] == reg_val) {
> + *index = i;
> + return 0;
> + }
> + }
> +
> + return -EINVAL;

Can be modified to use standard pattern (return errors first), but
it's up to maintainers, because the latter requires an additional
check after the loop.

...

> + for (i = 0; i < chip->num_avail_modes; i++) {
> + if (val == chip->freq_table[chip->freq_map[i]]) {
> + if (chip->accel_scale[chip->accel_scale_map[index]] ==
> + chip->accel_scale[chip->accel_scale_map[i]])
> + return sca3300_set_op_mode(data, i);

The
if (a) {
if (b) {
...
}
}

can be replaced by

if (a && b) {
...
}

> + }
> + }
> +
> + return -EINVAL;

As per above.

--
With Best Regards,
Andy Shevchenko