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

Samuel Holland
2019-12-16T19:45:14Z

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