mbox series

[v2,0/4] Add missing dt-binding properties to rt5682(s)

Message ID 20221024220015.1759428-1-nfraprado@collabora.com
Headers show
Series Add missing dt-binding properties to rt5682(s) | expand

Message

Nícolas F. R. A. Prado Oct. 24, 2022, 10 p.m. UTC 
This series adds the missing sound-dai-cells to rt5682s and supplies for
both rt5682s and rt5682.

These properties are already used by sc7180-trogdor.dtsi (and derived
DTs like sc7180-trogdor-kingoftown.dtsi), so the commits in this series
are really just documenting their usage.

v1: https://lore.kernel.org/all/20221021190908.1502026-1-nfraprado@collabora.com/

Changes in v2:
- Changed sound-dai-cells to 1 on rt5682s
- Added commit fixing sound-dai-cells on rt5682
- Added mention to commit messages that properties are already being
  used by DTs

Nícolas F. R. A. Prado (4):
  ASoC: dt-bindings: realtek,rt5682s: Add #sound-dai-cells
  ASoC: dt-bindings: realtek,rt5682s: Add AVDD and MICVDD supplies
  ASoC: dt-bindings: rt5682: Set sound-dai-cells to 1
  ASoC: dt-bindings: rt5682: Add AVDD, MICVDD and VBAT supplies

 .../devicetree/bindings/sound/realtek,rt5682s.yaml        | 7 +++++++
 Documentation/devicetree/bindings/sound/rt5682.txt        | 8 +++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

Comments

AngeloGioacchino Del Regno Oct. 25, 2022, 10:06 a.m. UTC | #1 
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@collabora.com>
> 

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?

Cheers,
Angelo
Krzysztof Kozlowski Oct. 25, 2022, 12:26 p.m. UTC | #2 
On 24/10/2022 18:00, Nícolas F. R. A. Prado wrote:
> 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@collabora.com>
> 
> ---
> 
> Changes in v2:
> - Added mention that property is already used in a DT to the commit
>   message

You already got an ack for it. Don't ignore it.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof
Chen-Yu Tsai Oct. 25, 2022, 7:57 p.m. UTC | #3 
On Tue, Oct 25, 2022 at 3:07 AM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 25/10/22 00:00, Nícolas F. R. A. Prado ha scritto:
> > The rt5682s codec is a DAI provider with two interfaces - AIF1 and AIF2
> > - and therefore should have a #sound-dai-cells property that is equal to
> > 1. Add it.
> >
> > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> >
>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>