[PATCH v2] cirrus-logic-framebuffer-i2c-support.patch

[PATCH v2] cirrus-logic-framebuffer-i2c-support.patch

[Krzysztof Halasa]

Hi,

I've attached the second version of the Cirrus Logic I2C patch.
Supersedes:
cirrus-logic-framebuffer-i2c-support.patch
cirrus-logic-framebuffer-i2c-support-fix.patch

Created against current Linus' tree.
Thanks.

Signed-off-by: Krzysztof Halasa <khc@pm.waw.pl>

diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 3badb48..1ab794e 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -137,6 +137,13 @@ config FB_CIRRUS
 	  Say N unless you have such a graphics board or plan to get one
 	  before you next recompile the kernel.
 
+config FB_CIRRUS_I2C
+	bool "Enable Cirrus Logic I2C support"
+	depends on FB_CIRRUS && I2C && I2C_ALGOBIT
+	help
+	  This enables I2C support for Cirrus Logic boards. Currently only
+	  Alpine chips (CL-GD543x and 544x) are supported.
+
 config FB_PM2
 	tristate "Permedia2 support"
 	depends on FB && ((AMIGA && BROKEN) || PCI)
diff --git a/drivers/video/cirrusfb.c b/drivers/video/cirrusfb.c
index 7355da0..d06cd7b 100644
--- a/drivers/video/cirrusfb.c
+++ b/drivers/video/cirrusfb.c
@@ -64,6 +64,10 @@ #define isPReP (machine_is(prep))
 #else
 #define isPReP 0
 #endif
+#ifdef CONFIG_FB_CIRRUS_I2C
+#include <linux/i2c.h>
+#include <linux/i2c-algo-bit.h>
+#endif
 
 #include "video/vga.h"
 #include "video/cirrus.h"
