On Mar 19, 2016 14:48, Peter Meerwald-Stadler wrote:
> > +#define MCP4131_WIPER_SHIFT (4)
>
> () not needed
OK
> > +struct mcp4131_data {
> > + struct spi_device *spi;
> > + unsigned long devid;
> > + struct mutex lock;
> > + u8 tx[2], rx[2];
>
> alignment requirements for SPI transfer?
Do you mean the ____cacheline_aligned attribute?
I did not add it because I'm not quite sure why it's needed there. Will have to
find it out...
Could you point me some materials where it's explained?
I think I can drop two separated buffers in favor of one buffer (e.g. buf[2]). I
saw drivers doing that. Do you think that's a good idea?
> > + data->rx[0] = 0;
> > + data->rx[1] = 0;
>
> initialization needed?
No. You're right.
> setup of data->xfer + data->tx is done outside the lock, this seems wrong
True. Will fix it in v3.
> > + dev_info(&spi->dev, "Registered %s\n", indio_dev->name);
>
> I'd rather drop this message
OK. Will leave only the dev_info for errors.
> > +static int mcp4131_remove(struct spi_device *spi)
> > +{
> > + struct iio_dev *indio_dev = spi_get_drvdata(spi);
> > + struct mcp4131_data *data = iio_priv(indio_dev);
> > +
> > + mutex_destroy(&data->lock);
>
> no need to call
>
> > + devm_iio_device_unregister(&spi->dev, indio_dev);
>
> don't call this explicitly, it is done automatically after _remove
That's why it's called managed (devm_*)?
> > +
> > + dev_info(&spi->dev, "Unregistered %s\n", indio_dev->name);
>
> don't
>
> I think the entire _remove can be removed
OK.
Thank you for the review. I'm learning a lot!
--
Slawomir Stepien
