| Message ID | 20240221195058.1281973-3-charles.perry@savoirfairelinux.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fpga: xilinx-selectmap: add new driver | expand |
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@savoirfairelinux.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@savoirfairelinux.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
On Feb 27, 2024, at 3:10 AM, Krzysztof Kozlowski krzysztof.kozlowski@linaro.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@savoirfairelinux.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@savoirfairelinux.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: 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. 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 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. 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. That leaves us with #1 or #3, which is to ask if the added complexity to the driver code and DT binding is worth it for a gain in naming convention. There might also be another solution that I haven't seen. Regards, Charles
On 03/03/2024 18:21, Charles Perry wrote: > On Feb 27, 2024, at 3:10 AM, Krzysztof Kozlowski krzysztof.kozlowski@linaro.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@savoirfairelinux.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@savoirfairelinux.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? Best regards, Krzysztof
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@linaro.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@savoirfairelinux.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@savoirfairelinux.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
On Mar 4, 2024, at 12:31 AM, Krzysztof Kozlowski krzysztof.kozlowski@linaro.org 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@linaro.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@savoirfairelinux.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@savoirfairelinux.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? >> I have a changelog in the cover letter: https://lore.kernel.org/all/20240221195058.1281973-1-charles.perry@savoirfairelinux.com/ >>> >>> 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. Regards, Charles
On 05/03/2024 03:27, Charles Perry wrote: > > > On Mar 4, 2024, at 12:31 AM, Krzysztof Kozlowski krzysztof.kozlowski@linaro.org 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@linaro.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@savoirfairelinux.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@savoirfairelinux.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? >>> > > I have a changelog in the cover letter: > https://lore.kernel.org/all/20240221195058.1281973-1-charles.perry@savoirfairelinux.com/ 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
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@linaro.org 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@linaro.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@savoirfairelinux.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@savoirfairelinux.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? > >> > > I have a changelog in the cover letter: > https://lore.kernel.org/all/20240221195058.1281973-1-charles.perry@savoirfairelinux.com/ > > >>> > >>> 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 > > > > > 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 >
----- On Mar 6, 2024, at 12:10 AM, Xu Yilun yilun.xu@linux.intel.com 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@linaro.org >> 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@linaro.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@savoirfairelinux.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@savoirfairelinux.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? >> >> >> >> I have a changelog in the cover letter: >> https://lore.kernel.org/all/20240221195058.1281973-1-charles.perry@savoirfairelinux.com/ >> >> >>> >> >>> 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
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@savoirfairelinux.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: + description: + config pin (referred to as PROGRAM_B in the manual) + maxItems: 1 + + done-gpios: + description: + config status pin (referred to as DONE in the manual) + maxItems: 1 + + init-b-gpios: + description: + initialization status and configuration error pin + (referred to as INIT_B in the manual) + maxItems: 1 + + csi-gpios: + description: + chip select pin (referred to as CSI_B in the manual) + Optional gpio for if the bus controller does not provide a chip select. + maxItems: 1 + + rdwr-gpios: + description: + read/write select pin (referred to as RDWR_B in the manual) + Optional gpio for if the bus controller does not provide this pin. + maxItems: 1 + +required: + - compatible + - reg + - prog_b-gpios + - done-gpios + - init-b-gpios + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + fpga-mgr@8000000 { + compatible = "xlnx,fpga-xc7s-selectmap"; + reg = <0x8000000 0x4>; + prog_b-gpios = <&gpio5 5 GPIO_ACTIVE_LOW>; + init-b-gpios = <&gpio5 8 GPIO_ACTIVE_LOW>; + done-gpios = <&gpio2 30 GPIO_ACTIVE_HIGH>; + csi-gpios = <&gpio3 19 GPIO_ACTIVE_LOW>; + rdwr-gpios = <&gpio3 10 GPIO_ACTIVE_LOW>; + }; +...
Document the SelectMAP interface of Xilinx 7 series FPGA. Signed-off-by: Charles Perry <charles.perry@savoirfairelinux.com> --- .../bindings/fpga/xlnx,fpga-selectmap.yaml | 86 +++++++++++++++++++ 1 file changed, 86 insertions(+) create mode 100644 Documentation/devicetree/bindings/fpga/xlnx,fpga-selectmap.yaml