diff mbox

[RFC,6/6] ASoC: samsung: Add DT bindings documentation for TM2 sound subsystem

Message ID 1465815160-28504-7-git-send-email-s.nawrocki@samsung.com
State Changes Requested, archived
Headers show

Commit Message

Sylwester Nawrocki June 13, 2016, 10:52 a.m. UTC
This patch adds DT binding documentation for Exnos5433 based TM2
and TM2E boards sound subsystem.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
 .../bindings/sound/samsung,tm2-wm5110.txt          | 39 ++++++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/samsung,tm2-wm5110.txt

Comments

Rob Herring June 14, 2016, 11:32 p.m. UTC | #1
On Mon, Jun 13, 2016 at 12:52:40PM +0200, Sylwester Nawrocki wrote:
> This patch adds DT binding documentation for Exnos5433 based TM2
> and TM2E boards sound subsystem.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
>  .../bindings/sound/samsung,tm2-wm5110.txt          | 39 ++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/samsung,tm2-wm5110.txt
> 
> diff --git a/Documentation/devicetree/bindings/sound/samsung,tm2-wm5110.txt b/Documentation/devicetree/bindings/sound/samsung,tm2-wm5110.txt
> new file mode 100644
> index 0000000..32f69fcc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/samsung,tm2-wm5110.txt
> @@ -0,0 +1,39 @@
> +Samsung Exynos5433 TM2(E) audio complex with WM5110 codec
> +
> +Required properties:
> +
> + - compatible : "samsung,tm2-audio"

SoC specific compatible string please.

> + - samsung,model : the user-visible name of this sound complex

I think we have a standard property for this.

> + - clocks : must contain an entry for each entry in clock-names,
> +   see ../clocks/clock-bindings.txt for details
> + - clock-names : must include the following entries:
> +   "mclk1", "mclk2"

> + - samsung,i2s-controller : the phandle of the I2S controller
> + - samsung,speaker-amplifier : the phandle of the MAX98504 amplifier

We should have standard properties for these. 2nd ones I've seen today.

> + - samsung,audio-routing : a list of the connections between audio
> +   components;  each entry is a pair of strings, the first being the
> +   connection's sink, the second being the connection's source;
> +   valid names for sources and sinks are the WM5110's and MAX98504's
> +   pins and the jacks on the board:
> +   HP, SPK, Main Mic, Sub Mic, Third Mic, Headset Mic.
> + - mic-bias-gpios : GPIO pin that enables the Main Mic bias regulator
> +
> +Example:
> +
> +sound {
> +	compatible = "samsung,tm2-audio";
> +	clocks = <&pmu_system_controller 0>, <&s2mps13_osc 2>;
> +	clock-names = "mclk1", "mclk2";
> +	samsung,i2s-controller = <&i2s0>;
> +	samsung,speaker-amplifier = <&max98504>;
> +	samsung,model = "wm5110";
> +	mic-bias-gpios = <&gpr3 2 0>;
> +	samsung,audio-routing =
> +		"HP", "HPOUT1L",
> +		"HP", "HPOUT1R",
> +		"SPK", "SPKOUT",
> +		"SPKOUT", "HPOUT2L",
> +		"SPKOUT", "HPOUT2R",
> +		"Main Mic", "MICBIAS2",
> +		"IN1R", "Main Mic";
> +};
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lars-Peter Clausen June 15, 2016, 9:40 a.m. UTC | #2
On 06/13/2016 12:52 PM, Sylwester Nawrocki wrote:
[...]
> + - compatible : "samsung,tm2-audio"
> + - samsung,model : the user-visible name of this sound complex
> + - clocks : must contain an entry for each entry in clock-names,
> +   see ../clocks/clock-bindings.txt for details
> + - clock-names : must include the following entries:
> +   "mclk1", "mclk2"

If these are the MCLKs that go to the CODEC they should be properties of the
CODEC node.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown June 15, 2016, 9:47 a.m. UTC | #3
On Tue, Jun 14, 2016 at 06:32:24PM -0500, Rob Herring wrote:
> On Mon, Jun 13, 2016 at 12:52:40PM +0200, Sylwester Nawrocki wrote:

> > +Samsung Exynos5433 TM2(E) audio complex with WM5110 codec

> > +Required properties:

> > + - compatible : "samsung,tm2-audio"

> SoC specific compatible string please.

No, this isn't a SoC IP - this is a binding for a board called TM2(E)
which has a bunch of chips on it including this.

