Re: [PATCH 12/13] dt-bindings: lpspi: Document nxp,lpspi-pincfg property

[Frank Li <Frank.li@nxp.com>]

On Tue, Aug 19, 2025 at 10:51:03AM +0100, James Clark wrote:
>
>
> On 18/08/2025 4:39 pm, Frank Li wrote:
> > On Mon, Aug 18, 2025 at 03:47:45PM +0100, James Clark wrote:
> > >
> > >
> > > On 14/08/2025 7:19 pm, Frank Li wrote:
> > > > On Thu, Aug 14, 2025 at 05:06:52PM +0100, James Clark wrote:
> > > > > Document the two valid pincfg values and the defaults.
> > > > >
> > > > > Although the hardware supports two more values for half-duplex modes,
> > > > > the driver doesn't support them so don't document them.
> > > >
> > > > binding doc should be first patch before drivers.
> > > >
> > > > binding descript hardware not driver, you should add all regardless if
> > > > driver support it.
> > > >
> > >
> > > Replied to same on "[PATCH 10/13] spi: spi-fsl-lpspi: Add compatible for
> > > S32G"
> > >
> > > > >
> > > > > Signed-off-by: James Clark <james.clark@linaro.org>
> > > > > ---
> > > > >    Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml | 14 ++++++++++++++
> > > > >    1 file changed, 14 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
> > > > > index ce7bd44ee17e..3f8833911807 100644
> > > > > --- a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
> > > > > +++ b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
> > > > > @@ -70,6 +70,19 @@ properties:
> > > > >      power-domains:
> > > > >        maxItems: 1
> > > > >
> > > > > +  nxp,pincfg:
> > > > > +    description:
> > > > > +      'Pin configuration value for CFGR1.PINCFG.
> > > > > +        - "sin-in-sout-out": SIN is used for input data and SOUT is used for
> > > > > +                             output data
> > > > > +        - "sout-in-sin-out": SOUT is used for input data and SIN is used for
> > > > > +                             output data
> > > > > +      If no value is specified then the default is "sin-in-sout-out" for host
> > > > > +      mode and "sout-in-sin-out" for target mode.'
> > > >
> > > > why need this? are there varible at difference boards? look like default
> > > > is more make sense.
> > > >
> > >
> > > + Larissa. I think this might also be a question for the hardware designers
> > > about why the feature to swap the pins was deemed worth including.
> > >
> > > I'm assuming the flexibility is given for routing reasons. If you have
> > > another device with the pins in one order then you can route it without a
> > > via if they happen to be in the same order.
> >
> > DT team need reason to judge if a new property is reasonable/neccesary. You
> > need  mention the reason why need this property. Here, some board design
> > swap sin/sout.
> >
> > >
> > > > SPI signal name is MOSI and MISO
> > > >
> > > > Frank
> > > >
> > >
> > > As mentioned in the commit message of "[PATCH 05/13] spi: spi-fsl-lpspi:
> > > Enumerate all pin configuration definitions" the names were taken directly
> > > from the reference manual and this doc text was too. I think diverging from
> > > CFGR1_PINCFG could be potentially quite confusing. And MOSI isn't mentioned
> > > once in S32G3RM rev 4:
> > >
> > >    Configures which pins are used for input and output data during serial
> > >    transfers. When performing parallel transfers, the Pin Configuration
> > >    field is ignored.
> > >
> > >      00b - SIN is used for input data and SOUT is used for output data
> > >      01b - SIN is used for both input and output data, only half-duplex
> > >            serial transfers are supported
> > >      10b - SOUT is used for both input and output data, only half-duplex
> > >            serial transfers are supported
> > >      11b - SOUT is used for input data and SIN is used for output data
> >
> > dt binding is ABI, design user easy understand property string.  like
> >
> > enum:
> >    - normal
> >    - swap
> >    - half-duplex-on-sin
> >    - half-duplex-on-sout
> >
> > Frank
> >
>
> If we're not directly using the names that get programmed into the register
> then it's better to remove the implicit 5th mode that you get for leaving it
> blank and to use that as "normal" instead.

make sense.

Frank
> Then "swap" is to swap whatever
> "normal" would have picked. Otherwise "normal" being a fixed value doesn't
> match up to the current "normal" behavior which is actually different value
> depending on host or target mode.
>
> So it would look like this with the "if no value is specified..." bit
> reworded too:
>
>    description:
>       'Pin configuration value for CFGR1.PINCFG.
>         - "normal": Hosts - SIN is used for input data and SOUT is used
>                       for output data.
>                     Targets - SOUT is used for input data and SIN is
>
>                       used for output data.
>         - "swap": The inverse of "normal"
>         - "half-duplex-on-sin": SIN is used for both input and output
>                   data. Unsupported.
>         - "half-duplex-on-sout": SOUT is used for both input and output
>                   data. Unsupported.
>       If no value is specified then the default is "normal".
>
> > >
> > > James
> > >
> > > > > +    enum:
> > > > > +      - sin-in-sout-out
> > > > > +      - sout-in-sin-out
> > > > > +
> > > > >    required:
> > > > >      - compatible
> > > > >      - reg
> > > > > @@ -95,4 +108,5 @@ examples:
> > > > >            spi-slave;
> > > > >            fsl,spi-only-use-cs1-sel;
> > > > >            num-cs = <2>;
> > > > > +        nxp,pincfg = "sout-in-sin-out";
> > > > >        };
> > > > >
> > > > > --
> > > > > 2.34.1
> > > > >
> > >
>