| Message ID | 20170117071714.21594-1-kernel@kempniu.pl (mailing list archive) |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | Andy Shevchenko |
| Headers | show |
On Tue, 2017-01-17 at 08:17 +0100, Michał Kępień wrote: > Fix coding style issues in dell-wmi-led which checkpatch complains about > to make sure the module gets a clean start in the x86 platform driver > subsystem. trivia: > Signed-off-by: Michał Kępień <kernel@kempniu.pl> > --- > This is an extra patch that Jacek asked for [1]. > > [1] https://lkml.org/lkml/2017/1/16/631 > > drivers/platform/x86/dell-wmi-led.c | 41 +++++++++++++++---------------------- > 1 file changed, 16 insertions(+), 25 deletions(-) > > diff --git a/drivers/platform/x86/dell-wmi-led.c b/drivers/platform/x86/dell-wmi-led.c [] > @@ -46,21 +46,16 @@ struct bios_args { > unsigned char off_time; > }; > > -static int dell_led_perform_fn(u8 length, > - u8 result_code, > - u8 device_id, > - u8 command, > - u8 on_time, > - u8 off_time) > +static int dell_led_perform_fn(u8 length, u8 result_code, u8 device_id, > + u8 command, u8 on_time, u8 off_time) > { > - struct bios_args *bios_return; > - u8 return_code; > - union acpi_object *obj; > struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; > + struct bios_args *bios_return, args; > struct acpi_buffer input; > + union acpi_object *obj; > acpi_status status; > + u8 return_code; > > - struct bios_args args; > args.length = length; > args.result_code = result_code; > args.device_id = device_id; This declaration might be nicer using struct bios_args args = { .length = length, .result_code = result_code, .device_id = device_id, [...] }; [] > @@ -138,24 +128,25 @@ static void dell_led_set(struct led_classdev *led_cdev, > } > > static int dell_led_blink(struct led_classdev *led_cdev, > - unsigned long *delay_on, > - unsigned long *delay_off) > + unsigned long *delay_on, unsigned long *delay_off) > { > unsigned long on_eighths; > unsigned long off_eighths; > > - /* The Dell LED delay is based on 125ms intervals. > - Need to round up to next interval. */ > + /* > + * The Dell LED delay is based on 125ms intervals. > + * Need to round up to next interval. > + */ > > on_eighths = (*delay_on + 124) / 125; > - if (0 == on_eighths) > + if (on_eighths == 0) > on_eighths = 1; > if (on_eighths > 255) > on_eighths = 255; > *delay_on = on_eighths * 125; > > off_eighths = (*delay_off + 124) / 125; > - if (0 == off_eighths) > + if (off_eighths == 0) > off_eighths = 1; > if (off_eighths > 255) > off_eighths = 255; These could use DIV_ROUND_UP and clamp()
> On Tue, 2017-01-17 at 08:17 +0100, Michał Kępień wrote: > > Fix coding style issues in dell-wmi-led which checkpatch complains about > > to make sure the module gets a clean start in the x86 platform driver > > subsystem. > > trivia: > > > Signed-off-by: Michał Kępień <kernel@kempniu.pl> > > --- > > This is an extra patch that Jacek asked for [1]. > > > > [1] https://lkml.org/lkml/2017/1/16/631 > > > > drivers/platform/x86/dell-wmi-led.c | 41 +++++++++++++++---------------------- > > 1 file changed, 16 insertions(+), 25 deletions(-) > > > > diff --git a/drivers/platform/x86/dell-wmi-led.c b/drivers/platform/x86/dell-wmi-led.c > [] > > @@ -46,21 +46,16 @@ struct bios_args { > > unsigned char off_time; > > }; > > > > -static int dell_led_perform_fn(u8 length, > > - u8 result_code, > > - u8 device_id, > > - u8 command, > > - u8 on_time, > > - u8 off_time) > > +static int dell_led_perform_fn(u8 length, u8 result_code, u8 device_id, > > + u8 command, u8 on_time, u8 off_time) > > { > > - struct bios_args *bios_return; > > - u8 return_code; > > - union acpi_object *obj; > > struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; > > + struct bios_args *bios_return, args; > > struct acpi_buffer input; > > + union acpi_object *obj; > > acpi_status status; > > + u8 return_code; > > > > - struct bios_args args; > > args.length = length; > > args.result_code = result_code; > > args.device_id = device_id; > > This declaration might be nicer using > > struct bios_args args = { > .length = length, > .result_code = result_code, > .device_id = device_id, > [...] > }; > > [] > > > @@ -138,24 +128,25 @@ static void dell_led_set(struct led_classdev *led_cdev, > > } > > > > static int dell_led_blink(struct led_classdev *led_cdev, > > - unsigned long *delay_on, > > - unsigned long *delay_off) > > + unsigned long *delay_on, unsigned long *delay_off) > > { > > unsigned long on_eighths; > > unsigned long off_eighths; > > > > - /* The Dell LED delay is based on 125ms intervals. > > - Need to round up to next interval. */ > > + /* > > + * The Dell LED delay is based on 125ms intervals. > > + * Need to round up to next interval. > > + */ > > > > on_eighths = (*delay_on + 124) / 125; > > - if (0 == on_eighths) > > + if (on_eighths == 0) > > on_eighths = 1; > > if (on_eighths > 255) > > on_eighths = 255; > > *delay_on = on_eighths * 125; > > > > off_eighths = (*delay_off + 124) / 125; > > - if (0 == off_eighths) > > + if (off_eighths == 0) > > off_eighths = 1; > > if (off_eighths > 255) > > off_eighths = 255; > > These could use DIV_ROUND_UP and clamp() Thanks for taking a look, Joe, I can certainly fix these. Jacek, as resending an updated version of this patch with Joe's suggestions taken into account would be even more confusing than the "PATCH v2 6+/6" subject I already resorted to, I suggest the following: if this series goes to v3, I will include an updated version of this patch in v3, but in case the remaining patches get acked in their current shape by all maintainers, I will send an updated version of this extra patch separately, after the rest of the series gets applied. Does this sound reasonable?
On Tue 2017-01-17 08:17:14, Michał Kępień wrote: > Fix coding style issues in dell-wmi-led which checkpatch complains about > to make sure the module gets a clean start in the x86 platform driver > subsystem. > > Signed-off-by: Michał Kępień <kernel@kempniu.pl> Acked-by: Pavel Machek <pavel@ucw.cz> > --- > This is an extra patch that Jacek asked for [1]. > > [1] https://lkml.org/lkml/2017/1/16/631 > > drivers/platform/x86/dell-wmi-led.c | 41 +++++++++++++++---------------------- > 1 file changed, 16 insertions(+), 25 deletions(-) > > diff --git a/drivers/platform/x86/dell-wmi-led.c b/drivers/platform/x86/dell-wmi-led.c > index d0232c7f1909..8753c4fc36b8 100644 > --- a/drivers/platform/x86/dell-wmi-led.c > +++ b/drivers/platform/x86/dell-wmi-led.c > @@ -46,21 +46,16 @@ struct bios_args { > unsigned char off_time; > }; > > -static int dell_led_perform_fn(u8 length, > - u8 result_code, > - u8 device_id, > - u8 command, > - u8 on_time, > - u8 off_time) > +static int dell_led_perform_fn(u8 length, u8 result_code, u8 device_id, > + u8 command, u8 on_time, u8 off_time) > { > - struct bios_args *bios_return; > - u8 return_code; > - union acpi_object *obj; > struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; > + struct bios_args *bios_return, args; > struct acpi_buffer input; > + union acpi_object *obj; > acpi_status status; > + u8 return_code; > > - struct bios_args args; > args.length = length; > args.result_code = result_code; > args.device_id = device_id; > @@ -71,11 +66,7 @@ static int dell_led_perform_fn(u8 length, > input.length = sizeof(struct bios_args); > input.pointer = &args; > > - status = wmi_evaluate_method(DELL_LED_BIOS_GUID, > - 1, > - 1, > - &input, > - &output); > + status = wmi_evaluate_method(DELL_LED_BIOS_GUID, 1, 1, &input, &output); > > if (ACPI_FAILURE(status)) > return status; > @@ -84,7 +75,7 @@ static int dell_led_perform_fn(u8 length, > > if (!obj) > return -EINVAL; > - else if (obj->type != ACPI_TYPE_BUFFER) { > + if (obj->type != ACPI_TYPE_BUFFER) { > kfree(obj); > return -EINVAL; > } > @@ -117,8 +108,7 @@ static int led_off(void) > 0); /* not used */ > } > > -static int led_blink(unsigned char on_eighths, > - unsigned char off_eighths) > +static int led_blink(unsigned char on_eighths, unsigned char off_eighths) > { > return dell_led_perform_fn(5, /* Length of command */ > INTERFACE_ERROR, /* Init to INTERFACE_ERROR */ > @@ -129,7 +119,7 @@ static int led_blink(unsigned char on_eighths, > } > > static void dell_led_set(struct led_classdev *led_cdev, > - enum led_brightness value) > + enum led_brightness value) > { > if (value == LED_OFF) > led_off(); > @@ -138,24 +128,25 @@ static void dell_led_set(struct led_classdev *led_cdev, > } > > static int dell_led_blink(struct led_classdev *led_cdev, > - unsigned long *delay_on, > - unsigned long *delay_off) > + unsigned long *delay_on, unsigned long *delay_off) > { > unsigned long on_eighths; > unsigned long off_eighths; > > - /* The Dell LED delay is based on 125ms intervals. > - Need to round up to next interval. */ > + /* > + * The Dell LED delay is based on 125ms intervals. > + * Need to round up to next interval. > + */ > > on_eighths = (*delay_on + 124) / 125; > - if (0 == on_eighths) > + if (on_eighths == 0) > on_eighths = 1; > if (on_eighths > 255) > on_eighths = 255; > *delay_on = on_eighths * 125; > > off_eighths = (*delay_off + 124) / 125; > - if (0 == off_eighths) > + if (off_eighths == 0) > off_eighths = 1; > if (off_eighths > 255) > off_eighths = 255; > -- > 2.11.0
Hi Michał, On 01/17/2017 10:19 AM, Michał Kępień wrote: >> On Tue, 2017-01-17 at 08:17 +0100, Michał Kępień wrote: >>> Fix coding style issues in dell-wmi-led which checkpatch complains about >>> to make sure the module gets a clean start in the x86 platform driver >>> subsystem. >> >> trivia: >> >>> Signed-off-by: Michał Kępień <kernel@kempniu.pl> >>> --- >>> This is an extra patch that Jacek asked for [1]. >>> >>> [1] https://lkml.org/lkml/2017/1/16/631 >>> >>> drivers/platform/x86/dell-wmi-led.c | 41 +++++++++++++++---------------------- >>> 1 file changed, 16 insertions(+), 25 deletions(-) >>> >>> diff --git a/drivers/platform/x86/dell-wmi-led.c b/drivers/platform/x86/dell-wmi-led.c >> [] >>> @@ -46,21 +46,16 @@ struct bios_args { >>> unsigned char off_time; >>> }; >>> >>> -static int dell_led_perform_fn(u8 length, >>> - u8 result_code, >>> - u8 device_id, >>> - u8 command, >>> - u8 on_time, >>> - u8 off_time) >>> +static int dell_led_perform_fn(u8 length, u8 result_code, u8 device_id, >>> + u8 command, u8 on_time, u8 off_time) >>> { >>> - struct bios_args *bios_return; >>> - u8 return_code; >>> - union acpi_object *obj; >>> struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; >>> + struct bios_args *bios_return, args; >>> struct acpi_buffer input; >>> + union acpi_object *obj; >>> acpi_status status; >>> + u8 return_code; >>> >>> - struct bios_args args; >>> args.length = length; >>> args.result_code = result_code; >>> args.device_id = device_id; >> >> This declaration might be nicer using >> >> struct bios_args args = { >> .length = length, >> .result_code = result_code, >> .device_id = device_id, >> [...] >> }; >> >> [] >> >>> @@ -138,24 +128,25 @@ static void dell_led_set(struct led_classdev *led_cdev, >>> } >>> >>> static int dell_led_blink(struct led_classdev *led_cdev, >>> - unsigned long *delay_on, >>> - unsigned long *delay_off) >>> + unsigned long *delay_on, unsigned long *delay_off) >>> { >>> unsigned long on_eighths; >>> unsigned long off_eighths; >>> >>> - /* The Dell LED delay is based on 125ms intervals. >>> - Need to round up to next interval. */ >>> + /* >>> + * The Dell LED delay is based on 125ms intervals. >>> + * Need to round up to next interval. >>> + */ >>> >>> on_eighths = (*delay_on + 124) / 125; >>> - if (0 == on_eighths) >>> + if (on_eighths == 0) >>> on_eighths = 1; >>> if (on_eighths > 255) >>> on_eighths = 255; >>> *delay_on = on_eighths * 125; >>> >>> off_eighths = (*delay_off + 124) / 125; >>> - if (0 == off_eighths) >>> + if (off_eighths == 0) >>> off_eighths = 1; >>> if (off_eighths > 255) >>> off_eighths = 255; >> >> These could use DIV_ROUND_UP and clamp() > > Thanks for taking a look, Joe, I can certainly fix these. > > Jacek, as resending an updated version of this patch with Joe's > suggestions taken into account would be even more confusing than the > "PATCH v2 6+/6" subject I already resorted to, I suggest the following: > if this series goes to v3, I will include an updated version of this > patch in v3, but in case the remaining patches get acked in their > current shape by all maintainers, I will send an updated version of this > extra patch separately, after the rest of the series gets applied. Does > this sound reasonable? Sure. I'll merge whole patch set after getting acks from sound and x86 platform drivers maintainers.
On Tue, Jan 17, 2017 at 9:17 AM, Michał Kępień <kernel@kempniu.pl> wrote: > Fix coding style issues in dell-wmi-led which checkpatch complains about > to make sure the module gets a clean start in the x86 platform driver > subsystem. > + status = wmi_evaluate_method(DELL_LED_BIOS_GUID, 1, 1, &input, &output); > You may remove extra line. > if (ACPI_FAILURE(status)) > return status; > - if (0 == on_eighths) > + if (on_eighths == 0) > on_eighths = 1; > if (on_eighths > 255) > on_eighths = 255; > - if (0 == off_eighths) > + if (off_eighths == 0) > off_eighths = 1; > if (off_eighths > 255) > off_eighths = 255; Obviously they both are custom clamp{_t}().
diff --git a/drivers/platform/x86/dell-wmi-led.c b/drivers/platform/x86/dell-wmi-led.c index d0232c7f1909..8753c4fc36b8 100644 --- a/drivers/platform/x86/dell-wmi-led.c +++ b/drivers/platform/x86/dell-wmi-led.c @@ -46,21 +46,16 @@ struct bios_args { unsigned char off_time; }; -static int dell_led_perform_fn(u8 length, - u8 result_code, - u8 device_id, - u8 command, - u8 on_time, - u8 off_time) +static int dell_led_perform_fn(u8 length, u8 result_code, u8 device_id, + u8 command, u8 on_time, u8 off_time) { - struct bios_args *bios_return; - u8 return_code; - union acpi_object *obj; struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; + struct bios_args *bios_return, args; struct acpi_buffer input; + union acpi_object *obj; acpi_status status; + u8 return_code; - struct bios_args args; args.length = length; args.result_code = result_code; args.device_id = device_id; @@ -71,11 +66,7 @@ static int dell_led_perform_fn(u8 length, input.length = sizeof(struct bios_args); input.pointer = &args; - status = wmi_evaluate_method(DELL_LED_BIOS_GUID, - 1, - 1, - &input, - &output); + status = wmi_evaluate_method(DELL_LED_BIOS_GUID, 1, 1, &input, &output); if (ACPI_FAILURE(status)) return status; @@ -84,7 +75,7 @@ static int dell_led_perform_fn(u8 length, if (!obj) return -EINVAL; - else if (obj->type != ACPI_TYPE_BUFFER) { + if (obj->type != ACPI_TYPE_BUFFER) { kfree(obj); return -EINVAL; } @@ -117,8 +108,7 @@ static int led_off(void) 0); /* not used */ } -static int led_blink(unsigned char on_eighths, - unsigned char off_eighths) +static int led_blink(unsigned char on_eighths, unsigned char off_eighths) { return dell_led_perform_fn(5, /* Length of command */ INTERFACE_ERROR, /* Init to INTERFACE_ERROR */ @@ -129,7 +119,7 @@ static int led_blink(unsigned char on_eighths, } static void dell_led_set(struct led_classdev *led_cdev, - enum led_brightness value) + enum led_brightness value) { if (value == LED_OFF) led_off(); @@ -138,24 +128,25 @@ static void dell_led_set(struct led_classdev *led_cdev, } static int dell_led_blink(struct led_classdev *led_cdev, - unsigned long *delay_on, - unsigned long *delay_off) + unsigned long *delay_on, unsigned long *delay_off) { unsigned long on_eighths; unsigned long off_eighths; - /* The Dell LED delay is based on 125ms intervals. - Need to round up to next interval. */ + /* + * The Dell LED delay is based on 125ms intervals. + * Need to round up to next interval. + */ on_eighths = (*delay_on + 124) / 125; - if (0 == on_eighths) + if (on_eighths == 0) on_eighths = 1; if (on_eighths > 255) on_eighths = 255; *delay_on = on_eighths * 125; off_eighths = (*delay_off + 124) / 125; - if (0 == off_eighths) + if (off_eighths == 0) off_eighths = 1; if (off_eighths > 255) off_eighths = 255;
Fix coding style issues in dell-wmi-led which checkpatch complains about to make sure the module gets a clean start in the x86 platform driver subsystem. Signed-off-by: Michał Kępień <kernel@kempniu.pl> --- This is an extra patch that Jacek asked for [1]. [1] https://lkml.org/lkml/2017/1/16/631 drivers/platform/x86/dell-wmi-led.c | 41 +++++++++++++++---------------------- 1 file changed, 16 insertions(+), 25 deletions(-)