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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 24 Jan 2022 15:08:57 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     LI Qingwu <Qing-wu.Li@...ca-geosystems.com.cn>
Cc:     Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>,
        Tomas Melin <tomas.melin@...sala.com>,
        devicetree <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V1 3/6] iio: accel: sca3300: modified to support multi chips

On Mon, Jan 24, 2022 at 11:39 AM LI Qingwu
<Qing-wu.Li@...ca-geosystems.com.cn> 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