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

From: Charles Perry <charles.perry@savoirfairelinux.com>
To: Xu Yilun <yilun.xu@linux.intel.com>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
	 yilun xu <yilun.xu@intel.com>, Rob Herring <robh+dt@kernel.org>,
	 mdf <mdf@kernel.org>,
	Allen VANDIVER <avandiver@markem-imaje.com>,
	 Brian CODY <bcody@markem-imaje.com>, hao wu <hao.wu@intel.com>,
	 Tom Rix <trix@redhat.com>,
	 krzysztof kozlowski+dt <krzysztof.kozlowski+dt@linaro.org>,
	 Conor Dooley <conor+dt@kernel.org>,
	 Michal Simek <michal.simek@amd.com>,
	 linux-fpga <linux-fpga@vger.kernel.org>,
	 devicetree <devicetree@vger.kernel.org>,
	 linux-kernel <linux-kernel@vger.kernel.org>,
	 linux-arm-kernel <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v4 2/3] dt-bindings: fpga: xlnx,fpga-selectmap: add DT schema
Date: Wed, 6 Mar 2024 09:29:21 -0500 (EST)	[thread overview]
Message-ID: <1394848790.1570451.1709735361852.JavaMail.zimbra@savoirfairelinux.com> (raw)
In-Reply-To: <ZegW2QpgO0Kk1Iip@yilunxu-OptiPlex-7050>

----- 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