Message-ID: <20251017085904.07e40e37@bootlin.com>
Date: Fri, 17 Oct 2025 08:59:04 +0200
From: Herve Codina <herve.codina@...tlin.com>
To: Nuno Sá <noname.nuno@...il.com>
Cc: Wolfram Sang <wsa+renesas@...g-engineering.com>, Jonathan Cameron	
 <jic23@...nel.org>, David Lechner <dlechner@...libre.com>, Nuno
 Sá	 <nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>,
 Rob Herring	 <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley	 <conor+dt@...nel.org>, Geert Uytterhoeven
 <geert+renesas@...der.be>, Magnus Damm <magnus.damm@...il.com>, Liam
 Girdwood <lgirdwood@...il.com>, Mark Brown	 <broonie@...nel.org>,
 linux-iio@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, Pascal Eberhard
 <pascal.eberhard@...com>, Miquel Raynal <miquel.raynal@...tlin.com>, Thomas
 Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH 2/4] iio: adc: Add support for the Renesas RZ/N1 ADC

Hi Nuno,

On Thu, 16 Oct 2025 16:26:28 +0100
Nuno Sá <noname.nuno@...il.com> wrote:

...

...
> > > > > > +
> > > > > > +	ret = rzn1_adc_core_get_regulators(rzn1_adc, &rzn1_adc-    
> > > > > > > adc_core[0],    
> > > > > > +					   "adc1-avdd", "adc1-vref");
> > > > > > +	if (ret)
> > > > > > +		return ret;
> > > > > > +
> > > > > > +	ret = rzn1_adc_core_get_regulators(rzn1_adc, &rzn1_adc-    
> > > > > > > adc_core[1],    
> > > > > > +					   "adc2-avdd", "adc2-vref");
> > > > > > +	if (ret)
> > > > > > +		return ret;      
> > > > > 
> > > > > Hmm, is avdd really an optional regulator? I mean can the ADC power up
> > > > > at
> > > > > all
> > > > > without a supply in AVDD? Even vref seems to be mandatory as we can't
> > > > > properly
> > > > > scale the sample without it.    
> > > > 
> > > > Where do you see that avdd is an optional regulator?    
> > > 
> > > You are using devm_regulator_get_optional(). That's for optional regulators.
> > >   
> > 
> > Indeed I use devm_regulator_get_optional().
> > 
> > We have two similar function to get regulators:
> > - devm_regulator_get() and
> > - devm_regulator_get_optional().
> > 
> > devm_regulator_get() returns a dummy regulator if the regulator is not
> > described in the device-tree. The calling code has no way to known if
> > the regulator was present or not.  
> 
> Yeah because it's mandatory and the part cannot work without power :). So we
> should not be allowed to operate without a regulator.
> 
> > 
> > On the other hand, devm_regulator_get_optional() returns -ENODEV when the
> > regulator is not described.
> > 
> > That's pretty confusing but it is the reality.
> > 
> > I use devm_regulator_get_optional() but check for -ENODEV to see if the
> > regulator is provided or not.
> > 
> > In order to use the ADC core (is_used flag), I need both the AVDD and the
> > VREF regulator available.  
> 
> And that is why I don't get why are we allowed to proceed if there's no
> regulators? That seems wrong to me. 
> 
> So I think the regulators should be mandatory in the bindings and a dummy
> regulator should also not be allowed in this case because that should get you 
> -EINVAL when calling regulator_get_voltage().
> 

I have 4 regulators: avdd1, vref1, avvd2, vref2.

The ADC controller can work with 2 internal ADC core (adc_core[0] and adc_core[1])
in the driver. Those internal core are not directly accessed by the driver. Only
the ADC controller is accesses.

Those cores have an AVDD and a VREF power supply.

We can use either adc_core[0] only, adc_core[1] only or both adc cores.

Depending on regulator described, the driver uses one or two adc cores.

This choice is done by:
--- 8< ---
static int rzn1_adc_set_iio_dev_channels(struct rzn1_adc *rzn1_adc,
					 struct iio_dev *indio_dev)
{
	int adc_used;

	adc_used = rzn1_adc->adc_core[0].is_used ? 0x01 : 0x00;
	adc_used |= rzn1_adc->adc_core[1].is_used ? 0x02 : 0x00;

	switch (adc_used) {
	case 0x01:
		indio_dev->channels = rzn1_adc1_channels;
		indio_dev->num_channels = ARRAY_SIZE(rzn1_adc1_channels);
		return 0;
	case 0x02:
		indio_dev->channels = rzn1_adc2_channels;
		indio_dev->num_channels = ARRAY_SIZE(rzn1_adc2_channels);
		return 0;
	case 0x03:
		indio_dev->channels = rzn1_adc1_adc2_channels;
		indio_dev->num_channels = ARRAY_SIZE(rzn1_adc1_adc2_channels);
		return 0;
	default:
		break;
	}
--- 8< ---

In rzn1_adc_core_get_regulators(), looking at one core I do the
following:
 - Try to get AVDD (using get_optional)
 - Try to get VREF (using get_optional)
 - Core is used only if both regulators are present.

For one core to be used, both regulators have to be present.

Regulators are mandatory but adc core usage is optional.

This optional usage depends on related regulator presence.


> > 
> > Hum, do you think it's worth a try?  
> 
> Not sure. But it got me thinking about all this handling in the pm runtime
> routines. So if in the resume() call you fail at some point and then disable
> stuff in your return path and then we get an unbind won't things (clocks and
> regulators) be unbalanced leading to splats? In fact by just looking at the
> unbind path [1] I can see:
> 
> 1. We call pm_runtime_get_sync(dev) which can fail;
> 2. Later on we call pm_runtime_put_sync(dev).
> 
> Not really sure if there's special handling in the pm core to be aware that
> resuming failed (the refcount seems to be incremented [2] before resuming so...)
> 
> Maybe I would keep it simple and get and enable clocks/regulators during probe
> and only care of rzn1_adc_power() in the runtime routines. My 2 cents.
> 
> [1]: https://elixir.bootlin.com/linux/v6.17.1/source/drivers/base/dd.c#L1249
> [2]: https://elixir.bootlin.com/linux/v6.17.1/source/drivers/base/power/runtime.c#L1189
> 

I see, thanks for those clarifications.
I will simplify and, as you proposed, I will only take care of rzn1_adc_power().

Best regards,
Hervé

Powered by blists - more mailing lists