diff mbox

dt-bindings: exynos: Document #sound-dai-cells property of the HDMI node

Message ID 20180307171111.14551-1-s.nawrocki@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

The #sound-dai-cells DT property is required to describe link between
the HDMI IP block and the SoC's audio subsystem.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
 Documentation/devicetree/bindings/display/exynos/exynos_hdmi.txt | 3 +++
 1 file changed, 3 insertions(+)

Comments

Rob Herring (Arm) March 8, 2018, 2:41 a.m. UTC | #1
On Wed, Mar 07, 2018 at 06:11:11PM +0100, Sylwester Nawrocki wrote:
> The #sound-dai-cells DT property is required to describe link between
> the HDMI IP block and the SoC's audio subsystem.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
>  Documentation/devicetree/bindings/display/exynos/exynos_hdmi.txt | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Rob Herring <robh@kernel.org>
Inki Dae March 8, 2018, 4:01 a.m. UTC | #2
Hi Sylwester,

2018년 03월 08일 02:11에 Sylwester Nawrocki 이(가) 쓴 글:
> The #sound-dai-cells DT property is required to describe link between
> the HDMI IP block and the SoC's audio subsystem.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
>  Documentation/devicetree/bindings/display/exynos/exynos_hdmi.txt | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/exynos/exynos_hdmi.txt b/Documentation/devicetree/bindings/display/exynos/exynos_hdmi.txt
> index 8715ff06c457..6b2a526ec586 100644
> --- a/Documentation/devicetree/bindings/display/exynos/exynos_hdmi.txt
> +++ b/Documentation/devicetree/bindings/display/exynos/exynos_hdmi.txt
> @@ -50,6 +50,9 @@ Required properties for Exynos 5433:
>  - clock-names: aliases for above clock specfiers.
>  - samsung,sysreg: handle to syscon used to control the system registers.
>  
> +Optional properties for Exynos 4210, 4212, 5420 and 5433:
> + - #sound-dai-cells: should be 0.
> +

Just trivial question. 'sound-dai-cells' property could affect hdmi driver? I looked into HDMI codec driver but I didn't find relevat code.
I mean that if this property never affect HDMI driver then this property would be a dead thing even through this can be declared optionally.

Thanks,
Inki Dae

