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

[<Conor.Dooley@xxxxxxxxxxxxx>]

On 05/08/2022 09:12, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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.

Right. Then the binding is correct and the driver should only bind
against "microchip,coreqspi-rtl-v2".

> 
> 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