Add I2S/ADV7511 audio support for ARC AXS10x boards

This patch adds audio support for the ADV7511 HDMI transmitter using ALSA SoC.

The code was ported from Analog Devices linux tree from commit 1770c4a1e32b ("Merge remote-tracking branch 'xilinx/master' into xcomm_zynq"), which is available at:
- https://github.com/analogdevicesinc/linux/

+- adi,enable-audio: If set the ADV7511 driver will register a codec interface
+  into ALSA SoC.

Quoted 7 lines · Jose Abreu

This patch adds audio support for the ADV7511 HDMI transmitter using ALSA SoC.

The code was ported from Analog Devices linux tree from commit 1770c4a1e32b ("Merge remote-tracking branch 'xilinx/master' into xcomm_zynq"), which is available at:
- https://github.com/analogdevicesinc/linux/

The reason why this driver is still out of tree, is because there has been no conclusion yet on how to go forward with the whole HDMI integration. So this is not going to get merged until that issue has been addressed.

[...]

Quoted 2 lines

+- adi,enable-audio: If set the ADV7511 driver will register a codec interface
+  into ALSA SoC.

This is not a description of the hardware.

[...]

include/sound/soc-dai.h struct snd_compr_stream; · line 36

Quoted 9 lines

include/sound/soc-dai.h +1 -0

index 964b7de..539c091 100644
@@ -33,6 +33,7 @@ struct snd_compr_stream;
 #define SND_SOC_DAIFMT_DSP_B		5 /* L data MSB during FRM LRC */
 #define SND_SOC_DAIFMT_AC97		6 /* AC97 */
 #define SND_SOC_DAIFMT_PDM		7 /* Pulse density modulation */
+#define SND_SOC_DAIFMT_SPDIF		8 /* SPDIF */

This needs to be a separate patch.

_______________________________________________
linux-snps-arc mailing list
linux-snps-arc at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

Quoted 7 lines · Jose Abreu

This patch adds audio support for the ADV7511 HDMI transmitter using ALSA SoC.

The code was ported from Analog Devices linux tree from commit 1770c4a1e32b ("Merge remote-tracking branch 'xilinx/master' into xcomm_zynq"), which is available at:
- https://github.com/analogdevicesinc/linux/

The reason why this driver is still out of tree, is because there has been no conclusion yet on how to go forward with the whole HDMI integration. So this is not going to get merged until that issue has been addressed.

[...]

Quoted 2 lines

+- adi,enable-audio: If set the ADV7511 driver will register a codec interface
+  into ALSA SoC.

This is not a description of the hardware.

[...]

include/sound/soc-dai.h struct snd_compr_stream; · line 36

Quoted 9 lines

include/sound/soc-dai.h +1 -0

index 964b7de..539c091 100644
@@ -33,6 +33,7 @@ struct snd_compr_stream;
 #define SND_SOC_DAIFMT_DSP_B		5 /* L data MSB during FRM LRC */
 #define SND_SOC_DAIFMT_AC97		6 /* AC97 */
 #define SND_SOC_DAIFMT_PDM		7 /* Pulse density modulation */
+#define SND_SOC_DAIFMT_SPDIF		8 /* SPDIF */

This needs to be a separate patch.

_______________________________________________
linux-snps-arc mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

Quoted 5 lines

[...]

Quoted 2 lines

+- adi,enable-audio: If set the ADV7511 driver will register a codec interface
+  into ALSA SoC.

This is not a description of the hardware.

Is this okay: "adi,enable-audio: Set this boolean parameter if ADV7511 transmitter routes audio signals" ?

On 04/08/2016 06:12 PM, Jose Abreu wrote:
[...]

Quoted 6 lines

Quoted 4 lines

[...]

Quoted 2 lines

+- adi,enable-audio: If set the ADV7511 driver will register a codec interface
+  into ALSA SoC.

This is not a description of the hardware.

Is this okay: "adi,enable-audio: Set this boolean parameter if ADV7511 transmitter routes audio signals" ?

