Received-SPF: pass (google.com: domain of linux-kernel+bounces-103432-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Thu, 14 Mar 2024 14:46:47 +0000
From: Jonathan Cameron <jic23@kernel.org>
To: vamoirid <vassilisamir@gmail.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>, lars@metafoo.de,
 ang.iglesiasg@gmail.com, mazziesaccount@gmail.com, ak@it-klinger.de,
 petre.rodan@subdimension.ro, linus.walleij@linaro.org,
 phil@raspberrypi.com, 579lpy@gmail.com, linux-iio@vger.kernel.org,
 linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 3/6] iio: pressure: add SCALE and RAW values for
 channels
Message-ID: <20240314144647.291a1100@jic23-huawei>
In-Reply-To: <ZfLYGL/vXMHUGghz@vamoirid-EDL-PC>
References: <20240313174007.1934983-1-vassilisamir@gmail.com>
	<20240313174007.1934983-4-vassilisamir@gmail.com>
	<ZfH4bET-HX0e3PO_@smile.fi.intel.com>
	<20240313195110.GB1938985@vamoiridPC>
	<ZfIGtUPZpyDBnWwz@smile.fi.intel.com>
	<20240313212812.GE1938985@vamoiridPC>
	<ZfLYGL/vXMHUGghz@vamoirid-EDL-PC>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Thu, 14 Mar 2024 11:57:28 +0100
vamoirid <vassilisamir@gmail.com> 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


> 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
> > > 
> > >