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 3/4] dt-bindings: Add Rockchip rk817 audio CODEC support Date: Wed, 21 Apr 2021 12:16:52 -0500 [thread overview] Message-ID: <SN6PR06MB53421B47D52055B391BF0D49A5479@SN6PR06MB5342.namprd06.prod.outlook.com> (raw) In-Reply-To: <375b3145-70cc-9351-76f5-b9a159dc244f@gmail.com> On Tue, Apr 20, 2021 at 09:56:35PM +0200, Johan Jonker wrote: > Hi Chris, > > Some comments. Have a look if they are useful. > > On 4/20/21 6:07 PM, Chris Morgan wrote: > > From: Chris Morgan <macromorgan@hotmail.com> > > > > Create dt-binding documentation to document rk817 codec. > > > > 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. > > > > .../devicetree/bindings/mfd/rk808.txt | 181 ++++++++++++++++++ > > 1 file changed, 181 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/mfd/rk808.txt b/Documentation/devicetree/bindings/mfd/rk808.txt > > index 04df07f6f793..31eaabd2e179 100644 > > --- a/Documentation/devicetree/bindings/mfd/rk808.txt > > +++ b/Documentation/devicetree/bindings/mfd/rk808.txt > > @@ -63,6 +63,11 @@ Optional RK809 properties: > > - vcc9-supply: The input supply for DCDC_REG5, SWITCH_REG2 > > > > Optional RK817 properties: > > +- clocks: The input clock for the audio codec > > +- clock-names: The clock name for the codec clock. Should be "mclk". > > #sound-dai-cells: > > Needed for the interpretation of sound dais. > Should be 0. Will add, thank you. > > Add empty line > > > +- codec: The child node for the codec to hold additional properties. > > This is a nodename and not a property. Add below "vcc9-supply". > Will change, thank you. > > +- mic-in-differential: Telling if the microphone uses differential mode. Should > > + be under the codec child node. > > This goes in a subnode. Maybe add indent a bit? > "mic-in-differential" is a property specific for Rockchip. > I've moved it after the codec description to hopefully clarify that its position. > Ask rob+dt for exact name. > Maybe this has to change to "rockchip,mic-in-differential" > Update code as well! > Will do. > Add new added property names explicit in your commit message, so rob+dt > can review more easy. > > > > - vcc8-supply: The input supply for BOOST > > - vcc9-supply: The input supply for OTG_SWITCH > > > > @@ -275,3 +280,179 @@ Example: > > }; > > }; > > }; > > Maybe add separator/title? > > > + > > + rk817: pmic@20 { > > + compatible = "rockchip,rk817"; > > + reg = <0x20>; > > > + interrupt-parent = <&gpio0>; > > Missing in properties. > This one was tricky; I honestly am not 100% sure but I think it's a required property. I added it to the required properties as I see it used on every implementation for this chip. > > + 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>, <&i2s1_2ch_mclk>; > > > + wakeup-source; > > Missing in properties. > Is this common for all rkXXX? > I don't think this is common for all, I believe it is optional for some. If I am not mistaken this is because there is a button connected to the PMIC that is used to wake/suspend/poweron/poweroff the device. I put it under optional for now as I'm not sure if every device has this or not. > > + #clock-cells = <1>; > > > + #sound-dai-cells = <0>; > > Missing in properties. Will fix. > > > + > > + vcc1-supply = <&vccsys>; > > + vcc2-supply = <&vccsys>; > > + vcc3-supply = <&vccsys>; > > + vcc4-supply = <&vccsys>; > > + vcc5-supply = <&vccsys>; > > + vcc6-supply = <&vccsys>; > > + vcc7-supply = <&vccsys>; > > + > > + regulators { > > + vdd_logic: DCDC_REG1 { > > + regulator-name = "vdd_logic"; > > + regulator-min-microvolt = <950000>; > > + regulator-max-microvolt = <1150000>; > > + regulator-ramp-delay = <6001>; > > + regulator-always-on; > > + regulator-boot-on; > > + > > + regulator-state-mem { > > + regulator-on-in-suspend; > > + regulator-suspend-microvolt = <950000>; > > + }; > > + }; > > + > > + vdd_arm: DCDC_REG2 { > > + regulator-name = "vdd_arm"; > > + regulator-min-microvolt = <950000>; > > + regulator-max-microvolt = <1350000>; > > + regulator-ramp-delay = <6001>; > > + regulator-always-on; > > + regulator-boot-on; > > + > > + regulator-state-mem { > > + regulator-off-in-suspend; > > + regulator-suspend-microvolt = <950000>; > > + }; > > + }; > > + > > + vcc_ddr: DCDC_REG3 { > > + regulator-name = "vcc_ddr"; > > + regulator-always-on; > > + regulator-boot-on; > > + > > + regulator-state-mem { > > + regulator-on-in-suspend; > > + }; > > + }; > > + > > + vcc_3v3: DCDC_REG4 { > > + regulator-name = "vcc_3v3"; > > + regulator-min-microvolt = <3300000>; > > + regulator-max-microvolt = <3300000>; > > + regulator-always-on; > > + regulator-boot-on; > > + > > + regulator-state-mem { > > + regulator-off-in-suspend; > > + regulator-suspend-microvolt = <3300000>; > > + }; > > + }; > > + > > + vcc_1v8: LDO_REG2 { > > + regulator-name = "vcc_1v8"; > > + regulator-min-microvolt = <1800000>; > > + regulator-max-microvolt = <1800000>; > > + regulator-always-on; > > + regulator-boot-on; > > + > > + regulator-state-mem { > > + regulator-on-in-suspend; > > + regulator-suspend-microvolt = <1800000>; > > + }; > > + }; > > + > > + vdd_1v0: LDO_REG3 { > > + regulator-name = "vdd_1v0"; > > + regulator-min-microvolt = <1000000>; > > + regulator-max-microvolt = <1000000>; > > + regulator-always-on; > > + regulator-boot-on; > > + > > + regulator-state-mem { > > + regulator-on-in-suspend; > > + regulator-suspend-microvolt = <1000000>; > > + }; > > + }; > > + > > + vcc3v3_pmu: LDO_REG4 { > > + regulator-name = "vcc3v3_pmu"; > > + regulator-min-microvolt = <3300000>; > > + regulator-max-microvolt = <3300000>; > > + regulator-always-on; > > + regulator-boot-on; > > + > > + regulator-state-mem { > > + regulator-on-in-suspend; > > + regulator-suspend-microvolt = <3300000>; > > + }; > > + }; > > + > > + vccio_sd: LDO_REG5 { > > + regulator-name = "vccio_sd"; > > + regulator-min-microvolt = <1800000>; > > + regulator-max-microvolt = <3300000>; > > + regulator-always-on; > > + regulator-boot-on; > > + > > + regulator-state-mem { > > + regulator-on-in-suspend; > > + regulator-suspend-microvolt = <3300000>; > > + }; > > + }; > > + > > + vcc_sd: LDO_REG6 { > > + regulator-name = "vcc_sd"; > > + regulator-min-microvolt = <3300000>; > > + regulator-max-microvolt = <3300000>; > > + regulator-boot-on; > > + > > + regulator-state-mem { > > + regulator-on-in-suspend; > > + regulator-suspend-microvolt = <3300000>; > > + }; > > + }; > > + > > + vcc_bl: LDO_REG7 { > > + regulator-name = "vcc_bl"; > > + regulator-min-microvolt = <3300000>; > > + regulator-max-microvolt = <3300000>; > > + > > + regulator-state-mem { > > + regulator-off-in-suspend; > > + regulator-suspend-microvolt = <3300000>; > > + }; > > + }; > > + > > + vcc_lcd: LDO_REG8 { > > + regulator-name = "vcc_lcd"; > > + regulator-min-microvolt = <2800000>; > > + regulator-max-microvolt = <2800000>; > > + > > + regulator-state-mem { > > + regulator-off-in-suspend; > > + regulator-suspend-microvolt = <2800000>; > > + }; > > + }; > > + > > + vcc_cam: LDO_REG9 { > > + regulator-name = "vcc_cam"; > > + regulator-min-microvolt = <3000000>; > > + regulator-max-microvolt = <3000000>; > > + > > + regulator-state-mem { > > + regulator-off-in-suspend; > > + regulator-suspend-microvolt = <3000000>; > > + }; > > + }; > > + }; > > Add empty line, like the rest. > My mistake, will fix. > > + rk817_codec: codec { > > > + mic-in-differential; > > See comment above. > rockchip,mic-in-differential ?? > > > + }; > > + }; > > >
WARNING: multiple messages have this Message-ID (diff)
From: Chris Morgan <macromorgan@hotmail.com> To: Johan Jonker <jbx6244@gmail.com> Cc: Chris Morgan <macroalpha82@gmail.com>, alsa-devel@alsa-project.org, broonie@kernel.org, lgirdwood@gmail.com, pierre-louis.bossart@linux.intel.com, tiwai@suse.com, heiko@sntech.de, lee.jones@linaro.org, robh+dt@kernel.org, perex@perex.cz, devicetree@vger.kernel.org, linux-rockchip@lists.infradead.org Subject: Re: [v7 3/4] dt-bindings: Add Rockchip rk817 audio CODEC support Date: Wed, 21 Apr 2021 12:16:52 -0500 [thread overview] Message-ID: <SN6PR06MB53421B47D52055B391BF0D49A5479@SN6PR06MB5342.namprd06.prod.outlook.com> (raw) In-Reply-To: <375b3145-70cc-9351-76f5-b9a159dc244f@gmail.com> On Tue, Apr 20, 2021 at 09:56:35PM +0200, Johan Jonker wrote: > Hi Chris, > > Some comments. Have a look if they are useful. > > On 4/20/21 6:07 PM, Chris Morgan wrote: > > From: Chris Morgan <macromorgan@hotmail.com> > > > > Create dt-binding documentation to document rk817 codec. > > > > 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. > > > > .../devicetree/bindings/mfd/rk808.txt | 181 ++++++++++++++++++ > > 1 file changed, 181 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/mfd/rk808.txt b/Documentation/devicetree/bindings/mfd/rk808.txt > > index 04df07f6f793..31eaabd2e179 100644 > > --- a/Documentation/devicetree/bindings/mfd/rk808.txt > > +++ b/Documentation/devicetree/bindings/mfd/rk808.txt > > @@ -63,6 +63,11 @@ Optional RK809 properties: > > - vcc9-supply: The input supply for DCDC_REG5, SWITCH_REG2 > > > > Optional RK817 properties: > > +- clocks: The input clock for the audio codec > > +- clock-names: The clock name for the codec clock. Should be "mclk". > > #sound-dai-cells: > > Needed for the interpretation of sound dais. > Should be 0. Will add, thank you. > > Add empty line > > > +- codec: The child node for the codec to hold additional properties. > > This is a nodename and not a property. Add below "vcc9-supply". > Will change, thank you. > > +- mic-in-differential: Telling if the microphone uses differential mode. Should > > + be under the codec child node. > > This goes in a subnode. Maybe add indent a bit? > "mic-in-differential" is a property specific for Rockchip. > I've moved it after the codec description to hopefully clarify that its position. > Ask rob+dt for exact name. > Maybe this has to change to "rockchip,mic-in-differential" > Update code as well! > Will do. > Add new added property names explicit in your commit message, so rob+dt > can review more easy. > > > > - vcc8-supply: The input supply for BOOST > > - vcc9-supply: The input supply for OTG_SWITCH > > > > @@ -275,3 +280,179 @@ Example: > > }; > > }; > > }; > > Maybe add separator/title? > > > + > > + rk817: pmic@20 { > > + compatible = "rockchip,rk817"; > > + reg = <0x20>; > > > + interrupt-parent = <&gpio0>; > > Missing in properties. > This one was tricky; I honestly am not 100% sure but I think it's a required property. I added it to the required properties as I see it used on every implementation for this chip. > > + 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>, <&i2s1_2ch_mclk>; > > > + wakeup-source; > > Missing in properties. > Is this common for all rkXXX? > I don't think this is common for all, I believe it is optional for some. If I am not mistaken this is because there is a button connected to the PMIC that is used to wake/suspend/poweron/poweroff the device. I put it under optional for now as I'm not sure if every device has this or not. > > + #clock-cells = <1>; > > > + #sound-dai-cells = <0>; > > Missing in properties. Will fix. > > > + > > + vcc1-supply = <&vccsys>; > > + vcc2-supply = <&vccsys>; > > + vcc3-supply = <&vccsys>; > > + vcc4-supply = <&vccsys>; > > + vcc5-supply = <&vccsys>; > > + vcc6-supply = <&vccsys>; > > + vcc7-supply = <&vccsys>; > > + > > + regulators { > > + vdd_logic: DCDC_REG1 { > > + regulator-name = "vdd_logic"; > > + regulator-min-microvolt = <950000>; > > + regulator-max-microvolt = <1150000>; > > + regulator-ramp-delay = <6001>; > > + regulator-always-on; > > + regulator-boot-on; > > + > > + regulator-state-mem { > > + regulator-on-in-suspend; > > + regulator-suspend-microvolt = <950000>; > > + }; > > + }; > > + > > + vdd_arm: DCDC_REG2 { > > + regulator-name = "vdd_arm"; > > + regulator-min-microvolt = <950000>; > > + regulator-max-microvolt = <1350000>; > > + regulator-ramp-delay = <6001>; > > + regulator-always-on; > > + regulator-boot-on; > > + > > + regulator-state-mem { > > + regulator-off-in-suspend; > > + regulator-suspend-microvolt = <950000>; > > + }; > > + }; > > + > > + vcc_ddr: DCDC_REG3 { > > + regulator-name = "vcc_ddr"; > > + regulator-always-on; > > + regulator-boot-on; > > + > > + regulator-state-mem { > > + regulator-on-in-suspend; > > + }; > > + }; > > + > > + vcc_3v3: DCDC_REG4 { > > + regulator-name = "vcc_3v3"; > > + regulator-min-microvolt = <3300000>; > > + regulator-max-microvolt = <3300000>; > > + regulator-always-on; > > + regulator-boot-on; > > + > > + regulator-state-mem { > > + regulator-off-in-suspend; > > + regulator-suspend-microvolt = <3300000>; > > + }; > > + }; > > + > > + vcc_1v8: LDO_REG2 { > > + regulator-name = "vcc_1v8"; > > + regulator-min-microvolt = <1800000>; > > + regulator-max-microvolt = <1800000>; > > + regulator-always-on; > > + regulator-boot-on; > > + > > + regulator-state-mem { > > + regulator-on-in-suspend; > > + regulator-suspend-microvolt = <1800000>; > > + }; > > + }; > > + > > + vdd_1v0: LDO_REG3 { > > + regulator-name = "vdd_1v0"; > > + regulator-min-microvolt = <1000000>; > > + regulator-max-microvolt = <1000000>; > > + regulator-always-on; > > + regulator-boot-on; > > + > > + regulator-state-mem { > > + regulator-on-in-suspend; > > + regulator-suspend-microvolt = <1000000>; > > + }; > > + }; > > + > > + vcc3v3_pmu: LDO_REG4 { > > + regulator-name = "vcc3v3_pmu"; > > + regulator-min-microvolt = <3300000>; > > + regulator-max-microvolt = <3300000>; > > + regulator-always-on; > > + regulator-boot-on; > > + > > + regulator-state-mem { > > + regulator-on-in-suspend; > > + regulator-suspend-microvolt = <3300000>; > > + }; > > + }; > > + > > + vccio_sd: LDO_REG5 { > > + regulator-name = "vccio_sd"; > > + regulator-min-microvolt = <1800000>; > > + regulator-max-microvolt = <3300000>; > > + regulator-always-on; > > + regulator-boot-on; > > + > > + regulator-state-mem { > > + regulator-on-in-suspend; > > + regulator-suspend-microvolt = <3300000>; > > + }; > > + }; > > + > > + vcc_sd: LDO_REG6 { > > + regulator-name = "vcc_sd"; > > + regulator-min-microvolt = <3300000>; > > + regulator-max-microvolt = <3300000>; > > + regulator-boot-on; > > + > > + regulator-state-mem { > > + regulator-on-in-suspend; > > + regulator-suspend-microvolt = <3300000>; > > + }; > > + }; > > + > > + vcc_bl: LDO_REG7 { > > + regulator-name = "vcc_bl"; > > + regulator-min-microvolt = <3300000>; > > + regulator-max-microvolt = <3300000>; > > + > > + regulator-state-mem { > > + regulator-off-in-suspend; > > + regulator-suspend-microvolt = <3300000>; > > + }; > > + }; > > + > > + vcc_lcd: LDO_REG8 { > > + regulator-name = "vcc_lcd"; > > + regulator-min-microvolt = <2800000>; > > + regulator-max-microvolt = <2800000>; > > + > > + regulator-state-mem { > > + regulator-off-in-suspend; > > + regulator-suspend-microvolt = <2800000>; > > + }; > > + }; > > + > > + vcc_cam: LDO_REG9 { > > + regulator-name = "vcc_cam"; > > + regulator-min-microvolt = <3000000>; > > + regulator-max-microvolt = <3000000>; > > + > > + regulator-state-mem { > > + regulator-off-in-suspend; > > + regulator-suspend-microvolt = <3000000>; > > + }; > > + }; > > + }; > > Add empty line, like the rest. > My mistake, will fix. > > + rk817_codec: codec { > > > + mic-in-differential; > > See comment above. > rockchip,mic-in-differential ?? > > > + }; > > + }; > > > _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip
next prev parent reply other threads:[~2021-04-21 17:18 UTC|newest] Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-04-20 16:07 [v7 1/4] mfd: Add Rockchip rk817 audio CODEC support Chris Morgan 2021-04-20 16:07 ` Chris Morgan 2021-04-20 16:07 ` Chris Morgan 2021-04-20 16:07 ` [v7 2/4] ASoC: " Chris Morgan 2021-04-20 16:07 ` Chris Morgan 2021-04-20 16:07 ` Chris Morgan 2021-04-20 16:19 ` Mark Brown 2021-04-20 16:19 ` Mark Brown 2021-04-20 16:19 ` Mark Brown 2021-04-20 18:28 ` kernel test robot 2021-04-20 18:28 ` kernel test robot 2021-04-20 20:34 ` kernel test robot 2021-04-20 20:34 ` kernel test robot 2021-04-20 16:07 ` [v7 3/4] dt-bindings: " Chris Morgan 2021-04-20 16:07 ` Chris Morgan 2021-04-20 16:07 ` Chris Morgan 2021-04-20 19:56 ` Johan Jonker 2021-04-20 19:56 ` Johan Jonker 2021-04-20 19:56 ` Johan Jonker 2021-04-21 17:16 ` Chris Morgan [this message] 2021-04-21 17:16 ` Chris Morgan 2021-04-20 16:07 ` [v7 4/4] arm64: dts: rockchip: add rk817 codec to Odroid Go Chris Morgan 2021-04-20 16:07 ` Chris Morgan 2021-04-20 16:07 ` Chris Morgan 2021-04-20 20:13 ` Johan Jonker 2021-04-20 20:13 ` Johan Jonker 2021-04-20 20:13 ` Johan Jonker 2021-04-21 17:19 ` Chris Morgan 2021-04-21 17:19 ` Chris Morgan
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=SN6PR06MB53421B47D52055B391BF0D49A5479@SN6PR06MB5342.namprd06.prod.outlook.com \ --to=macromorgan@hotmail.com \ --cc=alsa-devel@alsa-project.org \ --cc=broonie@kernel.org \ --cc=devicetree@vger.kernel.org \ --cc=heiko@sntech.de \ --cc=jbx6244@gmail.com \ --cc=lee.jones@linaro.org \ --cc=lgirdwood@gmail.com \ --cc=linux-rockchip@lists.infradead.org \ --cc=macroalpha82@gmail.com \ --cc=pierre-louis.bossart@linux.intel.com \ --cc=robh+dt@kernel.org \ --cc=tiwai@suse.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.