| Message ID | 20231221083622.3445726-1-yuklin.soo@starfivetech.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add Pinctrl driver for Starfive JH8100 SoC | expand |
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