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

[Krzysztof Kozlowski]

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.

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

Best regards,
Krzysztof