Re: [PATCH v5 2/8] dt-bindings: mailbox: Add a sun6i message box binding

[Samuel Holland <samuel@sholland.org>]

Hi,

On 12/16/19 8:04 AM, Maxime Ripard wrote:
> Hi,
> 
> On Sat, Dec 14, 2019 at 10:24:49PM -0600, Samuel Holland wrote:
>> This mailbox hardware is present in Allwinner sun6i, sun8i, sun9i, and
>> sun50i SoCs. Add a device tree binding for it. As it has only been
>> tested on the A83T, A64, H3/H5, and H6 SoCs, only those compatibles are
>> included.
>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>  .../mailbox/allwinner,sun6i-a31-msgbox.yaml   | 78 +++++++++++++++++++
>>  1 file changed, 78 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mailbox/allwinner,sun6i-a31-msgbox.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/mailbox/allwinner,sun6i-a31-msgbox.yaml b/Documentation/devicetree/bindings/mailbox/allwinner,sun6i-a31-msgbox.yaml
>> new file mode 100644
>> index 000000000000..dd746e07acfd
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mailbox/allwinner,sun6i-a31-msgbox.yaml
>> @@ -0,0 +1,78 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/mailbox/allwinner,sun6i-a31-msgbox.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Allwinner sunxi Message Box
>> +
>> +maintainers:
>> +  - Samuel Holland <samuel@sholland.org>
>> +
>> +description: |
>> +  The hardware message box on sun6i, sun8i, sun9i, and sun50i SoCs is a
>> +  two-user mailbox controller containing 8 unidirectional FIFOs. An interrupt
>> +  is raised for received messages, but software must poll to know when a
>> +  transmitted message has been acknowledged by the remote user. Each FIFO can
>> +  hold four 32-bit messages; when a FIFO is full, clients must wait before
>> +  attempting more transmissions.
>> +
>> +  Refer to ./mailbox.txt for generic information about mailbox device-tree
>> +  bindings.
>> +
>> +properties:
>> +  compatible:
>> +     items:
>> +      - enum:
>> +          - allwinner,sun8i-a83t-msgbox
>> +          - allwinner,sun8i-h3-msgbox
>> +          - allwinner,sun50i-a64-msgbox
>> +          - allwinner,sun50i-h6-msgbox
>> +      - const: allwinner,sun6i-a31-msgbox
> 
> This will fail for the A31, since it won't have two compatibles but
> just one.

You asked me earlier to only include compatibles that had been tested, so I did.
This hasn't been tested on the A31, so there's no A31-only compatible.

> You can have something like this if you want to do it with an enum:
> 
> compatible:
>   oneOf:
>     - const: allwinner,sun6i-a31-msgbox
>     - items:
>       - enum:
>         - allwinner,sun8i-a83t-msgbox
>         - allwinner,sun8i-h3-msgbox
>         - allwinner,sun50i-a64-msgbox
>         - allwinner,sun50i-h6-msgbox
>       - const: allwinner,sun6i-a31-msgbox
> 
>> +  reg:
>> +    items:
>> +      - description: MMIO register range
> 
> There's no need for an obvious description like this.
> Just set it to maxItems: 1

Will do for v6.

>> +
>> +  clocks:
>> +    maxItems: 1
>> +    description: bus clock
>> +
>> +  resets:
>> +    maxItems: 1
>> +    description: bus reset
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +    description: controller interrupt
> 
> Ditto, you can drop the description here.

Will do for v6.

>> +  '#mbox-cells':
>> +    const: 1
> 
> However, you should document what the argument is about?

Ok. "Number of cells used to encode a mailbox specifier" should work.

>> +required:
>> +  - compatible
>> +  - reg
>> +  - clocks
>> +  - resets
>> +  - interrupts
>> +  - '#mbox-cells'
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/sun8i-h3-ccu.h>
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    #include <dt-bindings/reset/sun8i-h3-ccu.h>
>> +
>> +    msgbox: mailbox@1c17000 {
>> +            compatible = "allwinner,sun8i-h3-msgbox",
>> +                         "allwinner,sun6i-a31-msgbox";
>> +            reg = <0x01c17000 0x1000>;
>> +            clocks = <&ccu CLK_BUS_MSGBOX>;
>> +            resets = <&ccu RST_BUS_MSGBOX>;
>> +            interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
>> +            #mbox-cells = <1>;
>> +    };
> 
> Look good otherwise, thanks!
> Maxime
> 

Thanks,
Samuel