Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

Krzysztof Kozlowski
2025-02-24T09:11:28Z

On Mon, Feb 24, 2025 at 01:59:34PM +0800, Ryan Chen wrote:
> Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,enable-dma
> and description for ast2600-i2cv2.
>
> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> ---
> .../devicetree/bindings/i2c/aspeed,i2c.yaml | 58 +++++++++++++++++++
> 1 file changed, 58 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> index 5b9bd2feda3b..356033d18f90 100644
> --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> @@ -44,12 +44,60 @@ properties:
> description: frequency of the bus clock in Hz defaults to 100 kHz when not
> specified
>
> + multi-master:
> + type: boolean
> + description:
> + states that there is another master active on this bus

Except that this wasn't ever tested...

Don't duplicate properties. i2c-controller schema has it already.

> +
> + aspeed,enable-dma:
> + type: boolean
> + description: |
> + I2C bus enable dma mode transfer.
> +
> + ASPEED ast2600 platform equipped with 16 I2C controllers that share a
> + single DMA engine. DTS files can specify the data transfer mode to/from
> + the device, either DMA or programmed I/O. However, hardware limitations

so what is byte mode?

> + may require a DTS to manually allocate which controller can use DMA mode.
> + The "aspeed,enable-dma" property allows control of this.
> +
> + In cases where one the hardware design results in a specific
> + controller handling a larger amount of data, a DTS would likely
> + enable DMA mode for that one controller.
> +
> + aspeed,enable-byte:
> + type: boolean
> + description: |
> + I2C bus enable byte mode transfer.

No, either this is expressed as lack of dma mode property or if you have
three modes, then rather some enum (aspeed,transfer-mode ?)

> +
> + aspeed,global-regs:
> + $ref: /schemas/types.yaml#/definitions/phandle
> + description: The phandle of i2c global register node.

For what? Same question as usual: do not repeat property name, but say
what is this used for and what exactly it points to.

s/i2c/I2C/ but then what is "I2C global register node"? This is how you
call your device in datasheet?

> +
> required:
> - reg
> - compatible
> - clocks
> - resets
>
> +allOf:
> + - $ref: /schemas/i2c/i2c-controller.yaml#
> + - if:
> + properties:
> + compatible:
> + contains:
> + const: aspeed,ast2600-i2cv2

NAK, undocumented compatible.

> +
> + then:
> + properties:
> + reg:
> + minItems: 2

Best regards,
Krzysztof