Re: [v7 4/4] arm64: dts: rockchip: add rk817 codec to Odroid Go

From: Chris Morgan <macromorgan@hotmail.com>
To: Johan Jonker <jbx6244@gmail.com>
Cc: devicetree@vger.kernel.org, alsa-devel@alsa-project.org,
	heiko@sntech.de, lgirdwood@gmail.com,
	pierre-louis.bossart@linux.intel.com, robh+dt@kernel.org,
	tiwai@suse.com, linux-rockchip@lists.infradead.org,
	broonie@kernel.org, Chris Morgan <macroalpha82@gmail.com>,
	lee.jones@linaro.org
Subject: Re: [v7 4/4] arm64: dts: rockchip: add rk817 codec to Odroid Go
Date: Wed, 21 Apr 2021 12:19:32 -0500	[thread overview]
Message-ID: <SN6PR06MB5342DB86A948FAADA4B175C7A5479@SN6PR06MB5342.namprd06.prod.outlook.com> (raw)
In-Reply-To: <c31c3417-0ded-3274-6879-bbd56f26a2aa@gmail.com>

On Tue, Apr 20, 2021 at 10:13:38PM +0200, Johan Jonker wrote:
> On 4/20/21 6:07 PM, Chris Morgan wrote:
> > From: Chris Morgan <macromorgan@hotmail.com>
> > 
> > Add the new rk817 codec driver to the Odroid Go Advance.
> > 
> > Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> > ---
> > Changes in v7:
> >  - Removed ifdef around register definitions for MFD.
> >  - Replaced codec documentation with updates to MFD documentation.
> >  - Reordered elements in example to comply with upstream rules.
> >  - Added binding update back for Odroid Go Advance as requested.
> >  - Submitting patches from gmail now.
> > Changes in v6:
> >  - Included additional project maintainers for correct subsystems.
> >  - Removed unneeded compatible from DT documentation.
> >  - Removed binding update for Odroid Go Advance (will do in seperate series).
> > Changes in v5:
> >  - Move register definitions from rk817_codec.h to main rk808.h register
> >    definitions.
> >  - Add volatile register for codec bits.
> >  - Add default values for codec bits.
> >  - Removed of_compatible from mtd driver (not necessary).
> >  - Switched to using parent regmap instead of private regmap for codec.
> > Changes in v4:
> >  - Created set_pll() call.
> >  - Created user visible gain control in mic.
> >  - Check for return value of clk_prepare_enable().
> >  - Removed duplicate clk_prepare_enable().
> >  - Split DT documentation to separate commit.
> > Changes in v3:
> >  - Use DAPM macros to set audio path.
> >  - Updated devicetree binding (as every rk817 has this codec chip).
> >  - Changed documentation to yaml format.
> >  - Split MFD changes to separate commit.
> > Changes in v2:
> >  - Fixed audio path registers to solve some bugs.
> > 
> >  .../boot/dts/rockchip/rk3326-odroid-go2.dts   | 36 +++++++++++++++++--
> >  1 file changed, 34 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3326-odroid-go2.dts b/arch/arm64/boot/dts/rockchip/rk3326-odroid-go2.dts
> > index 97fb93e1cc00..5356bcf6d99c 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3326-odroid-go2.dts
> > +++ b/arch/arm64/boot/dts/rockchip/rk3326-odroid-go2.dts
> > @@ -161,6 +161,29 @@ blue_led: led-0 {
> >  		};
> >  	};
> >  
> > +	rk817-sound {
> > +		compatible = "simple-audio-card";
> > +		simple-audio-card,format = "i2s";
> 
> > +		simple-audio-card,name = "rockchip,rk817-codec";
> 
> "simple-audio-card,name" is an exception to the Heiko's sort rules.
> Move above all other "simple-audio-card" properties.

Will do.

> 
> ===
> 
> "rockchip,rk817-codec" is too long for the "aplay -l" command.
> Maybe keep it in line with other boards
> 
> ?? "Analog" ??
> 

I can do analog if you want, or maybe just rk817-codec? I notice that several
boards (such as the pinebook pro) do have longish names (21 characters versus
20 for this board). Happy to change it though, your call.

> 
> > +		simple-audio-card,mclk-fs = <256>;
> > +		simple-audio-card,widgets =
> > +			"Microphone", "Mic Jack",
> > +			"Headphone", "Headphones",
> > +			"Speaker", "Speaker";
> > +		simple-audio-card,routing =
> > +			"MICL", "Mic Jack",
> > +			"Headphones", "HPOL",
> > +			"Headphones", "HPOR",
> > +			"Speaker", "SPKO";
> > +		simple-audio-card,hp-det-gpio = <&gpio2 RK_PC6 GPIO_ACTIVE_HIGH>;
> 
> Add empty line between nodes.
> 
> > +		simple-audio-card,cpu {
> > +			sound-dai = <&i2s1_2ch>;
> > +		};
> 
> Add empty line between nodes.
> 
> > +		simple-audio-card,codec {
> > +			sound-dai = <&rk817>;
> > +		};
> > +	};
> > +
> >  	vccsys: vccsys {
> >  		compatible = "regulator-fixed";
> >  		regulator-name = "vcc3v8_sys";
> > @@ -265,11 +288,14 @@ rk817: pmic@20 {
> >  		reg = <0x20>;
> >  		interrupt-parent = <&gpio0>;
> >  		interrupts = <RK_PB2 IRQ_TYPE_LEVEL_LOW>;
> > +		clock-output-names = "rk808-clkout1", "xin32k";
> > +		clock-names = "mclk";
> > +		clocks = <&cru SCLK_I2S1_OUT>;
> >  		pinctrl-names = "default";
> > -		pinctrl-0 = <&pmic_int>;
> > +		pinctrl-0 = <&pmic_int>, <&i2s1_2ch_mclk>;
> >  		wakeup-source;
> >  		#clock-cells = <1>;
> > -		clock-output-names = "rk808-clkout1", "xin32k";
> > +		#sound-dai-cells = <0>;
> >  
> >  		vcc1-supply = <&vccsys>;
> >  		vcc2-supply = <&vccsys>;
> > @@ -428,6 +454,10 @@ regulator-state-mem {
> >  				};
> >  			};
> >  		};
> > +
> > +		rk817_codec: codec {
> 
> > +			mic-in-differential;
> 
> This property name might have to change.
> 

Yep, will do.

> > +		};
> >  	};
> >  };
> >  
> > @@ -439,6 +469,8 @@ &i2c1 {
> >  
> >  /* I2S 1 Channel Used */
> >  &i2s1_2ch {
> 
> > +	resets = <&cru SRST_I2S1>, <&cru SRST_I2S1_H>;
> > +	reset-names = "reset-m", "reset-h";
> 
> Remove.
> "resets" and "reset-names" have no support in mainline.
> See rockchip-i2s.yaml
> 

Did not know that, will remove!

> >  	status = "okay";
> >  };
> >  
> > 
> 