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

  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;

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
property/binding does not break existing Tegra194 provider driver.

Signed-off-by: Dipen Patel <dipenp@nvidia.com>
---
 .../timestamp/nvidia,tegra194-hte.yaml        | 31 +++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)
  

On Thu, Mar 23, 2023 at 2:29 AM Dipen Patel <dipenp@nvidia.com> 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;
>
> 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
> property/binding does not break existing Tegra194 provider driver.
>
> Signed-off-by: Dipen Patel <dipenp@nvidia.com>

It is fair to assume that other operating systems will need this too
so I interpret the commit message as an example of the issues
faced by anyone making a driver for this HW.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
  

On Wed, 22 Mar 2023 18:29:23 -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;
> 
> 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
> property/binding does not break existing Tegra194 provider driver.
> 
> Signed-off-by: Dipen Patel <dipenp@nvidia.com>
> ---
>  .../timestamp/nvidia,tegra194-hte.yaml        | 31 +++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/timestamp/nvidia,tegra194-hte.example.dtb: timestamp@c1e0000: reg: [[0, 203292672], [0, 65536]] is too long
	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/timestamp/nvidia,tegra194-hte.yaml
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/timestamp/nvidia,tegra194-hte.example.dtb: timestamp@c1e0000: reg: [[0, 203292672], [0, 65536]] is too long
	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/timestamp/nvidia,tegra194-hte.yaml
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/timestamp/nvidia,tegra194-hte.example.dtb: timestamp@3aa0000: reg: [[0, 61472768], [0, 65536]] is too long
	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/timestamp/nvidia,tegra194-hte.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230323012929.10815-5-dipenp@nvidia.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
  

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.

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

> property/binding does not break existing Tegra194 provider driver.

Making a new property required is an ABI break.

> Signed-off-by: Dipen Patel <dipenp@nvidia.com>
> ---
>  .../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.

> +
>    '#timestamp-cells':
>      description:
>        This represents number of line id arguments as specified by the
> @@ -65,22 +71,43 @@ required:
>    - interrupts
>    - "#timestamp-cells"
>  
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - nvidia,tegra234-gte-aon
> +    then:
> +      required:
> +        - nvidia,gpio-controller

