On Apr 07, 2016 21:36, Peter Meerwald-Stadler wrote:
>
> > The following functions are supported:
> > - write, read potentiometer value
> > - potentiometer scale
>
> minor comments below, nice driver
Thank you for your comments!
> > +#include <linux/cache.h>
>
> for what is cache.h needed?
It is not needed here. I will delete it in v2.
> > +static int ds1803_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val, int *val2, long mask)
> > +{
> > + struct ds1803_data *data = iio_priv(indio_dev);
> > + int pot = chan->channel;
> > + s32 ret;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + ret = i2c_smbus_read_word_swapped(data->client, 0);
>
> maybe a #define to explain the name of register 0
Well this zero is not register nor any kind of command that DS1803 needs to send
back pots values.
DS1803 only requires the standard R/W bit to be set as read.
I used _word_ function series to get back a word (16 bits) as this poti returns
16 bits.
How should I named it?
#define NON_COMMAND 0
?
Or should I use different function? (2x i2c_smbus_read_byte?)
The i2c_smbus_read_byte() function also used 0 as command
for its transfers...
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* Get bits for given pot */
> > + *val = (pot == 0 ? ret >> 8 : ret & 255);
>
> often 0xff is used to mask a byte
OK.
> > + return IIO_VAL_INT;
> > +
> > + case IIO_CHAN_INFO_SCALE:
> > + *val = 1000 * data->cfg->kohms;
> > + *val2 = DS1803_MAX_POS;
> > + return IIO_VAL_FRACTIONAL;
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static int ds1803_write_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int val, int val2, long mask)
> > +{
> > + struct ds1803_data *data = iio_priv(indio_dev);
> > + int pot = chan->channel;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + if (val > DS1803_MAX_POS || val < 0)
>
> check that val2 is 0 or use .write_raw_get_fmt
At this point I do not know why should I do it, but I will look into that.
> > + indio_dev->dev.parent = dev;
> > + indio_dev->info = &ds1803_info;
> > + indio_dev->channels = ds1803_channels;
> > + indio_dev->num_channels = DS1803_NUM_WIPERS;
>
> ARRAY_SIZE(ds1803_channels)
Good point.
--
Slawomir Stepien
