linux-kernel - Re: [PATCH V5 3/5] 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:   Thu, 12 May 2022 19:31:11 +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>,
        linux-iio <linux-iio@...r.kernel.org>
Subject: Re: [PATCH V5 3/5] iio: accel: sca3300: modified to support multi chips

On Thu, May 12, 2022 at 6:03 PM LI Qingwu
<Qing-wu.Li@...ca-geosystems.com.cn> wrote:
>
> The driver supports sca3300 only, there are some other similar chips,
> for instance, SCL3300. This commit prepares the way for multiple chips
> and additional channels. Modify the driver to read the device ID and load
> the corresponding sensor information from the table to support multiple
> chips. add prepares for the addition of extra channels. Add prepares for
> handling the operation modes for multiple chips.

Reading it again I think you may format it better, i.e.

Prepare the way for multiple chips and additional channels:
- Modify the driver to read the device ID and load the corresponding
sensor information from the table to support multiple chips
- Add prepares for the addition of extra channels
- Prepare for handling the operation modes for multiple chips

...

> +struct sca3300_chip_info {
> +       const unsigned long *scan_masks;
> +       const struct iio_chan_spec *channels;
> +       u8 num_channels;
> +       u8 num_accel_scales;
> +       const int (*accel_scale)[2];
> +       const int *accel_scale_map;
> +       u8 num_freqs;
> +       const int *freq_table;
> +       const int *freq_map;

> +       const char *name;

You can put it in the first place.

> +       const int *avail_modes_table;
> +       u8 num_avail_modes;
> +       u8 chip_id;
> +};

...

> +static const struct sca3300_chip_info sca3300_chip_tbl[] = {
> +       {       .scan_masks = sca3300_scan_masks,

Keep { on a separate line.

> +               .channels = sca3300_channels,
> +               .num_channels = ARRAY_SIZE(sca3300_channels),
> +               .num_accel_scales = ARRAY_SIZE(sca3300_accel_scale)*2,
> +               .accel_scale = sca3300_accel_scale,
> +               .accel_scale_map = sca3300_accel_scale_map,
> +               .num_freqs = ARRAY_SIZE(sca3300_lp_freq),
> +               .freq_table = sca3300_lp_freq,
> +               .freq_map = sca3300_lp_freq_map,
> +               .name = "sca3300",
> +               .avail_modes_table = sca3300_avail_modes_map,
> +               .num_avail_modes = 4,
> +               .chip_id = SCA3300_WHOAMI_ID,
> +       },
> +};

...

> +       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&0x03)
> +                       break;
> +       }

> +

This blank line is not needed as I explained.

> +       if (i >= sca_data->chip->num_avail_modes)

== is enough and better to understand.

> +               return -EINVAL;
> +
> +       *index = i;
> +       return 0;
> +}

...

> +       int index;
> +       int i;

Both can be unsigned.

...

> +       for (i = 0; i < chip->num_avail_modes; i++) {
> +               if ((val == chip->freq_table[chip->freq_map[i]]) &&
> +                   (chip->accel_scale[chip->accel_scale_map[index]] ==
> +                    chip->accel_scale[chip->accel_scale_map[i]]))
> +                       break;
> +       }
> +
> +       if (i >= chip->num_avail_modes)
> +               return -EINVAL;

Two comments as per above for-loop case.

-- 
With Best Regards,
Andy Shevchenko