On Thu, 17 Feb 2022 06:27:04 +0000
LI Qingwu <Qing-wu.Li@xxxxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxx>
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, ®_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);
