RE: [PATCH V3 3/5] iio: accel: sca3300: modified to support multi chips

[LI Qingwu]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Sent: Wednesday, May 4, 2022 10:20 PM
> To: LI Qingwu <Qing-wu.Li@xxxxxxxxxxxxxxxxxxxxxxx>
> Cc: Jonathan Cameron <jic23@xxxxxxxxxx>; Lars-Peter Clausen
> <lars@xxxxxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>; Tomas Melin
> <tomas.melin@xxxxxxxxxxx>; devicetree <devicetree@xxxxxxxxxxxxxxx>; Linux
> Kernel Mailing List <linux-kernel@xxxxxxxxxxxxxxx>; linux-iio
> <linux-iio@xxxxxxxxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>
> Subject: Re: [PATCH V3 3/5] iio: accel: sca3300: modified to support multi chips
> 
> This email is not from Hexagon’s Office 365 instance. Please be careful while
> clicking links, opening attachments, or replying to this email.
> 
> 
> On Wed, May 4, 2022 at 3:36 PM LI Qingwu
> <Qing-wu.Li@xxxxxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > The drive support sca3300 only.
> 
> driver supports
> 
> > 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.
> 
> It seems you need to work more on the commit messages in all patches.
Thanks, Andy. I rely on grammar checking but it didn't point out. 
Let me fix it in the next version.
> 
> ...
> 
> > +struct sca3300_chip_info {
> > +       const struct iio_chan_spec *channels;
> > +       const int (*accel_scale_table)[2];
> > +       const int *accel_scale_modes_map;
> > +       const unsigned long *scan_masks;
> > +       const int *avail_modes_table;
> > +       const int *freq_modes_map;
> > +       const int *freq_table;
> > +       const u8 num_accel_scales;
> > +       const u8 num_avail_modes;
> > +       const u8 num_channels;
> > +       const u8 num_freqs;
> > +       const u8 chip_id;
> 
> Why do you have const qualifier on all members?  The last one is
> understandable, but the rest, esp. pointers should be justified.
Because I thought it was static and has fix value for each chip, unacceptable for you?
> 
> > +       const char *name;
> > +};
> 
> ...
> 
> > +static int sca3300_set_op_mode(struct sca3300_data *sca_data, int
> > +index) {
> > +       int mode;
> > +
> > +       if ((index < 0) || (index >= sca_data->chip->num_avail_modes))
> 
> Too many parentheses.
> 
> > +               return -EINVAL;
> > +
> > +       mode = sca_data->chip->avail_modes_table[index];
> > +
> > +       return sca3300_write_reg(sca_data, SCA3300_REG_MODE,
> mode); }
> 
> --
> With Best Regards,
> Andy Shevchenko