@@ -406,6 +410,11 @@ struct cirrusfb_info {
 	u32	pseudo_palette[16];
 	struct { u8 red, green, blue, pad; } palette[256];
 
+#ifdef CONFIG_FB_CIRRUS_I2C
+	int i2c_used;
+	struct i2c_adapter cirrus_ops;
+	struct i2c_algo_bit_data bit_cirrus_data;
+#endif
 #ifdef CONFIG_ZORRO
 	struct zorro_dev *zdev;
 #endif
@@ -2296,6 +2305,38 @@ static int cirrusfb_set_fbinfo(struct ci
 	return 0;
 }
 
+#ifdef CONFIG_FB_CIRRUS_I2C
+static void alpine_setsda(void *ptr, int state)
+{
+	struct cirrusfb_info *cinfo = ptr;
+	u8 reg = vga_rseq(cinfo->regbase, 0x08) & ~2;
+	if (state)
+		reg |= 2;
+	vga_wseq(cinfo->regbase, 0x08, reg);
+}
+
+static void alpine_setscl(void *ptr, int state)
+{
+	struct cirrusfb_info *cinfo = ptr;
+	u8 reg = vga_rseq(cinfo->regbase, 0x08) & ~1;
+	if (state)
+		reg |= 1;
+	vga_wseq(cinfo->regbase, 0x08, reg);
+}
+
+static int alpine_getsda(void *ptr)
+{
+	struct cirrusfb_info *cinfo = ptr;
+	return !!(vga_rseq(cinfo->regbase, 0x08) & 0x80);
+}
+
+static int alpine_getscl(void *ptr)
+{
+	struct cirrusfb_info *cinfo = ptr;
+	return !!(vga_rseq(cinfo->regbase, 0x08) & 0x04);
+}
+#endif
+
 static int cirrusfb_register(struct cirrusfb_info *cinfo)
 {
 	struct fb_info *info;
@@ -2335,6 +2376,36 @@ static int cirrusfb_register(struct cirr
 		goto err_dealloc_cmap;
 	}
 
+#ifdef CONFIG_FB_CIRRUS_I2C
+	cinfo->i2c_used = 0;
+	if (cinfo->btype == BT_ALPINE || cinfo->btype == BT_PICASSO4) {
+		vga_wseq(cinfo->regbase, 0x08, 0x41); /* DDC mode: SCL */
+		vga_wseq(cinfo->regbase, 0x08, 0x43); /* DDC mode: SCL + SDA */
+		cinfo->bit_cirrus_data.setsda = alpine_setsda;
+		cinfo->bit_cirrus_data.setscl = alpine_setscl;
+		cinfo->bit_cirrus_data.getsda = alpine_getsda;
+		cinfo->bit_cirrus_data.getscl = alpine_getscl;
+		cinfo->bit_cirrus_data.udelay = 5;
+		cinfo->bit_cirrus_data.mdelay = 1;
+		cinfo->bit_cirrus_data.timeout = HZ;
+		cinfo->bit_cirrus_data.data = cinfo;
+		cinfo->cirrus_ops.owner = THIS_MODULE;
+		cinfo->cirrus_ops.id = I2C_HW_B_CIRRUS;
+		cinfo->cirrus_ops.class = I2C_CLASS_DDC;
+		cinfo->cirrus_ops.algo_data = &cinfo->bit_cirrus_data;
+		cinfo->cirrus_ops.dev.parent = info->device;
+		strlcpy(cinfo->cirrus_ops.name, "Cirrus Logic DDC I2C adapter",
+			I2C_NAME_SIZE);
+		if (!(err = i2c_bit_add_bus(&cinfo->cirrus_ops))) {
+			printk(KERN_DEBUG "Initialized Cirrus Logic I2C"
+			       " adapter\n");
+			cinfo->i2c_used = 1;
+		} else
+			printk(KERN_WARNING "Unable to initialize Cirrus"
+			       " Logic I2C adapter (result = %i)\n", err);
+	}
+#endif
+
 	DPRINTK ("EXIT, returning 0\n");
 	return 0;
 
@@ -2350,6 +2421,10 @@ static void __devexit cirrusfb_cleanup (
 	struct cirrusfb_info *cinfo = info->par;
 	DPRINTK ("ENTER\n");
 
+#ifdef CONFIG_FB_CIRRUS_I2C
+	if (cinfo->i2c_used)
+		i2c_bit_del_bus(&cinfo->cirrus_ops);
+#endif
 	switch_monitor (cinfo, 0);
 
 	unregister_framebuffer (info);
diff --git a/include/linux/i2c-id.h b/include/linux/i2c-id.h
index 21338bb..e54c6e3 100644
--- a/include/linux/i2c-id.h
+++ b/include/linux/i2c-id.h
@@ -192,6 +192,7 @@ #define I2C_HW_B_SAVAGE		0x01001d /* sav
 #define I2C_HW_B_RADEON		0x01001e /* radeon framebuffer driver */
 #define I2C_HW_B_EM28XX		0x01001f /* em28xx video capture cards */
 #define I2C_HW_B_CX2341X	0x010020 /* Conexant CX2341X MPEG encoder cards */
+#define I2C_HW_B_CIRRUS		0x010021 /* Cirrus Logic framebuffer driver */
 
 /* --- PCF 8584 based algorithms					*/
 #define I2C_HW_P_LP		0x020000 /* Parallel port interface */

Re: [PATCH v2] cirrus-logic-framebuffer-i2c-support.patch

[Antonino A. Daplas]

Krzysztof Halasa wrote:
> Hi,
> 
> I've attached the second version of the Cirrus Logic I2C patch.
> Supersedes:
> cirrus-logic-framebuffer-i2c-support.patch
> cirrus-logic-framebuffer-i2c-support-fix.patch
> 
> Created against current Linus' tree.
> Thanks.
> 
> Signed-off-by: Krzysztof Halasa <khc@pm.waw.pl>
> 


> @@ -2335,6 +2376,36 @@ static int cirrusfb_register(struct cirr
>  		goto err_dealloc_cmap;
>  	}
>  
> +#ifdef CONFIG_FB_CIRRUS_I2C
> +	cinfo->i2c_used = 0;
> +	if (cinfo->btype == BT_ALPINE || cinfo->btype == BT_PICASSO4) {
> +		vga_wseq(cinfo->regbase, 0x08, 0x41); /* DDC mode: SCL */
> +		vga_wseq(cinfo->regbase, 0x08, 0x43); /* DDC mode: SCL + SDA */
> +		cinfo->bit_cirrus_data.setsda = alpine_setsda;
> +		cinfo->bit_cirrus_data.setscl = alpine_setscl;
> +		cinfo->bit_cirrus_data.getsda = alpine_getsda;
> +		cinfo->bit_cirrus_data.getscl = alpine_getscl;
> +		cinfo->bit_cirrus_data.udelay = 5;
> +		cinfo->bit_cirrus_data.mdelay = 1;
> +		cinfo->bit_cirrus_data.timeout = HZ;
> +		cinfo->bit_cirrus_data.data = cinfo;
> +		cinfo->cirrus_ops.owner = THIS_MODULE;
> +		cinfo->cirrus_ops.id = I2C_HW_B_CIRRUS;
> +		cinfo->cirrus_ops.class = I2C_CLASS_DDC;
> +		cinfo->cirrus_ops.algo_data = &cinfo->bit_cirrus_data;
> +		cinfo->cirrus_ops.dev.parent = info->device;
> +		strlcpy(cinfo->cirrus_ops.name, "Cirrus Logic DDC I2C adapter",
> +			I2C_NAME_SIZE);
> +		if (!(err = i2c_bit_add_bus(&cinfo->cirrus_ops))) {
> +			printk(KERN_DEBUG "Initialized Cirrus Logic I2C"
> +			       " adapter\n");
> +			cinfo->i2c_used = 1;
> +		} else
> +			printk(KERN_WARNING "Unable to initialize Cirrus"
> +			       " Logic I2C adapter (result = %i)\n", err);
> +	}
> +#endif

Why don't you create a separate function for this, ie cirrusfb_create_i2c()
or something. This way, we eliminate the #ifdef/#endif inside the function.

> +
>  	DPRINTK ("EXIT, returning 0\n");
>  	return 0;
>  
> @@ -2350,6 +2421,10 @@ static void __devexit cirrusfb_cleanup (
>  	struct cirrusfb_info *cinfo = info->par;
>  	DPRINTK ("ENTER\n");
>  
> +#ifdef CONFIG_FB_CIRRUS_I2C
> +	if (cinfo->i2c_used)
> +		i2c_bit_del_bus(&cinfo->cirrus_ops);
> +#endif

Same here.

Tony

Re: [PATCH v2] cirrus-logic-framebuffer-i2c-support.patch

[Krzysztof Halasa]

"Antonino A. Daplas" <adaplas@pol.net> writes:

> Why don't you create a separate function for this, ie cirrusfb_create_i2c()
> or something. This way, we eliminate the #ifdef/#endif inside the function.

#ifdef inside a function isn't a problem, while unnecessary complication
(= worse readability) is.
-- 
Krzysztof Halasa

Re: [PATCH v2] cirrus-logic-framebuffer-i2c-support.patch

[Antonino A. Daplas]

Krzysztof Halasa wrote:
> "Antonino A. Daplas" <adaplas@pol.net> writes:
> 
>> Why don't you create a separate function for this, ie cirrusfb_create_i2c()
>> or something. This way, we eliminate the #ifdef/#endif inside the function.
> 
> #ifdef inside a function isn't a problem, while unnecessary complication
> (= worse readability) is.

There are better reasons (ie, smaller patch), but worse readability is not
one of them.

I could more easily grasp the code flow of cirrusfb_register() if you
just inserted "cirrusfb_create_i2c_buses()" instead of:

+		cinfo->bit_cirrus_data.setsda = alpine_setsda;
+		cinfo->bit_cirrus_data.setscl = alpine_setscl;
+		cinfo->bit_cirrus_data.getsda = alpine_getsda;
+		cinfo->bit_cirrus_data.getscl = alpine_getscl;
+		cinfo->bit_cirrus_data.udelay = 5;
+		cinfo->bit_cirrus_data.mdelay = 1;
+		cinfo->bit_cirrus_data.timeout = HZ;
+		cinfo->bit_cirrus_data.data = cinfo;
+		cinfo->cirrus_ops.owner = THIS_MODULE;
+		cinfo->cirrus_ops.id = I2C_HW_B_CIRRUS;
+		cinfo->cirrus_ops.class = I2C_CLASS_DDC;
+		cinfo->cirrus_ops.algo_data = &cinfo->bit_cirrus_data;
+		cinfo->cirrus_ops.dev.parent = info->device;
+		strlcpy(cinfo->cirrus_ops.name, "Cirrus Logic DDC I2C adapter",
+			I2C_NAME_SIZE);
+		if (!(err = i2c_bit_add_bus(&cinfo->cirrus_ops))) {
+			printk(KERN_DEBUG "Initialized Cirrus Logic I2C"
+			       " adapter\n");
 			cinfo->i2c_used = 1;
 		} else
-			printk(KERN_WARNING "Unable to initialize Alpine I2C"
-			       "adapter (result = %i)\n", err);
+			printk(KERN_WARNING "Unable to initialize Cirrus"
+			       " Logic I2C adapter (result = %i)\n", err);

Tony

Re: [PATCH v2] cirrus-logic-framebuffer-i2c-support.patch

[Krzysztof Halasa]

"Antonino A. Daplas" <adaplas@pol.net> writes:

> There are better reasons (ie, smaller patch), but worse readability is not
> one of them.
>
> I could more easily grasp the code flow of cirrusfb_register() if you
> just inserted "cirrusfb_create_i2c_buses()" instead of:

Feel free to add another patch, while I don't see a need I have nothing
against :-)
-- 
Krzysztof Halasa

Re: [PATCH v2] cirrus-logic-framebuffer-i2c-support.patch

[Antonino A. Daplas]

Krzysztof Halasa wrote:
> "Antonino A. Daplas" <adaplas@pol.net> writes:
> 
>> There are better reasons (ie, smaller patch), but worse readability is not
>> one of them.
>>
>> I could more easily grasp the code flow of cirrusfb_register() if you
>> just inserted "cirrusfb_create_i2c_buses()" instead of:
> 
> Feel free to add another patch, while I don't see a need I have nothing
> against :-)

No, you fix the patch.  And while your at it, check your Kconfig
dependencies, ie check for impossible combinations such as CONFIG_I2C=m,
CONFIG_FB_CIRRUS=y.

Tony

Re: [PATCH v2] cirrus-logic-framebuffer-i2c-support.patch

[Krzysztof Halasa]

"Antonino A. Daplas" <adaplas@pol.net> writes:

>> Feel free to add another patch, while I don't see a need I have nothing
>> against :-)
>
> No, you fix the patch.

Look, I don't feel my patch needs such "fix". So if you think it does,
you have to do it.

> And while your at it, check your Kconfig
> dependencies, ie check for impossible combinations such as CONFIG_I2C=m,
> CONFIG_FB_CIRRUS=y.

You're right here, I don't know why I assumed DEPENDS does it
automatically.
-- 
Krzysztof Halasa

Re: [PATCH v2] cirrus-logic-framebuffer-i2c-support.patch

[Antonino A. Daplas]

Krzysztof Halasa wrote:
> "Antonino A. Daplas" <adaplas@pol.net> writes:
> 
>>> Feel free to add another patch, while I don't see a need I have nothing
>>> against :-)
>> No, you fix the patch.
> 
> Look, I don't feel my patch needs such "fix". So if you think it does,
> you have to do it.

