[PATCH] clk: mediatek: Fix MT2701 dependencies

[PATCH] clk: mediatek: Fix MT2701 dependencies

[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 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(-)

--- 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.
 


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] clk: mediatek: Fix MT2701 dependencies

[Andreas Färber]

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

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH] clk: mediatek: Fix MT2701 dependencies

[Jean Delvare]

On Mon, 9 Jan 2017 21:08:50 +0100, Andreas F=C3=A4rber wrote:
> Hi Jean,
>=20
> 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.
> >=20
> > Additionally, this driver shouldn't be proposed at all on non-mediatek
> > builds, unless build-testing.
> >=20
> > 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.
>=20
> 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:5=
3.000000000 +0100
> > +++ linux-4.10-rc2/drivers/clk/mediatek/Kconfig	2017-01-09 11:17:37.542=
344083 +0100
> > @@ -8,6 +8,7 @@ config COMMON_CLK_MEDIATEK
> > =20
> >  config COMMON_CLK_MT2701
> >  	bool "Clock driver for Mediatek MT2701"
> > +	depends on ARCH_MEDIATEK || COMPILE_TEST
> >  	select COMMON_CLK_MEDIATEK
> >  	default ARCH_MEDIATEK
>=20
> 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?
>=20
> Same for mt2701 pinctrl.
>=20
> http://kernel.opensuse.org/cgit/kernel-source/plain/config/arm64/default?=
id=3Dff90e915117c5d7a8bb00dc0bc1d3145ebe985ec

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,
>=20
> Reviewed-by: Andreas F=C3=A4rber <afaerber@suse.de>

Thanks for the review.

--=20
Jean Delvare
SUSE L3 Support

Re: [PATCH] clk: mediatek: Fix MT2701 dependencies

[Jean Delvare]

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.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] clk: mediatek: Fix MT2701 dependencies

[James Liao]

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.
> 

Re: [PATCH] clk: mediatek: Fix MT2701 dependencies

[Jean Delvare]

Hi James,

On Wed, 11 Jan 2017 09:56:28 +0800, James Liao wrote:
> On Tue, 2017-01-10 at 13:03 +0100, Jean Delvare wrote:
> > 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.

OK, so would the following change make sense?

---
 drivers/clk/mediatek/Kconfig |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

--- linux-4.10-rc2.orig/drivers/clk/mediatek/Kconfig	2017-01-09 11:17:37.542344083 +0100
+++ linux-4.10-rc2/drivers/clk/mediatek/Kconfig	2017-01-11 13:40:21.965890913 +0100
@@ -52,16 +52,16 @@ config COMMON_CLK_MT2701_BDPSYS
 
 config COMMON_CLK_MT8135
 	bool "Clock driver for Mediatek MT8135"
-	depends on ARCH_MEDIATEK || COMPILE_TEST
+	depends on (ARCH_MEDIATEK && !ARM64) || COMPILE_TEST
 	select COMMON_CLK_MEDIATEK
-	default ARCH_MEDIATEK
+	default y
 	---help---
 	  This driver supports Mediatek MT8135 clocks.
 
 config COMMON_CLK_MT8173
 	bool "Clock driver for Mediatek MT8173"
-	depends on ARCH_MEDIATEK || COMPILE_TEST
+	depends on (ARCH_MEDIATEK && ARM64) || COMPILE_TEST
 	select COMMON_CLK_MEDIATEK
-	default ARCH_MEDIATEK
+	default y
 	---help---
 	  This driver supports Mediatek MT8173 clocks.

Thanks,
-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] clk: mediatek: Fix MT2701 dependencies

[Jean Delvare]

Hi James,

On Wed, 11 Jan 2017 09:56:28 +0800, James Liao wrote:
> On Tue, 2017-01-10 at 13:03 +0100, Jean Delvare wrote:
> > 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.

OK, so would the following change make sense?

---
 drivers/clk/mediatek/Kconfig |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

--- linux-4.10-rc2.orig/drivers/clk/mediatek/Kconfig	2017-01-09 11:17:37.542344083 +0100
+++ linux-4.10-rc2/drivers/clk/mediatek/Kconfig	2017-01-11 13:40:21.965890913 +0100
@@ -52,16 +52,16 @@ config COMMON_CLK_MT2701_BDPSYS
 
 config COMMON_CLK_MT8135
 	bool "Clock driver for Mediatek MT8135"
-	depends on ARCH_MEDIATEK || COMPILE_TEST
+	depends on (ARCH_MEDIATEK && !ARM64) || COMPILE_TEST
 	select COMMON_CLK_MEDIATEK
-	default ARCH_MEDIATEK
+	default y
 	---help---
 	  This driver supports Mediatek MT8135 clocks.
 
 config COMMON_CLK_MT8173
 	bool "Clock driver for Mediatek MT8173"
-	depends on ARCH_MEDIATEK || COMPILE_TEST
+	depends on (ARCH_MEDIATEK && ARM64) || COMPILE_TEST
 	select COMMON_CLK_MEDIATEK
