[RFC,0/6] Add Pinctrl driver for Starfive JH8100 SoC

  Starfive JH8100 SoC consists of 4 pinctrl domains - sys_east,
sys_west, sys_gmac, and aon. This patch series adds pinctrl
drivers for these 4 pinctrl domains and this patch series is
depending on the JH8100 base patch series in [1] and [2].
The relevant dt-binding documentation for each pinctrl domain has
been updated accordingly.

[1] https://lore.kernel.org/lkml/20231201121410.95298-1-jeeheng.sia@starfivetech.com/
[2] https://lore.kernel.org/lkml/20231206115000.295825-1-jeeheng.sia@starfivetech.com/

Alex Soo (6):
  dt-bindings: pinctrl: starfive: add JH8100 pinctrl bindings
  pinctrl: starfive: jh8100: add pinctrl driver for sys_east domain
  pinctrl: starfive: jh8100: add pinctrl driver for sys_west domain
  pinctrl: starfive: jh8100: add pinctrl driver for sys_gmac domain
  pinctrl: starfive: jh8100: add pinctrl driver for AON domain
  riscv: dts: starfive: jh8100: add pinctrl device tree nodes

 .../pinctrl/starfive,jh8100-aon-pinctrl.yaml  |  183 +++
 .../starfive,jh8100-sys-east-pinctrl.yaml     |  188 +++
 .../starfive,jh8100-sys-gmac-pinctrl.yaml     |  124 ++
 .../starfive,jh8100-sys-west-pinctrl.yaml     |  188 +++
 MAINTAINERS                                   |    7 +
 arch/riscv/boot/dts/starfive/jh8100-evb.dts   |    5 +
 arch/riscv/boot/dts/starfive/jh8100-pinfunc.h |  418 +++++++
 arch/riscv/boot/dts/starfive/jh8100.dtsi      |   44 +
 drivers/pinctrl/starfive/Kconfig              |   57 +
 drivers/pinctrl/starfive/Makefile             |    6 +
 .../starfive/pinctrl-starfive-jh8100-aon.c    |  241 ++++
 .../pinctrl-starfive-jh8100-sys-east.c        |  326 +++++
 .../pinctrl-starfive-jh8100-sys-gmac.c        |  164 +++
 .../pinctrl-starfive-jh8100-sys-west.c        |  264 ++++
 .../starfive/pinctrl-starfive-jh8100.c        | 1090 +++++++++++++++++
 .../starfive/pinctrl-starfive-jh8100.h        |   85 ++
 .../pinctrl/starfive,jh8100-pinctrl.h         |  303 +++++
 17 files changed, 3693 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/starfive,jh8100-aon-pinctrl.yaml
 create mode 100644 Documentation/devicetree/bindings/pinctrl/starfive,jh8100-sys-east-pinctrl.yaml
 create mode 100644 Documentation/devicetree/bindings/pinctrl/starfive,jh8100-sys-gmac-pinctrl.yaml
 create mode 100644 Documentation/devicetree/bindings/pinctrl/starfive,jh8100-sys-west-pinctrl.yaml
 create mode 100644 arch/riscv/boot/dts/starfive/jh8100-pinfunc.h
 create mode 100644 drivers/pinctrl/starfive/pinctrl-starfive-jh8100-aon.c
 create mode 100644 drivers/pinctrl/starfive/pinctrl-starfive-jh8100-sys-east.c
 create mode 100644 drivers/pinctrl/starfive/pinctrl-starfive-jh8100-sys-gmac.c
 create mode 100644 drivers/pinctrl/starfive/pinctrl-starfive-jh8100-sys-west.c
 create mode 100644 drivers/pinctrl/starfive/pinctrl-starfive-jh8100.c
 create mode 100644 drivers/pinctrl/starfive/pinctrl-starfive-jh8100.h
 create mode 100644 include/dt-bindings/pinctrl/starfive,jh8100-pinctrl.h


base-commit: ddb42e41c0991b443dd6cc97213b478a7324d72b
  

On 21/12/2023 09:36, Alex Soo wrote:
> Starfive JH8100 SoC consists of 4 pinctrl domains - sys_east,
> sys_west, sys_gmac, and aon. This patch series adds pinctrl
> drivers for these 4 pinctrl domains and this patch series is
> depending on the JH8100 base patch series in [1] and [2].
> The relevant dt-binding documentation for each pinctrl domain has
> been updated accordingly.

Please explain why this is RFC. Every patch is RFC, so what is special
about here? Usually this means work is not finished and should not be
merged, neither reviewed. If you spelled out here the reasons, it would
be easier for us to understand whether we should complain about broken
and non-building code or not.

Best regards,
Krzysztof
  

Hi Alex,

thanks for your patch!

On Thu, Dec 21, 2023 at 9:36 AM Alex Soo <yuklin.soo@starfivetech.com> wrote:

>   pinctrl: starfive: jh8100: add pinctrl driver for sys_east domain
>   pinctrl: starfive: jh8100: add pinctrl driver for sys_west domain
>   pinctrl: starfive: jh8100: add pinctrl driver for sys_gmac domain
>   pinctrl: starfive: jh8100: add pinctrl driver for AON domain

To my eye it looks like a lot of code is duplicated between the four subdrivers.

The pattern from other pin controllers is to create a file with all the common
code and then subdrivers for each sub-pincontroller that have their own
probe but calls into the library.

C.f.
drivers/pinctrl/qcom/pinctrl-apq8064.c:

