Message-ID: <10e119ee5a76f1c47d7eb6a15989c8ffc00ffc5f.camel@gmail.com>
Date: Fri, 17 Oct 2025 09:26:26 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: Herve Codina <herve.codina@...tlin.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

On Fri, 2025-10-17 at 08:59 +0200, Herve Codina wrote:
> 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.
> 

Ok, then we could flip the logic and have boolean properties for the adc core
usage and depending on that, requesting the regulators. To me, the intent would
be more clear (at the expense of more FW properties).

Having said that, the above helps a lot in understanding what's going on and
explains the get_optional() usage. I'm not still 100% convinced but bah, fine :)

I would still argue that you should have a comment (likely in get_regulators())
explaining the logic and the optional usage.

Given the above I think you could also remove:

if (!adc_core->vref)
	return -ENODEV;

from rzn1_adc_core_get_vref_mv() since the channels are only exposed in case the
regulators are present.


- Nuno Sá

Powered by blists - more mailing lists