Re: [PATCH v2 5/5] iio: pressure: bmp280: Adds more tunable config parameters for BMP380

[Angel Iglesias]

Thank you for your comments!

On Mon, 2022-07-04 at 22:08 +0200, Andy Shevchenko wrote:
> On Mon, Jul 4, 2022 at 2:41 AM Angel Iglesias <ang.iglesiasg@xxxxxxxxx> wrote:
> > 
> > Allows to configure the IIR filter coefficient and the sampling frequency
> > The IIR filter coefficient is exposed using the sysfs attribute
> > "filter_low_pass_3db_frequency"
> 
> In all your commit messages, please pay attention to English grammar.
> Here you forgot all the periods.
> 
> ...
> 
> > +       BMP380_ODR_0_0015HZ
> 
> Keep a comma here.
> 
> ...
> 
> > +       /* BMP380 devices introduce sampling frequecy configuration. See
> 
> frequency.
> 
> > +        * datasheet sections 3.3.3. and 4.3.19.
> > +        *
> > +        * BMx280 devices allowed indirect configuration of sampling
> > frequency
> > +        * changing the t_standby duration between measurements. See
> > datasheet
> > +        * section 3.6.3
> > +        */
> 
> /*
>  * Multi-line comment style
>  * example. Use it.
>  */
> 
> ...
> 
> > +               if (unlikely(!data->chip_info->sampling_freq_avail)) {
> 
> Why unlikely() ? How does this improve code generation / performance?
> 
> ...

As Jonathan Cameron sugested on a previous version of the patch, even thought
this code should be safe (as if we are checking sampling frequency is because
the sensor is a BMP380 and has that property), it would be better to have a
sanity check just to be sure the property is really available. I used unlikely
macro to take into account that the property would be almost always initialized.

Now that you mention, probably this code won't be called too often to make the
"unlikely" branching hint make a meaningful performance difference

> > +               if (unlikely(!data->chip_info->iir_filter_coeffs_avail)) {
> 
> Ditto.
> 
> ...
> 
> > +                               /*
> > +                                * Error applying new configuration. Might
> > be
> > +                                * an invalid configuration, will try to
> > +                                * restore previous value just to be sure
> 
> Missed period. Please, check all your texts (commit messages,
> comments, etc) for proper English grammar.

Apologies, I'll be more careful before sending the revised patches next time

> 
> > +                                */
> 
> ...
> 
> > +                               /*
> > +                                * Error applying new configuration. Might
> > be
> > +                                * an invalid configuration, will try to
> > +                                * restore previous value just to be sure
> 
> Ditto.
> 
> > +                                */
> 
> ...
> 
> > +                               /*
> > +                                * Error applying new configuration. Might
> > be
> > +                                * an invalid configuration, will try to
> > +                                * restore previous value just to be sure
> 
> Ditto.
> 
> > +                                */
> 
> ...
> 
> > +                               /*
> > +                                * Error applying new configuration. Might
> > be
> > +                                * an invalid configuration, will try to
> > +                                * restore previous value just to be sure
> 
> Ditto.
> 
> > +                                */
> 
> ...
> 
> > +                               /*
> > +                                * Error applying new configuration. Might
> > be
> > +                                * an invalid configuration, will try to
> > +                                * restore previous value just to be sure
> 
> Ditto.
> 
> > +                                */
> 
> Why do you need to copy'n'paste dozens of the very same comment?
> Wouldn't it be enough to explain it somewhere at the top of the file
> or in the respective documentation (if it exists)?
> 
> ...
> 
> >         u8 osrs;
> >         unsigned int tmp;
> >         int ret;
> > +       bool change, aux;
> 
> Move them up, and probably use reversed xmas tree ordering ("longest
> line first" rule).
> 
> Also should be
>   bool change = false, aux;
> 
> ...
> 
> > +       change = change || aux;
> 
> change ||= aux;

I think I'm missing something, do you mean to use '|='?

> 
> And in other cases.
> 
> ...
> 
> > +               /* cycle sensor state machine to reset any measurement in
> > progress
> > +                * configuration errors are detected in a measurment cycle.
> 
> measurement
> 
> > +                * If the sampling frequency is too low, it is faster to
> > reset
> > +                * measurement cycle and restart measurements
> > +                */
> 
> Completely wrong comment style. Be consistent and follow the common
> standards in the Linux kernel.
> 
> ...
> 
> > +               /* wait before checking the configuration error flag.
> > +                * Worst case value for measure time indicated in the
> > datashhet
> 
> datasheet
> 
> > +                * in section 3.9.1 is used.
> > +                */
> 
> Ditto.
>