Re: [PATCH v2 13/13] ARM: dts: r9a06g032: Describe the QSPI controller

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Geert,

>> +               qspi0: spi@40005000 {
>> +                       compatible = "renesas,r9a06g032-qspi", "renesas,rzn1-qspi", "cdns,qspi-nor";
>> +                       reg = <0x40005000 0x1000>, <0x10000000 0x10000000>;
>> +                       interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
>> +                       clocks = <&sysctrl R9A06G032_CLK_QSPI0>, <&sysctrl R9A06G032_HCLK_QSPI0>,
>> +                                <&sysctrl R9A06G032_HCLK_QSPI0>;
>> +                       clock-names = "ref", "ahb", "apb";
>> +                       #address-cells = <1>;
>> +                       #size-cells = <0>;
>> +                       cdns,fifo-width = <4>;
>
> <4> is the default, right?

It is the default in the bindings indeed, however the driver does not
imply that default and errors out if the property is missing. The
property is also marked required in the bindings, which is kind of
incorrect I guess. Also, all DTS explicitly set this value to 4.

However looking into the RM I found "Transmit and receive FIFOs are 16
bytes". I haven't tested that, I will.

>> +                       cdns,trigger-address = <0>;
>
> Where in the RZ/N1 docs can I find if these two properties are
> correct?

This property is mandatory. Maybe I could just discard it for my
compatible, because it is only relevant for indirect modes, which are
unsupported.

>> +                       status = "disabled";
>> +               };
>> +
>>                 rtc0: rtc@40006000 {
>>                         compatible = "renesas,r9a06g032-rtc", "renesas,rzn1-rtc";
>>                         reg = <0x40006000 0x1000>;
>
> The rest LGTM, ignoring my comments on the bindings:
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks for the review, but I guess if I end up changing the DTS snippet
I might drop it. Or would you like me to keep it anyway?

Thanks,
Miquèl