Hi Liang,
> >> +maintainers:
> >> + - liang.yang at amlogic.com
> >> +
> >> +properties:
> >> + compatible:
> >> + enum:
> >> + - "amlogic,meson-gxl-nfc"
> >> + - "amlogic,meson-axg-nfc"
> >> +
> >> + reg:
> >> + maxItems: 2
> >> +
> >> + '#address-cells':
> >> + const: 1
> >
> > Not sure this property is needed.
> this is for the subnode, such as nand at 0.
Yes but if you refer to nand-controller.yaml you no longer need these.
> >
> >> +
> >> + '#size-cells':
> >> + const: 0
> >
> > Ditto. Plus, this one looks wrong anyway.
> this is for the subnode, such as nand at 0. do you mean s/''/""/?
Sorry, this is not "wrong anyway", my fault. But still, you don't need
this property for the same reason as above.
> >
> >> +
> >> + reg-names:
> >> + items:
> >> + - const: nfc
> >> + - const: emmc
> >
> > Why do you need the emmc register map? Do you really need to perform a
> > register access there?
> yes, we have to access the emmc register map. because the NFC clock comes from SDEMMC_CLOCK register.
But if it's a clock you should get the clock and call
clk_prepare_enable(), you don't need to poke directly in the registers.
Do you?
> >> +examples:
> >> + - |
> >> + #include <dt-bindings/clock/axg-clkc.h>
> >> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> >> + apb {
> >> + #address-cells = <2>;
> >> + #size-cells = <2>;
> >
> > Not sure you need this upper node in the example.
> use the upper node to indicate the "#address-cells" and "#size-cells". if i do not do that, dt_binding_check will report:
> ".....reg:0: [0, 30720, 0, 256] is too long" and
> ".....reg:1: [0, 28672, 0, 2048] is too long".
ok, maybe, I'll let bindings maintainer review that.
> >
> >> + nand-controller at 7800 {
> >> + #address-cells = <1>;
> >> + #size-cells = <0>;
> >> + compatible = "amlogic,meson-axg-nfc";
> >> + reg = <0x0 0x7800 0x0 0x100>,
> >> + <0x0 0x7000 0x0 0x800>;
> >> + reg-names = "nfc", "emmc";
> >> +
> >> + interrupts = <GIC_SPI 34 IRQ_TYPE_EDGE_RISING>;
> >> + clocks = <&clkc CLKID_SD_EMMC_C>,
> >> + <&clkc CLKID_FCLK_DIV2>;
> >> + clock-names = "core", "device";
> >> +
> >> + };
> >> + };
> >> +...
Thanks,
Miquèl