On Fri, 11 Oct 2024 08:37:49 -0700
Justin Weiss <justin@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> ---
> 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,
> +};
