lkml.org 
[lkml]   [2012]   [Apr]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 1/2] HID: hid-lg4ff: Add support for G27 LEDs
    From
    >> +	__u8  led_state;
    >> + struct led_classdev *led[5];
    >
    > Why not put this under the CONFIG_LEDS_CLASS condition as well?

    Can do, I don't think these are referenced anywhere else.

    >> + /* register led subsystem - G27 only */
    >> + entry->led_state = 0;
    >> + for (j = 0; j < 5; j++)
    >> + entry->led[j] = NULL;
    >> +
    >
    > The same here.

    OK.

    >> + if (error) {
    >> + hid_err(hid, "failed to register LED %d. Aborting.\n", j);
    >> +err:
    >> + /* Deregister LEDs (if any) but let the driver continue */
    >> + for (j = 0; j < 5; j++) {
    >> + led = entry->led[j];
    >> + entry->led[j] = NULL;
    >> + if (!led)
    >> + continue;
    >> + led_classdev_unregister(led);
    >> + kfree(led);
    >
    > Indentation seems to be wrong here.

    Bugger...

    >
    >> + }
    >> + break; /* Don't create any more LEDs */
    >
    > Putting the whole err: label outside the outer for cycle would make the
    > whole thing much more readable (and would save you the break). Could you
    > please redo this?

    The 'break' is actually not needed as 'j' has reached maximum value and
    the outer for loop would be at completion. I put the 'break' there just to
    highlight this fact.

    Maybe just a comment would be OK, such as 'on error fall though to driver
    completion'.

    The reason I'd like this code here is that it seems traditional to output
    the following 'hid_info' line once the driver is fully active.

    >> hid_info(hid, "Force feedback for Logitech Speed Force Wireless by
    >> Simon Wood <simon@mungewell.org>\n");

    Moving the 'err:' code would have to:
    1). Jump back to hit this hid_info line
    2). Duplicate the hid_info line
    3). Have err code called as a function

    Any major objections to keeping it as is?


    Man, it seems to be taking me ages to get this code right....
    Simon



    \
     
     \ /
      Last update: 2012-04-18 22:37    [W:0.050 / U:30.576 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site