Re: [PATCH v3 3/6] iio: pressure: add SCALE and RAW values for channels

[Vasileios Amoiridis]

On Thu, Mar 14, 2024 at 02:46:47PM +0000, Jonathan Cameron wrote:
> On Thu, 14 Mar 2024 11:57:28 +0100
> vamoirid <vassilisamir@xxxxxxxxx> wrote:
> 
> > On Wed, Mar 13, 2024 at 10:28:12PM +0100, Vasileios Amoiridis wrote:
> > > On Wed, Mar 13, 2024 at 10:04:05PM +0200, Andy Shevchenko wrote:  
> > > > On Wed, Mar 13, 2024 at 08:51:10PM +0100, Vasileios Amoiridis wrote:  
> > > > > On Wed, Mar 13, 2024 at 09:03:08PM +0200, Andy Shevchenko wrote:  
> > > > > > On Wed, Mar 13, 2024 at 06:40:04PM +0100, Vasileios Amoiridis wrote:  
> > > > > > > Add extra IIO_CHAN_INFO_SCALE and IIO_CHAN_INFO_RAW in order to be
> > > > > > > able to calculate the processed value with standard userspace IIO
> > > > > > > tools. Can be used for triggered buffers as well.  
> > > > 
> > > > ...
> > > >   
> > > > > > > +	case IIO_CHAN_INFO_RAW:
> > > > > > > +		switch (chan->type) {
> > > > > > > +		case IIO_HUMIDITYRELATIVE:
> > > > > > > +			*val = data->chip_info->read_humid(data);
> > > > > > > +			ret = IIO_VAL_INT;
> > > > > > > +			break;
> > > > > > > +		case IIO_PRESSURE:
> > > > > > > +			*val = data->chip_info->read_press(data);
> > > > > > > +			ret = IIO_VAL_INT;
> > > > > > > +			break;
> > > > > > > +		case IIO_TEMP:
> > > > > > > +			*val = data->chip_info->read_temp(data);
> > > > > > > +			ret = IIO_VAL_INT;
> > > > > > > +			break;
> > > > > > > +		default:
> > > > > > > +			ret = -EINVAL;
> > > > > > > +			break;  
> > > > > > 
> > > > > > Is it mutex that prevents us from returning here?
> > > > > > If so, perhaps switching to use cleanup.h first?  
> > > > > 
> > > > > I haven't seen cleanup.h used in any file and now that I searched,
> > > > > only 5-6 are including it.  
> > > > 
> > > > Hmm... Which repository you are checking with?
> > > > 
> > > > $ git grep -lw cleanup.h -- drivers/ | wc -l
> > > > 47
> > > > 
> > > > (Today's Linux Next)
> > > >  
> > > 
> > > I am checking the drivers/iio of 6.8 (on sunday) and I can only find 7
> > > drivers that use it.
> 
> Yes - but that's because it's new - most of the stuff in 6.8 was the proof
> points for the patches originally introducing support for autocleanup (so typically
> one or two cases for each type of handling) That doesn't mean we don't want it
> in drivers that are being worked upon if it gives a significant advantage.
> Some features we need will merge shortly, and a great deal more usage
> of this autocleanup will occur.
> 
> > >   
> > > > > I am currently thinking if the mutex
> > > > > that already exists is really needed since most of the drivers
> > > > > don't have it + I feel like this is something that should be done
> > > > > by IIO, thus maybe it's not even needed here.  
> > > >  
> > 
> > After some researching today, I realized that all the                           
> > {read/write}_{raw/avail}_{multi/}() functions are in drivers/iio/inkern.c
> > for channel mapping in the kernel and it looks like they are guarded by
> > the mutex_{un}lock(&iio_dev_opaque->info_exist_lock).
> 
> Why is that relevant to this patch which isn't using that interface at all?
> Those protections are to ensure that a consumer driver doesn't access a removed
> IIO device, not accesses directly from userspace.
> 
> >so I feel that the
> > mutexes in the aforementioned functions can be dropped. When you have the
> > time please have a look, maybe the could be dropped.
> 
> Identify what your locks are protecting.  Those existence locks have
> very specific purpose and should not be relied on for anything else.
> 
> If this driver is protecting state known only to itself, then it must
> be responsible for appropriate locking.
> 
> > 
> > In general, there is quite some cleaning that can be done in this driver
> > but is it wise to include it in the triggered buffer support series??? 
> 
> Generally if working on a driver and you see cleanup that you think should
> be done, it belongs before any series adding new features, precisely because
> that code can typically end up simpler as a result.  This sounds like one
> of those cases.  Normally that only includes things that are directly related
> to resulting code for new features (or applying the same cleanup across a driver)
> as we don't want to make people do a full scrub of a driver before adding
> anything as it will just create too much noise.
> 
> So for this case, it does look like a quick use of guard(mutex) in
> a precursor patch will simplify what you add here - hence that's a reasonable
> request for Andy to make.
> 
> Jonathan
> 

Hi Jonathan.

Thank you very much for the feedback once again. I didn't know that cleanup.h
was a new thing. I also didn't understand it when Andy mentioned it. Now that
I saw it better and I read about it, it certainly looks like a very good thing
to add.

I don't know if I sounded like I didn't like that request, but just to clarify,
I see it as a very good thing all the proposals that you do because first I
get to learn and understand how to write better code and second the users will
use a better driver! So please, the more requests, the better.

So a precursor patch adding the new functionality of the guard(mutex) in this
and possibly other places in the driver will be good indeed, thank you!

Best regards,
Vasilis
> 
> > I
> > have noticed quite some things that could be improved but I am hesitating
> > to do it now in order to not "pollute" this series with many cleanups and
> > leave it for another cleanup series for example.
> > 
> > Best regards,
> > Vasilis Amoiridis
> > 
> > > > > > > +		}
> > > > > > > +		break;  
> > > > 
> > > > -- 
> > > > With Best Regards,
> > > > Andy Shevchenko
> > > > 
> > > >   
>