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.0On 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,
> +};
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,
>> +};
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
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
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
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
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
© 2016 - 2024 Red Hat, Inc.