Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <13095aee-2776-8afe-7bf5-fd8f163c6b2a@linaro.org>
Date:   Mon, 18 Jul 2022 16:41:02 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH 2/2] dt-bindings: fpga: add binding doc for ecp5-spi fpga
 mgr
Content-Language: en-US
To:     Ivan Bornyakov <i.bornyakov@metrotek.ru>
Cc:     mdf@kernel.org, hao.wu@intel.com, yilun.xu@intel.com,
        trix@redhat.com, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, linux-fpga@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        system@metrotek.ru
References: <20220714122657.17972-1-i.bornyakov@metrotek.ru>
 <20220714122657.17972-3-i.bornyakov@metrotek.ru>
 <044a9736-a4ec-c250-7755-c80a5bcbe38b@linaro.org>
 <20220715100356.fwjomifweifn6zsr@h-e2.ddg>
 <2a96f734-010d-b42d-8418-715d7c420272@linaro.org>
 <20220718134654.w2b4vmixjijhvqta@h-e2.ddg>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20220718134654.w2b4vmixjijhvqta@h-e2.ddg>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@metrotek.ru>
>>>>> ---
>>>>>  .../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@metrotek.ru>
>>>>> +
>>>>> +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@microchip.com/
> 

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@robh.at.kernel.org/
> 
> 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