| Message ID | 20170109113621.31d384c9@endymion (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hi Jean, Am 09.01.2017 um 11:36 schrieb Jean Delvare: > If I say "no" to "Clock driver for Mediatek MT2701", I don't want to > be asked individually about each sub-driver. No means no. > > Additionally, this driver shouldn't be proposed at all on non-mediatek > builds, unless build-testing. > > Signed-off-by: Jean Delvare <jdelvare@suse.de> > Fixes: e9862118272a ("clk: mediatek: Add MT2701 clock support") > Cc: Shunli Wang <shunli.wang@mediatek.com> > Cc: James Liao <jamesjj.liao@mediatek.com> > Cc: Erin Lo <erin.lo@mediatek.com> > Cc: Stephen Boyd <sboyd@codeaurora.org> > Cc: Michael Turquette <mturquette@baylibre.com> > Cc: Matthias Brugger <matthias.bgg@gmail.com> > --- [...] > As another side note, I wonder why so many clock drivers have > "COMMON" in their symbol names. Looks wrong to me. It refers to the Common Clock Framework: https://www.kernel.org/doc/Documentation/clk.txt > --- linux-4.10-rc2.orig/drivers/clk/mediatek/Kconfig 2017-01-01 23:31:53.000000000 +0100 > +++ linux-4.10-rc2/drivers/clk/mediatek/Kconfig 2017-01-09 11:17:37.542344083 +0100 > @@ -8,6 +8,7 @@ config COMMON_CLK_MEDIATEK > > config COMMON_CLK_MT2701 > bool "Clock driver for Mediatek MT2701" > + depends on ARCH_MEDIATEK || COMPILE_TEST > select COMMON_CLK_MEDIATEK > default ARCH_MEDIATEK Should the default then become y for simplicity? Another aspect here is that this is a 32-bit SoC but it propagates into the arm64 configs, so maybe (ARCH_MEDIATEK && !ARM64) || COMPILE_TEST? Same for mt2701 pinctrl. http://kernel.opensuse.org/cgit/kernel-source/plain/config/arm64/default?id=ff90e915117c5d7a8bb00dc0bc1d3145ebe985ec > ---help--- > @@ -15,37 +16,37 @@ config COMMON_CLK_MT2701 > > config COMMON_CLK_MT2701_MMSYS > bool "Clock driver for Mediatek MT2701 mmsys" > - select COMMON_CLK_MT2701 > + depends on COMMON_CLK_MT2701 > ---help--- > This driver supports Mediatek MT2701 mmsys clocks. > > config COMMON_CLK_MT2701_IMGSYS > bool "Clock driver for Mediatek MT2701 imgsys" > - select COMMON_CLK_MT2701 > + depends on COMMON_CLK_MT2701 > ---help--- > This driver supports Mediatek MT2701 imgsys clocks. > > config COMMON_CLK_MT2701_VDECSYS > bool "Clock driver for Mediatek MT2701 vdecsys" > - select COMMON_CLK_MT2701 > + depends on COMMON_CLK_MT2701 > ---help--- > This driver supports Mediatek MT2701 vdecsys clocks. > > config COMMON_CLK_MT2701_HIFSYS > bool "Clock driver for Mediatek MT2701 hifsys" > - select COMMON_CLK_MT2701 > + depends on COMMON_CLK_MT2701 > ---help--- > This driver supports Mediatek MT2701 hifsys clocks. > > config COMMON_CLK_MT2701_ETHSYS > bool "Clock driver for Mediatek MT2701 ethsys" > - select COMMON_CLK_MT2701 > + depends on COMMON_CLK_MT2701 > ---help--- > This driver supports Mediatek MT2701 ethsys clocks. > > config COMMON_CLK_MT2701_BDPSYS > bool "Clock driver for Mediatek MT2701 bdpsys" > - select COMMON_CLK_MT2701 > + depends on COMMON_CLK_MT2701 > ---help--- > This driver supports Mediatek MT2701 bdpsys clocks. > Anyway, a step forward, Reviewed-by: Andreas Färber <afaerber@suse.de> Thanks, Andreas
Hi Jean, On Mon, 2017-01-09 at 11:36 +0100, Jean Delvare wrote: > If I say "no" to "Clock driver for Mediatek MT2701", I don't want to > be asked individually about each sub-driver. No means no. > > Additionally, this driver shouldn't be proposed at all on non-mediatek > builds, unless build-testing. > > Signed-off-by: Jean Delvare <jdelvare@suse.de> > Fixes: e9862118272a ("clk: mediatek: Add MT2701 clock support") > Cc: Shunli Wang <shunli.wang@mediatek.com> > Cc: James Liao <jamesjj.liao@mediatek.com> > Cc: Erin Lo <erin.lo@mediatek.com> > Cc: Stephen Boyd <sboyd@codeaurora.org> > Cc: Michael Turquette <mturquette@baylibre.com> > Cc: Matthias Brugger <matthias.bgg@gmail.com> Reviewed-by: James Liao <jamesjj.liao@mediatek.com> > --- > As a side note, is there any rationale for splitting this driver into > that many small sub-drivers? Looks overengineered to me. These are 'subsystem' clocks and can be controlled only if their corresponding drivers are ready to control power domains and clocks. > As another side note, I wonder why so many clock drivers have > "COMMON" in their symbol names. Looks wrong to me. CCF (Common Clock Framework) uses option 'COMMON_CLK', so CCF platform drivers use 'COMMON_CLK_' to be their option prefix. > drivers/clk/mediatek/Kconfig | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > --- linux-4.10-rc2.orig/drivers/clk/mediatek/Kconfig 2017-01-01 23:31:53.000000000 +0100 > +++ linux-4.10-rc2/drivers/clk/mediatek/Kconfig 2017-01-09 11:17:37.542344083 +0100 > @@ -8,6 +8,7 @@ config COMMON_CLK_MEDIATEK > > config COMMON_CLK_MT2701 > bool "Clock driver for Mediatek MT2701" > + depends on ARCH_MEDIATEK || COMPILE_TEST > select COMMON_CLK_MEDIATEK > default ARCH_MEDIATEK > ---help--- > @@ -15,37 +16,37 @@ config COMMON_CLK_MT2701 > > config COMMON_CLK_MT2701_MMSYS > bool "Clock driver for Mediatek MT2701 mmsys" > - select COMMON_CLK_MT2701 > + depends on COMMON_CLK_MT2701 > ---help--- > This driver supports Mediatek MT2701 mmsys clocks. > > config COMMON_CLK_MT2701_IMGSYS > bool "Clock driver for Mediatek MT2701 imgsys" > - select COMMON_CLK_MT2701 > + depends on COMMON_CLK_MT2701 > ---help--- > This driver supports Mediatek MT2701 imgsys clocks. > > config COMMON_CLK_MT2701_VDECSYS > bool "Clock driver for Mediatek MT2701 vdecsys" > - select COMMON_CLK_MT2701 > + depends on COMMON_CLK_MT2701 > ---help--- > This driver supports Mediatek MT2701 vdecsys clocks. > > config COMMON_CLK_MT2701_HIFSYS > bool "Clock driver for Mediatek MT2701 hifsys" > - select COMMON_CLK_MT2701 > + depends on COMMON_CLK_MT2701 > ---help--- > This driver supports Mediatek MT2701 hifsys clocks. > > config COMMON_CLK_MT2701_ETHSYS > bool "Clock driver for Mediatek MT2701 ethsys" > - select COMMON_CLK_MT2701 > + depends on COMMON_CLK_MT2701 > ---help--- > This driver supports Mediatek MT2701 ethsys clocks. > > config COMMON_CLK_MT2701_BDPSYS > bool "Clock driver for Mediatek MT2701 bdpsys" > - select COMMON_CLK_MT2701 > + depends on COMMON_CLK_MT2701 > ---help--- > This driver supports Mediatek MT2701 bdpsys clocks. > > > -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 9 Jan 2017 21:08:50 +0100, Andreas Färber wrote: > Hi Jean, > > Am 09.01.2017 um 11:36 schrieb Jean Delvare: > > If I say "no" to "Clock driver for Mediatek MT2701", I don't want to > > be asked individually about each sub-driver. No means no. > > > > Additionally, this driver shouldn't be proposed at all on non-mediatek > > builds, unless build-testing. > > > > Signed-off-by: Jean Delvare <jdelvare@suse.de> > > Fixes: e9862118272a ("clk: mediatek: Add MT2701 clock support") > > Cc: Shunli Wang <shunli.wang@mediatek.com> > > Cc: James Liao <jamesjj.liao@mediatek.com> > > Cc: Erin Lo <erin.lo@mediatek.com> > > Cc: Stephen Boyd <sboyd@codeaurora.org> > > Cc: Michael Turquette <mturquette@baylibre.com> > > Cc: Matthias Brugger <matthias.bgg@gmail.com> > > --- > [...] > > As another side note, I wonder why so many clock drivers have > > "COMMON" in their symbol names. Looks wrong to me. > > It refers to the Common Clock Framework: > https://www.kernel.org/doc/Documentation/clk.txt OK, thanks for the explanation. Still seems overkill to me to prefix everything with COMMON_CLK when the drivers live under drivers/clk, but oh well :-) > > --- linux-4.10-rc2.orig/drivers/clk/mediatek/Kconfig 2017-01-01 23:31:53.000000000 +0100 > > +++ linux-4.10-rc2/drivers/clk/mediatek/Kconfig 2017-01-09 11:17:37.542344083 +0100 > > @@ -8,6 +8,7 @@ config COMMON_CLK_MEDIATEK > > > > config COMMON_CLK_MT2701 > > bool "Clock driver for Mediatek MT2701" > > + depends on ARCH_MEDIATEK || COMPILE_TEST > > select COMMON_CLK_MEDIATEK > > default ARCH_MEDIATEK > > Should the default then become y for simplicity? I left it as is as it is the same already done in other drivers in the same directory. I agree "default y" would do the same in practice. > Another aspect here is that this is a 32-bit SoC but it propagates into > the arm64 configs, so maybe (ARCH_MEDIATEK && !ARM64) || COMPILE_TEST? > > Same for mt2701 pinctrl. > > http://kernel.opensuse.org/cgit/kernel-source/plain/config/arm64/default?id=ff90e915117c5d7a8bb00dc0bc1d3145ebe985ec Actually I thought the driver was needed primarily on arm64 because of this configuration file. If that's not the case then I can resubmit with the suggested change, no problem. What about MT8135 and MT8173, are they 32-bit SoCs as well? > (...) > Anyway, a step forward, > > Reviewed-by: Andreas Färber <afaerber@suse.de> Thanks for the review.
On Tue, 2017-01-10 at 13:03 +0100, Jean Delvare wrote: > On Mon, 9 Jan 2017 21:08:50 +0100, Andreas Färber wrote: > > Hi Jean, > > > > Am 09.01.2017 um 11:36 schrieb Jean Delvare: > > > If I say "no" to "Clock driver for Mediatek MT2701", I don't want to > > > be asked individually about each sub-driver. No means no. > > > > > > Additionally, this driver shouldn't be proposed at all on non-mediatek > > > builds, unless build-testing. > > > > > > Signed-off-by: Jean Delvare <jdelvare@suse.de> > > > Fixes: e9862118272a ("clk: mediatek: Add MT2701 clock support") > > > Cc: Shunli Wang <shunli.wang@mediatek.com> > > > Cc: James Liao <jamesjj.liao@mediatek.com> > > > Cc: Erin Lo <erin.lo@mediatek.com> > > > Cc: Stephen Boyd <sboyd@codeaurora.org> > > > Cc: Michael Turquette <mturquette@baylibre.com> > > > Cc: Matthias Brugger <matthias.bgg@gmail.com> > > > --- > > [...] > > > As another side note, I wonder why so many clock drivers have > > > "COMMON" in their symbol names. Looks wrong to me. > > > > It refers to the Common Clock Framework: > > https://www.kernel.org/doc/Documentation/clk.txt > > OK, thanks for the explanation. Still seems overkill to me to prefix > everything with COMMON_CLK when the drivers live under drivers/clk, but > oh well :-) > > > > --- linux-4.10-rc2.orig/drivers/clk/mediatek/Kconfig 2017-01-01 23:31:53.000000000 +0100 > > > +++ linux-4.10-rc2/drivers/clk/mediatek/Kconfig 2017-01-09 11:17:37.542344083 +0100 > > > @@ -8,6 +8,7 @@ config COMMON_CLK_MEDIATEK > > > > > > config COMMON_CLK_MT2701 > > > bool "Clock driver for Mediatek MT2701" > > > + depends on ARCH_MEDIATEK || COMPILE_TEST > > > select COMMON_CLK_MEDIATEK > > > default ARCH_MEDIATEK > > > > Should the default then become y for simplicity? > > I left it as is as it is the same already done in other drivers in the > same directory. I agree "default y" would do the same in practice. > > > Another aspect here is that this is a 32-bit SoC but it propagates into > > the arm64 configs, so maybe (ARCH_MEDIATEK && !ARM64) || COMPILE_TEST? > > > > Same for mt2701 pinctrl. > > > > http://kernel.opensuse.org/cgit/kernel-source/plain/config/arm64/default?id=ff90e915117c5d7a8bb00dc0bc1d3145ebe985ec > > Actually I thought the driver was needed primarily on arm64 because of > this configuration file. If that's not the case then I can resubmit > with the suggested change, no problem. > > What about MT8135 and MT8173, are they 32-bit SoCs as well? MT8135 is a 32-bit SoC and MT8173 is a 64-bit SoC. > > (...) > > Anyway, a step forward, > > > > Reviewed-by: Andreas Färber <afaerber@suse.de> > > Thanks for the review. > -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hallo Andreas, On Mon, 9 Jan 2017 21:08:50 +0100, Andreas Färber wrote: > Another aspect here is that this is a 32-bit SoC but it propagates into > the arm64 configs, so maybe (ARCH_MEDIATEK && !ARM64) || COMPILE_TEST? > > Same for mt2701 pinctrl. I took a look and the pinctrl situation is different: config PINCTRL_MT2701 bool "Mediatek MT2701 pin control" if COMPILE_TEST && !MACH_MT2701 (...) default MACH_MT2701 So the user will not be prompt about it on ARM64 (unless build-testing) because MACH_MT2701 isn't set on ARM64. The only issue with this construct is that you end up with useless symbols defined in the configuration file (they can only be "n".) So I would argue that the following is preferred: config PINCTRL_MT2701 bool "Mediatek MT2701 pin control" (...) depends on MACH_MT2701 || COMPILE_TEST default MACH_MT2701 And same for the other 3 PINCTRL_MT* options. Yes, that means the user will be asked about the options on Mediatek kernels, but actually I believe this is desirable, as advanced users should be allowed to disable specific drivers if they know what they are doing. Other users can just stick to the default.
--- linux-4.10-rc2.orig/drivers/clk/mediatek/Kconfig 2017-01-01 23:31:53.000000000 +0100 +++ linux-4.10-rc2/drivers/clk/mediatek/Kconfig 2017-01-09 11:17:37.542344083 +0100 @@ -8,6 +8,7 @@ config COMMON_CLK_MEDIATEK config COMMON_CLK_MT2701 bool "Clock driver for Mediatek MT2701" + depends on ARCH_MEDIATEK || COMPILE_TEST select COMMON_CLK_MEDIATEK default ARCH_MEDIATEK ---help--- @@ -15,37 +16,37 @@ config COMMON_CLK_MT2701 config COMMON_CLK_MT2701_MMSYS bool "Clock driver for Mediatek MT2701 mmsys" - select COMMON_CLK_MT2701 + depends on COMMON_CLK_MT2701 ---help--- This driver supports Mediatek MT2701 mmsys clocks. config COMMON_CLK_MT2701_IMGSYS bool "Clock driver for Mediatek MT2701 imgsys" - select COMMON_CLK_MT2701 + depends on COMMON_CLK_MT2701 ---help--- This driver supports Mediatek MT2701 imgsys clocks. config COMMON_CLK_MT2701_VDECSYS bool "Clock driver for Mediatek MT2701 vdecsys" - select COMMON_CLK_MT2701 + depends on COMMON_CLK_MT2701 ---help--- This driver supports Mediatek MT2701 vdecsys clocks. config COMMON_CLK_MT2701_HIFSYS bool "Clock driver for Mediatek MT2701 hifsys" - select COMMON_CLK_MT2701 + depends on COMMON_CLK_MT2701 ---help--- This driver supports Mediatek MT2701 hifsys clocks. config COMMON_CLK_MT2701_ETHSYS bool "Clock driver for Mediatek MT2701 ethsys" - select COMMON_CLK_MT2701 + depends on COMMON_CLK_MT2701 ---help--- This driver supports Mediatek MT2701 ethsys clocks. config COMMON_CLK_MT2701_BDPSYS bool "Clock driver for Mediatek MT2701 bdpsys" - select COMMON_CLK_MT2701 + depends on COMMON_CLK_MT2701 ---help--- This driver supports Mediatek MT2701 bdpsys clocks.
If I say "no" to "Clock driver for Mediatek MT2701", I don't want to be asked individually about each sub-driver. No means no. Additionally, this driver shouldn't be proposed at all on non-mediatek builds, unless build-testing. Signed-off-by: Jean Delvare <jdelvare@suse.de> Fixes: e9862118272a ("clk: mediatek: Add MT2701 clock support") Cc: Shunli Wang <shunli.wang@mediatek.com> Cc: James Liao <jamesjj.liao@mediatek.com> Cc: Erin Lo <erin.lo@mediatek.com> Cc: Stephen Boyd <sboyd@codeaurora.org> Cc: Michael Turquette <mturquette@baylibre.com> Cc: Matthias Brugger <matthias.bgg@gmail.com> --- As a side note, is there any rationale for splitting this driver into that many small sub-drivers? Looks overengineered to me. As another side note, I wonder why so many clock drivers have "COMMON" in their symbol names. Looks wrong to me. drivers/clk/mediatek/Kconfig | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)