Re: [PATCH 02/10] iio: dummy: Use automatic lock and direct mode cleanup.

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 4 Feb 2024 18:29:25 +0200
andy.shevchenko@gmail.com wrote:

> Sun, Jan 28, 2024 at 03:05:29PM +0000, Jonathan Cameron kirjoitti:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Given we now have iio_device_claim_direct_scoped() to perform automatic
> > releasing of direct mode at exit from the scope that follows it, this can
> > be used in conjunction with guard(mutex) etc remove a lot of special case
> > handling.
> > 
> > Note that in this particular example code, there is no real reason you can't
> > read channels via sysfs at the same time as filling the software buffer.
> > To make it look more like a real driver constrain raw and processed
> > channel reads from occurring whilst the buffer is in use.  
> 
> ...
> 
> > +		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> > +			guard(mutex)(&st->lock);
> > +			switch (chan->type) {
> > +			case IIO_VOLTAGE:
> > +				if (chan->output) {
> > +					/* Set integer part to cached value */
> > +					*val = st->dac_val;
> > +					return IIO_VAL_INT;
> > +				} else if (chan->differential) {
> > +					if (chan->channel == 1)
> > +						*val = st->differential_adc_val[0];
> > +					else
> > +						*val = st->differential_adc_val[1];
> > +					return IIO_VAL_INT;
> > +				} else {
> > +					*val = st->single_ended_adc_val;
> > +					return IIO_VAL_INT;
> > +				}  
> 
> Now you may go further and use only single return statement here.

True though this is an example driver and it's only really coincidence those returns
are the same so I'd rather keep it as explicitly matching *val with the return.

> 
> > +			case IIO_ACCEL:
> > +				*val = st->accel_val;
> > +				return IIO_VAL_INT;
> > +			default:
> > +				return -EINVAL;
> >  			}  
> 
> ...
> 
> > +		unreachable();  
> 
> Hmm... Is it really required? Why?

Try compiling without it. (I'm running with C=1 W=1 but think this will show up anyway.

Seems it can't tell we never leave the for loop.

> 
In file included from ./include/linux/preempt.h:11,                                                                                                                                     
                 from ./include/linux/spinlock.h:56,                                                                                                                                    
                 from ./include/linux/mmzone.h:8,                                                                                                                                       
                 from ./include/linux/gfp.h:7,                                                                                                                                          
                 from ./include/linux/slab.h:16,                                                                                                                                        
                 from drivers/iio/dummy/iio_simple_dummy.c:15:                                                                                                                          
drivers/iio/dummy/iio_simple_dummy.c: In function ‘iio_dummy_read_raw’:                         
./include/linux/cleanup.h:173:9: warning: this statement may fall through [-Wimplicit-fallthrough=]
  173 |         for (CLASS(_name, scope)(args), \                                                    
      |         ^~~                                                                             
./include/linux/iio/iio.h:667:9: note: in expansion of macro ‘scoped_cond_guard’                
  667 |         scoped_cond_guard(iio_claim_direct_try, fail, iio_dev)                               
      |         ^~~~~~~~~~~~~~~~~                                                               
drivers/iio/dummy/iio_simple_dummy.c:289:17: note: in expansion of macro ‘iio_device_claim_direct_scoped’
  289 |                 iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {                            
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~                                               
drivers/iio/dummy/iio_simple_dummy.c:316:9: note: here                                                        
  316 |         case IIO_CHAN_INFO_PROCESSED:                                                                 
      |         ^~~~                                      
> ...
> 
> P.S> I hope you are using --histogram diff algo when preparing patches.  
>