Eventually, I'm the one who's going to maintain the code, most
of the drivers in the video directory are practically abandoned. 
Additionally, this is mentioned in Documentation/CodingStyle.

2) #ifdefs are ugly

Code cluttered with ifdefs is difficult to read and maintain.  Don't do
it.  Instead, put your ifdefs in a header, and conditionally define
'static inline' functions, or macros, which are used in the code.
Let the compiler optimize away the "no-op" case.

Simple example, of poor code:

	dev = alloc_etherdev (sizeof(struct funky_private));
	if (!dev)
		return -ENODEV;
	#ifdef CONFIG_NET_FUNKINESS
	init_funky_net(dev);
	#endif

> 
>> And while your at it, check your Kconfig
>> dependencies, ie check for impossible combinations such as CONFIG_I2C=m,
>> CONFIG_FB_CIRRUS=y.
> 
> You're right here, I don't know why I assumed DEPENDS does it
> automatically.

Use select.

Tony

Re: [PATCH v2] cirrus-logic-framebuffer-i2c-support.patch

[Krzysztof Halasa]

"Antonino A. Daplas" <adaplas@pol.net> writes:

>> You're right here, I don't know why I assumed DEPENDS does it
>> automatically.
>
> Use select.

It has a known problem - if the "selected" thing changes (requires
another option etc) things are screwed. With "depends on", you don't
have to track such changes.

