| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20251018193128.799490fa@jic23-huawei>
Date: Sat, 18 Oct 2025 19:31:28 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Nuno Sá <noname.nuno@...il.com>
Cc: Herve Codina <herve.codina@...tlin.com>, Wolfram Sang
<wsa+renesas@...g-engineering.com>, 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, 17 Oct 2025 17:29:34 +0100
Nuno Sá <noname.nuno@...il.com> wrote:
> 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@...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
>
> 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á
I'd add a question on this under the --- in the next version of the binding
doc. This is a fairly unusual situation. I think the regulators presence is
sufficient but it may surprise people enough to make it worth calling out.
Jonathan
Powered by blists - more mailing lists