[PATCH 1/2] acpi,backlight: MSI S270 laptop support - ec_transaction()

[PATCH 1/2] acpi,backlight: MSI S270 laptop support - ec_transaction()

[Lennart Poettering]

From: Lennart Poettering <mzxreary@0pointer.de>

The attached patch unifies a few functions in ec.c:

    acpi_ec_poll_read()
    acpi_ec_poll_write()
    acpi_ec_poll_query()
    acpi_ec_intr_read()
    acpi_ec_intr_write()
    acpi_ec_intr_query()

They consist of very similar code which I unified as:

    acpi_ec_poll_transaction()
    acpi_ec_intr_transaction()

These new functions take as arguments an ACPI EC command, a few bytes
to write to the EC data register and a buffer for a few bytes to read
from the EC data register. The old _read(), _write(), _query() are
just special cases of these functions.

This saves a lot of very similar code. The primary reason for doing
this, however, is that my driver for MSI 270 laptops needs to issue
some non-standard EC commands in a safe way. Due to this I added a new
exported function similar to ec_write()/ec_write() which is called
ec_transaction() and is essentially just a wrapper around
acpi_ec_{poll,intr}_transaction().

Based on 2.6.17.

Please comment and/or apply!

Lennart

Signed-off-by: Lennart Poettering <mzxreary@0pointer.de>
---
--- linux-source-2.6.17/drivers/acpi/ec.c	2006-06-18 03:49:35.000000000 +0200
+++ linux-source-2.6.17.lennart/drivers/acpi/ec.c	2006-08-10 01:28:22.000000000 +0200
@@ -122,12 +122,12 @@ union acpi_ec {
 
 static int acpi_ec_poll_wait(union acpi_ec *ec, u8 event);
 static int acpi_ec_intr_wait(union acpi_ec *ec, unsigned int event);
-static int acpi_ec_poll_read(union acpi_ec *ec, u8 address, u32 * data);
-static int acpi_ec_intr_read(union acpi_ec *ec, u8 address, u32 * data);
-static int acpi_ec_poll_write(union acpi_ec *ec, u8 address, u8 data);
-static int acpi_ec_intr_write(union acpi_ec *ec, u8 address, u8 data);
-static int acpi_ec_poll_query(union acpi_ec *ec, u32 * data);
-static int acpi_ec_intr_query(union acpi_ec *ec, u32 * data);
+static int acpi_ec_poll_transaction(union acpi_ec *ec, u8 command,
+                                    const u8 *wdata, unsigned wdata_len,
+                                    u8 *rdata, unsigned rdata_len);
+static int acpi_ec_intr_transaction(union acpi_ec *ec, u8 command,
+                                    const u8 *wdata, unsigned wdata_len,
+                                    u8 *rdata, unsigned rdata_len);
 static void acpi_ec_gpe_poll_query(void *ec_cxt);
 static void acpi_ec_gpe_intr_query(void *ec_cxt);
 static u32 acpi_ec_gpe_poll_handler(void *data);
@@ -305,78 +305,46 @@ end:
 }
 #endif /* ACPI_FUTURE_USAGE */
 
+static int acpi_ec_transaction(union acpi_ec *ec, u8 command,
+                               const u8 *wdata, unsigned wdata_len,
+                               u8 *rdata, unsigned rdata_len)
+{
+        if (acpi_ec_poll_mode)
+                return acpi_ec_poll_transaction(ec, command, wdata, wdata_len, rdata, rdata_len);
+        else
+                return acpi_ec_intr_transaction(ec, command, wdata, wdata_len, rdata, rdata_len);
+}
+
 static int acpi_ec_read(union acpi_ec *ec, u8 address, u32 * data)
 {
-	if (acpi_ec_poll_mode)
-		return acpi_ec_poll_read(ec, address, data);
-	else
-		return acpi_ec_intr_read(ec, address, data);
+        int result;
+        u8 d;
+        result = acpi_ec_transaction(ec, ACPI_EC_COMMAND_READ, &address, 1, &d, 1);
+        *data = d;
+        return result;
 }
 static int acpi_ec_write(union acpi_ec *ec, u8 address, u8 data)
 {
-	if (acpi_ec_poll_mode)
-		return acpi_ec_poll_write(ec, address, data);
-	else
-		return acpi_ec_intr_write(ec, address, data);
+        u8 wdata[2] = { address, data };
+        return acpi_ec_transaction(ec, ACPI_EC_COMMAND_WRITE, wdata, 2, NULL, 0);
 }
