[v2,3/3] dt-bindings: gpio: Add Nuvoton NPCM750 serial I/O expansion interface(SGPIO)

  NPCM750 include two SGPIO modules.
Each module supports up to 64 input and 64 output pins.
the output pin must be serial to parallel device(such as the hc595)
the input in must be parallel to serial device(such as the hc165)

Signed-off-by: Jim Liu <JJLIU0@nuvoton.com>
---
Changes for v2:
   - modify description
---
 .../bindings/gpio/nuvoton,sgpio.yaml          | 79 +++++++++++++++++++
 1 file changed, 79 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/nuvoton,sgpio.yaml

+        status = "disabled";
+    };
  

On Tue, 08 Nov 2022 17:28:40 +0800, Jim Liu wrote:
> NPCM750 include two SGPIO modules.
> Each module supports up to 64 input and 64 output pins.
> the output pin must be serial to parallel device(such as the hc595)
> the input in must be parallel to serial device(such as the hc165)
> 
> Signed-off-by: Jim Liu <JJLIU0@nuvoton.com>
> ---
> Changes for v2:
>    - modify description
> ---
>  .../bindings/gpio/nuvoton,sgpio.yaml          | 79 +++++++++++++++++++
>  1 file changed, 79 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/nuvoton,sgpio.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/gpio/nuvoton,sgpio.example.dts:35.3-36.1 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:406: Documentation/devicetree/bindings/gpio/nuvoton,sgpio.example.dtb] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1492: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.
  

Hi Jim,

thanks for your patch!

On Tue, Nov 8, 2022 at 10:29 AM Jim Liu <jim.t90615@gmail.com> wrote:
>
> NPCM750 include two SGPIO modules.
> Each module supports up to 64 input and 64 output pins.
> the output pin must be serial to parallel device(such as the hc595)
> the input in must be parallel to serial device(such as the hc165)
>
> Signed-off-by: Jim Liu <JJLIU0@nuvoton.com>
> ---
> Changes for v2:
>    - modify description
(...)

This:

> +  GPIO pins can be programmed to support the following options
> +  - Support interrupt option for each input port and various interrupt
> +    sensitivity option (level-high, level-low, edge-high, edge-low)
> +  - Directly connected to APB bus and its shift clock is from APB bus clock
> +    divided by a programmable value.
> +  - nin_gpios is number of input GPIO lines
> +  - nout_gpios is number of output GPIO lines
> +  - ngpios is number of nin_gpios GPIO lines and nout_gpios GPIO lines.

Why does input/output have to be configured uniquely/static per system?

What is wrong with just using direction_input() and direction_output()
at runtime like everybody else?

> +        nin_gpios = <64>;
> +        nout_gpios = <64>;

Especially since in the example you just set them all to be both input
and output so they all depend on runtime direction configuration
anyway.

Yours,
Linus Walleij
  

On 08/11/2022 10:28, Jim Liu wrote:
> NPCM750 include two SGPIO modules.
> Each module supports up to 64 input and 64 output pins.
> the output pin must be serial to parallel device(such as the hc595)
> the input in must be parallel to serial device(such as the hc165)
> 
> Signed-off-by: Jim Liu <JJLIU0@nuvoton.com>
> ---
> Changes for v2:
>    - modify description
> ---
>  .../bindings/gpio/nuvoton,sgpio.yaml          | 79 +++++++++++++++++++
>  1 file changed, 79 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/nuvoton,sgpio.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpio/nuvoton,sgpio.yaml b/Documentation/devicetree/bindings/gpio/nuvoton,sgpio.yaml
> new file mode 100644
> index 000000000000..331e3cb28b98
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/nuvoton,sgpio.yaml
> @@ -0,0 +1,79 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/nuvoton,sgpio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Nuvoton SGPIO controller
> +
> +maintainers:
> +  - Jim LIU <JJLIU0@nuvoton.com>
> +
> +description:

description: |

> +  This SGPIO controller is for NUVOTON NPCM7xx and NPCM8xx SoC,
> +  NPCM7xx/NPCM8xx have two sgpio module each module can support up
> +  to 64 output pins,and up to 64 input pin.
> +  Nuvoton NPCM750 SGPIO module is base on serial to parallel IC (HC595)
> +  and parallel to serial IC (HC165).
> +  GPIO pins can be programmed to support the following options
> +  - Support interrupt option for each input port and various interrupt
> +    sensitivity option (level-high, level-low, edge-high, edge-low)
> +  - Directly connected to APB bus and its shift clock is from APB bus clock
> +    divided by a programmable value.
> +  - nin_gpios is number of input GPIO lines
> +  - nout_gpios is number of output GPIO lines
> +  - ngpios is number of nin_gpios GPIO lines and nout_gpios GPIO lines.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - nuvoton,npcm750-sgpio
> +      - nuvoton,npcm845-sgpio
> +
> +  reg:
> +    maxItems: 1
> +
> +  gpio-controller: true
> +
> +  '#gpio-cells':
> +    const: 2
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  nin_gpios: true
> +
> +  nout_gpios: true

