----- On Mar 6, 2024, at 12:10 AM, Xu Yilun yilun.xu@xxxxxxxxxxxxxxx wrote: > On Mon, Mar 04, 2024 at 09:27:04PM -0500, 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/ >> >> >>> >> >>> 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. > > I agree that deprecated names should not be used for new DT bindings, while > keeping backward compatibility to exsiting ones, unless there is other > DT side concern. > > I'm also good that the driver adapts to the DT binding change. > > What I'm concerned is the driver API: > > int xilinx_core_probe(struct xilinx_fpga_core *core, struct device *dev, > xilinx_write_func write, > - xilinx_write_one_dummy_byte_func write_one_dummy_byte) > + xilinx_write_one_dummy_byte_func write_one_dummy_byte, > + const char *prog_con_id, const char *init_con_id) > > You don't have to make every bus driver input the gpio names. The core > falls back to use old gpio names only for existing devices > (.compatible = "xlnx,fpga-slave-serial"). Then the issue could be > solved? > > Thanks, > Yilun > Ok, thank you for the guidance. Regards, Charles >> >> > >> > 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. >> >> Regards, >> Charles