While near selects (in the same build directory) are IMHO ok, far
ones are not.

I think it would be different if trying to select something which
can't be selected automatically resulted in a warning. I think
I have to look at it then, but for now I'll use something like
"depends on (FB_CIRRUS=m && I2C && I2C_ALGOBIT) ||
(FB_CIRRUS=y && I2C=y && I2C_ALGOBIT=y)".
-- 
Krzysztof Halasa

Re: [PATCH v2] cirrus-logic-framebuffer-i2c-support.patch

[Antonino A. Daplas]

Krzysztof Halasa wrote:
> "Antonino A. Daplas" <adaplas@pol.net> writes:
> 
>>> You're right here, I don't know why I assumed DEPENDS does it
>>> automatically.
>> Use select.
> 
> It has a known problem - if the "selected" thing changes (requires
> another option etc) things are screwed. With "depends on", you don't
> have to track such changes.
> 
> While near selects (in the same build directory) are IMHO ok, far
> ones are not.
> 
> I think it would be different if trying to select something which
> can't be selected automatically resulted in a warning. I think
> I have to look at it then, but for now I'll use something like
> "depends on (FB_CIRRUS=m && I2C && I2C_ALGOBIT) ||
> (FB_CIRRUS=y && I2C=y && I2C_ALGOBIT=y)".

