On Mar 22, 2016 17:10, Joachim Eastwood wrote: > Hi Slawomir, > > On 22 March 2016 at 16:44, Slawomir Stepien <sst@xxxxxxxxx> wrote: > > The following functionalities are supported: > > - write, read from volatile memory > > > > Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22060b.pdf > > > > Signed-off-by: Slawomir Stepien <sst@xxxxxxxxx> > > --- > > > +#include <linux/module.h> > > +#include <linux/spi/spi.h> > > +#include <linux/err.h> > > + > > +#include <linux/iio/iio.h> > > Give that you use that you have a some OF stuff in your driver you > should also include <linux/of.h>. Same goes for <linux/mutex.h>. > I am sure this builds fine without those includes, but you should > explicitly include stuff that you use. Oh yes that's true. > While you're at it you could also put the includes in alphabetic order. OK > > +static int mcp4131_read_raw(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, > > + int *val, int *val2, long mask) > > +{ > > + int err; > > + struct mcp4131_data *data = iio_priv(indio_dev); > > + int address = chan->channel; > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + mutex_lock(&data->lock); > > + > > + data->buf[0] = (address << MCP4131_WIPER_SHIFT) | MCP4131_READ; > > + data->buf[1] = 0; > > + > > + err = mcp4131_read(data->spi, data->buf, 2); > > + if (err) { > > + mutex_unlock(&data->lock); > > + return err; > > + } > > + > > + /* Error, bad address/command combination */ > > + if (!MCP4131_CMDERR(data->buf)) > > + return -EIO; > > Missing mutex unlock here. Oh ;) I'll fix that. > > + > > + *val = MCP4131_RAW(data->buf); > > + mutex_unlock(&data->lock); > > + > > + return IIO_VAL_INT; > > + > > + case IIO_CHAN_INFO_SCALE: > > + *val = 1000 * data->cfg->kohms; > > + *val2 = data->cfg->max_pos; > > + return IIO_VAL_FRACTIONAL; > > + } > > + > > + return -EINVAL; > > +} > > + > > +static int mcp4131_write_raw(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, > > + int val, int val2, long mask) > > +{ > > + int err; > > + struct mcp4131_data *data = iio_priv(indio_dev); > > + int address = chan->channel << MCP4131_WIPER_SHIFT; > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + if (val > data->cfg->max_pos || val < 0) > > + return -EINVAL; > > + break; > > + > > + default: > > + return -EINVAL; > > + } > > + > > + mutex_lock(&data->lock); > > + > > + data->buf[0] = address << MCP4131_WIPER_SHIFT; > > + data->buf[0] |= MCP4131_WRITE | (val >> 8); > > + data->buf[1] = val & 0xFF; /* 8 bits here */ > > + > > + err = spi_write(data->spi, data->buf, 2); > > + if (err) { > > + mutex_unlock(&data->lock); > > + return err; > > + } > > + mutex_unlock(&data->lock); > > + > > + return 0; > > This last part could be written as: > err = spi_write(data->spi, data->buf, 2); > mutex_unlock(&data->lock); > > return err; OK > Other than the things I noted driver looks good to me. > > > regards, > Joachim Eastwood -- Slawomir Stepien -- 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
