mbox series

[v5,0/3] Add Nuvoton NPCM SGPIO feature

Message ID 20230314092311.8924-1-jim.t90615@gmail.com
Headers show
Series Add Nuvoton NPCM SGPIO feature | expand

Message

Jim Liu March 14, 2023, 9:23 a.m. UTC 
This SGPIO controller is for NUVOTON NPCM7xx and NPCM8xx SoC.
Nuvoton NPCM SGPIO module is combine serial to parallel IC (HC595)
and parallel to serial IC (HC165), and use APB3 clock to control it.
This interface has 4 pins  (D_out , D_in, S_CLK, LDSH).
NPCM7xx/NPCM8xx have two sgpio module each module can support up
to 64 output pins,and up to 64 input pin, the pin is only for gpi or gpo.
GPIO pins have sequential, First half is gpo and second half is gpi.


Jim Liu (3):
  gpio: nuvoton: Add Nuvoton NPCM sgpio driver
  arm: dts: nuvoton: npcm: Add sgpio feature
  dt-bindings: gpio: add NPCM sgpio driver bindings

 .../bindings/gpio/nuvoton,sgpio.yaml          |  87 +++
 arch/arm/boot/dts/nuvoton-common-npcm7xx.dtsi |  30 +
 drivers/gpio/Kconfig                          |   8 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-npcm-sgpio.c                | 648 ++++++++++++++++++
 5 files changed, 774 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/nuvoton,sgpio.yaml
 create mode 100644 drivers/gpio/gpio-npcm-sgpio.c

Comments

Krzysztof Kozlowski March 14, 2023, 6:47 p.m. UTC | #1 
On 14/03/2023 10:23, Jim Liu wrote:
> Add the SGPIO controller to the NPCM7xx devicetree
> 
> Signed-off-by: Jim Liu <jim.t90615@gmail.com>
> ---
> Changes for v5:
>    - remove npcm8xx node
> Changes for v4:
>    - add npcm8xx gpio node
> Changes for v3:
>    - modify node name
>    - modify in/out property name
> Changes for v2:
>    - modify dts node
> ---
>  arch/arm/boot/dts/nuvoton-common-npcm7xx.dtsi | 30 +++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/nuvoton-common-npcm7xx.dtsi b/arch/arm/boot/dts/nuvoton-common-npcm7xx.dtsi
> index c7b5ef15b716..7f53774a01ec 100644
> --- a/arch/arm/boot/dts/nuvoton-common-npcm7xx.dtsi
> +++ b/arch/arm/boot/dts/nuvoton-common-npcm7xx.dtsi
> @@ -330,6 +330,36 @@
>  				status = "disabled";
>  			};
>  
> +			gpio8: gpio@101000 {
> +				compatible = "nuvoton,npcm750-sgpio";
> +				reg = <0x101000 0x200>;
> +				clocks = <&clk NPCM7XX_CLK_APB3>;
> +				interrupts = <GIC_SPI 19 IRQ_TYPE_LEVEL_HIGH>;
> +				bus-frequency = <8000000>;

Does not look like you tested the bindings. Please run `make
dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).

> +				gpio-controller;
> +				#gpio-cells = <2>;
> +				pinctrl-names = "default";
> +				pinctrl-0 = <&iox1_pins>;
> +				nuvoton,input-ngpios = <64>;
> +				nuvoton,output-ngpios = <64>;
> +				status = "disabled";
> +			};
> +
> +			gpio9: gpio@102000 {
> +				compatible = "nuvoton,npcm750-sgpio";
> +				reg = <0x102000 0x200>;
> +				clocks = <&clk NPCM7XX_CLK_APB3>;
> +				interrupts = <GIC_SPI 20 IRQ_TYPE_LEVEL_HIGH>;
> +				bus-frequency = <8000000>;

Does not look like you tested the bindings. Please run `make
dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).



Best regards,
Krzysztof
Krzysztof Kozlowski March 14, 2023, 6:49 p.m. UTC | #2 
On 14/03/2023 19:47, Krzysztof Kozlowski wrote:
> On 14/03/2023 10:23, Jim Liu wrote:
>> Add the SGPIO controller to the NPCM7xx devicetree
>>
>> Signed-off-by: Jim Liu <jim.t90615@gmail.com>
>> ---
>> Changes for v5:
>>    - remove npcm8xx node
>> Changes for v4:
>>    - add npcm8xx gpio node
>> Changes for v3:
>>    - modify node name
>>    - modify in/out property name
>> Changes for v2:
>>    - modify dts node
>> ---
>>  arch/arm/boot/dts/nuvoton-common-npcm7xx.dtsi | 30 +++++++++++++++++++
>>  1 file changed, 30 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/nuvoton-common-npcm7xx.dtsi b/arch/arm/boot/dts/nuvoton-common-npcm7xx.dtsi
>> index c7b5ef15b716..7f53774a01ec 100644
>> --- a/arch/arm/boot/dts/nuvoton-common-npcm7xx.dtsi
>> +++ b/arch/arm/boot/dts/nuvoton-common-npcm7xx.dtsi
>> @@ -330,6 +330,36 @@
>>  				status = "disabled";
>>  			};
>>  
>> +			gpio8: gpio@101000 {
>> +				compatible = "nuvoton,npcm750-sgpio";
>> +				reg = <0x101000 0x200>;
>> +				clocks = <&clk NPCM7XX_CLK_APB3>;
>> +				interrupts = <GIC_SPI 19 IRQ_TYPE_LEVEL_HIGH>;
>> +				bus-frequency = <8000000>;
> 
> Does not look like you tested the bindings. Please run `make
> dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).

Eh, wrong key shortcut. Should be:

Does not look like you tested the DTS against bindings. Please run `make
dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst
for instructions).

Best regards,
Krzysztof
Krzysztof Kozlowski March 15, 2023, 7:54 a.m. UTC | #3 
On 14/03/2023 19:45, Krzysztof Kozlowski wrote:
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/nuvoton,npcm7xx-clock.h>
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    gpio8: gpio@101000 {
>> +        compatible = "nuvoton,npcm750-sgpio";
>> +        reg = <0x101000 0x200>;
>> +        clocks = <&clk NPCM7XX_CLK_APB3>;
>> +        interrupts = <GIC_SPI 19 IRQ_TYPE_LEVEL_HIGH>;
>> +        gpio-controller;
>> +        #gpio-cells = <2>;
>> +        nuvoton,input-ngpios = <64>;
>> +        nuvoton,output-ngpios = <64>;
>> +        status = "disabled";
> 
> So this is fifth reminder...
> 
> https://lore.kernel.org/all/d56c24c2-a017-8468-0b3a-bd93d6024c69@linaro.org/
> 
> https://lore.kernel.org/all/39efdb85-f881-12ab-e258-61175f209b4c@linaro.org/
> 
> https://lore.kernel.org/all/9fc4d874-a0d0-6c5c-aeee-61ab817fdd9f@linaro.org/
> 
> This is a not-that-friendly-anymore reminder during the review process.
> 
> It seems my previous comments were not fully addressed. Maybe my
> feedback got lost between the quotes, maybe you just forgot to apply it.
> Please go back to the previous discussion and either implement all
> requested changes or keep discussing them.

BTW, you never responded that my comment is unclear, but I can imagine
that this was the cause of some misunderstanding. Therefore just in case
let's be clear: drop the "status line". Entire line.

Best regards,
Krzysztof