>  Example:
>  
>  	hdmi {
>
Marek Szyprowski March 8, 2018, 6:29 a.m. UTC | #3
Hi Inki,

On 2018-03-08 05:01, Inki Dae wrote:
> Hi Sylwester,
>
> 2018년 03월 08일 02:11에 Sylwester Nawrocki 이(가) 쓴 글:
>> The #sound-dai-cells DT property is required to describe link between
>> the HDMI IP block and the SoC's audio subsystem.
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> ---
>>   Documentation/devicetree/bindings/display/exynos/exynos_hdmi.txt | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/display/exynos/exynos_hdmi.txt b/Documentation/devicetree/bindings/display/exynos/exynos_hdmi.txt
>> index 8715ff06c457..6b2a526ec586 100644
>> --- a/Documentation/devicetree/bindings/display/exynos/exynos_hdmi.txt
>> +++ b/Documentation/devicetree/bindings/display/exynos/exynos_hdmi.txt
>> @@ -50,6 +50,9 @@ Required properties for Exynos 5433:
>>   - clock-names: aliases for above clock specfiers.
>>   - samsung,sysreg: handle to syscon used to control the system registers.
>>   
>> +Optional properties for Exynos 4210, 4212, 5420 and 5433:
>> + - #sound-dai-cells: should be 0.
>> +
> Just trivial question. 'sound-dai-cells' property could affect hdmi driver? I looked into HDMI codec driver but I didn't find relevat code.
> I mean that if this property never affect HDMI driver then this property would be a dead thing even through this can be declared optionally.

This property is used by ASoC framework when it is building connections
between all elements of the virtual 'sound card'. It allows generic
code to find proper driver for the digital audio interface (DAI) object.

Best regards
Inki Dae March 8, 2018, 6:50 a.m. UTC | #4
Hi Marek,

2018년 03월 08일 15:29에 Marek Szyprowski 이(가) 쓴 글:
> Hi Inki,
> 
> On 2018-03-08 05:01, Inki Dae wrote:
>> Hi Sylwester,
>>
>> 2018년 03월 08일 02:11에 Sylwester Nawrocki 이(가) 쓴 글:
>>> The #sound-dai-cells DT property is required to describe link between
>>> the HDMI IP block and the SoC's audio subsystem.
>>>
>>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>>> ---
>>>   Documentation/devicetree/bindings/display/exynos/exynos_hdmi.txt | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/exynos/exynos_hdmi.txt b/Documentation/devicetree/bindings/display/exynos/exynos_hdmi.txt
>>> index 8715ff06c457..6b2a526ec586 100644
>>> --- a/Documentation/devicetree/bindings/display/exynos/exynos_hdmi.txt
>>> +++ b/Documentation/devicetree/bindings/display/exynos/exynos_hdmi.txt
>>> @@ -50,6 +50,9 @@ Required properties for Exynos 5433:
>>>   - clock-names: aliases for above clock specfiers.
>>>   - samsung,sysreg: handle to syscon used to control the system registers.
>>>   +Optional properties for Exynos 4210, 4212, 5420 and 5433:
>>> + - #sound-dai-cells: should be 0.
>>> +
>> Just trivial question. 'sound-dai-cells' property could affect hdmi driver? I looked into HDMI codec driver but I didn't find relevat code.
>> I mean that if this property never affect HDMI driver then this property would be a dead thing even through this can be declared optionally.
> 
> This property is used by ASoC framework when it is building connections
> between all elements of the virtual 'sound card'. It allows generic
> code to find proper driver for the digital audio interface (DAI) object.

I also assumed that some place of ASoC framework checks this property. For this I looked into HDMI codec driver(sound/soc/codecs/hdmi-codec.c) and relevant interfaces of ASoC framework.
But I couldn't find it. :( Could you let me know which code of ASoC framework checks this? I saw this property only in 'snd_soc_of_get_dai_name' and 'snd_soc_of_get_dai_link_codecs' functions but seems these functions aren't called by the HDMI codec driver.

Thanks,
Inki Dae

> 
> Best regards
Marek Szyprowski March 8, 2018, 7:36 a.m. UTC | #5
Hi Inki,

On 2018-03-08 07:50, Inki Dae wrote:
> 2018년 03월 08일 15:29에 Marek Szyprowski 이(가) 쓴 글:
>> On 2018-03-08 05:01, Inki Dae wrote:
>>> 2018년 03월 08일 02:11에 Sylwester Nawrocki 이(가) 쓴 글:
>>>> The #sound-dai-cells DT property is required to describe link between
>>>> the HDMI IP block and the SoC's audio subsystem.
>>>>
>>>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>>>> ---
>>>>    Documentation/devicetree/bindings/display/exynos/exynos_hdmi.txt | 3 +++
>>>>    1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/display/exynos/exynos_hdmi.txt b/Documentation/devicetree/bindings/display/exynos/exynos_hdmi.txt
>>>> index 8715ff06c457..6b2a526ec586 100644
>>>> --- a/Documentation/devicetree/bindings/display/exynos/exynos_hdmi.txt
>>>> +++ b/Documentation/devicetree/bindings/display/exynos/exynos_hdmi.txt
>>>> @@ -50,6 +50,9 @@ Required properties for Exynos 5433:
>>>>    - clock-names: aliases for above clock specfiers.
>>>>    - samsung,sysreg: handle to syscon used to control the system registers.
>>>>    +Optional properties for Exynos 4210, 4212, 5420 and 5433:
>>>> + - #sound-dai-cells: should be 0.
>>>> +
>>> Just trivial question. 'sound-dai-cells' property could affect hdmi driver? I looked into HDMI codec driver but I didn't find relevat code.
>>> I mean that if this property never affect HDMI driver then this property would be a dead thing even through this can be declared optionally.
>> This property is used by ASoC framework when it is building connections
>> between all elements of the virtual 'sound card'. It allows generic
>> code to find proper driver for the digital audio interface (DAI) object.
> I also assumed that some place of ASoC framework checks this property. For this I looked into HDMI codec driver(sound/soc/codecs/hdmi-codec.c) and relevant interfaces of ASoC framework.
> But I couldn't find it. :( Could you let me know which code of ASoC framework checks this? I saw this property only in 'snd_soc_of_get_dai_name' and 'snd_soc_of_get_dai_link_codecs' functions but seems these functions aren't called by the HDMI codec driver.

It is used by snd_soc_of_get_dai_link_codecs() function, which is called
from respective board/machine driver. See sound/soc/samsung/odroid.c for
good example. It allows to automatically create connection to max98090 and
hdmi codec devices, which are specified in 'sound/codec' node (see
exynos5422-odroidxu3-audio.dtsi).

Best regards
Inki Dae March 8, 2018, 8:15 a.m. UTC | #6
Hi Marek,

2018년 03월 08일 16:36에 Marek Szyprowski 이(가) 쓴 글:
> Hi Inki,
> 
> On 2018-03-08 07:50, Inki Dae wrote:
>> 2018년 03월 08일 15:29에 Marek Szyprowski 이(가) 쓴 글:
>>> On 2018-03-08 05:01, Inki Dae wrote:
>>>> 2018년 03월 08일 02:11에 Sylwester Nawrocki 이(가) 쓴 글:
>>>>> The #sound-dai-cells DT property is required to describe link between
>>>>> the HDMI IP block and the SoC's audio subsystem.
>>>>>
>>>>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>>>>> ---
>>>>>    Documentation/devicetree/bindings/display/exynos/exynos_hdmi.txt | 3 +++
>>>>>    1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/display/exynos/exynos_hdmi.txt b/Documentation/devicetree/bindings/display/exynos/exynos_hdmi.txt
>>>>> index 8715ff06c457..6b2a526ec586 100644
>>>>> --- a/Documentation/devicetree/bindings/display/exynos/exynos_hdmi.txt
>>>>> +++ b/Documentation/devicetree/bindings/display/exynos/exynos_hdmi.txt
>>>>> @@ -50,6 +50,9 @@ Required properties for Exynos 5433:
>>>>>    - clock-names: aliases for above clock specfiers.
>>>>>    - samsung,sysreg: handle to syscon used to control the system registers.
>>>>>    +Optional properties for Exynos 4210, 4212, 5420 and 5433:
>>>>> + - #sound-dai-cells: should be 0.
>>>>> +
>>>> Just trivial question. 'sound-dai-cells' property could affect hdmi driver? I looked into HDMI codec driver but I didn't find relevat code.
>>>> I mean that if this property never affect HDMI driver then this property would be a dead thing even through this can be declared optionally.
>>> This property is used by ASoC framework when it is building connections
>>> between all elements of the virtual 'sound card'. It allows generic
>>> code to find proper driver for the digital audio interface (DAI) object.
>> I also assumed that some place of ASoC framework checks this property. For this I looked into HDMI codec driver(sound/soc/codecs/hdmi-codec.c) and relevant interfaces of ASoC framework.
>> But I couldn't find it. :( Could you let me know which code of ASoC framework checks this? I saw this property only in 'snd_soc_of_get_dai_name' and 'snd_soc_of_get_dai_link_codecs' functions but seems these functions aren't called by the HDMI codec driver.
> 
> It is used by snd_soc_of_get_dai_link_codecs() function, which is called
> from respective board/machine driver. See sound/soc/samsung/odroid.c for
> good example. It allows to automatically create connection to max98090 and
> hdmi codec devices, which are specified in 'sound/codec' node (see
> exynos5422-odroidxu3-audio.dtsi).

Ah, sorry, I missed to check that driver and checked only DRM drivers. :)
By the way, it seems 'sound-dai-cells' property never affect Exynos4210/4212/5420/5433. It seems that even through ALSA TM2 audio driver(tm2_wm5110.c) exists the driver never check the property.
However, this patch adds below description.
"Optional properties for Exynos 4210, 4212, 5420 and 5433"

