Re: [PATCH] staging: iio: ad7192: implement IIO_CHAN_INFO_SAMP_FREQ

[Eva Rachel Retuya <eraretuya@gmail.com>]

On Fri, Sep 30, 2016 at 02:45:31PM +0200, Lars-Peter Clausen wrote:
> On 09/30/2016 12:28 PM, Eva Rachel Retuya wrote:
> > Use IIO_CHAN_INFO_SAMP_FREQ to implement the sampling_frequency attribute
> > instead of using IIO_DEV_ATTR_SAMP_FREQ() macro.
> > 
> > Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
> 
> Hi,
> 
> Thanks for the patch. There is one tiny bug in the patch, see in the inline
> review.
> 
> The commit message can also be slightly improved. A commit message should
> typically contain why, what and how. For trivial cases some of these might
> be omitted but it is always a good idea to include them regardless. In this
> case the commit message only includes the what. The how is trivial, so that
> is OK. But the why is missing and not necessarily obvious to everybody, e.g.
> consider somebody reading the log 5 years from now.
> 
> For the why part the commit message could contain that this driver
> originally predates the availability of the IIO_CHAN_INFO_SAMP_FREQ
> attribute which is why it was not using it and using it has some advantages
> like it can be access by in-kernel consumers. And it also reduces the code size.
> 

Thank you for the comment. I sent the v2 with all your concerns
addressed.

Eva

> [...]
> > @@ -548,7 +499,7 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,
> >  			    long mask)
> >  {
> >  	struct ad7192_state *st = iio_priv(indio_dev);
> > -	int ret, i;
> > +	int ret, i, div;
> >  	unsigned int tmp;
> >  
> >  	ret = iio_device_claim_direct_mode(indio_dev);
> > @@ -572,10 +523,25 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,
> >  				break;
> >  			}
> >  		break;
> > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > +		if (!val)
> > +			return -EINVAL;
> 
> The direct return here bypasses the iio_device_release_direct_mode() and
> will keep the direct mode locked. This sould instead be
> 
> if (!val) {
> 	ret = -EINVAL;
> 	break;
> }
> 
> > +
> > +		div = st->mclk / (val * st->f_order * 1024);
> > +		if (div < 1 || div > 1023) {
> > +			ret = -EINVAL;
> > +			goto out;
> 
> Since this is in a switch statement and we want to jump to the end of the
> switch I'd just use break, instead of introducing a new label.
> 
> > +		}
> > +
> > +		st->mode &= ~AD7192_MODE_RATE(-1);
> > +		st->mode |= AD7192_MODE_RATE(div);
> > +		ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode);
> > +		break;
> >  	default:
> >  		ret = -EINVAL;
> >  	}
> >  
> > +out:
> >  	iio_device_release_direct_mode(indio_dev);
> >  
> >  	return ret;