Re: [RESEND v12 1/3] dt-bindings: mmc: mtk-sd: extend interrupts and pinctrls properties

[Axe Yang]

On Fri, 2022-06-03 at 09:28 +0200, Ulf Hansson wrote:
> On Wed, 25 May 2022 at 03:51, Axe Yang <axe.yang@xxxxxxxxxxxx> wrote:
> > 
> > Extend interrupts and pinctrls for SDIO wakeup interrupt feature.
> > This feature allow SDIO devices alarm asynchronous interrupt to
> > host
> > even when host stop providing clock to SDIO card. An extra wakeup
> > interrupt and pinctrl states for SDIO DAT1 pin state switching are
> > required in this scenario.
> > 
> > Reviewed-by: AngeloGioacchino Del Regno <
> > angelogioacchino.delregno@xxxxxxxxxxxxx>
> > Signed-off-by: Axe Yang <axe.yang@xxxxxxxxxxxx>
> > ---
> >  .../devicetree/bindings/mmc/mtk-sd.yaml       | 50
> > ++++++++++++++++++-
> >  1 file changed, 49 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/mmc/mtk-sd.yaml
> > b/Documentation/devicetree/bindings/mmc/mtk-sd.yaml
> > index 2a2e9fa8c188..e83bf10281d6 100644
> > --- a/Documentation/devicetree/bindings/mmc/mtk-sd.yaml
> > +++ b/Documentation/devicetree/bindings/mmc/mtk-sd.yaml
> > @@ -72,12 +72,27 @@ properties:
> >        - const: ahb_cg
> > 
> >    interrupts:
> > -    maxItems: 1
> > +    description:
> > +      Should at least contain MSDC GIC interrupt. To support SDIO
> > in-band wakeup, an extended
> > +      interrupt is required and be configured as wakeup source
> > irq.
> > +    minItems: 1
> > +    maxItems: 2
> > +
> > +  interrupt-names:
> > +    items:
> > +      - const: msdc
> > +      - const: sdio_wakeup
> > 
> >    pinctrl-names:
> > +    description:
> > +      Should at least contain default and state_uhs. To support
> > SDIO in-band wakeup, dat1 pin
> > +      will be switched between GPIO mode and SDIO DAT1 mode,
> > state_eint and state_dat1 are
> > +      mandatory in this scenarios.
> > +    minItems: 2
> >      items:
> >        - const: default
> >        - const: state_uhs
> > +      - const: state_eint
> 
> Don't you need something along the lines of the below instead? I mean
> the "state_eint" isn't always needed, right?
> 
> oneOf:
>   - items:
>       - const: default
>       - const: state_uhs
>     - items:
>         - const: default
>         - const: state_uhs
>         - const: state_eint
> 
No, it is not always needed. 
As Rob said, the 'minItems: 2' makes the 3rd item optional.
Combine 'minItems' and 'description' fields, it is easy for others to
understand how to set pinctrl related properities.

Anyway, If you insist 'oneOf' is the better way, I can update that in
next version. What do you think?

And thanks to your comment, I found a mistake in 'description', I
should remove descriptions related to 'state_dat1', and I will update
that in next version.

And do you have any comment on patch 2/3 and 3/3?


Regards,
Axe