-static int acpi_ec_poll_read(union acpi_ec *ec, u8 address, u32 * data)
+
+static int acpi_ec_poll_transaction(union acpi_ec *ec, u8 command,
+                                    const u8 *wdata, unsigned wdata_len,
+                                    u8 *rdata, unsigned rdata_len)
 {
 	acpi_status status = AE_OK;
 	int result = 0;
 	unsigned long flags = 0;
 	u32 glk = 0;
 
-	ACPI_FUNCTION_TRACE("acpi_ec_read");
+	ACPI_FUNCTION_TRACE("acpi_ec_poll_transaction");
 
-	if (!ec || !data)
+	if (!ec || !wdata || !wdata_len || (rdata_len && !rdata))
 		return_VALUE(-EINVAL);
 
-	*data = 0;
-
-	if (ec->common.global_lock) {
-		status = acpi_acquire_global_lock(ACPI_EC_UDELAY_GLK, &glk);
-		if (ACPI_FAILURE(status))
-			return_VALUE(-ENODEV);
-	}
-
-	spin_lock_irqsave(&ec->poll.lock, flags);
-
-	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_READ,
-				&ec->common.command_addr);
-	result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
-	if (result)
-		goto end;
-
-	acpi_hw_low_level_write(8, address, &ec->common.data_addr);
-	result = acpi_ec_wait(ec, ACPI_EC_EVENT_OBF);
-	if (result)
-		goto end;
-
-	acpi_hw_low_level_read(8, data, &ec->common.data_addr);
-
-	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Read [%02x] from address [%02x]\n",
-			  *data, address));
-
-      end:
-	spin_unlock_irqrestore(&ec->poll.lock, flags);
-
-	if (ec->common.global_lock)
-		acpi_release_global_lock(glk);
-
-	return_VALUE(result);
-}
-
-static int acpi_ec_poll_write(union acpi_ec *ec, u8 address, u8 data)
-{
-	int result = 0;
-	acpi_status status = AE_OK;
-	unsigned long flags = 0;
-	u32 glk = 0;
-
-	ACPI_FUNCTION_TRACE("acpi_ec_write");
-
-	if (!ec)
-		return_VALUE(-EINVAL);
+        if (rdata)
+                memset(rdata, 0, rdata_len);
 
 	if (ec->common.global_lock) {
 		status = acpi_acquire_global_lock(ACPI_EC_UDELAY_GLK, &glk);
@@ -386,25 +354,35 @@ static int acpi_ec_poll_write(union acpi
 
 	spin_lock_irqsave(&ec->poll.lock, flags);
 
-	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_WRITE,
-				&ec->common.command_addr);
-	result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
-	if (result)
-		goto end;
-
-	acpi_hw_low_level_write(8, address, &ec->common.data_addr);
-	result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
-	if (result)
-		goto end;
-
-	acpi_hw_low_level_write(8, data, &ec->common.data_addr);
+	acpi_hw_low_level_write(8, command, &ec->common.command_addr);
 	result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
 	if (result)
 		goto end;
 
-	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Wrote [%02x] to address [%02x]\n",
-			  data, address));
+        while (wdata_len > 0) {
 
+                acpi_hw_low_level_write(8, *(wdata++), &ec->common.data_addr);
+                
+                result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
+                if (result)
+                        goto end;
+                
+                wdata_len--;
+        }
+
+        while (rdata_len > 0) {
+                u32 d;
+                
+                result = acpi_ec_wait(ec, ACPI_EC_EVENT_OBF);
+                if (result)
+                        goto end;
+
+                acpi_hw_low_level_read(8, &d, &ec->common.data_addr);
+                *(rdata++) = (u8) d;
+                
+                rdata_len--;
+        }
+        
       end:
 	spin_unlock_irqrestore(&ec->poll.lock, flags);
 
@@ -414,17 +392,20 @@ static int acpi_ec_poll_write(union acpi
 	return_VALUE(result);
 }
 
