[EXT] Re: [PATCH v2 1/2] dt-bindings: i3c: Add NXP P3H2x4x i3c-hub support
Frank Li
Frank.li at nxp.com
Thu May 22 11:46:44 PDT 2025
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
More information about the linux-i3c
mailing list