diff mbox series

[v9,1/8] ASoC: dt-bindings: sun50i-codec-analog: Add headphone amp regulator supply

Message ID 20181122102325.29495-2-wens@csie.org (mailing list archive)
State Accepted
Commit 21dd30200e3d350fe420d23a2a00b17633a3d1a2
Headers show
Series Add support for audiocodec in Allwinner A64 | expand

Commit Message

Chen-Yu Tsai Nov. 22, 2018, 10:23 a.m. UTC
On the Allwinner A64 SoC, the audio codec has a built-in headphone
amplifier. This amplifier has a power supply separate from the rest of
the analog audio circuitry.

Add a regulator supply property to handle this.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 Documentation/devicetree/bindings/sound/sun50i-codec-analog.txt | 2 ++
 1 file changed, 2 insertions(+)

Comments

Maxime Ripard Nov. 22, 2018, 3:32 p.m. UTC | #1
On Thu, Nov 22, 2018 at 06:23:18PM +0800, Chen-Yu Tsai wrote:
> On the Allwinner A64 SoC, the audio codec has a built-in headphone
> amplifier. This amplifier has a power supply separate from the rest of
> the analog audio circuitry.
> 
> Add a regulator supply property to handle this.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> ---
>  Documentation/devicetree/bindings/sound/sun50i-codec-analog.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sound/sun50i-codec-analog.txt b/Documentation/devicetree/bindings/sound/sun50i-codec-analog.txt
> index 4f8ad0e04d20..056a098495cc 100644
> --- a/Documentation/devicetree/bindings/sound/sun50i-codec-analog.txt
> +++ b/Documentation/devicetree/bindings/sound/sun50i-codec-analog.txt
> @@ -4,9 +4,11 @@ Required properties:
>  - compatible: must be one of the following compatibles:
>  		- "allwinner,sun50i-a64-codec-analog"
>  - reg: must contain the registers location and length
> +- cpvdd-supply: Regulator supply for the headphone amplifier

I guess we should make this optional, since even if it's missing, it's
going to create a dummy regulator. and that allows to have backward
compatibility.

For the rest of the series,
Acked-by: Maxime Ripard <maxime.ripard@bootlin.com>

Thanks!
Maxime
Chen-Yu Tsai Nov. 22, 2018, 3:41 p.m. UTC | #2
On Thu, Nov 22, 2018 at 11:33 PM Maxime Ripard
<maxime.ripard@bootlin.com> wrote:
>
> On Thu, Nov 22, 2018 at 06:23:18PM +0800, Chen-Yu Tsai wrote:
> > On the Allwinner A64 SoC, the audio codec has a built-in headphone
> > amplifier. This amplifier has a power supply separate from the rest of
> > the analog audio circuitry.
> >
> > Add a regulator supply property to handle this.
> >
> > Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> > ---
> >  Documentation/devicetree/bindings/sound/sun50i-codec-analog.txt | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/sound/sun50i-codec-analog.txt b/Documentation/devicetree/bindings/sound/sun50i-codec-analog.txt
> > index 4f8ad0e04d20..056a098495cc 100644
> > --- a/Documentation/devicetree/bindings/sound/sun50i-codec-analog.txt
> > +++ b/Documentation/devicetree/bindings/sound/sun50i-codec-analog.txt
> > @@ -4,9 +4,11 @@ Required properties:
> >  - compatible: must be one of the following compatibles:
> >               - "allwinner,sun50i-a64-codec-analog"
> >  - reg: must contain the registers location and length
> > +- cpvdd-supply: Regulator supply for the headphone amplifier
>
> I guess we should make this optional, since even if it's missing, it's
> going to create a dummy regulator. and that allows to have backward
> compatibility.
>
> For the rest of the series,
> Acked-by: Maxime Ripard <maxime.ripard@bootlin.com>

This one (not sun8i-codec-analog) is new for the next release, so there's
no backward compatibility to consider. Plus none of the device tree changes
for A64 have been merged.

ChenYu
Maxime Ripard Nov. 22, 2018, 3:48 p.m. UTC | #3
On Thu, Nov 22, 2018 at 11:41:57PM +0800, Chen-Yu Tsai wrote:
> On Thu, Nov 22, 2018 at 11:33 PM Maxime Ripard
> <maxime.ripard@bootlin.com> wrote:
> >
> > On Thu, Nov 22, 2018 at 06:23:18PM +0800, Chen-Yu Tsai wrote:
> > > On the Allwinner A64 SoC, the audio codec has a built-in headphone
> > > amplifier. This amplifier has a power supply separate from the rest of
> > > the analog audio circuitry.
> > >
> > > Add a regulator supply property to handle this.
> > >
> > > Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> > > ---
> > >  Documentation/devicetree/bindings/sound/sun50i-codec-analog.txt | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/sound/sun50i-codec-analog.txt b/Documentation/devicetree/bindings/sound/sun50i-codec-analog.txt
> > > index 4f8ad0e04d20..056a098495cc 100644
> > > --- a/Documentation/devicetree/bindings/sound/sun50i-codec-analog.txt
> > > +++ b/Documentation/devicetree/bindings/sound/sun50i-codec-analog.txt
> > > @@ -4,9 +4,11 @@ Required properties:
> > >  - compatible: must be one of the following compatibles:
> > >               - "allwinner,sun50i-a64-codec-analog"
> > >  - reg: must contain the registers location and length
> > > +- cpvdd-supply: Regulator supply for the headphone amplifier
> >
> > I guess we should make this optional, since even if it's missing, it's
> > going to create a dummy regulator. and that allows to have backward
> > compatibility.
> >
> > For the rest of the series,
> > Acked-by: Maxime Ripard <maxime.ripard@bootlin.com>
> 
> This one (not sun8i-codec-analog) is new for the next release, so there's
> no backward compatibility to consider. Plus none of the device tree changes
> for A64 have been merged.