Well if the i2c code happens to depend on another module, I hope
that Jean would warn us in a timely manner :-). And even if Jean
failed to do so, it would immediately result in a compile
error/warning which should lead to an easy fix.

I still prefer 'select' just because it's easier to parse, but
either way is okay, though your method takes me a few more seconds
to understand.

Tony

Re: [PATCH v2] cirrus-logic-framebuffer-i2c-support.patch

[Krzysztof Halasa]

"Antonino A. Daplas" <adaplas@gmail.com> writes:

> Well if the i2c code happens to depend on another module, I hope
> that Jean would warn us in a timely manner :-).

It doesn't have to be another module. Just another config option.

> And even if Jean
> failed to do so, it would immediately result in a compile
> error/warning which should lead to an easy fix.

The recent events with selecting HDLC for synclink adapters don't
exactly confirm that but I have to say I'm not comfortable with
"depends on" either (because configuring the kernel is harder).

Will try to invent something :-)
-- 
Krzysztof Halasa

Re: [PATCH v2] cirrus-logic-framebuffer-i2c-support.patch

[Jean Delvare]

Hi Tony & Krzysztof,

> > I think it would be different if trying to select something which
> > can't be selected automatically resulted in a warning. I think
> > I have to look at it then, but for now I'll use something like
> > "depends on (FB_CIRRUS=m && I2C && I2C_ALGOBIT) ||
> > (FB_CIRRUS=y && I2C=y && I2C_ALGOBIT=y)".
> 
> Well if the i2c code happens to depend on another module, I hope
> that Jean would warn us in a timely manner :-). And even if Jean
> failed to do so, it would immediately result in a compile
> error/warning which should lead to an easy fix.

I2C and I2C_ALGOBIT are very unlikely to ever depend on anything, so
this isn't a problem.

> I still prefer 'select' just because it's easier to parse, but
> either way is okay, though your method takes me a few more seconds
> to understand.

The Right Way (TM) is to depend on I2C but select I2C_ALGOBIT. I have
plans to hide I2C_ALGOBIT from the configuration menu - the user should
never have to explicitely select it, it's only an helper module. So if
you depend on it, you'll be screwed.

-- 
Jean Delvare

Re: [PATCH v2] cirrus-logic-framebuffer-i2c-support.patch

[Krzysztof Halasa]

"Antonino A. Daplas" <adaplas@pol.net> writes:

> Eventually, I'm the one who's going to maintain the code, most
> of the drivers in the video directory are practically abandoned. 

BTW, it's fortunate that you are maintaing it. The I2C code in cirrusfb
uses vga_wseq() and vga_rseq(). Is it safe WRT races between I2C
adapter code and fb code? I don't see any locking here, and both
functions are non-atomic (write merging and posting will not break it,
but it looks like I need a lock for concurent access).
-- 
Krzysztof Halasa

Re: [PATCH v2] cirrus-logic-framebuffer-i2c-support.patch

[Antonino A. Daplas]

Krzysztof Halasa wrote:
> "Antonino A. Daplas" <adaplas@pol.net> writes:
> 
>> Eventually, I'm the one who's going to maintain the code, most
>> of the drivers in the video directory are practically abandoned. 
> 
> BTW, it's fortunate that you are maintaing it.

David Eger is the author, the last I heard from him was 2 years ago.
But I haven't received that many problem reports on cirrusfb.

However, changes that affect all drivers are my responsibility.
 
> The I2C code in cirrusfb
> uses vga_wseq() and vga_rseq(). Is it safe WRT races between I2C
> adapter code and fb code? I don't see any locking here, and both
> functions are non-atomic (write merging and posting will not break it,
> but it looks like I need a lock for concurent access).

