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 linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
