On 10/03/2023 18:19, Dipen Patel wrote: > On 3/10/23 12:45 AM, Krzysztof Kozlowski wrote: >> On 09/03/2023 19:49, Dipen Patel wrote: >>> On 3/8/23 10:16 PM, Krzysztof Kozlowski wrote: >>>> On 08/03/2023 21:09, Dipen Patel wrote: >>>>> On 3/8/23 11:05 AM, Krzysztof Kozlowski wrote: >>>>>> On 08/03/2023 19:45, Dipen Patel wrote: >>>>>>> On 2/16/23 6:17 AM, Krzysztof Kozlowski wrote: >>>>>>>> On 14/02/2023 12:55, Dipen Patel wrote: >>>>>>>>> Added timestamp provider support for the Tegra234 in devicetree >>>>>>>>> bindings. >>>>>>>> >>>>>>>> 1. Your commit does much more. You need to explain it why you drop some >>>>>>>> property. >>>>>>> ACK, will address it next patch >>>>>>>> >>>>>>>> 2. Bindings go before its usage (in the patchset). >>>>>>> Ack... >>>>>>>> >>>>>>>> 3. Please use scripts/get_maintainers.pl to get a list of necessary >>>>>>>> people and lists to CC. It might happen, that command when run on an >>>>>>>> older kernel, gives you outdated entries. Therefore please be sure you >>>>>>>> base your patches on recent Linux kernel. >>>>>>> It is based on recent linux at the time patch series was sent... >>>>>> >>>>>> That's good but then why you do not use scripts/get_maintainers.pl? The >>>>>> hint about recent kernel was just a hint... Just do not invent addresses >>>>>> by yourself and use the tool to get them right. >>>>>> >>>>> I will take a note for the next patch series to add any missing people. The current >>>>> list of people/group is what historically helped review this new timestamp/hte subsystem. >>>>> >>>>>> (...) >>>>>> >>>>>>>>> + properties: >>>>>>>>> + compatible: >>>>>>>>> + contains: >>>>>>>>> + enum: >>>>>>>>> + - nvidia,tegra194-gte-aon >>>>>>>> >>>>>>>> This is an ABI break. Does your driver handle it? >>>>>>> yes, handling patch is part of this patch series. >>>>>> >>>>>> Can you point me to the code which does it? I see "return -ENODEV;", so >>>>>> I think you do not handle ABI break. I could miss something but since >>>>>> you disagree with me, please at least bring some arguments... >>>>> Refer to patch https://patchwork.kernel.org/project/timestamp/patch/20230214115553.10416-3-dipenp@xxxxxxxxxx/ >>>>> which has compatible properties added and also code changes to reflect addition/deletion of some >>>>> properties. >>>> >>>> I referred to the code which breaks the ABI. >>>> >>>>> >>>>> I am not sure I have understood about ABI break comment. How else one should handle if >>>>> there is no related gpio controller property found? >>>> >>>> In a way it does not break existing users? There are many ways to handle >>>> it, but I don't know your code to point you. >>> >>> It is new subsystem and has only one driver which uses it so far. >> >> We do not talk about subsystem, but Tegra SoC, which is not new. Unless >> you meant this is new SoC/DTS? >> >>> This was a decision taken >>> after review comments (By Thierry, also in the mailing list) to add this property (nvidia,gpio-controller) >>> and necessary changes have been made to existing user. From now on, it has to follow this change. >> >> What is "it" which has to follow? There are rules for stable ABI and >> commit msg does not explain why they should not be followed. > > "It" here means hte-tegra194.c HTE provider which is the only one and not being used by any entity > yet. > >> >>> >>>> >>>>> I am assuming you are referring to the >>>>> below code from the patch 2 (link above) when you said "return -ENODEV". >>>> >>>> >>>> Your bindings patch points to ABI break without any >>>> explanation/justification. Then your code #2 patch actually breaks it, >>>> also without any justification. >>> I am going to add explanation/justification in the commit message in the next patch series. But to give >>> you context, discussion happened here https://patchwork.ozlabs.org/project/linux-gpio/patch/20221103174523.29592-3-dipenp@xxxxxxxxxx/ >> >> Either too many messages (and I missed something) or I could not find >> why ABI break is accepted and justified. > > https://patchwork.ozlabs.org/project/linux-gpio/patch/20221103174523.29592-5-dipenp@xxxxxxxxxx/#3000908 and > affected code/comment at https://patchwork.ozlabs.org/project/linux-gpio/patch/20221103174523.29592-5-dipenp@xxxxxxxxxx/#3000908. > > Will it help if I send new patch series with detailed commit message? Yes. If the binding is not used, it's a perfectly valid reason and should be mentioned in commit msg. Best regards, Krzysztof