These have several issues. No underscores, missing type, no description,
missing maxItems (if these were GPIOs...)

> +
> +  bus-frequency: true

Why? Bus frequency of what? This is a property of bus controllers. You
need to explain in details in description what is this about.

> +
> +required:
> +  - compatible
> +  - reg
> +  - gpio-controller
> +  - '#gpio-cells'
> +  - interrupts
> +  - nin_gpios
> +  - nout_gpios
> +  - clocks
> +  - bus-frequency
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/nuvoton,npcm7xx-clock.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    sgpio1: sgpio@101000 {
> +        compatible = "nuvoton,npcm750-sgpio";
> +        reg = <0x101000 0x200>;
> +        clocks = <&clk NPCM7XX_CLK_APB3>;
> +        interrupts = <GIC_SPI 19 IRQ_TYPE_LEVEL_HIGH>;
> +        bus-frequency = <16000000>;
> +        gpio-controller;
> +        #gpio-cells = <2>;
> +        nin_gpios = <64>;
> +        nout_gpios = <64>;
> +        status = "disabled";

Drop


Best regards,
Krzysztof
  

On Tue, Nov 8, 2022 at 10:29 AM Jim Liu <jim.t90615@gmail.com> wrote:

> +  nin_gpios: true
> +
> +  nout_gpios: true

My comment from v1 still holds.
I'd say just drop these two, it's too much trying to protect
the users from themselves.

> +  bus-frequency: true

Given that you have clocks already, what does this actually specify?
Which bus? The one the GPIO is connected to? Why is it different
from the frequency from the clocks? And what is it used for, why does
it need to be specified? So many questions.

A description is necessary.

I guess the : true means it is picked up from the core schemas somehow
but that doesn't make me smarter.

Yours,
Linus Walleij
  

Hi Linus and Krzysztof

This is a special feature of npcm750.
it's not a normal gpio.
It's similar to aspeed sgpio.

The spec as below:

The full name is "serial I/O expansion"  interface.
The NPCM7xx and NPCM8xx include two SGPIO modules.
This interface has 4 pins  (D_out , D_in, S_CLK, LDSH).
Each module includes eight input ports and eight output ports.
Each port can control eight pins.
Input ports only can be input ,output is so on.
So support up to 64 input pins and 64 output pins.

-S_CLK:
The clock is generated by APB3, so users can set the bus frequency and
the driver will set the spgio divided reg to
generate a similar clock to sgpio bus.

-D_out:
the output data is the serial data needed to connect to hc595 and the
data will output to hc595 parallel pins.
you can use dts nout_gpios to create the number of pins.

-D_in
this pin need to connect to hc165 and get the serial data from hc165.
you can use dts nin_gpios to create the number of pins.

LDSH:
this pin is used to get input data or send output data.
the user can't control this pin.
one operation cycle is include input and output
beginning the signal, the  LDSH is low and now will send output serial data ,
after finished output serial data the LDSH will be high and get serial
input data.

If you have any questions or are confused please let me know.
Your comments are most welcome.

Best regards,
Jim


On Wed, Nov 9, 2022 at 5:14 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Tue, Nov 8, 2022 at 10:29 AM Jim Liu <jim.t90615@gmail.com> wrote:
>
> > +  nin_gpios: true
> > +
> > +  nout_gpios: true
>
> My comment from v1 still holds.
> I'd say just drop these two, it's too much trying to protect
> the users from themselves.
>
> > +  bus-frequency: true
>
> Given that you have clocks already, what does this actually specify?
> Which bus? The one the GPIO is connected to? Why is it different
> from the frequency from the clocks? And what is it used for, why does
> it need to be specified? So many questions.
>
> A description is necessary.
>
> I guess the : true means it is picked up from the core schemas somehow
> but that doesn't make me smarter.
>
> Yours,
> Linus Walleij
  

On Fri, Nov 11, 2022 at 10:30 AM Jim Liu <jim.t90615@gmail.com> wrote:

> -D_out:
> the output data is the serial data needed to connect to hc595 and the
> data will output to hc595 parallel pins.
> you can use dts nout_gpios to create the number of pins.
>
> -D_in
> this pin need to connect to hc165 and get the serial data from hc165.
> you can use dts nin_gpios to create the number of pins.

In the example it seems you enable d_out and d_in for *all* 64
pins, correct? So they are all either input or output.

That in effect turns them into GPIOs, so I don't see the problem
with simply always doing this?

Just that things are configurable doesn't mean we always need
to provide means to configure them.

If you have a use case where the user wants to control this, then
that is another thing. Otherwise just make all pins input and output
and wait for a usecase that needs more control, maybe it will
never appear.

Yours,
Linus Walleij
  

Hi Linus

Thanks for your reply.

let me explain the gpio pin as below:

Our sgpio module has 64 pins output and 64 pins input.
Soc have 8 reg to control 64 output pins
and  8 reg to control 64 input pins.
so the pin is only for gpi or gpo.

The common property ngpio can be out or in.
so i need to create d_out and d_in to control it.
customers can set the number of output or input pins to use.
the driver will open the ports to use.
ex: if  i set d_out=9   and d_in=20
driver will open two output ports and three input ports.

Another method  is the driver default opens all ports , in this
situation the driver doesn't need d_out and d_in.


Best regards,
Jim



On Fri, Nov 11, 2022 at 10:20 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Fri, Nov 11, 2022 at 10:30 AM Jim Liu <jim.t90615@gmail.com> wrote:
>
> > -D_out:
> > the output data is the serial data needed to connect to hc595 and the
> > data will output to hc595 parallel pins.
> > you can use dts nout_gpios to create the number of pins.
> >
> > -D_in
> > this pin need to connect to hc165 and get the serial data from hc165.
> > you can use dts nin_gpios to create the number of pins.
>
> In the example it seems you enable d_out and d_in for *all* 64
> pins, correct? So they are all either input or output.
>
> That in effect turns them into GPIOs, so I don't see the problem
> with simply always doing this?
>
> Just that things are configurable doesn't mean we always need
> to provide means to configure them.
>
> If you have a use case where the user wants to control this, then
> that is another thing. Otherwise just make all pins input and output
> and wait for a usecase that needs more control, maybe it will
> never appear.
>
> Yours,
> Linus Walleij
  

On Mon, Nov 14, 2022 at 9:38 AM Jim Liu <jim.t90615@gmail.com> wrote:

> Our sgpio module has 64 pins output and 64 pins input.
> Soc have 8 reg to control 64 output pins
> and  8 reg to control 64 input pins.
> so the pin is only for gpi or gpo.
>
> The common property ngpio can be out or in.
> so i need to create d_out and d_in to control it.
> customers can set the number of output or input pins to use.
> the driver will open the ports to use.
> ex: if  i set d_out=9   and d_in=20
> driver will open two output ports and three input ports.
>
> Another method  is the driver default opens all ports , in this
> situation the driver doesn't need d_out and d_in.

Finally I get it!

Some of the above should go into the binding document so that
others understand it too.

Have you considered splitting this into 2 instances with 2 DT nodes:
one with up to 64 output-only pins and one with up to 64 input-only pins?
That means more nodes in the DT and more compatibles. If all
the registers are in the same place maybe this is not a good
idea.

If you feel you need to keep the two properties, create something custom
for your hardware because this is not generally useful, e.g.

nuvoton,input-ngpios = <...>
nuvoton,output-ngpios = <...>

By this nomenclature it also becomes more evident what is going on.

Yours,
Linus Walleij
  

Hi Linus and Krzysztof

Thanks for your understanding and your suggestion.
I will follow your suggestion to modify the yaml file.
-> nuvoton,input-ngpios = <...>
-> nuvoton,output-ngpios = <...>

And I don't think the node name needs to use gpio.
because it's not a general gpio, so I reference aspeed dts and use sgpio.
Could I use the sgpio node name or could you provide some suggestions?


If you have any questions or are confused please let me know.
Your comments are most welcome.

Best regards,
Jim
On Mon, Nov 14, 2022 at 6:13 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Mon, Nov 14, 2022 at 9:38 AM Jim Liu <jim.t90615@gmail.com> wrote:
>
> > Our sgpio module has 64 pins output and 64 pins input.
> > Soc have 8 reg to control 64 output pins
> > and  8 reg to control 64 input pins.
> > so the pin is only for gpi or gpo.
> >
> > The common property ngpio can be out or in.
> > so i need to create d_out and d_in to control it.
> > customers can set the number of output or input pins to use.
> > the driver will open the ports to use.
> > ex: if  i set d_out=9   and d_in=20
> > driver will open two output ports and three input ports.
> >
> > Another method  is the driver default opens all ports , in this
> > situation the driver doesn't need d_out and d_in.
>
> Finally I get it!
>
> Some of the above should go into the binding document so that
> others understand it too.
>
> Have you considered splitting this into 2 instances with 2 DT nodes:
> one with up to 64 output-only pins and one with up to 64 input-only pins?
> That means more nodes in the DT and more compatibles. If all
> the registers are in the same place maybe this is not a good
> idea.
>
> If you feel you need to keep the two properties, create something custom
> for your hardware because this is not generally useful, e.g.
>
> nuvoton,input-ngpios = <...>
> nuvoton,output-ngpios = <...>
>
> By this nomenclature it also becomes more evident what is going on.
>
> Yours,
> Linus Walleij
  

On 14/11/2022 09:38, Jim Liu wrote:
> Hi Linus
> 
> Thanks for your reply.
> 
> let me explain the gpio pin as below:
> 
> Our sgpio module has 64 pins output and 64 pins input.
> Soc have 8 reg to control 64 output pins
> and  8 reg to control 64 input pins.
> so the pin is only for gpi or gpo.
> 
> The common property ngpio can be out or in.
> so i need to create d_out and d_in to control it.
> customers can set the number of output or input pins to use.

Aren't customers interested in specific pins, not the number? IOW, why
do you assume pins should be set to output in ascending order (e.g. 1, 2
and 3) and not in a flexible way?

> the driver will open the ports to use.
> ex: if  i set d_out=9   and d_in=20

Why 20 means three input ports? 9 opening two outputs could have sense
if it was a mask but you did not say it is a mask.



Best regards,
Krzysztof
  

On 15/11/2022 10:21, Jim Liu wrote:
> Hi Linus and Krzysztof
> 
> Thanks for your understanding and your suggestion.
> I will follow your suggestion to modify the yaml file.
> -> nuvoton,input-ngpios = <...>
> -> nuvoton,output-ngpios = <...>
> 
> And I don't think the node name needs to use gpio.
> because it's not a general gpio, so I reference aspeed dts and use sgpio.
> Could I use the sgpio node name or could you provide some suggestions?

Aspeed DTS has poor code readability (not following several common DT
conventions), so using it as an example or argument is not correct
approach. Nodes have name "gpio" for GPIO controllers or one of
pinctrl.yaml for pin controllers.

Best regards,
Krzysztof
  

Hi Krzysztof

Thanks for your reply.

Our sgpio has 8  regs to control output and 8 regs to control input.
Each reg size is one byte.
and the sgpio interface has 4 pins(s_clk, d_out, d_in, LDSH).

The clock is generated by APB3, and one operation cycle includes input
and output
beginning the signal, the  LDSH is low and now will send output serial data ,
after finished output serial data the LDSH will be high and get serial
input data.

The in/out serial data size is byte * ports , and direct to update the regs.

> the driver will open the ports to use.
> ex: if  i set d_out=9   and d_in=20

The Soc is controlled by port, not by each bit.
So if users need 9 output pins, the driver needs to open two ports,
because each reg is one byte.
if users need 20 input pins ,the driver needs to open three ports.

The list has some rules for use, The first half is the output and the
second half is the input.
the example as below:
if i set d_out=8  d_in=8

root@buv-runbmc:~# gpioinfo 8
gpiochip8 - 16 lines:
        line   0:      unnamed       unused  output  active-high
        line   1:      unnamed       unused  output  active-high
        line   2:      unnamed       unused  output  active-high
        line   3:      unnamed       unused  output  active-high
        line   4:      unnamed       unused  output  active-high
        line   5:      unnamed       unused  output  active-high
        line   6:      unnamed       unused  output  active-high
        line   7:      unnamed       unused  output  active-high
        line   8:      unnamed       unused   input  active-high
        line   9:      unnamed       unused   input  active-high
        line  10:      unnamed       unused   input  active-high
        line  11:      unnamed       unused   input  active-high
        line  12:      unnamed       unused   input  active-high
        line  13:      unnamed       unused   input  active-high
        line  14:      unnamed       unused   input  active-high
        line  15:      unnamed       unused   input  active-high

the line0~line7 will map to output reg1 and line8~line15 will map to
input reg1 and so on.

and thanks again for your suggestions and information for dts naming.
So I need to modify it from sgpio1 to gpio8
am i correct?

Best regards,
Jim






On Tue, Nov 15, 2022 at 5:58 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 15/11/2022 10:21, Jim Liu wrote:
> > Hi Linus and Krzysztof
> >
> > Thanks for your understanding and your suggestion.
> > I will follow your suggestion to modify the yaml file.
> > -> nuvoton,input-ngpios = <...>
> > -> nuvoton,output-ngpios = <...>
> >
> > And I don't think the node name needs to use gpio.
> > because it's not a general gpio, so I reference aspeed dts and use sgpio.
> > Could I use the sgpio node name or could you provide some suggestions?
>
> Aspeed DTS has poor code readability (not following several common DT
> conventions), so using it as an example or argument is not correct
> approach. Nodes have name "gpio" for GPIO controllers or one of
> pinctrl.yaml for pin controllers.
>
> Best regards,
> Krzysztof
>