> > + - samsung,i2s-controller : the phandle of the I2S controller
> > + - samsung,speaker-amplifier : the phandle of the MAX98504 amplifier

> We should have standard properties for these. 2nd ones I've seen today.

No, these aren't fixed roles in a system, you couldn't have standard
handling for them.
Krzysztof Kozlowski June 16, 2016, 11:33 a.m. UTC | #4
On 06/13/2016 12:52 PM, Sylwester Nawrocki wrote:
> This patch adds DT binding documentation for Exnos5433 based TM2

s/Exnos5433/Exynos5433/

> and TM2E boards sound subsystem.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
>  .../bindings/sound/samsung,tm2-wm5110.txt          | 39 ++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/samsung,tm2-wm5110.txt
> 

Acked-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sylwester Nawrocki June 16, 2016, 4:39 p.m. UTC | #5
On 06/15/2016 11:40 AM, Lars-Peter Clausen wrote:
> On 06/13/2016 12:52 PM, Sylwester Nawrocki wrote:
> [...]
>> > + - compatible : "samsung,tm2-audio"
>> > + - samsung,model : the user-visible name of this sound complex
>> > + - clocks : must contain an entry for each entry in clock-names,
>> > +   see ../clocks/clock-bindings.txt for details
>> > + - clock-names : must include the following entries:
>> > +   "mclk1", "mclk2"
>
> If these are the MCLKs that go to the CODEC they should be properties of the
> CODEC node.

Yes, these are both CODEC clocks. I couldn't find any related clk code in
the wm5110 arizona codec driver and indeed the clocks stay disabled when
I specify them in the wm5110 DT node. AFAICS from other thread in this
mailing list it is going to take some time until the CODEC drivers are
updated. How about minimizing the requirement of those clocks, making them
optional and then when the CODEC driver gets updated specifying the clocks
in the codec node instead of the sound card one?
The MCLK1 rate could be just hard coded instead of using clk_get_rate(),
but without clk enable calls nothing works.


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lars-Peter Clausen June 16, 2016, 4:45 p.m. UTC | #6
On 06/16/2016 06:39 PM, Sylwester Nawrocki wrote:
> On 06/15/2016 11:40 AM, Lars-Peter Clausen wrote:
>> On 06/13/2016 12:52 PM, Sylwester Nawrocki wrote:
>> [...]
>>>> + - compatible : "samsung,tm2-audio"
>>>> + - samsung,model : the user-visible name of this sound complex
>>>> + - clocks : must contain an entry for each entry in clock-names,
>>>> +   see ../clocks/clock-bindings.txt for details
>>>> + - clock-names : must include the following entries:
>>>> +   "mclk1", "mclk2"
>>
>> If these are the MCLKs that go to the CODEC they should be properties of the
>> CODEC node.
> 
> Yes, these are both CODEC clocks. I couldn't find any related clk code in
> the wm5110 arizona codec driver and indeed the clocks stay disabled when
> I specify them in the wm5110 DT node. AFAICS from other thread in this
> mailing list it is going to take some time until the CODEC drivers are
> updated. How about minimizing the requirement of those clocks, making them
> optional and then when the CODEC driver gets updated specifying the clocks
> in the codec node instead of the sound card one?
> The MCLK1 rate could be just hard coded instead of using clk_get_rate(),
> but without clk enable calls nothing works.

You can still handle the clocks in the machine driver, but they should be
specified in the node of the CODEC to accurately represent the hardware.

The simple-card driver e.g. does this, requests the clock of the CODEC and
uses it to configure the CODEC input frequency based on it with
snd_soc_dai_set_sysclk().

- Lars

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sylwester Nawrocki June 17, 2016, 11:08 a.m. UTC | #7
On 06/16/2016 06:45 PM, Lars-Peter Clausen wrote:
> You can still handle the clocks in the machine driver, but they should be
> specified in the node of the CODEC to accurately represent the hardware.
> 
> The simple-card driver e.g. does this, requests the clock of the CODEC and
> uses it to configure the CODEC input frequency based on it with
> snd_soc_dai_set_sysclk().

