Re: [PATCH V4 04/10] dt-bindings: timestamp: Add nvidia,gpio-controller

[Dipen Patel]

On 3/25/23 4:09 AM, Krzysztof Kozlowski wrote:
> On 24/03/2023 19:51, Dipen Patel wrote:
>> On 3/24/23 10:13 AM, Rob Herring wrote:
>>> On Wed, Mar 22, 2023 at 06:29:23PM -0700, Dipen Patel wrote:
>>>> Introducing nvidia,gpio-controller property from Tegra234 SoCs onwards.
>>>> This is done to help below case.
>>>>
>>>> Without this property code would look like:
>>>> if (of_device_is_compatible(dev->of_node, "nvidia,tegra194-gte-aon"))
>>>> 	hte_dev->c = gpiochip_find("tegra194-gpio-aon",
>>>> 				   tegra_get_gpiochip_from_name);
>>>> else if (of_device_is_compatible(dev->of_node, "nvidia,tegra234-gte-aon"))
>>>> 	hte_dev->c = gpiochip_find("tegra234-gpio-aon",
>>>> 				   tegra_get_gpiochip_from_name);
>>>> else
>>>> 	return -ENODEV;
>>>
>>> Or you just put the name in match data.
>>
>> Not sure I have understood this comment, but "name" the first argument is
>> already there to supply to callback to match data. Also, this if else is
>> needed to know which "name" to provide.
> 
> The point is that of_device_is_compatible() do not really scale and make
> code more difficult to read. Your variant-customization should in
> general entirely come from match/driver data.

Perhaps I should not have mentioned driver related details here about how
this property will help, that detail will go in driver patch. In the next
patch series I will remove this commit and just focus on what this property
is.
> 
> 
>>>
>>>>
>>>> This means for every future addition of the compatible string, if else
>>>> condition statements have to be expanded.
>>>>
>>>> With the property:
>>>> gpio_ctrl = of_parse_phandle(dev->of_node, "nvidia,gpio-controller", 0);
>>>> ....
>>>> hte_dev->c = gpiochip_find(gpio_ctrl, tegra_get_gpiochip_from_of_node);
>>>>
>>>> This simplifies the code significantly. The introdunction of this
>>>
>>> typo
>>
>> ACK...
>>>
>>>> property/binding does not break existing Tegra194 provider driver.
>>>
>>> Making a new property required is an ABI break.
>> The driver code for the Tegra194 binds by old binding and does not need
>> this new property, the relevant code is part of this patch series.
>>>
>>>> Signed-off-by: Dipen Patel <dipenp@xxxxxxxxxx>
>>>> ---
>>>>  .../timestamp/nvidia,tegra194-hte.yaml        | 31 +++++++++++++++++--
>>>>  1 file changed, 29 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/timestamp/nvidia,tegra194-hte.yaml b/Documentation/devicetree/bindings/timestamp/nvidia,tegra194-hte.yaml
>>>> index eafc33e9ae2e..841273a3d8ae 100644
>>>> --- a/Documentation/devicetree/bindings/timestamp/nvidia,tegra194-hte.yaml
>>>> +++ b/Documentation/devicetree/bindings/timestamp/nvidia,tegra194-hte.yaml
>>>> @@ -51,6 +51,12 @@ properties:
>>>>        LIC instance has 11 slices and Tegra234 LIC has 17 slices.
>>>>      enum: [3, 11, 17]
>>>>  
>>>> +  nvidia,gpio-controller:
>>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>>> +    description:
>>>> +      The phandle to AON gpio controller instance. This is required to handle
>>>> +      namespace conversion between GPIO and GTE.
>>>
>>> Explain what the GPIO controller is needed for rather than how this 
>>> changes the driver.
>> Doesn't "This is required..." statement addresses why GPIO controller is needed
>> for part? Or do you want detail explanation which is already part of the commit?
> 
> Your bindings commit msg focused on driver and it is not really what it
> should be about.
ACK...
> 
> Best regards,
> Krzysztof
>