On Mon, Aug 3, 2015 at 1:00 AM, Lars-Peter Clausen <lars@xxxxxxxxxx> wrote: > On 08/02/2015 11:28 PM, Matt Ranostay wrote: >> On Sun, Aug 2, 2015 at 2:42 AM, Lars-Peter Clausen <lars@xxxxxxxxxx> wrote: >>> 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. >> >> 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 :). > >> >>> >>> 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. >>> >> Got it. >> >>>> +{ >>>> + 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. > > [...] >>>> +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@xxxxxxxxxxxxxxx > 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html