Re: [PATCH 2/2] iio: proximity: add support for PulsedLight LIDAR

[Lars-Peter Clausen <lars@metafoo.de>]

On 08/01/2015 05:58 AM, Matt Ranostay wrote:
[...]
> +
> +struct lidar_data {
> +	struct mutex lock;
> +	struct iio_dev *indio_dev;
> +	struct i2c_client *client;
> +
> +	/* config */
> +	int calib_bias;
> +
> +	u16 buffer[5]; /* 2 byte distance + 8 byte timestamp */

Needs to be in its own cacheline to avoid issues if the I2C controller is
using DMA.

	u16 buffer[5] ____cacheline_aligned;

> +};
[...]
> +static inline int lidar_read_byte(struct lidar_data *data, int reg)

I'd drop the inline. The compiler is smart enough to figure out whether it
makes sense to inline it or not.

> +{
> +	struct i2c_client *client = data->client;
> +	int ret;
> +
> +	ret = i2c_smbus_write_byte(client, reg);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "cannot write addr value");
> +		return ret;
> +	}
> +
> +	ret = i2c_smbus_read_byte(client);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "cannot read data value");
> +		return ret;
> +	}

Instead of using a write_byte/read_byte combination rather use
i2c_smbus_read_byte_data(). I think that will do the same, but in one atomic
operation.

> +
> +	return ret;
> +}
[...]
> +static int lidar_read_measurement(struct lidar_data *data, u16 *reg)
> +{
> +	int ret;
> +	int val;
> +
> +	ret = lidar_read_byte(data, LIDAR_REG_DATA_HBYTE);
> +	if (ret < 0)
> +		return ret;
> +	val = ret << 8;
> +
> +	ret = lidar_read_byte(data, LIDAR_REG_DATA_LBYTE);
> +	if (ret < 0)
> +		return ret;
> +	val |= ret;
> +
> +	/* correct any possible overflow or underflow */
> +	val += data->calib_bias / 10000;

Typically calib bias is only meant for a offset correction that is done in
hardware rather than in software. What's the usecase for doing it in
kernelspace rather than in userspace?

> +	if (val < 0)
> +		val = 0;
> +
> +	if (val > LIDAR_REG_DATA_MAX)
> +		val = LIDAR_REG_DATA_MAX;
> +
> +	*reg = val;
> +
> +	return 0;
> +}
[...]
> +static int lidar_read_raw(struct iio_dev *indio_dev,
> +			  struct iio_chan_spec const *chan,
> +			  int *val, int *val2, long mask)
> +{
> +	struct lidar_data *data = iio_priv(indio_dev);
> +	int ret = -EINVAL;
> +
> +	if (iio_buffer_enabled(indio_dev) && mask == IIO_CHAN_INFO_RAW)
> +		return -EBUSY;
> +
> +	mutex_lock(&data->lock);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW: {
> +		u16 reg;
> +
> +		ret = lidar_get_measurement(data, &reg);
> +		if (!ret) {
> +			*val = reg / 100;
> +			*val2 = (reg % 100) * 10000;
> +			ret = IIO_VAL_INT_PLUS_MICRO;

The raw attribute is supposed to return the raw value as a IIO_VAL_INT. A
scale attribute should be used to indicate the conversion factor to get the
proper unit.

> +		}
> +		break;
> +	}
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		*val = 0;
> +		*val2 = data->calib_bias;
> +		ret = IIO_VAL_INT_PLUS_MICRO;
> +		break;
> +	}
> +
> +	mutex_unlock(&data->lock);
> +
> +	return ret;
> +}
[...]
> +static struct iio_info lidar_info = {

const

> +	.driver_module = THIS_MODULE,
> +	.read_raw = lidar_read_raw,
> +	.write_raw = lidar_write_raw,
> +};
[...]
> +static struct i2c_driver lidar_driver = {
> +	.driver = {
> +		.name	= LIDAR_DRV_NAME,
> +		.owner	= THIS_MODULE,

You added a DT vendor prefix, but there is no of match table for the driver.

> +	},
> +	.probe		= lidar_probe,
> +	.remove		= lidar_remove,
> +	.id_table	= lidar_id,
> +};
> +module_i2c_driver(lidar_driver);