I don't think we need this property. There is no problem with registering the audio part unconditionally. As long as there is no connection we wont create a sound card that is exposed to userspace.

Hi Lars,

Quoted 12 lines · Lars-Peter Clausen

On 04/08/2016 06:12 PM, Jose Abreu wrote:
[...]

Quoted 6 lines

Quoted 4 lines

[...]

Quoted 2 lines

+- adi,enable-audio: If set the ADV7511 driver will register a codec interface
+  into ALSA SoC.

This is not a description of the hardware.

Is this okay: "adi,enable-audio: Set this boolean parameter if ADV7511 transmitter routes audio signals" ?

I don't think we need this property. There is no problem with registering the audio part unconditionally. As long as there is no connection we wont create a sound card that is exposed to userspace.

This change was suggested by Laurent Pinchart and was introduced in v3. Quoting Laurent:
"The idea is that enabling support for ADV7511 audio in the kernel isn't coupled with whether the system includes audio support. It would be confusing, and would also waste resources, to create a Linux sound device when no sound channel is routed on the board."

Quoted 16 lines · Jose Abreu

Hi Lars,

Quoted 12 lines · Lars-Peter Clausen

On 04/08/2016 06:12 PM, Jose Abreu wrote:
[...]

Quoted 6 lines

Quoted 4 lines

[...]

Quoted 2 lines

+- adi,enable-audio: If set the ADV7511 driver will register a codec interface
+  into ALSA SoC.

This is not a description of the hardware.

Is this okay: "adi,enable-audio: Set this boolean parameter if ADV7511 transmitter routes audio signals" ?

I don't think we need this property. There is no problem with registering the audio part unconditionally. As long as there is no connection we wont create a sound card that is exposed to userspace.

Quoted 6 lines

This change was suggested by Laurent Pinchart and was introduced in v3. Quoting Laurent:
"The idea is that enabling support for ADV7511 audio in the kernel isn't coupled with whether the system includes audio support. It would be confusing, and would also waste resources, to create a Linux sound device when no sound channel is routed on the board."

I wouldn't care too much about this at this point, the extra amount of resources required for registering the CODEC (but not the sound card) is just a few bytes (sizeof(struct snd_soc_codec)).

Nevertheless what we should do is describe the hardware and from this information infer whether there is a audio connection or not and if there is none we might skip registering the CODEC. In my opinion this hardware description should be modeled using of-graph, having a connection between the SoC side and the adv7511 SPDIF or I2S port.

Hi Lars,

Quoted 27 lines · Lars-Peter Clausen

Quoted 16 lines · Jose Abreu

Hi Lars,

Quoted 12 lines · Lars-Peter Clausen

On 04/08/2016 06:12 PM, Jose Abreu wrote:
[...]

Quoted 6 lines

Quoted 4 lines

[...]

Quoted 2 lines

+- adi,enable-audio: If set the ADV7511 driver will register a codec interface
+  into ALSA SoC.

This is not a description of the hardware.

Is this okay: "adi,enable-audio: Set this boolean parameter if ADV7511 transmitter routes audio signals" ?

I don't think we need this property. There is no problem with registering the audio part unconditionally. As long as there is no connection we wont create a sound card that is exposed to userspace.

Quoted 6 lines

This change was suggested by Laurent Pinchart and was introduced in v3. Quoting Laurent:
"The idea is that enabling support for ADV7511 audio in the kernel isn't coupled with whether the system includes audio support. It would be confusing, and would also waste resources, to create a Linux sound device when no sound channel is routed on the board."

I wouldn't care too much about this at this point, the extra amount of resources required for registering the CODEC (but not the sound card) is just a few bytes (sizeof(struct snd_soc_codec)).

Quoted 6 lines

Nevertheless what we should do is describe the hardware and from this information infer whether there is a audio connection or not and if there is none we might skip registering the CODEC. In my opinion this hardware description should be modeled using of-graph, having a connection between the SoC side and the adv7511 SPDIF or I2S port.

You mean something like this:

