| 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: <51d63f6b-a561-88f3-3718-b04539a7b3be@topic.nl>
Date: Mon, 30 Mar 2020 10:07:00 +0200
From: Mike Looijmans <mike.looijmans@...ic.nl>
To: Jonathan Cameron <jic23@...nel.org>
CC: devicetree@...r.kernel.org, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org, knaack.h@....de, lars@...afoo.de,
pmeerw@...erw.net, robh+dt@...nel.org, mark.rutland@....com
Subject: Re: [PATCH v5 2/2] iio: accel: Add support for the Bosch-Sensortec
BMI088
On 28-03-2020 17:20, Jonathan Cameron wrote:
> On Tue, 24 Mar 2020 09:03:08 +0100
> Mike Looijmans <mike.looijmans@...ic.nl> wrote:
>
>> The BMI088 is a combined module with both accelerometer and gyroscope.
>> This adds the accelerometer driver support for the SPI interface.
>> The gyroscope part is already supported by the BMG160 driver.
>>
>> Signed-off-by: Mike Looijmans <mike.looijmans@...ic.nl>
>
> Hi Mike,
>
> A few tiny things from me.
>
> For the sampling frequency, I wonder if we are better off going back
> to the list of values, but then also using the read_avail infrastructure
> to avoid having to carry them as a string as well?
The frequency range is just a simple power-of-two formula, so my take
was that a table would just be a waste of memory and resources.
A table lookup costs more resources and requires more code in this case.
>
> Thanks,
>
> Jonathan
>
>>
>> diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
>> index 5d91a6dda894..7ed9c82b731b 100644
>> --- a/drivers/iio/accel/Kconfig
>> +++ b/drivers/iio/accel/Kconfig
>> @@ -151,6 +151,23 @@ config BMC150_ACCEL_SPI
>> tristate
>> select REGMAP_SPI
>>
>> +config BMI088_ACCEL
>> + tristate "Bosch BMI088 Accelerometer Driver"
>> + select IIO_BUFFER
>> + select IIO_TRIGGERED_BUFFER
>> + select REGMAP
>> + select BMI088_ACCEL_SPI
>> + help
>> + Say yes here to build support for the Bosch BMI088 accelerometer.
>> +
>> + This is a combo module with both accelerometer and gyroscope.
>> + This driver is only implementing accelerometer part, which has
>> + its own address and register map.
>> +
>> +config BMI088_ACCEL_SPI
>> + tristate
>> + select REGMAP_SPI
>> +
>
> Hmm. So we list this driver even if SPI is disabled. Generally we try
> not to do that, as it makes for lots of things to pick on devices that
> don't actually have an SPI bus.
>
> Hence, please add a depends to the first Kconfig symbol so it's hidden
> if no SPI. When I2C is added it can become at least one of the two.
>
I'm okay with a depends on SPI. Adding the I2C support is not difficult,
but I don't want to release something into the kernel that I cannot test.
> ...
>
>> +
>> +static int bmi088_accel_get_sample_freq(struct bmi088_accel_data *data,
>> + int *val, int *val2)
>> +{
>> + unsigned int value;
>> + int ret;
>> +
>> + ret = regmap_read(data->regmap, BMI088_ACCEL_REG_ACC_CONF,
>> + &value);
>> + if (ret)
>> + return ret;
>> +
>> + value &= BMI088_ACCEL_MODE_ODR_MASK;
>> + if (value == BMI088_ACCEL_MODE_ODR_12_5) {
>> + *val = 12;
>> + *val2 = 500000;
>> + ret = IIO_VAL_INT_PLUS_MICRO;
>> + } else {
>> + *val = 25 << (value - BMI088_ACCEL_MODE_ODR_25);
>> + *val2 = 0;
>> + ret = IIO_VAL_INT;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int bmi088_accel_set_sample_freq(struct bmi088_accel_data *data, int val)
>> +{
>> + unsigned int value;
>> +
>> + if (val < 12 || val > 1600)
>> + return -EINVAL;
>> +
>> + value = fls(val) + 1;
>
> This leads to some 'novel' rounding to my mind.
>
> (12,16] = 12.5
> (16,32] = 25
> (32,64] = 50
> (64,128] = 100
>
> Generally we want to go faster if anything when talking about sampling frequencies,
> so I'd either like to see round up or precise value matching only.
I went for simplicity. The driver reports an "avail" range, so users
should not expect other values like "70" to actually work. The above is
the shortest inversion of the get_sample_freq function.
Just wanted to make it so that obvious things would work, and I feared
that a range would require one to spec "100.000" in decimal format just
because of the existence of the 12.5 value. So the driver is a bit
forgiving in that specifying "12" or "13" will also work.
For a more exact match I could also add something like:
if (val > 13 && (25 << (value - BMI088_ACCEL_MODE_ODR_25 ) != val)
return -EINVAL;
this would return -EINVAL for values like "26" or "1599".
>
>> +
>> + return regmap_update_bits(data->regmap, BMI088_ACCEL_REG_ACC_CONF,
>> + BMI088_ACCEL_MODE_ODR_MASK, value);
>> +}
>> +
>
> ...
>
>> +
>> +static int bmi088_accel_read_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan,
>> + int *val, int *val2, long mask)
>> +{
>> + struct bmi088_accel_data *data = iio_priv(indio_dev);
>> + int ret;
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_RAW:
>> + switch (chan->type) {
>> + case IIO_TEMP:
>> + return bmi088_accel_get_temp(data, val);
>> + case IIO_ACCEL:
>> + ret = iio_device_claim_direct_mode(indio_dev);
>> + if (ret)
>> + return ret;
>> +
>> + ret = bmi088_accel_get_axis(data, chan, val);
>> + iio_device_release_direct_mode(indio_dev);
>> + if (ret)
>> + return ret;
>> +
>> + return IIO_VAL_INT;
>> + default:
>> + return -EINVAL;
>> + }
>> + case IIO_CHAN_INFO_OFFSET:
>> + switch (chan->type) {
>> + case IIO_TEMP:
>> + /* Offset applies before scale */
>> + *val = BMI088_ACCEL_TEMP_OFFSET/BMI088_ACCEL_TEMP_UNIT;
>> + return IIO_VAL_INT;
>> + default:
>> + return -EINVAL;
>> + }
>> + case IIO_CHAN_INFO_SCALE:
>> + *val = 0;
>
> Why? In error paths it's not used, and it's set in the other two paths.
will remove
>
>> + switch (chan->type) {
>> + case IIO_TEMP:
>> + /* 0.125 degrees per LSB */
>> + *val = BMI088_ACCEL_TEMP_UNIT;
>> + return IIO_VAL_INT;
>> + case IIO_ACCEL:
>> + ret = regmap_read(data->regmap,
>> + BMI088_ACCEL_REG_ACC_RANGE, val);
>> + if (ret)
>> + return ret;
>> +
>> + *val2 = 15 - (*val & 0x3);
>> + *val = 3 * 980;
>> +
>> + return IIO_VAL_FRACTIONAL_LOG2;
>> + default:
>> + return -EINVAL;
>> + }
>> + case IIO_CHAN_INFO_SAMP_FREQ:
>> + mutex_lock(&data->mutex);
>> + ret = bmi088_accel_get_sample_freq(data, val, val2);
>> + mutex_unlock(&data->mutex);
>> + return ret;
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
> ...
>
--
Mike Looijmans
Powered by blists - more mailing lists