Re: [v4 2/3] dt-bindings: fpga: Add Efinix SPI programming bindings

[Krzysztof Kozlowski <krzk@kernel.org>]

On 03/03/2025 11:31, Conor Dooley wrote:
> On Mon, Mar 03, 2025 at 11:10:53AM +0100, Ian Dannapel wrote:
>> Hi Conor, thanks for the quick response.
>>
>> On Fri, Feb 28, 2025 at 7:28 PM Conor Dooley <conor@kernel.org> wrote:
>>>> +description: |
>>>> +  Efinix FPGAs (Trion, Topaz, and Titanium families) support loading bitstreams
>>>> +  through "SPI Passive Mode".
>>>> +  Note 1: Only bus width 1x is supported.
>>>> +  Note 2: Additional pins hogs for bus width configuration must be set
>>>> +  elsewhere, if necessary.
>>>> +  Note 3: Topaz and Titanium support is based on documentation but remains
>>>> +  untested.
>>>
>>> Points 1 and 3 here seem to be driver limitations, and shouldn't really
>>> be present in a document describing the hardware?
>>>
>> Yes, they are driver limitations and probably do not belong here.
>>
>>>> +properties:
>>>> +  compatible:
>>>> +    enum:
>>>> +      - efinix,trion-spi
>>>> +      - efinix,titanium-spi
>>>> +      - efinix,topaz-spi
>>>
>>>> +      - efinix,fpga-spi
>>>
>>> What hardware does this device represent? Other ones are obvious matches
>>> to the families you mention, but what is this one?
> 
>> The proposed compatible is a generic fallback for any Efinix FPGA Series.
> 
> If it is a fallback, your binding should look like:
> compatible:
>   items:
>     - enum:
>         - efinix,trion-spi
>         - efinix,titanium-spi
>         - efinix,topaz-spi
>     - const: efinix,fpga-spi
> 
> |+static const struct of_device_id efinix_spi_of_match[] = {
> |+       { .compatible = "efinix,trion-spi", },
> |+       { .compatible = "efinix,titanium-spi", },
> |+       { .compatible = "efinix,topaz-spi", },
> 
> And these three compatibles can/should be removed from the driver, since
> the fallback is required.
> 
> |+       { .compatible = "efinix,fpga-spi", },
> |+       {}

Yes, except that one of the devices should be the fallback, not generic
"fpga".


Best regards,
Krzysztof