-	default ARCH_MEDIATEK
+	default y
 	---help---
 	  This driver supports Mediatek MT8173 clocks.

Thanks,
-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] clk: mediatek: Fix MT2701 dependencies

[Yingjoe Chen]

On Wed, 2017-01-11 at 13:41 +0100, Jean Delvare wrote:
> Hi James,
> 
> On Wed, 11 Jan 2017 09:56:28 +0800, James Liao wrote:
> > On Tue, 2017-01-10 at 13:03 +0100, Jean Delvare wrote:
> > > 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.
> 
> OK, so would the following change make sense?
> 
> ---
>  drivers/clk/mediatek/Kconfig |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> --- linux-4.10-rc2.orig/drivers/clk/mediatek/Kconfig	2017-01-09 11:17:37.542344083 +0100
> +++ linux-4.10-rc2/drivers/clk/mediatek/Kconfig	2017-01-11 13:40:21.965890913 +0100
> @@ -52,16 +52,16 @@ config COMMON_CLK_MT2701_BDPSYS
>  
>  config COMMON_CLK_MT8135
>  	bool "Clock driver for Mediatek MT8135"
> -	depends on ARCH_MEDIATEK || COMPILE_TEST
> +	depends on (ARCH_MEDIATEK && !ARM64) || COMPILE_TEST


Why not just check for ARM?

+	depends on (ARCH_MEDIATEK && ARM) || COMPILE_TEST

Joe.C

Re: [PATCH] clk: mediatek: Fix MT2701 dependencies

[Jean Delvare]

Hi Joe,

On Wed, 11 Jan 2017 22:05:26 +0800, Yingjoe Chen wrote:
> On Wed, 2017-01-11 at 13:41 +0100, Jean Delvare wrote:
> > @@ -52,16 +52,16 @@ config COMMON_CLK_MT2701_BDPSYS
> >  
> >  config COMMON_CLK_MT8135
> >  	bool "Clock driver for Mediatek MT8135"
> > -	depends on ARCH_MEDIATEK || COMPILE_TEST
> > +	depends on (ARCH_MEDIATEK && !ARM64) || COMPILE_TEST
> 
> 
> Why not just check for ARM?
> 
> +	depends on (ARCH_MEDIATEK && ARM) || COMPILE_TEST

I don't know :D Andreas suggested it for MT2701, so I assumed ARM was
set also for 64-bit kernels (like X86 is set for 64-bit kernels) and
ARM64 had to be used to differentiate. But it seems ARM isn't set for
64-bit kernels, so you are right, checking for ARM should work and is
more straightforward. I'll fix it before submitting the patch.

Thanks for the review,
-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] clk: mediatek: Fix MT2701 dependencies

[James Liao]

Hi Jean,

On Wed, 2017-01-11 at 13:41 +0100, Jean Delvare wrote:
> Hi James,
> 
> On Wed, 11 Jan 2017 09:56:28 +0800, James Liao wrote:
> > On Tue, 2017-01-10 at 13:03 +0100, Jean Delvare wrote:
> > > 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.
> 
> OK, so would the following change make sense?
> 
> ---
>  drivers/clk/mediatek/Kconfig |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> --- linux-4.10-rc2.orig/drivers/clk/mediatek/Kconfig	2017-01-09 11:17:37.542344083 +0100
> +++ linux-4.10-rc2/drivers/clk/mediatek/Kconfig	2017-01-11 13:40:21.965890913 +0100
> @@ -52,16 +52,16 @@ config COMMON_CLK_MT2701_BDPSYS
>  
>  config COMMON_CLK_MT8135
>  	bool "Clock driver for Mediatek MT8135"
> -	depends on ARCH_MEDIATEK || COMPILE_TEST
> +	depends on (ARCH_MEDIATEK && !ARM64) || COMPILE_TEST
>  	select COMMON_CLK_MEDIATEK
> -	default ARCH_MEDIATEK
> +	default y
>  	---help---
>  	  This driver supports Mediatek MT8135 clocks.
>  
>  config COMMON_CLK_MT8173
>  	bool "Clock driver for Mediatek MT8173"
> -	depends on ARCH_MEDIATEK || COMPILE_TEST
> +	depends on (ARCH_MEDIATEK && ARM64) || COMPILE_TEST
>  	select COMMON_CLK_MEDIATEK
> -	default ARCH_MEDIATEK
> +	default y
>  	---help---
>  	  This driver supports Mediatek MT8173 clocks.
> 
> Thanks,

Although MT8173 is a 64-bit SoC, but 32-bit kernel can run on it. So I
think it no need to limit COMMON_CLK_MT8173 only be enabled on ARM64
platform.


Best regards,

James

Re: [PATCH] clk: mediatek: Fix MT2701 dependencies

[Jean Delvare]

Hi James,

