linux-kernel - Re: [PATCH v4 2/3] dt-bindings: fpga: xlnx,fpga-selectmap: add DT schema

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d377f0ea-2df2-4d4e-b1bc-8a4ca55eec15@linaro.org>
Date: Mon, 4 Mar 2024 08:31:41 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Charles Perry <charles.perry@...oirfairelinux.com>,
 Rob Herring <robh+dt@...nel.org>, yilun xu <yilun.xu@...el.com>
Cc: mdf <mdf@...nel.org>, Allen VANDIVER <avandiver@...kem-imaje.com>,
 Brian CODY <bcody@...kem-imaje.com>, hao wu <hao.wu@...el.com>,
 Tom Rix <trix@...hat.com>,
 krzysztof kozlowski+dt <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>, Michal Simek <michal.simek@....com>,
 linux-fpga <linux-fpga@...r.kernel.org>,
 devicetree <devicetree@...r.kernel.org>,
 linux-kernel <linux-kernel@...r.kernel.org>,
 linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v4 2/3] dt-bindings: fpga: xlnx,fpga-selectmap: add DT
 schema

On 04/03/2024 08:30, Krzysztof Kozlowski wrote:
> On 03/03/2024 18:21, Charles Perry wrote:
>> On Feb 27, 2024, at 3:10 AM, Krzysztof Kozlowski krzysztof.kozlowski@...aro.org wrote:
>>
>>> On 21/02/2024 20:50, Charles Perry wrote:
>>>> Document the SelectMAP interface of Xilinx 7 series FPGA.
>>>>
>>>> Signed-off-by: Charles Perry <charles.perry@...oirfairelinux.com>
>>>> ---
>>>>  .../bindings/fpga/xlnx,fpga-selectmap.yaml    | 86 +++++++++++++++++++
>>>>  1 file changed, 86 insertions(+)
>>>>  create mode 100644
>>>>  Documentation/devicetree/bindings/fpga/xlnx,fpga-selectmap.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/fpga/xlnx,fpga-selectmap.yaml
>>>> b/Documentation/devicetree/bindings/fpga/xlnx,fpga-selectmap.yaml
>>>> new file mode 100644
>>>> index 0000000000000..08a5e92781657
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/fpga/xlnx,fpga-selectmap.yaml
>>>> @@ -0,0 +1,86 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/fpga/xlnx,fpga-selectmap.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Xilinx SelectMAP FPGA interface
>>>> +
>>>> +maintainers:
>>>> +  - Charles Perry <charles.perry@...oirfairelinux.com>
>>>> +
>>>> +description: |
>>>> +  Xilinx 7 Series FPGAs support a method of loading the bitstream over a
>>>> +  parallel port named the SelectMAP interface in the documentation. Only
>>>> +  the x8 mode is supported where data is loaded at one byte per rising edge of
>>>> +  the clock, with the MSB of each byte presented to the D0 pin.
>>>> +
>>>> +  Datasheets:
>>>> +
>>>> https://www.xilinx.com/support/documentation/user_guides/ug470_7Series_Config.pdf
>>>> +
>>>> +allOf:
>>>> +  - $ref: /schemas/memory-controllers/mc-peripheral-props.yaml#
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    enum:
>>>> +      - xlnx,fpga-xc7s-selectmap
>>>> +      - xlnx,fpga-xc7a-selectmap
>>>> +      - xlnx,fpga-xc7k-selectmap
>>>> +      - xlnx,fpga-xc7v-selectmap
>>>> +
>>>> +  reg:
>>>> +    description:
>>>> +      At least 1 byte of memory mapped IO
>>>> +    maxItems: 1
>>>> +
>>>> +  prog_b-gpios:
>>>
>>> I commented on this and still see underscore. Nothing in commit msg
>>> explains why this should have underscore. Changelog is also vague -
>>> describes that you brought back underscores, instead of explaining why
>>> you did it.
>>>
>>> So the same comments as usual:
>>>
>>> No underscores in names.
>>>
>>> Best regards,
>>> Krzysztof
>>
>> Hello Krzysztof,
>>
>> Yes, I've gone full circle on that issue. Here's what I tried so far:
> 
> And what part of the commit description allows me to understand this?
> 
>>
>>  1) Reuse the same gpio names: Duplicates errors of the past, Krzysztof
>>     doesn't like it.
>>  2) Different gpio names for new driver only: Makes the driver code
>>     overly complicated, Yilun doesn't like it.
> 
> That's a new driver, right? So what is complicated here? You have new
> code and you take prog-b or prog_b?
> 
>>  3) Change gpio names for both drivers, deprecate the old names: Makes
>>     the DT binding and the driver code overly complicated, Rob doesn't
>>     like it.
> 
> I don't think I proposed changing existing bindings.
> 
>>
>> I think that while the driver code shouldn't be the driving force for
>> the DT spec, it can be a good indication that the spec is unpractical to
>> implement.
> 
> What is impractical in implementing this? You just pass either A or B to
> function requesting GPIO. Just choose proper name.
> 
>>
>> In this case, there are two interfaces on a chip that uses the same GPIO
>> protocol, it would only make sense that they use the same names, this
>> discards solution #2.
> 
> I don't understand this. You have devm_gpiod_get() in your new code. Why
> is it difficult to use different name?

And I forgot to emphasize: none of these is mentioned in commit msg, so
for v5 you will get exactly the same complains. And for every other
patch which repeats the same and does not clarify caveats or exceptions.

Best regards,
Krzysztof