Message-ID: <20251017174322.07997789@bootlin.com>
Date: Fri, 17 Oct 2025 17:43:22 +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

I Nuno,

On Fri, 17 Oct 2025 09:26:26 +0100
Nuno Sá <noname.nuno@...il.com> wrote:

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

This introduces a new property and duplicates the information:
- flag to tell if adc core is used
- regulators described only if used

And so, the possible flag set to "adc core used" but regulators not
described. This is error prone.


I have chosen to rely only on regulators description to avoid the
information redundancy.
  - regulators described -> adc core used
  - regulators not described -> adc core not used

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

Yes indeed, I will remove the adc_core->vref check.

Best regards,
Hervé

Powered by blists - more mailing lists