Is there a possibility for other boards based on Exynos4210/4212/5420/5433 SoC to use this property later?

Thanks,
Inki Dae

> 
> Best regards
Hi Inki,

Cc: alsa-devel@alsa-project.org

On 03/08/2018 09:15 AM, Inki Dae wrote:
> By the way, it seems 'sound-dai-cells' property never affect Exynos4210/4212> 5420/5433. It seems that even through ALSA TM2 audio driver(tm2_wm5110.c) 
> exists the driver never check the property.
>
> However, this patch adds below description.
>
> "Optional properties for Exynos 4210, 4212, 5420 and 5433"
> 
> Is there a possibility for other boards based on Exynos4210/4212/5420/5433 
> SoC to use this property later?

All these SoCs have the HDMI IP block which has one input DAI, connected 
internally over I2S bus with the IS2 controller.

I think there is no advantage in limiting ourselves now only to SoC's 
for which we currently rely on that DT property in current kernel code, 
just to update this documentation later when we actually put the property 
in dts files. 

In case of exynos5420 we already require #sound-dai-cells for Odroid and 
I have also a patch for exynos5420-peach-pit board which will need it as 
well.

As far as exynos4210 and exynos4212 are concerned it's a matter of adding
support for Odroid-U3, then we will also need this property because
we are going to use "multi-codec" (HDMI and external I2S0 pins for CODEC
are wired in parallel).

In case of exynos5433 it just happens that the code in current driver 
doesn't require #sound-dai-cells property - one of the reasons I made it
this way was to avoid dependency on dts, but it doesn't imply we should 
describe the HW in DT incompletely. Once the property is in dtbs we can 
update the driver to use more generic code, instead of open coding things.

Actually I have forgotten to add also exynos5250 to the list.
Inki Dae March 9, 2018, 2:35 a.m. UTC | #8
Hi Sylwester,

