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
