| Message ID | 20240120135529.899403-4-tim@feathertop.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | dt-bindings: rockchip: Add support for rk809 audio codec | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On 20/01/2024 14:55, Tim Lunn wrote: > Rockchip RK809 shares the same audio codec block as the rk817 mfd, and What rk817 has anything to do with this? > is compatible with the existing rk817_codec driver. This patch Please do not use "This commit/patch/change", but imperative mood. See longer explanation here: https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95 > introduces the clock required for the audio codec. > > This clock provides the I2S master clock for the audio data. The codec > driver finds the clock by the name "mclk" and will fail to register if > this is missing. Clock-names is kept here to keep compatibility with the > exisitng driver ABI and also to be consistent with the rk817 binding. Typo. Also, what consistency with rk817 driver? I really do not understand which problem you are solving here. Best regards, Krzysztof
On 1/22/24 19:16, Krzysztof Kozlowski wrote: > On 20/01/2024 14:55, Tim Lunn wrote: >> Rockchip RK809 shares the same audio codec block as the rk817 mfd, and > What rk817 has anything to do with this? The existing codec driver in linux already is from the rk817 and thus called rk817, however that driver is also compatible with the codec in rk809. > >> is compatible with the existing rk817_codec driver. This patch > Please do not use "This commit/patch/change", but imperative mood. See > longer explanation here: > https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95 I will check this as well. > >> introduces the clock required for the audio codec. >> >> This clock provides the I2S master clock for the audio data. The codec >> driver finds the clock by the name "mclk" and will fail to register if >> this is missing. Clock-names is kept here to keep compatibility with the >> exisitng driver ABI and also to be consistent with the rk817 binding. > Typo. > > Also, what consistency with rk817 driver? The rk817 codec driver that already exists in mainline linux tree. > > I really do not understand which problem you are solving here. I will fix the typo and try to clarify commit message further. > > > > Best regards, > Krzysztof >
On 23/01/2024 05:25, Tim Lunn wrote: > > On 1/22/24 19:16, Krzysztof Kozlowski wrote: >> On 20/01/2024 14:55, Tim Lunn wrote: >>> Rockchip RK809 shares the same audio codec block as the rk817 mfd, and >> What rk817 has anything to do with this? > > The existing codec driver in linux already is from the rk817 and thus > called rk817, however > that driver is also compatible with the codec in rk809. Sure, but how is this any related? Your commit msg says two independent things: 1. Something shares same audio codec block 2. Add clocks I don't see how they are anyhow related to each other, IOW, how from (1) comes (2). Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/mfd/rockchip,rk809.yaml b/Documentation/devicetree/bindings/mfd/rockchip,rk809.yaml index be0616201f52..0174261449ab 100644 --- a/Documentation/devicetree/bindings/mfd/rockchip,rk809.yaml +++ b/Documentation/devicetree/bindings/mfd/rockchip,rk809.yaml @@ -93,6 +93,13 @@ properties: unevaluatedProperties: false unevaluatedProperties: false + clocks: + maxItems: 1 + + clock-names: + items: + - const: mclk + '#sound-dai-cells': const: 0 @@ -135,6 +142,7 @@ additionalProperties: false examples: - | + #include <dt-bindings/clock/px30-cru.h> #include <dt-bindings/pinctrl/rockchip.h> #include <dt-bindings/interrupt-controller/irq.h> #include <dt-bindings/gpio/gpio.h> @@ -149,8 +157,10 @@ examples: clock-output-names = "xin32k", "rk808-clkout2"; interrupt-parent = <&gpio3>; interrupts = <10 IRQ_TYPE_LEVEL_LOW>; + clock-names = "mclk"; + clocks = <&cru SCLK_I2S1_OUT>; pinctrl-names = "default"; - pinctrl-0 = <&pmic_int_l_pin>; + pinctrl-0 = <&pmic_int_l_pin>, <&i2s1_2ch_mclk>; rockchip,system-power-controller; wakeup-source; #sound-dai-cells = <0>;
Rockchip RK809 shares the same audio codec block as the rk817 mfd, and is compatible with the existing rk817_codec driver. This patch introduces the clock required for the audio codec. This clock provides the I2S master clock for the audio data. The codec driver finds the clock by the name "mclk" and will fail to register if this is missing. Clock-names is kept here to keep compatibility with the exisitng driver ABI and also to be consistent with the rk817 binding. This series fixes the following warning from dtb check: pmic@20: '#sound-dai-cells', 'assigned-clock-parents', 'assigned-clocks', 'clock-names', 'clocks', 'codec' do not match any of the regexes: 'pinctrl-[0-9]+' Signed-off-by: Tim Lunn <tim@feathertop.org> --- Changes in v3: - split out clocks into separate patch and group example properties where properties are introduced. - Address review comments - drop clock descriptions that arent required - set maxitems on clocks Changes in v2: - Fix missing include and pinctrl for codec example .../devicetree/bindings/mfd/rockchip,rk809.yaml | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)