> +
>  additionalProperties: false
>  
>  examples:
>    - |
>      tegra_hte_aon: timestamp@c1e0000 {
>                compatible = "nvidia,tegra194-gte-aon";
> -              reg = <0xc1e0000 0x10000>;
> +              reg = <0x0 0xc1e0000 0x0 0x10000>;
> +              interrupts = <0 13 0x4>;
> +              nvidia,int-threshold = <1>;
> +              #timestamp-cells = <1>;
> +    };
> +
> +  - |
> +    tegra234_hte_aon: timestamp@c1e0000 {
> +              compatible = "nvidia,tegra234-gte-aon";
> +              reg = <0x0 0xc1e0000 0x0 0x10000>;
>                interrupts = <0 13 0x4>;
>                nvidia,int-threshold = <1>;
> +              nvidia,gpio-controller = <&gpio_aon>;
>                #timestamp-cells = <1>;
>      };
>  
>    - |
>      tegra_hte_lic: timestamp@3aa0000 {
>                compatible = "nvidia,tegra194-gte-lic";
> -              reg = <0x3aa0000 0x10000>;
> +              reg = <0x0 0x3aa0000 0x0 0x10000>;
>                interrupts = <0 11 0x4>;
>                nvidia,int-threshold = <1>;
>                #timestamp-cells = <1>;
> -- 
> 2.17.1
>
  

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.
> 
>>
>> 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@nvidia.com>
>> ---
>>  .../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?
> 
>> +
>>    '#timestamp-cells':
>>      description:
>>        This represents number of line id arguments as specified by the
>> @@ -65,22 +71,43 @@ required:
>>    - interrupts
>>    - "#timestamp-cells"
>>  
>> +allOf:
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - nvidia,tegra234-gte-aon
>> +    then:
>> +      required:
>> +        - nvidia,gpio-controller
> 
>> +
>>  additionalProperties: false
>>  
>>  examples:
>>    - |
>>      tegra_hte_aon: timestamp@c1e0000 {
>>                compatible = "nvidia,tegra194-gte-aon";
>> -              reg = <0xc1e0000 0x10000>;
>> +              reg = <0x0 0xc1e0000 0x0 0x10000>;
>> +              interrupts = <0 13 0x4>;
>> +              nvidia,int-threshold = <1>;
>> +              #timestamp-cells = <1>;
>> +    };
>> +
>> +  - |
>> +    tegra234_hte_aon: timestamp@c1e0000 {
>> +              compatible = "nvidia,tegra234-gte-aon";
>> +              reg = <0x0 0xc1e0000 0x0 0x10000>;
>>                interrupts = <0 13 0x4>;
>>                nvidia,int-threshold = <1>;
>> +              nvidia,gpio-controller = <&gpio_aon>;
>>                #timestamp-cells = <1>;
>>      };
>>  
>>    - |
>>      tegra_hte_lic: timestamp@3aa0000 {
>>                compatible = "nvidia,tegra194-gte-lic";
>> -              reg = <0x3aa0000 0x10000>;
>> +              reg = <0x0 0x3aa0000 0x0 0x10000>;
>>                interrupts = <0 11 0x4>;
>>                nvidia,int-threshold = <1>;
>>                #timestamp-cells = <1>;
>> -- 
>> 2.17.1
>>
  

On 23/03/2023 02:29, 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;
> 
> 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
> property/binding does not break existing Tegra194 provider driver.
> 
> Signed-off-by: Dipen Patel <dipenp@nvidia.com>
> ---
>  .../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.
> +
>    '#timestamp-cells':
>      description:
>        This represents number of line id arguments as specified by the
> @@ -65,22 +71,43 @@ required:
>    - interrupts
>    - "#timestamp-cells"
>  
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - nvidia,tegra234-gte-aon
> +    then:
> +      required:
> +        - nvidia,gpio-controller
> +
>  additionalProperties: false
>  
>  examples:
>    - |
>      tegra_hte_aon: timestamp@c1e0000 {
>                compatible = "nvidia,tegra194-gte-aon";
> -              reg = <0xc1e0000 0x10000>;
> +              reg = <0x0 0xc1e0000 0x0 0x10000>;

This is not really explained in commit msg... are you sure you tested it?


Best regards,
Krzysztof
  

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.


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

Best regards,
Krzysztof
  

On 3/25/23 4:07 AM, Krzysztof Kozlowski wrote:
> On 23/03/2023 02:29, 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;
>>
>> 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
>> property/binding does not break existing Tegra194 provider driver.
>>
>> Signed-off-by: Dipen Patel <dipenp@nvidia.com>
>> ---
>>  .../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.
>> +
>>    '#timestamp-cells':
>>      description:
>>        This represents number of line id arguments as specified by the
>> @@ -65,22 +71,43 @@ required:
>>    - interrupts
>>    - "#timestamp-cells"
>>  
>> +allOf:
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - nvidia,tegra234-gte-aon
>> +    then:
>> +      required:
>> +        - nvidia,gpio-controller
>> +
>>  additionalProperties: false
>>  
>>  examples:
>>    - |
>>      tegra_hte_aon: timestamp@c1e0000 {
>>                compatible = "nvidia,tegra194-gte-aon";
>> -              reg = <0xc1e0000 0x10000>;
>> +              reg = <0x0 0xc1e0000 0x0 0x10000>;
> 
> This is not really explained in commit msg... are you sure you tested it?
I have to revert this part back in next patch as when I upgraded dtsschema it gave me errors.
> 
> 
> Best regards,
> Krzysztof
>
  

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

On 04/04/2023 06:24, Dipen Patel wrote:
> 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.

Regardless of this commit, driver match data is the way to go, not
of_device_is_compatible().



Best regards,
Krzysztof
  

On Mon, Apr 03, 2023 at 09:24:17PM -0700, Dipen Patel wrote:
> 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.

I think the point that Rob and Krzysztof are trying to make that rather
than adding a new property for this, we can add a const char *gpio field
to struct tegra_hte_data and then set that to the compatible string of
the GPIO controller that we need this for.

To be honest, I slightly prefer the explicit phandle reference, but it
also complicates things a bit and looking up by compatible string isn't
all that bad.

Thierry
  

On Mon, Mar 27, 2023 at 09:58:19AM -0700, Dipen Patel wrote:
> On 3/25/23 4:07 AM, Krzysztof Kozlowski wrote:
> > On 23/03/2023 02:29, 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;
> >>
> >> 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
> >> property/binding does not break existing Tegra194 provider driver.
> >>
> >> Signed-off-by: Dipen Patel <dipenp@nvidia.com>
> >> ---
> >>  .../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.
> >> +
> >>    '#timestamp-cells':
> >>      description:
> >>        This represents number of line id arguments as specified by the
> >> @@ -65,22 +71,43 @@ required:
> >>    - interrupts
> >>    - "#timestamp-cells"
> >>  
> >> +allOf:
> >> +  - if:
> >> +      properties:
> >> +        compatible:
> >> +          contains:
> >> +            enum:
> >> +              - nvidia,tegra234-gte-aon
> >> +    then:
> >> +      required:
> >> +        - nvidia,gpio-controller
> >> +
> >>  additionalProperties: false
> >>  
> >>  examples:
> >>    - |
> >>      tegra_hte_aon: timestamp@c1e0000 {
> >>                compatible = "nvidia,tegra194-gte-aon";
> >> -              reg = <0xc1e0000 0x10000>;
> >> +              reg = <0x0 0xc1e0000 0x0 0x10000>;
> > 
> > This is not really explained in commit msg... are you sure you tested it?
> I have to revert this part back in next patch as when I upgraded dtsschema it gave me errors.

We need the 0x0 in the DTS files because we have #address-cells = <2>
and #size-tells = <2>. For the examples, those default to just 1 cell,
so this can't be an exact copy of what we have in the DTS files.

Please make sure to always validate the bindings and examples.

Thierry
  

On 4/4/23 3:30 AM, Thierry Reding wrote:
> On Mon, Mar 27, 2023 at 09:58:19AM -0700, Dipen Patel wrote:
>> On 3/25/23 4:07 AM, Krzysztof Kozlowski wrote:
>>> On 23/03/2023 02:29, 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;
>>>>
>>>> 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
>>>> property/binding does not break existing Tegra194 provider driver.
>>>>
>>>> Signed-off-by: Dipen Patel <dipenp@nvidia.com>
>>>> ---
>>>>  .../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.
>>>> +
>>>>    '#timestamp-cells':
>>>>      description:
>>>>        This represents number of line id arguments as specified by the
>>>> @@ -65,22 +71,43 @@ required:
>>>>    - interrupts
>>>>    - "#timestamp-cells"
>>>>  
>>>> +allOf:
>>>> +  - if:
>>>> +      properties:
>>>> +        compatible:
>>>> +          contains:
>>>> +            enum:
>>>> +              - nvidia,tegra234-gte-aon
>>>> +    then:
>>>> +      required:
>>>> +        - nvidia,gpio-controller
>>>> +
>>>>  additionalProperties: false
>>>>  
>>>>  examples:
>>>>    - |
>>>>      tegra_hte_aon: timestamp@c1e0000 {
>>>>                compatible = "nvidia,tegra194-gte-aon";
>>>> -              reg = <0xc1e0000 0x10000>;
>>>> +              reg = <0x0 0xc1e0000 0x0 0x10000>;
>>>
>>> This is not really explained in commit msg... are you sure you tested it?
>> I have to revert this part back in next patch as when I upgraded dtsschema it gave me errors.
> 
> We need the 0x0 in the DTS files because we have #address-cells = <2>
> and #size-tells = <2>. For the examples, those default to just 1 cell,
> so this can't be an exact copy of what we have in the DTS files.
> 
> Please make sure to always validate the bindings and examples.

Ack...
> 
> Thierry
  

On 4/4/23 3:28 AM, Thierry Reding wrote:
> On Mon, Apr 03, 2023 at 09:24:17PM -0700, Dipen Patel wrote:
>> 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.
> 
> I think the point that Rob and Krzysztof are trying to make that rather
> than adding a new property for this, we can add a const char *gpio field
> to struct tegra_hte_data and then set that to the compatible string of
> the GPIO controller that we need this for.

This means it will have to track the label of the gpio controller and for each
new provider, we have to touch the driver to set the char *field. Also, I think
having gpio controller property in the DT presents/describes the tegra HTE provider
perfectly as it does have hard dependency on the tegra gpio controller.

> 
> To be honest, I slightly prefer the explicit phandle reference, but it
> also complicates things a bit and looking up by compatible string isn't
> all that bad.
> 
> Thierry
  

On 4/3/23 10:33 PM, Krzysztof Kozlowski wrote:
> On 04/04/2023 06:24, Dipen Patel wrote:
>> 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.
> 
> Regardless of this commit, driver match data is the way to go, not
> of_device_is_compatible().

I agree...

> 
> 
> 
> Best regards,
> Krzysztof
>