On Mon, May 9, 2022 at 8:49 AM LI Qingwu
<Qing-wu.Li@xxxxxxxxxxxxxxxxxxxxxxx> wrote:
>
> The driver supports sca3300 only.
> There are some other similar chips, for instance, SCL3300.
> Prepare the way for multiple chips and additional channels.
> Modify the driver to read the device id.
> Add the tables for the corresponding id to support multiple chips.
> Add prepares for the addition of extra channels.
> Add prepares for handling the operation modes for multiple chips.
Fancy style. Utilize the entire width of the line, i.e. ~72 characters.
...
> +struct sca3300_chip_info {
> + const struct iio_chan_spec *channels;
> + u8 num_channels;
> + const unsigned long *scan_masks;
> + const int (*accel_scale)[2];
> + const int *accel_scale_map;
> + u8 num_accel_scales;
> + const int *freq_table;
> + const int *freq_map;
> + u8 num_freqs;
> + const int *avail_modes_table;
> + u8 num_avail_modes;
> + const char *name;
> + u8 chip_id;
I recommend to shuffle it in a way that u8 members go closer to each
other if possible.
Like:
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;
It will save a few bytes per object instance due to padding reduction.
The idea about grouping all of them also can be done despite the
comment given previously by someone.
> +};
...
> + for (i = 0; i < sca_data->chip->num_avail_modes; i++) {
> + if (sca_data->chip->avail_modes_table[i] == reg_val) {
> + *index = i;
> + return 0;
> + }
> + }
> +
> + return -EINVAL;
Can be modified to use standard pattern (return errors first), but
it's up to maintainers, because the latter requires an additional
check after the loop.
...
> + for (i = 0; i < chip->num_avail_modes; i++) {
> + if (val == chip->freq_table[chip->freq_map[i]]) {
> + if (chip->accel_scale[chip->accel_scale_map[index]] ==
> + chip->accel_scale[chip->accel_scale_map[i]])
> + return sca3300_set_op_mode(data, i);
The
if (a) {
if (b) {
...
}
}
can be replaced by
if (a && b) {
...
}
> + }
> + }
> +
> + return -EINVAL;
As per above.
--
With Best Regards,
Andy Shevchenko
