[PATCH 0/3] Add i2c driver for Bosch BMI260 IMU

[PATCH 0/3] Add i2c driver for Bosch BMI260 IMU

[Justin Weiss]

Add support for the Bosch BMI260 IMU to the BMI270 device driver.

The BMI270 and BMI260 have nearly identical register maps, but have
different chip IDs and firmware.

The BMI260 is the IMU on a number of handheld PCs. Unfortunately,
these devices often misidentify it in ACPI as a BMI160 ("BMI0160," for
example), and it can only be correctly identified using the chip
ID. I've changed the driver to fail if the chip ID isn't recognized so
the firmware initialization data isn't sent to incompatible devices.

Also add triggered buffer and scale / sampling frequency attributes,
which the input tools commonly used on handheld PCs require to support
IMUs.

Like the BMI270, the BMI260 requires firmware to be provided.

Signed-off-by: Justin Weiss <justin@justinweiss.com>
---

Justin Weiss (3):
  iio: imu: Add i2c driver for bmi260 imu
  iio: imu: Add triggered buffer for Bosch BMI270 IMU
  iio: imu: Add scale and sampling frequency to BMI270 IMU

 drivers/iio/imu/bmi270/Kconfig       |   1 +
 drivers/iio/imu/bmi270/bmi270.h      |  24 +-
 drivers/iio/imu/bmi270/bmi270_core.c | 369 ++++++++++++++++++++++++++-
 drivers/iio/imu/bmi270/bmi270_i2c.c  |  22 +-
 drivers/iio/imu/bmi270/bmi270_spi.c  |  11 +-
 5 files changed, 413 insertions(+), 14 deletions(-)


base-commit: 96be67caa0f0420d4128cb67f07bbd7a6f49e03a
-- 
2.47.0

[PATCH 1/3] iio: imu: Add i2c driver for bmi260 imu

[Justin Weiss]

Adds i2c support for the Bosch BMI260 6-axis IMU to the Bosch BMI270
driver. Setup and operation is nearly identical to the Bosch BMI270,
but has a different chip ID and requires different firmware.

Firmware is requested and loaded from userspace.

Signed-off-by: Justin Weiss <justin@justinweiss.com>
---
 drivers/iio/imu/bmi270/bmi270.h      | 16 ++++++++++++++-
 drivers/iio/imu/bmi270/bmi270_core.c | 29 +++++++++++++++++++++++-----
 drivers/iio/imu/bmi270/bmi270_i2c.c  | 22 ++++++++++++++++++---
 drivers/iio/imu/bmi270/bmi270_spi.c  | 11 ++++++++---
 4 files changed, 66 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/imu/bmi270/bmi270.h b/drivers/iio/imu/bmi270/bmi270.h
index 8ac20ad7ee94..51e374fd4290 100644
--- a/drivers/iio/imu/bmi270/bmi270.h
+++ b/drivers/iio/imu/bmi270/bmi270.h
@@ -10,10 +10,24 @@ struct device;
 struct bmi270_data {
 	struct device *dev;
 	struct regmap *regmap;
+	const struct bmi270_chip_info *chip_info;
+};
+
+enum bmi270_device_type {
+	BMI260,
+	BMI270,
+};
+
+struct bmi270_chip_info {
+	const char *name;
+	int chip_id;
+	const char *fw_name;
 };
 
 extern const struct regmap_config bmi270_regmap_config;
+extern const struct bmi270_chip_info bmi270_chip_info[];
 
-int bmi270_core_probe(struct device *dev, struct regmap *regmap);
+int bmi270_core_probe(struct device *dev, struct regmap *regmap,
+		      const struct bmi270_chip_info *chip_info);
 
 #endif  /* BMI270_H_ */
diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c
index aeda7c4228df..e5ee80c12166 100644
--- a/drivers/iio/imu/bmi270/bmi270_core.c
+++ b/drivers/iio/imu/bmi270/bmi270_core.c
@@ -11,6 +11,7 @@
 #include "bmi270.h"
 
 #define BMI270_CHIP_ID_REG				0x00
+#define BMI260_CHIP_ID_VAL				0x27
 #define BMI270_CHIP_ID_VAL				0x24
 #define BMI270_CHIP_ID_MSK				GENMASK(7, 0)
 
@@ -55,6 +56,7 @@
 #define BMI270_PWR_CTRL_ACCEL_EN_MSK			BIT(2)
 #define BMI270_PWR_CTRL_TEMP_EN_MSK			BIT(3)
 
+#define BMI260_INIT_DATA_FILE "bmi260-init-data.fw"
 #define BMI270_INIT_DATA_FILE "bmi270-init-data.fw"
 
 enum bmi270_scan {
@@ -66,6 +68,20 @@ enum bmi270_scan {
 	BMI270_SCAN_GYRO_Z,
 };
 
+const struct bmi270_chip_info bmi270_chip_info[] = {
+	[BMI260] = {
+		.name = "bmi260",
+		.chip_id = BMI260_CHIP_ID_VAL,
+		.fw_name = BMI260_INIT_DATA_FILE,
+	},
+	[BMI270] = {
+		.name = "bmi270",
+		.chip_id = BMI270_CHIP_ID_VAL,
+		.fw_name = BMI270_INIT_DATA_FILE,
+	}
+};
+EXPORT_SYMBOL_NS_GPL(bmi270_chip_info, IIO_BMI270);
+
 static int bmi270_get_data(struct bmi270_data *bmi270_device,
 			   int chan_type, int axis, int *val)
 {
@@ -154,8 +170,8 @@ static int bmi270_validate_chip_id(struct bmi270_data *bmi270_device)
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to read chip id");
 
-	if (chip_id != BMI270_CHIP_ID_VAL)
-		dev_info(dev, "Unknown chip id 0x%x", chip_id);
+	if (chip_id != bmi270_device->chip_info->chip_id)
+		return dev_err_probe(dev, -ENODEV, "Unknown chip id 0x%x", chip_id);
 
 	return 0;
 }
@@ -187,7 +203,8 @@ static int bmi270_write_calibration_data(struct bmi270_data *bmi270_device)
 		return dev_err_probe(dev, ret,
 				     "Failed to prepare device to load init data");
 
-	ret = request_firmware(&init_data, BMI270_INIT_DATA_FILE, dev);
+	ret = request_firmware(&init_data,
+			       bmi270_device->chip_info->fw_name, dev);
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to load init data file");
 
@@ -274,7 +291,8 @@ static int bmi270_chip_init(struct bmi270_data *bmi270_device)
 	return bmi270_configure_imu(bmi270_device);
 }
 
-int bmi270_core_probe(struct device *dev, struct regmap *regmap)
+int bmi270_core_probe(struct device *dev, struct regmap *regmap,
+		      const struct bmi270_chip_info *chip_info)
 {
 	int ret;
 	struct bmi270_data *bmi270_device;
@@ -287,6 +305,7 @@ int bmi270_core_probe(struct device *dev, struct regmap *regmap)
 	bmi270_device = iio_priv(indio_dev);
 	bmi270_device->dev = dev;
 	bmi270_device->regmap = regmap;
+	bmi270_device->chip_info = chip_info;
 
 	ret = bmi270_chip_init(bmi270_device);
 	if (ret)
@@ -294,7 +313,7 @@ int bmi270_core_probe(struct device *dev, struct regmap *regmap)
 
 	indio_dev->channels = bmi270_channels;
 	indio_dev->num_channels = ARRAY_SIZE(bmi270_channels);
-	indio_dev->name = "bmi270";
+	indio_dev->name = chip_info->name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &bmi270_info;
 
diff --git a/drivers/iio/imu/bmi270/bmi270_i2c.c b/drivers/iio/imu/bmi270/bmi270_i2c.c
index e9025d22d5cc..c8c90666c76b 100644
--- a/drivers/iio/imu/bmi270/bmi270_i2c.c
+++ b/drivers/iio/imu/bmi270/bmi270_i2c.c
@@ -18,28 +18,44 @@ static int bmi270_i2c_probe(struct i2c_client *client)
 {
 	struct regmap *regmap;
 	struct device *dev = &client->dev;
+	const struct bmi270_chip_info *chip_info;
+
+	chip_info = i2c_get_match_data(client);
+	if (!chip_info)
+		return -ENODEV;
 
 	regmap = devm_regmap_init_i2c(client, &bmi270_i2c_regmap_config);
 	if (IS_ERR(regmap))
 		return dev_err_probe(dev, PTR_ERR(regmap),
 				     "Failed to init i2c regmap");
 
-	return bmi270_core_probe(dev, regmap);
+	return bmi270_core_probe(dev, regmap, chip_info);
 }
 
 static const struct i2c_device_id bmi270_i2c_id[] = {
-	{ "bmi270", 0 },
+	{ "bmi260", (kernel_ulong_t)&bmi270_chip_info[BMI260] },
+	{ "bmi270", (kernel_ulong_t)&bmi270_chip_info[BMI270] },
 	{ }
 };
 
+static const struct acpi_device_id bmi270_acpi_match[] = {
+	{ "BOSC0260", (kernel_ulong_t)&bmi270_chip_info[BMI260] },
+	{ "BMI0260",  (kernel_ulong_t)&bmi270_chip_info[BMI260] },
+	{ "BOSC0160", (kernel_ulong_t)&bmi270_chip_info[BMI260] },
+	{ "BMI0160",  (kernel_ulong_t)&bmi270_chip_info[BMI260] },
+	{ "10EC5280", (kernel_ulong_t)&bmi270_chip_info[BMI260] },
+	{ },
+};
+
 static const struct of_device_id bmi270_of_match[] = {
-	{ .compatible = "bosch,bmi270" },
+	{ .compatible = "bosch,bmi270", .data = &bmi270_chip_info[BMI270] },
 	{ }
 };
 
 static struct i2c_driver bmi270_i2c_driver = {
 	.driver = {
 		.name = "bmi270_i2c",
+		.acpi_match_table = bmi270_acpi_match,
 		.of_match_table = bmi270_of_match,
 	},
 	.probe = bmi270_i2c_probe,
diff --git a/drivers/iio/imu/bmi270/bmi270_spi.c b/drivers/iio/imu/bmi270/bmi270_spi.c
index 34d5ba6273bb..3d240f9651bc 100644
--- a/drivers/iio/imu/bmi270/bmi270_spi.c
+++ b/drivers/iio/imu/bmi270/bmi270_spi.c
@@ -50,6 +50,11 @@ static int bmi270_spi_probe(struct spi_device *spi)
 {
 	struct regmap *regmap;
 	struct device *dev = &spi->dev;
+	const struct bmi270_chip_info *chip_info;
+
+	chip_info = spi_get_device_match_data(spi);
+	if (!chip_info)
+		return -ENODEV;
 
 	regmap = devm_regmap_init(dev, &bmi270_regmap_bus, dev,
 				  &bmi270_spi_regmap_config);
@@ -57,16 +62,16 @@ static int bmi270_spi_probe(struct spi_device *spi)
 		return dev_err_probe(dev, PTR_ERR(regmap),
 				     "Failed to init i2c regmap");
 
-	return bmi270_core_probe(dev, regmap);
+	return bmi270_core_probe(dev, regmap, chip_info);
 }
 
 static const struct spi_device_id bmi270_spi_id[] = {
-	{ "bmi270" },
+	{ "bmi270", (kernel_ulong_t)&bmi270_chip_info[BMI270] },
 	{ }
 };
 
 static const struct of_device_id bmi270_of_match[] = {
-	{ .compatible = "bosch,bmi270" },
+	{ .compatible = "bosch,bmi270", .data = &bmi270_chip_info[BMI270] },
 	{ }
 };
 
-- 
2.47.0

Re: [PATCH 1/3] iio: imu: Add i2c driver for bmi260 imu

[Jonathan Cameron]

On Fri, 11 Oct 2024 08:37:47 -0700
Justin Weiss <justin@justinweiss.com> wrote:

Title needs an edit to reflect the description that follows.

iio: imu: bmi270: Add i2c support for bmi260


> Adds i2c support for the Bosch BMI260 6-axis IMU to the Bosch BMI270
> driver. Setup and operation is nearly identical to the Bosch BMI270,
> but has a different chip ID and requires different firmware.
> 
> Firmware is requested and loaded from userspace.
> 
> Signed-off-by: Justin Weiss <justin@justinweiss.com>
> ---
>  drivers/iio/imu/bmi270/bmi270.h      | 16 ++++++++++++++-
>  drivers/iio/imu/bmi270/bmi270_core.c | 29 +++++++++++++++++++++++-----
>  drivers/iio/imu/bmi270/bmi270_i2c.c  | 22 ++++++++++++++++++---
>  drivers/iio/imu/bmi270/bmi270_spi.c  | 11 ++++++++---
>  4 files changed, 66 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/imu/bmi270/bmi270.h b/drivers/iio/imu/bmi270/bmi270.h
> index 8ac20ad7ee94..51e374fd4290 100644
> --- a/drivers/iio/imu/bmi270/bmi270.h
> +++ b/drivers/iio/imu/bmi270/bmi270.h
> @@ -10,10 +10,24 @@ struct device;
>  struct bmi270_data {
>  	struct device *dev;
>  	struct regmap *regmap;
> +	const struct bmi270_chip_info *chip_info;
> +};
> +
> +enum bmi270_device_type {
> +	BMI260,
> +	BMI270,
> +};
It is obviously fairly trivial in this case, but the 'ideal' form for
a patch series adding this flexibility is:
Patch 1) Add a noop refactor to include the configuration structures etc.
Patch 2) Add the support for the new device.

First patch can then be reviewed on basis it's not destructive and second one just for
the chip specific data added


> diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c
> index aeda7c4228df..e5ee80c12166 100644
> --- a/drivers/iio/imu/bmi270/bmi270_core.c
> +++ b/drivers/iio/imu/bmi270/bmi270_core.c
> @@ -11,6 +11,7 @@

>  static int bmi270_get_data(struct bmi270_data *bmi270_device,
>  			   int chan_type, int axis, int *val)
>  {
> @@ -154,8 +170,8 @@ static int bmi270_validate_chip_id(struct bmi270_data *bmi270_device)
>  	if (ret)
>  		return dev_err_probe(dev, ret, "Failed to read chip id");
>  
> -	if (chip_id != BMI270_CHIP_ID_VAL)
> -		dev_info(dev, "Unknown chip id 0x%x", chip_id);
> +	if (chip_id != bmi270_device->chip_info->chip_id)
If we have multiple known IDs it can be slightly more friendly to check them all
and if another one matches, just moan about broken firmware before carrying on
using the correct data.

> +		return dev_err_probe(dev, -ENODEV, "Unknown chip id 0x%x", chip_id);
>  
>  	return 0;
>  }
> @@ -187,7 +203,8 @@ static int bmi270_write_calibration_data(struct bmi270_data *bmi270_device)
>  		return dev_err_probe(dev, ret,
>  				     "Failed to prepare device to load init data");
>  
> -	ret = request_firmware(&init_data, BMI270_INIT_DATA_FILE, dev);
> +	ret = request_firmware(&init_data,
> +			       bmi270_device->chip_info->fw_name, dev);
>  	if (ret)
>  		return dev_err_probe(dev, ret, "Failed to load init data file");
>  
> @@ -274,7 +291,8 @@ static int bmi270_chip_init(struct bmi270_data *bmi270_device)
>  	return bmi270_configure_imu(bmi270_device);
>  }

