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

[Krzysztof Kozlowski]

On 05/03/2024 03:27, Charles Perry wrote:
> 
> 
> On Mar 4, 2024, at 12:31 AM, Krzysztof Kozlowski krzysztof.kozlowski@xxxxxxxxxx wrote:
> 
>> 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@xxxxxxxxxx
>>>> 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@xxxxxxxxxxxxxxxxxxxx>
>>>>>> ---
>>>>>>  .../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@xxxxxxxxxxxxxxxxxxxx>
>>>>>> +
>>>>>> +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?
>>>
> 
> I have a changelog in the cover letter:
> https://lore.kernel.org/all/20240221195058.1281973-1-charles.perry@xxxxxxxxxxxxxxxxxxxx/

Commit should stand on its own. Many reviewers do not read cover letter,
I mostly ignore it and only check the changelog part.

> 
>>>>
>>>>  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.
>>>
> 
> It's not complicated but it requires more code than if "prog_b" had been
> used. 
>  
>>>>
>>>> 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?
> 
> Yilun asked to avoid changing the names between the two drivers.
> First comment in this mail:
> https://lore.kernel.org/all/Zb9GkY6cMtR+4xOX@yilunxu-OptiPlex-7050/
> 
> Yilun, let me know if this is something you'd accept as this is a concern
> for the device tree maintainers.
> 
>>
>> 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
> 
> Should I keep my changelog in the individual commits? I thought the norm
> was to put this the cover letter.

I speak about the commit msg, not changelog. According to this commit,
there is absolutely no reason to keep prog_b name. This goes to git
stays there and later someone will check the history and will have the
same concerns: why the hell new binding was allowed to use underscore?
Why DT maintainers gave here exception? If they gave here exception, I
deserve exception as well. Such arguments are brought at least once per
month.

Your commit must stand on its own.

Best regards,
Krzysztof