-static int acpi_ec_intr_read(union acpi_ec *ec, u8 address, u32 * data)
+static int acpi_ec_intr_transaction(union acpi_ec *ec, u8 command,
+                                    const u8 *wdata, unsigned wdata_len,
+                                    u8 *rdata, unsigned rdata_len)
 {
 	int status = 0;
 	u32 glk;
 
-	ACPI_FUNCTION_TRACE("acpi_ec_read");
+	ACPI_FUNCTION_TRACE("acpi_ec_intr_transaction");
 
-	if (!ec || !data)
+	if (!ec || !wdata || !wdata_len || (rdata_len && !rdata))
 		return_VALUE(-EINVAL);
 
-	*data = 0;
+        if (rdata)
+                memset(rdata, 0, rdata_len);
 
 	if (ec->common.global_lock) {
 		status = acpi_acquire_global_lock(ACPI_EC_UDELAY_GLK, &glk);
@@ -440,22 +421,39 @@ static int acpi_ec_intr_read(union acpi_
 		printk(KERN_DEBUG PREFIX "read EC, IB not empty\n");
 		goto end;
 	}
-	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_READ,
-				&ec->common.command_addr);
+        
+	acpi_hw_low_level_write(8, command, &ec->common.command_addr);
 	status = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
 	if (status) {
 		printk(KERN_DEBUG PREFIX "read EC, IB not empty\n");
+                goto end;
 	}
 
-	acpi_hw_low_level_write(8, address, &ec->common.data_addr);
-	status = acpi_ec_wait(ec, ACPI_EC_EVENT_OBF);
-	if (status) {
-		printk(KERN_DEBUG PREFIX "read EC, OB not full\n");
-		goto end;
-	}
-	acpi_hw_low_level_read(8, data, &ec->common.data_addr);
-	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Read [%02x] from address [%02x]\n",
-			  *data, address));
+        while (wdata_len > 0) {
+                
+                acpi_hw_low_level_write(8, *(wdata++), &ec->common.data_addr);
+
+                status = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
+                if (status) {
+                        printk(KERN_DEBUG PREFIX "read EC, IB not empty\n");
+                        goto end;
+                }
+                
+                wdata_len--;
+        }
+
+        while (rdata_len > 0) {
+                u32 d;
+                
+                status = acpi_ec_wait(ec, ACPI_EC_EVENT_OBF);
+                if (status) {
+                        printk(KERN_DEBUG PREFIX "read EC, OB not full\n");
+                        goto end;
+                }
+                acpi_hw_low_level_read(8, &d, &ec->common.data_addr);
+                *(rdata++) = (u8) d;
+                rdata_len--;
+        }
 
       end:
 	up(&ec->intr.sem);
@@ -466,55 +464,6 @@ static int acpi_ec_intr_read(union acpi_
 	return_VALUE(status);
 }
 
-static int acpi_ec_intr_write(union acpi_ec *ec, u8 address, u8 data)
-{
-	int status = 0;
-	u32 glk;
-
-	ACPI_FUNCTION_TRACE("acpi_ec_write");
-
-	if (!ec)
-		return_VALUE(-EINVAL);
-
-	if (ec->common.global_lock) {
-		status = acpi_acquire_global_lock(ACPI_EC_UDELAY_GLK, &glk);
-		if (ACPI_FAILURE(status))
-			return_VALUE(-ENODEV);
-	}
-
-	WARN_ON(in_interrupt());
-	down(&ec->intr.sem);
-
-	status = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
-	if (status) {
-		printk(KERN_DEBUG PREFIX "write EC, IB not empty\n");
-	}
-	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_WRITE,
-				&ec->common.command_addr);
-	status = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
-	if (status) {
-		printk(KERN_DEBUG PREFIX "write EC, IB not empty\n");
-	}
-
-	acpi_hw_low_level_write(8, address, &ec->common.data_addr);
-	status = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
-	if (status) {
-		printk(KERN_DEBUG PREFIX "write EC, IB not empty\n");
-	}
-
-	acpi_hw_low_level_write(8, data, &ec->common.data_addr);
-
-	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Wrote [%02x] to address [%02x]\n",
-			  data, address));
-
-	up(&ec->intr.sem);
-
-	if (ec->common.global_lock)
-		acpi_release_global_lock(glk);
-
-	return_VALUE(status);
-}
-
 /*
  * Externally callable EC access functions. For now, assume 1 EC only
  */
@@ -557,106 +506,41 @@ int ec_write(u8 addr, u8 val)
 
 EXPORT_SYMBOL(ec_write);
 
