Re: [PATCH v3 3/3] dt-bindings: mmc: xenon: add marvell,sdhci-xenon compatible

[Krzysztof Kozlowski <krzk@kernel.org>]

On 25/03/2022 01:07, Chris Packham wrote:
> The armada-37xx SoC dtsi includes this as a compatible string. Add it to
> the dt-binding.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> 
> Notes:
>     Changes in v3:
>     - new
> 
>  .../devicetree/bindings/mmc/marvell,xenon-sdhci.yaml          | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/marvell,xenon-sdhci.yaml b/Documentation/devicetree/bindings/mmc/marvell,xenon-sdhci.yaml
> index 326ac3fa36b5..776bed5046fa 100644
> --- a/Documentation/devicetree/bindings/mmc/marvell,xenon-sdhci.yaml
> +++ b/Documentation/devicetree/bindings/mmc/marvell,xenon-sdhci.yaml
> @@ -31,6 +31,10 @@ properties:
>            - const: marvell,armada-ap807-sdhci
>            - const: marvell,armada-ap806-sdhci
>  
> +      - items:
> +          - const: marvell,armada-3700-sdhci
> +          - const: marvell,sdhci-xenon

Do you know of any usages of marvell,armada-3700-sdhci alone (without
sdhci-xenon)? If not, it should be removed from the enum before (the one
added in your patch #2). It does not look correct to have it both as
standalone compatible and one compatible with sdhci-xenon. Either it is
compatible with sdhci-xenon or not. :)

I suggested before to make this change here as separate patch, but I
think I missed the fact that it is simple correction of
armada-3700-sdhci compatible. Such simple corrections can be done within
same patch as conversion, with explanation in commit msg (which was
missing in your v1).

To avoid unnecessary patch changes you could squash it with v1 and
include this in the commit msg (actually extend it to say that you are
correcting the 3700-sdhci compatible), or create patch #2 that way:

+    oneOf:
+      - enum:
+          - marvell,armada-cp110-sdhci
+          - marvell,armada-ap806-sdhci
+      - items:
+          - const: marvell,armada-ap807-sdhci
+          - const: marvell,armada-ap806-sdhci
+      - items:
+          - marvell,armada-3700-sdhci

so now you will only add xenon here. But then it is so small patch that
with explanation we usually include it in conversion.

Best regards,
Krzysztof