Re: [PATCH V2 4/6] dt-bindings: timestamp: Add Tegra234 support

[Dipen Patel <dipenp@nvidia.com>]

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@nvidia.com/
>>>> 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@nvidia.com/
> 
> 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@nvidia.com/#3000908 and
affected code/comment at https://patchwork.ozlabs.org/project/linux-gpio/patch/20221103174523.29592-5-dipenp@nvidia.com/#3000908.

Will it help if I send new patch series with detailed commit message?
> 
> 
> Best regards,
> Krzysztof
>