diff mbox series

[v2,2/2] arm64: dts: imx8mn-bsh-smm-s2pro: Add tlv320aic31xx audio card node

Message ID 20220210134049.32576-2-ariel.dalessandro@collabora.com
State New
Headers show
Series [v2,1/2] ASoC: bindings: fsl-asoc-card: Add compatible for tlv320aic31xx codec | expand

Commit Message

Ariel D'Alessandro Feb. 10, 2022, 1:40 p.m. UTC 
BSH SystemMaster (SMM) S2 PRO board comes with an audio card based on
tlv320aic31xx family codec.

The audio card exposes two playback devices, one of them using the EASRC
(Enhanced Asynchronous Sample Rate Converter) module. Note that this
would require SDMA and EASRC firmware in order to work.

Signed-off-by: Ariel D'Alessandro <ariel.dalessandro@collabora.com>
Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
---
 .../dts/freescale/imx8mn-bsh-smm-s2pro.dts    | 94 +++++++++++++++++++
 1 file changed, 94 insertions(+)

Comments

Ariel D'Alessandro Feb. 14, 2022, 11:45 a.m. UTC | #1 
Hi Shawn,

Thanks for the review, will address all the below changes in v3.

Regards,
Ariel

On 2/13/22 01:18, Shawn Guo wrote:
> On Thu, Feb 10, 2022 at 10:40:49AM -0300, Ariel D'Alessandro wrote:
>> BSH SystemMaster (SMM) S2 PRO board comes with an audio card based on
>> tlv320aic31xx family codec.
>>
>> The audio card exposes two playback devices, one of them using the EASRC
>> (Enhanced Asynchronous Sample Rate Converter) module. Note that this
>> would require SDMA and EASRC firmware in order to work.
>>
>> Signed-off-by: Ariel D'Alessandro <ariel.dalessandro@collabora.com>
>> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
>> ---
>>  .../dts/freescale/imx8mn-bsh-smm-s2pro.dts    | 94 +++++++++++++++++++
>>  1 file changed, 94 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/freescale/imx8mn-bsh-smm-s2pro.dts b/arch/arm64/boot/dts/freescale/imx8mn-bsh-smm-s2pro.dts
>> index c6a8ed6745c1..3621354b4a92 100644
>> --- a/arch/arm64/boot/dts/freescale/imx8mn-bsh-smm-s2pro.dts
>> +++ b/arch/arm64/boot/dts/freescale/imx8mn-bsh-smm-s2pro.dts
>> @@ -7,6 +7,7 @@
>>  /dts-v1/;
>>  
>>  #include "imx8mn-bsh-smm-s2-common.dtsi"
>> +#include <dt-bindings/sound/tlv320aic31xx.h>
>>  
>>  / {
>>  	model = "BSH SMM S2 PRO";
>> @@ -16,6 +17,69 @@ memory@40000000 {
>>  		device_type = "memory";
>>  		reg = <0x0 0x40000000 0x0 0x20000000>;
>>  	};
>> +
>> +	sound-tlv320aic31xx {
>> +		compatible = "fsl,imx-audio-tlv320aic31xx";
>> +		model = "tlv320aic31xx-hifi";
>> +		audio-cpu = <&sai3>;
>> +		audio-codec = <&codec>;
>> +		audio-asrc = <&easrc>;
>> +		audio-routing =
>> +			"Ext Spk", "SPL",
>> +			"Ext Spk", "SPR";
>> +		mclk-id = <PLL_CLKIN_BCLK>;
>> +	};
>> +
>> +	vdd_input: vdd_input {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "vdd_input";
>> +		regulator-min-microvolt = <5000000>;
>> +		regulator-max-microvolt = <5000000>;
>> +	};
>> +};
>> +
>> +&easrc {
>> +	fsl,asrc-rate  = <48000>;
> 
> Double spaces before '='.
> 
>> +	fsl,asrc-format = <10>;
>> +	status = "okay";
>> +};
>> +
>> +&i2c2 {
>> +	clock-frequency = <400000>;
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&pinctrl_i2c2>;
>> +	status = "okay";
>> +
>> +	codec: tlv320dac3101@18 {
> 
> tlv320dac3101: audio-codec@18
> 
>> +		#sound-dai-cells = <0>;
>> +		compatible = "ti,tlv320dac3101";
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pinctrl_dac_rst>;
>> +		reg = <0x18>;
>> +
>> +		ai31xx-micbias-vg = <MICBIAS_AVDDV>;
>> +
>> +		HPVDD-supply = <&buck4_reg>;
>> +		SPRVDD-supply = <&vdd_input>;
>> +		SPLVDD-supply = <&vdd_input>;
>> +		AVDD-supply = <&buck4_reg>;
>> +		IOVDD-supply = <&buck4_reg>;
>> +		DVDD-supply = <&buck5_reg>;
>> +		reset-gpios = <&gpio1 6 GPIO_ACTIVE_LOW>;
>> +
>> +		clocks = <&clk IMX8MN_CLK_SAI3_ROOT>;
>> +		clock-names = "mclk";
>> +	};
>> +};
>> +
>> +&sai3 {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&pinctrl_sai3>;
>> +	assigned-clocks = <&clk IMX8MN_CLK_SAI3>;
>> +	assigned-clock-parents = <&clk IMX8MN_AUDIO_PLL1_OUT>;
>> +	assigned-clock-rates = <24576000>;
>> +	fsl,sai-mclk-direction-output;
>> +	status = "okay";
>>  };
>>  
>>  /* eMMC */
>> @@ -30,6 +94,36 @@ &usdhc1 {
>>  };
>>  
>>  &iomuxc {
>> +	pinctrl_dac_rst: dac_rst {
> 
> Name the node more in the same style as other pinctrl nodes.
> 
> Shawn
> 
>> +		fsl,pins = <
>> +			MX8MN_IOMUXC_GPIO1_IO06_GPIO1_IO6		0x19 /* DAC_RST */
>> +		>;
>> +	};
>> +
>> +	pinctrl_espi2: espi2grp {
>> +		fsl,pins = <
>> +			MX8MN_IOMUXC_ECSPI2_SCLK_ECSPI2_SCLK            0x082
>> +			MX8MN_IOMUXC_ECSPI2_MOSI_ECSPI2_MOSI            0x082
>> +			MX8MN_IOMUXC_ECSPI2_MISO_ECSPI2_MISO            0x082
>> +			MX8MN_IOMUXC_ECSPI2_SS0_ECSPI2_SS0		0x040
>> +		>;
>> +	};
>> +
>> +	pinctrl_i2c2: i2c2grp {
>> +		fsl,pins = <
>> +			MX8MN_IOMUXC_I2C2_SCL_I2C2_SCL			0x400000c3
>> +			MX8MN_IOMUXC_I2C2_SDA_I2C2_SDA			0x400000c3
>> +		>;
>> +	};
>> +
>> +	pinctrl_sai3: sai3grp {
>> +		fsl,pins = <
>> +			MX8MN_IOMUXC_SAI3_TXFS_SAI3_TX_SYNC		0xd6
>> +			MX8MN_IOMUXC_SAI3_TXC_SAI3_TX_BCLK		0xd6
>> +			MX8MN_IOMUXC_SAI3_TXD_SAI3_TX_DATA0		0xd6
>> +		>;
>> +	};
>> +
>>  	pinctrl_usdhc1: usdhc1grp {
>>  		fsl,pins = <
>>  			MX8MN_IOMUXC_SD1_CLK_USDHC1_CLK			0x40000090
>> -- 
>> 2.34.1
>>
Fabio Estevam March 10, 2022, 12:29 p.m. UTC | #2 
Hi Ariel,

On Thu, Feb 10, 2022 at 10:41 AM Ariel D'Alessandro
<ariel.dalessandro@collabora.com> wrote:

> +&i2c2 {
> +       clock-frequency = <400000>;
> +       pinctrl-names = "default";
> +       pinctrl-0 = <&pinctrl_i2c2>;
> +       status = "okay";
> +
> +       codec: tlv320dac3101@18 {
> +               #sound-dai-cells = <0>;
> +               compatible = "ti,tlv320dac3101";
> +               pinctrl-names = "default";
> +               pinctrl-0 = <&pinctrl_dac_rst>;
> +               reg = <0x18>;
> +
> +               ai31xx-micbias-vg = <MICBIAS_AVDDV>;
> +
> +               HPVDD-supply = <&buck4_reg>;
> +               SPRVDD-supply = <&vdd_input>;
> +               SPLVDD-supply = <&vdd_input>;
> +               AVDD-supply = <&buck4_reg>;
> +               IOVDD-supply = <&buck4_reg>;
> +               DVDD-supply = <&buck5_reg>;
> +               reset-gpios = <&gpio1 6 GPIO_ACTIVE_LOW>;
> +
> +               clocks = <&clk IMX8MN_CLK_SAI3_ROOT>;
> +               clock-names = "mclk";

The clocks and clock-names properties are not documented in the codec bindings.

Also, the driver does not use call clk_get() on this mclk clock.

You should drop the clocks and clock-names properties.
Ariel D'Alessandro March 17, 2022, 6:47 p.m. UTC | #3 
Hi Fabio,

On 3/10/22 09:29, Fabio Estevam wrote:
> Hi Ariel,
> 
> On Thu, Feb 10, 2022 at 10:41 AM Ariel D'Alessandro
> <ariel.dalessandro@collabora.com> wrote:
> 
>> +&i2c2 {
>> +       clock-frequency = <400000>;
>> +       pinctrl-names = "default";
>> +       pinctrl-0 = <&pinctrl_i2c2>;
>> +       status = "okay";
>> +
>> +       codec: tlv320dac3101@18 {
>> +               #sound-dai-cells = <0>;
>> +               compatible = "ti,tlv320dac3101";
>> +               pinctrl-names = "default";
>> +               pinctrl-0 = <&pinctrl_dac_rst>;
>> +               reg = <0x18>;
>> +
>> +               ai31xx-micbias-vg = <MICBIAS_AVDDV>;
>> +
>> +               HPVDD-supply = <&buck4_reg>;
>> +               SPRVDD-supply = <&vdd_input>;
>> +               SPLVDD-supply = <&vdd_input>;
>> +               AVDD-supply = <&buck4_reg>;
>> +               IOVDD-supply = <&buck4_reg>;
>> +               DVDD-supply = <&buck5_reg>;
>> +               reset-gpios = <&gpio1 6 GPIO_ACTIVE_LOW>;
>> +
>> +               clocks = <&clk IMX8MN_CLK_SAI3_ROOT>;
>> +               clock-names = "mclk";
> 
> The clocks and clock-names properties are not documented in the codec bindings.
> 
> Also, the driver does not use call clk_get() on this mclk clock.
> 
> You should drop the clocks and clock-names properties.

The sound card driver is calling clk_get() on the codec's clock. See
sound/soc/fsl/fsl-asoc-card.c:

    /* Get the MCLK rate only, and leave it controlled by CODEC drivers */
    if (codec_dev) {
        struct clk *codec_clk = clk_get(codec_dev, NULL);

        if (!IS_ERR(codec_clk)) {
            priv->codec_priv.mclk_freq = clk_get_rate(codec_clk);
            clk_put(codec_clk);
        }
    }

Regards,
Ariel
Ariel D'Alessandro March 25, 2022, 11:19 a.m. UTC | #4 
Hi Fabio,

On 3/17/22 15:47, Ariel D'Alessandro wrote:
> Hi Fabio,
> 
> On 3/10/22 09:29, Fabio Estevam wrote:
>> Hi Ariel,
>>
>> On Thu, Feb 10, 2022 at 10:41 AM Ariel D'Alessandro
>> <ariel.dalessandro@collabora.com> wrote:
>>
>>> +&i2c2 {
>>> +       clock-frequency = <400000>;
>>> +       pinctrl-names = "default";
>>> +       pinctrl-0 = <&pinctrl_i2c2>;
>>> +       status = "okay";
>>> +
>>> +       codec: tlv320dac3101@18 {
>>> +               #sound-dai-cells = <0>;
>>> +               compatible = "ti,tlv320dac3101";
>>> +               pinctrl-names = "default";
>>> +               pinctrl-0 = <&pinctrl_dac_rst>;
>>> +               reg = <0x18>;
>>> +
>>> +               ai31xx-micbias-vg = <MICBIAS_AVDDV>;
>>> +
>>> +               HPVDD-supply = <&buck4_reg>;
>>> +               SPRVDD-supply = <&vdd_input>;
>>> +               SPLVDD-supply = <&vdd_input>;
>>> +               AVDD-supply = <&buck4_reg>;
>>> +               IOVDD-supply = <&buck4_reg>;
>>> +               DVDD-supply = <&buck5_reg>;
>>> +               reset-gpios = <&gpio1 6 GPIO_ACTIVE_LOW>;
>>> +
>>> +               clocks = <&clk IMX8MN_CLK_SAI3_ROOT>;
>>> +               clock-names = "mclk";
>>
>> The clocks and clock-names properties are not documented in the codec bindings.
>>
>> Also, the driver does not use call clk_get() on this mclk clock.
>>
>> You should drop the clocks and clock-names properties.
> 
> The sound card driver is calling clk_get() on the codec's clock. See
> sound/soc/fsl/fsl-asoc-card.c:
> 
>     /* Get the MCLK rate only, and leave it controlled by CODEC drivers */
>     if (codec_dev) {
>         struct clk *codec_clk = clk_get(codec_dev, NULL);
> 
>         if (!IS_ERR(codec_clk)) {
>             priv->codec_priv.mclk_freq = clk_get_rate(codec_clk);
>             clk_put(codec_clk);
>         }
>     }

Do you have any other comments on this one? Otherwise, could you perhaps
ack this patch? See latest version:
  [PATCH v4] arm64: dts: imx8mn-bsh-smm-s2pro: Add tlv320aic31xx audio
card node

Thanks
Ariel
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/freescale/imx8mn-bsh-smm-s2pro.dts b/arch/arm64/boot/dts/freescale/imx8mn-bsh-smm-s2pro.dts
index c6a8ed6745c1..3621354b4a92 100644
--- a/arch/arm64/boot/dts/freescale/imx8mn-bsh-smm-s2pro.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mn-bsh-smm-s2pro.dts
@@ -7,6 +7,7 @@ 
 /dts-v1/;
 
 #include "imx8mn-bsh-smm-s2-common.dtsi"
+#include <dt-bindings/sound/tlv320aic31xx.h>
 
 / {
 	model = "BSH SMM S2 PRO";
@@ -16,6 +17,69 @@  memory@40000000 {
 		device_type = "memory";
 		reg = <0x0 0x40000000 0x0 0x20000000>;
 	};
+
+	sound-tlv320aic31xx {
+		compatible = "fsl,imx-audio-tlv320aic31xx";
+		model = "tlv320aic31xx-hifi";
+		audio-cpu = <&sai3>;
+		audio-codec = <&codec>;
+		audio-asrc = <&easrc>;
+		audio-routing =
+			"Ext Spk", "SPL",
+			"Ext Spk", "SPR";
+		mclk-id = <PLL_CLKIN_BCLK>;
+	};
+
+	vdd_input: vdd_input {
+		compatible = "regulator-fixed";
+		regulator-name = "vdd_input";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+	};
+};
+
+&easrc {
+	fsl,asrc-rate  = <48000>;
+	fsl,asrc-format = <10>;
+	status = "okay";
+};
+
+&i2c2 {
+	clock-frequency = <400000>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_i2c2>;
+	status = "okay";
+
+	codec: tlv320dac3101@18 {
+		#sound-dai-cells = <0>;
+		compatible = "ti,tlv320dac3101";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_dac_rst>;
+		reg = <0x18>;
+
+		ai31xx-micbias-vg = <MICBIAS_AVDDV>;
+
+		HPVDD-supply = <&buck4_reg>;
+		SPRVDD-supply = <&vdd_input>;
+		SPLVDD-supply = <&vdd_input>;
+		AVDD-supply = <&buck4_reg>;
+		IOVDD-supply = <&buck4_reg>;
+		DVDD-supply = <&buck5_reg>;
+		reset-gpios = <&gpio1 6 GPIO_ACTIVE_LOW>;
+
+		clocks = <&clk IMX8MN_CLK_SAI3_ROOT>;
+		clock-names = "mclk";
+	};
+};
+
+&sai3 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_sai3>;
+	assigned-clocks = <&clk IMX8MN_CLK_SAI3>;
+	assigned-clock-parents = <&clk IMX8MN_AUDIO_PLL1_OUT>;
+	assigned-clock-rates = <24576000>;
+	fsl,sai-mclk-direction-output;
+	status = "okay";
 };
 
 /* eMMC */
@@ -30,6 +94,36 @@  &usdhc1 {
 };
 
 &iomuxc {
+	pinctrl_dac_rst: dac_rst {
+		fsl,pins = <
+			MX8MN_IOMUXC_GPIO1_IO06_GPIO1_IO6		0x19 /* DAC_RST */
+		>;
+	};
+
+	pinctrl_espi2: espi2grp {
+		fsl,pins = <
+			MX8MN_IOMUXC_ECSPI2_SCLK_ECSPI2_SCLK            0x082
+			MX8MN_IOMUXC_ECSPI2_MOSI_ECSPI2_MOSI            0x082
+			MX8MN_IOMUXC_ECSPI2_MISO_ECSPI2_MISO            0x082
+			MX8MN_IOMUXC_ECSPI2_SS0_ECSPI2_SS0		0x040
+		>;
+	};
+
+	pinctrl_i2c2: i2c2grp {
+		fsl,pins = <
+			MX8MN_IOMUXC_I2C2_SCL_I2C2_SCL			0x400000c3
+			MX8MN_IOMUXC_I2C2_SDA_I2C2_SDA			0x400000c3
+		>;
+	};
+
+	pinctrl_sai3: sai3grp {
+		fsl,pins = <
+			MX8MN_IOMUXC_SAI3_TXFS_SAI3_TX_SYNC		0xd6
+			MX8MN_IOMUXC_SAI3_TXC_SAI3_TX_BCLK		0xd6
+			MX8MN_IOMUXC_SAI3_TXD_SAI3_TX_DATA0		0xd6
+		>;
+	};
+
 	pinctrl_usdhc1: usdhc1grp {
 		fsl,pins = <
 			MX8MN_IOMUXC_SD1_CLK_USDHC1_CLK			0x40000090