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

From: Melin Tomas
Date: Fri May 06 2022 - 16:12:41 EST

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@xxxxxxxxxx>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Reviewed-by: Jonathan Cameron <jic23@xxxxxxxxxx>
> Signed-off-by: LI Qingwu <Qing-wu.Li@xxxxxxxxxxxxxxxxxxxxxxx>
> ---
> 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) {