sound_playback: sound_playback {
    compatible = "simple-audio-card";
    [...]
    simple-audio-card,format = "i2s";
    [...]
}

adv7511@xx {
    compatible = "adi,adv7511";
    [...]

    ports {
        [...]
        /* Audio Output */
        port@x {
            reg = <x>;
            endpoint {
                remote-endpoint = <&sound_playback>;
            }
        }
    }
}

Quoted 46 lines · Jose Abreu

Hi Lars,

Quoted 27 lines · Lars-Peter Clausen

Quoted 16 lines · Jose Abreu

Hi Lars,

Quoted 12 lines · Lars-Peter Clausen

On 04/08/2016 06:12 PM, Jose Abreu wrote:
[...]

Quoted 6 lines

Quoted 4 lines

[...]

Quoted 2 lines

+- adi,enable-audio: If set the ADV7511 driver will register a codec interface
+  into ALSA SoC.

This is not a description of the hardware.

Is this okay: "adi,enable-audio: Set this boolean parameter if ADV7511 transmitter routes audio signals" ?

I don't think we need this property. There is no problem with registering the audio part unconditionally. As long as there is no connection we wont create a sound card that is exposed to userspace.

Quoted 6 lines

This change was suggested by Laurent Pinchart and was introduced in v3. Quoting Laurent:
"The idea is that enabling support for ADV7511 audio in the kernel isn't coupled with whether the system includes audio support. It would be confusing, and would also waste resources, to create a Linux sound device when no sound channel is routed on the board."

I wouldn't care too much about this at this point, the extra amount of resources required for registering the CODEC (but not the sound card) is just a few bytes (sizeof(struct snd_soc_codec)).

Quoted 6 lines

Nevertheless what we should do is describe the hardware and from this information infer whether there is a audio connection or not and if there is none we might skip registering the CODEC. In my opinion this hardware description should be modeled using of-graph, having a connection between the SoC side and the adv7511 SPDIF or I2S port.

You mean something like this:

sound_playback: sound_playback {
    compatible = "simple-audio-card";
    [...]
    simple-audio-card,format = "i2s";
    [...]
}

Quoted 15 lines

adv7511@xx {
    compatible = "adi,adv7511";
    [...]

    ports {
        [...]
        /* Audio Output */
        port@x {
            reg = <x>;
            endpoint {
                remote-endpoint = <&sound_playback>;
            }
        }
    }
}

Yes, something like that. Not exactly like that, but similar. One of the core concepts of of-graph is that there is always a description of the connection from both sides, this way each side can independently figure out where it is connected.

Currently there is also zero support of of-graph in ASoC, so a bit of work is required to get this integrated properly.

Quoted 3 lines

Currently there is also zero support of of-graph in ASoC, so a bit of work is required to get this integrated properly.

I also believe this would be the better option but in the meantime can't I integrate the audio like it is being done in the dw-hdmi driver[1]? In this driver the audio is registered as a sound card and is conditionally built using Kconfig, just like I was doing in the previous versions. I know you said the HDMI audio is still an open issue but it seems that for this case it was accepted.

[1] http://lxr.free-electrons.com/source/drivers/gpu/drm/bridge/dw-hdmi-ahb-audio.c

+ Optional properties:
+ - snps,use-dmaengine: If set the driver will use ALSA DMA engine. If set
+ it is required to use the properties 'dmas' and 'dma-names'.

Quoted 3 lines · Jose Abreu

+ Optional properties:
+ - snps,use-dmaengine: If set the driver will use ALSA DMA engine. If set
+ it is required to use the properties 'dmas' and 'dma-names'.

This is not a good interface, it's describing Linux internal APIs. If the device needs to operate in PIO mode it should just do that.

Hi Mark,

Quoted 7 lines · Mark Brown

Quoted 3 lines · Jose Abreu

+ Optional properties:
+ - snps,use-dmaengine: If set the driver will use ALSA DMA engine. If set
+ it is required to use the properties 'dmas' and 'dma-names'.

This is not a good interface, it's describing Linux internal APIs. If the device needs to operate in PIO mode it should just do that.

