On Mar 20, 2016 17:12, Joachim Eastwood wrote:
> Hi Slawomir,
>
> On 20 March 2016 at 15:30, Slawomir Stepien <s...@poczta.fm> wrote:
> > The following functionalities are supported:
> > - write, read from volatile memory
>
> I think it would be useful if you could put 'potentiometer' either in
> the subject and/or commit text so it is more obvious what this driver
> is for.
Ok
> > + spi_message_init(&data->msg);
> > + spi_message_add_tail(&data->xfer, &data->msg);
> > +
> > + err = spi_sync(spi, &data->msg);
> > + if (err) {
> > + dev_err(&spi->dev, "spi_sync(): %d\n", err);
> > + return err;
> > + }
>
> Isn't this init, add, sync sequence basically open coding of what
> spi_write/spi_read does?
> If you could use those you could also get rid transfer/message structs
> in priv data.
> Also it these any reason why the data buffer can just be a local
> variable in mcp4131_read_raw/mcp4131_write_raw?
> If it could be I think it should be possible to move the lock into the
> mcp4131_exec function.
Ok I'll try that.
> > + case IIO_CHAN_INFO_SCALE:
> > + *val = 1000 * data->cfg->kohms;
> > + *val2 = data->cfg->max_pos;
> > + mutex_unlock(&data->lock);
>
> Is locking really necessary for IIO_CHAN_INFO_SCALE?
> Isn't all data->cfg stuff constant?
Yes you're right here. Didn't see it like that.
> > +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;
> > +
> > + mutex_lock(&data->lock);
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + if (val > data->cfg->max_pos || val < 0) {
> > + mutex_unlock(&data->lock);
> > + return -EINVAL;
> > + }
> > + break;
> > + default:
> > + mutex_unlock(&data->lock);
> > + return -EINVAL;
> > + }
> > +
> > + err = mcp4131_exec(data, address, MCP4131_WRITE, val);
> > + mutex_unlock(&data->lock);
>
> While this is not a huge function it is usually good practice to keep
> the locking scope as small as possible.
>
> So wouldn't this be sufficient here.
> mutex_lock(&data->lock);
> err = mcp4131_exec(data, address, MCP4131_WRITE, val);
> mutex_unlock(&data->lock);
>
> Of course if you are able move the lock into mcp4131_exec this will go away.
I'll see if it's possible to move the whole locking into this function.
Thank you for comments.
> regards,
> Joachim Eastwood
--
Slawomir Stepien