static int apq8064_pinctrl_probe(struct platform_device *pdev)
{
        return msm_pinctrl_probe(pdev, &apq8064_pinctrl);
}

And that function is in drivers/pinctrl/qcom/pinctrl-msm.c
and you find great inspiration in the qcom Kconfig and Makefile
and drivers/pinctrl/qcom/pinctrl-msm.h
that you can copypaste to pull this off.

Maybe you should start with a patch that extract the common stuff
from the existing jh7100/jh7110 drivers and then reuse that for
jh8100?

Yours,
Linus Walleij
  

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Friday, December 22, 2023 12:20 AM
> To: Yuklin Soo <yuklin.soo@starfivetech.com>; Linus Walleij
> <linus.walleij@linaro.org>; Bartosz Golaszewski
> <bartosz.golaszewski@linaro.org>; Hal Feng <hal.feng@starfivetech.com>;
> Leyfoon Tan <leyfoon.tan@starfivetech.com>; Jianlong Huang
> <jianlong.huang@starfivetech.com>; Emil Renner Berthing
> <kernel@esmil.dk>; Rob Herring <robh@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>;
> Drew Fustini <drew@beagleboard.org>
> Cc: linux-gpio@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; linux-riscv@lists.infradead.org; Paul Walmsley
> <paul.walmsley@sifive.com>; Palmer Dabbelt <palmer@dabbelt.com>;
> Albert Ou <aou@eecs.berkeley.edu>
> Subject: Re: [RFC PATCH 0/6] Add Pinctrl driver for Starfive JH8100 SoC
> 
> On 21/12/2023 09:36, Alex Soo wrote:
> > Starfive JH8100 SoC consists of 4 pinctrl domains - sys_east,
> > sys_west, sys_gmac, and aon. This patch series adds pinctrl drivers
> > for these 4 pinctrl domains and this patch series is depending on the
> > JH8100 base patch series in [1] and [2].
> > The relevant dt-binding documentation for each pinctrl domain has been
> > updated accordingly.
> 
> Please explain why this is RFC. Every patch is RFC, so what is special about
> here? Usually this means work is not finished and should not be merged,
> neither reviewed. If you spelled out here the reasons, it would be easier for
> us to understand whether we should complain about broken and non-
> building code or not.

This JH8100 SoC pinctrl patch is dependent on the following: 
-	Initial device tree support and dt-bindings for JH8100 SoC
https://lore.kernel.org/lkml/20231214-platonic-unhearing-27e2ec3d8f75@spud/
-	Clock & Reset Support for JH8100 SoC
 https://lore.kernel.org/lkml/20231206115000.295825-1-jeeheng.sia@starfivetech.com/
 

Refer to the first link, there is maintainer feedback that if our evaluation platform is FPGA-based (since actual silicon is still unavailable), they are not keen on merging the patches, and things like pinctrl or clock drivers should first be submitted as “not to be merged”, in other words, as RFC patches.

> 
> Best regards,
> Krzysztof
  

> -----Original Message-----
> From: Linus Walleij <linus.walleij@linaro.org>
> Sent: Saturday, December 23, 2023 1:59 AM
> To: Yuklin Soo <yuklin.soo@starfivetech.com>
> Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Hal Feng
> <hal.feng@starfivetech.com>; Leyfoon Tan <leyfoon.tan@starfivetech.com>;
> Jianlong Huang <jianlong.huang@starfivetech.com>; Emil Renner Berthing
> <kernel@esmil.dk>; Rob Herring <robh@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>;
> Drew Fustini <drew@beagleboard.org>; linux-gpio@vger.kernel.org; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-
> riscv@lists.infradead.org; Paul Walmsley <paul.walmsley@sifive.com>;
> Palmer Dabbelt <palmer@dabbelt.com>; Albert Ou
> <aou@eecs.berkeley.edu>
> Subject: Re: [RFC PATCH 0/6] Add Pinctrl driver for Starfive JH8100 SoC
> 
> Hi Alex,
> 
> thanks for your patch!
> 
> On Thu, Dec 21, 2023 at 9:36 AM Alex Soo <yuklin.soo@starfivetech.com>
> wrote:
> 
> >   pinctrl: starfive: jh8100: add pinctrl driver for sys_east domain
> >   pinctrl: starfive: jh8100: add pinctrl driver for sys_west domain
> >   pinctrl: starfive: jh8100: add pinctrl driver for sys_gmac domain
> >   pinctrl: starfive: jh8100: add pinctrl driver for AON domain
> 
> To my eye it looks like a lot of code is duplicated between the four subdrivers.
> 
> The pattern from other pin controllers is to create a file with all the common
> code and then subdrivers for each sub-pincontroller that have their own
> probe but calls into the library.
> 
> C.f.
> drivers/pinctrl/qcom/pinctrl-apq8064.c:
> 
> static int apq8064_pinctrl_probe(struct platform_device *pdev) {
>         return msm_pinctrl_probe(pdev, &apq8064_pinctrl); }
> 
> And that function is in drivers/pinctrl/qcom/pinctrl-msm.c
> and you find great inspiration in the qcom Kconfig and Makefile and
> drivers/pinctrl/qcom/pinctrl-msm.h
> that you can copypaste to pull this off.
> 
> Maybe you should start with a patch that extract the common stuff from the
> existing jh7100/jh7110 drivers and then reuse that for jh8100?

The duplicated codes in the subdrivers have been moved to the main driver.
The subdrivers will call those functions from the main driver.
The test results are successful and will submit in the next version for review.

> 
> Yours,
> Linus Walleij