> diff --git a/drivers/iio/imu/bmi270/bmi270_i2c.c b/drivers/iio/imu/bmi270/bmi270_i2c.c
> index e9025d22d5cc..c8c90666c76b 100644
> --- a/drivers/iio/imu/bmi270/bmi270_i2c.c
> +++ b/drivers/iio/imu/bmi270/bmi270_i2c.c
> @@ -18,28 +18,44 @@ static int bmi270_i2c_probe(struct i2c_client *client)
>  {
>  	struct regmap *regmap;
>  	struct device *dev = &client->dev;
> +	const struct bmi270_chip_info *chip_info;
> +
> +	chip_info = i2c_get_match_data(client);
> +	if (!chip_info)
> +		return -ENODEV;
>  
>  	regmap = devm_regmap_init_i2c(client, &bmi270_i2c_regmap_config);
>  	if (IS_ERR(regmap))
>  		return dev_err_probe(dev, PTR_ERR(regmap),
>  				     "Failed to init i2c regmap");
>  
> -	return bmi270_core_probe(dev, regmap);
> +	return bmi270_core_probe(dev, regmap, chip_info);
>  }
>  
>  static const struct i2c_device_id bmi270_i2c_id[] = {
> -	{ "bmi270", 0 },
> +	{ "bmi260", (kernel_ulong_t)&bmi270_chip_info[BMI260] },
> +	{ "bmi270", (kernel_ulong_t)&bmi270_chip_info[BMI270] },
>  	{ }
>  };
>  
> +static const struct acpi_device_id bmi270_acpi_match[] = {
> +	{ "BOSC0260", (kernel_ulong_t)&bmi270_chip_info[BMI260] },
> +	{ "BMI0260",  (kernel_ulong_t)&bmi270_chip_info[BMI260] },
> +	{ "BOSC0160", (kernel_ulong_t)&bmi270_chip_info[BMI260] },
> +	{ "BMI0160",  (kernel_ulong_t)&bmi270_chip_info[BMI260] },

Sigh.  That's not a valid ACPI ID or PNP ID.
(Well technically it is, but it belongs to the Benson Instrument Company
not Bosch)

Which of these have been seen in the wild?
For any that are not of the BOSC0160 type form add a comment giving
a device on which they are in use.

> +	{ "10EC5280", (kernel_ulong_t)&bmi270_chip_info[BMI260] },

What's this one?  There is no such vendor ID.

> +	{ },
No trailing comma on null terminators like this.

> +};
> +
>  static const struct of_device_id bmi270_of_match[] = {
> -	{ .compatible = "bosch,bmi270" },
> +	{ .compatible = "bosch,bmi270", .data = &bmi270_chip_info[BMI270] },

Add the 260 here as well + add to the dt docs.

>  	{ }
>  };
>  
>  static struct i2c_driver bmi270_i2c_driver = {
>  	.driver = {
>  		.name = "bmi270_i2c",
> +		.acpi_match_table = bmi270_acpi_match,
>  		.of_match_table = bmi270_of_match,
>  	},
>  	.probe = bmi270_i2c_probe,
> diff --git a/drivers/iio/imu/bmi270/bmi270_spi.c b/drivers/iio/imu/bmi270/bmi270_spi.c
> index 34d5ba6273bb..3d240f9651bc 100644
> --- a/drivers/iio/imu/bmi270/bmi270_spi.c
> +++ b/drivers/iio/imu/bmi270/bmi270_spi.c
> @@ -50,6 +50,11 @@ static int bmi270_spi_probe(struct spi_device *spi)
>  {
>  	struct regmap *regmap;
>  	struct device *dev = &spi->dev;
> +	const struct bmi270_chip_info *chip_info;
> +
> +	chip_info = spi_get_device_match_data(spi);
> +	if (!chip_info)
> +		return -ENODEV;
>  
>  	regmap = devm_regmap_init(dev, &bmi270_regmap_bus, dev,
>  				  &bmi270_spi_regmap_config);
> @@ -57,16 +62,16 @@ static int bmi270_spi_probe(struct spi_device *spi)
>  		return dev_err_probe(dev, PTR_ERR(regmap),
>  				     "Failed to init i2c regmap");
>  
> -	return bmi270_core_probe(dev, regmap);
> +	return bmi270_core_probe(dev, regmap, chip_info);
>  }
>  
>  static const struct spi_device_id bmi270_spi_id[] = {
> -	{ "bmi270" },
> +	{ "bmi270", (kernel_ulong_t)&bmi270_chip_info[BMI270] },
>  	{ }
>  };
>  
>  static const struct of_device_id bmi270_of_match[] = {
> -	{ .compatible = "bosch,bmi270" },
> +	{ .compatible = "bosch,bmi270", .data = &bmi270_chip_info[BMI270] },

If the bmi260 supports SPI, should be added here as well. (I've no idea if it does!)

Or is this because you can't test it?

>  	{ }
>  };
>  

Re: [PATCH 1/3] iio: imu: Add i2c driver for bmi260 imu

[Justin Weiss]

Jonathan Cameron <jic23@kernel.org> writes:

> On Fri, 11 Oct 2024 08:37:47 -0700
> Justin Weiss <justin@justinweiss.com> wrote:
>
> Title needs an edit to reflect the description that follows.
>
> iio: imu: bmi270: Add i2c support for bmi260

Thanks, I'll fix this in v2.

>> Adds i2c support for the Bosch BMI260 6-axis IMU to the Bosch BMI270
>> driver. Setup and operation is nearly identical to the Bosch BMI270,
>> but has a different chip ID and requires different firmware.
>> 
>> Firmware is requested and loaded from userspace.
>> 
>> Signed-off-by: Justin Weiss <justin@justinweiss.com>
>> ---
>>  drivers/iio/imu/bmi270/bmi270.h      | 16 ++++++++++++++-
>>  drivers/iio/imu/bmi270/bmi270_core.c | 29 +++++++++++++++++++++++-----
>>  drivers/iio/imu/bmi270/bmi270_i2c.c  | 22 ++++++++++++++++++---
>>  drivers/iio/imu/bmi270/bmi270_spi.c  | 11 ++++++++---
>>  4 files changed, 66 insertions(+), 12 deletions(-)
>> 
>> diff --git a/drivers/iio/imu/bmi270/bmi270.h b/drivers/iio/imu/bmi270/bmi270.h
>> index 8ac20ad7ee94..51e374fd4290 100644
>> --- a/drivers/iio/imu/bmi270/bmi270.h
>> +++ b/drivers/iio/imu/bmi270/bmi270.h
>> @@ -10,10 +10,24 @@ struct device;
>>  struct bmi270_data {
>>  	struct device *dev;
>>  	struct regmap *regmap;
>> +	const struct bmi270_chip_info *chip_info;
>> +};
>> +
>> +enum bmi270_device_type {
>> +	BMI260,
>> +	BMI270,
>> +};
> It is obviously fairly trivial in this case, but the 'ideal' form for
> a patch series adding this flexibility is:
> Patch 1) Add a noop refactor to include the configuration structures etc.
> Patch 2) Add the support for the new device.
>
> First patch can then be reviewed on basis it's not destructive and second one just for
> the chip specific data added

Makes sense, I'll split these up for v2.

>> diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c
>> index aeda7c4228df..e5ee80c12166 100644
>> --- a/drivers/iio/imu/bmi270/bmi270_core.c
>> +++ b/drivers/iio/imu/bmi270/bmi270_core.c
>> @@ -11,6 +11,7 @@
>
>>  static int bmi270_get_data(struct bmi270_data *bmi270_device,
>>  			   int chan_type, int axis, int *val)
>>  {
>> @@ -154,8 +170,8 @@ static int bmi270_validate_chip_id(struct bmi270_data *bmi270_device)
>>  	if (ret)
>>  		return dev_err_probe(dev, ret, "Failed to read chip id");
>>  
>> -	if (chip_id != BMI270_CHIP_ID_VAL)
>> -		dev_info(dev, "Unknown chip id 0x%x", chip_id);
>> +	if (chip_id != bmi270_device->chip_info->chip_id)
> If we have multiple known IDs it can be slightly more friendly to check them all
> and if another one matches, just moan about broken firmware before carrying on
> using the correct data.

Sounds good. I'll dev_info a message if it doesn't match what the
chip_info expects, and then switch to the chip_info corresponding to the
chip ID returned by the device.

>
>> +		return dev_err_probe(dev, -ENODEV, "Unknown chip id 0x%x", chip_id);
>>  
>>  	return 0;
>>  }
>> @@ -187,7 +203,8 @@ static int bmi270_write_calibration_data(struct bmi270_data *bmi270_device)
>>  		return dev_err_probe(dev, ret,
>>  				     "Failed to prepare device to load init data");
>>  
>> -	ret = request_firmware(&init_data, BMI270_INIT_DATA_FILE, dev);
>> +	ret = request_firmware(&init_data,
>> +			       bmi270_device->chip_info->fw_name, dev);
>>  	if (ret)
>>  		return dev_err_probe(dev, ret, "Failed to load init data file");
>>  
>> @@ -274,7 +291,8 @@ static int bmi270_chip_init(struct bmi270_data *bmi270_device)
>>  	return bmi270_configure_imu(bmi270_device);
>>  }
>
>> diff --git a/drivers/iio/imu/bmi270/bmi270_i2c.c b/drivers/iio/imu/bmi270/bmi270_i2c.c
>> index e9025d22d5cc..c8c90666c76b 100644
>> --- a/drivers/iio/imu/bmi270/bmi270_i2c.c
>> +++ b/drivers/iio/imu/bmi270/bmi270_i2c.c
>> @@ -18,28 +18,44 @@ static int bmi270_i2c_probe(struct i2c_client *client)
>>  {
>>  	struct regmap *regmap;
>>  	struct device *dev = &client->dev;
>> +	const struct bmi270_chip_info *chip_info;
>> +
>> +	chip_info = i2c_get_match_data(client);
>> +	if (!chip_info)
>> +		return -ENODEV;
>>  
>>  	regmap = devm_regmap_init_i2c(client, &bmi270_i2c_regmap_config);
>>  	if (IS_ERR(regmap))
>>  		return dev_err_probe(dev, PTR_ERR(regmap),
>>  				     "Failed to init i2c regmap");
>>  
>> -	return bmi270_core_probe(dev, regmap);
>> +	return bmi270_core_probe(dev, regmap, chip_info);
>>  }
>>  
>>  static const struct i2c_device_id bmi270_i2c_id[] = {
>> -	{ "bmi270", 0 },
>> +	{ "bmi260", (kernel_ulong_t)&bmi270_chip_info[BMI260] },
>> +	{ "bmi270", (kernel_ulong_t)&bmi270_chip_info[BMI270] },
>>  	{ }
>>  };
>>  
>> +static const struct acpi_device_id bmi270_acpi_match[] = {
>> +	{ "BOSC0260", (kernel_ulong_t)&bmi270_chip_info[BMI260] },
>> +	{ "BMI0260",  (kernel_ulong_t)&bmi270_chip_info[BMI260] },
>> +	{ "BOSC0160", (kernel_ulong_t)&bmi270_chip_info[BMI260] },
>> +	{ "BMI0160",  (kernel_ulong_t)&bmi270_chip_info[BMI260] },
>
> Sigh.  That's not a valid ACPI ID or PNP ID.
> (Well technically it is, but it belongs to the Benson Instrument Company
> not Bosch)
>
> Which of these have been seen in the wild?
> For any that are not of the BOSC0160 type form add a comment giving
> a device on which they are in use.

I know of the BMI0160 (this seems to be the most common way the BMI260
is identified on handheld PCs), and the 10EC5280 has been seen in the
wild, as described here:
https://lore.kernel.org/all/CAFqHKTm2WRNkcSoBEE=oNbfu_9d9RagQHLydmv6q1=snO_MXyA@mail.gmail.com/

I have not personally seen any devices using BMI0260, but I'll add
comments to the BMI0160 and 10EC5280 entries with some examples of
devices that use those IDs.

>> +	{ "10EC5280", (kernel_ulong_t)&bmi270_chip_info[BMI260] },
>
> What's this one?  There is no such vendor ID.
>
>> +	{ },
> No trailing comma on null terminators like this.

Thanks, will fix in v2.

>> +};
>> +
>>  static const struct of_device_id bmi270_of_match[] = {
>> -	{ .compatible = "bosch,bmi270" },
>> +	{ .compatible = "bosch,bmi270", .data = &bmi270_chip_info[BMI270] },
>
> Add the 260 here as well + add to the dt docs.

Will fix in v2.

>>  	{ }
>>  };
>>  
>>  static struct i2c_driver bmi270_i2c_driver = {
>>  	.driver = {
>>  		.name = "bmi270_i2c",
>> +		.acpi_match_table = bmi270_acpi_match,
>>  		.of_match_table = bmi270_of_match,
>>  	},
>>  	.probe = bmi270_i2c_probe,
>> diff --git a/drivers/iio/imu/bmi270/bmi270_spi.c b/drivers/iio/imu/bmi270/bmi270_spi.c
>> index 34d5ba6273bb..3d240f9651bc 100644
>> --- a/drivers/iio/imu/bmi270/bmi270_spi.c
>> +++ b/drivers/iio/imu/bmi270/bmi270_spi.c
>> @@ -50,6 +50,11 @@ static int bmi270_spi_probe(struct spi_device *spi)
>>  {
>>  	struct regmap *regmap;
>>  	struct device *dev = &spi->dev;
>> +	const struct bmi270_chip_info *chip_info;
>> +
>> +	chip_info = spi_get_device_match_data(spi);
>> +	if (!chip_info)
>> +		return -ENODEV;
>>  
>>  	regmap = devm_regmap_init(dev, &bmi270_regmap_bus, dev,
>>  				  &bmi270_spi_regmap_config);
>> @@ -57,16 +62,16 @@ static int bmi270_spi_probe(struct spi_device *spi)
>>  		return dev_err_probe(dev, PTR_ERR(regmap),
>>  				     "Failed to init i2c regmap");
>>  
>> -	return bmi270_core_probe(dev, regmap);
>> +	return bmi270_core_probe(dev, regmap, chip_info);
>>  }
>>  
>>  static const struct spi_device_id bmi270_spi_id[] = {
>> -	{ "bmi270" },
>> +	{ "bmi270", (kernel_ulong_t)&bmi270_chip_info[BMI270] },
>>  	{ }
>>  };
>>  
>>  static const struct of_device_id bmi270_of_match[] = {
>> -	{ .compatible = "bosch,bmi270" },
>> +	{ .compatible = "bosch,bmi270", .data = &bmi270_chip_info[BMI270] },
>
> If the bmi260 supports SPI, should be added here as well. (I've no idea if it does!)
>
> Or is this because you can't test it?

Yeah, it was because I can't test it, the BMI260 does support SPI. I can
add entries here, though.

Should the ACPI match entries from I2C also go here? All of the devices
with mismatched IDs seem to use I2C so there might not be as much of a
problem here.

>>  	{ }
>>  };
>>  

Thanks again,
Justin

Re: [PATCH 1/3] iio: imu: Add i2c driver for bmi260 imu

[Jonathan Cameron]

> >> +static const struct acpi_device_id bmi270_acpi_match[] = {
> >> +	{ "BOSC0260", (kernel_ulong_t)&bmi270_chip_info[BMI260] },
> >> +	{ "BMI0260",  (kernel_ulong_t)&bmi270_chip_info[BMI260] },
> >> +	{ "BOSC0160", (kernel_ulong_t)&bmi270_chip_info[BMI260] },
> >> +	{ "BMI0160",  (kernel_ulong_t)&bmi270_chip_info[BMI260] },  
> >
> > Sigh.  That's not a valid ACPI ID or PNP ID.
> > (Well technically it is, but it belongs to the Benson Instrument Company
> > not Bosch)
> >
> > Which of these have been seen in the wild?
> > For any that are not of the BOSC0160 type form add a comment giving
> > a device on which they are in use.  
> 
> I know of the BMI0160 (this seems to be the most common way the BMI260
> is identified on handheld PCs), and the 10EC5280 has been seen in the
> wild, as described here:
> https://lore.kernel.org/all/CAFqHKTm2WRNkcSoBEE=oNbfu_9d9RagQHLydmv6q1=snO_MXyA@mail.gmail.com/
> 
> I have not personally seen any devices using BMI0260, but I'll add
> comments to the BMI0160 and 10EC5280 entries with some examples of
> devices that use those IDs.

Drop any we don't have evidence are out there.

Do we have any confirmation from Bosch (or products in the wild) for
the structurally correct BOSC0160 etc?  Those would normally have
to be tracked by Bosch as allocated for this purpose.

> 
> >> +	{ "10EC5280", (kernel_ulong_t)&bmi270_chip_info[BMI260] },  
> >
> > What's this one?  There is no such vendor ID.
> >  
>
...

> >>  
> >>  static const struct of_device_id bmi270_of_match[] = {
> >> -	{ .compatible = "bosch,bmi270" },
> >> +	{ .compatible = "bosch,bmi270", .data = &bmi270_chip_info[BMI270] },  
> >
> > If the bmi260 supports SPI, should be added here as well. (I've no idea if it does!)
> >
> > Or is this because you can't test it?  
> 
> Yeah, it was because I can't test it, the BMI260 does support SPI. I can
> add entries here, though.
> 
> Should the ACPI match entries from I2C also go here? All of the devices
> with mismatched IDs seem to use I2C so there might not be as much of a
> problem here.
We want the incorrect formatted ones to be as hard to use as possible to discourage
them going into new products.  Can't do anything to solve the i2c cases
but definitely don't want to allow them for SPI as well if no evidence
of products where it yet matters.

If we have confirmation from Bosch of the BOSC forms, then those I would like
in the SPI drivers as well (to point to the correct option for anyone using
this in future!)

Jonathan
 
> 
> >>  	{ }
> >>  };
> >>    
> 
> Thanks again,
> Justin

Re: [PATCH 1/3] iio: imu: Add i2c driver for bmi260 imu

[Justin Weiss]

Jonathan Cameron <jic23@kernel.org> writes:

>> >> +static const struct acpi_device_id bmi270_acpi_match[] = {
>> >> +	{ "BOSC0260", (kernel_ulong_t)&bmi270_chip_info[BMI260] },
>> >> +	{ "BMI0260",  (kernel_ulong_t)&bmi270_chip_info[BMI260] },
>> >> +	{ "BOSC0160", (kernel_ulong_t)&bmi270_chip_info[BMI260] },
>> >> +	{ "BMI0160",  (kernel_ulong_t)&bmi270_chip_info[BMI260] },  
>> >
>> > Sigh.  That's not a valid ACPI ID or PNP ID.
>> > (Well technically it is, but it belongs to the Benson Instrument Company
>> > not Bosch)
>> >
>> > Which of these have been seen in the wild?
>> > For any that are not of the BOSC0160 type form add a comment giving
>> > a device on which they are in use.  
>> 
>> I know of the BMI0160 (this seems to be the most common way the BMI260
>> is identified on handheld PCs), and the 10EC5280 has been seen in the
>> wild, as described here:
>> https://lore.kernel.org/all/CAFqHKTm2WRNkcSoBEE=oNbfu_9d9RagQHLydmv6q1=snO_MXyA@mail.gmail.com/
>> 
>> I have not personally seen any devices using BMI0260, but I'll add
>> comments to the BMI0160 and 10EC5280 entries with some examples of
>> devices that use those IDs.
>
> Drop any we don't have evidence are out there.
>
> Do we have any confirmation from Bosch (or products in the wild) for
> the structurally correct BOSC0160 etc?  Those would normally have
> to be tracked by Bosch as allocated for this purpose.
>

I haven't seen any reported, but the Windows driver INF has all five of
these entries listed. I don't see any evidence of the BOSC0160 or
BOSC0260 being used other than that Windows driver file.

BMI0160 seems by far the most common, with some appearances of 10EC5280
(some AYANEO devices, possibly some GPD Win Max 2 devices) and BMI0260
(OrangePi NEO).

>> 
>> >> +	{ "10EC5280", (kernel_ulong_t)&bmi270_chip_info[BMI260] },  
>> >
>> > What's this one?  There is no such vendor ID.
>> >  
>>
> ...
>
>> >>  
>> >>  static const struct of_device_id bmi270_of_match[] = {
>> >> -	{ .compatible = "bosch,bmi270" },
>> >> +	{ .compatible = "bosch,bmi270", .data = &bmi270_chip_info[BMI270] },  
>> >
>> > If the bmi260 supports SPI, should be added here as well. (I've no idea if it does!)
>> >
>> > Or is this because you can't test it?  
>> 
>> Yeah, it was because I can't test it, the BMI260 does support SPI. I can
>> add entries here, though.
>> 
>> Should the ACPI match entries from I2C also go here? All of the devices
>> with mismatched IDs seem to use I2C so there might not be as much of a
>> problem here.
> We want the incorrect formatted ones to be as hard to use as possible to discourage
> them going into new products.  Can't do anything to solve the i2c cases
> but definitely don't want to allow them for SPI as well if no evidence
> of products where it yet matters.
>
> If we have confirmation from Bosch of the BOSC forms, then those I would like
> in the SPI drivers as well (to point to the correct option for anyone using
> this in future!)
>
> Jonathan
>  

Agreed. Since we don't have confirmation of the correct values here or
any that are in use, I would be OK either adding the single BOSC0260
entry (as a guess, which may or may not be used) or leaving it out
entirely until an entry is needed.

Justin

>> 
>> >>  	{ }
>> >>  };
>> >>    
>> 
>> Thanks again,
>> Justin

Re: [PATCH 1/3] iio: imu: Add i2c driver for bmi260 imu

[Jonathan Cameron]

On Sun, 13 Oct 2024 13:36:51 -0700
Justin Weiss <justin@justinweiss.com> wrote:

> Jonathan Cameron <jic23@kernel.org> writes:
> 
> >> >> +static const struct acpi_device_id bmi270_acpi_match[] = {
> >> >> +	{ "BOSC0260", (kernel_ulong_t)&bmi270_chip_info[BMI260] },
> >> >> +	{ "BMI0260",  (kernel_ulong_t)&bmi270_chip_info[BMI260] },
> >> >> +	{ "BOSC0160", (kernel_ulong_t)&bmi270_chip_info[BMI260] },
> >> >> +	{ "BMI0160",  (kernel_ulong_t)&bmi270_chip_info[BMI260] },    
> >> >
> >> > Sigh.  That's not a valid ACPI ID or PNP ID.
> >> > (Well technically it is, but it belongs to the Benson Instrument Company
> >> > not Bosch)
> >> >
> >> > Which of these have been seen in the wild?
> >> > For any that are not of the BOSC0160 type form add a comment giving
> >> > a device on which they are in use.    
> >> 
> >> I know of the BMI0160 (this seems to be the most common way the BMI260
> >> is identified on handheld PCs), and the 10EC5280 has been seen in the
> >> wild, as described here:
> >> https://lore.kernel.org/all/CAFqHKTm2WRNkcSoBEE=oNbfu_9d9RagQHLydmv6q1=snO_MXyA@mail.gmail.com/
> >> 
> >> I have not personally seen any devices using BMI0260, but I'll add
> >> comments to the BMI0160 and 10EC5280 entries with some examples of
> >> devices that use those IDs.  
> >
> > Drop any we don't have evidence are out there.
> >
> > Do we have any confirmation from Bosch (or products in the wild) for
> > the structurally correct BOSC0160 etc?  Those would normally have
> > to be tracked by Bosch as allocated for this purpose.
> >  
> 
> I haven't seen any reported, but the Windows driver INF has all five of
> these entries listed. I don't see any evidence of the BOSC0160 or
> BOSC0260 being used other than that Windows driver file.

Ok.  Lets leave the extras out for now and see if anyone screams.

> 
> BMI0160 seems by far the most common, with some appearances of 10EC5280
> (some AYANEO devices, possibly some GPD Win Max 2 devices) and BMI0260
> (OrangePi NEO).
> 
> >>   
> >> >> +	{ "10EC5280", (kernel_ulong_t)&bmi270_chip_info[BMI260] },    
> >> >
> >> > What's this one?  There is no such vendor ID.
> >> >    
> >>  
> > ...
> >  
> >> >>  
> >> >>  static const struct of_device_id bmi270_of_match[] = {
> >> >> -	{ .compatible = "bosch,bmi270" },
> >> >> +	{ .compatible = "bosch,bmi270", .data = &bmi270_chip_info[BMI270] },    
> >> >
> >> > If the bmi260 supports SPI, should be added here as well. (I've no idea if it does!)
> >> >
> >> > Or is this because you can't test it?    
> >> 
> >> Yeah, it was because I can't test it, the BMI260 does support SPI. I can
> >> add entries here, though.
> >> 
> >> Should the ACPI match entries from I2C also go here? All of the devices
> >> with mismatched IDs seem to use I2C so there might not be as much of a
> >> problem here.  
> > We want the incorrect formatted ones to be as hard to use as possible to discourage
> > them going into new products.  Can't do anything to solve the i2c cases
> > but definitely don't want to allow them for SPI as well if no evidence
> > of products where it yet matters.
> >
> > If we have confirmation from Bosch of the BOSC forms, then those I would like
> > in the SPI drivers as well (to point to the correct option for anyone using
> > this in future!)
> >
> > Jonathan
> >    
> 
> Agreed. Since we don't have confirmation of the correct values here or
> any that are in use, I would be OK either adding the single BOSC0260
> entry (as a guess, which may or may not be used) or leaving it out
> entirely until an entry is needed.
> 
Add that one as it might encourage anyone who happens to consider Linux
support to do this right...

If anyone has contacts at Bosch to moan at about their garbage use
of ACPI IDs in windows drivers, please do.

Jonathan

> Justin
> 
> >>   
> >> >>  	{ }
> >> >>  };
> >> >>      
> >> 
> >> Thanks again,
> >> Justin  

[PATCH 2/3] iio: imu: Add triggered buffer for Bosch BMI270 IMU

[Justin Weiss]

Set up a triggered buffer for the accel and angl_vel values.

Signed-off-by: Justin Weiss <justin@justinweiss.com>
---
 drivers/iio/imu/bmi270/Kconfig       |  1 +
 drivers/iio/imu/bmi270/bmi270.h      |  8 +++++
 drivers/iio/imu/bmi270/bmi270_core.c | 47 ++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+)

diff --git a/drivers/iio/imu/bmi270/Kconfig b/drivers/iio/imu/bmi270/Kconfig
index 0ffd29794fda..6362acc706da 100644
--- a/drivers/iio/imu/bmi270/Kconfig
+++ b/drivers/iio/imu/bmi270/Kconfig
@@ -6,6 +6,7 @@
 config BMI270
 	tristate
 	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
 
 config BMI270_I2C
 	tristate "Bosch BMI270 I2C driver"
diff --git a/drivers/iio/imu/bmi270/bmi270.h b/drivers/iio/imu/bmi270/bmi270.h
index 51e374fd4290..335400c34b0d 100644
--- a/drivers/iio/imu/bmi270/bmi270.h
+++ b/drivers/iio/imu/bmi270/bmi270.h
@@ -11,6 +11,14 @@ struct bmi270_data {
 	struct device *dev;
 	struct regmap *regmap;
 	const struct bmi270_chip_info *chip_info;
+
+	/*
+	 * Ensure natural alignment for timestamp if present.
+	 * Max length needed: 2 * 3 channels + 4 bytes padding + 8 byte ts.
+	 * If fewer channels are enabled, less space may be needed, as
+	 * long as the timestamp is still aligned to 8 bytes.
+	 */
+	__le16 buf[12] __aligned(8);
 };
 
 enum bmi270_device_type {
diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c
index e5ee80c12166..f49db5d1bffd 100644
--- a/drivers/iio/imu/bmi270/bmi270_core.c
+++ b/drivers/iio/imu/bmi270/bmi270_core.c
@@ -7,6 +7,8 @@
 #include <linux/regmap.h>
 
 #include <linux/iio/iio.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
 
 #include "bmi270.h"
 
@@ -66,6 +68,7 @@ enum bmi270_scan {
 	BMI270_SCAN_GYRO_X,
 	BMI270_SCAN_GYRO_Y,
 	BMI270_SCAN_GYRO_Z,
+	BMI270_SCAN_TIMESTAMP,
 };
 
 const struct bmi270_chip_info bmi270_chip_info[] = {
@@ -82,6 +85,29 @@ const struct bmi270_chip_info bmi270_chip_info[] = {
 };
 EXPORT_SYMBOL_NS_GPL(bmi270_chip_info, IIO_BMI270);
 
+static irqreturn_t bmi270_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct bmi270_data *bmi270_device = iio_priv(indio_dev);
+	int i, ret, j = 0, base = BMI270_ACCEL_X_REG;
+	__le16 sample;
+
+	for_each_set_bit(i, indio_dev->active_scan_mask, indio_dev->masklength) {
+		ret = regmap_bulk_read(bmi270_device->regmap,
+				       base + i * sizeof(sample),
+				       &sample, sizeof(sample));
+		if (ret)
+			goto done;
+		bmi270_device->buf[j++] = sample;
+	}
+
+	iio_push_to_buffers_with_timestamp(indio_dev, bmi270_device->buf, pf->timestamp);
+done:
+	iio_trigger_notify_done(indio_dev->trig);
+	return IRQ_HANDLED;
+}
+
 static int bmi270_get_data(struct bmi270_data *bmi270_device,
 			   int chan_type, int axis, int *val)
 {
@@ -139,6 +165,13 @@ static const struct iio_info bmi270_info = {
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
 	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |	\
 		BIT(IIO_CHAN_INFO_FREQUENCY),			\
+	.scan_index = BMI270_SCAN_ACCEL_##_axis,		\
+	.scan_type = {						\
+		.sign = 's',					\
+		.realbits = 16,				\
+		.storagebits = 16,				\
+		.endianness = IIO_LE				\
+	},	                                                \
 }
 
 #define BMI270_ANG_VEL_CHANNEL(_axis) {				\
@@ -148,6 +181,13 @@ static const struct iio_info bmi270_info = {
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
 	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |	\
 		BIT(IIO_CHAN_INFO_FREQUENCY),			\
+	.scan_index = BMI270_SCAN_GYRO_##_axis,		\
+	.scan_type = {						\
+		.sign = 's',					\
+		.realbits = 16,				\
+		.storagebits = 16,				\
+		.endianness = IIO_LE				\
+	},	                                                \
 }
 
 static const struct iio_chan_spec bmi270_channels[] = {
@@ -157,6 +197,7 @@ static const struct iio_chan_spec bmi270_channels[] = {
 	BMI270_ANG_VEL_CHANNEL(X),
 	BMI270_ANG_VEL_CHANNEL(Y),
 	BMI270_ANG_VEL_CHANNEL(Z),
+	IIO_CHAN_SOFT_TIMESTAMP(BMI270_SCAN_TIMESTAMP),
 };
 
 static int bmi270_validate_chip_id(struct bmi270_data *bmi270_device)
@@ -317,6 +358,12 @@ int bmi270_core_probe(struct device *dev, struct regmap *regmap,
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &bmi270_info;
 
+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
+					      iio_pollfunc_store_time,
+					      bmi270_trigger_handler, NULL);
+	if (ret)
+		return ret;
+
 	return devm_iio_device_register(dev, indio_dev);
 }
 EXPORT_SYMBOL_NS_GPL(bmi270_core_probe, IIO_BMI270);
-- 
2.47.0

Re: [PATCH 2/3] iio: imu: Add triggered buffer for Bosch BMI270 IMU

[Jonathan Cameron]

On Fri, 11 Oct 2024 08:37:48 -0700
Justin Weiss <justin@justinweiss.com> wrote:

> Set up a triggered buffer for the accel and angl_vel values.
> 
> Signed-off-by: Justin Weiss <justin@justinweiss.com>
Hi Justin

A few suggestions inline. Other than the DMA safe buffer thing, looks good
but you might want to consider using a single bulk read.

My cynical view is that if someone paid for an IMU they probably want all
the channels, so optimizing for that case is a good plan.

> ---
>  drivers/iio/imu/bmi270/Kconfig       |  1 +
>  drivers/iio/imu/bmi270/bmi270.h      |  8 +++++
>  drivers/iio/imu/bmi270/bmi270_core.c | 47 ++++++++++++++++++++++++++++
>  3 files changed, 56 insertions(+)
> 
> diff --git a/drivers/iio/imu/bmi270/Kconfig b/drivers/iio/imu/bmi270/Kconfig
> index 0ffd29794fda..6362acc706da 100644
> --- a/drivers/iio/imu/bmi270/Kconfig
> +++ b/drivers/iio/imu/bmi270/Kconfig
> @@ -6,6 +6,7 @@
>  config BMI270
>  	tristate
>  	select IIO_BUFFER

Hmm. The IIO_BUFFER select shouldn't have been here as no obvious use
in the driver. Ah well - this patch 'fixes' that :)

> +	select IIO_TRIGGERED_BUFFER
>  
>  config BMI270_I2C
>  	tristate "Bosch BMI270 I2C driver"
> diff --git a/drivers/iio/imu/bmi270/bmi270.h b/drivers/iio/imu/bmi270/bmi270.h
> index 51e374fd4290..335400c34b0d 100644
> --- a/drivers/iio/imu/bmi270/bmi270.h
> +++ b/drivers/iio/imu/bmi270/bmi270.h
> @@ -11,6 +11,14 @@ struct bmi270_data {
>  	struct device *dev;
>  	struct regmap *regmap;
>  	const struct bmi270_chip_info *chip_info;
> +
> +	/*
> +	 * Ensure natural alignment for timestamp if present.
> +	 * Max length needed: 2 * 3 channels + 4 bytes padding + 8 byte ts.
> +	 * If fewer channels are enabled, less space may be needed, as
> +	 * long as the timestamp is still aligned to 8 bytes.
> +	 */
> +	__le16 buf[12] __aligned(8);
>  };
>  
>  enum bmi270_device_type {
> diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c
> index e5ee80c12166..f49db5d1bffd 100644
> --- a/drivers/iio/imu/bmi270/bmi270_core.c
> +++ b/drivers/iio/imu/bmi270/bmi270_core.c
> @@ -7,6 +7,8 @@
>  #include <linux/regmap.h>
>  
>  #include <linux/iio/iio.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
>  
>  #include "bmi270.h"
>  
> @@ -66,6 +68,7 @@ enum bmi270_scan {
>  	BMI270_SCAN_GYRO_X,
>  	BMI270_SCAN_GYRO_Y,
>  	BMI270_SCAN_GYRO_Z,
> +	BMI270_SCAN_TIMESTAMP,
>  };
>  
>  const struct bmi270_chip_info bmi270_chip_info[] = {
> @@ -82,6 +85,29 @@ const struct bmi270_chip_info bmi270_chip_info[] = {
>  };
>  EXPORT_SYMBOL_NS_GPL(bmi270_chip_info, IIO_BMI270);
>  
> +static irqreturn_t bmi270_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct bmi270_data *bmi270_device = iio_priv(indio_dev);
> +	int i, ret, j = 0, base = BMI270_ACCEL_X_REG;
> +	__le16 sample;
> +
> +	for_each_set_bit(i, indio_dev->active_scan_mask, indio_dev->masklength) {
> +		ret = regmap_bulk_read(bmi270_device->regmap,
> +				       base + i * sizeof(sample),
> +				       &sample, sizeof(sample));

This is always a fun corner.
regmap doesn't guarantee to bounce buffer the data used by the underlying
transport. In the case of SPI that means we need a DMA safe buffer for bulk
accesses.  In practice it may well bounce the data today but there are optmizations
that would make it zero copy that might get applied in future.

Easiest way to do that is put your sample variable in the iio_priv structure
at the end and mark it __aligned(IIO_DMA_MINALIGN)

Given you are reading a bunch of contiguous registers here it may well make
sense to do a single bulk read directly into buf and then use
the available_scan_masks to let the IIO core know it always gets a full set
of samples. Then if the user selects a subset the IIO core will reorganize
the data that they get presented with.

Whether that makes sense from a performance point of view depends on
the speed of the spi transfers vs the cost of setting up the individual ones.

You could optimize contiguous reads in here, but probably not worth that
complexity.


> +		if (ret)
> +			goto done;
> +		bmi270_device->buf[j++] = sample;

It's not a huge buffer and you aren't DMAing into it, so maybe just put this
on the stack?

> +	}
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, bmi270_device->buf, pf->timestamp);
> +done:
> +	iio_trigger_notify_done(indio_dev->trig);
> +	return IRQ_HANDLED;
> +}

Re: [PATCH 2/3] iio: imu: Add triggered buffer for Bosch BMI270 IMU

[Justin Weiss]

Jonathan Cameron <jic23@kernel.org> writes:

> On Fri, 11 Oct 2024 08:37:48 -0700
> Justin Weiss <justin@justinweiss.com> wrote:
>
>> Set up a triggered buffer for the accel and angl_vel values.
>> 
>> Signed-off-by: Justin Weiss <justin@justinweiss.com>
> Hi Justin
>
> A few suggestions inline. Other than the DMA safe buffer thing, looks good
> but you might want to consider using a single bulk read.
>
> My cynical view is that if someone paid for an IMU they probably want all
> the channels, so optimizing for that case is a good plan.
>
>> ---
>>  drivers/iio/imu/bmi270/Kconfig       |  1 +
>>  drivers/iio/imu/bmi270/bmi270.h      |  8 +++++
>>  drivers/iio/imu/bmi270/bmi270_core.c | 47 ++++++++++++++++++++++++++++
>>  3 files changed, 56 insertions(+)
>> 
>> diff --git a/drivers/iio/imu/bmi270/Kconfig b/drivers/iio/imu/bmi270/Kconfig
>> index 0ffd29794fda..6362acc706da 100644
>> --- a/drivers/iio/imu/bmi270/Kconfig
>> +++ b/drivers/iio/imu/bmi270/Kconfig
>> @@ -6,6 +6,7 @@
>>  config BMI270
>>  	tristate
>>  	select IIO_BUFFER
>
> Hmm. The IIO_BUFFER select shouldn't have been here as no obvious use
> in the driver. Ah well - this patch 'fixes' that :)
>
>> +	select IIO_TRIGGERED_BUFFER
>>  
>>  config BMI270_I2C
>>  	tristate "Bosch BMI270 I2C driver"
>> diff --git a/drivers/iio/imu/bmi270/bmi270.h b/drivers/iio/imu/bmi270/bmi270.h
>> index 51e374fd4290..335400c34b0d 100644
>> --- a/drivers/iio/imu/bmi270/bmi270.h
>> +++ b/drivers/iio/imu/bmi270/bmi270.h
>> @@ -11,6 +11,14 @@ struct bmi270_data {
>>  	struct device *dev;
>>  	struct regmap *regmap;
>>  	const struct bmi270_chip_info *chip_info;
>> +
>> +	/*
>> +	 * Ensure natural alignment for timestamp if present.
>> +	 * Max length needed: 2 * 3 channels + 4 bytes padding + 8 byte ts.
>> +	 * If fewer channels are enabled, less space may be needed, as
>> +	 * long as the timestamp is still aligned to 8 bytes.
>> +	 */
>> +	__le16 buf[12] __aligned(8);
>>  };
>>  
>>  enum bmi270_device_type {
>> diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c
>> index e5ee80c12166..f49db5d1bffd 100644
>> --- a/drivers/iio/imu/bmi270/bmi270_core.c
>> +++ b/drivers/iio/imu/bmi270/bmi270_core.c
>> @@ -7,6 +7,8 @@
>>  #include <linux/regmap.h>
>>  
>>  #include <linux/iio/iio.h>
>> +#include <linux/iio/triggered_buffer.h>
>> +#include <linux/iio/trigger_consumer.h>
>>  
>>  #include "bmi270.h"
>>  
>> @@ -66,6 +68,7 @@ enum bmi270_scan {
>>  	BMI270_SCAN_GYRO_X,
>>  	BMI270_SCAN_GYRO_Y,
>>  	BMI270_SCAN_GYRO_Z,
>> +	BMI270_SCAN_TIMESTAMP,
>>  };
>>  
>>  const struct bmi270_chip_info bmi270_chip_info[] = {
>> @@ -82,6 +85,29 @@ const struct bmi270_chip_info bmi270_chip_info[] = {
>>  };
>>  EXPORT_SYMBOL_NS_GPL(bmi270_chip_info, IIO_BMI270);
>>  
>> +static irqreturn_t bmi270_trigger_handler(int irq, void *p)
>> +{
>> +	struct iio_poll_func *pf = p;
>> +	struct iio_dev *indio_dev = pf->indio_dev;
>> +	struct bmi270_data *bmi270_device = iio_priv(indio_dev);
>> +	int i, ret, j = 0, base = BMI270_ACCEL_X_REG;
>> +	__le16 sample;
>> +
>> +	for_each_set_bit(i, indio_dev->active_scan_mask, indio_dev->masklength) {
>> +		ret = regmap_bulk_read(bmi270_device->regmap,
>> +				       base + i * sizeof(sample),
>> +				       &sample, sizeof(sample));
>
> This is always a fun corner.
> regmap doesn't guarantee to bounce buffer the data used by the underlying
> transport. In the case of SPI that means we need a DMA safe buffer for bulk
> accesses.  In practice it may well bounce the data today but there are optmizations
> that would make it zero copy that might get applied in future.
>
> Easiest way to do that is put your sample variable in the iio_priv structure
> at the end and mark it __aligned(IIO_DMA_MINALIGN)
>
> Given you are reading a bunch of contiguous registers here it may well make
> sense to do a single bulk read directly into buf and then use
> the available_scan_masks to let the IIO core know it always gets a full set
> of samples. Then if the user selects a subset the IIO core will reorganize
> the data that they get presented with.

That's convenient :-)

It should make this much simpler. To clarify, I'll use regmap_bulk_read
to read all of the registers at once into a stack-allocated buffer, and
then push that buffer. Then I can remove bmi270_device->buf entirely,
and avoid the DMA problem that way.

Then I'll provide one avail_mask that sets all of the
BMI270_SCAN_ACCEL_* and BMI270_SCAN_GYRO_* bits.

> Whether that makes sense from a performance point of view depends on
> the speed of the spi transfers vs the cost of setting up the individual ones.
>
> You could optimize contiguous reads in here, but probably not worth that
> complexity.
>
>> +		if (ret)
>> +			goto done;
>> +		bmi270_device->buf[j++] = sample;
>
> It's not a huge buffer and you aren't DMAing into it, so maybe just put this
> on the stack?
>
>> +	}
>> +
>> +	iio_push_to_buffers_with_timestamp(indio_dev, bmi270_device->buf, pf->timestamp);
>> +done:
>> +	iio_trigger_notify_done(indio_dev->trig);
>> +	return IRQ_HANDLED;
>> +}

Re: [PATCH 2/3] iio: imu: Add triggered buffer for Bosch BMI270 IMU

[Jonathan Cameron]

On Sat, 12 Oct 2024 19:43:19 -0700
Justin Weiss <justin@justinweiss.com> wrote:

> Jonathan Cameron <jic23@kernel.org> writes:
> 
> > On Fri, 11 Oct 2024 08:37:48 -0700
> > Justin Weiss <justin@justinweiss.com> wrote:
> >  
> >> Set up a triggered buffer for the accel and angl_vel values.
> >> 
> >> Signed-off-by: Justin Weiss <justin@justinweiss.com>  
> > Hi Justin
> >
> > A few suggestions inline. Other than the DMA safe buffer thing, looks good
> > but you might want to consider using a single bulk read.
> >
> > My cynical view is that if someone paid for an IMU they probably want all
> > the channels, so optimizing for that case is a good plan.
> >  
> >> ---
> >>  drivers/iio/imu/bmi270/Kconfig       |  1 +
> >>  drivers/iio/imu/bmi270/bmi270.h      |  8 +++++
> >>  drivers/iio/imu/bmi270/bmi270_core.c | 47 ++++++++++++++++++++++++++++
> >>  3 files changed, 56 insertions(+)
> >> 
> >> diff --git a/drivers/iio/imu/bmi270/Kconfig b/drivers/iio/imu/bmi270/Kconfig
> >> index 0ffd29794fda..6362acc706da 100644
> >> --- a/drivers/iio/imu/bmi270/Kconfig
> >> +++ b/drivers/iio/imu/bmi270/Kconfig
> >> @@ -6,6 +6,7 @@
> >>  config BMI270
> >>  	tristate
> >>  	select IIO_BUFFER  
> >
> > Hmm. The IIO_BUFFER select shouldn't have been here as no obvious use
> > in the driver. Ah well - this patch 'fixes' that :)
> >  
> >> +	select IIO_TRIGGERED_BUFFER
> >>  
> >>  config BMI270_I2C
> >>  	tristate "Bosch BMI270 I2C driver"
> >> diff --git a/drivers/iio/imu/bmi270/bmi270.h b/drivers/iio/imu/bmi270/bmi270.h
> >> index 51e374fd4290..335400c34b0d 100644
> >> --- a/drivers/iio/imu/bmi270/bmi270.h
> >> +++ b/drivers/iio/imu/bmi270/bmi270.h
> >> @@ -11,6 +11,14 @@ struct bmi270_data {
> >>  	struct device *dev;
> >>  	struct regmap *regmap;
> >>  	const struct bmi270_chip_info *chip_info;
> >> +
> >> +	/*
> >> +	 * Ensure natural alignment for timestamp if present.
> >> +	 * Max length needed: 2 * 3 channels + 4 bytes padding + 8 byte ts.
> >> +	 * If fewer channels are enabled, less space may be needed, as
> >> +	 * long as the timestamp is still aligned to 8 bytes.
> >> +	 */
> >> +	__le16 buf[12] __aligned(8);
> >>  };
> >>  
> >>  enum bmi270_device_type {
> >> diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c
> >> index e5ee80c12166..f49db5d1bffd 100644
> >> --- a/drivers/iio/imu/bmi270/bmi270_core.c
> >> +++ b/drivers/iio/imu/bmi270/bmi270_core.c
> >> @@ -7,6 +7,8 @@
> >>  #include <linux/regmap.h>
> >>  
> >>  #include <linux/iio/iio.h>
> >> +#include <linux/iio/triggered_buffer.h>
> >> +#include <linux/iio/trigger_consumer.h>
> >>  
> >>  #include "bmi270.h"
> >>  
> >> @@ -66,6 +68,7 @@ enum bmi270_scan {
> >>  	BMI270_SCAN_GYRO_X,
> >>  	BMI270_SCAN_GYRO_Y,
> >>  	BMI270_SCAN_GYRO_Z,
> >> +	BMI270_SCAN_TIMESTAMP,
> >>  };
> >>  
> >>  const struct bmi270_chip_info bmi270_chip_info[] = {
> >> @@ -82,6 +85,29 @@ const struct bmi270_chip_info bmi270_chip_info[] = {
> >>  };
> >>  EXPORT_SYMBOL_NS_GPL(bmi270_chip_info, IIO_BMI270);
> >>  
> >> +static irqreturn_t bmi270_trigger_handler(int irq, void *p)
> >> +{
> >> +	struct iio_poll_func *pf = p;
> >> +	struct iio_dev *indio_dev = pf->indio_dev;
> >> +	struct bmi270_data *bmi270_device = iio_priv(indio_dev);
> >> +	int i, ret, j = 0, base = BMI270_ACCEL_X_REG;
> >> +	__le16 sample;
> >> +
> >> +	for_each_set_bit(i, indio_dev->active_scan_mask, indio_dev->masklength) {
> >> +		ret = regmap_bulk_read(bmi270_device->regmap,
> >> +				       base + i * sizeof(sample),
> >> +				       &sample, sizeof(sample));  
> >
> > This is always a fun corner.
> > regmap doesn't guarantee to bounce buffer the data used by the underlying
> > transport. In the case of SPI that means we need a DMA safe buffer for bulk
> > accesses.  In practice it may well bounce the data today but there are optmizations
> > that would make it zero copy that might get applied in future.
> >
> > Easiest way to do that is put your sample variable in the iio_priv structure
> > at the end and mark it __aligned(IIO_DMA_MINALIGN)
> >
> > Given you are reading a bunch of contiguous registers here it may well make
> > sense to do a single bulk read directly into buf and then use
> > the available_scan_masks to let the IIO core know it always gets a full set
> > of samples. Then if the user selects a subset the IIO core will reorganize
> > the data that they get presented with.  
> 
> That's convenient :-)
> 
> It should make this much simpler. To clarify, I'll use regmap_bulk_read
> to read all of the registers at once into a stack-allocated buffer, and
> then push that buffer. Then I can remove bmi270_device->buf entirely,
> and avoid the DMA problem that way.

Given this supports SPI. The target buffer can't be on the stack.
You still need the __aligned(IIO_DMA_MINALIGN) element in your iio_priv()
structure.

> 
> Then I'll provide one avail_mask that sets all of the
> BMI270_SCAN_ACCEL_* and BMI270_SCAN_GYRO_* bits.
Otherwise your description is what I'd expect to see.

> 
> > Whether that makes sense from a performance point of view depends on
> > the speed of the spi transfers vs the cost of setting up the individual ones.
> >
> > You could optimize contiguous reads in here, but probably not worth that
> > complexity.
> >  
> >> +		if (ret)
> >> +			goto done;
> >> +		bmi270_device->buf[j++] = sample;  
> >
> > It's not a huge buffer and you aren't DMAing into it, so maybe just put this
> > on the stack?
This would only be correct for the case where you aren't DMAing directly into it.
I guess I confused the message above with this point!

Jonathan

> >  
> >> +	}
> >> +
> >> +	iio_push_to_buffers_with_timestamp(indio_dev, bmi270_device->buf, pf->timestamp);
> >> +done:
> >> +	iio_trigger_notify_done(indio_dev->trig);
> >> +	return IRQ_HANDLED;
> >> +}  

Re: [PATCH 2/3] iio: imu: Add triggered buffer for Bosch BMI270 IMU

[Justin Weiss]

Jonathan Cameron <jic23@kernel.org> writes:

> On Sat, 12 Oct 2024 19:43:19 -0700
> Justin Weiss <justin@justinweiss.com> wrote:
>
>> Jonathan Cameron <jic23@kernel.org> writes:
>> 
>> > On Fri, 11 Oct 2024 08:37:48 -0700
>> > Justin Weiss <justin@justinweiss.com> wrote:
>> >  
>> >> Set up a triggered buffer for the accel and angl_vel values.
>> >> 
>> >> Signed-off-by: Justin Weiss <justin@justinweiss.com>  
>> > Hi Justin
>> >
>> > A few suggestions inline. Other than the DMA safe buffer thing, looks good
>> > but you might want to consider using a single bulk read.
>> >
>> > My cynical view is that if someone paid for an IMU they probably want all
>> > the channels, so optimizing for that case is a good plan.
>> >  
>> >> ...
>> >> 
>> >> +	__le16 sample;
>> >> +
>> >> +	for_each_set_bit(i, indio_dev->active_scan_mask, indio_dev->masklength) {
>> >> +		ret = regmap_bulk_read(bmi270_device->regmap,
>> >> +				       base + i * sizeof(sample),
>> >> +				       &sample, sizeof(sample));  
>> >
>> > This is always a fun corner.
>> > regmap doesn't guarantee to bounce buffer the data used by the underlying
>> > transport. In the case of SPI that means we need a DMA safe buffer for bulk
>> > accesses.  In practice it may well bounce the data today but there are optmizations
>> > that would make it zero copy that might get applied in future.
>> >
>> > Easiest way to do that is put your sample variable in the iio_priv structure
>> > at the end and mark it __aligned(IIO_DMA_MINALIGN)
>> >
>> > Given you are reading a bunch of contiguous registers here it may well make
>> > sense to do a single bulk read directly into buf and then use
>> > the available_scan_masks to let the IIO core know it always gets a full set
>> > of samples. Then if the user selects a subset the IIO core will reorganize
>> > the data that they get presented with.  
>> 
>> That's convenient :-)
>> 
>> It should make this much simpler. To clarify, I'll use regmap_bulk_read
>> to read all of the registers at once into a stack-allocated buffer, and
>> then push that buffer. Then I can remove bmi270_device->buf entirely,
>> and avoid the DMA problem that way.
>
> Given this supports SPI. The target buffer can't be on the stack.
> You still need the __aligned(IIO_DMA_MINALIGN) element in your iio_priv()
> structure.
>

Got it. I see that the BMI323 driver does the regmap_read into the
DMA-aligned buffer, and then copies it to the timestamp-aligned buffer,
which it then sends to iio_push_to_buffers_with_timestamp. Is that a
good way to handle this?

I think the timestamp-aligned buffer could still be stack-allocated in
that case. Or maybe a second buffer isn't even necessary, if
DMA_MINALIGN is at least the correct alignment for
iio_push_to_buffers_with_timestamp and I could pass the DMA-aligned
buffer in.

Justin

>> 
>> Then I'll provide one avail_mask that sets all of the
>> BMI270_SCAN_ACCEL_* and BMI270_SCAN_GYRO_* bits.
> Otherwise your description is what I'd expect to see.
>
>> 
>> > Whether that makes sense from a performance point of view depends on
>> > the speed of the spi transfers vs the cost of setting up the individual ones.
>> >
>> > You could optimize contiguous reads in here, but probably not worth that
>> > complexity.
>> >  
>> >> +		if (ret)
>> >> +			goto done;
>> >> +		bmi270_device->buf[j++] = sample;  
>> >
>> > It's not a huge buffer and you aren't DMAing into it, so maybe just put this
>> > on the stack?
> This would only be correct for the case where you aren't DMAing directly into it.
> I guess I confused the message above with this point!
>
> Jonathan
>
>> >  
>> >> +	}
>> >> +
>> >> +	iio_push_to_buffers_with_timestamp(indio_dev, bmi270_device->buf, pf->timestamp);
>> >> +done:
>> >> +	iio_trigger_notify_done(indio_dev->trig);
>> >> +	return IRQ_HANDLED;
>> >> +}  

Re: [PATCH 2/3] iio: imu: Add triggered buffer for Bosch BMI270 IMU

[Jonathan Cameron]

On Sun, 13 Oct 2024 13:54:36 -0700
Justin Weiss <justin@justinweiss.com> wrote:

> Jonathan Cameron <jic23@kernel.org> writes:
> 
> > On Sat, 12 Oct 2024 19:43:19 -0700
> > Justin Weiss <justin@justinweiss.com> wrote:
> >  
> >> Jonathan Cameron <jic23@kernel.org> writes:
> >>   
> >> > On Fri, 11 Oct 2024 08:37:48 -0700
> >> > Justin Weiss <justin@justinweiss.com> wrote:
> >> >    
> >> >> Set up a triggered buffer for the accel and angl_vel values.
> >> >> 
> >> >> Signed-off-by: Justin Weiss <justin@justinweiss.com>    
> >> > Hi Justin
> >> >
> >> > A few suggestions inline. Other than the DMA safe buffer thing, looks good
> >> > but you might want to consider using a single bulk read.
> >> >
> >> > My cynical view is that if someone paid for an IMU they probably want all
> >> > the channels, so optimizing for that case is a good plan.
> >> >    
> >> >> ...
> >> >> 
> >> >> +	__le16 sample;
> >> >> +
> >> >> +	for_each_set_bit(i, indio_dev->active_scan_mask, indio_dev->masklength) {
> >> >> +		ret = regmap_bulk_read(bmi270_device->regmap,
> >> >> +				       base + i * sizeof(sample),
> >> >> +				       &sample, sizeof(sample));    
> >> >
> >> > This is always a fun corner.
> >> > regmap doesn't guarantee to bounce buffer the data used by the underlying
> >> > transport. In the case of SPI that means we need a DMA safe buffer for bulk
> >> > accesses.  In practice it may well bounce the data today but there are optmizations
> >> > that would make it zero copy that might get applied in future.
> >> >
> >> > Easiest way to do that is put your sample variable in the iio_priv structure
> >> > at the end and mark it __aligned(IIO_DMA_MINALIGN)
> >> >
> >> > Given you are reading a bunch of contiguous registers here it may well make
> >> > sense to do a single bulk read directly into buf and then use
> >> > the available_scan_masks to let the IIO core know it always gets a full set
> >> > of samples. Then if the user selects a subset the IIO core will reorganize
> >> > the data that they get presented with.    
> >> 
> >> That's convenient :-)
> >> 
> >> It should make this much simpler. To clarify, I'll use regmap_bulk_read
> >> to read all of the registers at once into a stack-allocated buffer, and
> >> then push that buffer. Then I can remove bmi270_device->buf entirely,
> >> and avoid the DMA problem that way.  
> >
> > Given this supports SPI. The target buffer can't be on the stack.
> > You still need the __aligned(IIO_DMA_MINALIGN) element in your iio_priv()
> > structure.
> >  
> 
> Got it. I see that the BMI323 driver does the regmap_read into the
> DMA-aligned buffer, and then copies it to the timestamp-aligned buffer,
> which it then sends to iio_push_to_buffers_with_timestamp. Is that a
> good way to handle this?

Yes.  We don't have a zero copy path so that's the best we can do.

> 
> I think the timestamp-aligned buffer could still be stack-allocated in
> that case.

No it can't.  __aligned doesn't do anything useful for us on the stack
as we don't control what comes after it.  You could in theory force alignment
and pad - as long as compilers now respect __aligned  for this case (they
didn't use to!)  It's potentially a few hundred bytes on the stack, so
better on the heap.

> Or maybe a second buffer isn't even necessary, if
> DMA_MINALIGN is at least the correct alignment for
> iio_push_to_buffers_with_timestamp and I could pass the DMA-aligned
> buffer in.

Common trick is to just DMA into almost what you already have in bmi270_data.
But now the timestamp is in a fixed place you can use a structure to handle
the alignment for you.
	/*
+	 * Where IIO_DMA_MINALIGN is larger than 8 bytes, align to that
+	 * to ensure a DMA safe buffer.
+	 */
+	//__le16 buf[12] __aligned(IIO_DMA_MINALIGN);
	struct {
		__le16 channels[6];
		aligned_s64 timestamp; //type is only in iio tree currently.
	} data __aligned(IIO_DMA_MINALIGN);

The aligned_s64 is needed for x86_32 where s64 is only aligned to 4 bytes
whereas IIO assume natural alignment of everything (so 8 bytes).
 };

Then pass that to the push_to_buffers call just as you currently do.

Jonathan


 

[PATCH 3/3] iio: imu: Add scale and sampling frequency to BMI270 IMU

[Justin Weiss]

Add read and write functions and create _available entries. Use
IIO_CHAN_INFO_SAMP_FREQ instead of IIO_CHAN_INFO_FREQUENCY to match
the BMI160 / BMI323 drivers.

Signed-off-by: Justin Weiss <justin@justinweiss.com>
---
 drivers/iio/imu/bmi270/bmi270_core.c | 293 ++++++++++++++++++++++++++-
 1 file changed, 291 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c
index f49db5d1bffd..ce7873dc3211 100644
--- a/drivers/iio/imu/bmi270/bmi270_core.c
+++ b/drivers/iio/imu/bmi270/bmi270_core.c
@@ -7,6 +7,7 @@
 #include <linux/regmap.h>
 
 #include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
 #include <linux/iio/triggered_buffer.h>
 #include <linux/iio/trigger_consumer.h>
 
@@ -34,6 +35,9 @@
 #define BMI270_ACC_CONF_BWP_NORMAL_MODE			0x02
 #define BMI270_ACC_CONF_FILTER_PERF_MSK			BIT(7)
 
+#define BMI270_ACC_CONF_RANGE_REG			0x41
+#define BMI270_ACC_CONF_RANGE_MSK			GENMASK(1, 0)
+
 #define BMI270_GYR_CONF_REG				0x42
 #define BMI270_GYR_CONF_ODR_MSK				GENMASK(3, 0)
 #define BMI270_GYR_CONF_ODR_200HZ			0x09
@@ -42,6 +46,9 @@
 #define BMI270_GYR_CONF_NOISE_PERF_MSK			BIT(6)
 #define BMI270_GYR_CONF_FILTER_PERF_MSK			BIT(7)
 
+#define BMI270_GYR_CONF_RANGE_REG			0x43
+#define BMI270_GYR_CONF_RANGE_MSK			GENMASK(2, 0)
+
 #define BMI270_INIT_CTRL_REG				0x59
 #define BMI270_INIT_CTRL_LOAD_DONE_MSK			BIT(0)
 
@@ -85,6 +92,236 @@ const struct bmi270_chip_info bmi270_chip_info[] = {
 };
 EXPORT_SYMBOL_NS_GPL(bmi270_chip_info, IIO_BMI270);
 
+enum bmi270_sensor_type {
+	BMI270_ACCEL	= 0,
+	BMI270_GYRO,
+};
+
+struct bmi270_scale {
+	u8 bits;
+	int uscale;
+};
+
+struct bmi270_odr {
+	u8 bits;
+	int odr;
+	int uodr;
+};
+
+static const struct bmi270_scale bmi270_accel_scale[] = {
+	{ 0x00, 598},
+	{ 0x01, 1197},
+	{ 0x02, 2394},
+	{ 0x03, 4788},
+};
+
+static const struct bmi270_scale bmi270_gyro_scale[] = {
+	{ 0x00, 1065},
+	{ 0x01, 532},
+	{ 0x02, 266},
+	{ 0x03, 133},
+	{ 0x04, 66},
+};
+
+struct bmi270_scale_item {
+	const struct bmi270_scale *tbl;
+	int num;
+};
+
+static const struct bmi270_scale_item bmi270_scale_table[] = {
+	[BMI270_ACCEL] = {
+		.tbl	= bmi270_accel_scale,
+		.num	= ARRAY_SIZE(bmi270_accel_scale),
+	},
+	[BMI270_GYRO] = {
+		.tbl	= bmi270_gyro_scale,
+		.num	= ARRAY_SIZE(bmi270_gyro_scale),
+	},
+};
+
+static const struct bmi270_odr bmi270_accel_odr[] = {
+	{0x01, 0, 781250},
+	{0x02, 1, 562500},
+	{0x03, 3, 125000},
+	{0x04, 6, 250000},
+	{0x05, 12, 500000},
+	{0x06, 25, 0},
+	{0x07, 50, 0},
+	{0x08, 100, 0},
+	{0x09, 200, 0},
+	{0x0A, 400, 0},
+	{0x0B, 800, 0},
+	{0x0C, 1600, 0},
+};
+
+static const struct bmi270_odr bmi270_gyro_odr[] = {
+	{0x06, 25, 0},
+	{0x07, 50, 0},
+	{0x08, 100, 0},
+	{0x09, 200, 0},
+	{0x0A, 400, 0},
+	{0x0B, 800, 0},
+	{0x0C, 1600, 0},
+	{0x0D, 3200, 0},
+};
+
+struct bmi270_odr_item {
+	const struct bmi270_odr *tbl;
+	int num;
+};
+
+static const struct  bmi270_odr_item bmi270_odr_table[] = {
+	[BMI270_ACCEL] = {
+		.tbl	= bmi270_accel_odr,
+		.num	= ARRAY_SIZE(bmi270_accel_odr),
+	},
+	[BMI270_GYRO] = {
+		.tbl	= bmi270_gyro_odr,
+		.num	= ARRAY_SIZE(bmi270_gyro_odr),
+	},
+};
+
+static int bmi270_set_scale(struct bmi270_data *data,
+			    int chan_type, int uscale)
+{
+	int i;
+	int reg;
+	struct bmi270_scale_item bmi270_scale_item;
+
+	switch (chan_type) {
+	case IIO_ACCEL:
+		reg = BMI270_ACC_CONF_RANGE_REG;
+		bmi270_scale_item = bmi270_scale_table[BMI270_ACCEL];
+		break;
+	case IIO_ANGL_VEL:
+		reg = BMI270_GYR_CONF_RANGE_REG;
+		bmi270_scale_item = bmi270_scale_table[BMI270_GYRO];
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	for (i = 0; i < bmi270_scale_item.num; i++)
+		if (bmi270_scale_item.tbl[i].uscale == uscale)
+			break;
+
+	if (i == bmi270_scale_item.num)
+		return -EINVAL;
+
+	return regmap_write(data->regmap, reg,
+			    bmi270_scale_item.tbl[i].bits);
+}
+
+static int bmi270_get_scale(struct bmi270_data *bmi270_device,
+			    int chan_type, int *uscale)
+{
+	int i, ret, val;
+	int reg;
+	struct bmi270_scale_item bmi270_scale_item;
+
+	switch (chan_type) {
+	case IIO_ACCEL:
+		reg = BMI270_ACC_CONF_RANGE_REG;
+		bmi270_scale_item = bmi270_scale_table[BMI270_ACCEL];
+		break;
+	case IIO_ANGL_VEL:
+		reg = BMI270_GYR_CONF_RANGE_REG;
+		bmi270_scale_item = bmi270_scale_table[BMI270_GYRO];
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = regmap_read(bmi270_device->regmap, reg, &val);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < bmi270_scale_item.num; i++)
+		if (bmi270_scale_item.tbl[i].bits == val) {
+			*uscale = bmi270_scale_item.tbl[i].uscale;
+			return 0;
+		}
+
+	return -EINVAL;
+}
+
+static int bmi270_set_odr(struct bmi270_data *data, int chan_type,
+			  int odr, int uodr)
+{
+	int i;
+	int reg, mask;
+	struct bmi270_odr_item bmi270_odr_item;
+
+	switch (chan_type) {
+	case IIO_ACCEL:
+		reg = BMI270_ACC_CONF_REG;
+		mask = BMI270_ACC_CONF_ODR_MSK;
+		bmi270_odr_item = bmi270_odr_table[BMI270_ACCEL];
+		break;
+	case IIO_ANGL_VEL:
+		reg = BMI270_GYR_CONF_REG;
+		mask = BMI270_GYR_CONF_ODR_MSK;
+		bmi270_odr_item = bmi270_odr_table[BMI270_GYRO];
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	for (i = 0; i < bmi270_odr_item.num; i++)
+		if (bmi270_odr_item.tbl[i].odr == odr &&
+		    bmi270_odr_item.tbl[i].uodr == uodr)
+			break;
+
+	if (i >= bmi270_odr_item.num)
+		return -EINVAL;
+
+	return regmap_update_bits(data->regmap,
+				  reg,
+				  mask,
+				  bmi270_odr_item.tbl[i].bits);
+}
+
+static int bmi270_get_odr(struct bmi270_data *data, int chan_type,
+			  int *odr, int *uodr)
+{
+	int i, val, ret;
+	int reg, mask;
+	struct bmi270_odr_item bmi270_odr_item;
+
+	switch (chan_type) {
+	case IIO_ACCEL:
+		reg = BMI270_ACC_CONF_REG;
+		mask = BMI270_ACC_CONF_ODR_MSK;
+		bmi270_odr_item = bmi270_odr_table[BMI270_ACCEL];
+		break;
+	case IIO_ANGL_VEL:
+		reg = BMI270_GYR_CONF_REG;
+		mask = BMI270_GYR_CONF_ODR_MSK;
+		bmi270_odr_item = bmi270_odr_table[BMI270_GYRO];
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = regmap_read(data->regmap, reg, &val);
+	if (ret)
+		return ret;
+
+	val &= mask;
+
+	for (i = 0; i < bmi270_odr_item.num; i++)
+		if (val == bmi270_odr_item.tbl[i].bits)
+			break;
+
+	if (i >= bmi270_odr_item.num)
+		return -EINVAL;
+
+	*odr = bmi270_odr_item.tbl[i].odr;
+	*uodr = bmi270_odr_item.tbl[i].uodr;
+
+	return 0;
+}
+
 static irqreturn_t bmi270_trigger_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;
@@ -149,13 +386,65 @@ static int bmi270_read_raw(struct iio_dev *indio_dev,
 			return ret;
 
 		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		*val = 0;
+		ret = bmi270_get_scale(bmi270_device, chan->type, val2);
+		return ret ? ret : IIO_VAL_INT_PLUS_MICRO;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		ret = bmi270_get_odr(bmi270_device, chan->type, val, val2);
+		return ret ? ret : IIO_VAL_INT_PLUS_MICRO;
 	default:
 		return -EINVAL;
 	}
 }
 
+static int bmi270_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val, int val2, long mask)
+{
+	struct bmi270_data *data = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		return bmi270_set_scale(data, chan->type, val2);
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		return bmi270_set_odr(data, chan->type, val, val2);
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static
+IIO_CONST_ATTR(in_accel_sampling_frequency_available,
+	       "0.78125 1.5625 3.125 6.25 12.5 25 50 100 200 400 800 1600");
+static
+IIO_CONST_ATTR(in_anglvel_sampling_frequency_available,
+	       "25 50 100 200 400 800 1600 3200");
+static
+IIO_CONST_ATTR(in_accel_scale_available,
+	       "0.000598 0.001197 0.002394 0.004788");
+static
+IIO_CONST_ATTR(in_anglvel_scale_available,
+	       "0.001065 0.000532 0.000266 0.000133 0.000066");
+
+static struct attribute *bmi270_attrs[] = {
+	&iio_const_attr_in_accel_sampling_frequency_available.dev_attr.attr,
+	&iio_const_attr_in_anglvel_sampling_frequency_available.dev_attr.attr,
+	&iio_const_attr_in_accel_scale_available.dev_attr.attr,
+	&iio_const_attr_in_anglvel_scale_available.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group bmi270_attrs_group = {
+	.attrs = bmi270_attrs,
+};
+
 static const struct iio_info bmi270_info = {
 	.read_raw = bmi270_read_raw,
+	.write_raw = bmi270_write_raw,
+	.attrs = &bmi270_attrs_group,
 };
 
 #define BMI270_ACCEL_CHANNEL(_axis) {				\
@@ -164,7 +453,7 @@ static const struct iio_info bmi270_info = {
 	.channel2 = IIO_MOD_##_axis,				\
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
 	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |	\
-		BIT(IIO_CHAN_INFO_FREQUENCY),			\
+		BIT(IIO_CHAN_INFO_SAMP_FREQ),			\
 	.scan_index = BMI270_SCAN_ACCEL_##_axis,		\
 	.scan_type = {						\
 		.sign = 's',					\
@@ -180,7 +469,7 @@ static const struct iio_info bmi270_info = {
 	.channel2 = IIO_MOD_##_axis,				\
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
 	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |	\
-		BIT(IIO_CHAN_INFO_FREQUENCY),			\
+		BIT(IIO_CHAN_INFO_SAMP_FREQ),			\
 	.scan_index = BMI270_SCAN_GYRO_##_axis,		\
 	.scan_type = {						\
 		.sign = 's',					\
-- 
2.47.0

Re: [PATCH 3/3] iio: imu: Add scale and sampling frequency to BMI270 IMU

[Jonathan Cameron]

On Fri, 11 Oct 2024 08:37:49 -0700
Justin Weiss <justin@justinweiss.com> wrote:

> Add read and write functions and create _available entries. Use
> IIO_CHAN_INFO_SAMP_FREQ instead of IIO_CHAN_INFO_FREQUENCY to match
> the BMI160 / BMI323 drivers.

Ah.  Please break dropping _FREQUENCY change out as a separate fix
with fixes tag etc and drag it to start of the patch. It was never
wired to anything anyway

That's a straight forward ABI bug so we want that to land ahead
of the rest of the series.

Does this device have a data ready interrupt and if so what affect
do the different ODRs for each type of sensor have on that?
If there are separate data ready signals, you probably want to 
go with a dual buffer setup from the start as it is hard to unwind
that later.

> 
> Signed-off-by: Justin Weiss <justin@justinweiss.com>
> ---
>  drivers/iio/imu/bmi270/bmi270_core.c | 293 ++++++++++++++++++++++++++-
>  1 file changed, 291 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c
> index f49db5d1bffd..ce7873dc3211 100644
> --- a/drivers/iio/imu/bmi270/bmi270_core.c
> +++ b/drivers/iio/imu/bmi270/bmi270_core.c
> @@ -7,6 +7,7 @@
>  #include <linux/regmap.h>
>  
>  #include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
>  #include <linux/iio/triggered_buffer.h>
>  #include <linux/iio/trigger_consumer.h>
>  
> @@ -34,6 +35,9 @@
>  #define BMI270_ACC_CONF_BWP_NORMAL_MODE			0x02
>  #define BMI270_ACC_CONF_FILTER_PERF_MSK			BIT(7)
>  
> +#define BMI270_ACC_CONF_RANGE_REG			0x41
> +#define BMI270_ACC_CONF_RANGE_MSK			GENMASK(1, 0)
> +
>  #define BMI270_GYR_CONF_REG				0x42
>  #define BMI270_GYR_CONF_ODR_MSK				GENMASK(3, 0)
>  #define BMI270_GYR_CONF_ODR_200HZ			0x09
> @@ -42,6 +46,9 @@
>  #define BMI270_GYR_CONF_NOISE_PERF_MSK			BIT(6)
>  #define BMI270_GYR_CONF_FILTER_PERF_MSK			BIT(7)
>  
> +#define BMI270_GYR_CONF_RANGE_REG			0x43
> +#define BMI270_GYR_CONF_RANGE_MSK			GENMASK(2, 0)
> +
>  #define BMI270_INIT_CTRL_REG				0x59
>  #define BMI270_INIT_CTRL_LOAD_DONE_MSK			BIT(0)
>  
> @@ -85,6 +92,236 @@ const struct bmi270_chip_info bmi270_chip_info[] = {
>  };
>  EXPORT_SYMBOL_NS_GPL(bmi270_chip_info, IIO_BMI270);
>  
> +enum bmi270_sensor_type {
> +	BMI270_ACCEL	= 0,
> +	BMI270_GYRO,
> +};
> +
> +struct bmi270_scale {
> +	u8 bits;
> +	int uscale;
> +};
> +
> +struct bmi270_odr {
> +	u8 bits;
> +	int odr;
> +	int uodr;
> +};
> +
> +static const struct bmi270_scale bmi270_accel_scale[] = {
> +	{ 0x00, 598},
	{ 0x00, 598 },

So space before } for all these.

> +	{ 0x01, 1197},
> +	{ 0x02, 2394},
> +	{ 0x03, 4788},
> +};
> +
> +static const struct bmi270_scale bmi270_gyro_scale[] = {
> +	{ 0x00, 1065},
> +	{ 0x01, 532},
> +	{ 0x02, 266},
> +	{ 0x03, 133},
> +	{ 0x04, 66},
> +};
> +
> +struct bmi270_scale_item {
> +	const struct bmi270_scale *tbl;
> +	int num;
> +};
> +
> +static const struct bmi270_scale_item bmi270_scale_table[] = {
> +	[BMI270_ACCEL] = {
> +		.tbl	= bmi270_accel_scale,
> +		.num	= ARRAY_SIZE(bmi270_accel_scale),
> +	},
> +	[BMI270_GYRO] = {
> +		.tbl	= bmi270_gyro_scale,
> +		.num	= ARRAY_SIZE(bmi270_gyro_scale),
> +	},
> +};
> +
> +static const struct bmi270_odr bmi270_accel_odr[] = {
> +	{0x01, 0, 781250},
> +	{0x02, 1, 562500},
> +	{0x03, 3, 125000},
> +	{0x04, 6, 250000},
> +	{0x05, 12, 500000},
> +	{0x06, 25, 0},
> +	{0x07, 50, 0},
> +	{0x08, 100, 0},
> +	{0x09, 200, 0},
> +	{0x0A, 400, 0},
> +	{0x0B, 800, 0},
> +	{0x0C, 1600, 0},
> +};
> +
> +static const struct bmi270_odr bmi270_gyro_odr[] = {
> +	{0x06, 25, 0},
> +	{0x07, 50, 0},
> +	{0x08, 100, 0},
> +	{0x09, 200, 0},
> +	{0x0A, 400, 0},
> +	{0x0B, 800, 0},
> +	{0x0C, 1600, 0},
> +	{0x0D, 3200, 0},
> +};
> +
> +struct bmi270_odr_item {
> +	const struct bmi270_odr *tbl;
> +	int num;
> +};
> +
> +static const struct  bmi270_odr_item bmi270_odr_table[] = {
> +	[BMI270_ACCEL] = {
> +		.tbl	= bmi270_accel_odr,
> +		.num	= ARRAY_SIZE(bmi270_accel_odr),
> +	},
> +	[BMI270_GYRO] = {
> +		.tbl	= bmi270_gyro_odr,
> +		.num	= ARRAY_SIZE(bmi270_gyro_odr),
> +	},
> +};
> +
> +static int bmi270_set_scale(struct bmi270_data *data,
> +			    int chan_type, int uscale)
> +{
> +	int i;
> +	int reg;
> +	struct bmi270_scale_item bmi270_scale_item;
> +
> +	switch (chan_type) {
> +	case IIO_ACCEL:
> +		reg = BMI270_ACC_CONF_RANGE_REG;
> +		bmi270_scale_item = bmi270_scale_table[BMI270_ACCEL];
> +		break;
> +	case IIO_ANGL_VEL:
> +		reg = BMI270_GYR_CONF_RANGE_REG;
> +		bmi270_scale_item = bmi270_scale_table[BMI270_GYRO];
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < bmi270_scale_item.num; i++)
> +		if (bmi270_scale_item.tbl[i].uscale == uscale)
> +			break;
> +
> +	if (i == bmi270_scale_item.num)
> +		return -EINVAL;
> +
> +	return regmap_write(data->regmap, reg,
> +			    bmi270_scale_item.tbl[i].bits);
Maybe do the write in the if above, (or use the continue approach mentioned
below + do the write in the for loop.
Then any exit from the loop that isn't a return is a failure and you an save the check.

> +}
> +
> +static int bmi270_get_scale(struct bmi270_data *bmi270_device,
> +			    int chan_type, int *uscale)
> +{
> +	int i, ret, val;
> +	int reg;
> +	struct bmi270_scale_item bmi270_scale_item;
> +
> +	switch (chan_type) {
> +	case IIO_ACCEL:
> +		reg = BMI270_ACC_CONF_RANGE_REG;
> +		bmi270_scale_item = bmi270_scale_table[BMI270_ACCEL];
> +		break;
> +	case IIO_ANGL_VEL:
> +		reg = BMI270_GYR_CONF_RANGE_REG;
> +		bmi270_scale_item = bmi270_scale_table[BMI270_GYRO];
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = regmap_read(bmi270_device->regmap, reg, &val);
> +	if (ret)
> +		return ret;

No masking?

> +
> +	for (i = 0; i < bmi270_scale_item.num; i++)
> +		if (bmi270_scale_item.tbl[i].bits == val) {
Flip the condition to reduce indent

		if (bmi270_scale_item.tbl[i].bits != val)
			continue;

		*uscale.

> +			*uscale = bmi270_scale_item.tbl[i].uscale;
> +			return 0;
> +		}
> +
> +	return -EINVAL;
> +}
> +
> +static int bmi270_set_odr(struct bmi270_data *data, int chan_type,
> +			  int odr, int uodr)
> +{
> +	int i;
> +	int reg, mask;
> +	struct bmi270_odr_item bmi270_odr_item;
> +
> +	switch (chan_type) {
> +	case IIO_ACCEL:
> +		reg = BMI270_ACC_CONF_REG;
> +		mask = BMI270_ACC_CONF_ODR_MSK;
> +		bmi270_odr_item = bmi270_odr_table[BMI270_ACCEL];
> +		break;
> +	case IIO_ANGL_VEL:
> +		reg = BMI270_GYR_CONF_REG;
> +		mask = BMI270_GYR_CONF_ODR_MSK;
> +		bmi270_odr_item = bmi270_odr_table[BMI270_GYRO];
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < bmi270_odr_item.num; i++)
> +		if (bmi270_odr_item.tbl[i].odr == odr &&
> +		    bmi270_odr_item.tbl[i].uodr == uodr)
> +			break;
> +
> +	if (i >= bmi270_odr_item.num)
> +		return -EINVAL;
> +
> +	return regmap_update_bits(data->regmap,
> +				  reg,
> +				  mask,
> +				  bmi270_odr_item.tbl[i].bits);

combine parameters on each line to get nearer to 80 char limit.

> +}
> +
> +static int bmi270_get_odr(struct bmi270_data *data, int chan_type,
> +			  int *odr, int *uodr)
> +{
> +	int i, val, ret;
> +	int reg, mask;
> +	struct bmi270_odr_item bmi270_odr_item;
> +
> +	switch (chan_type) {
> +	case IIO_ACCEL:
> +		reg = BMI270_ACC_CONF_REG;
> +		mask = BMI270_ACC_CONF_ODR_MSK;
> +		bmi270_odr_item = bmi270_odr_table[BMI270_ACCEL];
> +		break;
> +	case IIO_ANGL_VEL:
> +		reg = BMI270_GYR_CONF_REG;
> +		mask = BMI270_GYR_CONF_ODR_MSK;
> +		bmi270_odr_item = bmi270_odr_table[BMI270_GYRO];
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = regmap_read(data->regmap, reg, &val);
> +	if (ret)
> +		return ret;
> +
> +	val &= mask;

I'd just duplicate the regmap_read to allow easy use FIELD_GET etc.


	switch (chan_type) {
	case IIO_ACCEL:
		ret = regmap_read(data->regmap, BMI270_ACC_CONF_REG, &val);
		if (ret)
			return ret;		
		val = FIELD_GET(val, BMI270_ACC_CONF_ODR_MSK);
		bmi270_odr_item = bmi270_odr_table[BMI270_ACCEL];
		break;
	case IIO_ANGL_VEL:
		ret = regmap_read(data->regmap, BMI270_GYR_CONF_REG, &val);
		if (ret)
			return ret;
		val = FIELD_GET(val, BMI270_GYR_CONF_ODR_MSK);
		bmi270_odr_item = bmi270_odr_table[BMI270_GYRO];
		break;
	default:
		return -EINVAL;
	}


> +
> +	for (i = 0; i < bmi270_odr_item.num; i++)
> +		if (val == bmi270_odr_item.tbl[i].bits)
> +			break;
> +
> +	if (i >= bmi270_odr_item.num)
> +		return -EINVAL;
> +
> +	*odr = bmi270_odr_item.tbl[i].odr;
> +	*uodr = bmi270_odr_item.tbl[i].uodr;
> +
> +	return 0;
> +}
> +
> +static
> +IIO_CONST_ATTR(in_accel_sampling_frequency_available,
> +	       "0.78125 1.5625 3.125 6.25 12.5 25 50 100 200 400 800 1600");
> +static
> +IIO_CONST_ATTR(in_anglvel_sampling_frequency_available,
> +	       "25 50 100 200 400 800 1600 3200");
> +static
> +IIO_CONST_ATTR(in_accel_scale_available,
> +	       "0.000598 0.001197 0.002394 0.004788");
> +static
> +IIO_CONST_ATTR(in_anglvel_scale_available,
> +	       "0.001065 0.000532 0.000266 0.000133 0.000066");
> +
> +static struct attribute *bmi270_attrs[] = {
> +	&iio_const_attr_in_accel_sampling_frequency_available.dev_attr.attr,
> +	&iio_const_attr_in_anglvel_sampling_frequency_available.dev_attr.attr,
> +	&iio_const_attr_in_accel_scale_available.dev_attr.attr,
> +	&iio_const_attr_in_anglvel_scale_available.dev_attr.attr,
Please use the read_avail callback and info_mask_xxx_avail masks to specify
these exist for all the channels.

Doing this as custom attrs predates that infrastructure and we are
slowly converting drivers over.  The main advantage beyond enforced
ABI is that can get to the values from in kernel consumers of the data.

> +	NULL,
No comma here.
> +};
> +
> +static const struct attribute_group bmi270_attrs_group = {
> +	.attrs = bmi270_attrs,
> +};

Re: [PATCH 3/3] iio: imu: Add scale and sampling frequency to BMI270 IMU

[Justin Weiss]

Jonathan Cameron <jic23@kernel.org> writes:

> On Fri, 11 Oct 2024 08:37:49 -0700
> Justin Weiss <justin@justinweiss.com> wrote:
>
>> Add read and write functions and create _available entries. Use
>> IIO_CHAN_INFO_SAMP_FREQ instead of IIO_CHAN_INFO_FREQUENCY to match
>> the BMI160 / BMI323 drivers.
>
> Ah.  Please break dropping _FREQUENCY change out as a separate fix
> with fixes tag etc and drag it to start of the patch. It was never
> wired to anything anyway
>
> That's a straight forward ABI bug so we want that to land ahead
> of the rest of the series.

Thanks, I'll pull that into its own change and make it the first patch.

> Does this device have a data ready interrupt and if so what affect
> do the different ODRs for each type of sensor have on that?
> If there are separate data ready signals, you probably want to 
> go with a dual buffer setup from the start as it is hard to unwind
> that later.

It has data ready interrupts for both accelerometer and gyroscope and a
FIFO interrupt. I had held off on interrupts to keep this change
simpler, but if it's a better idea to get it in earlier, I can add it
alongside the triggered buffer change.

>
>> 
>> Signed-off-by: Justin Weiss <justin@justinweiss.com>
>> ---
>>  drivers/iio/imu/bmi270/bmi270_core.c | 293 ++++++++++++++++++++++++++-
>>  1 file changed, 291 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c
>> index f49db5d1bffd..ce7873dc3211 100644
>> --- a/drivers/iio/imu/bmi270/bmi270_core.c
>> +++ b/drivers/iio/imu/bmi270/bmi270_core.c
>> @@ -7,6 +7,7 @@
>>  #include <linux/regmap.h>
>>  
>>  #include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>>  #include <linux/iio/triggered_buffer.h>
>>  #include <linux/iio/trigger_consumer.h>
>>  
>> @@ -34,6 +35,9 @@
>>  #define BMI270_ACC_CONF_BWP_NORMAL_MODE			0x02
>>  #define BMI270_ACC_CONF_FILTER_PERF_MSK			BIT(7)
>>  
>> +#define BMI270_ACC_CONF_RANGE_REG			0x41
>> +#define BMI270_ACC_CONF_RANGE_MSK			GENMASK(1, 0)
>> +
>>  #define BMI270_GYR_CONF_REG				0x42
>>  #define BMI270_GYR_CONF_ODR_MSK				GENMASK(3, 0)
>>  #define BMI270_GYR_CONF_ODR_200HZ			0x09
>> @@ -42,6 +46,9 @@
>>  #define BMI270_GYR_CONF_NOISE_PERF_MSK			BIT(6)
>>  #define BMI270_GYR_CONF_FILTER_PERF_MSK			BIT(7)
>>  
>> +#define BMI270_GYR_CONF_RANGE_REG			0x43
>> +#define BMI270_GYR_CONF_RANGE_MSK			GENMASK(2, 0)
>> +
>>  #define BMI270_INIT_CTRL_REG				0x59
>>  #define BMI270_INIT_CTRL_LOAD_DONE_MSK			BIT(0)
>>  
>> @@ -85,6 +92,236 @@ const struct bmi270_chip_info bmi270_chip_info[] = {
>>  };
>>  EXPORT_SYMBOL_NS_GPL(bmi270_chip_info, IIO_BMI270);
>>  
>> +enum bmi270_sensor_type {
>> +	BMI270_ACCEL	= 0,
>> +	BMI270_GYRO,
>> +};
>> +
>> +struct bmi270_scale {
>> +	u8 bits;
>> +	int uscale;
>> +};
>> +
>> +struct bmi270_odr {
>> +	u8 bits;
>> +	int odr;
>> +	int uodr;
>> +};
>> +
>> +static const struct bmi270_scale bmi270_accel_scale[] = {
>> +	{ 0x00, 598},
> 	{ 0x00, 598 },
>
> So space before } for all these.

Will fix in v2.

>> +	{ 0x01, 1197},
>> +	{ 0x02, 2394},
>> +	{ 0x03, 4788},
>> +};
>> +
>> +static const struct bmi270_scale bmi270_gyro_scale[] = {
>> +	{ 0x00, 1065},
>> +	{ 0x01, 532},
>> +	{ 0x02, 266},
>> +	{ 0x03, 133},
>> +	{ 0x04, 66},
>> +};
>> +
>> +struct bmi270_scale_item {
>> +	const struct bmi270_scale *tbl;
>> +	int num;
>> +};
>> +
>> +static const struct bmi270_scale_item bmi270_scale_table[] = {
>> +	[BMI270_ACCEL] = {
>> +		.tbl	= bmi270_accel_scale,
>> +		.num	= ARRAY_SIZE(bmi270_accel_scale),
>> +	},
>> +	[BMI270_GYRO] = {
>> +		.tbl	= bmi270_gyro_scale,
>> +		.num	= ARRAY_SIZE(bmi270_gyro_scale),
>> +	},
>> +};
>> +
>> +static const struct bmi270_odr bmi270_accel_odr[] = {
>> +	{0x01, 0, 781250},
>> +	{0x02, 1, 562500},
>> +	{0x03, 3, 125000},
>> +	{0x04, 6, 250000},
>> +	{0x05, 12, 500000},
>> +	{0x06, 25, 0},
>> +	{0x07, 50, 0},
>> +	{0x08, 100, 0},
>> +	{0x09, 200, 0},
>> +	{0x0A, 400, 0},
>> +	{0x0B, 800, 0},
>> +	{0x0C, 1600, 0},
>> +};
>> +
>> +static const struct bmi270_odr bmi270_gyro_odr[] = {
>> +	{0x06, 25, 0},
>> +	{0x07, 50, 0},
>> +	{0x08, 100, 0},
>> +	{0x09, 200, 0},
>> +	{0x0A, 400, 0},
>> +	{0x0B, 800, 0},
>> +	{0x0C, 1600, 0},
>> +	{0x0D, 3200, 0},
>> +};
>> +
>> +struct bmi270_odr_item {
>> +	const struct bmi270_odr *tbl;
>> +	int num;
>> +};
>> +
>> +static const struct  bmi270_odr_item bmi270_odr_table[] = {
>> +	[BMI270_ACCEL] = {
>> +		.tbl	= bmi270_accel_odr,
>> +		.num	= ARRAY_SIZE(bmi270_accel_odr),
>> +	},
>> +	[BMI270_GYRO] = {
>> +		.tbl	= bmi270_gyro_odr,
>> +		.num	= ARRAY_SIZE(bmi270_gyro_odr),
>> +	},
>> +};
>> +
>> +static int bmi270_set_scale(struct bmi270_data *data,
>> +			    int chan_type, int uscale)
>> +{
>> +	int i;
>> +	int reg;
>> +	struct bmi270_scale_item bmi270_scale_item;
>> +
>> +	switch (chan_type) {
>> +	case IIO_ACCEL:
>> +		reg = BMI270_ACC_CONF_RANGE_REG;
>> +		bmi270_scale_item = bmi270_scale_table[BMI270_ACCEL];
>> +		break;
>> +	case IIO_ANGL_VEL:
>> +		reg = BMI270_GYR_CONF_RANGE_REG;
>> +		bmi270_scale_item = bmi270_scale_table[BMI270_GYRO];
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	for (i = 0; i < bmi270_scale_item.num; i++)
>> +		if (bmi270_scale_item.tbl[i].uscale == uscale)
>> +			break;
>> +
>> +	if (i == bmi270_scale_item.num)
>> +		return -EINVAL;
>> +
>> +	return regmap_write(data->regmap, reg,
>> +			    bmi270_scale_item.tbl[i].bits);
> Maybe do the write in the if above, (or use the continue approach mentioned
> below + do the write in the for loop.
> Then any exit from the loop that isn't a return is a failure and you an save the check.

Thanks for this suggestion, I'll change all of these loops to use continue / return.

>> +}
>> +
>> +static int bmi270_get_scale(struct bmi270_data *bmi270_device,
>> +			    int chan_type, int *uscale)
>> +{
>> +	int i, ret, val;
>> +	int reg;
>> +	struct bmi270_scale_item bmi270_scale_item;
>> +
>> +	switch (chan_type) {
>> +	case IIO_ACCEL:
>> +		reg = BMI270_ACC_CONF_RANGE_REG;
>> +		bmi270_scale_item = bmi270_scale_table[BMI270_ACCEL];
>> +		break;
>> +	case IIO_ANGL_VEL:
>> +		reg = BMI270_GYR_CONF_RANGE_REG;
>> +		bmi270_scale_item = bmi270_scale_table[BMI270_GYRO];
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = regmap_read(bmi270_device->regmap, reg, &val);
>> +	if (ret)
>> +		return ret;
>
> No masking?

Looks like I missed this. I'll fix it in v2.

>
>> +
>> +	for (i = 0; i < bmi270_scale_item.num; i++)
>> +		if (bmi270_scale_item.tbl[i].bits == val) {
> Flip the condition to reduce indent
>
> 		if (bmi270_scale_item.tbl[i].bits != val)
> 			continue;
>
> 		*uscale.
>
>> +			*uscale = bmi270_scale_item.tbl[i].uscale;
>> +			return 0;
>> +		}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static int bmi270_set_odr(struct bmi270_data *data, int chan_type,
>> +			  int odr, int uodr)
>> +{
>> +	int i;
>> +	int reg, mask;
>> +	struct bmi270_odr_item bmi270_odr_item;
>> +
>> +	switch (chan_type) {
>> +	case IIO_ACCEL:
>> +		reg = BMI270_ACC_CONF_REG;
>> +		mask = BMI270_ACC_CONF_ODR_MSK;
>> +		bmi270_odr_item = bmi270_odr_table[BMI270_ACCEL];
>> +		break;
>> +	case IIO_ANGL_VEL:
>> +		reg = BMI270_GYR_CONF_REG;
>> +		mask = BMI270_GYR_CONF_ODR_MSK;
>> +		bmi270_odr_item = bmi270_odr_table[BMI270_GYRO];
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	for (i = 0; i < bmi270_odr_item.num; i++)
>> +		if (bmi270_odr_item.tbl[i].odr == odr &&
>> +		    bmi270_odr_item.tbl[i].uodr == uodr)
>> +			break;
>> +
>> +	if (i >= bmi270_odr_item.num)
>> +		return -EINVAL;
>> +
>> +	return regmap_update_bits(data->regmap,
>> +				  reg,
>> +				  mask,
>> +				  bmi270_odr_item.tbl[i].bits);
>
> combine parameters on each line to get nearer to 80 char limit.

Will fix in v2.

>
>> +}
>> +
>> +static int bmi270_get_odr(struct bmi270_data *data, int chan_type,
>> +			  int *odr, int *uodr)
>> +{
>> +	int i, val, ret;
>> +	int reg, mask;
>> +	struct bmi270_odr_item bmi270_odr_item;
>> +
>> +	switch (chan_type) {
>> +	case IIO_ACCEL:
>> +		reg = BMI270_ACC_CONF_REG;
>> +		mask = BMI270_ACC_CONF_ODR_MSK;
>> +		bmi270_odr_item = bmi270_odr_table[BMI270_ACCEL];
>> +		break;
>> +	case IIO_ANGL_VEL:
>> +		reg = BMI270_GYR_CONF_REG;
>> +		mask = BMI270_GYR_CONF_ODR_MSK;
>> +		bmi270_odr_item = bmi270_odr_table[BMI270_GYRO];
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = regmap_read(data->regmap, reg, &val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	val &= mask;
>
> I'd just duplicate the regmap_read to allow easy use FIELD_GET etc.
>
>
> 	switch (chan_type) {
> 	case IIO_ACCEL:
> 		ret = regmap_read(data->regmap, BMI270_ACC_CONF_REG, &val);
> 		if (ret)
> 			return ret;		
> 		val = FIELD_GET(val, BMI270_ACC_CONF_ODR_MSK);
> 		bmi270_odr_item = bmi270_odr_table[BMI270_ACCEL];
> 		break;
> 	case IIO_ANGL_VEL:
> 		ret = regmap_read(data->regmap, BMI270_GYR_CONF_REG, &val);
> 		if (ret)
> 			return ret;
> 		val = FIELD_GET(val, BMI270_GYR_CONF_ODR_MSK);
> 		bmi270_odr_item = bmi270_odr_table[BMI270_GYRO];
> 		break;
> 	default:
> 		return -EINVAL;
> 	}

Thanks, that's nicer. I'll fix it in v2.

>> +
>> +	for (i = 0; i < bmi270_odr_item.num; i++)
>> +		if (val == bmi270_odr_item.tbl[i].bits)
>> +			break;
>> +
>> +	if (i >= bmi270_odr_item.num)
>> +		return -EINVAL;
>> +
>> +	*odr = bmi270_odr_item.tbl[i].odr;
>> +	*uodr = bmi270_odr_item.tbl[i].uodr;
>> +
>> +	return 0;
>> +}
>> +
>> +static
>> +IIO_CONST_ATTR(in_accel_sampling_frequency_available,
>> +	       "0.78125 1.5625 3.125 6.25 12.5 25 50 100 200 400 800 1600");
>> +static
>> +IIO_CONST_ATTR(in_anglvel_sampling_frequency_available,
>> +	       "25 50 100 200 400 800 1600 3200");
>> +static
>> +IIO_CONST_ATTR(in_accel_scale_available,
>> +	       "0.000598 0.001197 0.002394 0.004788");
>> +static
>> +IIO_CONST_ATTR(in_anglvel_scale_available,
>> +	       "0.001065 0.000532 0.000266 0.000133 0.000066");
>> +
>> +static struct attribute *bmi270_attrs[] = {
>> +	&iio_const_attr_in_accel_sampling_frequency_available.dev_attr.attr,
>> +	&iio_const_attr_in_anglvel_sampling_frequency_available.dev_attr.attr,
>> +	&iio_const_attr_in_accel_scale_available.dev_attr.attr,
>> +	&iio_const_attr_in_anglvel_scale_available.dev_attr.attr,
> Please use the read_avail callback and info_mask_xxx_avail masks to specify
> these exist for all the channels.
>
> Doing this as custom attrs predates that infrastructure and we are
> slowly converting drivers over.  The main advantage beyond enforced
> ABI is that can get to the values from in kernel consumers of the data.
>

Great, I'll remove these and implement read_avail.

>> +	NULL,
> No comma here.

Will fix in v2.

>> +};
>> +
>> +static const struct attribute_group bmi270_attrs_group = {
>> +	.attrs = bmi270_attrs,
>> +};

Re: [PATCH 3/3] iio: imu: Add scale and sampling frequency to BMI270 IMU

[Jonathan Cameron]

On Sat, 12 Oct 2024 19:45:18 -0700
Justin Weiss <justin@justinweiss.com> wrote:

> Jonathan Cameron <jic23@kernel.org> writes:
> 
> > On Fri, 11 Oct 2024 08:37:49 -0700
> > Justin Weiss <justin@justinweiss.com> wrote:
> >  
> >> Add read and write functions and create _available entries. Use
> >> IIO_CHAN_INFO_SAMP_FREQ instead of IIO_CHAN_INFO_FREQUENCY to match
> >> the BMI160 / BMI323 drivers.  
> >
> > Ah.  Please break dropping _FREQUENCY change out as a separate fix
> > with fixes tag etc and drag it to start of the patch. It was never
> > wired to anything anyway
> >
> > That's a straight forward ABI bug so we want that to land ahead
> > of the rest of the series.  
> 
> Thanks, I'll pull that into its own change and make it the first patch.
> 
> > Does this device have a data ready interrupt and if so what affect
> > do the different ODRs for each type of sensor have on that?
> > If there are separate data ready signals, you probably want to 
> > go with a dual buffer setup from the start as it is hard to unwind
> > that later.  
> 
> It has data ready interrupts for both accelerometer and gyroscope and a
> FIFO interrupt. I had held off on interrupts to keep this change
> simpler, but if it's a better idea to get it in earlier, I can add it
> alongside the triggered buffer change.

Ok. So the challenge is that IIO buffers are only described by external
metadata.  We don't carry tags within them.  Hence if you are using
either effectively separate datastreams (the two data ready interrupts)
or a fifo that is tagged data (how this difference of speed is normally handled
if it's one buffer) then when we push them into IIO buffers, they have
to go into separate buffers.

In older drivers this was done via the heavy weight option of registering
two separate IIO devices. Today we have the ability to support multiple buffers
in one driver. I'm not sure we've yet used it for this case, so I think
there may still be some gaps around triggering that will matter for the
separate dataready interrupt case (fifo is fine as no trigger involved).
Looking again at that code, it looks like there may need to be quite
a bit more work to cover this case proeprly.

We may be able to have a migration path from the simple case you have
(where timing is an external trigger) to multiple buffers.
It would involve:
1) Initial solution where the frequencies must match if the fifo is in use.
   Non fifo trigger from data ready might work but we'd need to figure out
   if they run in close enough timing.
2) Solution where we add a second buffer and if the channels are enabled
   in that we can allow separate timing for the two sensor types.

This is one of those hardware features that seems like a good idea
from the hardware design point of view but assumes a very specific
sort of software model :(

Jonathan

Re: [PATCH 3/3] iio: imu: Add scale and sampling frequency to BMI270 IMU

[Justin Weiss]

Jonathan Cameron <jic23@kernel.org> writes:

> On Sat, 12 Oct 2024 19:45:18 -0700
> Justin Weiss <justin@justinweiss.com> wrote:
>
>> Jonathan Cameron <jic23@kernel.org> writes:
>> 
>> > On Fri, 11 Oct 2024 08:37:49 -0700
>> > Justin Weiss <justin@justinweiss.com> wrote:
>> >  
>> >> Add read and write functions and create _available entries. Use
>> >> IIO_CHAN_INFO_SAMP_FREQ instead of IIO_CHAN_INFO_FREQUENCY to match
>> >> the BMI160 / BMI323 drivers.  
>> >
>> > Ah.  Please break dropping _FREQUENCY change out as a separate fix
>> > with fixes tag etc and drag it to start of the patch. It was never
>> > wired to anything anyway
>> >
>> > That's a straight forward ABI bug so we want that to land ahead
>> > of the rest of the series.  
>> 
>> Thanks, I'll pull that into its own change and make it the first patch.
>> 
>> > Does this device have a data ready interrupt and if so what affect
>> > do the different ODRs for each type of sensor have on that?
>> > If there are separate data ready signals, you probably want to 
>> > go with a dual buffer setup from the start as it is hard to unwind
>> > that later.  
>> 
>> It has data ready interrupts for both accelerometer and gyroscope and a
>> FIFO interrupt. I had held off on interrupts to keep this change
>> simpler, but if it's a better idea to get it in earlier, I can add it
>> alongside the triggered buffer change.
>
> Ok. So the challenge is that IIO buffers are only described by external
> metadata.  We don't carry tags within them.  Hence if you are using
> either effectively separate datastreams (the two data ready interrupts)
> or a fifo that is tagged data (how this difference of speed is normally handled
> if it's one buffer) then when we push them into IIO buffers, they have
> to go into separate buffers.
>
> In older drivers this was done via the heavy weight option of registering
> two separate IIO devices. Today we have the ability to support multiple buffers
> in one driver. I'm not sure we've yet used it for this case, so I think
> there may still be some gaps around triggering that will matter for the
> separate dataready interrupt case (fifo is fine as no trigger involved).
> Looking again at that code, it looks like there may need to be quite
> a bit more work to cover this case proeprly.
>
> We may be able to have a migration path from the simple case you have
> (where timing is an external trigger) to multiple buffers.
> It would involve:
> 1) Initial solution where the frequencies must match if the fifo is in use.
>    Non fifo trigger from data ready might work but we'd need to figure out
>    if they run in close enough timing.
> 2) Solution where we add a second buffer and if the channels are enabled
>    in that we can allow separate timing for the two sensor types.
>
> This is one of those hardware features that seems like a good idea
> from the hardware design point of view but assumes a very specific
> sort of software model :(
>
> Jonathan

Hm, that does sound tricky. If there's an example I can follow, I can
make an attempt at it. Otherwise, if there's a change I can make now
that would help with migrating in the future, I can do that instead.

Of the devices I've looked at, only one has had the interrupts usable
and that one only had a single pin available. So if this change doesn't
make it harder to add later if it's necessary, I would still be OK going
without full support for now.

Justin

Re: [PATCH 3/3] iio: imu: Add scale and sampling frequency to BMI270 IMU

[Jonathan Cameron]

On Sun, 13 Oct 2024 13:55:36 -0700
Justin Weiss <justin@justinweiss.com> wrote:

> Jonathan Cameron <jic23@kernel.org> writes:
> 
> > On Sat, 12 Oct 2024 19:45:18 -0700
> > Justin Weiss <justin@justinweiss.com> wrote:
> >  
> >> Jonathan Cameron <jic23@kernel.org> writes:
> >>   
> >> > On Fri, 11 Oct 2024 08:37:49 -0700
> >> > Justin Weiss <justin@justinweiss.com> wrote:
> >> >    
> >> >> Add read and write functions and create _available entries. Use
> >> >> IIO_CHAN_INFO_SAMP_FREQ instead of IIO_CHAN_INFO_FREQUENCY to match
> >> >> the BMI160 / BMI323 drivers.    
> >> >
> >> > Ah.  Please break dropping _FREQUENCY change out as a separate fix
> >> > with fixes tag etc and drag it to start of the patch. It was never
> >> > wired to anything anyway
> >> >
> >> > That's a straight forward ABI bug so we want that to land ahead
> >> > of the rest of the series.    
> >> 
> >> Thanks, I'll pull that into its own change and make it the first patch.
> >>   
> >> > Does this device have a data ready interrupt and if so what affect
> >> > do the different ODRs for each type of sensor have on that?
> >> > If there are separate data ready signals, you probably want to 
> >> > go with a dual buffer setup from the start as it is hard to unwind
> >> > that later.    
> >> 
> >> It has data ready interrupts for both accelerometer and gyroscope and a
> >> FIFO interrupt. I had held off on interrupts to keep this change
> >> simpler, but if it's a better idea to get it in earlier, I can add it
> >> alongside the triggered buffer change.  
> >
> > Ok. So the challenge is that IIO buffers are only described by external
> > metadata.  We don't carry tags within them.  Hence if you are using
> > either effectively separate datastreams (the two data ready interrupts)
> > or a fifo that is tagged data (how this difference of speed is normally handled
> > if it's one buffer) then when we push them into IIO buffers, they have
> > to go into separate buffers.
> >
> > In older drivers this was done via the heavy weight option of registering
> > two separate IIO devices. Today we have the ability to support multiple buffers
> > in one driver. I'm not sure we've yet used it for this case, so I think
> > there may still be some gaps around triggering that will matter for the
> > separate dataready interrupt case (fifo is fine as no trigger involved).
> > Looking again at that code, it looks like there may need to be quite
> > a bit more work to cover this case proeprly.
> >
> > We may be able to have a migration path from the simple case you have
> > (where timing is an external trigger) to multiple buffers.
> > It would involve:
> > 1) Initial solution where the frequencies must match if the fifo is in use.
> >    Non fifo trigger from data ready might work but we'd need to figure out
> >    if they run in close enough timing.
> > 2) Solution where we add a second buffer and if the channels are enabled
> >    in that we can allow separate timing for the two sensor types.
> >
> > This is one of those hardware features that seems like a good idea
> > from the hardware design point of view but assumes a very specific
> > sort of software model :(
> >
> > Jonathan  
> 
> Hm, that does sound tricky. If there's an example I can follow, I can
> make an attempt at it.

I don't think it ever got used for a device like this - so probably no
examples, but I might have forgotten one. (this was a few years back).

> Otherwise, if there's a change I can make now
> that would help with migrating in the future, I can do that instead.
> 
> Of the devices I've looked at, only one has had the interrupts usable
> and that one only had a single pin available.
Lovely!  

> So if this change doesn't
> make it harder to add later if it's necessary, I would still be OK going
> without full support for now.
I stopped being lazy and opened the datasheet.

Hmm. We have auxiliary channels as well.  oh goody.
Considering just the fifo as that's the high performance route.

Basically we can do headerless mode trivially as that's just one buffer.
(same ODR for all sensors).
We could do headered version but without messing with multiple buffers
that would be only when all sensors have same ODR (after a messy
transition period perhaps - that bit of the datasheet is less than
intuitive!) The reason we might do headered mode is to support the
timestamps but we can probably get those via a quick read of other
registers after draining the fifo.

So I'm fine with just not supporting the weird corner cases unless
we get someone turning up who
a) cares
b) if foolish (or motivated) enough to do the necessary work 
c) (if they are lucky) we have the infrastructure in place because someone
   else needed the missing bits.

Jonathan


> 
> Justin

Re: [PATCH 3/3] iio: imu: Add scale and sampling frequency to BMI270 IMU

[Justin Weiss]

Jonathan Cameron <jic23@kernel.org> writes:

> On Sun, 13 Oct 2024 13:55:36 -0700
> Justin Weiss <justin@justinweiss.com> wrote:
>
>> Jonathan Cameron <jic23@kernel.org> writes:
>> 
>> > On Sat, 12 Oct 2024 19:45:18 -0700
>> > Justin Weiss <justin@justinweiss.com> wrote:
>> >  
>> >> Jonathan Cameron <jic23@kernel.org> writes:
>> >>   
>> >> > On Fri, 11 Oct 2024 08:37:49 -0700
>> >> > Justin Weiss <justin@justinweiss.com> wrote:
>> >> >    
>> >> >> Add read and write functions and create _available entries. Use
>> >> >> IIO_CHAN_INFO_SAMP_FREQ instead of IIO_CHAN_INFO_FREQUENCY to match
>> >> >> the BMI160 / BMI323 drivers.    
>> >> >
>> >> > Ah.  Please break dropping _FREQUENCY change out as a separate fix
>> >> > with fixes tag etc and drag it to start of the patch. It was never
>> >> > wired to anything anyway
>> >> >
>> >> > That's a straight forward ABI bug so we want that to land ahead
>> >> > of the rest of the series.    
>> >> 
>> >> Thanks, I'll pull that into its own change and make it the first patch.
>> >>   
>> >> > Does this device have a data ready interrupt and if so what affect
>> >> > do the different ODRs for each type of sensor have on that?
>> >> > If there are separate data ready signals, you probably want to 
>> >> > go with a dual buffer setup from the start as it is hard to unwind
>> >> > that later.    
>> >> 
>> >> It has data ready interrupts for both accelerometer and gyroscope and a
>> >> FIFO interrupt. I had held off on interrupts to keep this change
>> >> simpler, but if it's a better idea to get it in earlier, I can add it
>> >> alongside the triggered buffer change.  
>> >
>> > Ok. So the challenge is that IIO buffers are only described by external
>> > metadata.  We don't carry tags within them.  Hence if you are using
>> > either effectively separate datastreams (the two data ready interrupts)
>> > or a fifo that is tagged data (how this difference of speed is normally handled
>> > if it's one buffer) then when we push them into IIO buffers, they have
>> > to go into separate buffers.
>> >
>> > In older drivers this was done via the heavy weight option of registering
>> > two separate IIO devices. Today we have the ability to support multiple buffers
>> > in one driver. I'm not sure we've yet used it for this case, so I think
>> > there may still be some gaps around triggering that will matter for the
>> > separate dataready interrupt case (fifo is fine as no trigger involved).
>> > Looking again at that code, it looks like there may need to be quite
>> > a bit more work to cover this case proeprly.
>> >
>> > We may be able to have a migration path from the simple case you have
>> > (where timing is an external trigger) to multiple buffers.
>> > It would involve:
>> > 1) Initial solution where the frequencies must match if the fifo is in use.
>> >    Non fifo trigger from data ready might work but we'd need to figure out
>> >    if they run in close enough timing.
>> > 2) Solution where we add a second buffer and if the channels are enabled
>> >    in that we can allow separate timing for the two sensor types.
>> >
>> > This is one of those hardware features that seems like a good idea
>> > from the hardware design point of view but assumes a very specific
>> > sort of software model :(
>> >
>> > Jonathan  
>> 
>> Hm, that does sound tricky. If there's an example I can follow, I can
>> make an attempt at it.
>
> I don't think it ever got used for a device like this - so probably no
> examples, but I might have forgotten one. (this was a few years back).
>
>> Otherwise, if there's a change I can make now
>> that would help with migrating in the future, I can do that instead.
>> 
>> Of the devices I've looked at, only one has had the interrupts usable
>> and that one only had a single pin available.
> Lovely!  
>
>> So if this change doesn't
>> make it harder to add later if it's necessary, I would still be OK going
>> without full support for now.
> I stopped being lazy and opened the datasheet.
>
> Hmm. We have auxiliary channels as well.  oh goody.
> Considering just the fifo as that's the high performance route.
>
> Basically we can do headerless mode trivially as that's just one buffer.
> (same ODR for all sensors).
> We could do headered version but without messing with multiple buffers
> that would be only when all sensors have same ODR (after a messy
> transition period perhaps - that bit of the datasheet is less than
> intuitive!) The reason we might do headered mode is to support the
> timestamps but we can probably get those via a quick read of other
> registers after draining the fifo.

OK, that sounds good. It looks like the BMI323 driver approximates
timestamps by slicing up the time period between the last flush and the
current flush. It seems like that could also work.

If I understand it right, the simple way forward would be to use only
the fifo watermark interrupt, to set the fifo to headerless mode, and
only allow that buffer to be enabled when the ODR is the same between
the accel and gyro sensors.

Since that sounds like a fairly independent change, I can hold it for a
future patch, unless you think it belongs in this set.

Thank you for the rest of the feedback and advice, I really appreciate
it. I think I have enough for another revision soon.

Justin

> So I'm fine with just not supporting the weird corner cases unless
> we get someone turning up who
> a) cares
> b) if foolish (or motivated) enough to do the necessary work 
> c) (if they are lucky) we have the infrastructure in place because someone
>    else needed the missing bits.
>
> Jonathan
>
>
>> 
>> Justin

Re: [PATCH 3/3] iio: imu: Add scale and sampling frequency to BMI270 IMU

[Jonathan Cameron]

On Tue, 15 Oct 2024 18:20:24 -0700
Justin Weiss <justin@justinweiss.com> wrote:

> Jonathan Cameron <jic23@kernel.org> writes:
> 
> > On Sun, 13 Oct 2024 13:55:36 -0700
> > Justin Weiss <justin@justinweiss.com> wrote:
> >  
> >> Jonathan Cameron <jic23@kernel.org> writes:
> >>   
> >> > On Sat, 12 Oct 2024 19:45:18 -0700
> >> > Justin Weiss <justin@justinweiss.com> wrote:
> >> >    
> >> >> Jonathan Cameron <jic23@kernel.org> writes:
> >> >>     
> >> >> > On Fri, 11 Oct 2024 08:37:49 -0700
> >> >> > Justin Weiss <justin@justinweiss.com> wrote:
> >> >> >      
> >> >> >> Add read and write functions and create _available entries. Use
> >> >> >> IIO_CHAN_INFO_SAMP_FREQ instead of IIO_CHAN_INFO_FREQUENCY to match
> >> >> >> the BMI160 / BMI323 drivers.      
> >> >> >
> >> >> > Ah.  Please break dropping _FREQUENCY change out as a separate fix
> >> >> > with fixes tag etc and drag it to start of the patch. It was never
> >> >> > wired to anything anyway
> >> >> >
> >> >> > That's a straight forward ABI bug so we want that to land ahead
> >> >> > of the rest of the series.      
> >> >> 
> >> >> Thanks, I'll pull that into its own change and make it the first patch.
> >> >>     
> >> >> > Does this device have a data ready interrupt and if so what affect
> >> >> > do the different ODRs for each type of sensor have on that?
> >> >> > If there are separate data ready signals, you probably want to 
> >> >> > go with a dual buffer setup from the start as it is hard to unwind
> >> >> > that later.      
> >> >> 
> >> >> It has data ready interrupts for both accelerometer and gyroscope and a
> >> >> FIFO interrupt. I had held off on interrupts to keep this change
> >> >> simpler, but if it's a better idea to get it in earlier, I can add it
> >> >> alongside the triggered buffer change.    
> >> >
> >> > Ok. So the challenge is that IIO buffers are only described by external
> >> > metadata.  We don't carry tags within them.  Hence if you are using
> >> > either effectively separate datastreams (the two data ready interrupts)
> >> > or a fifo that is tagged data (how this difference of speed is normally handled
> >> > if it's one buffer) then when we push them into IIO buffers, they have
> >> > to go into separate buffers.
> >> >
> >> > In older drivers this was done via the heavy weight option of registering
> >> > two separate IIO devices. Today we have the ability to support multiple buffers
> >> > in one driver. I'm not sure we've yet used it for this case, so I think
> >> > there may still be some gaps around triggering that will matter for the
> >> > separate dataready interrupt case (fifo is fine as no trigger involved).
> >> > Looking again at that code, it looks like there may need to be quite
> >> > a bit more work to cover this case proeprly.
> >> >
> >> > We may be able to have a migration path from the simple case you have
> >> > (where timing is an external trigger) to multiple buffers.
> >> > It would involve:
> >> > 1) Initial solution where the frequencies must match if the fifo is in use.
> >> >    Non fifo trigger from data ready might work but we'd need to figure out
> >> >    if they run in close enough timing.
> >> > 2) Solution where we add a second buffer and if the channels are enabled
> >> >    in that we can allow separate timing for the two sensor types.
> >> >
> >> > This is one of those hardware features that seems like a good idea
> >> > from the hardware design point of view but assumes a very specific
> >> > sort of software model :(
> >> >
> >> > Jonathan    
> >> 
> >> Hm, that does sound tricky. If there's an example I can follow, I can
> >> make an attempt at it.  
> >
> > I don't think it ever got used for a device like this - so probably no
> > examples, but I might have forgotten one. (this was a few years back).
> >  
> >> Otherwise, if there's a change I can make now
> >> that would help with migrating in the future, I can do that instead.
> >> 
> >> Of the devices I've looked at, only one has had the interrupts usable
> >> and that one only had a single pin available.  
> > Lovely!  
> >  
> >> So if this change doesn't
> >> make it harder to add later if it's necessary, I would still be OK going
> >> without full support for now.  
> > I stopped being lazy and opened the datasheet.
> >
> > Hmm. We have auxiliary channels as well.  oh goody.
> > Considering just the fifo as that's the high performance route.
> >
> > Basically we can do headerless mode trivially as that's just one buffer.
> > (same ODR for all sensors).
> > We could do headered version but without messing with multiple buffers
> > that would be only when all sensors have same ODR (after a messy
> > transition period perhaps - that bit of the datasheet is less than
> > intuitive!) The reason we might do headered mode is to support the
> > timestamps but we can probably get those via a quick read of other
> > registers after draining the fifo.  
> 
> OK, that sounds good. It looks like the BMI323 driver approximates
> timestamps by slicing up the time period between the last flush and the
> current flush. It seems like that could also work.
> 
> If I understand it right, the simple way forward would be to use only
> the fifo watermark interrupt, to set the fifo to headerless mode, and
> only allow that buffer to be enabled when the ODR is the same between
> the accel and gyro sensors.
> 
> Since that sounds like a fairly independent change, I can hold it for a
> future patch, unless you think it belongs in this set.
Indeed fine to leave it as it stands for this series.
We've established a compatible path forwards if those features get added
so all looks good to me.

Jonathan

> 
> Thank you for the rest of the feedback and advice, I really appreciate
> it. I think I have enough for another revision soon.
> 
> Justin
> 
> > So I'm fine with just not supporting the weird corner cases unless
> > we get someone turning up who
> > a) cares
> > b) if foolish (or motivated) enough to do the necessary work 
> > c) (if they are lucky) we have the infrastructure in place because someone
> >    else needed the missing bits.
> >
> > Jonathan
> >
> >  
> >> 
> >> Justin  

Re: [PATCH 0/3] Add i2c driver for Bosch BMI260 IMU

[Jonathan Cameron]

On Fri, 11 Oct 2024 08:37:46 -0700
Justin Weiss <justin@justinweiss.com> wrote:

> Add support for the Bosch BMI260 IMU to the BMI270 device driver.
> 
> The BMI270 and BMI260 have nearly identical register maps, but have
> different chip IDs and firmware.
> 
> The BMI260 is the IMU on a number of handheld PCs. Unfortunately,
> these devices often misidentify it in ACPI as a BMI160 ("BMI0160," for
> example), and it can only be correctly identified using the chip
> ID. I've changed the driver to fail if the chip ID isn't recognized so
> the firmware initialization data isn't sent to incompatible devices.

So just to check, is the firmware always specific to an individual chip?

Normally we strongly resist hard checks on mismatched IDs because they break
the option for using fallback compatibles to get some support on older
kernels for newer devices, but if the firmware is locked to a
device then that is a good justification.  Fallback compatibles in DT
will never work here.

Note that means you need a specific compatible in
Documentation/devicetree/bindings/iio/imu/bosch,bmi270.yaml

Technically you could match on a single ID and figure it out, but that
will lead to potential confusion if an older kernel is used with a binding
written against current kernel and the driver just doesn't work. Not a regression
but in my view inelegant.


Make sure you include this detail about specific firmware selection in there
as well.

Jonathan

> 
> Also add triggered buffer and scale / sampling frequency attributes,
> which the input tools commonly used on handheld PCs require to support
> IMUs.
> 
> Like the BMI270, the BMI260 requires firmware to be provided.
> 
> Signed-off-by: Justin Weiss <justin@justinweiss.com>
> ---
> 
> Justin Weiss (3):
>   iio: imu: Add i2c driver for bmi260 imu
>   iio: imu: Add triggered buffer for Bosch BMI270 IMU
>   iio: imu: Add scale and sampling frequency to BMI270 IMU
> 
>  drivers/iio/imu/bmi270/Kconfig       |   1 +
>  drivers/iio/imu/bmi270/bmi270.h      |  24 +-
>  drivers/iio/imu/bmi270/bmi270_core.c | 369 ++++++++++++++++++++++++++-
>  drivers/iio/imu/bmi270/bmi270_i2c.c  |  22 +-
>  drivers/iio/imu/bmi270/bmi270_spi.c  |  11 +-
>  5 files changed, 413 insertions(+), 14 deletions(-)
> 
> 
> base-commit: 96be67caa0f0420d4128cb67f07bbd7a6f49e03a

Re: [PATCH 0/3] Add i2c driver for Bosch BMI260 IMU

[Justin Weiss]

Thanks for the review! I really appreciate it.

Jonathan Cameron <jic23@kernel.org> writes:

> On Fri, 11 Oct 2024 08:37:46 -0700
> Justin Weiss <justin@justinweiss.com> wrote:
>
>> The BMI260 is the IMU on a number of handheld PCs. Unfortunately,
>> these devices often misidentify it in ACPI as a BMI160 ("BMI0160," for
>> example), and it can only be correctly identified using the chip
>> ID. I've changed the driver to fail if the chip ID isn't recognized so
>> the firmware initialization data isn't sent to incompatible devices.
>
> So just to check, is the firmware always specific to an individual chip?

For these devices, yes. The BMI160 does not have firmware initialization
data. The 260 and 270 have different firmware data from each other.

> Normally we strongly resist hard checks on mismatched IDs because they break
> the option for using fallback compatibles to get some support on older
> kernels for newer devices, but if the firmware is locked to a
> device then that is a good justification.  Fallback compatibles in DT
> will never work here.

The specific problem I'm trying to avoid with this hard check is the
situation when a device actually has a BMI160, this driver matches
"BMI0160", and sends the BMI260 firmware data to a BMI160 chip.

I suppose this driver could target this situation by only failing to
probe if it detects the BMI160 chip ID. That would imply that the device
is a BMI160 and should not be handled by this driver.

Then, as you suggested in another response, the driver could check the
other chip IDs individually. If the driver detects the 260 chip ID, it
would send the BMI260 firmware and so on with the 270. Otherwise, it
would use the chip_info found by match_data. This would at least handle
the problem we see in shipped devices while keeping it flexible for the
future. I'm happy to make those changes if they make more sense to you.

There's an older thread here that provides more background about the
DSDT confusion:
https://lore.kernel.org/all/CAFqHKTm2WRNkcSoBEE=oNbfu_9d9RagQHLydmv6q1=snO_MXyA@mail.gmail.com/

Justin