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

[Lucas Stach <l.stach@xxxxxxxxxxxxxx>]

Hi Daniel,

On Do, 2019-11-28 at 11:10 +0000, Daniel Baluta wrote:
> 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>
> 
They are pretty generic, but I don't really see a reason to
differentiate them more. Both sound paths are using a similar codec/hpa
setup, the only difference is the SAI. If I name them after the SAI,
will this really help much?

> 
> > +		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.

I agree that this might seem like thoughtless generic naming from the
outside, but actually it lines up pretty well with the naming used in
the schematics. I'm really reluctant to change the naming here, as any
change will inevitably make it harder to match up the DTS with the
schematics.

Regards,
Lucas

> > +			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.