Re: [PATCH v2] iio: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X

[Slawomir Stepien]

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