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

[Slawomir Stepien]

On Mar 20, 2016 10:25, Jonathan Cameron wrote:
> >> +struct mcp4131_data {
> >> +	struct spi_device *spi;
> 
> This is only used to lookup elements of your cfg array, I'd just have
> a pointer to the relevant element of that array in here instead.
> 
> struct mcp4131_cfg *cfg;
> 
> and in probe do
> data->cfg = &mcp4131_cfg[id];

Great idea. I'll use it in v3.

> >> +	unsigned long devid;
> >> +	struct mutex lock;
> >> +	u8 tx[2], rx[2];
> > 
> > alignment requirements for SPI transfer?
> By which he means put them at the end of this structure and
> mark the with __cacheline_aligned.  It's not technically about alignment
> but rather about ensuring nothing else is in the cacheline which will on some
> spi devices be scrubbed when a transaction occurs.

Thank you for this explanation. I'll move it at the and mark it with the
attribute.

> >> +	data->rx[0] = 0;
> >> +	data->rx[1] = 0;
> > 
> > initialization needed?
> > 
> > setup of data->xfer + data->tx is done outside the lock, this seems wrong
> agreed.

I'll lock the mutex just before switching the mask in _read_raw and in
write_raw like this:

mutex_lock(&data->lock);

switch(mask) {
case IIO_CHAN_INFO_RAW:
(...)
}

mutex_unlock(&data->lock);

> Now I'd change the way you are doing this slightly so that you have
> data->cfg pointing to mcp4131[data->devid].  Moves the 'what part am I?'
> question to a single place in the probe function giving slightly cleaner code.
> >> +		*val = 1000 * mcp4131_cfg[data->devid].kohms;
> >> +		*val2 = mcp4131_cfg[data->devid].max_pos;
> >> +		return IIO_VAL_FRACTIONAL;

Something like this:

		*val = 1000 * data->cfg->kohms;
		*val2 = data->cfg->max_pos;
		mutex_unlock(&data->lock);
		return IIO_VAL_FRACTIONAL;
?

> >> +	dev_info(&spi->dev, "Registered %s\n", indio_dev->name);
> > 
> > I'd rather drop this message
> Agreed, adds noise and it's easy to check if the register succeeded anyway
> by just looking to see if the device is there in sysfs.

OK

> >> +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
> Hmm. This is an oddity, the mutex_destroy exists to aid in debugging locking
> issues by explicity marking the mutex as do not use - iff the mutex
> debugging is enabled.  In this case the storage is promptly deleted anyway
> so any attempt to use the mutex would result in a null pointer dereference
> anyway.  Hence probably not worth having it here.

OK.

Thank your for all the explanations. This helps a lot.

-- 
Slawomir Stepien