Re: [PATCH v3 3/4] iio: adc: Add Maxim max9611 ADC driver

[jacopo <jacopo@jmondi.org>]

Hi Jonathan, Peter,

On Sat, Mar 25, 2017 at 05:13:38PM +0100, Peter Meerwald-Stadler wrote:
> 
> On Sat, 25 Mar 2017, Jonathan Cameron wrote:
> > > +	switch (mask) {
> > > +	case IIO_CHAN_INFO_RAW:
> > > +		mutex_lock(&dev->lock);
> > > +
> > > +		switch (chan->address) {
> > > +		case MAX9611_CHAN_TEMPERATURE:
> > > +			ret = max9611_read_single(dev, CONF_TEMP,
> > > +						  &adc_data);
> > > +			if (ret)
> > I'm not personally keen on jumping out of deep indentations like this
> > just save on repeating one line.  I'd pull the unlock back here and return
> > directly as I feel it'll improve readability.
> > Actually come to think of it, why does the lock need to be held for
> > the next line anyway?  adc_data is on the stack so doesn't matter if we
> > have concurrent readers, once the i2c transaction is finished.
> > Just unlock before checking ret.

[snip]

> > > +
> > > +			mutex_unlock(&dev->lock);
> > > +			return IIO_VAL_INT;
> > > +		}
> > > +
> 
> this falls through, no break?
> 
> I'd move
>                 mutex_unlock(&dev->lock);
>                 return IIO_VAL_INT;
> here
> 

On the two comments above: re-thinking on this a bit, the only part
that actually needs protection is gain selection routine, which
depends on consecutive reads of what is returned from the ADC bus.

All other variables (including the ones coming from outside, as val
and val2 are) are not shared and do no need protection at this level.

I should move the mutex in max9611_read_* functions (maybe even
read_single() does not need that, as I2c bus access is protected
already), and simplify the read_raw() one.
Just saying it out loud so there will be no surprises in v4.

Thanks
   j