| Message ID | 20210420160747.22942-3-macroalpha82@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v7,1/4] mfd: Add Rockchip rk817 audio CODEC support | expand |
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. 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". > +- 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. Ask rob+dt for exact name. Maybe this has to change to "rockchip,mic-in-differential" Update code as well! 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. > + 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? > + #clock-cells = <1>; > + #sound-dai-cells = <0>; Missing in properties. > + > + 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. > + rk817_codec: codec { > + mic-in-differential; See comment above. rockchip,mic-in-differential ?? > + }; > + }; >
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 ?? > > > + }; > > + }; > > >
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". +- codec: The child node for the codec to hold additional properties. +- mic-in-differential: Telling if the microphone uses differential mode. Should + be under the codec child node. - vcc8-supply: The input supply for BOOST - vcc9-supply: The input supply for OTG_SWITCH @@ -275,3 +280,179 @@ Example: }; }; }; + + rk817: pmic@20 { + compatible = "rockchip,rk817"; + 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>, <&i2s1_2ch_mclk>; + wakeup-source; + #clock-cells = <1>; + #sound-dai-cells = <0>; + + 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>; + }; + }; + }; + rk817_codec: codec { + mic-in-differential; + }; + };