| 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: <20241001194252.778fb555@jic23-huawei>
Date: Tue, 1 Oct 2024 19:42:52 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: <Jianping.Shen@...bosch.com>
Cc: <lars@...afoo.de>, <robh@...nel.org>, <krzk+dt@...nel.org>,
<conor+dt@...nel.org>, <dima.fedrau@...il.com>,
<marcelo.schmitt1@...il.com>, <linux-iio@...r.kernel.org>,
<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<Christian.Lorenz3@...bosch.com>, <Ulrike.Frauendorf@...bosch.com>,
<Kai.Dolde@...bosch.com>
Subject: Re: [PATCH v8 2/2] iio: imu: smi240: add driver
On Sat, 28 Sep 2024 18:11:21 +0100
Jonathan Cameron <jic23@...nel.org> wrote:
> On Mon, 23 Sep 2024 14:40:17 +0200
> <Jianping.Shen@...bosch.com> wrote:
>
> > From: Shen Jianping <Jianping.Shen@...bosch.com>
> >
> > add the iio driver for bosch imu smi240. The smi240 is a combined
> > three axis angular rate and three axis acceleration sensor module
> > with a measurement range of +/-300°/s and up to 16g. A synchronous
> > acc and gyro sampling can be triggered by setting the capture bit
> > in spi read command.
> >
> > Implemented features:
> > * raw data access for each axis through sysfs
> > * tiggered buffer for continuous sampling
> > * synchronous acc and gyro data from tiggered buffer
> >
> > Signed-off-by: Shen Jianping <Jianping.Shen@...bosch.com>
> At least one endian issue remaining ;(
I'd unintentionally left this on my tree after the build issue.
Dropped now from the testing branch of iio.git.
>
> Make sure you run at least C=1 builds before sending patches to the list
> CHECK drivers/iio/imu/smi240.c
> drivers/iio/imu/smi240.c:223:14: warning: incorrect type in assignment (different base types)
> drivers/iio/imu/smi240.c:223:14: expected unsigned short [usertype]
> drivers/iio/imu/smi240.c:223:14: got restricted __le16 [usertype]
>
>
> > +
> > +static int smi240_regmap_spi_read(void *context, const void *reg_buf,
> > + size_t reg_size, void *val_buf,
> > + size_t val_size)
> > +{
> > + int ret;
> > + u32 request, response;
> > + u16 *val = val_buf;
> > + struct spi_device *spi = context;
> > + struct iio_dev *indio_dev = dev_get_drvdata(&spi->dev);
> > + struct smi240_data *iio_priv_data = iio_priv(indio_dev);
> > +
> > + if (reg_size != 1 || val_size != 2)
> > + return -EINVAL;
> > +
> > + request = FIELD_PREP(SMI240_WRITE_BUS_ID_MASK, SMI240_BUS_ID);
> > + request |= FIELD_PREP(SMI240_WRITE_CAP_BIT_MASK, iio_priv_data->capture);
> > + request |= FIELD_PREP(SMI240_WRITE_ADDR_MASK, *(u8 *)reg_buf);
> > + request |= smi240_crc3(request, SMI240_CRC_INIT, SMI240_CRC_POLY);
> > +
> > + iio_priv_data->spi_buf = cpu_to_be32(request);
> > +
> > + /*
> > + * SMI240 module consists of a 32Bit Out Of Frame (OOF)
> > + * SPI protocol, where the slave interface responds to
> > + * the Master request in the next frame.
> > + * CS signal must toggle (> 700 ns) between the frames.
> > + */
> > + ret = spi_write(spi, &iio_priv_data->spi_buf, sizeof(request));
> > + if (ret)
> > + return ret;
> > +
> > + ret = spi_read(spi, &iio_priv_data->spi_buf, sizeof(response));
> > + if (ret)
> > + return ret;
> > +
> > + response = be32_to_cpu(iio_priv_data->spi_buf);
> > +
> > + if (!smi240_sensor_data_is_valid(response))
> > + return -EIO;
> > +
> > + *val = cpu_to_le16(FIELD_GET(SMI240_READ_DATA_MASK, response));
> So this is line sparse doesn't like which is reasonable given you are forcing
> an le16 value into a u16.
> Minimal fix is just to change type of val to __le16 *
>
> I still find the endian handling in here mess and am not convinced
> the complexity is strictly necessary or correct.
>
> I'd expect the requirements of reordering to be same in read and write
> directions (unless device is really crazy), so why do we need
> a conversion to le16 here but not one from le16 in the write?
>
>
> > +
> > + return 0;
> > +}
> > +
> > +static int smi240_regmap_spi_write(void *context, const void *data,
> > + size_t count)
> > +{
> > + u8 reg_addr;
> > + u16 reg_data;
> > + u32 request;
> > + const u8 *data_ptr = data;
> > + struct spi_device *spi = context;
> > + struct iio_dev *indio_dev = dev_get_drvdata(&spi->dev);
> > + struct smi240_data *iio_priv_data = iio_priv(indio_dev);
> > +
> > + if (count < 2)
> > + return -EINVAL;
> > +
> > + reg_addr = data_ptr[0];
> > + memcpy(®_data, &data_ptr[1], sizeof(reg_data));
> > +
> > + request = FIELD_PREP(SMI240_WRITE_BUS_ID_MASK, SMI240_BUS_ID);
> > + request |= FIELD_PREP(SMI240_WRITE_BIT_MASK, 1);
> > + request |= FIELD_PREP(SMI240_WRITE_ADDR_MASK, reg_addr);
> > + request |= FIELD_PREP(SMI240_WRITE_DATA_MASK, reg_data);
>
> This is built as fields in a native 32 bit register.
> My gut feeling is that you don't want the REGMAP_ENDIAN_LITTLE but
> rather use REGMAP_ENDIAN_NATIVE.
>
> The explicit reorder to be32 is fine though as that is just
> switching from the this native endian value to the byte ordering on
> the bus.
>
> > + request |= smi240_crc3(request, SMI240_CRC_INIT, SMI240_CRC_POLY);
> > +
> > + iio_priv_data->spi_buf = cpu_to_be32(request);
> > +
> > + return spi_write(spi, &iio_priv_data->spi_buf, sizeof(request));
> > +}
> > +
> > +static const struct regmap_bus smi240_regmap_bus = {
> > + .read = smi240_regmap_spi_read,
> > + .write = smi240_regmap_spi_write,
> > +};
> > +
> > +static const struct regmap_config smi240_regmap_config = {
> > + .reg_bits = 8,
> > + .val_bits = 16,
> > + .val_format_endian = REGMAP_ENDIAN_LITTLE,
> > +};
>
Powered by blists - more mailing lists