lkml.org 
[lkml]   [2006]   [Aug]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRE: [PATCH 1/2] acpi,backlight: MSI S270 laptop support - ec_transaction()
Date
From

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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2006-08-14 17:29    [from the cache]
©2003-2011 Jasper Spaans