-static int acpi_ec_query(union acpi_ec *ec, u32 * data)
-{
-	if (acpi_ec_poll_mode)
-		return acpi_ec_poll_query(ec, data);
-	else
-		return acpi_ec_intr_query(ec, data);
-}
-static int acpi_ec_poll_query(union acpi_ec *ec, u32 * data)
+int ec_transaction(u8 command,
+                   const u8 *wdata, unsigned wdata_len,
+                   u8 *rdata, unsigned rdata_len)
 {
-	int result = 0;
-	acpi_status status = AE_OK;
-	unsigned long flags = 0;
-	u32 glk = 0;
-
-	ACPI_FUNCTION_TRACE("acpi_ec_query");
-
-	if (!ec || !data)
-		return_VALUE(-EINVAL);
-
-	*data = 0;
-
-	if (ec->common.global_lock) {
-		status = acpi_acquire_global_lock(ACPI_EC_UDELAY_GLK, &glk);
-		if (ACPI_FAILURE(status))
-			return_VALUE(-ENODEV);
-	}
-
-	/*
-	 * Query the EC to find out which _Qxx method we need to evaluate.
-	 * Note that successful completion of the query causes the ACPI_EC_SCI
-	 * bit to be cleared (and thus clearing the interrupt source).
-	 */
-	spin_lock_irqsave(&ec->poll.lock, flags);
-
-	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_QUERY,
-				&ec->common.command_addr);
-	result = acpi_ec_wait(ec, ACPI_EC_EVENT_OBF);
-	if (result)
-		goto end;
-
-	acpi_hw_low_level_read(8, data, &ec->common.data_addr);
-	if (!*data)
-		result = -ENODATA;
+	union acpi_ec *ec;
 
-      end:
-	spin_unlock_irqrestore(&ec->poll.lock, flags);
+	if (!first_ec)
+		return -ENODEV;
 
-	if (ec->common.global_lock)
-		acpi_release_global_lock(glk);
+	ec = acpi_driver_data(first_ec);
 
-	return_VALUE(result);
+	return acpi_ec_transaction(ec, command, wdata, wdata_len, rdata, rdata_len);
 }