On Thu, 12 Jan 2017 11:39:52 +0800, James Liao wrote:
> On Wed, 2017-01-11 at 13:41 +0100, Jean Delvare wrote:
> >  config COMMON_CLK_MT8173
> >  	bool "Clock driver for Mediatek MT8173"
> > -	depends on ARCH_MEDIATEK || COMPILE_TEST
> > +	depends on (ARCH_MEDIATEK && ARM64) || COMPILE_TEST
> >  	select COMMON_CLK_MEDIATEK
> > -	default ARCH_MEDIATEK
> > +	default y
> >  	---help---
> >  	  This driver supports Mediatek MT8173 clocks.
> 
> Although MT8173 is a 64-bit SoC, but 32-bit kernel can run on it. So I
> think it no need to limit COMMON_CLK_MT8173 only be enabled on ARM64
> platform.

OK, I'll leave that one alone then, thanks for the information.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] clk: mediatek: Fix MT2701 dependencies

[Jean Delvare]

Hi James,

On Thu, 12 Jan 2017 11:39:52 +0800, James Liao wrote:
> On Wed, 2017-01-11 at 13:41 +0100, Jean Delvare wrote:
> >  config COMMON_CLK_MT8173
> >  	bool "Clock driver for Mediatek MT8173"
> > -	depends on ARCH_MEDIATEK || COMPILE_TEST
> > +	depends on (ARCH_MEDIATEK && ARM64) || COMPILE_TEST
> >  	select COMMON_CLK_MEDIATEK
> > -	default ARCH_MEDIATEK
> > +	default y
> >  	---help---
> >  	  This driver supports Mediatek MT8173 clocks.
> 
> Although MT8173 is a 64-bit SoC, but 32-bit kernel can run on it. So I
> think it no need to limit COMMON_CLK_MT8173 only be enabled on ARM64
> platform.

OK, I'll leave that one alone then, thanks for the information.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] clk: mediatek: Fix MT2701 dependencies

[Stephen Boyd]

On 01/12, Jean Delvare wrote:
> Hi James,
> 
> On Thu, 12 Jan 2017 11:39:52 +0800, James Liao wrote:
> > On Wed, 2017-01-11 at 13:41 +0100, Jean Delvare wrote:
> > >  config COMMON_CLK_MT8173
> > >  	bool "Clock driver for Mediatek MT8173"
> > > -	depends on ARCH_MEDIATEK || COMPILE_TEST
> > > +	depends on (ARCH_MEDIATEK && ARM64) || COMPILE_TEST
> > >  	select COMMON_CLK_MEDIATEK
> > > -	default ARCH_MEDIATEK
> > > +	default y
> > >  	---help---
> > >  	  This driver supports Mediatek MT8173 clocks.
> > 
> > Although MT8173 is a 64-bit SoC, but 32-bit kernel can run on it. So I
> > think it no need to limit COMMON_CLK_MT8173 only be enabled on ARM64
> > platform.
> 
> OK, I'll leave that one alone then, thanks for the information.
> 

Is this patch going another round? I haven't seen anything on the
list so far.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] clk: mediatek: Fix MT2701 dependencies

[Jean Delvare]

On Fri, 20 Jan 2017 15:22:14 -0800, Stephen Boyd wrote:
> On 01/12, Jean Delvare wrote:
> > Hi James,
> > 
> > On Thu, 12 Jan 2017 11:39:52 +0800, James Liao wrote:
> > > On Wed, 2017-01-11 at 13:41 +0100, Jean Delvare wrote:
> > > >  config COMMON_CLK_MT8173
> > > >  	bool "Clock driver for Mediatek MT8173"
> > > > -	depends on ARCH_MEDIATEK || COMPILE_TEST
> > > > +	depends on (ARCH_MEDIATEK && ARM64) || COMPILE_TEST
> > > >  	select COMMON_CLK_MEDIATEK
> > > > -	default ARCH_MEDIATEK
> > > > +	default y
> > > >  	---help---
> > > >  	  This driver supports Mediatek MT8173 clocks.
> > > 
> > > Although MT8173 is a 64-bit SoC, but 32-bit kernel can run on it. So I
> > > think it no need to limit COMMON_CLK_MT8173 only be enabled on ARM64
> > > platform.
> > 
> > OK, I'll leave that one alone then, thanks for the information.
> 
> Is this patch going another round? I haven't seen anything on the
> list so far.

Sorry for the delay, I'll send an update later today.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] clk: mediatek: Fix MT2701 dependencies

[Jean Delvare]

Hallo Andreas,

On Mon, 9 Jan 2017 21:08:50 +0100, Andreas F=C3=A4rber 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?
>=20
> 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.

--=20
Jean Delvare
SUSE L3 Support

Re: [PATCH] clk: mediatek: Fix MT2701 dependencies

[Jean Delvare]

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.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] clk: mediatek: Fix MT2701 dependencies

[James Liao]

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.
>  
> 
> 

Re: [PATCH] clk: mediatek: Fix MT2701 dependencies

[James Liao]

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.
>  
> 
>