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

From: Dipen Patel <dipenp@nvidia.com>
To: Krzysztof Kozlowski <krzk@kernel.org>,
	thierry.reding@gmail.com, jonathanh@nvidia.com,
	linux-kernel@vger.kernel.org, linux-tegra@vger.kernel.org,
	linux-gpio@vger.kernel.org, linus.walleij@linaro.org,
	devicetree@vger.kernel.org, linux-doc@vger.kernel.org,
	robh+dt@kernel.org, timestamp@lists.linux.dev
Subject: Re: [PATCH V2 4/6] dt-bindings: timestamp: Add Tegra234 support
Date: Thu, 9 Mar 2023 10:49:57 -0800	[thread overview]
Message-ID: <c1a78a59-c8ae-81e5-b641-a7cb75062ab3@nvidia.com> (raw)
In-Reply-To: <ab9f7730-d399-0786-67e5-aad57716809e@kernel.org>

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. 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@nvidia.com/

> 
> Best regards,
> Krzysztof
> 