2022-05-09 13:59:17

by LI Qingwu

Subject: [PATCH V1 1/5] iio: accel: bmi088: Modified the scale calculate

The units after application of scale are 100*m/s^2,
The scale calculation is only for the device
with the range of 3, 6, 12, and 24g,
but some other chips have a range of 2, 4, 6, and 8g.

Modified the formula to a scale list.
The scales in the list are calculated by 1/sensitivity*9.8.
The new units after the application of scale are m/s^2.

Signed-off-by: LI Qingwu <[email protected]>
---
drivers/iio/accel/bmi088-accel-core.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/accel/bmi088-accel-core.c b/drivers/iio/accel/bmi088-accel-core.c
index a06dae5c971d..0c58ffdd00e3 100644
--- a/drivers/iio/accel/bmi088-accel-core.c
+++ b/drivers/iio/accel/bmi088-accel-core.c
@@ -119,6 +119,7 @@ struct bmi088_accel_chip_info {
u8 chip_id;
const struct iio_chan_spec *channels;
int num_channels;
+ const int scale_table[4][2];
};

struct bmi088_accel_data {
@@ -280,6 +281,7 @@ static int bmi088_accel_read_raw(struct iio_dev *indio_dev,
struct bmi088_accel_data *data = iio_priv(indio_dev);
struct device *dev = regmap_get_device(data->regmap);
int ret;
+ int reg;

switch (mask) {
case IIO_CHAN_INFO_RAW:
@@ -330,13 +332,12 @@ static int bmi088_accel_read_raw(struct iio_dev *indio_dev,
return ret;

ret = regmap_read(data->regmap,
- BMI088_ACCEL_REG_ACC_RANGE, val);
+ BMI088_ACCEL_REG_ACC_RANGE, &reg);
if (ret)
goto out_read_raw_pm_put;
-
- *val2 = 15 - (*val & 0x3);
- *val = 3 * 980;
- ret = IIO_VAL_FRACTIONAL_LOG2;
+ *val = data->chip_info->scale_table[reg][0];
+ *val2 = data->chip_info->scale_table[reg][1];
+ ret = IIO_VAL_INT_PLUS_MICRO;

goto out_read_raw_pm_put;
default:
@@ -432,6 +433,7 @@ static const struct bmi088_accel_chip_info bmi088_accel_chip_info_tbl[] = {
.chip_id = 0x1E,
.channels = bmi088_accel_channels,
.num_channels = ARRAY_SIZE(bmi088_accel_channels),
+ .scale_table = {{0, 897}, {0, 1795}, {0, 3590}, {0, 7179}},
},
};

--
2.25.1

2022-05-10 07:53:14

by Alexandru Ardelean

Subject: Re: [PATCH V1 1/5] iio: accel: bmi088: Modified the scale calculate

On Mon, May 9, 2022 at 4:46 PM LI Qingwu
<[email protected]> wrote:
>
> The units after application of scale are 100*m/s^2,
> The scale calculation is only for the device
> with the range of 3, 6, 12, and 24g,
> but some other chips have a range of 2, 4, 6, and 8g.
>
> Modified the formula to a scale list.
> The scales in the list are calculated by 1/sensitivity*9.8.
> The new units after the application of scale are m/s^2.
>
> Signed-off-by: LI Qingwu <[email protected]>
> ---
> drivers/iio/accel/bmi088-accel-core.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/accel/bmi088-accel-core.c b/drivers/iio/accel/bmi088-accel-core.c
> index a06dae5c971d..0c58ffdd00e3 100644
> --- a/drivers/iio/accel/bmi088-accel-core.c
> +++ b/drivers/iio/accel/bmi088-accel-core.c
> @@ -119,6 +119,7 @@ struct bmi088_accel_chip_info {
> u8 chip_id;
> const struct iio_chan_spec *channels;
> int num_channels;
> + const int scale_table[4][2];
> };
>
> struct bmi088_accel_data {
> @@ -280,6 +281,7 @@ static int bmi088_accel_read_raw(struct iio_dev *indio_dev,
> struct bmi088_accel_data *data = iio_priv(indio_dev);
> struct device *dev = regmap_get_device(data->regmap);
> int ret;
> + int reg;
>
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> @@ -330,13 +332,12 @@ static int bmi088_accel_read_raw(struct iio_dev *indio_dev,
> return ret;
>
> ret = regmap_read(data->regmap,
> - BMI088_ACCEL_REG_ACC_RANGE, val);
> + BMI088_ACCEL_REG_ACC_RANGE, &reg);

Curios: do we need to validate the value of `reg` here?
If it goes outside of the 0...3 range, I can see a bit of access
violation (kernel panic) happening.

> if (ret)
> goto out_read_raw_pm_put;
> -
> - *val2 = 15 - (*val & 0x3);
> - *val = 3 * 980;
> - ret = IIO_VAL_FRACTIONAL_LOG2;

Not sure what the policy is about changing this now.
I'll leave this to Jonathan.

> + *val = data->chip_info->scale_table[reg][0];
> + *val2 = data->chip_info->scale_table[reg][1];
> + ret = IIO_VAL_INT_PLUS_MICRO;
>
> goto out_read_raw_pm_put;
> default:
> @@ -432,6 +433,7 @@ static const struct bmi088_accel_chip_info bmi088_accel_chip_info_tbl[] = {
> .chip_id = 0x1E,
> .channels = bmi088_accel_channels,
> .num_channels = ARRAY_SIZE(bmi088_accel_channels),
> + .scale_table = {{0, 897}, {0, 1795}, {0, 3590}, {0, 7179}},

I like the idea of this scale_table.
It makes things easier to see (what's happening) without looking into
the datasheet much.

> },
> };
>
> --
> 2.25.1
>

2022-05-15 18:43:26

by Jonathan Cameron

Subject: Re: [PATCH V1 1/5] iio: accel: bmi088: Modified the scale calculate

On Tue, 10 May 2022 09:01:20 +0300
Alexandru Ardelean <[email protected]> wrote:

> On Mon, May 9, 2022 at 4:46 PM LI Qingwu
> <[email protected]> wrote:
> >
> > The units after application of scale are 100*m/s^2,
> > The scale calculation is only for the device
> > with the range of 3, 6, 12, and 24g,
> > but some other chips have a range of 2, 4, 6, and 8g.
> >
> > Modified the formula to a scale list.
> > The scales in the list are calculated by 1/sensitivity*9.8.
> > The new units after the application of scale are m/s^2.
> >
> > Signed-off-by: LI Qingwu <[email protected]>
> > ---
> > drivers/iio/accel/bmi088-accel-core.c | 12 +++++++-----
> > 1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iio/accel/bmi088-accel-core.c b/drivers/iio/accel/bmi088-accel-core.c
> > index a06dae5c971d..0c58ffdd00e3 100644
> > --- a/drivers/iio/accel/bmi088-accel-core.c
> > +++ b/drivers/iio/accel/bmi088-accel-core.c
> > @@ -119,6 +119,7 @@ struct bmi088_accel_chip_info {
> > u8 chip_id;
> > const struct iio_chan_spec *channels;
> > int num_channels;
> > + const int scale_table[4][2];
> > };
> >
> > struct bmi088_accel_data {
> > @@ -280,6 +281,7 @@ static int bmi088_accel_read_raw(struct iio_dev *indio_dev,
> > struct bmi088_accel_data *data = iio_priv(indio_dev);
> > struct device *dev = regmap_get_device(data->regmap);
> > int ret;
> > + int reg;
> >
> > switch (mask) {
> > case IIO_CHAN_INFO_RAW:
> > @@ -330,13 +332,12 @@ static int bmi088_accel_read_raw(struct iio_dev *indio_dev,
> > return ret;
> >
> > ret = regmap_read(data->regmap,
> > - BMI088_ACCEL_REG_ACC_RANGE, val);
> > + BMI088_ACCEL_REG_ACC_RANGE, &reg);
>
> Curios: do we need to validate the value of `reg` here?
> If it goes outside of the 0...3 range, I can see a bit of access
> violation (kernel panic) happening.
>
> > if (ret)
> > goto out_read_raw_pm_put;
> > -
> > - *val2 = 15 - (*val & 0x3);
> > - *val = 3 * 980;
> > - ret = IIO_VAL_FRACTIONAL_LOG2;
>
> Not sure what the policy is about changing this now.
> I'll leave this to Jonathan.

It shouldn't be a problem as userspace ABI is a decimal string either way.
Only impact might be on precision and those tend to be minor.

>
> > + *val = data->chip_info->scale_table[reg][0];
> > + *val2 = data->chip_info->scale_table[reg][1];
> > + ret = IIO_VAL_INT_PLUS_MICRO;
> >
> > goto out_read_raw_pm_put;
> > default:
> > @@ -432,6 +433,7 @@ static const struct bmi088_accel_chip_info bmi088_accel_chip_info_tbl[] = {
> > .chip_id = 0x1E,
> > .channels = bmi088_accel_channels,
> > .num_channels = ARRAY_SIZE(bmi088_accel_channels),
> > + .scale_table = {{0, 897}, {0, 1795}, {0, 3590}, {0, 7179}},
>
> I like the idea of this scale_table.
> It makes things easier to see (what's happening) without looking into
> the datasheet much.
>
> > },
> > };
> >
> > --
> > 2.25.1
> >