Re: [PATCH] acpi,msi-laptop: Fall back to EC polling mode for MSI laptop specific EC commands

From: Len Brown
Date: Thu May 10 2007 - 03:54:26 EST

Next message: Satyam Sharma: "Re: [PATCH 2/3] Add hard_irq_disable()"
Previous message: Russell King: "Re: [GIT PULL] MMC updates"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Applied.

thanks,
-Len

On Friday 04 May 2007 14:34, Alexey Starikovskiy wrote:
> Ugly, but harmless. ACK.
>
> Regards,
> Alex.
>
> On 5/4/07, Lennart Poettering <mzxreary@xxxxxxxxxxx> wrote:
> > From: Lennart Poettering <mzxreary@xxxxxxxxxxx>
> >
> > The ACPI EC that is used in MSI laptops knows some non-standard
> > commands for changing the screen brighntess and a few other things,
> > which are used by the msi-laptop.c driver. Unfortunately for these
> > commands no GPE events for IBF and OBF are triggered. Since nowadays
> > the EC code uses the ec_intr=1 mode by default, this causes these
> > operations to timeout, although they don't fail. In result, all
> > operations that you can do with the msi-laptop.c driver take more or
> > less 1s to complete, which is awfully slow.
> >
> > In one of the more recent kernels (2.6.20?) the EC subsystem has been
> > revamped. With that change the EC timeout has been increased. before
> > that increase the MSI EC accesses were slow -- but not *that* slow,
> > hence I took notice of this limitation of the MSI EC hardware only very
> > recently.
> >
> > The standard EC operations on the MSI EC as defined in the ACPI spec
> > support GPE events properly.
> >
> > The following patch adds a new argument "force_poll" to the
> > ec_transaction() function (and friends). If set to 1, the function
> > will poll for IBF/OBF even if ec_intr=1 is enabled. If set to 0 the
> > current behaviour is used. The msi-laptop driver is modified to make
> > use of this new flag, so that OBF/IBF is polled for the special MSI EC
> > transactions -- but only for them.
> >
> > Patch is against Linus' current git kernel. Should also apply against
> > Len's current linux-acpi kernel.
> >
> > Len, please merge!
> >
> > Signed-off-by: Lennart Poettering <mzxreary@xxxxxxxxxxx>
> > ---
> > drivers/acpi/ec.c | 39 +++++++++++++++++++++++----------------
> > drivers/misc/msi-laptop.c | 12 ++++++------
> > include/linux/acpi.h | 3 ++-
> > 3 files changed, 31 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> > index e08cf98..82f496c 100644
> > --- a/drivers/acpi/ec.c
> > +++ b/drivers/acpi/ec.c
> > @@ -147,9 +147,10 @@ static inline int acpi_ec_check_status(struct acpi_ec *ec, enum ec_event event,
> > return 0;
> > }
> >
> > -static int acpi_ec_wait(struct acpi_ec *ec, enum ec_event event, unsigned count)
> > +static int acpi_ec_wait(struct acpi_ec *ec, enum ec_event event,
> > + unsigned count, int force_poll)
> > {
> > - if (acpi_ec_mode == EC_POLL) {
> > + if (unlikely(force_poll) || acpi_ec_mode == EC_POLL) {
> > unsigned long delay = jiffies + msecs_to_jiffies(ACPI_EC_DELAY);
> > while (time_before(jiffies, delay)) {
> > if (acpi_ec_check_status(ec, event, 0))
> > @@ -173,14 +174,15 @@ static int acpi_ec_wait(struct acpi_ec *ec, enum ec_event event, unsigned count)
> >
> > static int acpi_ec_transaction_unlocked(struct acpi_ec *ec, u8 command,
> > const u8 * wdata, unsigned wdata_len,
> > - u8 * rdata, unsigned rdata_len)
> > + u8 * rdata, unsigned rdata_len,
> > + int force_poll)
> > {
> > int result = 0;
> > unsigned count = atomic_read(&ec->event_count);
> > acpi_ec_write_cmd(ec, command);
> >
> > for (; wdata_len > 0; --wdata_len) {
> > - result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0, count);
> > + result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0, count, force_poll);
> > if (result) {
> > printk(KERN_ERR PREFIX
> > "write_cmd timeout, command = %d\n", command);
> > @@ -191,7 +193,7 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec, u8 command,
> > }
> >
> > if (!rdata_len) {
> > - result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0, count);
> > + result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0, count, force_poll);
> > if (result) {
> > printk(KERN_ERR PREFIX
> > "finish-write timeout, command = %d\n", command);
> > @@ -202,7 +204,7 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec, u8 command,
> > }
> >
> > for (; rdata_len > 0; --rdata_len) {
> > - result = acpi_ec_wait(ec, ACPI_EC_EVENT_OBF_1, count);
> > + result = acpi_ec_wait(ec, ACPI_EC_EVENT_OBF_1, count, force_poll);
> > if (result) {
> > printk(KERN_ERR PREFIX "read timeout, command = %d\n",
> > command);
> > @@ -217,7 +219,8 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec, u8 command,
> >
> > static int acpi_ec_transaction(struct acpi_ec *ec, u8 command,
> > const u8 * wdata, unsigned wdata_len,
> > - u8 * rdata, unsigned rdata_len)
> > + u8 * rdata, unsigned rdata_len,
> > + int force_poll)
> > {
> > int status;
> > u32 glk;
> > @@ -240,7 +243,7 @@ static int acpi_ec_transaction(struct acpi_ec *ec, u8 command,
> > /* Make sure GPE is enabled before doing transaction */
> > acpi_enable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
> >
> > - status = acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0, 0);
> > + status = acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0, 0, 0);
> > if (status) {
> > printk(KERN_DEBUG PREFIX
> > "input buffer is not empty, aborting transaction\n");
> > @@ -249,7 +252,8 @@ static int acpi_ec_transaction(struct acpi_ec *ec, u8 command,
> >
> > status = acpi_ec_transaction_unlocked(ec, command,
> > wdata, wdata_len,
> > - rdata, rdata_len);
> > + rdata, rdata_len,
> > + force_poll);
> >
> > end:
> >
> > @@ -267,12 +271,12 @@ static int acpi_ec_transaction(struct acpi_ec *ec, u8 command,
> > int acpi_ec_burst_enable(struct acpi_ec *ec)
> > {
> > u8 d;
> > - return acpi_ec_transaction(ec, ACPI_EC_BURST_ENABLE, NULL, 0, &d, 1);
> > + return acpi_ec_transaction(ec, ACPI_EC_BURST_ENABLE, NULL, 0, &d, 1, 0);
> > }
> >
> > int acpi_ec_burst_disable(struct acpi_ec *ec)
> > {
> > - return acpi_ec_transaction(ec, ACPI_EC_BURST_DISABLE, NULL, 0, NULL, 0);
> > + return acpi_ec_transaction(ec, ACPI_EC_BURST_DISABLE, NULL, 0, NULL, 0, 0);
> > }
> >
> > static int acpi_ec_read(struct acpi_ec *ec, u8 address, u8 * data)
> > @@ -281,7 +285,7 @@ static int acpi_ec_read(struct acpi_ec *ec, u8 address, u8 * data)
> > u8 d;
> >
> > result = acpi_ec_transaction(ec, ACPI_EC_COMMAND_READ,
> > - &address, 1, &d, 1);
> > + &address, 1, &d, 1, 0);
> > *data = d;
> > return result;
> > }
> > @@ -290,7 +294,7 @@ static int acpi_ec_write(struct acpi_ec *ec, u8 address, u8 data)
> > {
> > u8 wdata[2] = { address, data };
> > return acpi_ec_transaction(ec, ACPI_EC_COMMAND_WRITE,
> > - wdata, 2, NULL, 0);
> > + wdata, 2, NULL, 0, 0);
> > }
> >
> > /*
> > @@ -349,13 +353,15 @@ EXPORT_SYMBOL(ec_write);
> >
> > int ec_transaction(u8 command,
> > const u8 * wdata, unsigned wdata_len,
> > - u8 * rdata, unsigned rdata_len)
> > + u8 * rdata, unsigned rdata_len,
> > + int force_poll)
> > {
> > if (!first_ec)
> > return -ENODEV;
> >
> > return acpi_ec_transaction(first_ec, command, wdata,
> > - wdata_len, rdata, rdata_len);
> > + wdata_len, rdata, rdata_len,
> > + force_poll);
> > }
> >
> > EXPORT_SYMBOL(ec_transaction);
> > @@ -374,7 +380,7 @@ static int acpi_ec_query(struct acpi_ec *ec, u8 * data)
> > * bit to be cleared (and thus clearing the interrupt source).
> > */
> >
> > - result = acpi_ec_transaction(ec, ACPI_EC_COMMAND_QUERY, NULL, 0, &d, 1);
> > + result = acpi_ec_transaction(ec, ACPI_EC_COMMAND_QUERY, NULL, 0, &d, 1, 0);
> > if (result)
> > return result;
> >
> > @@ -410,6 +416,7 @@ static u32 acpi_ec_gpe_handler(void *data)
> > acpi_status status = AE_OK;
> > u8 value;
> > struct acpi_ec *ec = data;
> > +
> > atomic_inc(&ec->event_count);
> >
> > if (acpi_ec_mode == EC_INTR) {
> > diff --git a/drivers/misc/msi-laptop.c b/drivers/misc/msi-laptop.c
> > index 68c4b58..41e901f 100644
> > --- a/drivers/misc/msi-laptop.c
> > +++ b/drivers/misc/msi-laptop.c
> > @@ -85,7 +85,7 @@ static int set_lcd_level(int level)
> > buf[0] = 0x80;
> > buf[1] = (u8) (level*31);
> >
> > - return ec_transaction(MSI_EC_COMMAND_LCD_LEVEL, buf, sizeof(buf), NULL, 0);
> > + return ec_transaction(MSI_EC_COMMAND_LCD_LEVEL, buf, sizeof(buf), NULL, 0, 1);
> > }
> >
> > static int get_lcd_level(void)
> > @@ -93,7 +93,7 @@ static int get_lcd_level(void)
> > u8 wdata = 0, rdata;
> > int result;
> >
> > - result = ec_transaction(MSI_EC_COMMAND_LCD_LEVEL, &wdata, 1, &rdata, 1);
> > + result = ec_transaction(MSI_EC_COMMAND_LCD_LEVEL, &wdata, 1, &rdata, 1, 1);
> > if (result < 0)
> > return result;
> >
> > @@ -105,7 +105,7 @@ static int get_auto_brightness(void)
> > u8 wdata = 4, rdata;
> > int result;
> >
> > - result = ec_transaction(MSI_EC_COMMAND_LCD_LEVEL, &wdata, 1, &rdata, 1);
> > + result = ec_transaction(MSI_EC_COMMAND_LCD_LEVEL, &wdata, 1, &rdata, 1, 1);
> > if (result < 0)
> > return result;
> >
> > @@ -119,14 +119,14 @@ static int set_auto_brightness(int enable)
> >
> > wdata[0] = 4;
> >
> > - result = ec_transaction(MSI_EC_COMMAND_LCD_LEVEL, wdata, 1, &rdata, 1);
> > + result = ec_transaction(MSI_EC_COMMAND_LCD_LEVEL, wdata, 1, &rdata, 1, 1);
> > if (result < 0)
> > return result;
> >
> > wdata[0] = 0x84;
> > wdata[1] = (rdata & 0xF7) | (enable ? 8 : 0);
> >
> > - return ec_transaction(MSI_EC_COMMAND_LCD_LEVEL, wdata, 2, NULL, 0);
> > + return ec_transaction(MSI_EC_COMMAND_LCD_LEVEL, wdata, 2, NULL, 0, 1);
> > }
> >
> > static int get_wireless_state(int *wlan, int *bluetooth)
> > @@ -134,7 +134,7 @@ static int get_wireless_state(int *wlan, int *bluetooth)
> > u8 wdata = 0, rdata;
> > int result;
> >
> > - result = ec_transaction(MSI_EC_COMMAND_WIRELESS, &wdata, 1, &rdata, 1);
> > + result = ec_transaction(MSI_EC_COMMAND_WIRELESS, &wdata, 1, &rdata, 1, 1);
> > if (result < 0)
> > return -1;
> >
> > diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> > index 8bcfaa4..fccd8b5 100644
> > --- a/include/linux/acpi.h
> > +++ b/include/linux/acpi.h
> > @@ -182,7 +182,8 @@ 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);
> > + u8 *rdata, unsigned rdata_len,
> > + int force_poll);
> >
> > #endif /*CONFIG_ACPI_EC*/
> > -
> > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Satyam Sharma: "Re: [PATCH 2/3] Add hard_irq_disable()"
Previous message: Russell King: "Re: [GIT PULL] MMC updates"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]