The only register touched by the i2c code is EEPROM control (CL_SEQR8).
This is never touched by the rest of the cirrusfb code. So I don't
think concurrent access is a problem (unless the hardware has restrictions
such as no other register accesses are allowed while this register is being
accessed). 

The framebuffer layer is serialized by
acquire_console_sem()/release_console_sem(). If you think concurrent access
is a problem, you can always use that.

Tony

Re: [PATCH v2] cirrus-logic-framebuffer-i2c-support.patch

[Krzysztof Halasa]

"Antonino A. Daplas" <adaplas@gmail.com> writes:

> David Eger is the author, the last I heard from him was 2 years ago.
> But I haven't received that many problem reports on cirrusfb.

Doesn't matter, what is important is that you know the stuff.

> The only register touched by the i2c code is EEPROM control (CL_SEQR8).
> This is never touched by the rest of the cirrusfb code. So I don't
> think concurrent access is a problem (unless the hardware has restrictions
> such as no other register accesses are allowed while this register is being
> accessed).

I mean I'm using (simplified):

unsigned char vga_rseq (void *regbase, unsigned char reg)
{
        vga_w (regbase, VGA_SEQ_I, reg);
        return vga_r (regbase, VGA_SEQ_D);
}

and

void vga_wseq (void *regbase, unsigned char reg, unsigned char val)
{
#ifdef VGA_OUTW_WRITE
        vga_w_fast (regbase, VGA_SEQ_I, reg, val);
#else
        vga_w (regbase, VGA_SEQ_I, reg);
        vga_w (regbase, VGA_SEQ_D, val);
#endif /* VGA_OUTW_WRITE */
}


How do I know the following will not happen:

taskA) alpine_getsda() called from I2C subsystem
 taskB) cirrusfb_blank() called from elsewhere
taskA) vga_rseq() calls vga_w()
 taskB) vga_rseq() or _wseq() calls vga_w()
taskA) vga_r() performed with wrong index (set by taskB).

I see most of vga_[rw]seq() calls are at init/set_mode time but still
this could be a problem.

This question isn't limited to cirrusfb only, of course.

> The framebuffer layer is serialized by
> acquire_console_sem()/release_console_sem(). If you think concurrent access
> is a problem, you can always use that.

It's quite big... While I haven't investigated that, I rather thought
about some small lock for vga_rseq() and vga_wseq(). Not sure.

Another thing... How is access to VGA registers shared between
X11 and the framebuffer?
-- 
Krzysztof Halasa

Re: [PATCH v2] cirrus-logic-framebuffer-i2c-support.patch

[Antonino A. Daplas]

Krzysztof Halasa wrote:
> "Antonino A. Daplas" <adaplas@gmail.com> writes:
> 
>> David Eger is the author, the last I heard from him was 2 years ago.
>> But I haven't received that many problem reports on cirrusfb.
> 
> Doesn't matter, what is important is that you know the stuff.
> 
>> The only register touched by the i2c code is EEPROM control (CL_SEQR8).
>> This is never touched by the rest of the cirrusfb code. So I don't
>> think concurrent access is a problem (unless the hardware has restrictions
>> such as no other register accesses are allowed while this register is being
>> accessed).
> 
> I mean I'm using (simplified):

Yes, I realized that.

>>> The framebuffer layer is serialized by
>> acquire_console_sem()/release_console_sem(). If you think concurrent access
>> is a problem, you can always use that.
> 
> It's quite big... While I haven't investigated that, I rather thought
> about some small lock for vga_rseq() and vga_wseq(). Not sure.

Yes, the console semaphore is quite heavy. It's also used to flush the
printk buffer.  Since the framebuffer can be called in any context,
spinlocks may be the least expensive.

> 
> Another thing... How is access to VGA registers shared between
> X11 and the framebuffer?

This is no-man's land.  Basically X grabs the VT with KD_GRAPHICS mode
set.  When in KD_GRAPHICS mode, the framebuffer console will not
send any commands to the drivers. The problem is trying to do
framebuffer operation while in X, we don't have any guards on that.
Just try fbset mymode while in X.

Re: [PATCH v2] cirrus-logic-framebuffer-i2c-support.patch

[Krzysztof Halasa]

"Antonino A. Daplas" <adaplas@pol.net> writes:

