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    [W:0.023 / U:32.368 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site