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. 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. > >> 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/ > > Best regards, > Krzysztof >
