Re: [PATCH 1/2] dt-bindings: i2c: Add Tegra20 I2C target controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] dt-bindings: i2c: tegra20: driver-specific compatible for
  target mode
- [Low] dt-bindings: i2c: tegra20: missing clock constraints for target
  compatible
--

commit 49b20f39ced9dd1d6790b83dd4714812990cb33d
Author: Marc Dietrich <marvin24@gmx.de>

dt-bindings: i2c: Add Tegra20 I2C target controller

This commit adds the "nvidia,tegra20-i2c-target" compatible string and an
example for the Tegra20 I2C target controller to the Tegra20 I2C binding.

> diff --git a/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml b/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml
> index 51241c1293e3e..2d7ee8e8750d4 100644
> --- a/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml
> @@ -13,11 +13,15 @@ title: NVIDIA Tegra I2C controller driver
>  properties:
>    compatible:
>      oneOf:
> -      - description: Tegra20 has 4 generic I2C controller. This can support
> -          master and slave mode of I2C communication. The i2c-tegra driver
> -          only support master mode of I2C communication. Driver of I2C
> -          controller is only compatible with "nvidia,tegra20-i2c".
> +      - description: Tegra20 has 3 instances of a generic I2C controller.
> +          It supports controller and target mode of I2C communication.
> +          The i2c-tegra driver only supports master mode of I2C communication.
> +          Driver of I2C controller is only compatible with "nvidia,tegra20-i2c".
>          const: nvidia,tegra20-i2c
> +      - description: The i2c-tegra-target driver supports the target mode of
> +          the generic Tegra20 I2C controller. Driver of the I2C target
> +          controller is only compatible with "nvidia,tegra20-i2c-target".
> +        const: nvidia,tegra20-i2c-target

[Severity: Medium]
Should target mode be selected via a child node using I2C_OWN_SLAVE_ADDRESS
rather than a separate "nvidia,tegra20-i2c-target" compatible string?

The three generic Tegra20 I2C controllers (such as i2c@7000c500 in the
example below) are the same hardware IP block already described by
"nvidia,tegra20-i2c", containing both controller and target registers
(sl_cnfg, sl_addr1, sl_addr2 in tegra20_i2c_regs) in the same MMIO region.
Defining a separate compatible string encodes Linux driver structure and
operating mode into the Devicetree ABI and requires boards to override the
SoC controller node's compatible property to use target mode.

> @@ -222,3 +226,18 @@ examples:
>          #address-cells = <1>;
>          #size-cells = <0>;
>      };
> +
> +  - |
> +    i2c@7000c500 {
> +        compatible = "nvidia,tegra20-i2c-target";
> +        reg = <0x7000c500 0x100>;
> +        interrupts = <0 92 0x04>;
> +        clocks = <&tegra_car 67>, <&tegra_car 124>;
> +        clock-names = "div-clk", "fast-clk";

[Severity: Low]
If "nvidia,tegra20-i2c-target" is kept, should it also be added to the
allOf conditional that constrains clocks and clock-names for
"nvidia,tegra20-i2c"?

The top-level properties allow a single clock with an arbitrary name, and
the allOf rule enforcing minItems: 2 and clock-names "div-clk" and
"fast-clk" only matches "nvidia,tegra20-i2c" and "nvidia,tegra30-i2c",
leaving "nvidia,tegra20-i2c-target" nodes unvalidated for the second clock:

Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml:
  - if:
      properties:
        compatible:
          contains:
            enum:
              - nvidia,tegra20-i2c
              - nvidia,tegra30-i2c
    then:
      properties:
        clocks:
          minItems: 2
        clock-names:
          items:
            - const: div-clk
            - const: fast-clk

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115182616.354580-1-marvin24@gmx.de?part=1