Re: [PATCH v12 2/7] dt-bindings: i3c: Add NXP P3H2x4x i3c-hub support

On Wed, Jun 17, 2026 at 11:13:31AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Medium] The binding documents GPIO capability for target ports but omits the standard GPIO provider properties, violating subsystem guidelines.
> - [Medium] The binding fails to include compatible strings for the 4-port variants and unconditionally allows up to 8 ports.
> - [Low] The unit address in the I3C example does not match its `reg` property.
> --
>
> commit af3b77595fece3e15864b37b9b683be835a0e1eb
> Author: Aman Kumar Pandey <[email protected]>
>
> dt-bindings: i3c: Add NXP P3H2x4x i3c-hub support
>
> This commit adds device tree bindings for the NXP P3H2x4x multi-port I3C hub
> family. It supports connecting a host via I3C/I2C/SMBus to multiple downstream
> peripherals across up to 8 ports.
>
> > diff --git a/Documentation/devicetree/bindings/i3c/nxp,p3h2840.yaml b/Documentation/devicetree/bindings/i3c/nxp,p3h2840.yaml
> > new file mode 100644
> > index 0000000000000..c080eeb0eeaa8
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/i3c/nxp,p3h2840.yaml
> > @@ -0,0 +1,291 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +# Copyright 2025 NXP
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/i3c/nxp,p3h2840.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: NXP P3H2X4X I3C HUB
> > +
> > +maintainers:
> > +  - Aman Kumar Pandey <[email protected]>
> > +  - Vikash Bansal <[email protected]>
> > +  - Lakshay Piplani <[email protected]>
> > +
> > +description: |
> > +  P3H2x4x (P3H2440/P3H2441/P3H2840/P3H2841) is a family of multiport I3C
> > +  hub devices that connect to:-
> > +  1. A host CPU via I3C/I2C/SMBus bus on upstream side and connect to multiple
> > +     peripheral devices on the downstream  side.
> > +  2. Have two Controller Ports which can support either
> > +     I2C/SMBus or I3C buses and connect to a CPU, BMC or SOC.
> > +  3. P3H2840/ P3H2841 are 8 port I3C hub with eight I3C/I2C Target Port.
> > +  4. P3H2440/ P3H2441 are 4 port I3C hub with four I3C/I2C Target Port.
> > +     Target ports can be configured as I2C/SMBus, I3C or GPIO and connect to
> > +     peripherals.
>
> [Severity: Medium]
> Since the description advertises that target ports can be configured as GPIO,
> should this binding include the standard GPIO provider properties like
> gpio-controller and #gpio-cells?
>
> Because unevaluatedProperties is false and there are no child node patterns
> defined for GPIO ports in patternProperties, won't adding a GPIO child node
> fail schema validation?
>
> > +
> > +properties:
> > +  compatible:
> > +    const: nxp,p3h2840
>
> [Severity: Medium]
> The commit message and description state the binding supports the P3H2440
> and P3H2441 4-port hubs alongside the 8-port P3H2840/P3H2841.
>
> Should compatible strings be included for the 4-port variants here as well?
>
> [ ... ]
>
> > +patternProperties:
> > +  "^i3c@[0-7]$":
> > +    type: object
> > +    $ref: /schemas/i3c/i3c.yaml#
> > +    unevaluatedProperties: false
> > +
> > +    properties:
> > +      reg:
> > +        description:
> > +          The I3C HUB Target Port number.
> > +        maximum: 7
>
> [Severity: Medium]
> Does unconditionally allowing a maximum port index of 7 cause issues for the
> 4-port variants mentioned in the description?
>
> It seems this allows invalid configurations that define non-existent ports
> (4-7) for the 4-port hardware.
>
> [ ... ]
>
> > +examples:
> > +  - |
> > +    i3c {
> > +        #address-cells = <3>;
> > +        #size-cells = <0>;
> > +
> > +        hub@70,236153000c2 {
> > +            reg = <0x70 0x236 0x3000c2>;
>
> [Severity: Low]
> Does the unit address in this example match its reg property?
>
> The reg property combines to a provisioned ID of 0x236003000c2, which appears
> to differ from 236153000c2 in the unit address. Could this mismatch trigger a
> simple_bus_reg warning during make dt_binding_check?

I remember I3C mask 0x15, it'd better kept node name consisent with reg

Frank

>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

-- 
linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c