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