Message-ID: <c1a78a59-c8ae-81e5-b641-a7cb75062ab3@nvidia.com>
Date:   Thu, 9 Mar 2023 10:49:57 -0800
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.8.0
Subject: Re: [PATCH V2 4/6] dt-bindings: timestamp: Add Tegra234 support
Content-Language: en-US
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
References: <20230214115553.10416-1-dipenp@nvidia.com>
 <20230214115553.10416-5-dipenp@nvidia.com>
 <3c0ad963-ce69-bd5b-20cd-888e5fbdecaf@kernel.org>
 <7a8027c9-dc73-3684-c5f2-3071f315b3cd@nvidia.com>
 <a5e897e5-4cb9-d50f-47a8-ffb8bd8774cb@kernel.org>
 <18f9a6ca-a61b-4cbb-b729-1fdb6d48651a@nvidia.com>
 <ab9f7730-d399-0786-67e5-aad57716809e@kernel.org>
From:   Dipen Patel <dipenp@nvidia.com>
In-Reply-To: <ab9f7730-d399-0786-67e5-aad57716809e@kernel.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?VWZKVGR3Mm1iM1BzMHZubDlrZXFQNnVkNndUUjl2Vk1IMW1DU3ZRMlJ6OEN1?=
 =?utf-8?B?QjdXOUVHNnFSMFZ4cG82K3BWUGR5ZXJ6amRQd1UzN2xqbXg4QXcxOXBYLzAv?=
 =?utf-8?B?b0dxbndOZ00ySmZkUkxKSkpPU3hrK3pUdmRLdFhrSVJ5MWJmVXd0bHd1SEZ4?=
 =?utf-8?B?REE0bWhsbjBUSUFOcExpelN0TndCQlJOOWNhYm9TRDhxeEZKMFdMMSt5dEp3?=
 =?utf-8?B?d2l1b2Z3L3dDTFhNc0Jma3ZFY2F6dTUrWStEcjFoR2lTSmFadG5VengzMjFS?=
 =?utf-8?B?UkNGYlFNcUEwWE5WSHZRZEZQbUVpdTB6cWdNMWVnc3ROZFN4N3U1UXkrMnZl?=
 =?utf-8?B?YUlsMmsvamVsMTZ1V2ZZMWxpeHVuYW9qL1JWNkdnbkhzRW9paitvckZPamRE?=
 =?utf-8?B?N0xBTkNva0tTMUNkRktXeFA1WSt5bjByN013NUlzOE9uNGRKS0xWZHN2S0w1?=
 =?utf-8?B?UThZT3ZRK1NoTUM4dkd1ZTNEMFk1cklNeG84UWVTUlNsME13WWNOMzJ2UjBa?=
 =?utf-8?B?d1FuOU5pTXc5RWx3Z0hmb3dVZE1nQVhybjhmOENRbG50UmZ6YkIwRnhVQzVP?=
 =?utf-8?B?ZzNNcU9JZHBaeHh3WUxveTg2Qm9oZGFDV29VRVF4MU9wT1d2MjZyWXJzb3RI?=
 =?utf-8?B?Q1pXQUVveHNUU2s3NFpINjg1TjNMdXY0UWJ3M0tKTC81WTJCay8zeEpYa0JN?=
 =?utf-8?B?NTJCRmZIak9YMVFjZDA3YVFuSDJTckQwS3BLajlkcFdHYVJCN2o0eTVSb2NG?=
 =?utf-8?B?eTBmaVJPbmQ2MzYxa291TzNWeUFLTlg5R3hOSjhOcXFlV2sxMXdCU2UvY0tN?=
 =?utf-8?B?dGxoOGU4YnhLdDNIMFlLSmMxV1NwTXZOdnM3WFRvKy8zZjhuQlphdDczZHVB?=
 =?utf-8?B?Y3YxWW5mdXlzUS9KRnIzNHRQSUJVQldvbHFDY2E5dFBFWU5PSTl0eHdRSnlw?=
 =?utf-8?B?RjQ5Q0VqWXNEb0h6VStaVGR1T2NQdURnTWtuUG5qdnpoTW5mZ01zVFh5RnBp?=
 =?utf-8?B?RXlMMHpUa0ljSmdFZWZkTVFsZWhySTlpNllBa3BUdW1jWVV5S0tKTG5uM1RT?=
 =?utf-8?B?akYvYVozZXdHSGhNOExYNkkraFo0RDR3MEN6UUNnbE43bDlRREtLR2JMNHNM?=
 =?utf-8?B?eVdkdFM4em5QcWRnWWsyNjg2Ykp1QUs5aVlkdkY3cjdmMzZ1OXA0RnM4cXdq?=
 =?utf-8?B?bFZSY3d6U29IRXFXNWY4MWppWXBFcFpaZ0NCQUw3ZWhLQWlkNlZ2ZXBidDds?=
 =?utf-8?B?eUxqRHo5OFdBaWxDVC82Nkx0UFpRQVVKWk5KdWtjTXBJL0w4Um1zRnRiYkxH?=
 =?utf-8?B?TDhNb0RrbStNbG9CblB5bWJ6dm13UjVTZlNpcEZpeXRMdU9yOEpGTWtBajNS?=
 =?utf-8?B?ckV1Vk9leXVTQzFzQ2VqWlVTUWRLR296dFJRNmNJSk5Sa2o5azVTUEIwcTlo?=
 =?utf-8?B?Z09GQXRKeUJFVmJGcXdJNEhwUzNoeGhuTGwvRS8wWnBWd1R1aWMzSmNsNUk3?=
 =?utf-8?B?Q0NxK2RhcUtNWDBmQSswQXRJUmoreXlaWEYxYXA2Zkd3dXdMbkpyU3duN3M3?=
 =?utf-8?B?TWdnWHc5T2FlODh0ZVlnM1ZWWHM1RFA5ajJHZnVpZHo2K29xa3BreWZMRzcy?=
 =?utf-8?B?OGNMemtnelZSNW96NVVJK2pzYVdyWGRvOVhWWExEdDhjZUMxeDNGbWtJekxM?=
 =?utf-8?B?cjFHcFlTOXROWGVlM2Vnd2FZaXhtUW9TOW5wVDg1d2VkWkN3anUraCt4d1Jv?=
 =?utf-8?B?VGIxQjFpbDRlaVlhNkhkL2JIUXY1ZnVOalNUVXR4MWZ5NXIrOWNNRUl1cEF0?=
 =?utf-8?B?U1dXbUsrS0NxQlV0dWFsU3NnZ2xpdmVjZWZxNmYrMTBIWnN5bHFYbHl0TTZL?=
 =?utf-8?B?dnhnbXl2TXZydjdDS1ZnMGY1YnBmT2xOY2tTTFJQOU9jQS9NbEc4Ti82V2lR?=
 =?utf-8?B?TzVMVnhmeXhVRnp2UGI3K0U4dkNHdGNlZ2FzcklNMEhiYmhoL0t6TkZHNjcr?=
 =?utf-8?B?dGJ1WFZSQ0huWnFHU2FyMEZWN1ZGZ0J0bkNEZUo0MnlFdW8xSStBQ0lReVUw?=
 =?utf-8?B?eVdWSUJabGE2My9uWktReXFjZExRRUR2Q0JsMGQ2dXpzS3h6UEtzRXF0b2pR?=
 =?utf-8?Q?YAOXRA2jw/QKzmNq2t+cKh16d?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9f26c445-68f1-4588-0541-08db20cf1658
X-MS-Exchange-CrossTenant-AuthSource: IA1PR12MB6604.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Mar 2023 18:50:01.0430
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: kUPdx02WsFfQ+nLTU50zjGBpur7L2nm6HAhaPsx5S7pO5o0Yb4Y6lnOO/S1RuMDwPSreXe9GanlzfFT+XE/d2A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR12MB5171
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.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
>