Re: [PATCH 3/4] staging: iio: accel: Use sign_extend32 and adjust a switch statement

[Jonathan Cameron]

On Mon, 12 Feb 2018 16:10:01 +0300
Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:

> On Mon, Feb 12, 2018 at 05:24:58PM +0530, Himanshu Jha wrote:
> > Use sign_extend32 function instead of manually coding it. Also, adjust a  
>                                                             ^^^^^
> > switch block to explicitly match channels and return -EINVAL as default
> > case which improves code readability.  
> 
> Greg likes to say something along the lines of "when you start your
> sentence with "Also, " that could be a clue that it should be a separate
> patch.".
> 
> > 
> > Signed-off-by: Himanshu Jha <himanshujha199640@xxxxxxxxx>
> > ---
> >  drivers/staging/iio/accel/adis16201.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/accel/adis16201.c b/drivers/staging/iio/accel/adis16201.c
> > index 011d2c5..6800347 100644
> > --- a/drivers/staging/iio/accel/adis16201.c
> > +++ b/drivers/staging/iio/accel/adis16201.c
> > @@ -112,12 +112,17 @@ static int adis16201_read_raw(struct iio_dev *indio_dev,
> >  	case IIO_CHAN_INFO_SCALE:
> >  		switch (chan->type) {
> >  		case IIO_VOLTAGE:
> > -			if (chan->channel == 0) {
> > +			switch (chan->channel) {
> > +			case 0:
> >  				*val = 1;
> >  				*val2 = 220000;
> > -			} else {
> > +				break;
> > +			case 1:
> >  				*val = 0;
> >  				*val2 = 610000;
> > +				break;
> > +			default:
> > +				return -EINVAL;
> >  			}  
> 
> I don't think this improves readability.  The -EINVAL is to handle a
> driver bug which we haven't introduced yet.  Probably we would be better
> off printing a warning or something?  But it feels weird to introduce so
> much code to handle a bug which would actually be pretty difficult to
> write.  The original code is fine.

Hmm. My thought here was that it is not obvious from the code
that we only have channel 0 and channel 1.  The if statement
kind of implies that channel 0 is special compared to 'all the other'
elements where as what we are actually doing is picking from
a set of options. So semantically it's a switch statement being
implemented as an if else pair ;)

Perhaps we can compromise on the addition of a comment on the else
case to say it only applies to channel 1?

Dan, what do you think?

It isn't particularly important either way though so feel free to
just drop this one.

> 
> >  			return IIO_VAL_INT_PLUS_MICRO;
> >  		case IIO_TEMP:
> > @@ -155,9 +160,7 @@ static int adis16201_read_raw(struct iio_dev *indio_dev,
> >  		if (ret)
> >  			return ret;
> >  
> > -		val16 &= (1 << bits) - 1;
> > -		val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
> > -		*val = val16;
> > +		*val = sign_extend32(val16, bits - 1);  
> 
> Yeah.  This is a nice clean up.
> 
> regards,
> dan carpenter
>