Re: [PATCH V5 3/5] iio: accel: sca3300: modified to support multi chips

From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
Cc: Jonathan Cameron <jic23@kernel.org>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Rob Herring <robh+dt@kernel.org>,
	Tomas Melin <tomas.melin@vaisala.com>,
	devicetree <devicetree@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-iio <linux-iio@vger.kernel.org>
Subject: Re: [PATCH V5 3/5] iio: accel: sca3300: modified to support multi chips
Date: Thu, 12 May 2022 19:31:11 +0200	[thread overview]
Message-ID: <CAHp75Vc=duGJ+Y4QHvTG4ZZej3JbYCbhfj+LKhiOcd-+-sU5aw@mail.gmail.com> (raw)
In-Reply-To: <20220512160312.3880433-4-Qing-wu.Li@leica-geosystems.com.cn>

On Thu, May 12, 2022 at 6:03 PM LI Qingwu
<Qing-wu.Li@leica-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