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