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

[Dipen Patel <dipenp@nvidia.com>]

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 am not sure I have understood about ABI break comment. How else one should handle if
there is no related gpio controller property found? I am assuming you are referring to the
below code from the patch 2 (link above) when you said "return -ENODEV".


-		hte_dev->c = gpiochip_find("tegra194-gpio-aon",
-					   tegra_get_gpiochip_from_name);
+		gpio_ctrl = of_parse_phandle(dev->of_node,
+					     "nvidia,gpio-controller", 0);
+		if (!gpio_ctrl) {
+			dev_err(dev, "gpio controller node not found\n");
+			return -ENODEV;
+		}
+
+		hte_dev->c = gpiochip_find(gpio_ctrl,
+					   tegra_get_gpiochip_from_of_node)

> 
> 
> Best regards,
> Krzysztof
>