Ok, that works for me then.

Thanks!
Maxime
Chen-Yu Tsai Nov. 23, 2018, 2:11 a.m. UTC | #4
On Thu, Nov 22, 2018 at 11:48 PM Maxime Ripard
<maxime.ripard@bootlin.com> wrote:
>
> On Thu, Nov 22, 2018 at 11:41:57PM +0800, Chen-Yu Tsai wrote:
> > On Thu, Nov 22, 2018 at 11:33 PM Maxime Ripard
> > <maxime.ripard@bootlin.com> wrote:
> > >
> > > On Thu, Nov 22, 2018 at 06:23:18PM +0800, Chen-Yu Tsai wrote:
> > > > On the Allwinner A64 SoC, the audio codec has a built-in headphone
> > > > amplifier. This amplifier has a power supply separate from the rest of
> > > > the analog audio circuitry.
> > > >
> > > > Add a regulator supply property to handle this.
> > > >
> > > > Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> > > > ---
> > > >  Documentation/devicetree/bindings/sound/sun50i-codec-analog.txt | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/sound/sun50i-codec-analog.txt b/Documentation/devicetree/bindings/sound/sun50i-codec-analog.txt
> > > > index 4f8ad0e04d20..056a098495cc 100644
> > > > --- a/Documentation/devicetree/bindings/sound/sun50i-codec-analog.txt
> > > > +++ b/Documentation/devicetree/bindings/sound/sun50i-codec-analog.txt
> > > > @@ -4,9 +4,11 @@ Required properties:
> > > >  - compatible: must be one of the following compatibles:
> > > >               - "allwinner,sun50i-a64-codec-analog"
> > > >  - reg: must contain the registers location and length
> > > > +- cpvdd-supply: Regulator supply for the headphone amplifier
> > >
> > > I guess we should make this optional, since even if it's missing, it's
> > > going to create a dummy regulator. and that allows to have backward
> > > compatibility.
> > >
> > > For the rest of the series,
> > > Acked-by: Maxime Ripard <maxime.ripard@bootlin.com>
> >
> > This one (not sun8i-codec-analog) is new for the next release, so there's
> > no backward compatibility to consider. Plus none of the device tree changes
> > for A64 have been merged.
>
> Ok, that works for me then.

Well, I misspoke. The driver was introduced in 4.20-rc1, and these changes will
end up in 4.21. But again, since we don't have any device tree changes already
merged, we should be fine.

Implementation wise, there really isn't a "required" variant of the regulator
API. The API will use the dummy regulator if it's missing.

ChenYu
Mark Brown Nov. 23, 2018, 1:31 p.m. UTC | #5
On Thu, Nov 22, 2018 at 04:32:41PM +0100, Maxime Ripard wrote:
> On Thu, Nov 22, 2018 at 06:23:18PM +0800, Chen-Yu Tsai wrote:

> > +- cpvdd-supply: Regulator supply for the headphone amplifier

> I guess we should make this optional, since even if it's missing, it's
> going to create a dummy regulator. and that allows to have backward
> compatibility.

Regulators should never be marked as optional unless they can be
physically absent.  It is true that Linux has code to handle partially
defined device trees but we shouldn't rely on other OSs having similar
code and it's better practice to have board device trees be fully
specified, it means that if we need the regulator information for some
future thing it's more likely to be there and it's less likely that bugs
which do things like cause the regulator to be powered off incorrectly
will be introduced in future.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/sound/sun50i-codec-analog.txt b/Documentation/devicetree/bindings/sound/sun50i-codec-analog.txt
index 4f8ad0e04d20..056a098495cc 100644
--- a/Documentation/devicetree/bindings/sound/sun50i-codec-analog.txt
+++ b/Documentation/devicetree/bindings/sound/sun50i-codec-analog.txt
@@ -4,9 +4,11 @@  Required properties:
 - compatible: must be one of the following compatibles:
 		- "allwinner,sun50i-a64-codec-analog"
 - reg: must contain the registers location and length
+- cpvdd-supply: Regulator supply for the headphone amplifier
 
 Example:
 	codec_analog: codec-analog@1f015c0 {
 		compatible = "allwinner,sun50i-a64-codec-analog";
 		reg = <0x01f015c0 0x4>;
+		cpvdd-supply = <&reg_eldo1>;
 	};