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

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

I'll post a v4 with all requested changes below. Just one remark inline 
about the shared buffer and sizeof.


On 22-03-2020 01:16, Andy Shevchenko wrote:
> On Thu, Mar 19, 2020 at 04:48:42PM +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.
> 
> ...
> 
>> +#include <linux/acpi.h>
> 
> How this is being used?
> 
> ...
> 
>> +EXPORT_SYMBOL_GPL(bmi088_regmap_conf);
>> +
>> +
> 
> Too many blank lines
> 
> ...
> 
>> +#ifdef CONFIG_PM
> 
> Hmm... Why do you need this? Doesn't PM runtime have the stubs for !PM case?
> 
>> +#else
>> +static int bmi088_accel_set_power_state(struct bmi088_accel_data *data,
>> +	bool on)
>> +{
>> +	return 0;
>> +}
>> +#endif
> 
> ...
> 
>> +	ret = regmap_write(data->regmap, BMI088_ACCEL_REG_PWR_CTRL,
>> +				on_off ? 0x4 : 0x0);
>> +	if (ret < 0) {
> 
> Why all these ' < 0'? I don't remember that regmap API returns positive numbers
> from this type of calls.
> 
> ...
> 
>> +
>> +static int bmi088_accel_set_sample_freq(struct bmi088_accel_data *data, int val)
>> +{
>> +	unsigned int value = BMI088_ACCEL_MODE_ODR_1600;
>> +	unsigned int freq = 1600;
>> +	int ret;
>> +
>> +	if (val < 12 || val > 1600)
>> +		return -EINVAL;
> 
>> +	while (freq > val && value > BMI088_ACCEL_MODE_ODR_12_5) {
>> +		--value;
>> +		freq >>= 1;
>> +	}
> 
> You can use bit operations instead of loop.
> 
>> +	ret = regmap_update_bits(data->regmap, BMI088_ACCEL_REG_ACC_CONF,
>> +				 0x0f, value);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return 0;
> 
> 	return regmap_...(...);
> 
>> +}
> 
> ...
> 
>> +static int bmi088_accel_get_temp(struct bmi088_accel_data *data, int *val)
>> +{
>> +	int ret;
>> +	__s16 temp;
>> +
>> +	mutex_lock(&data->mutex);
> 
>> +	ret = regmap_bulk_read(data->regmap, BMI088_ACCEL_REG_TEMP,
>> +			       &data->buffer, 2);
> 
> sizeof() ?

The buffer is a shared buffer, it will grow to accommodate reading all 
axis and timestamp in a single read (9 bytes) and for FIFO reads in 
foreseeable future.

I could use sizeof(temp) here though, but that wouldn't that be more 
confusing?


> 
>> +	temp = get_unaligned_be16(data->buffer);
>> +
>> +	mutex_unlock(&data->mutex);
>> +
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	*val = temp >> 5;
>> +
>> +	return IIO_VAL_INT;
>> +}
> 
> ...
> 
>> +static int bmi088_accel_get_axis(struct bmi088_accel_data *data,
>> +				 struct iio_chan_spec const *chan,
>> +				 int *val)
>> +{
>> +	int ret;
> 
>> +	__s16 raw_val;
> 
> s16 ?
> Also applies to the rest.
> 
>> +	return IIO_VAL_INT;
>> +}
> 
> ...
> 
>> +static int bmi088_accel_write_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_SAMP_FREQ:
>> +		mutex_lock(&data->mutex);
>> +		ret = bmi088_accel_set_sample_freq(data, val);
>> +		mutex_unlock(&data->mutex);
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
>> +	}
>> +
> 
>> +	return ret;
> 
> Replace break with this?
> 
>> +}
> 
> ...
> 
>> +static struct attribute *bmi088_accel_attributes[] = {
>> +	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
> 
>> +	NULL,
> 
> Terminators are good w/o comma.
> 
>> +};
> 
> ...
> 
>> +static const unsigned long bmi088_accel_scan_masks[] = {
>> +				BIT(AXIS_X) | BIT(AXIS_Y) | BIT(AXIS_Z),
>> +				0};
> 
> Indentation issues.
> 
>> +
>> +
> 
> Too many blank lines.
> 
> ...
> 
>> +	/* Validate chip ID */
>> +	for (i = 0; i < ARRAY_SIZE(bmi088_accel_chip_info_tbl); i++) {
>> +		if (bmi088_accel_chip_info_tbl[i].chip_id == val) {
>> +			data->chip_info = &bmi088_accel_chip_info_tbl[i];
>> +			break;
>> +		}
>> +	}
> 
>> +
> 
> No need for this blank line.
> 
>> +	if (!data->chip_info) {
> 
> More usual pattern to check loop counter against array size.
> 
>> +		dev_err(dev, "Invalid chip %x\n", val);
>> +		return -ENODEV;
>> +	}
> 
> ...
> 
>> +	/* Set Default Range */
>> +	ret = regmap_write(data->regmap, BMI088_ACCEL_REG_ACC_RANGE,
>> +			   BMI088_ACCEL_RANGE_6G);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return 0;
> 
> 	return regmap_...();
> 
> ...
> 
>> +	ret = iio_device_register(indio_dev);
>> +	if (ret < 0) {
>> +		dev_err(dev, "Unable to register iio device\n");
>> +		return ret;
>> +	}
>> +
>> +	return 0;
> 
> 	if (ret)
> 		dev_err();
> 	return ret;
> 
> ...
> 
>> +	ret = bmi088_accel_set_mode(data, BMI088_ACCEL_MODE_SUSPEND);
>> +	if (ret < 0)
>> +		return -EAGAIN;
> 
> Is this error code dictated by PM runtime API?
> Otherwise
> 	return bmi088_...();
> 
>> +
>> +	return 0;
> 


-- 
Mike Looijmans