| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHp75VdBv8BJVzBCMzWKpm0RrqX=K_QPQ4cgdshqXP3Uy+hVHQ@mail.gmail.com>
Date: Mon, 4 Jul 2022 22:08:18 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Angel Iglesias <ang.iglesiasg@...il.com>
Cc: Jonathan Cameron <jic23@...nel.org>,
Lars-Peter Clausen <lars@...afoo.de>,
"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
Paul Cercueil <paul@...pouillou.net>,
Ulf Hansson <ulf.hansson@...aro.org>,
linux-iio <linux-iio@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 5/5] iio: pressure: bmp280: Adds more tunable config
parameters for BMP380
On Mon, Jul 4, 2022 at 2:41 AM Angel Iglesias <ang.iglesiasg@...il.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
Powered by blists - more mailing lists