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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux