[EXT] Re: [PATCH v2 1/2] dt-bindings: i3c: Add NXP P3H2x4x i3c-hub support

[Frank Li]

On Wed, May 14, 2025 at 12:10:11PM +0200, Krzysztof Kozlowski wrote:
> On 12/05/2025 13:45, Aman Kumar Pandey wrote:
> >>>
> >>> Signed-off-by: Aman Kumar Pandey <aman.kumarpandey at nxp.com>
> >>> Signed-off-by: Vikash Bansal <vikash.bansal at nxp.com>
> >>> ---
> >>> V1 -> V2: Cleaned up coding style and addressed review comments
> >>> ---
> >>>  .../bindings/i3c/p3h2840-i3c-hub.yaml         | 332 ++++++++++++++++++
> >>
> >> Not much improved. I have doubts that you really looked at other bindings.
> >>
> >> Filename matching compatible.
> >>
> >>
> >
> > Thanks for reviewing the patch.
> > There is no existing entry like this one. It is the first device tree entry for any i3c target device being added to Linux upstream.
> > For I3C target device, compatible is not required as per i3c.yml. I am adding compatible for backward compatibility with i2c binding ( i3c target device have backward compatibility).
> > Should I use compatible matching filename in this case ?
>
> Do you have compatible here? Yes, you have, so I do not get what is
> unclear in my comment. Rename the binding to match the compatible, which
> you have here.
>
>
> >
> >>>  MAINTAINERS                                   |   8 +
> >>>  2 files changed, 340 insertions(+)
> >
> >
> >>> +
> >>> +properties:
> >>> +  $nodename:
> >>> +    pattern: "^hub@[@.*]+,[0-9a-f]+$"
> >>
> >> Drop
> >>
> >
> > Sorry, did not understand your point. Could you please give some clarification ?
>
> Drop this nodename. Remove it.
>
> >
> >>> +
> >
> >>> +
> >>> +  cp0-ldo-enable:
> >>> +    type: boolean
> >>> +    description:
> >>> +      Enables the on-die LDO for controller Port 0.
> >>> +
> >>> +  cp1-ldo-enable:
> >>> +    type: boolean
> >>> +    description:
> >>> +      Enables the on-die LDO for controller Port 1.
> >>> +
> >>> +  tp0145-ldo-enable:
> >>> +    type: boolean
> >>> +    description:
> >>> +      Enables the on-die LDO for target ports 0/1/4/5.
> >>> +
> >>> +  tp2367-ldo-enable:
> >>> +    type: boolean
> >>> +    description:
> >>> +      Enables the on-die LDO for target ports 2/3/6/7.
> >>> +
> >>
> >> NAK for all above properties.
> >>
> >
> > These are not vender specific properties. all these properties are as per i3c hub specification.
> >  What should I change for above properties ?
>
>
> What do they represent? Regulators? Why would you add custom properties
> for controlling regulators? Why would you enable LDOs for ports which
> have nothing connected? Why would you not enable LDOs for ports which
> have something there? Why these are not properties of the ports?
>

power supply cp[0,1] has two options: one from internal ldo, the other from the
other regulator.

So I think it should be *-supply

cp0-supply:
  description: phandle to cp0 power supply.

because internal ldo can provide difference voltage, I suggest export as
standard regulator

regulators:
  properties:
    cp0:
      type: object
      $ref: /schemas/regulator/regulator.yaml#
      unevaluatedProperties: false

Frank


> So many questions.
>
>
> Best regards,
> Krzysztof