-static int acpi_ec_intr_query(union acpi_ec *ec, u32 * data)
-{
-	int status = 0;
-	u32 glk;
-
-	ACPI_FUNCTION_TRACE("acpi_ec_query");
 
-	if (!ec || !data)
-		return_VALUE(-EINVAL);
-	*data = 0;
+EXPORT_SYMBOL(ec_transaction);
 
-	if (ec->common.global_lock) {
-		status = acpi_acquire_global_lock(ACPI_EC_UDELAY_GLK, &glk);
-		if (ACPI_FAILURE(status))
-			return_VALUE(-ENODEV);
-	}
-
-	down(&ec->intr.sem);
-
-	status = acpi_ec_wait(ec, ACPI_EC_EVENT_IBE);
-	if (status) {
-		printk(KERN_DEBUG PREFIX "query EC, IB not empty\n");
-		goto end;
-	}
+static int acpi_ec_query(union acpi_ec *ec, u32 * data)
+{
+        int result;
+        u8 d;
+        
 	/*
 	 * Query the EC to find out which _Qxx method we need to evaluate.
 	 * Note that successful completion of the query causes the ACPI_EC_SCI
 	 * bit to be cleared (and thus clearing the interrupt source).
 	 */
-	acpi_hw_low_level_write(8, ACPI_EC_COMMAND_QUERY,
-				&ec->common.command_addr);
-	status = acpi_ec_wait(ec, ACPI_EC_EVENT_OBF);
-	if (status) {
-		printk(KERN_DEBUG PREFIX "query EC, OB not full\n");
-		goto end;
-	}
 
-	acpi_hw_low_level_read(8, data, &ec->common.data_addr);
-	if (!*data)
-		status = -ENODATA;
+        if ((result = acpi_ec_transaction(ec, ACPI_EC_COMMAND_QUERY, NULL, 0, &d, 1)))
+                return_VALUE(result);
 
-      end:
-	up(&ec->intr.sem);
+        if (!d)
+		return_VALUE(-ENODATA);
 
-	if (ec->common.global_lock)
-		acpi_release_global_lock(glk);
-
-	return_VALUE(status);
+        *data = d;
+        return_VALUE(0);
 }
 
 /* --------------------------------------------------------------------------
--- linux-source-2.6.17/include/linux/acpi.h	2006-06-18 03:49:35.000000000 +0200
+++ linux-source-2.6.17.lennart/include/linux/acpi.h	2006-08-08 17:45:24.000000000 +0200
@@ -486,6 +486,9 @@ void acpi_pci_unregister_driver(struct a
 
 extern int ec_read(u8 addr, u8 *val);
 extern int ec_write(u8 addr, u8 val);
+extern int ec_transaction(u8 command,
+                          const u8 *wdata, unsigned wdata_len,
+                          u8 *rdata, unsigned rdata_len);
 
 #endif /*CONFIG_ACPI_EC*/
 

RE: [PATCH 1/2] acpi,backlight: MSI S270 laptop support - ec_transaction()

[Yu, Luming]

First of all, thanks for your patch.
 
>+static int acpi_ec_transaction(union acpi_ec *ec, u8 command,
>+                               const u8 *wdata, unsigned wdata_len,
>+                               u8 *rdata, unsigned rdata_len)

I agree the name: transaction sounds better than read/write, and
can reduce redundant code from separate read, write function.
But, I guess using argument : u8 address will make the patch
looks better.

>+{
>+        if (acpi_ec_poll_mode)
>+                return acpi_ec_poll_transaction(ec, command, 
>wdata, wdata_len, rdata, rdata_len);
>+        else
>+                return acpi_ec_intr_transaction(ec, command, 
>wdata, wdata_len, rdata, rdata_len);
>+}
>+

It would be better to use a function pointer instead of
using if-lese statement which looks not so neat.

> static int acpi_ec_read(union acpi_ec *ec, u8 address, u32 * data)
> {
>-	if (acpi_ec_poll_mode)
>-		return acpi_ec_poll_read(ec, address, data);
>-	else
>-		return acpi_ec_intr_read(ec, address, data);
>+        int result;
>+        u8 d;
>+        result = acpi_ec_transaction(ec, 
>ACPI_EC_COMMAND_READ, &address, 1, &d, 1);
>+        *data = d;
>+        return result;
> }

Due to missing argument: address, you have to pass address in
argument: wdata. This kind of code style is prone to error.


> static int acpi_ec_write(union acpi_ec *ec, u8 address, u8 data)
> {
>-	if (acpi_ec_poll_mode)
>-		return acpi_ec_poll_write(ec, address, data);
>-	else
>-		return acpi_ec_intr_write(ec, address, data);
>+        u8 wdata[2] = { address, data };
>+        return acpi_ec_transaction(ec, ACPI_EC_COMMAND_WRITE, 
>wdata, 2, NULL, 0);
> }

It would be more clear if there is argument : address.

>-static int acpi_ec_poll_read(union acpi_ec *ec, u8 address, 
>u32 * data)
>+
>+static int acpi_ec_poll_transaction(union acpi_ec *ec, u8 command,
>+                                    const u8 *wdata, unsigned 
>wdata_len,
>+                                    u8 *rdata, unsigned rdata_len)
> {
> 	acpi_status status = AE_OK;
> 	int result = 0;
> 	unsigned long flags = 0;
> 	u32 glk = 0;
> 
>-	ACPI_FUNCTION_TRACE("acpi_ec_read");
>+	ACPI_FUNCTION_TRACE("acpi_ec_poll_transaction");
> 
>-	if (!ec || !data)
>+	if (!ec || !wdata || !wdata_len || (rdata_len && !rdata))
> 		return_VALUE(-EINVAL);


why return -EINVAL if wdata_len == 0?

Thanks
Luming

Re: [PATCH 1/2] acpi,backlight: MSI S270 laptop support - ec_transaction()

[Lennart Poettering]

On Mon, 14.08.06 23:25, Yu, Luming (luming.yu@intel.com) wrote:

Hi

> First of all, thanks for your patch.

You're welcome!

> >+static int acpi_ec_transaction(union acpi_ec *ec, u8 command,
> >+                               const u8 *wdata, unsigned wdata_len,
> >+                               u8 *rdata, unsigned rdata_len)
> 
> I agree the name: transaction sounds better than read/write, and
> can reduce redundant code from separate read, write function.
> But, I guess using argument : u8 address will make the patch
> looks better.

I don't understand? You mean to add an additional parameter "u8
address" which would more or less be what wdata[0] is in my
suggestion?

This wouldn't work. Some of the EC commands do not take addresses as
arguments. In fact only "RD_EC" (read byte) and "WR_EC" (write byte)
take addresses. All others, BE_EC (burst enable), BD_EC (burst
disable) and QR_EC (query EC) do not take any argument at all.  

Please note that ec_transaction() is *not* an API for reading/writing
a series of bytes from/to EC *memory*. Instead It is an API to write a
command to the EC control port and then read/write a series of bytes
from/to the EC data port.

The reason I did this patch was that I needed a generic way to access
the ACPI EC for my MSI S270 laptop driver. The non-standard EC MSI
builds into its laptops knows a few non-standard commands, besides the
standard commands listed above. These commands are not from the 0x80
range like RD/WR/BE/BD/QR, but from the 0x10 range. The data these
commands take is completely arbitrary, the commands have no notion of
an "address".

> >+{
> >+        if (acpi_ec_poll_mode)
> >+                return acpi_ec_poll_transaction(ec, command, 
> >wdata, wdata_len, rdata, rdata_len);
> >+        else
> >+                return acpi_ec_intr_transaction(ec, command, 
> >wdata, wdata_len, rdata, rdata_len);
> >+}
> >+
> 
> It would be better to use a function pointer instead of
> using if-lese statement which looks not so neat.

Hmm, this is just the way the original acpi_ec_read()/acpi_ec_write()
functions worked. I didn't want to change too much of the logic,
because, honestly, I didn't understand everything in that source
file, such as the full semantics of the "poll" version of the
functions in contrast to the "intr" version.

> > static int acpi_ec_read(union acpi_ec *ec, u8 address, u32 * data)
> > {
> >-	if (acpi_ec_poll_mode)
> >-		return acpi_ec_poll_read(ec, address, data);
> >-	else
> >-		return acpi_ec_intr_read(ec, address, data);
> >+        int result;
> >+        u8 d;
> >+        result = acpi_ec_transaction(ec, 
> >ACPI_EC_COMMAND_READ, &address, 1, &d, 1);
> >+        *data = d;
> >+        return result;
> > }
> 
> Due to missing argument: address, you have to pass address in
> argument: wdata. This kind of code style is prone to error.

See above. 

If we'd pass the address seperately we couldn't use
acpi_ec_transaction() to implement acpi_ec_query(), because QR_EC
doesn't take any address argument (wdata_len = 0). 

> > static int acpi_ec_write(union acpi_ec *ec, u8 address, u8 data)
> > {
> >-	if (acpi_ec_poll_mode)
> >-		return acpi_ec_poll_write(ec, address, data);
> >-	else
> >-		return acpi_ec_intr_write(ec, address, data);
> >+        u8 wdata[2] = { address, data };
> >+        return acpi_ec_transaction(ec, ACPI_EC_COMMAND_WRITE, 
> >wdata, 2, NULL, 0);
> > }
> 
> It would be more clear if there is argument : address.

See above.

> 
> >-static int acpi_ec_poll_read(union acpi_ec *ec, u8 address, 
> >u32 * data)
> >+
> >+static int acpi_ec_poll_transaction(union acpi_ec *ec, u8 command,
> >+                                    const u8 *wdata, unsigned 
> >wdata_len,
> >+                                    u8 *rdata, unsigned rdata_len)
> > {
> > 	acpi_status status = AE_OK;
> > 	int result = 0;
> > 	unsigned long flags = 0;
> > 	u32 glk = 0;
> > 
> >-	ACPI_FUNCTION_TRACE("acpi_ec_read");
> >+	ACPI_FUNCTION_TRACE("acpi_ec_poll_transaction");
> > 
> >-	if (!ec || !data)
> >+	if (!ec || !wdata || !wdata_len || (rdata_len && !rdata))
> > 		return_VALUE(-EINVAL);
> 
> 
> why return -EINVAL if wdata_len == 0?

Got me! This is a bug. This should read:

If (!ec || (wdata_len && !wdata) || (rdata_len && !rdata)) ...

BTW: I noticed that ec.c recieved quite a few cleanups in the
2.6.18-pre kernels. Apparently no logic really changed, but some
tracing has been removed among other minor stuff. My patch doesn't
apply cleanly anymore. 

Shall I prepare an updated patch?

Thanks for reviewing,
        Lennart

-- 
Lennart Poettering; lennart [at] poettering [dot] net
ICQ# 11060553; GPG 0x1A015CC4; http://0pointer.net/lennart/

Re: [PATCH 1/2] acpi,backlight: MSI S270 laptop support - ec_transaction()

[Len Brown]

On Monday 14 August 2006 20:27, Lennart Poettering wrote:

> Shall I prepare an updated patch?

That would be great Lennart.

thanks,
-Len