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

On Mon, Aug 3, 2015 at 1:00 AM, Lars-Peter Clausen [off-list ref] wrote:

On 08/02/2015 11:28 PM, Matt Ranostay wrote:

quoted

On Sun, Aug 2, 2015 at 2:42 AM, Lars-Peter Clausen [off-list ref] wrote:

quoted

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.

Ah though this was only needed for SPI.

At least some I2C master drivers directly use the buffer for DMA. But I was
being stupid here anyway, you don't actually pass the buffer itself to the
I2C transfer functions so everything is fine as it was.

Will back that change out in v3 :).

quoted

quoted

        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.

Got it.

quoted

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.

Yes I would normally do that but this device doesn't seem to support
that functionally, always returns zeros.

That's an interesting device. Maybe add a comment explaining the oddity. I'm
sure I'm not the only one who'll wonder about this.

Ok will do. It seems to need a STOP condition between the read and
write. Hence why i2c_smbus_read_byte_data doesn't work.

[...]

quoted

quoted

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.

So without of_match_table it isn't needed to have a vendor id?
"pulsedlight,lidar" maps to the i2c_device_id

I thinking the other way around. If you intend to instantiate the device via
devicetree it is better to explicit add a of_device_id table rather than
relying on the implicit mechanism that uses i2c_device_id.

You should also add an entry for the device to
Documentation/devicetree/bindings/i2c/trivial-devices.txt

Ok seems logical will do in v3.

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

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