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

On 02/08/15 10:42, Lars-Peter Clausen wrote:

On 08/01/2015 05:58 AM, Matt Ranostay wrote:
[...]

quoted

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

Would do if spi, but in the case of i2c I thought all bus drivers were
obliged to deal with this rather than leaving it to the client drivers?

Has this changed?

	u16 buffer[5] ____cacheline_aligned;

quoted

+};

[...]

quoted

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

quoted

+{
+	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.

quoted

+
+	return ret;
+}

[...]

quoted

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

quoted

+	if (val < 0)
+		val = 0;
+
+	if (val > LIDAR_REG_DATA_MAX)
+		val = LIDAR_REG_DATA_MAX;
+
+	*reg = val;
+
+	return 0;
+}

[...]

quoted

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

quoted

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

[...]

quoted

+static struct iio_info lidar_info = {

const

quoted

+	.driver_module = THIS_MODULE,
+	.read_raw = lidar_read_raw,
+	.write_raw = lidar_write_raw,
+};

[...]

quoted

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

quoted

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

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html