I added this interface because there is no direct way to check if DMA is available on the I2S controller so it is not possible to automatically change between DMA and PIO mode. As the I2S controller can be built with or without DMA support it is necessary to somehow check if DMA is enabled or not and according to that use either ALSA DMA engine or the custom platform driver sent in these patches. I did not want to remove drivers functionality so I added this property to the DT. This way a user can select between DMA and PIO mode.

Quoted 11 lines · Jose Abreu

Hi Mark,

Quoted 7 lines · Mark Brown

Quoted 3 lines · Jose Abreu

+ Optional properties:
+ - snps,use-dmaengine: If set the driver will use ALSA DMA engine. If set
+ it is required to use the properties 'dmas' and 'dma-names'.

This is not a good interface, it's describing Linux internal APIs. If the device needs to operate in PIO mode it should just do that.

Quoted 7 lines

I added this interface because there is no direct way to check if DMA is available on the I2S controller so it is not possible to automatically change between DMA and PIO mode. As the I2S controller can be built with or without DMA support it is necessary to somehow check if DMA is enabled or not and according to that use either ALSA DMA engine or the custom platform driver sent in these patches. I did not want to remove drivers functionality so I added this property to the DT. This way a user can select between DMA and PIO mode.

That's OK, but you need to describe the hardware, not the indented behavior of the software driver.

Hi Lars,

Quoted 19 lines · Lars-Peter Clausen

Quoted 18 lines · Jose Abreu

Hi Mark,

Quoted 7 lines · Mark Brown

Quoted 3 lines · Jose Abreu

+ Optional properties:
+ - snps,use-dmaengine: If set the driver will use ALSA DMA engine. If set
+ it is required to use the properties 'dmas' and 'dma-names'.

This is not a good interface, it's describing Linux internal APIs. If the device needs to operate in PIO mode it should just do that.

I added this interface because there is no direct way to check if DMA is available on the I2S controller so it is not possible to automatically change between DMA and PIO mode. As the I2S controller can be built with or without DMA support it is necessary to somehow check if DMA is enabled or not and according to that use either ALSA DMA engine or the custom platform driver sent in these patches. I did not want to remove drivers functionality so I added this property to the DT. This way a user can select between DMA and PIO mode.

Quoted 3 lines

That's OK, but you need to describe the hardware, not the indented behavior of the software driver.

Is this okay: "snps,use-dmaengine: Set this boolean paramater if I2S controller has DMA support. If set the properties 'dmas' and 'dma-names' must be also set" ?

Quoted 23 lines · Jose Abreu

Hi Lars,

Quoted 19 lines · Lars-Peter Clausen

Quoted 18 lines · Jose Abreu

Hi Mark,

Quoted 7 lines · Mark Brown

Quoted 3 lines · Jose Abreu

+ Optional properties:
+ - snps,use-dmaengine: If set the driver will use ALSA DMA engine. If set
+ it is required to use the properties 'dmas' and 'dma-names'.

This is not a good interface, it's describing Linux internal APIs. If the device needs to operate in PIO mode it should just do that.

I added this interface because there is no direct way to check if DMA is available on the I2S controller so it is not possible to automatically change between DMA and PIO mode. As the I2S controller can be built with or without DMA support it is necessary to somehow check if DMA is enabled or not and according to that use either ALSA DMA engine or the custom platform driver sent in these patches. I did not want to remove drivers functionality so I added this property to the DT. This way a user can select between DMA and PIO mode.

Quoted 5 lines

Quoted 3 lines

That's OK, but you need to describe the hardware, not the indented behavior of the software driver.

Is this okay: "snps,use-dmaengine: Set this boolean paramater if I2S controller has DMA support. If set the properties 'dmas' and 'dma-names' must be also set" ?

The description is better. But the name of the property is still imperative rather then descriptive. It tells the software what should be done rather then describing what the hardware looks like.

Since there is already the dmas property which is present if a DMA is connected and is absent when no DMA is present it should be enough to just check that property rather than requiring an additional one.

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel