| 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: <e527ea625d48239fb765a1a9e0a9c458936b06bf.camel@gmail.com>
Date: Tue, 25 Feb 2025 09:26:56 +0000
From: Nuno Sá <noname.nuno@...il.com>
To: Jonathan Cameron <jic23@...nel.org>, "Budai, Robert"
<Robert.Budai@...log.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, "Hennerich, Michael"
<Michael.Hennerich@...log.com>, "Sa, Nuno" <Nuno.Sa@...log.com>,
"Gradinariu, Ramona" <Ramona.Gradinariu@...log.com>, "Miclaus, Antoniu"
<Antoniu.Miclaus@...log.com>, Rob Herring <robh@...nel.org>, Krzysztof
Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Jonathan Corbet <corbet@....net>, "linux-iio@...r.kernel.org"
<linux-iio@...r.kernel.org>, "devicetree@...r.kernel.org"
<devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "linux-doc@...r.kernel.org"
<linux-doc@...r.kernel.org>
Subject: Re: [RESEND PATCH v8 5/6] iio: imu: adis16550: add adis16550 support
On Sat, 2025-02-22 at 12:38 +0000, Jonathan Cameron wrote:
> On Thu, 20 Feb 2025 13:43:04 +0000
> "Budai, Robert" <Robert.Budai@...log.com> wrote:
>
> > > -----Original Message-----
> > > From: Nuno Sá <noname.nuno@...il.com>
> > > Sent: Thursday, February 20, 2025 10:22 AM
> > > To: Budai, Robert <Robert.Budai@...log.com>; Lars-Peter Clausen
> > > <lars@...afoo.de>; Hennerich, Michael <Michael.Hennerich@...log.com>;
> > > Sa, Nuno <Nuno.Sa@...log.com>; Gradinariu, Ramona
> > > <Ramona.Gradinariu@...log.com>; Miclaus, Antoniu
> > > <Antoniu.Miclaus@...log.com>; Jonathan Cameron <jic23@...nel.org>; Rob
> > > Herring <robh@...nel.org>; Krzysztof Kozlowski <krzk+dt@...nel.org>; Conor
> > > Dooley <conor+dt@...nel.org>; Jonathan Corbet <corbet@....net>; linux-
> > > iio@...r.kernel.org; devicetree@...r.kernel.org;
> > > linux-kernel@...r.kernel.org;
> > > linux-doc@...r.kernel.org
> > > Subject: Re: [RESEND PATCH v8 5/6] iio: imu: adis16550: add adis16550
> > > support
> > >
> > > [External]
> > >
> > > On Mon, 2025-02-17 at 12:57 +0200, Robert Budai wrote:
> > > > The ADIS16550 is a complete inertial system that includes a triaxis
> > > > gyroscope and a triaxis accelerometer. Each inertial sensor in the
> > > > ADIS16550 combines industry leading MEMS only technology with signal
> > > > conditioning that optimizes dynamic performance. The factory calibration
> > > > characterizes each sensor for sensitivity, bias, and alignment. As a
> > > > result, each sensor has its own dynamic compensation formulas that
> > > > provide accurate sensor measurements.
> > > >
> > > > Co-developed-by: Ramona Gradinariu <ramona.gradinariu@...log.com>
> > > > Signed-off-by: Ramona Gradinariu <ramona.gradinariu@...log.com>
> > > > Co-developed-by: Antoniu Miclaus <antoniu.miclaus@...log.com>
> > > > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@...log.com>
> > > > Signed-off-by: Nuno Sá <nuno.sa@...log.com>
> > > > Signed-off-by: Robert Budai <robert.budai@...log.com>
> > > > ---
> > > >
> > >
> > > I guess it would make sense a Co-developed-by: for Robert?
>
> The From is set to Robert, so that is implicit. It is an odd chain
> though. Now I've looked at it this doesn't
> seem quite right but I am struggling with what it should be :(
>
> I think this chain means that Ramona, Antoniu and Robert made significant
> contributions. Robert's was after Nuno had handled the patch on some
> ADI tree?
>
So, I was the one that original wrote the driver (I think about 3 years ago) and
the needed modifications on the adis lib. Then, Ramona, Antoniu and Robert all
worked on the driver after me when someone remembered it was about time to
upstream this :)
...
>
>
> diff --git a/drivers/iio/imu/adis16550.c b/drivers/iio/imu/adis16550.c
> index 16f01c61a429..8ec3afe6140b 100644
> --- a/drivers/iio/imu/adis16550.c
> +++ b/drivers/iio/imu/adis16550.c
> @@ -509,10 +509,10 @@ static int adis16550_get_accl_filter_freq(struct
> adis16550 *st, int *freq_hz)
> static int adis16550_set_accl_filter_freq(struct adis16550 *st, int freq_hz)
> {
> u8 en = freq_hz ? 1 : 0;
> + u16 val = FIELD_PREP(ADIS16550_ACCL_FIR_EN_MASK, en);
>
> return __adis_update_bits(&st->adis, ADIS16550_REG_CONFIG,
> - ADIS16550_ACCL_FIR_EN_MASK,
> - FIELD_PREP(ADIS16550_ACCL_FIR_EN_MASK, en));
> + ADIS16550_ACCL_FIR_EN_MASK, val);
> }
>
> static int adis16550_get_gyro_filter_freq(struct adis16550 *st, int *freq_hz)
> @@ -535,10 +535,10 @@ static int adis16550_get_gyro_filter_freq(struct
> adis16550 *st, int *freq_hz)
> static int adis16550_set_gyro_filter_freq(struct adis16550 *st, int freq_hz)
> {
> u8 en = freq_hz ? 1 : 0;
> + u16 val = FIELD_PREP(ADIS16550_GYRO_FIR_EN_MASK, en);
>
> return __adis_update_bits(&st->adis, ADIS16550_REG_CONFIG,
> - ADIS16550_GYRO_FIR_EN_MASK,
> - FIELD_PREP(ADIS16550_GYRO_FIR_EN_MASK, en));
> + ADIS16550_GYRO_FIR_EN_MASK, val);
> }
>
LGTM
> enum {
> @@ -832,7 +832,6 @@ static u32 adis16550_validate_crc(__be32 *buffer, const u8
> n_elem)
>
> static irqreturn_t adis16550_trigger_handler(int irq, void *p)
> {
> - u32 crc;
> int ret;
> u16 dummy;
> bool valid;
> @@ -1143,7 +1142,7 @@ module_spi_driver(adis16550_driver);
> MODULE_AUTHOR("Nuno Sa <nuno.sa@...log.com>");
> MODULE_AUTHOR("Ramona Gradinariu <ramona.gradinariu@...log.com>");
> MODULE_AUTHOR("Antoniu Miclaus <antoniu.miclaus@...log.com>");
> -MODULE_AUTHOR("Robert Budai <robert.budai@...log.com>")
> +MODULE_AUTHOR("Robert Budai <robert.budai@...log.com>");
> MODULE_DESCRIPTION("Analog Devices ADIS16550 IMU driver");
> MODULE_IMPORT_NS("IIO_
>
> > >
> > > Anyways, all looks good except for one thing that I just spotted...
> > >
> > > > v8:
> > > > - removed __aligned from struct adis16550, as suggested
> > > > - crc buffer extraction into the crc check function
> > > > - passed buffer into crc validation as original, __be32 and performed
> > > > check
> > > > using be32_to_cpu conversion of the buffer
> > > > - added trailing comma to line 993
> > > > - removed trailing comma from line 877
> > > >
> > > > drivers/iio/imu/Kconfig | 13 +
> > > > drivers/iio/imu/Makefile | 1 +
> > > > drivers/iio/imu/adis16550.c | 1149
> > > +++++++++++++++++++++++++++++++++++
> > > > 3 files changed, 1163 insertions(+)
> > > > create mode 100644 drivers/iio/imu/adis16550.c
> > > >
> > >
> > > ...
> > >
> > > >
> > > > +static int adis16550_set_freq_hz(struct adis16550 *st, u32 freq_hz)
> > > > +{
> > > > + u16 dec;
> > > > + int ret;
> > > > + u32 sample_rate = st->clk_freq_hz;
> > > > + /*
> > > > + * The optimal sample rate for the supported IMUs is between
> > > > + * int_clk - 1000 and int_clk + 500.
> > > > + */
> > > > + u32 max_sample_rate = st->info->int_clk * 1000 + 500000;
> > > > + u32 min_sample_rate = st->info->int_clk * 1000 - 1000000;
> > > > +
> > > > + if (!freq_hz)
> > > > + return -EINVAL;
> > > > +
> > > > + adis_dev_auto_lock(&st->adis);
> > > > +
> > > > + if (st->sync_mode == ADIS16550_SYNC_MODE_SCALED) {
> > > > + unsigned long scaled_rate = lcm(st->clk_freq_hz,
> > > > freq_hz);
> > > > + int sync_scale;
> > > > +
> > > > + if (scaled_rate > max_sample_rate)
> > > > + scaled_rate = max_sample_rate / st->clk_freq_hz
> > > > * st-
> > > > > clk_freq_hz;
> > > > + else
> > > > + scaled_rate = max_sample_rate / scaled_rate *
> > > > scaled_rate;
> > > > +
> > > > + if (scaled_rate < min_sample_rate)
> > > > + scaled_rate = roundup(min_sample_rate, st-
> > > > > clk_freq_hz);
> > > > +
> > >
> > > I would imagine the above is the same deal as in other devices [1] or do
> > > you
> > > know for a fact this one is different? Maybe it's simple enough for
> > > Jonathan to
> > > tweak while applying...
> > >
> > > [1]:
> > > https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v6.13.3/source
> > > /drivers/iio/imu/adis16475.c*L364__;Iw!!A3Ni8CS0y2Y!7Y71yPaQAxVzNRd
> > > O_jT7wEz4k-
> > > s6z4tJHOcES84HYkq8qNGsgJH7zxwjfPNjLF3OEGVInSolo1ennLU_mwpmEbo$
> > >
> > > - Nuno Sá
> >
> > [Robert Budai]
> > No differences were found in the scaled_sync behavior of the ADIS16475 and
> > ADIS16550. It is safe to add from my side.
> >
> If we want to do such a module parameter override I am fine with adding
> it but as a separate patch on top that provides the reasoning etc.
> It is a little to messy for me to do as a tweak.
>
That's fair
- Nuno Sá
Powered by blists - more mailing lists