[PATCH 13/13] fix ps3fb glue allowing a modular build

[PATCH 13/13] fix ps3fb glue allowing a modular build

[Al Viro]

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 arch/powerpc/platforms/ps3/htab.c |    2 ++
 drivers/video/Kconfig             |    2 +-
 include/asm-powerpc/ps3fb.h       |    5 -----
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/platforms/ps3/htab.c b/arch/powerpc/platforms/ps3/htab.c
index e12e59f..67d6f58 100644
--- a/arch/powerpc/platforms/ps3/htab.c
+++ b/arch/powerpc/platforms/ps3/htab.c
@@ -235,7 +235,9 @@ static void ps3_hpte_invalidate(unsigned long slot, unsigned long va,
 static void ps3_hpte_clear(void)
 {
 	/* Make sure to clean up the frame buffer device first */
+#ifdef CONFIG_PS3_FB
 	ps3fb_cleanup();
+#endif
 
 	lv1_unmap_htab(htab_addr);
 }
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 7f5a598..ab43a32 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -1615,7 +1615,7 @@ config FB_IBM_GXT4500
 	  adaptor, found on some IBM System P (pSeries) machines.
 
 config FB_PS3
-	bool "PS3 GPU framebuffer driver"
+	tristate "PS3 GPU framebuffer driver"
 	depends on FB && PS3_PS3AV
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
diff --git a/include/asm-powerpc/ps3fb.h b/include/asm-powerpc/ps3fb.h
index ad81cf4..a447387 100644
--- a/include/asm-powerpc/ps3fb.h
+++ b/include/asm-powerpc/ps3fb.h
@@ -43,13 +43,8 @@ struct ps3fb_ioctl_res {
 
 #ifdef __KERNEL__
 
-#ifdef CONFIG_FB_PS3
 extern void ps3fb_flip_ctl(int on);
 extern void ps3fb_cleanup(void);
-#else
-static inline void ps3fb_flip_ctl(int on) {}
-static inline void ps3fb_cleanup(void) {}
-#endif
 
 #endif /* __KERNEL__ */
 
-- 
1.5.0-rc2.GIT

Re: [PATCH 13/13] fix ps3fb glue allowing a modular build

[Geert Uytterhoeven]

On Wed, 14 Mar 2007, Al Viro wrote:
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

NAK

There are several problems with making it modular. I did try, cfr. the
incomplete patchlets below.

> ---
>  arch/powerpc/platforms/ps3/htab.c |    2 ++
>  drivers/video/Kconfig             |    2 +-
>  include/asm-powerpc/ps3fb.h       |    5 -----
>  3 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/ps3/htab.c b/arch/powerpc/platforms/ps3/htab.c
> index e12e59f..67d6f58 100644
> --- a/arch/powerpc/platforms/ps3/htab.c
> +++ b/arch/powerpc/platforms/ps3/htab.c
> @@ -235,7 +235,9 @@ static void ps3_hpte_invalidate(unsigned long slot, unsigned long va,
>  static void ps3_hpte_clear(void)
>  {
>  	/* Make sure to clean up the frame buffer device first */
> +#ifdef CONFIG_PS3_FB
>  	ps3fb_cleanup();
> +#endif

I'm not sure it will survive a kexec() of a new kernel if ps3fb_cleanup()
isn't called when ps3fb.ko has been loaded. But that's an issue for later...

> index ad81cf4..a447387 100644
> --- a/include/asm-powerpc/ps3fb.h
> +++ b/include/asm-powerpc/ps3fb.h
> @@ -43,13 +43,8 @@ struct ps3fb_ioctl_res {
>  
>  #ifdef __KERNEL__
>  
> -#ifdef CONFIG_FB_PS3
>  extern void ps3fb_flip_ctl(int on);
>  extern void ps3fb_cleanup(void);
> -#else
> -static inline void ps3fb_flip_ctl(int on) {}
> -static inline void ps3fb_cleanup(void) {}
> -#endif

Now it fails to link with:

| drivers/built-in.o: In function `ps3av_cmd_avb_param':ps3-linux-2.6.21-rc3/drivers/ps3/ps3av_cmd.c:855: undefined reference to `.ps3fb_flip_ctl'
| :ps3-linux-2.6.21-rc3/drivers/ps3/ps3av_cmd.c:869: undefined reference to `.ps3fb_flip_ctl'

Which could be fixed with something like:

--- ps3-linux-2.6.20.orig/drivers/ps3/ps3av.c
+++ ps3-linux-2.6.20/drivers/ps3/ps3av.c
@@ -868,6 +868,22 @@ int ps3av_dev_close(void)
 
 EXPORT_SYMBOL_GPL(ps3av_dev_close);
 
+void ps3av_register_flip_ctl(void (*flip_ctl)(int on))
+{
+	mutex_lock(&ps3av.mutex);
+	ps3av.flip_ctl = flip_ctl;
+	mutex_unlock(&ps3av.mutex);
+}
+EXPORT_SYMBOL_GPL(ps3av_register_flip_ctl);
+
+void ps3av_flip_ctl(int on)
+{
+	mutex_lock(&ps3av.mutex);
+	if (ps3av.flip_ctl)
+		ps3av.flip_ctl(on);
+	mutex_unlock(&ps3av.mutex);
+}
+
 static int ps3av_probe(struct ps3_vuart_port_device *dev)
 {
 	int res;
--- ps3-linux-2.6.20.orig/drivers/ps3/ps3av_cmd.c
+++ ps3-linux-2.6.20/drivers/ps3/ps3av_cmd.c
@@ -852,7 +852,7 @@ int ps3av_cmd_avb_param(struct ps3av_pkt
 {
 	int res;
 
-	ps3fb_flip_ctl(0);	/* flip off */
+	ps3av_flip_ctl(0);	/* flip off */
 
 	/* avb packet */
 	res = ps3av_do_pkt(PS3AV_CID_AVB_PARAM, send_len, sizeof(*avb),
@@ -866,7 +866,7 @@ int ps3av_cmd_avb_param(struct ps3av_pkt
 			 res);
 
       out:
-	ps3fb_flip_ctl(1);	/* flip on */
+	ps3av_flip_ctl(1);	/* flip on */
 	return res;
 }
 
--- ps3-linux-2.6.20.orig/include/asm-powerpc/ps3av.h
+++ ps3-linux-2.6.20/include/asm-powerpc/ps3av.h
@@ -660,6 +660,7 @@ struct ps3av {
 	u32 audio_port;
 	int ps3av_mode;
 	int ps3av_mode_old;
+	void (*flip_ctl)(int on);
 };
 
 /** command status **/
@@ -734,5 +735,7 @@ extern int ps3av_video_mute(int);
 extern int ps3av_audio_mute(int);
 extern int ps3av_dev_open(void);
 extern int ps3av_dev_close(void);
+extern void ps3av_register_flip_ctl(void (*func)(int on));
+extern void ps3av_flip_ctl(int on);
 
 #endif	/* _ASM_POWERPC_PS3AV_H_ */

and calling ps3av_register_flip_ctl() from ps3fb at the appropriate places.

Still, the module won't load, as ps3fb needs ps3fb_videomemory[] (do you
know a better way to allocate a few mebibytes of physically contiguous
memory?):

--- ps3-linux-2.6.20.orig/arch/powerpc/platforms/ps3/setup.c
+++ ps3-linux-2.6.20/arch/powerpc/platforms/ps3/setup.c
@@ -115,12 +115,13 @@ static void prealloc(struct ps3_prealloc
 	       p->address);
 }
 
-#ifdef CONFIG_FB_PS3
+#if defined(CONFIG_FB_PS3) || defined(CONFIG_FB_PS3_MODULE)
 struct ps3_prealloc ps3fb_videomemory = {
     .name = "ps3fb videomemory",
     .size = CONFIG_FB_PS3_DEFAULT_SIZE_M*1024*1024,
     .align = 1024*1024			/* the GPU requires 1 MiB alignment */
 };
+EXPORT_SYMBOL_GPL(ps3fb_videomemory);
 #define prealloc_ps3fb_videomemory()	prealloc(&ps3fb_videomemory)
 
 static int __init early_parse_ps3fb(char *p)

And finally, make sure CONFIG_LOGO=n, as there's a bug in the logo code: logos
are __initdata but the logo code still tries to draw them for a modular fbdev.
Originally (eons ago) this case was handled by the flag initmem_freed, which no
longer exists.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

Re: [PATCH 13/13] fix ps3fb glue allowing a modular build

[Al Viro]

On Wed, Mar 14, 2007 at 10:50:16AM +0100, Geert Uytterhoeven wrote:
> On Wed, 14 Mar 2007, Al Viro wrote:
> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> 
> NAK
> 
> There are several problems with making it modular. I did try, cfr. the
> incomplete patchlets below.

[snip]

> And finally, make sure CONFIG_LOGO=n, as there's a bug in the logo code: logos
> are __initdata but the logo code still tries to draw them for a modular fbdev.
> Originally (eons ago) this case was handled by the flag initmem_freed, which no
> longer exists.

Lovely...  Consider that one withdrawn, please.  FWIW, if we leave it bool
(and flip_ctl mess alone would make a good reason for that) we probably want
to have it depend on FB=y

Re: [PATCH 13/13] fix ps3fb glue allowing a modular build

[Geert Uytterhoeven]

	Hi Al,

On Wed, 14 Mar 2007, Al Viro wrote:
> On Wed, Mar 14, 2007 at 10:50:16AM +0100, Geert Uytterhoeven wrote:
> > On Wed, 14 Mar 2007, Al Viro wrote:
> > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > 
> > NAK
> 
> Lovely...  Consider that one withdrawn, please.  FWIW, if we leave it bool
> (and flip_ctl mess alone would make a good reason for that) we probably want
> to have it depend on FB=y

What's the advantage of making FB_PS3 depend on FB=y vs. plain FB?
In both cases it can be enabled if FB=y only, right? Or am I missing something?

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

Re: [PATCH 13/13] fix ps3fb glue allowing a modular build

[Al Viro]

On Wed, Mar 14, 2007 at 05:17:45PM +0100, Geert Uytterhoeven wrote:
> 	Hi Al,
> 
> On Wed, 14 Mar 2007, Al Viro wrote:
> > On Wed, Mar 14, 2007 at 10:50:16AM +0100, Geert Uytterhoeven wrote:
> > > On Wed, 14 Mar 2007, Al Viro wrote:
> > > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > > 
> > > NAK
> > 
> > Lovely...  Consider that one withdrawn, please.  FWIW, if we leave it bool
> > (and flip_ctl mess alone would make a good reason for that) we probably want
> > to have it depend on FB=y
> 
> What's the advantage of making FB_PS3 depend on FB=y vs. plain FB?
> In both cases it can be enabled if FB=y only, right? Or am I missing something?

Nope.  How can kconfig distinguish that from a boolean option in modular
driver?  bool *can* depend on tristate and be selected when tristate is
set to m.

Re: [PATCH 13/13] fix ps3fb glue allowing a modular build

[Geert Uytterhoeven]

On Wed, 14 Mar 2007, Al Viro wrote:
> On Wed, Mar 14, 2007 at 05:17:45PM +0100, Geert Uytterhoeven wrote:
> > On Wed, 14 Mar 2007, Al Viro wrote:
> > > On Wed, Mar 14, 2007 at 10:50:16AM +0100, Geert Uytterhoeven wrote:
> > > > On Wed, 14 Mar 2007, Al Viro wrote:
> > > > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > > > 
> > > > NAK
> > > 
> > > Lovely...  Consider that one withdrawn, please.  FWIW, if we leave it bool
> > > (and flip_ctl mess alone would make a good reason for that) we probably want
> > > to have it depend on FB=y
> > 
> > What's the advantage of making FB_PS3 depend on FB=y vs. plain FB?
> > In both cases it can be enabled if FB=y only, right? Or am I missing something?
> 
> Nope.  How can kconfig distinguish that from a boolean option in modular
> driver?  bool *can* depend on tristate and be selected when tristate is
> set to m.

Sorry, you're right. I tried it before sending the previous mail, but
apparently I made a mistake. I tried again, and now it indeed allows me to
select FB_PS3.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

Re: [PATCH 13/13] fix ps3fb glue allowing a modular build

[Linus Torvalds]

On Wed, 14 Mar 2007, Al Viro wrote:
> 
> Nope.  How can kconfig distinguish that from a boolean option in modular
> driver?  bool *can* depend on tristate and be selected when tristate is
> set to m.

Btw, this is one of those things that easily causes problems.

In many ways it would be nice if we had two different kinds of "bool": one 
where "m" in the dependency chain means "y" is ok, and one where "m" means 
"n".

We used to have "dep_bool" and "dep_mbool" for this, a long time ago. It 
got dropped in the Kconfig language rewrite, and I think it was a mistake.

So I think it would be nice to re-introduce it. As it is, we have a number 
of Kconfig language constructs that are just unnecessarily hard to 
understand, because we end up having to add a "= y" or similar.

The rule *used* to be: "dep_mbool" was a boolean that was valid even for 
modules, while "dep_bool" was a boolean that was valid only for straigth 
"y", and a module would turn it off.

Maybe not "bool" vs "mbool", but it might be nice to have

	bool FB_PS3
		depends strictly on FB

ie a "depends strictly" refuses to upgrade a bool dependency from "m" to 
"y", while a regular depends allows it.

Or something.. The "depends strictly on X" thing would really be just a 
mental shorthand for "depends on (X)=y" (it's actually longer to type, but 
I think it's a bit more intuitive, thus "mental shortcut").

		Linus

Re: [PATCH 13/13] fix ps3fb glue allowing a modular build

[Geert Uytterhoeven]

On Wed, 14 Mar 2007, Linus Torvalds wrote:
> On Wed, 14 Mar 2007, Al Viro wrote:
> > Nope.  How can kconfig distinguish that from a boolean option in modular
> > driver?  bool *can* depend on tristate and be selected when tristate is
> > set to m.
> 
> Btw, this is one of those things that easily causes problems.

I just found another caveat: while `select' is some super-override-it-all that
ignores depends on non-existent symbols, it doesn't promote `m' to `y'.
E.g. `select FB_CFB_FILLRECT' from a bool still gives FB_CFB_FILLRECT=m if
FB=m.

Anyway, here's a fix for the 3 problematic cases I found.

Subject: bool fbdevs must depend on FB = y

Frame buffer device drivers that cannot be built as modules must depend on
`FB = y'.  Correct the 3 remaining offenders.

Signed-off-by: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>

diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 7f5a598..e4f0dd0 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -1320,7 +1320,7 @@ config FB_AU1100
 
 config FB_AU1200
 	bool "Au1200 LCD Driver"
-	depends on FB && MIPS && SOC_AU1200
+	depends on (FB = y) && MIPS && SOC_AU1200
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
@@ -1470,7 +1470,7 @@ config FB_G364
 
 config FB_68328
 	bool "Motorola 68328 native frame buffer support"
-	depends on FB && (M68328 || M68EZ328 || M68VZ328)
+	depends on (FB = y) && (M68328 || M68EZ328 || M68VZ328)
  	select FB_CFB_FILLRECT
  	select FB_CFB_COPYAREA
  	select FB_CFB_IMAGEBLIT
@@ -1616,7 +1616,7 @@ config FB_IBM_GXT4500
 
 config FB_PS3
 	bool "PS3 GPU framebuffer driver"
-	depends on FB && PS3_PS3AV
+	depends on (FB = y) && PS3_PS3AV
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

Re: [PATCH 13/13] fix ps3fb glue allowing a modular build

[Al Viro]

On Wed, Mar 14, 2007 at 10:30:31AM -0700, Linus Torvalds wrote:
> > Nope.  How can kconfig distinguish that from a boolean option in modular
> > driver?  bool *can* depend on tristate and be selected when tristate is
> > set to m.
> 
> Btw, this is one of those things that easily causes problems.
> 
> In many ways it would be nice if we had two different kinds of "bool": one 
> where "m" in the dependency chain means "y" is ok, and one where "m" means 
> "n".

*nod*

> Maybe not "bool" vs "mbool", but it might be nice to have
> 
> 	bool FB_PS3
> 		depends strictly on FB
> 
> ie a "depends strictly" refuses to upgrade a bool dependency from "m" to 
> "y", while a regular depends allows it.
> 
> Or something.. The "depends strictly on X" thing would really be just a 
> mental shorthand for "depends on (X)=y" (it's actually longer to type, but 
> I think it's a bit more intuitive, thus "mental shortcut").

There's a fun side question, though: what should allmodconfig do?  FB=m,
FB_PS3=n?  Or FB=y, FB_PS3=y?

Re: [PATCH 13/13] fix ps3fb glue allowing a modular build

[Geert Uytterhoeven]

On Wed, 14 Mar 2007, Al Viro wrote:
> On Wed, Mar 14, 2007 at 10:30:31AM -0700, Linus Torvalds wrote:
> > Maybe not "bool" vs "mbool", but it might be nice to have
> > 
> > 	bool FB_PS3
> > 		depends strictly on FB
> > 
> > ie a "depends strictly" refuses to upgrade a bool dependency from "m" to 
> > "y", while a regular depends allows it.
> > 
> > Or something.. The "depends strictly on X" thing would really be just a 
> > mental shorthand for "depends on (X)=y" (it's actually longer to type, but 
> > I think it's a bit more intuitive, thus "mental shortcut").
> 
> There's a fun side question, though: what should allmodconfig do?  FB=m,
> FB_PS3=n?  Or FB=y, FB_PS3=y?

>From `make help':
| New config selecting modules when possible

FB can be a module, so FB=m, FB_PS3=n.

It doesn't say anything about things that can't be modules :-)

But I agree the chances of getting a system that doesn't work increase...

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

Re: [PATCH 13/13] fix ps3fb glue allowing a modular build

[Al Viro]

On Wed, Mar 14, 2007 at 07:09:40PM +0100, Geert Uytterhoeven wrote:
> On Wed, 14 Mar 2007, Al Viro wrote:
> > On Wed, Mar 14, 2007 at 10:30:31AM -0700, Linus Torvalds wrote:
> > > Maybe not "bool" vs "mbool", but it might be nice to have
> > > 
> > > 	bool FB_PS3
> > > 		depends strictly on FB
> > > 
> > > ie a "depends strictly" refuses to upgrade a bool dependency from "m" to 
> > > "y", while a regular depends allows it.
> > > 
> > > Or something.. The "depends strictly on X" thing would really be just a 
> > > mental shorthand for "depends on (X)=y" (it's actually longer to type, but 
> > > I think it's a bit more intuitive, thus "mental shortcut").
> > 
> > There's a fun side question, though: what should allmodconfig do?  FB=m,
> > FB_PS3=n?  Or FB=y, FB_PS3=y?
> 
> >From `make help':
> | New config selecting modules when possible
> 
> FB can be a module, so FB=m, FB_PS3=n.
> 
> It doesn't say anything about things that can't be modules :-)
> 
> But I agree the chances of getting a system that doesn't work increase...

No, I realize what kind of behaviour we'll get if we go for dependency on
FB=y.  However, if we really introduce a new kconfig primitive, it might
make sense to teach allmodconfig to deal with it.

kconfig `bool' (was: Re: [PATCH 13/13] fix ps3fb glue allowing a modular build)

[Geert Uytterhoeven]

On Wed, 14 Mar 2007, Linus Torvalds wrote:
> On Wed, 14 Mar 2007, Al Viro wrote:
> > Nope.  How can kconfig distinguish that from a boolean option in modular
> > driver?  bool *can* depend on tristate and be selected when tristate is
> > set to m.
> 
> Btw, this is one of those things that easily causes problems.
> 
> In many ways it would be nice if we had two different kinds of "bool": one 
> where "m" in the dependency chain means "y" is ok, and one where "m" means 
> "n".
> 
> We used to have "dep_bool" and "dep_mbool" for this, a long time ago. It 
> got dropped in the Kconfig language rewrite, and I think it was a mistake.
> 
> So I think it would be nice to re-introduce it. As it is, we have a number 
> of Kconfig language constructs that are just unnecessarily hard to 
> understand, because we end up having to add a "= y" or similar.
> 
> The rule *used* to be: "dep_mbool" was a boolean that was valid even for 
> modules, while "dep_bool" was a boolean that was valid only for straigth 
> "y", and a module would turn it off.
> 
> Maybe not "bool" vs "mbool", but it might be nice to have
> 
> 	bool FB_PS3
> 		depends strictly on FB
> 
> ie a "depends strictly" refuses to upgrade a bool dependency from "m" to 
> "y", while a regular depends allows it.
> 
> Or something.. The "depends strictly on X" thing would really be just a 
> mental shorthand for "depends on (X)=y" (it's actually longer to type, but 
> I think it's a bit more intuitive, thus "mental shortcut").

I've been thinking about this a bit more...

Kconfig knows about the following types:
  o bool
  o tristate
  o string
  o hex
  o int

However, from a semantical point of view, they can be subdivided in 2 classes:
  1. driver/subsystem/library enablers (i.e. things that are used in a Makefile
     to decide whether to compile a unit or not):
       o tristate (y=builtin, m=loadable, n=disabled)
       o bool (y=builtin, n=disabled)
  2. options (i.e. things that control some features, limits, or default
     values):
       o bool (y=true, n=false)
       o string (literal)
       o hex (literal)
       o int (literal)

The confusion arises from the 2 different semantics for `bool': for the former,
a `depends on' obviously cannot be `builtin' if the dependency is `modular',
while for the latter, it can be `true' if the dependency is `modular'.

It would be nice if Kconfig would make the distinction between the two types of
`bool' explicit.  Anyone with a good replacement name for the first type of
`bool'? I can't find one.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

Re: kconfig `bool' (was: Re: [PATCH 13/13] fix ps3fb glue allowing a modular build)

[Jan Engelhardt]

On Mar 20 2007 22:06, Geert Uytterhoeven wrote:
>> 
>> Maybe not "bool" vs "mbool", but it might be nice to have
>> 
>> 	bool FB_PS3
>> 		depends strictly on FB
>> 
>> ie a "depends strictly" refuses to upgrade a bool dependency from "m" to 
>> "y", while a regular depends allows it.
>> 
>> Or something.. The "depends strictly on X" thing would really be just a 
>> mental shorthand for "depends on (X)=y" (it's actually longer to type, but 
>> I think it's a bit more intuitive, thus "mental shortcut").
>
>I've been thinking about this a bit more...
>
>Kconfig knows about the following types:
>  o bool
>  o tristate
>  o string
>  o hex
>  o int
>
>However, from a semantical point of view, they can be subdivided in 2 classes:
>  1. driver/subsystem/library enablers (i.e. things that are used in a Makefile
>     to decide whether to compile a unit or not):
>       o tristate (y=builtin, m=loadable, n=disabled)
>       o bool (y=builtin, n=disabled)
>  2. options (i.e. things that control some features, limits, or default
>     values):
>       o bool (y=true, n=false)
>       o string (literal)
>       o hex (literal)
>       o int (literal)
>
>The confusion arises from the 2 different semantics for `bool': for the former,
>a `depends on' obviously cannot be `builtin' if the dependency is `modular',
>while for the latter, it can be `true' if the dependency is `modular'.

I think it was once (is still?) possible to have something like

  <M> Foo
      <*> Bar

which would mean: include bar.o into foo.ko. If one chose to

  <M> Foo
      <M> Bar

you'd get foo.ko and bar.ko, with a modinfo dependency of course.



Jan
-- 

Re: kconfig `bool' (was: Re: [PATCH 13/13] fix ps3fb glue allowing a modular build)

[Roman Zippel]

Hi,

On Tue, 20 Mar 2007, Geert Uytterhoeven wrote:

> On Wed, 14 Mar 2007, Linus Torvalds wrote:
> > In many ways it would be nice if we had two different kinds of "bool": one 
> > where "m" in the dependency chain means "y" is ok, and one where "m" means 
> > "n".
> > 
> > We used to have "dep_bool" and "dep_mbool" for this, a long time ago. It 
> > got dropped in the Kconfig language rewrite, and I think it was a mistake.

Well, it wasn't really dropped, just the syntax changed...
There were only one or two rules which depended on the dep_bool syntax and 
it were special cases anyway.

> > So I think it would be nice to re-introduce it. As it is, we have a number 
> > of Kconfig language constructs that are just unnecessarily hard to 
> > understand, because we end up having to add a "= y" or similar.
> > 
> > The rule *used* to be: "dep_mbool" was a boolean that was valid even for 
> > modules, while "dep_bool" was a boolean that was valid only for straigth 
> > "y", and a module would turn it off.
> > 
> > Maybe not "bool" vs "mbool", but it might be nice to have
> > 
> > 	bool FB_PS3
> > 		depends strictly on FB
> > 
> > ie a "depends strictly" refuses to upgrade a bool dependency from "m" to 
> > "y", while a regular depends allows it.
> > 
> > Or something.. The "depends strictly on X" thing would really be just a 
> > mental shorthand for "depends on (X)=y" (it's actually longer to type, but 
> > I think it's a bit more intuitive, thus "mental shortcut").

I'm not sure this would really an improvement, for the casual reader X=y 
is IMO more readable if he knows the basic kconfig syntax, whereas he had 
to lookup first what "strictly" means. Also the potential user had to know 
about this syntax, but it's simply not needed often enough that it would 
be common and might be easily forgotten if one fixed the driver and 
changed the bool to tristate.

> I've been thinking about this a bit more...
> 
> Kconfig knows about the following types:
>   o bool
>   o tristate
>   o string
>   o hex
>   o int
> 
> However, from a semantical point of view, they can be subdivided in 2 classes:
>   1. driver/subsystem/library enablers (i.e. things that are used in a Makefile
>      to decide whether to compile a unit or not):
>        o tristate (y=builtin, m=loadable, n=disabled)
>        o bool (y=builtin, n=disabled)
>   2. options (i.e. things that control some features, limits, or default
>      values):
>        o bool (y=true, n=false)
>        o string (literal)
>        o hex (literal)
>        o int (literal)
> 
> The confusion arises from the 2 different semantics for `bool': for the former,
> a `depends on' obviously cannot be `builtin' if the dependency is `modular',
> while for the latter, it can be `true' if the dependency is `modular'.

What I'm considering in this context is to introduce a syntax to 
specifically describe modules (e.g. to also generate the Makefile from 
it):

module FB_PS3
	bool ".."
	depends on FB

In this context it would be valid to add the requirement that the 
dependencies must be 'y'.

bye, Roman

Re: [PATCH 13/13] fix ps3fb glue allowing a modular build

[Antonino A. Daplas]

[-- Attachment #1: Type: text/plain, Size: 717 bytes --]

On Wed, 2007-03-14 at 10:50 +0100, Geert Uytterhoeven wrote:
> On Wed, 14 Mar 2007, Al Viro wrote:
> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> 

> And finally, make sure CONFIG_LOGO=n, as there's a bug in the logo code: logos
> are __initdata but the logo code still tries to draw them for a modular fbdev.
> Originally (eons ago) this case was handled by the flag initmem_freed, which no
> longer exists.
> 

True, I tried to prevent the logo from being drawn if the driver is
loaded first prior to fbcon, but the code will still draw the logo if
the load order is reversed.  Can you try this patch?  It will only
permit the drawing of the logo if both the driver and fbcon are compiled
statically.

Tony

[-- Attachment #2: logo_fix.diff --]
[-- Type: text/x-patch, Size: 2340 bytes --]

diff --git a/drivers/video/console/fbcon.c b/drivers/video/console/fbcon.c
index bd131d4..12e8a3b 100644
--- a/drivers/video/console/fbcon.c
+++ b/drivers/video/console/fbcon.c
@@ -107,7 +107,9 @@ static struct display fb_display[MAX_NR_
 
 static signed char con2fb_map[MAX_NR_CONSOLES];
 static signed char con2fb_map_boot[MAX_NR_CONSOLES];
+#ifndef MODULE
 static int logo_height;
+#endif
 static int logo_lines;
 /* logo_shown is an index to vc_cons when >= 0; otherwise follows FBCON_LOGO
    enums.  */
@@ -576,6 +578,13 @@ static int fbcon_takeover(int show_logo)
 	return err;
 }
 
+#ifdef MODULE
+static void fbcon_prepare_logo(struct vc_data *vc, struct fb_info *info,
+			       int cols, int rows, int new_cols, int new_rows)
+{
+	logo_shown = FBCON_LOGO_DONTSHOW;
+}
+#else
 static void fbcon_prepare_logo(struct vc_data *vc, struct fb_info *info,
 			       int cols, int rows, int new_cols, int new_rows)
 {
@@ -584,6 +593,11 @@ static void fbcon_prepare_logo(struct vc
 	int cnt, erase = vc->vc_video_erase_char, step;
 	unsigned short *save = NULL, *r, *q;
 
+	if (info->flags & FBINFO_MODULE) {
+		logo_shown = FBCON_LOGO_DONTSHOW;
+		goto done;
+	}
+
 	/*
 	 * remove underline attribute from erase character
 	 * if black and white framebuffer.
@@ -654,7 +668,10 @@ static void fbcon_prepare_logo(struct vc
 		logo_shown = FBCON_LOGO_DRAW;
 		vc->vc_top = logo_lines;
 	}
+
+done:
 }
+#endif /* MODULE */
 
 #ifdef CONFIG_FB_TILEBLITTING
 static void set_blitting_type(struct vc_data *vc, struct fb_info *info)
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 45f3839..08c292d 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -418,7 +418,8 @@ int fb_prepare_logo(struct fb_info *info
 
 	memset(&fb_logo, 0, sizeof(struct logo_data));
 
-	if (info->flags & FBINFO_MISC_TILEBLITTING)
+	if (info->flags & FBINFO_MISC_TILEBLITTING ||
+	    info->flags & FBINFO_MODULE)
 		return 0;
 
 	if (info->fix.visual == FB_VISUAL_DIRECTCOLOR) {
@@ -483,7 +484,8 @@ int fb_show_logo(struct fb_info *info, i
 	struct fb_image image;
 
 	/* Return if the frame buffer is not mapped or suspended */
-	if (fb_logo.logo == NULL || info->state != FBINFO_STATE_RUNNING)
+	if (fb_logo.logo == NULL || info->state != FBINFO_STATE_RUNNING ||
+	    info->flags & FBINFO_MODULE)
 		return 0;
 
 	image.depth = 8;