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. While you're at it you could also put the includes in alphabetic order. > +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. > + > + *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; Other than the things I noted driver looks good to me. regards, Joachim Eastwood -- 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
