| 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: <20250222123815.0521df87@jic23-huawei>
Date: Sat, 22 Feb 2025 12:38:15 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: "Budai, Robert" <Robert.Budai@...log.com>
Cc: Nuno Sá <noname.nuno@...il.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>, "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 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?
In the end Roberts was large enough to take 'ownership' rather
than the original 'from'. That part is unusual. We rarely do that.
However, from what I can see publicly that isn't what happened.
This patch v1 was posted with Robert as Co-developed and Nuno as
the From + SoB (which puts Nuno at same level as a co-developer but
the one who posted it).
So really the ownership trail should have remained the same with
the From set to Nuno. However that gives us another fun corner.
Robert then started handing the series. That would normally require
a SoB to cover that part, but he already has one. Gah.
Anyhow I think the best way to fix this up is actually to
add Co-developed-by for Nuno which does transfer patch ownership
but at least refects that all of you played some significant part.
Alternative is modify the author ship to reflect earliest posting.
To keep things moving I'm going to assume you are fine with
me adding Co-developed by to explain Nuno's SoB where relevant
rather than changing the From and adding Co-developed-by for Robert.
The DT binding has a similar issue. There we have Ramona who
handled the patch but no co-developed. That's clearly not right
as v1 is form Ramona. There I'll make it Codev Ramona.
If you'd prefer to fix this all up with the original From:
then send me a new version and I'll switch out what I have applied.
Interesting this patch doesn't build and has a warning. I tweaked as:
For some reason the first of the u16 val cases didn't trigger the size
mismatch that __adis_update_bits() has a BUILD_BUG check on
but given the similarity of the two cases I'm not sure why and I tweaked
them both. Give me a shout if any of this breaks things.
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);
}
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.
Jonathan
> Best regards,
> Robert B
Powered by blists - more mailing lists