| 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:
<FR2PPF4571F02BC65A770A684C8F2C85C8D8C2AA@FR2PPF4571F02BC.DEUP281.PROD.OUTLOOK.COM>
Date: Wed, 13 Aug 2025 10:10:30 +0000
From: Remi Buisson <Remi.Buisson@....com>
To: Jonathan Cameron <jic23@...nel.org>,
Remi Buisson via B4 Relay
<devnull+remi.buisson.tdk.com@...nel.org>
CC: David Lechner <dlechner@...libre.com>,
Nuno Sá
<nuno.sa@...log.com>,
Andy Shevchenko <andy@...nel.org>, Rob Herring
<robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley
<conor+dt@...nel.org>,
"linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>,
"linux-iio@...r.kernel.org"
<linux-iio@...r.kernel.org>,
"devicetree@...r.kernel.org"
<devicetree@...r.kernel.org>
Subject: RE: [PATCH v3 4/8] iio: imu: inv_icm45600: add IMU IIO devices
>
>
>From: Jonathan Cameron <jic23@...nel.org>
>Sent: Thursday, July 24, 2025 6:16 PM
>To: Remi Buisson via B4 Relay <devnull+remi.buisson.tdk.com@...nel.org>
>Cc: Remi Buisson <Remi.Buisson@....com>; David Lechner <dlechner@...libre.com>; Nuno Sá <nuno.sa@...log.com>; Andy Shevchenko <andy@...nel.org>; Rob Herring <robh@...nel.org>; Krzysztof Kozlowski <krzk+dt@...nel.org>; Conor Dooley <conor+dt@...nel.org>; linux-kernel@...r.kernel.org; linux-iio@...r.kernel.org; devicetree@...r.kernel.org
>Subject: Re: [PATCH v3 4/8] iio: imu: inv_icm45600: add IMU IIO devices
>
>On Thu, 17 Jul 2025 13:25:56 +0000
>Remi Buisson via B4 Relay <devnull+remi.buisson.tdk.com@...nel.org> wrote:
>
>> From: Remi Buisson <remi.buisson@....com>
>>
>> Add IIO devices for accelerometer and gyroscope sensors
>> with data polling interface and FIFO parsing.
>> Attributes: raw, scale, sampling_frequency, calibbias.
>> Temperature is available as a processed channel.
>>
>> Signed-off-by: Remi Buisson <remi.buisson@....com>
>
>
>> diff --git a/drivers/iio/imu/inv_icm45600/inv_icm45600_accel.c b/drivers/iio/imu/inv_icm45600/inv_icm45600_accel.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..bd6b85e25e1792744769916f6d529615f9acf723
>> --- /dev/null
>> +++ b/drivers/iio/imu/inv_icm45600/inv_icm45600_accel.c
>
>If possible it would have been better to do accel and gyro in different patches
>(or more manageable size). Note I didn't read this one on the assumption any
>issues are likely to the same (and I'm out of time for today).
Thanks for the review.
Yes the files are symmetrical.
I will split the gyro and accel files in different patches for the next version.
>
>> diff --git a/drivers/iio/imu/inv_icm45600/inv_icm45600_gyro.c b/drivers/iio/imu/inv_icm45600/inv_icm45600_gyro.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..48373befafa0efe0e5dcb2c97b2c836436ce7d78
>> --- /dev/null
>> +++ b/drivers/iio/imu/inv_icm45600/inv_icm45600_gyro.c
>> +static const struct iio_chan_spec_ext_info inv_icm45600_gyro_ext_infos[] = {
>> + IIO_MOUNT_MATRIX(IIO_SHARED_BY_ALL, inv_icm45600_get_mount_matrix),
>> + { },
>
>No comma on a terminating entry like this. We don't want to make it easy
>to put things after it.
OK
>
>> +};
>
>> +/* IIO format int + nano */
>> +const int inv_icm45686_gyro_scale[][2] = {
>> + /* +/- 4000dps => 0.002130529 rad/s */
>> + [INV_ICM45686_GYRO_FS_4000DPS] = {0, 2130529},
>> + /* +/- 2000dps => 0.001065264 rad/s */
>> + [INV_ICM45686_GYRO_FS_2000DPS] = {0, 1065264},
>> + /* +/- 1000dps => 0.000532632 rad/s */
>> + [INV_ICM45686_GYRO_FS_1000DPS] = {0, 532632},
>> + /* +/- 500dps => 0.000266316 rad/s */
>> + [INV_ICM45686_GYRO_FS_500DPS] = {0, 266316},
>> + /* +/- 250dps => 0.000133158 rad/s */
>> + [INV_ICM45686_GYRO_FS_250DPS] = {0, 133158},
>> + /* +/- 125dps => 0.000066579 rad/s */
>> + [INV_ICM45686_GYRO_FS_125DPS] = {0, 66579},
>> + /* +/- 62.5dps => 0.000033290 rad/s */
>> + [INV_ICM45686_GYRO_FS_62_5DPS] = {0, 33290},
>> + /* +/- 31.25dps => 0.000016645 rad/s */
>> + [INV_ICM45686_GYRO_FS_31_25DPS] = {0, 16645},
>> + /* +/- 15.625dps => 0.000008322 rad/s */
>> + [INV_ICM45686_GYRO_FS_15_625DPS] = {0, 8322},
>
> [INV_ICM45686_GYRO_FS_15_625DPS] = { 0, 8322 },
>etc
Ok.
>
>> +};
>
>> +
>> +static int inv_icm45600_gyro_write_offset(struct inv_icm45600_state *st,
>> + struct iio_chan_spec const *chan,
>> + int val, int val2)
>> +{
>
>> +
>> + /* clamp value limited to 14 bits signed */
>
>Use clamp() then! :)
Good point !
>
>> + if (offset < -8192)
>> + offset = -8192;
>> + else if (offset > 8191)
>> + offset = 8191;
>> +
>> + st->buffer.u16 = cpu_to_le16(offset & INV_ICM45600_GYRO_OFFUSER_MASK);
>> +
>> + ret = pm_runtime_resume_and_get(dev);
>> + if (ret)
>> + return ret;
>> +
>> + scoped_guard(mutex, &st->lock)
>> + ret = regmap_bulk_write(st->map, reg, &st->buffer.u16, sizeof(st->buffer.u16));
>> +
>> + pm_runtime_put_autosuspend(dev);
>> + return ret;
>> +}
>
>
>> +static int inv_icm45600_gyro_hwfifo_flush(struct iio_dev *indio_dev,
>> + unsigned int count)
>> +{
>> + struct inv_icm45600_state *st = iio_device_get_drvdata(indio_dev);
>> + int ret;
>> +
>> + if (count == 0)
>> + return 0;
>> +
>> + guard(mutex)(&st->lock);
>> +
>> + ret = inv_icm45600_buffer_hwfifo_flush(st, count);
>> + if (!ret)
>> + ret = st->fifo.nb.gyro;
>I'd prefer keeping errors out of line.
> if (ret)
> return ret;
>
> return st->fifo.mb.gyro;
>
>Such error formatting helps a bit when people are reading a lot of code
>because it is very common.
>
Yes, agreed.
>> +
>> + return ret;
>> +}
>
>> +int inv_icm45600_gyro_parse_fifo(struct iio_dev *indio_dev)
>> +{
>> + struct inv_icm45600_state *st = iio_device_get_drvdata(indio_dev);
>> + struct inv_icm45600_sensor_state *gyro_st = iio_priv(indio_dev);
>> + struct inv_sensors_timestamp *ts = &gyro_st->ts;
>> + ssize_t i, size;
>> + unsigned int no;
>> + const struct inv_icm45600_fifo_sensor_data *accel, *gyro;
>> + const __le16 *timestamp;
>> + const s8 *temp;
>> + unsigned int odr;
>> + s64 ts_val;
>> + struct inv_icm45600_gyro_buffer buffer;
>
>Can we give some sort of order to these local variables. Pretty much anything is
>fine. Failing anything else, reverse xmas tree. Maybe also push any
>that are local to the loop in there?
Sure.
>
>> +
>> + /* parse all fifo packets */
>> + for (i = 0, no = 0; i < st->fifo.count; i += size, ++no) {
>> + size = inv_icm45600_fifo_decode_packet(&st->fifo.data[i],
>> + &accel, &gyro, &temp, ×tamp, &odr);
>> + /* quit if error or FIFO is empty */
>> + if (size <= 0)
>> + return size;
>> +
>> + /* skip packet if no gyro data or data is invalid */
>> + if (gyro == NULL || !inv_icm45600_fifo_is_data_valid(gyro))
>> + continue;
>> +
>> + /* update odr */
>> + if (odr & INV_ICM45600_SENSOR_GYRO)
>> + inv_sensors_timestamp_apply_odr(ts, st->fifo.period,
>> + st->fifo.nb.total, no);
>> +
>> + /* buffer is copied to userspace, zeroing it to avoid any data leak */
>> + memset(&buffer, 0, sizeof(buffer));
>We only care about not leaking data that is sensitive. Not stale readings or
>similar so can just do
> struct inv_icm45600_gyro_buffer buffer = { };
>above and skip the memset here.
Ok.
>
>> + memcpy(&buffer.gyro, gyro, sizeof(buffer.gyro));
>> + /* convert 8 bits FIFO temperature in high resolution format */
>> + buffer.temp = temp ? (*temp * 64) : 0;
>> + ts_val = inv_sensors_timestamp_pop(ts);
>> + iio_push_to_buffers_with_timestamp(indio_dev, &buffer, ts_val);
>> + }
>> +
>> + return 0;
>> +}
>>
>
>
Powered by blists - more mailing lists