| Message ID | 20230310190634.5053-3-dipenp@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On 10/03/2023 20:06, Dipen Patel wrote: > Added timestamp provider support for the Tegra234 in devicetree > bindings. In addition, it addresses review comments from the > previous review round as follows: > - Removes nvidia,slices property. This was not necessary as it > is a constant value and can be hardcoded inside the driver code. > - Adds nvidia,gpio-controller property. This simplifies how GTE driver > retrieves GPIO controller instance, see below explanation. > > 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); > > We haven't technically started making use of these bindings, so > backwards-compatibility shouldn't be an issue yet. Unfortunately, I don't understand this statement. The nvidia,tegra194-gte-aon with removed property is in a released kernel v6.2. What does it mean "technically"? It's a released kernel thus it is a released ABI. And since DTS always go to separate branch, your patch #4 breaks existing DTS (return -ENODEV;) - it is not bisectable. > > Signed-off-by: Dipen Patel <dipenp@nvidia.com> > --- Best regards, Krzysztof
On 3/12/23 8:47 AM, Krzysztof Kozlowski wrote: > On 10/03/2023 20:06, Dipen Patel wrote: >> Added timestamp provider support for the Tegra234 in devicetree >> bindings. In addition, it addresses review comments from the >> previous review round as follows: >> - Removes nvidia,slices property. This was not necessary as it >> is a constant value and can be hardcoded inside the driver code. >> - Adds nvidia,gpio-controller property. This simplifies how GTE driver >> retrieves GPIO controller instance, see below explanation. >> >> 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); >> >> We haven't technically started making use of these bindings, so >> backwards-compatibility shouldn't be an issue yet. > > Unfortunately, I don't understand this statement. The > nvidia,tegra194-gte-aon with removed property is in a released kernel > v6.2. What does it mean "technically"? It's a released kernel thus it is > a released ABI. There is no active user of that driver, so even if it breaks 6.2, it is fine as there is no one to complain about it. > > And since DTS always go to separate branch, your patch #4 breaks > existing DTS (return -ENODEV;) - it is not bisectable. > >> >> Signed-off-by: Dipen Patel <dipenp@nvidia.com> >> --- > > > Best regards, > Krzysztof >
On 3/13/23 2:57 PM, Linus Walleij wrote: > Hi Dipen, > > thanks for maintaining HTE! > > On Fri, Mar 10, 2023 at 8:06 PM Dipen Patel <dipenp@nvidia.com> wrote: > >> - nvidia,slices: >> - $ref: /schemas/types.yaml#/definitions/uint32 > > I would not delete this, just mark it deprecated. > > nvidia,slices: > $ref: /schemas/types.yaml#/definitions/uint32 > deprecated: true > > (And remove it from required, of course) > > This way you do not need to explain about why it was > deleted, it's just deprecated, which is fine. Great suggestion, thanks, will make changes in the next patch. > > Yours, > Linus Walleij
On 3/13/23 4:49 PM, Dipen Patel wrote: > On 3/13/23 2:57 PM, Linus Walleij wrote: >> Hi Dipen, >> >> thanks for maintaining HTE! >> >> On Fri, Mar 10, 2023 at 8:06 PM Dipen Patel <dipenp@nvidia.com> wrote: >> >>> - nvidia,slices: >>> - $ref: /schemas/types.yaml#/definitions/uint32 >> >> I would not delete this, just mark it deprecated. >> >> nvidia,slices: >> $ref: /schemas/types.yaml#/definitions/uint32 >> deprecated: true >> >> (And remove it from required, of course) >> >> This way you do not need to explain about why it was >> deleted, it's just deprecated, which is fine. > > Great suggestion, thanks, will make changes in the next patch. However, as I understood, current point of contention/discussion is addition of the nvidia,gpio-controller property. >> >> Yours, >> Linus Walleij >
On 13/03/2023 23:49, Dipen Patel wrote: > On 3/13/23 2:57 PM, Linus Walleij wrote: >> Hi Dipen, >> >> thanks for maintaining HTE! >> >> On Fri, Mar 10, 2023 at 8:06 PM Dipen Patel <dipenp@nvidia.com> wrote: >> >>> - nvidia,slices: >>> - $ref: /schemas/types.yaml#/definitions/uint32 >> >> I would not delete this, just mark it deprecated. >> >> nvidia,slices: >> $ref: /schemas/types.yaml#/definitions/uint32 >> deprecated: true >> >> (And remove it from required, of course) >> >> This way you do not need to explain about why it was >> deleted, it's just deprecated, which is fine. > > Great suggestion, thanks, will make changes in the next patch. When you deprecate it, please make this a separate patch because it is separate from adding Tegra234 support. Similarly you will want to have a separate patch to remove support of the property from the driver as well. Jon
diff --git a/Documentation/devicetree/bindings/timestamp/nvidia,tegra194-hte.yaml b/Documentation/devicetree/bindings/timestamp/nvidia,tegra194-hte.yaml index c31e207d1652..eb904ac2f331 100644 --- a/Documentation/devicetree/bindings/timestamp/nvidia,tegra194-hte.yaml +++ b/Documentation/devicetree/bindings/timestamp/nvidia,tegra194-hte.yaml @@ -4,7 +4,7 @@ $id: http://devicetree.org/schemas/timestamp/nvidia,tegra194-hte.yaml# $schema: http://devicetree.org/meta-schemas/core.yaml# -title: Tegra194 on chip generic hardware timestamping engine (HTE) +title: Tegra on chip generic hardware timestamping engine (HTE) provider maintainers: - Dipen Patel <dipenp@nvidia.com> @@ -23,6 +23,8 @@ properties: enum: - nvidia,tegra194-gte-aon - nvidia,tegra194-gte-lic + - nvidia,tegra234-gte-aon + - nvidia,tegra234-gte-lic reg: maxItems: 1 @@ -38,14 +40,11 @@ properties: minimum: 1 maximum: 256 - nvidia,slices: - $ref: /schemas/types.yaml#/definitions/uint32 + nvidia,gpio-controller: + $ref: /schemas/types.yaml#/definitions/phandle description: - HTE lines are arranged in 32 bit slice where each bit represents different - line/signal that it can enable/configure for the timestamp. It is u32 - property and depends on the HTE instance in the chip. The value 3 is for - GPIO GTE and 11 for IRQ GTE. - enum: [3, 11] + The phandle to AON gpio controller instance. This is required to handle + namespace conversion between GPIO and GTE. '#timestamp-cells': description: @@ -59,9 +58,20 @@ required: - compatible - reg - interrupts - - nvidia,slices - "#timestamp-cells" +allOf: + - if: + properties: + compatible: + contains: + enum: + - nvidia,tegra194-gte-aon + - nvidia,tegra234-gte-aon + then: + required: + - nvidia,gpio-controller + additionalProperties: false examples: @@ -71,7 +81,7 @@ examples: reg = <0xc1e0000 0x10000>; interrupts = <0 13 0x4>; nvidia,int-threshold = <1>; - nvidia,slices = <3>; + nvidia,gpio-controller = <&gpio_aon>; #timestamp-cells = <1>; }; @@ -81,7 +91,6 @@ examples: reg = <0x3aa0000 0x10000>; interrupts = <0 11 0x4>; nvidia,int-threshold = <1>; - nvidia,slices = <11>; #timestamp-cells = <1>; };
Added timestamp provider support for the Tegra234 in devicetree bindings. In addition, it addresses review comments from the previous review round as follows: - Removes nvidia,slices property. This was not necessary as it is a constant value and can be hardcoded inside the driver code. - Adds nvidia,gpio-controller property. This simplifies how GTE driver retrieves GPIO controller instance, see below explanation. 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); We haven't technically started making use of these bindings, so backwards-compatibility shouldn't be an issue yet. Signed-off-by: Dipen Patel <dipenp@nvidia.com> --- v2: - Removed nvidia,slices property - Added nvidia,gpio-controller based on review comments from Thierry, this will help simplify the hte provider driver. v3: - Explained changes in detail in commit message - Added allOf section per review comment .../timestamp/nvidia,tegra194-hte.yaml | 31 ++++++++++++------- 1 file changed, 20 insertions(+), 11 deletions(-)