| 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: <texwv5s2tvcy34bwr4iruj5xofmea663pwletmpqpuh66zulmv@m7qvjgqbhalv>
Date: Mon, 19 Jan 2026 11:21:59 +0000
From: Rodrigo Alencar <455.rodrigo.alencar@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...el.com>,
rodrigo.alencar@...log.com
Cc: linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-doc@...r.kernel.org, Jonathan Cameron <jic23@...nel.org>,
David Lechner <dlechner@...libre.com>, Andy Shevchenko <andy@...nel.org>,
Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich <Michael.Hennerich@...log.com>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Jonathan Corbet <corbet@....net>
Subject: Re: [PATCH v4 3/7] iio: frequency: adf41513: driver implementation
On 26/01/19 09:31AM, Andy Shevchenko wrote:
> On Fri, Jan 16, 2026 at 02:32:22PM +0000, Rodrigo Alencar via B4 Relay wrote:
...
> > +struct adf41513_pll_settings {
> > + enum adf41513_pll_mode mode;
>
> Sounds to me like a room to improve the layout here,
>
I am targeting a 32-bit cpu, just moved in_value down:
would this be fine? (pahole output):
struct adf41513_pll_settings {
enum adf41513_pll_mode mode; /* 0 4 */
u8 r_counter; /* 4 1 */
u8 ref_doubler; /* 5 1 */
u8 ref_div2; /* 6 1 */
u8 prescaler; /* 7 1 */
u64 target_frequency_uhz; /* 8 8 */
u64 actual_frequency_uhz; /* 16 8 */
u64 pfd_frequency_uhz; /* 24 8 */
u32 frac1; /* 32 4 */
u32 frac2; /* 36 4 */
u32 mod2; /* 40 4 */
u16 int_value; /* 44 2 */
/* size: 48, cachelines: 1, members: 12 */
/* padding: 2 */
/* last cacheline: 48 bytes */
};
> > + /* reference path parameters */
> > + u8 r_counter;
> > + u8 ref_doubler;
> > + u8 ref_div2;
> > + u8 prescaler;
> > +
> > + /* frequency parameters */
> > + u64 target_frequency_uhz;
> > + u64 actual_frequency_uhz;
> > + u64 pfd_frequency_uhz;
> > +
> > + /* pll parameters */
> > + u16 int_value;
> > + u32 frac1;
> > + u32 frac2;
> > + u32 mod2;
> > +};
>
...
> > +static int adf41513_parse_uhz(const char *str, u64 *freq_uhz)
> > +{
> > + u64 uhz = 0;
> > + int f_count = ADF41513_HZ_DECIMAL_PRECISION;
> > + bool frac_part = false;
> > +
> > + if (str[0] == '+')
> > + str++;
> > +
> > + while (*str && f_count > 0) {
> > + if ('0' <= *str && *str <= '9') {
> > + uhz = uhz * 10 + *str - '0';
> > + if (frac_part)
> > + f_count--;
> > + } else if (*str == '\n') {
> > + if (*(str + 1) == '\0')
> > + break;
> > + return -EINVAL;
>
> > + } else if (*str == '.' && !frac_part) {
>
> This can be found by strchr() / strrchr() (depending on the expectations of
> the input).
>
> > + frac_part = true;
> > + } else {
> > + return -EINVAL;
> > + }
> > + str++;
> > + }
>
> With the above the rest becomes just a couple of simple_strtoull() calls with
> a couple of int_pow(10) calls (and some validation on top).
>
> > + for (; f_count > 0; f_count--)
> > + uhz *= 10;
>
> This is int_pow(10).
>
> > + *freq_uhz = uhz;
> > +
> > + return 0;
> > +}
The current implementation is kind of a stripped version of
__iio_str_to_fixpoint(). Would you prefer something like this, then?:
static int adf41513_parse_uhz(const char *str, u64 *freq_uhz)
{
u64 integer_part = 0, fractional_part = 0;
const char *decimal_point;
char *endptr;
int frac_digits;
if (str[0] == '+')
str++;
/* Find decimal point */
decimal_point = strchr(str, '.');
if (decimal_point) {
/* Parse integer part (if exists before decimal point) */
if (decimal_point > str) {
integer_part = simple_strtoull(str, &endptr, 10);
if (endptr != decimal_point)
return -EINVAL;
}
/* Parse fractional part */
fractional_part = simple_strtoull(decimal_point + 1, &endptr, 10);
if (*endptr != '\0' && *endptr != '\n')
return -EINVAL;
/* Adjust for desired precision */
frac_digits = strcspn(decimal_point + 1, "\n");
if (frac_digits > ADF41513_HZ_DECIMAL_PRECISION)
fractional_part /= int_pow(10, frac_digits - ADF41513_HZ_DECIMAL_PRECISION);
else
fractional_part *= int_pow(10, ADF41513_HZ_DECIMAL_PRECISION - frac_digits);
} else {
/* No decimal point - just parse the integer */
ret = kstrtoull(str, 10, &integer_part);
if (ret)
return ret;
}
/* Combine integer and fractional parts */
*freq_uhz = integer_part * int_pow(10, ADF41513_HZ_DECIMAL_PRECISION) + fractional_part;
return 0;
}
> ...
>
> > +static int adf41513_uhz_to_str(u64 freq_uhz, char *buf)
> > +{
> > + u32 frac_part;
> > + u64 int_part = div_u64_rem(freq_uhz, MICRO, &frac_part);
>
> Perhaps MICROHZ_PER_HZ? This will be consistent with the int_value in
> _calc_*() below.
Here, the meaning is different. int_part is in Hz and frac_part in uHz.
Will add the suffixes to the variables.
> > + return sysfs_emit(buf, "%llu.%06u\n", int_part, frac_part);
> > +}
>
...
> > + if (freq_error_uhz > (result->pfd_frequency_uhz >> 1) && int_value < max_int) {
> > + int_value++;
> > + freq_error_uhz = result->pfd_frequency_uhz - freq_error_uhz;
> > + }
>
> This and below the part for frac check seems very similar, I would consider
> adding a helper.
>
It is similar, but in each case different variables are being handled.
This one we can only deal with INT, the next one FRAC1 and the last MOD2.
I am not sure how a single helper can deal with all of them separately.
> > + if (freq_error_uhz > st->data.freq_resolution_uhz)
> > + return -ERANGE;
> > +
...
> > + /* calculate required mod2 based on target resolution / 2 */
> > + mod2 = DIV64_U64_ROUND_CLOSEST(result->pfd_frequency_uhz << 1,
> > + st->data.freq_resolution_uhz * ADF41513_FIXED_MODULUS);
>
> This also seems familiar with the above mentioned check (for 50% tolerance).
Here, there is no check, MOD2 has plenty of range to deal with the
requested frequency resolution. Also this is the last attempt, after
integer mode and fixed-modulus failed to achieve the requested frequency
value.
Kind regards,
Rodrigo Alencar
Powered by blists - more mailing lists