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.
>
> 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.
>
>> +
>> + return ret;
>> +}
> [...]
>> +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?
There is hardware functionality for this but do does nothing more than
add/subtraction to the final result.
Could just drop it completely.
>
>> + if (val < 0)
>> + val = 0;
>> +
>> + if (val > LIDAR_REG_DATA_MAX)
>> + val = LIDAR_REG_DATA_MAX;
>> +
>> + *reg = val;
>> +
>> + return 0;
>> +}
> [...]
>> +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, ®);
>> + 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.
Ok makes sense.
>
>> + }
>> + 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;
>> +}
> [...]
>> +static struct iio_info lidar_info = {
>
> const
>
>> + .driver_module = THIS_MODULE,
>> + .read_raw = lidar_read_raw,
>> + .write_raw = lidar_write_raw,
>> +};
> [...]
>> +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
>
>> + },
>> + .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@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
