| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87jzecpvpd.fsf@justinweiss.com>
Date: Sat, 12 Oct 2024 19:45:18 -0700
From: Justin Weiss <justin@...tinweiss.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Alex Lanzano <lanzano.alex@...il.com>, Lars-Peter Clausen
<lars@...afoo.de>, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org, "Derek J . Clark"
<derekjohn.clark@...il.com>, Philip Müller
<philm@...jaro.org>
Subject: Re: [PATCH 3/3] iio: imu: Add scale and sampling frequency to
BMI270 IMU
Jonathan Cameron <jic23@...nel.org> writes:
> On Fri, 11 Oct 2024 08:37:49 -0700
> Justin Weiss <justin@...tinweiss.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@...tinweiss.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,
>> +};
Powered by blists - more mailing lists