Re: [PATCH v2 2/4] ASoC: dt-bindings: realtek,rt5682s: Add AVDD and MICVDD supplies

[Nícolas F. R. A. Prado]

On Tue, Oct 25, 2022 at 12:06:23PM +0200, AngeloGioacchino Del Regno wrote:
> Il 25/10/22 00:00, Nícolas F. R. A. Prado ha scritto:
> > The rt5682s codec can have two supplies: AVDD and MICVDD. They are
> > already used by sc7180-trogdor-kingoftown.dtsi, so document them in the
> > binding.
> > 
> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@xxxxxxxxxxxxx>
> > 
> 
> I also don't like these uppercase supply names... I wonder if it's worth changing
> the driver to get "avdd" *or* "AVDD" (so, if "avdd" fails -> backwards compat)...
> 
> ...this way, we can change the devicetree to use the lowercase names without
> breaking abi.
> 
> Of course, this commit would need to be changed to document only the lowercase
> supply names.
> 
> Driver-wise, we have a rt5682s_supply_names array... we could do something like:
> 
> static const char *rt5682s_supply_names_legacy[RT5682S_NUM_SUPPLIES] = {
> 	[RT5682S_SUPPLY_AVDD] = "AVDD",
> 	[RT5682S_SUPPLY_MICVDD] = "MICVDD",
> };
> 
> static const char *rt5682s_supply_names[RT5682S_NUM_SUPPLIES] = {
> 	[RT5682S_SUPPLY_AVDD] = "avdd",
> 	[RT5682S_SUPPLY_MICVDD] = "micvdd",
> };
> 
> for (...) assign_supply_names;
> ret = devm_regulator_bulk_get(...);
> 
> if (ret) {
> 	for (...) assign_legacy_supply_names;
> 	ret = devm_regulator_bulk_get(...)
> 	if (ret)
> 		return ret;
> }
> 
> What do you think?

Hi,

I took a stab at this, but the resulting code wasn't as elegant. The default
behavior of the regulator core when a regulator is missing is to just print
a warning and give a dummy regulator in its place. A way around this is the
"optional" variant, but it doesn't have a bulk version (in fact seems like it
was added and then removed a few years back, but I haven't dug out the reason).

So the result was a code block that wasn't nearly as neat as your draft above
and it didn't seem worth it to add this complexity just to gain the usage of
lowercase properties, which is why in the end I decided to not include this in
the series I just sent [1]. 

I've included the patch below. If you do think there's a more reasonable
approach or if having the lowercase supplies is worth it, let me know.

Thanks,
Nícolas

[1] https://lore.kernel.org/all/20221028205540.3197304-1-nfraprado@xxxxxxxxxxxxx