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 <[email protected]>
---
Changes in v2:
- Added mention that property is already used in a DT to the commit
message
Documentation/devicetree/bindings/sound/realtek,rt5682s.yaml | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/sound/realtek,rt5682s.yaml b/Documentation/devicetree/bindings/sound/realtek,rt5682s.yaml
index 1c0b06d82369..ac1dea5b4450 100644
--- a/Documentation/devicetree/bindings/sound/realtek,rt5682s.yaml
+++ b/Documentation/devicetree/bindings/sound/realtek,rt5682s.yaml
@@ -90,6 +90,10 @@ properties:
"#sound-dai-cells":
const: 1
+ AVDD-supply: true
+
+ MICVDD-supply: true
+
additionalProperties: false
required:
--
2.38.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 <[email protected]>
>
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
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 <[email protected]>
>
> ---
>
> 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 <[email protected]>
Best regards,
Krzysztof
On Mon, Oct 24, 2022 at 3:01 PM Nícolas F. R. A. Prado
<[email protected]> wrote:
>
> The rt5682s codec can have two supplies: AVDD and MICVDD. They are
The actual chip also has LDO1_IN (for digital core and charge pump)
and DBVDD (for I/O). However in the Chromebook designs these two
and AVDD are all provided from the same power rail, through separate
filter banks.
Neither does the datasheet specify the ordering of AVDD, DBVDD, and
LDO1_IN for power sequencing, just that three should be toggled together.
Should we model these? Or wait until some design actually splits these?
ChenYu
> already used by sc7180-trogdor-kingoftown.dtsi, so document them in the
> binding.
>
> Signed-off-by: Nícolas F. R. A. Prado <[email protected]>
>
> ---
>
> Changes in v2:
> - Added mention that property is already used in a DT to the commit
> message
>
> Documentation/devicetree/bindings/sound/realtek,rt5682s.yaml | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/sound/realtek,rt5682s.yaml b/Documentation/devicetree/bindings/sound/realtek,rt5682s.yaml
> index 1c0b06d82369..ac1dea5b4450 100644
> --- a/Documentation/devicetree/bindings/sound/realtek,rt5682s.yaml
> +++ b/Documentation/devicetree/bindings/sound/realtek,rt5682s.yaml
> @@ -90,6 +90,10 @@ properties:
> "#sound-dai-cells":
> const: 1
>
> + AVDD-supply: true
> +
> + MICVDD-supply: true
> +
> additionalProperties: false
>
> required:
> --
> 2.38.1
>
On Tue, Oct 25, 2022 at 01:12:49PM -0700, Chen-Yu Tsai wrote:
> On Mon, Oct 24, 2022 at 3:01 PM N?colas F. R. A. Prado
> > The rt5682s codec can have two supplies: AVDD and MICVDD. They are
> Neither does the datasheet specify the ordering of AVDD, DBVDD, and
> LDO1_IN for power sequencing, just that three should be toggled together.
> Should we model these? Or wait until some design actually splits these?
Yes, the driver for a chip should be a driver for the chip not for some
specific board.
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 <[email protected]>
> >
>
> 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?
No one seems opposed to it, so I'll add that to the next version.
Thanks,
N?colas
On Tue, Oct 25, 2022 at 01:12:49PM -0700, Chen-Yu Tsai wrote:
> On Mon, Oct 24, 2022 at 3:01 PM N?colas F. R. A. Prado
> <[email protected]> wrote:
> >
> > The rt5682s codec can have two supplies: AVDD and MICVDD. They are
>
> The actual chip also has LDO1_IN (for digital core and charge pump)
> and DBVDD (for I/O). However in the Chromebook designs these two
> and AVDD are all provided from the same power rail, through separate
> filter banks.
What about rt5682 (no s), does that chip also have these same supplies?
Also, since you already gave the purpose of these other supplies, could you also
tell the purpose of AVDD, MICVDD and (for rt5682) VBAT? That way I could add
some description for them in the binding.
Thanks,
N?colas
>
> Neither does the datasheet specify the ordering of AVDD, DBVDD, and
> LDO1_IN for power sequencing, just that three should be toggled together.
>
> Should we model these? Or wait until some design actually splits these?
[..]
On Thu, Oct 27, 2022 at 10:36:27AM -0400, N?colas F. R. A. Prado wrote:
> Also, since you already gave the purpose of these other supplies, could you also
> tell the purpose of AVDD, MICVDD and (for rt5682) VBAT? That way I could add
> some description for them in the binding.
Those are all very conventional names - analog, microphone and battery
supplies.
On Thu, Oct 27, 2022 at 7:36 AM Nícolas F. R. A. Prado
<[email protected]> wrote:
>
> On Tue, Oct 25, 2022 at 01:12:49PM -0700, Chen-Yu Tsai wrote:
> > On Mon, Oct 24, 2022 at 3:01 PM Nícolas F. R. A. Prado
> > <[email protected]> wrote:
> > >
> > > The rt5682s codec can have two supplies: AVDD and MICVDD. They are
> >
> > The actual chip also has LDO1_IN (for digital core and charge pump)
> > and DBVDD (for I/O). However in the Chromebook designs these two
> > and AVDD are all provided from the same power rail, through separate
> > filter banks.
>
> What about rt5682 (no s), does that chip also have these same supplies?
>
> Also, since you already gave the purpose of these other supplies, could you also
> tell the purpose of AVDD, MICVDD and (for rt5682) VBAT? That way I could add
> some description for them in the binding.
As Mark mentioned in his reply, these are quite standard names.
AVDD is for the analog bits. MICVDD is for the microphone bias.
VBAT is called battery power in the datasheet. The block diagram
shows it going through an internal controllable LDO whose output
then powers MICVDD. This could be used in designs that don't
include a suitable external supply for MICVDD. If MICVDD is provided,
then one would turn the internal LDO off.
So either VBAT or MICVDD has to be provided.
ChenYu
> Thanks,
> Nícolas
>
> >
> > Neither does the datasheet specify the ordering of AVDD, DBVDD, and
> > LDO1_IN for power sequencing, just that three should be toggled together.
> >
> > Should we model these? Or wait until some design actually splits these?
> [..]
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 <[email protected]>
> >
>
> 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/[email protected]
From 8de4a86f10ba2e13458afe63fe658df685b21b45 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?N=C3=ADcolas=20F=2E=20R=2E=20A=2E=20Prado?=
<[email protected]>
Date: Thu, 27 Oct 2022 16:10:22 -0400
Subject: [PATCH] ASoC: rt5682s: Handle lowercase supply name and fallback if
needed
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Supply names provided by devicetree are conventionally lowercase. In
order to be able to use lowercase names without breaking existing
setups, detect if any of the older names are used and if so fallback to
them.
Signed-off-by: N?colas F. R. A. Prado <[email protected]>
---
sound/soc/codecs/rt5682s.c | 40 ++++++++++++++++++++++++++++++--------
1 file changed, 32 insertions(+), 8 deletions(-)
diff --git a/sound/soc/codecs/rt5682s.c b/sound/soc/codecs/rt5682s.c
index 466a37f3500c..3cefa016be77 100644
--- a/sound/soc/codecs/rt5682s.c
+++ b/sound/soc/codecs/rt5682s.c
@@ -41,11 +41,16 @@ static const struct rt5682s_platform_data i2s_default_platform_data = {
.dai_clk_names[RT5682S_DAI_BCLK_IDX] = "rt5682-dai-bclk",
};
-static const char *rt5682s_supply_names[RT5682S_NUM_SUPPLIES] = {
+static const char *rt5682s_legacy_supply_names[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",
+};
+
static const struct reg_sequence patch_list[] = {
{RT5682S_I2C_CTRL, 0x0007},
{RT5682S_DIG_IN_CTRL_1, 0x0000},
@@ -3090,7 +3095,9 @@ static int rt5682s_i2c_probe(struct i2c_client *i2c)
struct rt5682s_platform_data *pdata = dev_get_platdata(&i2c->dev);
struct rt5682s_priv *rt5682s;
int i, ret;
+ struct regulator *reg;
unsigned int val;
+ bool using_legacy_supply_names = false;
rt5682s = devm_kzalloc(&i2c->dev, sizeof(struct rt5682s_priv), GFP_KERNEL);
if (!rt5682s)
@@ -3112,14 +3119,31 @@ static int rt5682s_i2c_probe(struct i2c_client *i2c)
return ret;
}
- for (i = 0; i < ARRAY_SIZE(rt5682s->supplies); i++)
- rt5682s->supplies[i].supply = rt5682s_supply_names[i];
+ for (i = 0; i < ARRAY_SIZE(rt5682s_supply_names); i++) {
+ reg = devm_regulator_get_optional(&i2c->dev, rt5682s_supply_names[i]);
+ if (IS_ERR(reg)) {
+ if (PTR_ERR(reg) == -ENODEV) {
+ using_legacy_supply_names = true;
+ break;
+ } else {
+ dev_err(&i2c->dev, "Failed to request supplies: %d\n", ret);
+ return PTR_ERR(reg);
+ }
+ }
- ret = devm_regulator_bulk_get(&i2c->dev,
- ARRAY_SIZE(rt5682s->supplies), rt5682s->supplies);
- if (ret) {
- dev_err(&i2c->dev, "Failed to request supplies: %d\n", ret);
- return ret;
+ rt5682s->supplies[i].consumer = reg;
+ }
+
+ if (using_legacy_supply_names) {
+ for (i = 0; i < ARRAY_SIZE(rt5682s->supplies); i++)
+ rt5682s->supplies[i].supply = rt5682s_legacy_supply_names[i];
+
+ ret = devm_regulator_bulk_get(&i2c->dev, ARRAY_SIZE(rt5682s->supplies),
+ rt5682s->supplies);
+ if (ret) {
+ dev_err(&i2c->dev, "Failed to request supplies: %d\n", ret);
+ return ret;
+ }
}
ret = devm_add_action_or_reset(&i2c->dev, rt5682s_i2c_disable_regulators, rt5682s);
--
2.38.1