Re: [PATCH 2/2] dt-bindings: fpga: add binding doc for ecp5-spi fpga mgr

[Krzysztof Kozlowski]

On 18/07/2022 15:46, Ivan Bornyakov wrote:
> On Mon, Jul 18, 2022 at 03:06:18PM +0200, Krzysztof Kozlowski wrote:
>> On 15/07/2022 12:03, Ivan Bornyakov wrote:
>>> On Fri, Jul 15, 2022 at 11:33:54AM +0200, Krzysztof Kozlowski wrote:
>>>> On 14/07/2022 14:26, Ivan Bornyakov wrote:
>>>>> Add Device Tree Binding doc for Lattice ECP5 FPGA manager using slave
>>>>> SPI to load .bit formatted uncompressed bitstream image.
>>>>>
>>>>> Signed-off-by: Ivan Bornyakov <i.bornyakov@xxxxxxxxxxx>
>>>>> ---
>>>>>  .../fpga/lattice,ecp5-spi-fpga-mgr.yaml       | 71 +++++++++++++++++++
>>>>>  1 file changed, 71 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/fpga/lattice,ecp5-spi-fpga-mgr.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/fpga/lattice,ecp5-spi-fpga-mgr.yaml b/Documentation/devicetree/bindings/fpga/lattice,ecp5-spi-fpga-mgr.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..79868f9c84e2
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/fpga/lattice,ecp5-spi-fpga-mgr.yaml
>>>>> @@ -0,0 +1,71 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/fpga/lattice,ecp5-spi-fpga-mgr.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Lattice ECP5 FPGA manager.
>>>>> +
>>>>> +maintainers:
>>>>> +  - Ivan Bornyakov <i.bornyakov@xxxxxxxxxxx>
>>>>> +
>>>>> +description:
>>>>> +  Device Tree Bindings for Lattice ECP5 FPGA Manager using slave SPI to
>>>>> +  load the uncompressed bitstream in .bit format.
>>>>
>>>> s/Device Tree Bindings for//
>>>>
>>>> Instead describe the hardware you are adding bindings for. What is a
>>>> "Manager"? It is so broad and unspecific... It is some dedicated
>>>> hardware to communicate with FPGA or you just called this regular FPGA
>>>> interface exposed to the CPU/SoC?
>>>>
>>>
>>> "FPGA Manager" is a kernel subsystem that exports a set of functions for
>>> programming an FPGA with a bitstream image.
>>> See Documentation/driver-api/fpga/fpga-mgr.rst
>>
>> This is what you want to include in the bindings document? How is it
>> related to bindings? We do not talk about driver API but we talk about
>> hardware. Bindings are for the hardware.
>>
> 
> I've send out v3 not too long ago. If you found the wording there not
> good enough, could you look through
> Documentation/devicetree/bindings/fpga/ and point me to a proper example?
> 
>>>
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    enum:
>>>>> +      - lattice,ecp5-spi-fpga-mgr
>>>>
>>>> Do not encode interface name in compatible so no "spi".
>>>>
>>>
>>> Recently when I submitted FPGA manager for Microchip PolarFire, I was
>>> asked the opposite, to add "spi" in compatible. The reason was that FPGA
>>> can be programmed through other interfaces as well.
>>
>> I don't see such comment from Rob (DT maintainer):
>> https://lore.kernel.org/all/?q=%22dt-bindings%3A+fpga%3A+add+binding+doc+for+microchip-spi+fpga+mgr%22
>>
>> Can you point me to it?
>>
> 
> Yeah, it was not Rob but other developer:
> https://lore.kernel.org/all/79328410-e56f-7c8a-9d17-de9bfdb98f51@xxxxxxxxxxxxx/
> 

The type of bus should not be included in the compatible. It's obvious
by looking at the parent, so Conor's comment was not helpful, IMO.

> And at that point I had not even written the bindings doc, so neither
> you nor Rob weren't in the Cc.
> 
> But eventually Rob reviewed DT bindings doc for PolarFire with
> compatible string to be "microchip,mpf-spi-fpga-mgr"
> https://lore.kernel.org/all/YkORrgC1FdzaKCMW@xxxxxxxxxxxxxxxxxx/
> 
> So I thought it was OK.

If spi was at the end, probably would be easier to spot thus would
trigger a comment.


Best regards,
Krzysztof