| 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: <20240914173229.57548005@jic23-huawei>
Date: Sat, 14 Sep 2024 17:32:29 +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 v7 2/2] iio: imu: smi240: add driver
On Fri, 13 Sep 2024 12:00:11 +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>
Hi Shen,
I suspect I led you astray. regmap core seems unlikely to feed us
little endian buffers on writes (they should be CPU endian I think)
so there should be memcpy() for that not a get_unaligned_le16()
A few other minor comments inline that I missed before.
I'd probably have just tidied those up whilst applying if it wasn't
for the above question.
Jonathan
> diff --git a/drivers/iio/imu/smi240.c b/drivers/iio/imu/smi240.c
> new file mode 100644
> index 00000000000..892e14f3f60
> --- /dev/null
> +++ b/drivers/iio/imu/smi240.c
> @@ -0,0 +1,611 @@
> +// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
> +/*
> + * Copyright (c) 2024 Robert Bosch GmbH.
> + */
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
Conventionally this goes..
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/spi/spi.h>
> +#include <linux/units.h>
here.
> +
> +#include <asm/unaligned.h>
> +
> +#define SMI240_CHIP_ID 0x0024
> +
> +#define SMI240_SOFT_CONFIG_EOC_MASK BIT_MASK(0)
> +#define SMI240_SOFT_CONFIG_GYR_BW_MASK BIT_MASK(1)
BIT() only
You aren't applying these to a kernel bitmap which is an array
of unsigned longs (hence these macros have % BITS_PER_LONG
which makes no sense here).
> +#define SMI240_SOFT_CONFIG_ACC_BW_MASK BIT_MASK(2)
> +#define SMI240_SOFT_CONFIG_BITE_AUTO_MASK BIT_MASK(3)
> +#define SMI240_SOFT_CONFIG_BITE_REP_MASK GENMASK(6, 4)
> +
> +#define SMI240_CHIP_ID_REG 0x00
> +#define SMI240_SOFT_CONFIG_REG 0x0A
> +#define SMI240_TEMP_CUR_REG 0x10
> +#define SMI240_ACCEL_X_CUR_REG 0x11
> +#define SMI240_GYRO_X_CUR_REG 0x14
> +#define SMI240_DATA_CAP_FIRST_REG 0x17
> +#define SMI240_CMD_REG 0x2F
> +
> +#define SMI240_SOFT_RESET_CMD 0xB6
> +
> +#define SMI240_BITE_SEQUENCE_DELAY_US 140000
> +#define SMI240_FILTER_FLUSH_DELAY_US 60000
> +#define SMI240_DIGITAL_STARTUP_DELAY_US 120000
> +#define SMI240_MECH_STARTUP_DELAY_US 100000
> +
> +#define SMI240_CRC_INIT 0x05
> +#define SMI240_CRC_POLY 0x0B
> +#define SMI240_BUS_ID 0x00
> +
> +#define SMI240_SD_BIT_MASK 0x80000000
> +#define SMI240_CS_BIT_MASK 0x00000008
BIT() as mentioned below for these as well.
> +
> +#define SMI240_BUS_ID_MASK GENMASK(31, 30)
> +#define SMI240_WRITE_ADDR_MASK GENMASK(29, 22)
> +#define SMI240_WRITE_BIT_MASK 0x00200000
> +#define SMI240_WRITE_DATA_MASK GENMASK(18, 3)
> +#define SMI240_CAP_BIT_MASK 0x00100000
BIT(20) ?
Same for other single bit masks.
> +#define SMI240_READ_DATA_MASK GENMASK(19, 4)
> +
> +/* T°C = (temp / 256) + 25 */
> +#define SMI240_TEMP_OFFSET 6400 // 25 * 256
> +#define SMI240_TEMP_SCALE 3906250 // (1 / 256) * 1e9
> +
> +#define SMI240_ACCEL_SCALE 500 // (1 / 2000) * 1e6
> +#define SMI240_GYRO_SCALE 10000 // (1 / 100) * 1e6
Even in this case, use traditional /* */ style comments.
> +
> +}
> +
> +static int smi240_regmap_spi_write(void *context, const void *data,
> + size_t count)
> +{
> + u8 reg_addr;
> + u16 reg_data;
> + u32 request;
> + 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 = ((u8 *)data)[0];
> + reg_data = get_unaligned_le16(&((u8 *)data)[1]);
Why is the regmap core giving us an le16?
I probably sent you wrong way with this earlier :(
memcpy probably the correct choice here.
> +
> + request = FIELD_PREP(SMI240_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);
> + 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));
> +}
Powered by blists - more mailing lists