> This is no-man's land.  Basically X grabs the VT with KD_GRAPHICS mode
> set.  When in KD_GRAPHICS mode, the framebuffer console will not
> send any commands to the drivers. The problem is trying to do
> framebuffer operation while in X, we don't have any guards on that.
> Just try fbset mymode while in X.

You mean it will not bomb? Good, but still there is I2C question
- can I2C accesses to the graphics chip corrupt X11 and vice versa?
I can't see anything preventing that, and while we can disable
graphic operations while Xserver is running, we can't disable I2C
(non-DDC).

Probably X11 should be disallowed to use I2C directly? I should
probably Cc: this to XOrg as well...
-- 
Krzysztof Halasa

Re: [PATCH v2] cirrus-logic-framebuffer-i2c-support.patch

[Antonino A. Daplas]

Krzysztof Halasa wrote:
> "Antonino A. Daplas" <adaplas@pol.net> writes:
> 
>> This is no-man's land.  Basically X grabs the VT with KD_GRAPHICS mode
>> set.  When in KD_GRAPHICS mode, the framebuffer console will not
>> send any commands to the drivers. The problem is trying to do
>> framebuffer operation while in X, we don't have any guards on that.
>> Just try fbset mymode while in X.
> 
> You mean it will not bomb?

If you pray hard enough and you do the operation while in a VC, no it
won't :-). But it's almost guaranteed to bomb if you do the framebuffer
operations while in X.

> Good, but still there is I2C question
> - can I2C accesses to the graphics chip corrupt X11 and vice versa?

That I don't know, but I doubt it.

> I can't see anything preventing that, and while we can disable
> graphic operations while Xserver is running, we can't disable I2C
> (non-DDC).

No, there's nothing to prevent simultaneous access.

> 
> Probably X11 should be disallowed to use I2C directly? I should
> probably Cc: this to XOrg as well...

X has its own i2c functionality which is completely separate from the
kernel i2c layer.

Tony

Re: [PATCH v2] cirrus-logic-framebuffer-i2c-support.patch

[Krzysztof Halasa]

"Antonino A. Daplas" <adaplas@pol.net> writes:

> X has its own i2c functionality which is completely separate from the
> kernel i2c layer.

Yes, but X11 could use I2C adapter functionality provided by the
kernel.
-- 
Krzysztof Halasa

Re: [PATCH v2] cirrus-logic-framebuffer-i2c-support.patch

[Jean Delvare]

> > X has its own i2c functionality which is completely separate from the
> > kernel i2c layer.
> 
> Yes, but X11 could use I2C adapter functionality provided by the
> kernel.

Yes, it should. Now, go convince the X folks to do so...

In practice, I would guess that both X and the framebuffer drivers only
use the I2C/DDC channel to read the monitor's EDID at initialization
time, so the risk of concurrent accesses is thin.

Good luck,
-- 
Jean Delvare

Re: [PATCH v2] cirrus-logic-framebuffer-i2c-support.patch

[Krzysztof Halasa]

Hi,

Jean Delvare <khali@linux-fr.org> writes:

> In practice, I would guess that both X and the framebuffer drivers only
> use the I2C/DDC channel to read the monitor's EDID at initialization
> time, so the risk of concurrent accesses is thin.

Yes. It's a bit different when I log console messages to the EEPROM
(connected to VGA DDC pins), but still... I don't have X on this
machine :-)

Looks like this very small project gets bigger.
-- 
Krzysztof Halasa

Re: [PATCH v2] cirrus-logic-framebuffer-i2c-support.patch

[Antonino A. Daplas]

Krzysztof Halasa wrote:
> "Antonino A. Daplas" <adaplas@pol.net> writes:
> 
>> Eventually, I'm the one who's going to maintain the code, most
>> of the drivers in the video directory are practically abandoned. 
> 
> BTW, it's fortunate that you are maintaing it. The I2C code in cirrusfb
> uses vga_wseq() and vga_rseq(). Is it safe WRT races between I2C
> adapter code and fb code? I don't see any locking here, and both
> functions are non-atomic (write merging and posting will not break it,
> but it looks like I need a lock for concurent access).

Correcting myself: vga_wseq() is not a problem in little endian machines
because the 2 single-byte writes are done with a single 2-byte write.
vga_rseq() is not safe though.

I don't know how big a problem this is, reading the i2c bus while the
video mode is changing..., but I agree that it might be worthwhile
to at least lock sequencer access.

Tony