Re: [PATCH 2/2] arm64: dts: zii-ultra: add sound support

[Daniel Baluta <daniel.baluta@xxxxxxx>]

Few comments inline about the naming:

On Wed, 2019-11-27 at 19:21 +0100, Lucas Stach wrote:
> This adds all the necessary device nodes to get audio support on both
> the
> RMB3 and Zest boards.
> 
> Signed-off-by: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
> ---
>  .../dts/freescale/imx8mq-zii-ultra-rmb3.dts   | 87 ++++++++++++++++
>  .../dts/freescale/imx8mq-zii-ultra-zest.dts   | 30 ++++++
>  .../boot/dts/freescale/imx8mq-zii-ultra.dtsi  | 98
> +++++++++++++++++++
>  3 files changed, 215 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mq-zii-ultra-rmb3.dts
> b/arch/arm64/boot/dts/freescale/imx8mq-zii-ultra-rmb3.dts
> index 6b3581366d67..d5e06885c683 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mq-zii-ultra-rmb3.dts
> +++ b/arch/arm64/boot/dts/freescale/imx8mq-zii-ultra-rmb3.dts
> @@ -10,6 +10,56 @@
>  / {
>  	model = "ZII Ultra RMB3 Board";
>  	compatible = "zii,imx8mq-ultra-rmb3", "zii,imx8mq-ultra",
> "fsl,imx8mq";
> +
> +	sound1 {

I find sound1 / sound2 pretty generic. Perhaps we can go with something
like sound-<codec_name>


> +		compatible = "simple-audio-card";
> +		simple-audio-card,name = "RMB3 audio front";
> +		simple-audio-card,format = "i2s";
> +		simple-audio-card,bitclock-master = <&sound1_codec>;
> +		simple-audio-card,frame-master = <&sound1_codec>;
> +		simple-audio-card,widgets =
> +			"Headphone", "Headphone Jack Front";
> +		simple-audio-card,routing =
> +			"Headphone Jack Front", "HPA1 HPLEFT",
> +			"Headphone Jack Front", "HPA1 HPRIGHT",
> +			"HPA1 LEFTIN", "HPL",
> +			"HPA1 RIGHTIN", "HPR";
> +		simple-audio-card,aux-devs = <&hpa1>;
> +
> +		sound1_cpu: simple-audio-card,cpu {
> +			sound-dai = <&sai2>;
> +		};
> +
> +		sound1_codec: simple-audio-card,codec {
> +			sound-dai = <&codec1>;

Codec1 here is too generic. Perpahs we can add real code name.

> +			clocks = <&cs2000>;
> +		};
> +	};
> +
> +	sound2 {
> +		compatible = "simple-audio-card";
> +		simple-audio-card,name = "RMB3 audio back";
> +		simple-audio-card,format = "i2s";
> +		simple-audio-card,bitclock-master = <&sound2_codec>;
> +		simple-audio-card,frame-master = <&sound2_codec>;
> +		simple-audio-card,widgets =
> +			"Headphone", "Headphone Jack Back";
> +		simple-audio-card,routing =
> +			"Headphone Jack Back", "HPA2 HPLEFT",
> +			"Headphone Jack Back", "HPA2 HPRIGHT",
> +			"HPA2 LEFTIN", "HPL",
> +			"HPA2 RIGHTIN", "HPR";
> +		simple-audio-card,aux-devs = <&hpa2>;
> +
> +		sound2_cpu: simple-audio-card,cpu {
> +			sound-dai = <&sai3>;
> +		};
> +
> +		sound2_codec: simple-audio-card,codec {
> +			sound-dai = <&codec2>;
Ditto.

> +			clocks = <&cs2000>;
> +		};
> +	};
>  };
>  
>  &ecspi1 {
> @@ -27,6 +77,23 @@
>  	};
>  };
>  
> +&i2c1 {
> +	codec2: codec@18 {

Here we might go with something like: 

tlv320dac3100: codec@18


> +		compatible = "ti,tlv320dac3100";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_codec2>;
> +		reg = <0x18>;
> +		#sound-dai-cells = <0>;
> +		HPVDD-supply = <&reg_3p3v>;
> +		SPRVDD-supply = <&reg_3p3v>;
> +		SPLVDD-supply = <&reg_3p3v>;
> +		AVDD-supply = <&reg_3p3v>;
> +		IOVDD-supply = <&reg_3p3v>;
> +		DVDD-supply = <&vgen4_reg>;
> +		gpio-reset = <&gpio3 4 GPIO_ACTIVE_HIGH>;
> +	};
> +};
> 

This is just a thought, naming is hard. But it should help with
readability.