> -----Original Message-----
> From: Andy Shevchenko <[email protected]>
> Sent: Wednesday, May 4, 2022 10:20 PM
> To: LI Qingwu <[email protected]>
> Cc: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> <[email protected]>; Rob Herring <[email protected]>; Tomas Melin
> <[email protected]>; devicetree <[email protected]>; Linux
> Kernel Mailing List <[email protected]>; linux-iio
> <[email protected]>; Rob Herring <[email protected]>
> 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
> <[email protected]> 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
On Wed, May 4, 2022 at 4:35 PM LI Qingwu
<[email protected]> wrote:
> > From: Andy Shevchenko <[email protected]>
> > Sent: Wednesday, May 4, 2022 10:20 PM
> > On Wed, May 4, 2022 at 3:36 PM LI Qingwu
> > <[email protected]> wrote:
...
> > > +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?
But why const qualifier? What is the point of it for example for u8
members if the entire object is qualified as const below in the same
patch?
On top of that, please explain what in your opinion the "const ...
*foo" gives us, and what we will lose if we remove the "const" part
out of them.
> > > + const char *name;
> > > +};
--
With Best Regards,
Andy Shevchenko
> -----Original Message-----
> From: Andy Shevchenko <[email protected]>
> Sent: Wednesday, May 4, 2022 10:39 PM
> To: LI Qingwu <[email protected]>
> Cc: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> <[email protected]>; Rob Herring <[email protected]>; Tomas Melin
> <[email protected]>; devicetree <[email protected]>; Linux
> Kernel Mailing List <[email protected]>; linux-iio
> <[email protected]>; Rob Herring <[email protected]>
> 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 4:35 PM LI Qingwu
> <[email protected]> wrote:
> > > From: Andy Shevchenko <[email protected]>
> > > Sent: Wednesday, May 4, 2022 10:20 PM On Wed, May 4, 2022 at 3:36 PM
> > > LI Qingwu <[email protected]> wrote:
>
> ...
>
> > > > +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?
>
> But why const qualifier? What is the point of it for example for u8 members if
> the entire object is qualified as const below in the same patch?
>
> On top of that, please explain what in your opinion the "const ...
> *foo" gives us, and what we will lose if we remove the "const" part out of them.
Ah, you are right, those const are unnecessary for nonpointer members.
for the pointers, the contexts that the pointer points to are still writable.
what about if I remove all the const from nonpointer and keep it for the pointers?
Like：
const struct iio_chan_spec *channels;
const int (*accel_scale_table)[2];
const int (*incli_scale_table)[2];
const int *accel_scale_modes_map;
const int *incli_scale_modes_map;
const unsigned long *scan_masks;
const int *avail_modes_table;
const int *freq_modes_map;
const int *freq_table;
const char *name;
u8 num_accel_scales;
u8 num_incli_scales;
u8 num_avail_modes;
u8 num_channels;
u8 num_freqs;
u8 chip_id;
bool angle;
>
> > > > + const char *name;
> > > > +};
>
> --
> With Best Regards,
> Andy Shevchenko