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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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() ?

> +	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;

-- 
With Best Regards,
Andy Shevchenko