I was assuming parsing the CODEC node was not preferred. Thanks for the
clarification, I have reworked the machine driver to use the CODEC node.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring June 20, 2016, 5:49 p.m. UTC | #8
On Wed, Jun 15, 2016 at 4:47 AM, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Jun 14, 2016 at 06:32:24PM -0500, Rob Herring wrote:
>> On Mon, Jun 13, 2016 at 12:52:40PM +0200, Sylwester Nawrocki wrote:
>
>> > +Samsung Exynos5433 TM2(E) audio complex with WM5110 codec
>
>> > +Required properties:
>
>> > + - compatible : "samsung,tm2-audio"
>
>> SoC specific compatible string please.
>
> No, this isn't a SoC IP - this is a binding for a board called TM2(E)
> which has a bunch of chips on it including this.

Okay, good.

>
>> > + - samsung,i2s-controller : the phandle of the I2S controller
>> > + - samsung,speaker-amplifier : the phandle of the MAX98504 amplifier
>
>> We should have standard properties for these. 2nd ones I've seen today.
>
> No, these aren't fixed roles in a system, you couldn't have standard
> handling for them.

What do you mean? It is silly for us to put vendor prefixes on all of
these. There are dozens of examples in the binding docs of
<vendor>,i2s-controller and <vendor>,audio-codec. Yes, dropping just
the vendor prefix doesn't buy much (maybe some string space), but it
certainly adds nothing.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sylwester Nawrocki June 21, 2016, 9:48 a.m. UTC | #9
On 06/20/2016 07:49 PM, Rob Herring wrote:
>>>> + - samsung,i2s-controller : the phandle of the I2S controller
>>>> + - samsung,speaker-amplifier : the phandle of the MAX98504 amplifier

I'm considering changing this to a more generic (samsung,?)audio-amplifier
in next iteration. Or maybe just listing MAX98504 as second entry of the
(samsung,)audio-codec(s) property value, since capabilities and functionality 
of the device in the system is not limited to just simple audio power 
amplification. It accepts analogue audio signal on its input and is 
transmitting digital audio data stream in the feedback path to the WM5110 
codec's DSP.

>>> >> We should have standard properties for these. 2nd ones I've seen today.
>> >
>> > No, these aren't fixed roles in a system, you couldn't have standard
>> > handling for them.
>
> What do you mean? It is silly for us to put vendor prefixes on all of
> these. There are dozens of examples in the binding docs of
> <vendor>,i2s-controller and <vendor>,audio-codec. Yes, dropping just
> the vendor prefix doesn't buy much (maybe some string space), but it
> certainly adds nothing.

It also looked unnecessary to me to at first sight to be adding
vendor prefixes to those properties. We could look at the compatible
string in the pointed node for handling any differences.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/sound/samsung,tm2-wm5110.txt b/Documentation/devicetree/bindings/sound/samsung,tm2-wm5110.txt
new file mode 100644
index 0000000..32f69fcc
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/samsung,tm2-wm5110.txt
@@ -0,0 +1,39 @@ 
+Samsung Exynos5433 TM2(E) audio complex with WM5110 codec
+
+Required properties:
+
+ - compatible : "samsung,tm2-audio"
+ - samsung,model : the user-visible name of this sound complex
+ - clocks : must contain an entry for each entry in clock-names,
+   see ../clocks/clock-bindings.txt for details
+ - clock-names : must include the following entries:
+   "mclk1", "mclk2"
+ - samsung,i2s-controller : the phandle of the I2S controller
+ - samsung,speaker-amplifier : the phandle of the MAX98504 amplifier
+ - samsung,audio-routing : a list of the connections between audio
+   components;  each entry is a pair of strings, the first being the
+   connection's sink, the second being the connection's source;
+   valid names for sources and sinks are the WM5110's and MAX98504's
+   pins and the jacks on the board:
+   HP, SPK, Main Mic, Sub Mic, Third Mic, Headset Mic.
+ - mic-bias-gpios : GPIO pin that enables the Main Mic bias regulator
+
+Example:
+
+sound {
+	compatible = "samsung,tm2-audio";
+	clocks = <&pmu_system_controller 0>, <&s2mps13_osc 2>;
+	clock-names = "mclk1", "mclk2";
+	samsung,i2s-controller = <&i2s0>;
+	samsung,speaker-amplifier = <&max98504>;
+	samsung,model = "wm5110";
+	mic-bias-gpios = <&gpr3 2 0>;
+	samsung,audio-routing =
+		"HP", "HPOUT1L",
+		"HP", "HPOUT1R",
+		"SPK", "SPKOUT",
+		"SPKOUT", "HPOUT2L",
+		"SPKOUT", "HPOUT2R",
+		"Main Mic", "MICBIAS2",
+		"IN1R", "Main Mic";
+};