| Message ID | 20180227211539.5708-2-kernel@kempniu.pl (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Darren Hart |
| Headers | show |
On Tue, Feb 27, 2018 at 11:15 PM, Michał Kępień <kernel@kempniu.pl> wrote: > Various functions exposed by the firmware through the FUNC interface > tend to use a consistent set of integers for denoting the type of > operation to be performed for a specified feature. Use named constants > instead of integers in each call_fext_func() invocation in order to more > clearly convey the intent of each call. > > Note that FUNC_FLAGS is a bit peculiar: > +/* FUNC interface - operations */ > +#define OP_GET BIT(1) > +#define OP_GET_CAPS 0 > +#define OP_GET_EVENTS BIT(0) > +#define OP_GET_EXT BIT(2) > +#define OP_SET BIT(0) > +#define OP_SET_EXT (BIT(2) | BIT(0)) Hmm... this looks unordered a bit. And plain 0 doesn't look right in this concept (something like (0 << 0) would probably do it).
On Wed, Feb 28, 2018 at 06:08:52PM +0200, Andy Shevchenko wrote: > On Tue, Feb 27, 2018 at 11:15 PM, Micha?? K??pie?? <kernel@kempniu.pl> wrote: > > Various functions exposed by the firmware through the FUNC interface > > tend to use a consistent set of integers for denoting the type of > > operation to be performed for a specified feature. Use named constants > > instead of integers in each call_fext_func() invocation in order to more > > clearly convey the intent of each call. > > > > Note that FUNC_FLAGS is a bit peculiar: > > > +/* FUNC interface - operations */ > > +#define OP_GET BIT(1) > > +#define OP_GET_CAPS 0 > > +#define OP_GET_EVENTS BIT(0) > > +#define OP_GET_EXT BIT(2) > > +#define OP_SET BIT(0) > > +#define OP_SET_EXT (BIT(2) | BIT(0)) > > Hmm... this looks unordered a bit. It seems to be ordered alphabetically on the identifier. Andy, is it preferred to order defines like this based on resolved numeric order? There is a lack of apparent consistency in the numeric mapping; for example, OP_SET_EXT includes the OP_SET bit, but OP_GET_EXT does not include the OP_GET bit. However, after inspecting the code I think this is simply reflecting what the hardware expects. > And plain 0 doesn't look right in this concept (something like (0 << > 0) would probably do it). Given that all other definitions are in terms of BIT(), to my eye "(0 << 0)" looks as much out of place as plain "0". However, if the convention in this case would be to use the former then I have no objections. I presume the "(0 << 0)" idea comes from the fact that BIT() ultimately expands to some form of shift. Regards jonathan
> On Wed, Feb 28, 2018 at 06:08:52PM +0200, Andy Shevchenko wrote: > > On Tue, Feb 27, 2018 at 11:15 PM, Micha?? K??pie?? <kernel@kempniu.pl> wrote: > > > Various functions exposed by the firmware through the FUNC interface > > > tend to use a consistent set of integers for denoting the type of > > > operation to be performed for a specified feature. Use named constants > > > instead of integers in each call_fext_func() invocation in order to more > > > clearly convey the intent of each call. > > > > > > Note that FUNC_FLAGS is a bit peculiar: > > > > > +/* FUNC interface - operations */ > > > +#define OP_GET BIT(1) > > > +#define OP_GET_CAPS 0 > > > +#define OP_GET_EVENTS BIT(0) > > > +#define OP_GET_EXT BIT(2) > > > +#define OP_SET BIT(0) > > > +#define OP_SET_EXT (BIT(2) | BIT(0)) > > > > Hmm... this looks unordered a bit. > > It seems to be ordered alphabetically on the identifier. Andy, is it > preferred to order defines like this based on resolved numeric order? Just to expand on what Jonathan wrote above: if you take a peek at the end result of the patch series, you will notice a pattern: constants in each section are ordered alphabetically by their name. I wanted all sections to be consistently ordered. If you would rather have me order things by the bit index, sure, no problem, just please note that the order above is not accidental. > There is a lack of apparent consistency in the numeric mapping; for example, > OP_SET_EXT includes the OP_SET bit, but OP_GET_EXT does not include the > OP_GET bit. However, after inspecting the code I think this is simply > reflecting what the hardware expects. Exactly, I could not find any rule which would explain the way the integers hardcoded into the various firmware functions could be mapped to their purpose. The whole point of this series is just to give someone looking at the module code a quick hint about the purpose of each call; with plain integers used instead of constants, these calls look a bit too arbitrary for my taste. > > And plain 0 doesn't look right in this concept (something like (0 << > > 0) would probably do it). > > Given that all other definitions are in terms of BIT(), to my eye "(0 << 0)" > looks as much out of place as plain "0". However, if the convention in this > case would be to use the former then I have no objections. I presume the > "(0 << 0)" idea comes from the fact that BIT() ultimately expands to some > form of shift. Yes, I would guess so. The syntax suggested by Andy looked odd and superfluous to me at first, but grepping the tree for this construct seems to suggest that it is a pretty common thing. So no problem, I will tweak this in v2. I understand I should apply the same concept in these cases: +/* Constants related to FUNC_BACKLIGHT */ +#define FEAT_BACKLIGHT_POWER BIT(2) +#define STATE_BACKLIGHT_OFF (BIT(0) | BIT(1)) +#define STATE_BACKLIGHT_ON 0 +#define FEAT_RADIO_LED BIT(5) +#define STATE_RADIO_LED_OFF 0 +#define STATE_RADIO_LED_ON BIT(5) Right?
On Sun, Mar 04, 2018 at 08:44:26PM +0100, Micha?? K??pie?? wrote: > > On Wed, Feb 28, 2018 at 06:08:52PM +0200, Andy Shevchenko wrote: > > > And plain 0 doesn't look right in this concept (something like (0 << > > > 0) would probably do it). > > > > Given that all other definitions are in terms of BIT(), to my eye "(0 << 0)" > > looks as much out of place as plain "0". However, if the convention in this > > case would be to use the former then I have no objections. I presume the > > "(0 << 0)" idea comes from the fact that BIT() ultimately expands to some > > form of shift. > > Yes, I would guess so. The syntax suggested by Andy looked odd and > superfluous to me at first, but grepping the tree for this construct > seems to suggest that it is a pretty common thing. So no problem, I > will tweak this in v2. I understand I should apply the same concept in > these cases: > > +/* Constants related to FUNC_BACKLIGHT */ > +#define FEAT_BACKLIGHT_POWER BIT(2) > +#define STATE_BACKLIGHT_OFF (BIT(0) | BIT(1)) > +#define STATE_BACKLIGHT_ON 0 > > +#define FEAT_RADIO_LED BIT(5) > +#define STATE_RADIO_LED_OFF 0 > +#define STATE_RADIO_LED_ON BIT(5) > > Right? I suspect so. Regards jonathan
On Sun, Mar 04, 2018 at 08:44:26PM +0100, Michał Kępień wrote: > > On Wed, Feb 28, 2018 at 06:08:52PM +0200, Andy Shevchenko wrote: > > > On Tue, Feb 27, 2018 at 11:15 PM, Micha?? K??pie?? <kernel@kempniu.pl> wrote: > > > > Various functions exposed by the firmware through the FUNC interface > > > > tend to use a consistent set of integers for denoting the type of > > > > operation to be performed for a specified feature. Use named constants > > > > instead of integers in each call_fext_func() invocation in order to more > > > > clearly convey the intent of each call. > > > > > > > > Note that FUNC_FLAGS is a bit peculiar: > > > > > > > +/* FUNC interface - operations */ > > > > +#define OP_GET BIT(1) > > > > +#define OP_GET_CAPS 0 > > > > +#define OP_GET_EVENTS BIT(0) > > > > +#define OP_GET_EXT BIT(2) > > > > +#define OP_SET BIT(0) > > > > +#define OP_SET_EXT (BIT(2) | BIT(0)) > > > > > > Hmm... this looks unordered a bit. > > > > It seems to be ordered alphabetically on the identifier. Andy, is it > > preferred to order defines like this based on resolved numeric order? > > Just to expand on what Jonathan wrote above: if you take a peek at the > end result of the patch series, you will notice a pattern: constants in > each section are ordered alphabetically by their name. I wanted all > sections to be consistently ordered. If you would rather have me order > things by the bit index, sure, no problem, just please note that the > order above is not accidental. Hrm. In my experience it is more typical to order by value (bit), that's a little less obvious when using the BIT()|BIT() macros though. So long as it's consistent, I think that's what matters most.
On Tue, Mar 6, 2018 at 1:16 AM, Darren Hart <dvhart@infradead.org> wrote: > On Sun, Mar 04, 2018 at 08:44:26PM +0100, Michał Kępień wrote: >> > On Wed, Feb 28, 2018 at 06:08:52PM +0200, Andy Shevchenko wrote: >> > > On Tue, Feb 27, 2018 at 11:15 PM, Micha?? K??pie?? <kernel@kempniu.pl> wrote: >> > > > Various functions exposed by the firmware through the FUNC interface >> > > > tend to use a consistent set of integers for denoting the type of >> > > > operation to be performed for a specified feature. Use named constants >> > > > instead of integers in each call_fext_func() invocation in order to more >> > > > clearly convey the intent of each call. >> > > > >> > > > Note that FUNC_FLAGS is a bit peculiar: >> > > >> > > > +/* FUNC interface - operations */ >> > > > +#define OP_GET BIT(1) >> > > > +#define OP_GET_CAPS 0 >> > > > +#define OP_GET_EVENTS BIT(0) >> > > > +#define OP_GET_EXT BIT(2) >> > > > +#define OP_SET BIT(0) >> > > > +#define OP_SET_EXT (BIT(2) | BIT(0)) >> > > >> > > Hmm... this looks unordered a bit. >> > >> > It seems to be ordered alphabetically on the identifier. Andy, is it >> > preferred to order defines like this based on resolved numeric order? >> >> Just to expand on what Jonathan wrote above: if you take a peek at the >> end result of the patch series, you will notice a pattern: constants in >> each section are ordered alphabetically by their name. I wanted all >> sections to be consistently ordered. If you would rather have me order >> things by the bit index, sure, no problem, just please note that the >> order above is not accidental. > > Hrm. In my experience it is more typical to order by value (bit), that's a > little less obvious when using the BIT()|BIT() macros though. So long as it's > consistent, I think that's what matters most. What I'm trying to tell is about consistency of style. So, imagine if we have two bitfields in some register, one with one bit and the other with two. And for latter one only 3 states are possible (00, 10, 11), so, REG1_FLDA BIT(0) REG1_FLDB_STATE0 0 REG1_FLDB_STATE2 BIT(2) REG1_FLDB_STATE3 BIT(2) | BIT(3) // or 0x6, or (3 << 1) Above is not what I would like to see. The consistent may be one of the following REG1_FLDA BIT(0) REG1_FLDB_STATE0 (0 << 1) REG1_FLDB_STATE2 (2 << 1) REG1_FLDB_STATE3 (3 << 1) or (implying that in the code _SHIFT is used) REG1_FLDA BIT(0) REG1_FLDB_SHIFT 1 REG1_FLDB_STATE0 0 REG1_FLDB_STATE2 2 REG1_FLDB_STATE3 3 or (perhaps less wanted) REG1_FLDA (1 << 0) REG1_FLDB_STATE0 (0 << 1) REG1_FLDB_STATE2 (2 << 1) REG1_FLDB_STATE3 (3 << 1)
Andy, > What I'm trying to tell is about consistency of style. I completely agree with all you wrote, those are all good suggestions. But you started your reasoning with: > So, imagine if we have two bitfields in some register, one with one > bit and the other with two. We are not looking at a hardware register here. Rather, I am trying to bring at least _some_ order to an arbitrary set of values that the vendor assumed would be fun to scatter around a dozen of firmware functions. Some hardware features are controlled by setting a specific bit in the value being passed to a function; in other cases entire integers are taken into account; in yet another case *two* bits in a value control state. There is no reason or order to be found here. In the case of OP_* constants, perhaps the simplest thing to do would be to define them as integers, not bitfields. But that still results in a mess: #define OP_GET 0x2 #define OP_GET_CAPS 0x0 #define OP_GET_EVENTS 0x1 #define OP_GET_EXT 0x4 #define OP_SET 0x1 #define OP_SET_EXT 0x5 or: #define OP_GET_CAPS 0x0 #define OP_GET_EVENTS 0x1 #define OP_SET 0x1 #define OP_GET 0x2 #define OP_GET_EXT 0x4 #define OP_SET_EXT 0x5 Even worse, what am I supposed to do with crap like radio LED control, where 0x20 (bit 5) is passed in one argument to select the desired feature and 0x20 or 0 is passed as another argument to select the desired state of that feature? How do I name and define constants that I can subsequently use in call_fext_func() invocations to spare the reader the need to learn the hard way that this: return call_fext_func(device, FUNC_FLAGS, 0x5, RADIO_LED_ON, 0x0); is actually supposed to turn the radio LED *off*? This is the best I could come up with: #define FEAT_RADIO_LED BIT(5) #define STATE_RADIO_LED_OFF (0 << 0) #define STATE_RADIO_LED_ON BIT(5) Yes, it is ugly. But the resulting call is (IMHO) a lot more clear than the original one: return fext_flags(device, OP_SET_EXT, FEAT_RADIO_LED, STATE_RADIO_LED_OFF); All of the above is why I was inclined to just use alphabetic ordering for all constants defined in fujitsu-laptop. This approach brings at least _some_ consistency to this mess (which only the vendor is to blame for, of course). If you would rather have me take a different approach, I am all ears.
On Tue, Mar 6, 2018 at 10:59 PM, Michał Kępień <kernel@kempniu.pl> wrote: > #define OP_GET_CAPS 0x0 > #define OP_GET_EVENTS 0x1 > #define OP_SET 0x1 > #define OP_GET 0x2 > #define OP_GET_EXT 0x4 > #define OP_SET_EXT 0x5 This one looks pretty much okay (logical pairs IIUC).
> > #define OP_GET_CAPS 0x0 > > #define OP_GET_EVENTS 0x1 > > #define OP_SET 0x1 > > #define OP_GET 0x2 > > #define OP_GET_EXT 0x4 > > #define OP_SET_EXT 0x5 > > This one looks pretty much okay (logical pairs IIUC). Sadly, no, these are not logical pairs. But maybe this is a reasonable compromise anyway: - OP_GET_CAPS seems to be consistent between different functions; it is an operation which returns a bitfield describing given model's "capabilities" in a certain area (LEDs, buttons, etc.), - some functions expose only OP_GET_CAPS, OP_SET, and OP_GET, - some functions expose only OP_GET_CAPS and OP_GET_EVENTS, - some function expose OP_GET_CAPS, OP_GET_EVENTS, OP_GET_EXT and OP_SET_EXT (but not OP_SET or OP_GET, probably because 0x1 is already "taken" by OP_GET_EVENTS). So, given the above, does this layout look reasonable to you (at least somewhat) or would you rather see these constants shuffled around in some other way?
On Sat, Mar 10, 2018 at 10:10 PM, Michał Kępień <kernel@kempniu.pl> wrote: >> > #define OP_GET_CAPS 0x0 >> > #define OP_GET_EVENTS 0x1 >> > #define OP_SET 0x1 >> > #define OP_GET 0x2 >> > #define OP_GET_EXT 0x4 >> > #define OP_SET_EXT 0x5 >> >> This one looks pretty much okay (logical pairs IIUC). > > Sadly, no, these are not logical pairs. But maybe this is a reasonable > compromise anyway: > > - OP_GET_CAPS seems to be consistent between different functions; it > is an operation which returns a bitfield describing given model's > "capabilities" in a certain area (LEDs, buttons, etc.), > > - some functions expose only OP_GET_CAPS, OP_SET, and OP_GET, > > - some functions expose only OP_GET_CAPS and OP_GET_EVENTS, > > - some function expose OP_GET_CAPS, OP_GET_EVENTS, OP_GET_EXT and > OP_SET_EXT (but not OP_SET or OP_GET, probably because 0x1 is > already "taken" by OP_GET_EVENTS). Interesting. Does it mean there is no logic between functions on the same platform and what they are expose? May be those sets might be groupped by what kind of functions expose them? > So, given the above, does this layout look reasonable to you (at least > somewhat) or would you rather see these constants shuffled around in > some other way? If no other way is feasible right now, the above is okay to me.
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c index 13bcdfea5349..74775caeb609 100644 --- a/drivers/platform/x86/fujitsu-laptop.c +++ b/drivers/platform/x86/fujitsu-laptop.c @@ -87,6 +87,14 @@ /* FUNC interface - responses */ #define UNSUPPORTED_CMD BIT(31) +/* FUNC interface - operations */ +#define OP_GET BIT(1) +#define OP_GET_CAPS 0 +#define OP_GET_EVENTS BIT(0) +#define OP_GET_EXT BIT(2) +#define OP_SET BIT(0) +#define OP_SET_EXT (BIT(2) | BIT(0)) + /* FUNC interface - status flags */ #define FLAG_RFKILL BIT(5) #define FLAG_LID BIT(8) @@ -264,10 +272,10 @@ static int bl_update_status(struct backlight_device *b) if (fext) { if (b->props.power == FB_BLANK_POWERDOWN) - call_fext_func(fext, FUNC_BACKLIGHT, 0x1, + call_fext_func(fext, FUNC_BACKLIGHT, OP_SET, BACKLIGHT_PARAM_POWER, BACKLIGHT_OFF); else - call_fext_func(fext, FUNC_BACKLIGHT, 0x1, + call_fext_func(fext, FUNC_BACKLIGHT, OP_SET, BACKLIGHT_PARAM_POWER, BACKLIGHT_ON); } @@ -597,11 +605,13 @@ static int logolamp_set(struct led_classdev *cdev, if (brightness < LED_FULL) always = FUNC_LED_OFF; - ret = call_fext_func(device, FUNC_LEDS, 0x1, LOGOLAMP_POWERON, poweron); + ret = call_fext_func(device, FUNC_LEDS, OP_SET, + LOGOLAMP_POWERON, poweron); if (ret < 0) return ret; - return call_fext_func(device, FUNC_LEDS, 0x1, LOGOLAMP_ALWAYS, always); + return call_fext_func(device, FUNC_LEDS, OP_SET, + LOGOLAMP_ALWAYS, always); } static enum led_brightness logolamp_get(struct led_classdev *cdev) @@ -609,11 +619,11 @@ static enum led_brightness logolamp_get(struct led_classdev *cdev) struct acpi_device *device = to_acpi_device(cdev->dev->parent); int ret; - ret = call_fext_func(device, FUNC_LEDS, 0x2, LOGOLAMP_ALWAYS, 0x0); + ret = call_fext_func(device, FUNC_LEDS, OP_GET, LOGOLAMP_ALWAYS, 0x0); if (ret == FUNC_LED_ON) return LED_FULL; - ret = call_fext_func(device, FUNC_LEDS, 0x2, LOGOLAMP_POWERON, 0x0); + ret = call_fext_func(device, FUNC_LEDS, OP_GET, LOGOLAMP_POWERON, 0x0); if (ret == FUNC_LED_ON) return LED_HALF; @@ -626,11 +636,11 @@ static int kblamps_set(struct led_classdev *cdev, struct acpi_device *device = to_acpi_device(cdev->dev->parent); if (brightness >= LED_FULL) - return call_fext_func(device, FUNC_LEDS, 0x1, KEYBOARD_LAMPS, - FUNC_LED_ON); + return call_fext_func(device, FUNC_LEDS, OP_SET, + KEYBOARD_LAMPS, FUNC_LED_ON); else - return call_fext_func(device, FUNC_LEDS, 0x1, KEYBOARD_LAMPS, - FUNC_LED_OFF); + return call_fext_func(device, FUNC_LEDS, OP_SET, + KEYBOARD_LAMPS, FUNC_LED_OFF); } static enum led_brightness kblamps_get(struct led_classdev *cdev) @@ -638,8 +648,8 @@ static enum led_brightness kblamps_get(struct led_classdev *cdev) struct acpi_device *device = to_acpi_device(cdev->dev->parent); enum led_brightness brightness = LED_OFF; - if (call_fext_func(device, - FUNC_LEDS, 0x2, KEYBOARD_LAMPS, 0x0) == FUNC_LED_ON) + if (call_fext_func(device, FUNC_LEDS, OP_GET, + KEYBOARD_LAMPS, 0x0) == FUNC_LED_ON) brightness = LED_FULL; return brightness; @@ -651,11 +661,11 @@ static int radio_led_set(struct led_classdev *cdev, struct acpi_device *device = to_acpi_device(cdev->dev->parent); if (brightness >= LED_FULL) - return call_fext_func(device, FUNC_FLAGS, 0x5, RADIO_LED_ON, - RADIO_LED_ON); + return call_fext_func(device, FUNC_FLAGS, OP_SET_EXT, + RADIO_LED_ON, RADIO_LED_ON); else - return call_fext_func(device, FUNC_FLAGS, 0x5, RADIO_LED_ON, - 0x0); + return call_fext_func(device, FUNC_FLAGS, OP_SET_EXT, + RADIO_LED_ON, 0x0); } static enum led_brightness radio_led_get(struct led_classdev *cdev) @@ -663,7 +673,8 @@ static enum led_brightness radio_led_get(struct led_classdev *cdev) struct acpi_device *device = to_acpi_device(cdev->dev->parent); enum led_brightness brightness = LED_OFF; - if (call_fext_func(device, FUNC_FLAGS, 0x4, 0x0, 0x0) & RADIO_LED_ON) + if (call_fext_func(device, FUNC_FLAGS, OP_GET_EXT, + 0x0, 0x0) & RADIO_LED_ON) brightness = LED_FULL; return brightness; @@ -675,13 +686,13 @@ static int eco_led_set(struct led_classdev *cdev, struct acpi_device *device = to_acpi_device(cdev->dev->parent); int curr; - curr = call_fext_func(device, FUNC_LEDS, 0x2, ECO_LED, 0x0); + curr = call_fext_func(device, FUNC_LEDS, OP_GET, ECO_LED, 0x0); if (brightness >= LED_FULL) - return call_fext_func(device, FUNC_LEDS, 0x1, ECO_LED, - curr | ECO_LED_ON); + return call_fext_func(device, FUNC_LEDS, OP_SET, + ECO_LED, curr | ECO_LED_ON); else - return call_fext_func(device, FUNC_LEDS, 0x1, ECO_LED, - curr & ~ECO_LED_ON); + return call_fext_func(device, FUNC_LEDS, OP_SET, + ECO_LED, curr & ~ECO_LED_ON); } static enum led_brightness eco_led_get(struct led_classdev *cdev) @@ -689,7 +700,8 @@ static enum led_brightness eco_led_get(struct led_classdev *cdev) struct acpi_device *device = to_acpi_device(cdev->dev->parent); enum led_brightness brightness = LED_OFF; - if (call_fext_func(device, FUNC_LEDS, 0x2, ECO_LED, 0x0) & ECO_LED_ON) + if (call_fext_func(device, FUNC_LEDS, OP_GET, + ECO_LED, 0x0) & ECO_LED_ON) brightness = LED_FULL; return brightness; @@ -701,8 +713,8 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device) struct led_classdev *led; int ret; - if (call_fext_func(device, - FUNC_LEDS, 0x0, 0x0, 0x0) & LOGOLAMP_POWERON) { + if (call_fext_func(device, FUNC_LEDS, OP_GET_CAPS, + 0x0, 0x0) & LOGOLAMP_POWERON) { led = devm_kzalloc(&device->dev, sizeof(*led), GFP_KERNEL); if (!led) return -ENOMEM; @@ -715,9 +727,10 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device) return ret; } - if ((call_fext_func(device, - FUNC_LEDS, 0x0, 0x0, 0x0) & KEYBOARD_LAMPS) && - (call_fext_func(device, FUNC_BUTTONS, 0x0, 0x0, 0x0) == 0x0)) { + if ((call_fext_func(device, FUNC_LEDS, OP_GET_CAPS, + 0x0, 0x0) & KEYBOARD_LAMPS) && + (call_fext_func(device, FUNC_BUTTONS, OP_GET_CAPS, + 0x0, 0x0) == 0x0)) { led = devm_kzalloc(&device->dev, sizeof(*led), GFP_KERNEL); if (!led) return -ENOMEM; @@ -758,9 +771,10 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device) * bit 14 seems to indicate presence of said led as well. * Confirm by testing the status. */ - if ((call_fext_func(device, FUNC_LEDS, 0x0, 0x0, 0x0) & BIT(14)) && - (call_fext_func(device, - FUNC_LEDS, 0x2, ECO_LED, 0x0) != UNSUPPORTED_CMD)) { + if ((call_fext_func(device, FUNC_LEDS, OP_GET_CAPS, + 0x0, 0x0) & BIT(14)) && + (call_fext_func(device, FUNC_LEDS, OP_GET, + ECO_LED, 0x0) != UNSUPPORTED_CMD)) { led = devm_kzalloc(&device->dev, sizeof(*led), GFP_KERNEL); if (!led) return -ENOMEM; @@ -802,14 +816,15 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device) pr_info("ACPI: %s [%s]\n", acpi_device_name(device), acpi_device_bid(device)); - while (call_fext_func(device, FUNC_BUTTONS, 0x1, 0x0, 0x0) != 0 && + while (call_fext_func(device, FUNC_BUTTONS, OP_GET_EVENTS, + 0x0, 0x0) != 0 && i++ < MAX_HOTKEY_RINGBUFFER_SIZE) ; /* No action, result is discarded */ acpi_handle_debug(device->handle, "Discarded %i ringbuffer entries\n", i); - priv->flags_supported = call_fext_func(device, FUNC_FLAGS, 0x0, 0x0, - 0x0); + priv->flags_supported = call_fext_func(device, FUNC_FLAGS, OP_GET_CAPS, + 0x0, 0x0); /* Make sure our bitmask of supported functions is cleared if the RFKILL function block is not implemented, like on the S7020. */ @@ -817,17 +832,18 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device) priv->flags_supported = 0; if (priv->flags_supported) - priv->flags_state = call_fext_func(device, FUNC_FLAGS, 0x4, 0x0, - 0x0); + priv->flags_state = call_fext_func(device, FUNC_FLAGS, + OP_GET_EXT, 0x0, 0x0); /* Suspect this is a keymap of the application panel, print it */ acpi_handle_info(device->handle, "BTNI: [0x%x]\n", - call_fext_func(device, FUNC_BUTTONS, 0x0, 0x0, 0x0)); + call_fext_func(device, FUNC_BUTTONS, OP_GET_CAPS, + 0x0, 0x0)); /* Sync backlight power status */ if (fujitsu_bl && fujitsu_bl->bl_device && acpi_video_get_backlight_type() == acpi_backlight_vendor) { - if (call_fext_func(fext, FUNC_BACKLIGHT, 0x2, + if (call_fext_func(fext, FUNC_BACKLIGHT, OP_GET, BACKLIGHT_PARAM_POWER, 0x0) == BACKLIGHT_OFF) fujitsu_bl->bl_device->props.power = FB_BLANK_POWERDOWN; else @@ -912,11 +928,11 @@ static void acpi_fujitsu_laptop_notify(struct acpi_device *device, u32 event) } if (priv->flags_supported) - priv->flags_state = call_fext_func(device, FUNC_FLAGS, 0x4, 0x0, - 0x0); + priv->flags_state = call_fext_func(device, FUNC_FLAGS, + OP_GET_EXT, 0x0, 0x0); - while ((irb = call_fext_func(device, - FUNC_BUTTONS, 0x1, 0x0, 0x0)) != 0 && + while ((irb = call_fext_func(device, FUNC_BUTTONS, OP_GET_EVENTS, + 0x0, 0x0)) != 0 && i++ < MAX_HOTKEY_RINGBUFFER_SIZE) { scancode = irb & 0x4ff; if (sparse_keymap_entry_from_scancode(priv->input, scancode)) @@ -933,7 +949,8 @@ static void acpi_fujitsu_laptop_notify(struct acpi_device *device, u32 event) * handled in software; its state is queried using FUNC_FLAGS */ if ((priv->flags_supported & BIT(26)) && - (call_fext_func(device, FUNC_FLAGS, 0x1, 0x0, 0x0) & BIT(26))) + (call_fext_func(device, FUNC_FLAGS, OP_GET_EVENTS, + 0x0, 0x0) & BIT(26))) sparse_keymap_report_event(priv->input, BIT(26), 1, true); }
Various functions exposed by the firmware through the FUNC interface tend to use a consistent set of integers for denoting the type of operation to be performed for a specified feature. Use named constants instead of integers in each call_fext_func() invocation in order to more clearly convey the intent of each call. Note that FUNC_FLAGS is a bit peculiar: - operations 0x4 (OP_GET_EXT) and 0x5 (OP_SET_EXT) are used for, respectively, getting and setting feature states, instead of 0x2 and 0x1 used by other FUNC interfaces, - operation 0x1 is used for retrieving events (OP_GET_EVENTS). Signed-off-by: Michał Kępień <kernel@kempniu.pl> --- drivers/platform/x86/fujitsu-laptop.c | 103 ++++++++++++++++++++-------------- 1 file changed, 60 insertions(+), 43 deletions(-)