Re: [PATCH 2/4] iio: adc: Add support for the Renesas RZ/N1 ADC

["Nuno Sá" <noname.nuno@gmail.com>]

On Fri, 2025-10-17 at 17:43 +0200, Herve Codina wrote:
> I Nuno,
> 
> On Fri, 17 Oct 2025 09:26:26 +0100
> Nuno Sá <noname.nuno@gmail.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@gmail.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

I'll leave it up to you but while I know it introduces new properties, you could
still do it in a way that minimizes errors:

 - In the bindings, if the property is set, then the regulators are a
__required__;
 - In the driver, if the boolean is true, then use devm_regulator_get()

- Nuno Sá