| Message ID | 20221024220015.1759428-3-nfraprado@collabora.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Add missing dt-binding properties to rt5682(s) | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
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
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
On Mon, Oct 24, 2022 at 3:01 PM Nícolas F. R. A. Prado <nfraprado@collabora.com> 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 <nfraprado@collabora.com> > > --- > > 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 <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? 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 > <nfraprado@collabora.com> 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 <nfraprado@collabora.com> 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 > > <nfraprado@collabora.com> 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 Thu, Oct 27, 2022 at 10:48 AM Chen-Yu Tsai <wenst@chromium.org> wrote: > > On Thu, Oct 27, 2022 at 7:36 AM Nícolas F. R. A. Prado > <nfraprado@collabora.com> 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 > > > <nfraprado@collabora.com> 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? (Missed this question) The RT5682 has the same supplies, plus the VBAT one. ChenYu > > 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 Thu, Oct 27, 2022 at 11:11:08AM -0700, Chen-Yu Tsai wrote: > On Thu, Oct 27, 2022 at 10:48 AM Chen-Yu Tsai <wenst@chromium.org> wrote: > > > > On Thu, Oct 27, 2022 at 7:36 AM Nícolas F. R. A. Prado > > <nfraprado@collabora.com> 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 > > > > <nfraprado@collabora.com> 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? > > (Missed this question) > > The RT5682 has the same supplies, plus the VBAT one. > > ChenYu > > > > 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. So if I get this right we should be making the following supplies required: AVDD VBAT or MICVDD (on rt5682s there's no VBAT so just MICVDD) And for LDO1_ON and DBVDD what would be the best way to model? Should we make them required in the binding as well and add them pointing to the same regulator from AVDD in the chromebook DT or just leave them as optional? Thanks, Nícolas > > > > 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 Thu, Oct 27, 2022 at 12:20 PM Nícolas F. R. A. Prado <nfraprado@collabora.com> wrote: > > On Thu, Oct 27, 2022 at 11:11:08AM -0700, Chen-Yu Tsai wrote: > > On Thu, Oct 27, 2022 at 10:48 AM Chen-Yu Tsai <wenst@chromium.org> wrote: > > > > > > On Thu, Oct 27, 2022 at 7:36 AM Nícolas F. R. A. Prado > > > <nfraprado@collabora.com> 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 > > > > > <nfraprado@collabora.com> 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? > > > > (Missed this question) > > > > The RT5682 has the same supplies, plus the VBAT one. > > > > ChenYu > > > > > > 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. > > So if I get this right we should be making the following supplies required: > AVDD > VBAT or MICVDD (on rt5682s there's no VBAT so just MICVDD) That's right. > And for LDO1_ON and DBVDD what would be the best way to model? Should we make > them required in the binding as well and add them pointing to the same regulator > from AVDD in the chromebook DT or just leave them as optional? They should be required. And we then describe them to match the board. ChenYu > Thanks, > Nícolas > > > > > > > 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? > > > > [..]
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:
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 Documentation/devicetree/bindings/sound/realtek,rt5682s.yaml | 4 ++++ 1 file changed, 4 insertions(+)