This patch replace some magic numbers. I believe it makes
the driver more readable.
The magic number 0x26 is the XO system embedded controller
(EC) command 'DCON power enable/disable'.
Number 0x41, and 0x42 are special memory controller settings
register. The 0x41 initialize bit sequence 0x101 means:
enable memory power down function and special SDRAM clock
delay for synchronize SDRAM output and clock signal.
The 0x42 initialize squence 0x101 is wrong. According to
the specification Bit 8 is reserved, thus not in use.
I removed it.
Signed-off-by: Jens Frederich <[email protected]>
diff --git a/drivers/staging/olpc_dcon/olpc_dcon.c b/drivers/staging/olpc_dcon/olpc_dcon.c
index 7c460f2..5ca4fa4 100644
--- a/drivers/staging/olpc_dcon/olpc_dcon.c
+++ b/drivers/staging/olpc_dcon/olpc_dcon.c
@@ -90,9 +90,10 @@ static int dcon_hw_init(struct dcon_priv *dcon, int is_init)
/* SDRAM setup/hold time */
dcon_write(dcon, 0x3a, 0xc040);
- dcon_write(dcon, 0x41, 0x0000);
- dcon_write(dcon, 0x41, 0x0101);
- dcon_write(dcon, 0x42, 0x0101);
+ dcon_write(dcon, DCON_REG_MEM_OPT_A, 0x0000); /* clear option bits */
+ dcon_write(dcon, DCON_REG_MEM_OPT_A,
+ MEM_DLL_CLOCK_DELAY | MEM_POWER_DOWN);
+ dcon_write(dcon, DCON_REG_MEM_OPT_B, MEM_SOFT_RESET);
/* Colour swizzle, AA, no passthrough, backlight */
if (is_init) {
@@ -126,7 +127,7 @@ static int dcon_bus_stabilize(struct dcon_priv *dcon, int is_powered_down)
power_up:
if (is_powered_down) {
x = 1;
- x = olpc_ec_cmd(0x26, (unsigned char *)&x, 1, NULL, 0);
+ x = olpc_ec_cmd(EC_DCON_POWER_MODE, (u8 *)&x, 1, NULL, 0);
if (x) {
pr_warn("unable to force dcon to power up: %d!\n", x);
return x;
@@ -144,7 +145,7 @@ power_up:
pr_err("unable to stabilize dcon's smbus, reasserting power and praying.\n");
BUG_ON(olpc_board_at_least(olpc_board(0xc2)));
x = 0;
- olpc_ec_cmd(0x26, (unsigned char *)&x, 1, NULL, 0);
+ olpc_ec_cmd(EC_DCON_POWER_MODE, (u8 *)&x, 1, NULL, 0);
msleep(100);
is_powered_down = 1;
goto power_up; /* argh, stupid hardware.. */
@@ -208,7 +209,7 @@ static void dcon_sleep(struct dcon_priv *dcon, bool sleep)
if (sleep) {
x = 0;
- x = olpc_ec_cmd(0x26, (unsigned char *)&x, 1, NULL, 0);
+ x = olpc_ec_cmd(EC_DCON_POWER_MODE, (u8 *)&x, 1, NULL, 0);
if (x)
pr_warn("unable to force dcon to power down: %d!\n", x);
else
diff --git a/drivers/staging/olpc_dcon/olpc_dcon.h b/drivers/staging/olpc_dcon/olpc_dcon.h
index 997bded..524ee49 100644
--- a/drivers/staging/olpc_dcon/olpc_dcon.h
+++ b/drivers/staging/olpc_dcon/olpc_dcon.h
@@ -22,15 +22,24 @@
#define MODE_DEBUG (1<<14)
#define MODE_SELFTEST (1<<15)
-#define DCON_REG_HRES 2
-#define DCON_REG_HTOTAL 3
-#define DCON_REG_HSYNC_WIDTH 4
-#define DCON_REG_VRES 5
-#define DCON_REG_VTOTAL 6
-#define DCON_REG_VSYNC_WIDTH 7
-#define DCON_REG_TIMEOUT 8
-#define DCON_REG_SCAN_INT 9
-#define DCON_REG_BRIGHT 10
+#define DCON_REG_HRES 0x2
+#define DCON_REG_HTOTAL 0x3
+#define DCON_REG_HSYNC_WIDTH 0x4
+#define DCON_REG_VRES 0x5
+#define DCON_REG_VTOTAL 0x6
+#define DCON_REG_VSYNC_WIDTH 0x7
+#define DCON_REG_TIMEOUT 0x8
+#define DCON_REG_SCAN_INT 0x9
+#define DCON_REG_BRIGHT 0x10
+#define DCON_REG_MEM_OPT_A 0x41
+#define DCON_REG_MEM_OPT_B 0x42
+
+/* Load Delay Locked Loop (DLL) settings for clock delay */
+#define MEM_DLL_CLOCK_DELAY (1<<0)
+/* Memory controller power down function */
+#define MEM_POWER_DOWN (1<<8)
+/* Memory controller software reset */
+#define MEM_SOFT_RESET (1<<0)
/* Status values */
diff --git a/include/linux/olpc-ec.h b/include/linux/olpc-ec.h
index 5bb6e76..2925df3 100644
--- a/include/linux/olpc-ec.h
+++ b/include/linux/olpc-ec.h
@@ -6,6 +6,7 @@
#define EC_WRITE_SCI_MASK 0x1b
#define EC_WAKE_UP_WLAN 0x24
#define EC_WLAN_LEAVE_RESET 0x25
+#define EC_DCON_POWER_MODE 0x26
#define EC_READ_EB_MODE 0x2a
#define EC_SET_SCI_INHIBIT 0x32
#define EC_SET_SCI_INHIBIT_RELEASE 0x34
--
1.7.9.5
Please Cc Daniel on these. Cjb and myself are no longer at olpc.
On Sat, 3 Aug 2013 22:44:35 +0200
Jens Frederich <[email protected]> wrote:
> This patch replace some magic numbers. I believe it makes
> the driver more readable.
>
> The magic number 0x26 is the XO system embedded controller
> (EC) command 'DCON power enable/disable'.
>
> Number 0x41, and 0x42 are special memory controller settings
> register. The 0x41 initialize bit sequence 0x101 means:
> enable memory power down function and special SDRAM clock
> delay for synchronize SDRAM output and clock signal.
>
> The 0x42 initialize squence 0x101 is wrong. According to
> the specification Bit 8 is reserved, thus not in use.
> I removed it.
>
> Signed-off-by: Jens Frederich <[email protected]>
>
> diff --git a/drivers/staging/olpc_dcon/olpc_dcon.c
> b/drivers/staging/olpc_dcon/olpc_dcon.c index 7c460f2..5ca4fa4 100644
> --- a/drivers/staging/olpc_dcon/olpc_dcon.c
> +++ b/drivers/staging/olpc_dcon/olpc_dcon.c
> @@ -90,9 +90,10 @@ static int dcon_hw_init(struct dcon_priv *dcon,
> int is_init)
> /* SDRAM setup/hold time */
> dcon_write(dcon, 0x3a, 0xc040);
> - dcon_write(dcon, 0x41, 0x0000);
> - dcon_write(dcon, 0x41, 0x0101);
> - dcon_write(dcon, 0x42, 0x0101);
> + dcon_write(dcon, DCON_REG_MEM_OPT_A, 0x0000); /* clear
> option bits */
> + dcon_write(dcon, DCON_REG_MEM_OPT_A,
> + MEM_DLL_CLOCK_DELAY |
> MEM_POWER_DOWN);
> + dcon_write(dcon, DCON_REG_MEM_OPT_B, MEM_SOFT_RESET);
>
> /* Colour swizzle, AA, no passthrough, backlight */
> if (is_init) {
> @@ -126,7 +127,7 @@ static int dcon_bus_stabilize(struct dcon_priv
> *dcon, int is_powered_down) power_up:
> if (is_powered_down) {
> x = 1;
> - x = olpc_ec_cmd(0x26, (unsigned char *)&x, 1, NULL,
> 0);
> + x = olpc_ec_cmd(EC_DCON_POWER_MODE, (u8 *)&x, 1,
> NULL, 0); if (x) {
> pr_warn("unable to force dcon to power up:
> %d!\n", x); return x;
> @@ -144,7 +145,7 @@ power_up:
> pr_err("unable to stabilize dcon's smbus,
> reasserting power and praying.\n");
> BUG_ON(olpc_board_at_least(olpc_board(0xc2))); x = 0;
> - olpc_ec_cmd(0x26, (unsigned char *)&x, 1, NULL, 0);
> + olpc_ec_cmd(EC_DCON_POWER_MODE, (u8 *)&x, 1, NULL,
> 0); msleep(100);
> is_powered_down = 1;
> goto power_up; /* argh, stupid hardware.. */
> @@ -208,7 +209,7 @@ static void dcon_sleep(struct dcon_priv *dcon,
> bool sleep)
> if (sleep) {
> x = 0;
> - x = olpc_ec_cmd(0x26, (unsigned char *)&x, 1, NULL,
> 0);
> + x = olpc_ec_cmd(EC_DCON_POWER_MODE, (u8 *)&x, 1,
> NULL, 0); if (x)
> pr_warn("unable to force dcon to power down:
> %d!\n", x); else
> diff --git a/drivers/staging/olpc_dcon/olpc_dcon.h
> b/drivers/staging/olpc_dcon/olpc_dcon.h index 997bded..524ee49 100644
> --- a/drivers/staging/olpc_dcon/olpc_dcon.h
> +++ b/drivers/staging/olpc_dcon/olpc_dcon.h
> @@ -22,15 +22,24 @@
> #define MODE_DEBUG (1<<14)
> #define MODE_SELFTEST (1<<15)
>
> -#define DCON_REG_HRES 2
> -#define DCON_REG_HTOTAL 3
> -#define DCON_REG_HSYNC_WIDTH 4
> -#define DCON_REG_VRES 5
> -#define DCON_REG_VTOTAL 6
> -#define DCON_REG_VSYNC_WIDTH 7
> -#define DCON_REG_TIMEOUT 8
> -#define DCON_REG_SCAN_INT 9
> -#define DCON_REG_BRIGHT 10
> +#define DCON_REG_HRES 0x2
> +#define DCON_REG_HTOTAL 0x3
> +#define DCON_REG_HSYNC_WIDTH 0x4
> +#define DCON_REG_VRES 0x5
> +#define DCON_REG_VTOTAL 0x6
> +#define DCON_REG_VSYNC_WIDTH 0x7
> +#define DCON_REG_TIMEOUT 0x8
> +#define DCON_REG_SCAN_INT 0x9
> +#define DCON_REG_BRIGHT 0x10
> +#define DCON_REG_MEM_OPT_A 0x41
> +#define DCON_REG_MEM_OPT_B 0x42
> +
> +/* Load Delay Locked Loop (DLL) settings for clock delay */
> +#define MEM_DLL_CLOCK_DELAY (1<<0)
> +/* Memory controller power down function */
> +#define MEM_POWER_DOWN (1<<8)
> +/* Memory controller software reset */
> +#define MEM_SOFT_RESET (1<<0)
>
> /* Status values */
>
> diff --git a/include/linux/olpc-ec.h b/include/linux/olpc-ec.h
> index 5bb6e76..2925df3 100644
> --- a/include/linux/olpc-ec.h
> +++ b/include/linux/olpc-ec.h
> @@ -6,6 +6,7 @@
> #define EC_WRITE_SCI_MASK 0x1b
> #define EC_WAKE_UP_WLAN 0x24
> #define EC_WLAN_LEAVE_RESET 0x25
> +#define EC_DCON_POWER_MODE 0x26
> #define EC_READ_EB_MODE 0x2a
> #define EC_SET_SCI_INHIBIT 0x32
> #define EC_SET_SCI_INHIBIT_RELEASE 0x34
On Sat, Aug 3, 2013 at 11:16 PM, Andres Salomon <[email protected]> wrote:
> Please Cc Daniel on these. Cjb and myself are no longer at olpc.
>
Sorry, I've forgotten it. I will update the the TODO list.
On Sat, Aug 3, 2013 at 11:16 PM, Andres Salomon <[email protected]> wrote:
> Please Cc Daniel on these. Cjb and myself are no longer at olpc.
>
Do you know what's with Jon Nettleton? He is also on the TODO list?
On Sat, 3 Aug 2013 23:36:15 +0200
Jens Frederich <[email protected]> wrote:
> On Sat, Aug 3, 2013 at 11:16 PM, Andres Salomon <[email protected]>
> wrote:
> > Please Cc Daniel on these. Cjb and myself are no longer at olpc.
> >
>
> Do you know what's with Jon Nettleton? He is also on the TODO list?
Let's ask. Jon, do you still want to be Cc'd on DCON and other OLPC
patches?
On Sat, Aug 03, 2013 at 10:44:35PM +0200, Jens Frederich wrote:
> @@ -126,7 +127,7 @@ static int dcon_bus_stabilize(struct dcon_priv *dcon, int is_powered_down)
> power_up:
> if (is_powered_down) {
> x = 1;
> - x = olpc_ec_cmd(0x26, (unsigned char *)&x, 1, NULL, 0);
> + x = olpc_ec_cmd(EC_DCON_POWER_MODE, (u8 *)&x, 1, NULL, 0);
^^^^^^^^
You didn't introduce this but using "x" as the inbuf here messy.
It should be char instead of an int. The code won't work on big
endian systems. I know this hardware is only available on little
endian systems and that's why it's not a bug. It's just an ugly
thing to do.
(Since you didn't introduce this, it means your patch is fine and
you can ignore this email. I am just commenting in case anyone
wants to fix clean it up).
> if (x) {
> pr_warn("unable to force dcon to power up: %d!\n", x);
> return x;
regards,
dan carpenter
On Sat, Aug 03, 2013 at 02:38:45PM -0700, Andres Salomon wrote:
> On Sat, 3 Aug 2013 23:36:15 +0200
> Jens Frederich <[email protected]> wrote:
>
> > On Sat, Aug 3, 2013 at 11:16 PM, Andres Salomon <[email protected]>
> > wrote:
> > > Please Cc Daniel on these. Cjb and myself are no longer at olpc.
> > >
> >
> > Do you know what's with Jon Nettleton? He is also on the TODO list?
>
> Let's ask. Jon, do you still want to be Cc'd on DCON and other OLPC
> patches?
No one reads the TODO files... Just update MAINTAINERS so that
get_maintainer.pl CC's the right people automatically.
regards,
dan carpenter
On Sun, Aug 4, 2013 at 5:57 AM, Jon Nettleton <[email protected]> wrote:
>
> On Aug 3, 2013 11:38 PM, "Andres Salomon" <[email protected]> wrote:
>>
>> On Sat, 3 Aug 2013 23:36:15 +0200
>> Jens Frederich <[email protected]> wrote:
>>
>> > On Sat, Aug 3, 2013 at 11:16 PM, Andres Salomon <[email protected]>
>> > wrote:
>> > > Please Cc Daniel on these. Cjb and myself are no longer at olpc.
>> > >
>> >
>> > Do you know what's with Jon Nettleton? He is also on the TODO list?
>>
>> Let's ask. Jon, do you still want to be Cc'd on DCON and other OLPC
>> patches?
>
> Please. I still get enough support requests in the side that keeping up to
> date in changes us helpful.
>
Okay Jon, you're still on the OLPC CC list.
thanks,
Jens
On Sun, Aug 4, 2013 at 1:14 AM, Dan Carpenter <[email protected]> wrote:
> On Sat, Aug 03, 2013 at 10:44:35PM +0200, Jens Frederich wrote:
>> @@ -126,7 +127,7 @@ static int dcon_bus_stabilize(struct dcon_priv *dcon, int is_powered_down)
>> power_up:
>> if (is_powered_down) {
>> x = 1;
>> - x = olpc_ec_cmd(0x26, (unsigned char *)&x, 1, NULL, 0);
>> + x = olpc_ec_cmd(EC_DCON_POWER_MODE, (u8 *)&x, 1, NULL, 0);
> ^^^^^^^^
> You didn't introduce this but using "x" as the inbuf here messy.
> It should be char instead of an int. The code won't work on big
> endian systems. I know this hardware is only available on little
> endian systems and that's why it's not a bug. It's just an ugly
> thing to do.
>
Hello Dan, you're right. It doesn't matter whether the current
hardware is little or big endian. The driver should be able to
support both. I will fix it soon.
thanks,
Jens