Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220704003337.208696-1-ang.iglesiasg@gmail.com>
In-Reply-To: <20220704003337.208696-1-ang.iglesiasg@gmail.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Mon, 4 Jul 2022 22:08:18 +0200
Message-ID: <CAHp75VdBv8BJVzBCMzWKpm0RrqX=K_QPQ4cgdshqXP3Uy+hVHQ@mail.gmail.com>
Subject: Re: [PATCH v2 5/5] iio: pressure: bmp280: Adds more tunable config
 parameters for BMP380
To:     Angel Iglesias <ang.iglesiasg@gmail.com>
Cc:     Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>,
        Paul Cercueil <paul@crapouillou.net>,
        Ulf Hansson <ulf.hansson@linaro.org>,
        linux-iio <linux-iio@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Jul 4, 2022 at 2:41 AM Angel Iglesias <ang.iglesiasg@gmail.com> 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?

...

> +               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.

> +                                */

...

> +                               /*
> +                                * 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;

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.

-- 
With Best Regards,
Andy Shevchenko