> Subject: Re: [PATCH 1/4] dt-bindings: mfd: nxp,bbnsm: Add binding for nxp
> bbnsm
>
> On 21/11/2022 07:51, Jacky Bai wrote:
> > Add binding for NXP BBNSM(Battery-Backed Non-Secure Module).
> >
> > Signed-off-by: Jacky Bai <ping.bai@xxxxxxx>
> > ---
...
> > +
> > + properties:
> > + compatible:
> > + const: nxp,bbnsm-rtc
>
>
> Missing ref to rtc.yaml.
Ok will include in v2.
>
> > +
> > + regmap:
>
> Use vendor prefix, descriptive name and description. Where is the type of
> 'regmap' defined?
Type is missed. Will add a description and type define if necessary.
>
> > + maxItems: 1
>
> I don't think this is correct. Rob explained the simple-mfd means children
do
> not depend on anything from the parent, but taking a regmap from its
parent
> contradicts it.
For this BBNSM module, basically, it provides two sperate & different
function: RTC and ON/OFF button control. But
no separate register offset range for each of these functions. For example,
the interrupt enable control,
Interrupt status and basic function control are mixed in the same registers'
different bit.
Any good suggestion on how to handle such case? ^_^
>
> > +
> > + interrupts:
> > + maxItems: 1
>
> You have same interrupt and same address space used by two devices.
>
> Both arguments (depending on parent regmap, sharing interrupt) suggests
> that this is one device block, so describing it with simple-mfd is quite
> unflexible.
>
It is depends on how SoC integrates this BBNSM module. From the BBNSM side,
it has separate IRQ lines for RTC function and ON/OFF function. Different
IRQ lines
can be used for RTC and ON/OFF button. But in current i.MX93 SoC, those
interrupts
are ORed together at SoC level. That's why same interrupt in the example.
> > +
> > + required:
> > + - compatible
> > + - regmap
> > + - interrupts
> > +
> > + additionalProperties: false
> > +
> > + pwrkey:
> > + type: object
> > + $ref: /schemas/input/input.yaml#
> > +
> > + properties:
> > + compatible:
> > + const: nxp,bbnsm-pwrkey
> > +
> > + regmap:
> > + maxItems: 1
> > +
> > + interrupts:
> > + maxItems: 1
> > +
> > + linux,code: true
> > +
> > + required:
> > + - compatible
> > + - regmap
> > + - interrupts
> > +
> > + additionalProperties: false
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - rtc
> > + - pwrkey
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + bbnsm: bbnsm@44440000 {
> > + compatible = "nxp,bbnsm", "syscon", "simple-mfd";
> > + reg = <0x44440000 0x10000>;
> > +
> > + bbnsm_rtc: rtc {
> > + compatible = "nxp,bbnsm-rtc";
>
> Use 4 spaces for example indentation.
>
Ok, will fix it.
Thx
BR
> > + regmap = <&bbnsm>;
> > + interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>;
> > + };
> > +
> > + bbnsm_pwrkey: pwrkey {
> > + compatible = "nxp,bbnsm-pwrkey";
> > + regmap = <&bbnsm>;
> > + interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>;
> > + linux,code = <KEY_POWER>;
> > + };
> > + };
>
> Best regards,
> Krzysztof
<<attachment: smime.p7s>>
