Messages in this thread | | | From | Andy Shevchenko <> | Date | Thu, 12 May 2022 19:31:11 +0200 | 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@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, ®_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
| |