Re: [PATCH v4] iio: accel: Add support for the Bosch-Sensortec BMI088

[Mike Looijmans <mike.looijmans@topic.nl>]

On 23-03-2020 12:31, Andy Shevchenko wrote:
> On Mon, Mar 23, 2020 at 10:28:30AM +0100, Mike Looijmans 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.
> 
> 
> Thank you, the comment about shared buffer given to v3 still applies.
> Also see below.
> 
> ...
> 
>> +enum bmi088_accel_axis {
>> +	AXIS_X,
>> +	AXIS_Y,
>> +	AXIS_Z,
> 
>> +	AXIS_MAX,
> 
> If it's a terminator entry, comma is not needed.

Actually it isn't used at all, I'll remove it.

> 
>> +};
> 
> ...
> 
>> +/* Available ODR (output data rates) in Hz */
>> +enum bmi088_odr_modes {
>> +	BMI088_ACCEL_MODE_ODR_12_5 = 0x5,
>> +	BMI088_ACCEL_MODE_ODR_25 = 0x6,
>> +	BMI088_ACCEL_MODE_ODR_50 = 0x7,
>> +	BMI088_ACCEL_MODE_ODR_100 = 0x8,
>> +	BMI088_ACCEL_MODE_ODR_200 = 0x9,
>> +	BMI088_ACCEL_MODE_ODR_400 = 0xa,
>> +	BMI088_ACCEL_MODE_ODR_800 = 0xb,
>> +	BMI088_ACCEL_MODE_ODR_1600 = 0xc,
>> +};
> 
> I'm wondering if you need this enum at all? Only 3 out of 8 are in use, and 25
> of them can be still derived from the 12.5 one.
> 
> Maybe replace with comment and ranges?
> 
> (But it's up to you, I have no strong opinion here)

Comment and ranges sounds better, it's all power-of-two stuff, and the 
values aren't actually being used.

> 
> ...
> 
>> +static int bmi088_accel_set_power_state(struct bmi088_accel_data *data, bool on)
>> +{
>> +	struct device *dev = regmap_get_device(data->regmap);
>> +	int ret;
>> +
>> +	if (on) {
>> +		ret = pm_runtime_get_sync(dev);
> 
> 		if (ret < 0)
> 			pm_runtime_put_noidle(dev);
> 
> See below.
> 
>> +	} else {
>> +		pm_runtime_mark_last_busy(dev);
>> +		ret = pm_runtime_put_autosuspend(dev);
>> +	}
>> +
>> +	if (ret < 0) {
>> +		dev_err(dev, "Failed: %s(%d)\n", __func__, on);
> 
>> +		if (on)
>> +			pm_runtime_put_noidle(dev);
> 
> Perhaps refactor as above?
> 
> In this case it maybe simple...
> 
>> +
>> +		return ret;
>> +	}
>> +
>> +	return 0;
> 
> ...like this
> 
> 	if (ret < 0)
> 		dev_err(dev, "Failed: %s(%d)\n", __func__, on);
> 
> 	return ret < 0 ? ret : 0;
> 
> (I guess compiler is clever enough to avoid condition twice, but again, I have
>   no strong opinion)

As most of the method body depends on that "bool" argument, I would 
actually just split it into separate "enable" and "disable" methods. 
Simpler to read and understand, and probably doesn't make a difference 
in compiled size either.

> 
>> +}
> 
> ...
> 
>> +static int bmi088_accel_enable(struct bmi088_accel_data *data, bool on_off)
>> +{
>> +	struct device *dev = regmap_get_device(data->regmap);
>> +	int ret;
>> +
> 
>> +	ret = regmap_write(data->regmap, BMI088_ACCEL_REG_PWR_CTRL,
>> +			   on_off ? 0x4 : 0x0);
> 
> I think
> 
> 	u32 val = on_off ? 0x4 : 0x0;
> 	...
> 	ret = regmap_write(data->regmap, BMI088_ACCEL_REG_PWR_CTRL, val);
> 
> will look better.
> 

Agree

> 
>> +	if (ret) {
>> +		dev_err(dev, "Error writing ACC_PWR_CTRL reg\n");
>> +		return ret;
>> +	}
> 
>> +	return 0;
>> +}
>> +
>> +/* In suspend mode, only the accelerometer is powered down. */
>> +static int bmi088_accel_set_mode(struct bmi088_accel_data *data,
>> +				enum bmi088_power_modes mode)
>> +{
>> +	struct device *dev = regmap_get_device(data->regmap);
>> +	int ret;
>> +
>> +	ret = regmap_write(data->regmap, BMI088_ACCEL_REG_PWR_CONF,
>> +			   mode == BMI088_ACCEL_MODE_SUSPEND ? 0x3 : 0x0);
> 
> Ditto.
> 
>> +	if (ret) {
>> +		dev_err(dev, "Error writing ACCEL_PWR_CONF reg\n");
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
> 
> ...
> 
>> +static int bmi088_accel_set_bw(struct bmi088_accel_data *data,
>> +				enum bmi088_odr_modes odr_mode,
>> +				enum bmi088_osr_modes osr_mode)
>> +{
>> +	struct device *dev = regmap_get_device(data->regmap);
>> +	int ret;
>> +	u8 value = (osr_mode << 4) | (odr_mode & 0xF);
> 
> _MASK (GENMASK() + _SHIFT? (See also below)
> 
> 	u8 value = (osr_mode << _SHIFT) | (odr_mode & _MASK);
> 	int ret;
> 
> (note reverse xmas tree ordering)
> 
>> +
>> +	ret = regmap_write(data->regmap, BMI088_ACCEL_REG_ACC_CONF, value);
>> +	if (ret) {
>> +		dev_err(dev, "Error writing ACCEL_PWR_CONF reg\n");
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int bmi088_accel_get_sample_freq(struct bmi088_accel_data *data,
>> +					int* val, int *val2)
>> +{
>> +	unsigned int value;
>> +	int ret;
> 
>> +	value &= 0xf; /* ODR in lower 4 bits */
> 
> _MASK? See above.
> 
>> +	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 = BMI088_ACCEL_MODE_ODR_1600;
>> +
>> +	if (val < 12 || val > 1600)
>> +		return -EINVAL;
>> +
> 
>> +	value = fls(val) + (BMI088_ACCEL_MODE_ODR_12_5 - 4);
> 
> Wouldn't be
> 
> 	value = fls(val) + 1;
> 
> more obvious? Or, perhaps,
> 
> 	roundup_pow_of_two()
> 
> ?
> 
>> +	return regmap_update_bits(data->regmap, BMI088_ACCEL_REG_ACC_CONF,
>> +				  0x0f, value);
> 
> _MASK ?
> 
>> +}
> 
>> +	if (ret)
>> +		return ret;
>> +
> 
>> +	*val = temp >> 5;
> 
> Magic shift.
> 
> ...
> 
>> +	s16 raw_val;
> 
>> +	ret = regmap_bulk_read(data->regmap,
>> +			       BMI088_ACCEL_AXIS_TO_REG(chan->scan_index),
>> +			       data->buffer, 2);
>> +	raw_val = get_unaligned_le16(data->buffer);
> 
> 
> I'm wondering if you can simple use le16_to_cpu()? I guess that buffer is
> always aligned and since you access it always by even addresses, it implies
> aligned access. Applies to other places as well.
> 
> ...
> 
>> +		case IIO_ACCEL:
> 
>> +		{
> 
> Why do you need block?
> 
>> +			ret = regmap_read(data->regmap,
>> +					  BMI088_ACCEL_REG_ACC_RANGE, val);
>> +			if (ret)
>> +				return ret;
>> +
> 
>> +			*val2 =  15 - (*val & 0x3);
> 
> Extra spaces.
> 
>> +			*val = 3 * 980;
>> +
>> +			return IIO_VAL_FRACTIONAL_LOG2;
>> +		}
>> +		default:
>> +			return -EINVAL;
>> +		}
> 
> ...
> 
>> +static const unsigned long bmi088_accel_scan_masks[] = {
>> +	BIT(AXIS_X) | BIT(AXIS_Y) | BIT(AXIS_Z),
>> +	0
> 
> + comma or is it agreed value for termination the list?
> 
>> +};
> 
> ...
> 
> 
>> +	usleep_range(BMI088_ACCEL_MAX_STARTUP_TIME_MS * 1000,
>> +		BMI088_ACCEL_MAX_STARTUP_TIME_MS * 1000 * 2);
> 
> 	unsigned long /* or what is used */ sleep = BMI088_ACCEL_MAX_STARTUP_TIME_MS * USEC_PER_MSEC;
> 	...
> 	usleep_range(sleep, 2 * sleep);
> 
> ?
> 
> ...
> 
>> +	addr[0] |= 0x80; /* bit7 = RW = '1' */
> 
> BIT(7) ?
> 
> ...
> 
>> +static const struct spi_device_id bmi088_accel_id[] = {
> 
>> +	{"bmi088_accel", 0},
> 
> ', 0' part is not needed.
> 
>> +	{}
>> +};
> 
> 
> 
>> +++ b/drivers/iio/accel/bmi088-accel.h
>> @@ -0,0 +1,12 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef BMI088_ACCEL_H
>> +#define BMI088_ACCEL_H
>> +
>> +extern const struct regmap_config bmi088_regmap_conf;
>> +extern const struct dev_pm_ops bmi088_accel_pm_ops;
> 
> Do you need extern?

probably not.

> 
>> +int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap, int irq,
>> +			    const char *name, bool block_supported);
>> +int bmi088_accel_core_remove(struct device *dev);
> 
> This needs
> 
> #include <linux/types.h>
> 
> struct device;
> struct regmap;
> 

Hmm, and "struct regmap_config" as well I guess (see above)


-- 
Mike Looijmans