Re: [PATCH V1 3/6] iio: accel: sca3300: modified to support multi chips

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

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, &reg_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