Re: [PATCH v3 2/4] spi: dt-binding: add coreqspi as a fallback for mpfs-qspi

[Krzysztof Kozlowski]

On 05/08/2022 09:34, Conor.Dooley@xxxxxxxxxxxxx wrote:
> On 05/08/2022 07:49, Krzysztof Kozlowski wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 05/08/2022 07:30, Naga Sureshkumar Relli wrote:
>>> diff --git a/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml b/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml
>>> index a47d4923b51b..84d32c1a4d60 100644
>>> --- a/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml
>>> +++ b/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml
>>> @@ -18,10 +18,12 @@ allOf:
>>>
>>>   properties:
>>>     compatible:
>>> -    enum:
>>> -      - microchip,mpfs-spi
>>> -      - microchip,mpfs-qspi
>>> -      - microchip,coreqspi-rtl-v2 # FPGA QSPI
>>> +   oneOf:
>>> +    - items:
>>> +        - const: microchip,mpfs-qspi
>>> +        - const: microchip,coreqspi-rtl-v2
>>
>> Eh, this does not make sense after looking at your driver...
> 
> What is wrong with explicitly binding the driver to both of the
> compatible strings? The "hard" peripheral in the SoC part of the
> FPGA is a superset of version 2 of the coreQSPI IP so the fallback
> used in the binding here makes sense to me. coreQSPI can be
> instantiated in the FPGA fabric and used there, so it needs a
> compatible of its own.
> 
> That brings me back to the original point question, why not
> explicitly bind the driver to both of the compatible strings it
> is known to work for?

There is nothing particularly bad with matching to both of compatibles.
It is valid code. There are however questions/issues with that:

1. It is redundant. I did not look too much at the driver, but none of
the of_device_id entries have some driver data to differentiate,
therefore - for the driver - the devices are identical. If they are
identical and according to binding compatible, use less code and just
one compatible.

2. Otherwise, maybe the devices are not actually fully compatible?
That's the second problem. If one writes binding like that and codes it
in driver differently, it looks like it was not investigated really and
I ask questions...

Best regards,
Krzysztof