Re: [PATCH V1 3/6] iio: accel: sca3300: modified to support multi chips
From: Andy Shevchenko
Date: Mon Jan 24 2022 - 08:10:42 EST
On Mon, Jan 24, 2022 at 11:39 AM LI Qingwu
<Qing-wu.Li@xxxxxxxxxxxxxxxxxxxxxxx> 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.
...
> /* 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,
Stray change ?
...
> +struct sca3300_chip_info {
> + enum sca3300_chip_type chip_type;
> + const char *name;
> + u8 chip_id;
> + const struct iio_chan_spec *channels;
> + int num_channels;
Ordering these as
const struct iio_chan_spec *channels;
int num_channels;
u8 chip_id;
will save 4 bytes in some cases.
> + unsigned long scan_masks;
> +};
...
> }
> return -EINVAL;
> -
> case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> ret = sca3300_read_reg(data, SCA3300_REG_MODE, ®_val);
Another stray change?
...
> int value = 0;
> int ret;
> + int i = 0;
Reversed xmas tree order?
...
> * Wait 15ms for settling of signal paths.
> */
> usleep_range(16e3, 50e3);
> -
Leave this blank line, so you will have a better reading of the code
and comment.
> + for (i = 0; i < ARRAY_SIZE(sca3300_chip_info_tbl); i++) {
> + ret = sca3300_read_reg(sca_data, SCA3300_REG_WHOAMI, &value);
> + if (ret)
> + return ret;
> + if (sca3300_chip_info_tbl[i].chip_id == value) {
> + sca_data->chip_info = &sca3300_chip_info_tbl[i];
> + indio_dev->name = sca3300_chip_info_tbl[i].name;
> + indio_dev->channels = sca3300_chip_info_tbl[i].channels;
> + indio_dev->num_channels = sca3300_chip_info_tbl[i].num_channels;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->available_scan_masks = sca3300_chip_info_tbl[i].scan_masks;
You may do it after the below check. Thus here
if (sca3300_chip_info_tbl[i].chip_id == value)
break;
> + break;
> + }
> + }
> + if (i == ARRAY_SIZE(sca3300_chip_info_tbl)) {
> + dev_err(&sca_data->spi->dev, "Invalid chip %x\n", value);
> return -ENODEV;
> }
> return 0;
...
> indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*sca_data));
> if (!indio_dev)
> return -ENOMEM;
> -
> sca_data = iio_priv(indio_dev);
> mutex_init(&sca_data->lock);
Stray change for sure.
...
> 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;
> +
Ditto.
>
> ret = sca3300_init(sca_data, indio_dev);
--
With Best Regards,
Andy Shevchenko