2018년 03월 08일 20:22에 Sylwester Nawrocki 이(가) 쓴 글:
> Hi Inki,
> 
> Cc: alsa-devel@alsa-project.org
> 
> On 03/08/2018 09:15 AM, Inki Dae wrote:
>> By the way, it seems 'sound-dai-cells' property never affect Exynos4210/4212> 5420/5433. It seems that even through ALSA TM2 audio driver(tm2_wm5110.c) 
>> exists the driver never check the property.
>>
>> However, this patch adds below description.
>>
>> "Optional properties for Exynos 4210, 4212, 5420 and 5433"
>>
>> Is there a possibility for other boards based on Exynos4210/4212/5420/5433 
>> SoC to use this property later?
> 
> All these SoCs have the HDMI IP block which has one input DAI, connected 
> internally over I2S bus with the IS2 controller.
> 
> I think there is no advantage in limiting ourselves now only to SoC's 
> for which we currently rely on that DT property in current kernel code, 
> just to update this documentation later when we actually put the property 
> in dts files.
> 
> In case of exynos5420 we already require #sound-dai-cells for Odroid and

Maybe exynos5422? Odroid XU3/4 use Exynos5422.
 
> I have also a patch for exynos5420-peach-pit board which will need it as 
> well.
> 
> As far as exynos4210 and exynos4212 are concerned it's a matter of adding
> support for Odroid-U3, then we will also need this property because
> we are going to use "multi-codec" (HDMI and external I2S0 pins for CODEC
> are wired in parallel).
> 
> In case of exynos5433 it just happens that the code in current driver 
> doesn't require #sound-dai-cells property - one of the reasons I made it
> this way was to avoid dependency on dts, but it doesn't imply we should 
> describe the HW in DT incompletely. Once the property is in dtbs we can 
> update the driver to use more generic code, instead of open coding things.
> 

Good idea.

> Actually I have forgotten to add also exynos5250 to the list.

Seems that '#sound-dai-cells' is required mandatorily in case of Exynos5422 because Odroid XU3/4 Audio driver checks this property, and the driver returns error if the property doesn't exist in its device tree.
And if other SoCs - Exynos4210/4212/5250/5420/5433 - require this property with additional driver works later then this property would also be required mandatorily for them not optionally. In this case we may need to modify the description again.

So my opinion is to add only the description required mandatorily if I understood correctly.

Thanks,
Inki Dae

>
On 03/09/2018 03:35 AM, Inki Dae wrote:
> 2018년 03월 08일 20:22에 Sylwester Nawrocki 이(가) 쓴 글:
>> On 03/08/2018 09:15 AM, Inki Dae wrote:
[...]
>>> Is there a possibility for other boards based on Exynos4210/4212/5420/5433 
>>> SoC to use this property later?
>>
>> All these SoCs have the HDMI IP block which has one input DAI, connected 
>> internally over I2S bus with the I2S controller.
>>
>> I think there is no advantage in limiting ourselves now only to SoC's 
>> for which we currently rely on that DT property in current kernel code, 
>> just to update this documentation later when we actually put the property 
>> in dts files.
>>
>> In case of exynos5420 we already require #sound-dai-cells for Odroid and
> 
> Maybe exynos5422? Odroid XU3/4 use Exynos5422.

Yes, my mistake, Exynos5422 for Odroid and Exynos5420 for Peach-Pit.
It just happens we use "samsung,exynos5420-hdmi" compatible for that both
SoCs.
 

> Seems that '#sound-dai-cells' is required mandatorily in case of Exynos5422 
> because Odroid XU3/4 Audio driver checks this property, and the driver 
> returns error if the property doesn't exist in its device tree.
> And if other SoCs - Exynos4210/4212/5250/5420/5433 - require this property 
> with additional driver works later then this property would also be required 
> mandatorily for them not optionally. In this case we may need to modify the 
> description again.
> 
> So my opinion is to add only the description required mandatorily if I 
> understood correctly.

You are right, it would be better to make this a mandatory property, I only
made it optional because now this property is missing in many dts files.
Let me resend with #sound-dai-cells description moved to the "Required 
properties" paragraph.

--
Thanks,
Sylwester
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/display/exynos/exynos_hdmi.txt b/Documentation/devicetree/bindings/display/exynos/exynos_hdmi.txt
index 8715ff06c457..6b2a526ec586 100644
--- a/Documentation/devicetree/bindings/display/exynos/exynos_hdmi.txt
+++ b/Documentation/devicetree/bindings/display/exynos/exynos_hdmi.txt
@@ -50,6 +50,9 @@  Required properties for Exynos 5433:
 - clock-names: aliases for above clock specfiers.
 - samsung,sysreg: handle to syscon used to control the system registers.
 
+Optional properties for Exynos 4210, 4212, 5420 and 5433:
+ - #sound-dai-cells: should be 0.
+
 Example:
 
 	hdmi {