Re: [PATCH v4 6/6] arm64: dts: amlogic: Add Amlogic S4 Audio

[Jiebing Chen]

在 2025/3/20 3:31, Krzysztof Kozlowski 写道:
> [ EXTERNAL EMAIL ]
>
> On 19/03/2025 11:38, Jiebing Chen wrote:
> > > >    };
> > > >
> > > >    &pwm_ef {
> > > > diff --git a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi
> > > > index 957577d986c0675a503115e1ccbc4387c2051620..83edafc2646438e3e6b1945fa1c4b327254a4131 100644
> > > > --- a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi
> > > > +++ b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi
> > > > @@ -11,7 +11,11 @@
> > > >    #include <dt-bindings/clock/amlogic,s4-peripherals-clkc.h>
> > > >    #include <dt-bindings/power/meson-s4-power.h>
> > > >    #include <dt-bindings/reset/amlogic,meson-s4-reset.h>
> > > > -
> > > Why?
> > The following files are included that the audio driver depends on
> Do you understand how emails work and patch review? I commented under
> your change, not unrelated code being there already.
Sorry, I will check the rule. Thanks you!
> > it is same as sm1 chip
> >
> > > > +#include <dt-bindings/clock/axg-audio-clkc.h>
> > > > +#include <dt-bindings/reset/amlogic,meson-axg-audio-arb.h>
> > > > +#include <dt-bindings/reset/amlogic,meson-g12a-audio-reset.h>
> > > > +#include <dt-bindings/sound/meson-g12a-toacodec.h>
> > > > +#include <dt-bindings/sound/meson-g12a-tohdmitx.h>
> > > Old style was correct.
> > I didn't understand where you were referring to, I'm guessing that's
> > what it was about
> Read your patch.
>
>
> > the following changes to tdmif_a
> >
> > old:
> >
> > clock-names = "mclk", "sclk", "lrclk";
> >
> > new:
> > clock-names = "sclk", "lrclk","mclk";
> > it fix warning
> How is this related to the patch hunk?
>
> Do you understand how patch format works?
>
>
> ...
>
> > > >         timer {
> > > >                 compatible = "arm,armv8-timer";
> > > >                 interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> > > > @@ -101,7 +135,6 @@ apb4: bus@fe000000 {
> > > >                         #address-cells = <2>;
> > > >                         #size-cells = <2>;
> > > >                         ranges = <0x0 0x0 0x0 0xfe000000 0x0 0x480000>;
> > > > -
> > > Why? What is happening in this patch - why are you changing so many
> > > other pieces?
> You did not respond here, so I assume you will fix this and do intensive
> review before posting next version.
>
> > > >                         clkc_periphs: clock-controller@0 {
> > > >                                 compatible = "amlogic,s4-peripherals-clkc";
> > > >                                 reg = <0x0 0x0 0x0 0x49c>;
> > > > @@ -134,6 +167,17 @@ clkc_pll: clock-controller@8000 {
> > > >                                 #clock-cells = <1>;
> > > >                         };
> > > >
> > > > +                     acodec: audio-controller@1a000 {
> > > > +                             compatible = "amlogic,t9015";
> > > > +                             reg = <0x0 0x1A000 0x0 0x14>;
> > > > +                             #sound-dai-cells = <0>;
> > > > +                             sound-name-prefix = "ACODEC";
> > > > +                             clocks = <&clkc_periphs CLKID_ACODEC>;
> > > > +                             clock-names = "pclk";
> > > > +                             resets = <&reset RESET_ACODEC>;
> > > > +                             AVDD-supply = <&vddio_ao1v8>;
> > > > +                     };
> > > > +
> > > >                         watchdog@2100 {
> > > >                                 compatible = "amlogic,s4-wdt", "amlogic,t7-wdt";
> > > >                                 reg = <0x0 0x2100 0x0 0x10>;
> > > > @@ -850,3 +894,327 @@ emmc: mmc@fe08c000 {
> > > >                 };
> > > >         };
> > > >    };
> > > > +
> > > > +&apb4 {
> > > > +     audio: bus@330000 {
> > > > +             compatible = "simple-bus";
> > > > +             reg = <0x0 0x330000 0x0 0x1000>;
> > > That's not a simple bus in such case.
> > these code base on old dts like sm1/g12a, we didn't easily change any of
> > the relevant properties
> >
> > To be consistent with the previous one
> Still NAK.
>
> You cannot add bugs just to be consistent.
>
>
> Best regards,
> Krzysztof