[PATCH 0/7] fujitsu-laptop: Consistent naming of constants

[PATCH 0/7] fujitsu-laptop: Consistent naming of constants

[Michał Kępień]

This patch series is an attempt to organize all the named constants used
throughout fujitsu-laptop so that their names more clearly convey their
purpose: a set of prefixes is introduced to "map" constant names to
call_fext_func() parameters.

The chosen constant naming patterns are a compromise between readability
and enabling longer conditional expressions to fit into the 80-character
line length limit.  Some changes (like those in patches 4/7 and 5/7) may
be perceived as bikeshedding, so please just think of them as proposals,
not fixes.

Finally, patch 7/7 again [1] proposes a set of helper functions which
seem to be making quite a difference in terms of code readability in
certain places (especially in long conditional expressions).  YMMV,
though, feel free to disagree.

This patch series should be applied on top of for-next as it builds on
the previous patch series I submitted.

[1] https://www.spinics.net/lists/platform-driver-x86/msg11633.html

 drivers/platform/x86/fujitsu-laptop.c | 228 +++++++++++++++++++---------------
 1 file changed, 127 insertions(+), 101 deletions(-)

-- 
2.16.2

[PATCH 1/7] platform/x86: fujitsu-laptop: Define constants for FUNC operations

[Michał Kępień]

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(-)

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);
 }
 
-- 
2.16.2

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: Define constants for FUNC operations

[Andy Shevchenko]

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).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: Define constants for FUNC operations

[Jonathan Woithe]

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

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: Define constants for FUNC operations

[Michał Kępień]

> 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?

-- 
Best regards,
Michał Kępień

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: Define constants for FUNC operations

[Jonathan Woithe]

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

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: Define constants for FUNC operations

[Darren Hart]

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.

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: Define constants for FUNC operations

[Andy Shevchenko]

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)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: Define constants for FUNC operations

[Michał Kępień]

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.

-- 
Best regards,
Michał Kępień

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: Define constants for FUNC operations

[Andy Shevchenko]

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).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: Define constants for FUNC operations

[Michał Kępień]

> > #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?

-- 
Best regards,
Michał Kępień

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: Define constants for FUNC operations

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 2/7] platform/x86: fujitsu-laptop: Define constants for FUNC features

[Michał Kępień]

Various functions exposed by the firmware through the FUNC interface
allow operations to be performed on certain features.  Make sure these
features are referred to by consistently named constants instead of
integers in order to better convey the intent of each call_fext_func()
invocation.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/fujitsu-laptop.c | 53 +++++++++++++++++++----------------
 1 file changed, 29 insertions(+), 24 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 74775caeb609..087b5d1f2f4a 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -103,15 +103,18 @@
 /* FUNC interface - LED control */
 #define FUNC_LED_OFF			BIT(0)
 #define FUNC_LED_ON			(BIT(0) | BIT(16) | BIT(17))
-#define LOGOLAMP_POWERON		BIT(13)
-#define LOGOLAMP_ALWAYS			BIT(14)
-#define KEYBOARD_LAMPS			BIT(8)
+#define FEAT_LOGOLAMP_POWERON		BIT(13)
+#define FEAT_LOGOLAMP_ALWAYS		BIT(14)
+#define FEAT_KEYBOARD_LAMPS		BIT(8)
+
+#define FEAT_RADIO_LED			BIT(5)
 #define RADIO_LED_ON			BIT(5)
-#define ECO_LED				BIT(16)
+
+#define FEAT_ECO_LED			BIT(16)
 #define ECO_LED_ON			BIT(19)
 
 /* FUNC interface - backlight power control */
-#define BACKLIGHT_PARAM_POWER		BIT(2)
+#define FEAT_BACKLIGHT_POWER		BIT(2)
 #define BACKLIGHT_OFF			(BIT(0) | BIT(1))
 #define BACKLIGHT_ON			0
 
@@ -273,10 +276,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, OP_SET,
-				       BACKLIGHT_PARAM_POWER, BACKLIGHT_OFF);
+				       FEAT_BACKLIGHT_POWER, BACKLIGHT_OFF);
 		else
 			call_fext_func(fext, FUNC_BACKLIGHT, OP_SET,
-				       BACKLIGHT_PARAM_POWER, BACKLIGHT_ON);
+				       FEAT_BACKLIGHT_POWER, BACKLIGHT_ON);
 	}
 
 	return set_lcd_level(device, b->props.brightness);
@@ -606,12 +609,12 @@ static int logolamp_set(struct led_classdev *cdev,
 		always = FUNC_LED_OFF;
 
 	ret = call_fext_func(device, FUNC_LEDS, OP_SET,
-			     LOGOLAMP_POWERON, poweron);
+			     FEAT_LOGOLAMP_POWERON, poweron);
 	if (ret < 0)
 		return ret;
 
 	return call_fext_func(device, FUNC_LEDS, OP_SET,
-			      LOGOLAMP_ALWAYS, always);
+			      FEAT_LOGOLAMP_ALWAYS, always);
 }
 
 static enum led_brightness logolamp_get(struct led_classdev *cdev)
@@ -619,11 +622,13 @@ 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, OP_GET, LOGOLAMP_ALWAYS, 0x0);
+	ret = call_fext_func(device, FUNC_LEDS, OP_GET,
+			     FEAT_LOGOLAMP_ALWAYS, 0x0);
 	if (ret == FUNC_LED_ON)
 		return LED_FULL;
 
-	ret = call_fext_func(device, FUNC_LEDS, OP_GET, LOGOLAMP_POWERON, 0x0);
+	ret = call_fext_func(device, FUNC_LEDS, OP_GET,
+			     FEAT_LOGOLAMP_POWERON, 0x0);
 	if (ret == FUNC_LED_ON)
 		return LED_HALF;
 
@@ -637,10 +642,10 @@ static int kblamps_set(struct led_classdev *cdev,
 
 	if (brightness >= LED_FULL)
 		return call_fext_func(device, FUNC_LEDS, OP_SET,
-				      KEYBOARD_LAMPS, FUNC_LED_ON);
+				      FEAT_KEYBOARD_LAMPS, FUNC_LED_ON);
 	else
 		return call_fext_func(device, FUNC_LEDS, OP_SET,
-				      KEYBOARD_LAMPS, FUNC_LED_OFF);
+				      FEAT_KEYBOARD_LAMPS, FUNC_LED_OFF);
 }
 
 static enum led_brightness kblamps_get(struct led_classdev *cdev)
@@ -649,7 +654,7 @@ static enum led_brightness kblamps_get(struct led_classdev *cdev)
 	enum led_brightness brightness = LED_OFF;
 
 	if (call_fext_func(device, FUNC_LEDS, OP_GET,
-			   KEYBOARD_LAMPS, 0x0) == FUNC_LED_ON)
+			   FEAT_KEYBOARD_LAMPS, 0x0) == FUNC_LED_ON)
 		brightness = LED_FULL;
 
 	return brightness;
@@ -662,10 +667,10 @@ static int radio_led_set(struct led_classdev *cdev,
 
 	if (brightness >= LED_FULL)
 		return call_fext_func(device, FUNC_FLAGS, OP_SET_EXT,
-				      RADIO_LED_ON, RADIO_LED_ON);
+				      FEAT_RADIO_LED, RADIO_LED_ON);
 	else
 		return call_fext_func(device, FUNC_FLAGS, OP_SET_EXT,
-				      RADIO_LED_ON, 0x0);
+				      FEAT_RADIO_LED, 0x0);
 }
 
 static enum led_brightness radio_led_get(struct led_classdev *cdev)
@@ -686,13 +691,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, OP_GET, ECO_LED, 0x0);
+	curr = call_fext_func(device, FUNC_LEDS, OP_GET, FEAT_ECO_LED, 0x0);
 	if (brightness >= LED_FULL)
 		return call_fext_func(device, FUNC_LEDS, OP_SET,
-				      ECO_LED, curr | ECO_LED_ON);
+				      FEAT_ECO_LED, curr | ECO_LED_ON);
 	else
 		return call_fext_func(device, FUNC_LEDS, OP_SET,
-				      ECO_LED, curr & ~ECO_LED_ON);
+				      FEAT_ECO_LED, curr & ~ECO_LED_ON);
 }
 
 static enum led_brightness eco_led_get(struct led_classdev *cdev)
@@ -701,7 +706,7 @@ static enum led_brightness eco_led_get(struct led_classdev *cdev)
 	enum led_brightness brightness = LED_OFF;
 
 	if (call_fext_func(device, FUNC_LEDS, OP_GET,
-			   ECO_LED, 0x0) & ECO_LED_ON)
+			   FEAT_ECO_LED, 0x0) & ECO_LED_ON)
 		brightness = LED_FULL;
 
 	return brightness;
@@ -714,7 +719,7 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
 	int ret;
 
 	if (call_fext_func(device, FUNC_LEDS, OP_GET_CAPS,
-			   0x0, 0x0) & LOGOLAMP_POWERON) {
+			   0x0, 0x0) & FEAT_LOGOLAMP_POWERON) {
 		led = devm_kzalloc(&device->dev, sizeof(*led), GFP_KERNEL);
 		if (!led)
 			return -ENOMEM;
@@ -728,7 +733,7 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
 	}
 
 	if ((call_fext_func(device, FUNC_LEDS, OP_GET_CAPS,
-			    0x0, 0x0) & KEYBOARD_LAMPS) &&
+			    0x0, 0x0) & FEAT_KEYBOARD_LAMPS) &&
 	    (call_fext_func(device, FUNC_BUTTONS, OP_GET_CAPS,
 			    0x0, 0x0) == 0x0)) {
 		led = devm_kzalloc(&device->dev, sizeof(*led), GFP_KERNEL);
@@ -774,7 +779,7 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
 	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)) {
+			    FEAT_ECO_LED, 0x0) != UNSUPPORTED_CMD)) {
 		led = devm_kzalloc(&device->dev, sizeof(*led), GFP_KERNEL);
 		if (!led)
 			return -ENOMEM;
@@ -844,7 +849,7 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 	if (fujitsu_bl && fujitsu_bl->bl_device &&
 	    acpi_video_get_backlight_type() == acpi_backlight_vendor) {
 		if (call_fext_func(fext, FUNC_BACKLIGHT, OP_GET,
-				   BACKLIGHT_PARAM_POWER, 0x0) == BACKLIGHT_OFF)
+				   FEAT_BACKLIGHT_POWER, 0x0) == BACKLIGHT_OFF)
 			fujitsu_bl->bl_device->props.power = FB_BLANK_POWERDOWN;
 		else
 			fujitsu_bl->bl_device->props.power = FB_BLANK_UNBLANK;
-- 
2.16.2

[PATCH 3/7] platform/x86: fujitsu-laptop: Define constants for FUNC feature states

[Michał Kępień]

Various functions exposed by the firmware through the FUNC interface
allow read/write access to the state of certain features.  Make sure
these states are referred to by consistently named constants instead of
integers in order to better convey the intent of each call_fext_func()
invocation.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/fujitsu-laptop.c | 50 +++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 23 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 087b5d1f2f4a..3e824e961260 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -101,22 +101,23 @@
 #define FLAG_DOCK			BIT(9)
 
 /* FUNC interface - LED control */
-#define FUNC_LED_OFF			BIT(0)
-#define FUNC_LED_ON			(BIT(0) | BIT(16) | BIT(17))
+#define STATE_LED_OFF			BIT(0)
+#define STATE_LED_ON			(BIT(0) | BIT(16) | BIT(17))
 #define FEAT_LOGOLAMP_POWERON		BIT(13)
 #define FEAT_LOGOLAMP_ALWAYS		BIT(14)
 #define FEAT_KEYBOARD_LAMPS		BIT(8)
 
 #define FEAT_RADIO_LED			BIT(5)
-#define RADIO_LED_ON			BIT(5)
+#define STATE_RADIO_LED_OFF		0
+#define STATE_RADIO_LED_ON		BIT(5)
 
 #define FEAT_ECO_LED			BIT(16)
-#define ECO_LED_ON			BIT(19)
+#define STATE_ECO_LED_ON		BIT(19)
 
 /* FUNC interface - backlight power control */
 #define FEAT_BACKLIGHT_POWER		BIT(2)
-#define BACKLIGHT_OFF			(BIT(0) | BIT(1))
-#define BACKLIGHT_ON			0
+#define STATE_BACKLIGHT_OFF		(BIT(0) | BIT(1))
+#define STATE_BACKLIGHT_ON		0
 
 /* Scancodes read from the GIRB register */
 #define KEY1_CODE			0x410
@@ -276,10 +277,12 @@ static int bl_update_status(struct backlight_device *b)
 	if (fext) {
 		if (b->props.power == FB_BLANK_POWERDOWN)
 			call_fext_func(fext, FUNC_BACKLIGHT, OP_SET,
-				       FEAT_BACKLIGHT_POWER, BACKLIGHT_OFF);
+				       FEAT_BACKLIGHT_POWER,
+				       STATE_BACKLIGHT_OFF);
 		else
 			call_fext_func(fext, FUNC_BACKLIGHT, OP_SET,
-				       FEAT_BACKLIGHT_POWER, BACKLIGHT_ON);
+				       FEAT_BACKLIGHT_POWER,
+				       STATE_BACKLIGHT_ON);
 	}
 
 	return set_lcd_level(device, b->props.brightness);
@@ -599,14 +602,14 @@ static int logolamp_set(struct led_classdev *cdev,
 			enum led_brightness brightness)
 {
 	struct acpi_device *device = to_acpi_device(cdev->dev->parent);
-	int poweron = FUNC_LED_ON, always = FUNC_LED_ON;
+	int poweron = STATE_LED_ON, always = STATE_LED_ON;
 	int ret;
 
 	if (brightness < LED_HALF)
-		poweron = FUNC_LED_OFF;
+		poweron = STATE_LED_OFF;
 
 	if (brightness < LED_FULL)
-		always = FUNC_LED_OFF;
+		always = STATE_LED_OFF;
 
 	ret = call_fext_func(device, FUNC_LEDS, OP_SET,
 			     FEAT_LOGOLAMP_POWERON, poweron);
@@ -624,12 +627,12 @@ static enum led_brightness logolamp_get(struct led_classdev *cdev)
 
 	ret = call_fext_func(device, FUNC_LEDS, OP_GET,
 			     FEAT_LOGOLAMP_ALWAYS, 0x0);
-	if (ret == FUNC_LED_ON)
+	if (ret == STATE_LED_ON)
 		return LED_FULL;
 
 	ret = call_fext_func(device, FUNC_LEDS, OP_GET,
 			     FEAT_LOGOLAMP_POWERON, 0x0);
-	if (ret == FUNC_LED_ON)
+	if (ret == STATE_LED_ON)
 		return LED_HALF;
 
 	return LED_OFF;
@@ -642,10 +645,10 @@ static int kblamps_set(struct led_classdev *cdev,
 
 	if (brightness >= LED_FULL)
 		return call_fext_func(device, FUNC_LEDS, OP_SET,
-				      FEAT_KEYBOARD_LAMPS, FUNC_LED_ON);
+				      FEAT_KEYBOARD_LAMPS, STATE_LED_ON);
 	else
 		return call_fext_func(device, FUNC_LEDS, OP_SET,
-				      FEAT_KEYBOARD_LAMPS, FUNC_LED_OFF);
+				      FEAT_KEYBOARD_LAMPS, STATE_LED_OFF);
 }
 
 static enum led_brightness kblamps_get(struct led_classdev *cdev)
@@ -654,7 +657,7 @@ static enum led_brightness kblamps_get(struct led_classdev *cdev)
 	enum led_brightness brightness = LED_OFF;
 
 	if (call_fext_func(device, FUNC_LEDS, OP_GET,
-			   FEAT_KEYBOARD_LAMPS, 0x0) == FUNC_LED_ON)
+			   FEAT_KEYBOARD_LAMPS, 0x0) == STATE_LED_ON)
 		brightness = LED_FULL;
 
 	return brightness;
@@ -667,10 +670,10 @@ static int radio_led_set(struct led_classdev *cdev,
 
 	if (brightness >= LED_FULL)
 		return call_fext_func(device, FUNC_FLAGS, OP_SET_EXT,
-				      FEAT_RADIO_LED, RADIO_LED_ON);
+				      FEAT_RADIO_LED, STATE_RADIO_LED_ON);
 	else
 		return call_fext_func(device, FUNC_FLAGS, OP_SET_EXT,
-				      FEAT_RADIO_LED, 0x0);
+				      FEAT_RADIO_LED, STATE_RADIO_LED_OFF);
 }
 
 static enum led_brightness radio_led_get(struct led_classdev *cdev)
@@ -679,7 +682,7 @@ static enum led_brightness radio_led_get(struct led_classdev *cdev)
 	enum led_brightness brightness = LED_OFF;
 
 	if (call_fext_func(device, FUNC_FLAGS, OP_GET_EXT,
-			   0x0, 0x0) & RADIO_LED_ON)
+			   0x0, 0x0) & STATE_RADIO_LED_ON)
 		brightness = LED_FULL;
 
 	return brightness;
@@ -694,10 +697,10 @@ static int eco_led_set(struct led_classdev *cdev,
 	curr = call_fext_func(device, FUNC_LEDS, OP_GET, FEAT_ECO_LED, 0x0);
 	if (brightness >= LED_FULL)
 		return call_fext_func(device, FUNC_LEDS, OP_SET,
-				      FEAT_ECO_LED, curr | ECO_LED_ON);
+				      FEAT_ECO_LED, curr | STATE_ECO_LED_ON);
 	else
 		return call_fext_func(device, FUNC_LEDS, OP_SET,
-				      FEAT_ECO_LED, curr & ~ECO_LED_ON);
+				      FEAT_ECO_LED, curr & ~STATE_ECO_LED_ON);
 }
 
 static enum led_brightness eco_led_get(struct led_classdev *cdev)
@@ -706,7 +709,7 @@ static enum led_brightness eco_led_get(struct led_classdev *cdev)
 	enum led_brightness brightness = LED_OFF;
 
 	if (call_fext_func(device, FUNC_LEDS, OP_GET,
-			   FEAT_ECO_LED, 0x0) & ECO_LED_ON)
+			   FEAT_ECO_LED, 0x0) & STATE_ECO_LED_ON)
 		brightness = LED_FULL;
 
 	return brightness;
@@ -849,7 +852,8 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 	if (fujitsu_bl && fujitsu_bl->bl_device &&
 	    acpi_video_get_backlight_type() == acpi_backlight_vendor) {
 		if (call_fext_func(fext, FUNC_BACKLIGHT, OP_GET,
-				   FEAT_BACKLIGHT_POWER, 0x0) == BACKLIGHT_OFF)
+				   FEAT_BACKLIGHT_POWER,
+				   0x0) == STATE_BACKLIGHT_OFF)
 			fujitsu_bl->bl_device->props.power = FB_BLANK_POWERDOWN;
 		else
 			fujitsu_bl->bl_device->props.power = FB_BLANK_UNBLANK;
-- 
2.16.2

[PATCH 4/7] platform/x86: fujitsu-laptop: Rename constants defining hotkey codes

[Michał Kępień]

Rename KEYx_CODE constants to EVENT_HKx, so that their names better fit
the OP_GET_EVENTS operation they are used with.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/fujitsu-laptop.c | 36 +++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 3e824e961260..5f8c89155b51 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -120,11 +120,11 @@
 #define STATE_BACKLIGHT_ON		0
 
 /* Scancodes read from the GIRB register */
-#define KEY1_CODE			0x410
-#define KEY2_CODE			0x411
-#define KEY3_CODE			0x412
-#define KEY4_CODE			0x413
-#define KEY5_CODE			0x420
+#define EVENT_HK1			0x410
+#define EVENT_HK2			0x411
+#define EVENT_HK3			0x412
+#define EVENT_HK4			0x413
+#define EVENT_HK5			0x420
 
 /* Hotkey ringbuffer limits */
 #define MAX_HOTKEY_RINGBUFFER_SIZE	100
@@ -470,28 +470,28 @@ static void acpi_fujitsu_bl_notify(struct acpi_device *device, u32 event)
 /* ACPI device for hotkey handling */
 
 static const struct key_entry keymap_default[] = {
-	{ KE_KEY, KEY1_CODE, { KEY_PROG1 } },
-	{ KE_KEY, KEY2_CODE, { KEY_PROG2 } },
-	{ KE_KEY, KEY3_CODE, { KEY_PROG3 } },
-	{ KE_KEY, KEY4_CODE, { KEY_PROG4 } },
-	{ KE_KEY, KEY5_CODE, { KEY_RFKILL } },
+	{ KE_KEY, EVENT_HK1, { KEY_PROG1 } },
+	{ KE_KEY, EVENT_HK2, { KEY_PROG2 } },
+	{ KE_KEY, EVENT_HK3, { KEY_PROG3 } },
+	{ KE_KEY, EVENT_HK4, { KEY_PROG4 } },
+	{ KE_KEY, EVENT_HK5, { KEY_RFKILL } },
 	{ KE_KEY, BIT(26),   { KEY_TOUCHPAD_TOGGLE } },
 	{ KE_END, 0 }
 };
 
 static const struct key_entry keymap_s64x0[] = {
-	{ KE_KEY, KEY1_CODE, { KEY_SCREENLOCK } },	/* "Lock" */
-	{ KE_KEY, KEY2_CODE, { KEY_HELP } },		/* "Mobility Center */
-	{ KE_KEY, KEY3_CODE, { KEY_PROG3 } },
-	{ KE_KEY, KEY4_CODE, { KEY_PROG4 } },
+	{ KE_KEY, EVENT_HK1, { KEY_SCREENLOCK } },	/* "Lock" */
+	{ KE_KEY, EVENT_HK2, { KEY_HELP } },		/* "Mobility Center */
+	{ KE_KEY, EVENT_HK3, { KEY_PROG3 } },
+	{ KE_KEY, EVENT_HK4, { KEY_PROG4 } },
 	{ KE_END, 0 }
 };
 
 static const struct key_entry keymap_p8010[] = {
-	{ KE_KEY, KEY1_CODE, { KEY_HELP } },		/* "Support" */
-	{ KE_KEY, KEY2_CODE, { KEY_PROG2 } },
-	{ KE_KEY, KEY3_CODE, { KEY_SWITCHVIDEOMODE } },	/* "Presentation" */
-	{ KE_KEY, KEY4_CODE, { KEY_WWW } },		/* "WWW" */
+	{ KE_KEY, EVENT_HK1, { KEY_HELP } },		/* "Support" */
+	{ KE_KEY, EVENT_HK2, { KEY_PROG2 } },
+	{ KE_KEY, EVENT_HK3, { KEY_SWITCHVIDEOMODE } },	/* "Presentation" */
+	{ KE_KEY, EVENT_HK4, { KEY_WWW } },		/* "WWW" */
 	{ KE_END, 0 }
 };
 
-- 
2.16.2

[PATCH 5/7] platform/x86: fujitsu-laptop: Tweak how constants are commented and laid out

[Michał Kępień]

Update comments used for each group of constants to better reflect their
current purpose.  Ensure the layout of groups of constants follows the
order in which call_fext_func() takes its arguments.  Use alphabetic
ordering for groups of constants.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/fujitsu-laptop.c | 55 +++++++++++++++++------------------
 1 file changed, 27 insertions(+), 28 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 5f8c89155b51..5acf1ccc6864 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -78,15 +78,15 @@
 
 #define ACPI_FUJITSU_NOTIFY_CODE	0x80
 
-/* FUNC interface - command values */
-#define FUNC_FLAGS			BIT(12)
-#define FUNC_LEDS			(BIT(12) | BIT(0))
-#define FUNC_BUTTONS			(BIT(12) | BIT(1))
-#define FUNC_BACKLIGHT			(BIT(12) | BIT(2))
-
 /* FUNC interface - responses */
 #define UNSUPPORTED_CMD			BIT(31)
 
+/* FUNC interface - function selectors */
+#define FUNC_BACKLIGHT			(BIT(12) | BIT(2))
+#define FUNC_BUTTONS			(BIT(12) | BIT(1))
+#define FUNC_FLAGS			BIT(12)
+#define FUNC_LEDS			(BIT(12) | BIT(0))
+
 /* FUNC interface - operations */
 #define OP_GET				BIT(1)
 #define OP_GET_CAPS			0
@@ -95,41 +95,40 @@
 #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)
-#define FLAG_DOCK			BIT(9)
-
-/* FUNC interface - LED control */
-#define STATE_LED_OFF			BIT(0)
-#define STATE_LED_ON			(BIT(0) | BIT(16) | BIT(17))
-#define FEAT_LOGOLAMP_POWERON		BIT(13)
-#define FEAT_LOGOLAMP_ALWAYS		BIT(14)
-#define FEAT_KEYBOARD_LAMPS		BIT(8)
-
-#define FEAT_RADIO_LED			BIT(5)
-#define STATE_RADIO_LED_OFF		0
-#define STATE_RADIO_LED_ON		BIT(5)
-
-#define FEAT_ECO_LED			BIT(16)
-#define STATE_ECO_LED_ON		BIT(19)
-
-/* FUNC interface - backlight power control */
+/* Constants related to FUNC_BACKLIGHT */
 #define FEAT_BACKLIGHT_POWER		BIT(2)
 #define STATE_BACKLIGHT_OFF		(BIT(0) | BIT(1))
 #define STATE_BACKLIGHT_ON		0
 
-/* Scancodes read from the GIRB register */
+/* Constants related to FUNC_BUTTONS */
 #define EVENT_HK1			0x410
 #define EVENT_HK2			0x411
 #define EVENT_HK3			0x412
 #define EVENT_HK4			0x413
 #define EVENT_HK5			0x420
 
-/* Hotkey ringbuffer limits */
 #define MAX_HOTKEY_RINGBUFFER_SIZE	100
 #define RINGBUFFERSIZE			40
 
+/* Constant related to FUNC_FLAGS */
+#define FLAG_DOCK			BIT(9)
+#define FLAG_LID			BIT(8)
+#define FLAG_RFKILL			BIT(5)
+
+/* Constants related to FUNC_LEDS */
+#define FEAT_KEYBOARD_LAMPS		BIT(8)
+#define FEAT_LOGOLAMP_ALWAYS		BIT(14)
+#define FEAT_LOGOLAMP_POWERON		BIT(13)
+#define STATE_LED_OFF			BIT(0)
+#define STATE_LED_ON			(BIT(0) | BIT(16) | BIT(17))
+
+#define FEAT_RADIO_LED			BIT(5)
+#define STATE_RADIO_LED_OFF		0
+#define STATE_RADIO_LED_ON		BIT(5)
+
+#define FEAT_ECO_LED			BIT(16)
+#define STATE_ECO_LED_ON		BIT(19)
+
 /* Module parameters */
 static int use_alt_lcd_levels = -1;
 static bool disable_brightness_adjust;
-- 
2.16.2

[PATCH 6/7] platform/x86: fujitsu-laptop: More accurately represent the hotkey ring buffer managed by firmware

[Michał Kępień]

The MAX_HOTKEY_RINGBUFFER_SIZE constant is set to 100, which allows up
to 100 hotkey events to be drained from the firmware ring buffer upon
module load.  However, no known firmware is capable of holding more than
16 hotkey events in its internal ring buffer:

    Method (SIRB, 1, NotSerialized)
    {
        If ((BNCT < 0x10))
        {
            CreateWordField (BNBF, BNSP, BNP1)
            BNP1 = Arg0
            BNCT++
            BNSP += 0x02
            If ((BNSP >= 0x20))
            {
                BNSP = Zero
            }
        }
    }

The RINGBUFFERSIZE constant is set to 40, which allows the module to
queue up to 40 hotkey events for delivery to user space.  As this value
seems arbitrarily chosen and 16 should be more than enough for any
practical use case, merge the two aforementioned constants into one
(HOTKEY_RINGBUFFER_SIZE) in order to simplify code and more accurately
represent the underlying data structure.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/fujitsu-laptop.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 5acf1ccc6864..46c9f4441c24 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -107,8 +107,7 @@
 #define EVENT_HK4			0x413
 #define EVENT_HK5			0x420
 
-#define MAX_HOTKEY_RINGBUFFER_SIZE	100
-#define RINGBUFFERSIZE			40
+#define HOTKEY_RINGBUFFER_SIZE		16
 
 /* Constant related to FUNC_FLAGS */
 #define FLAG_DOCK			BIT(9)
@@ -815,7 +814,7 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 
 	/* kfifo */
 	spin_lock_init(&priv->fifo_lock);
-	ret = kfifo_alloc(&priv->fifo, RINGBUFFERSIZE * sizeof(int),
+	ret = kfifo_alloc(&priv->fifo, HOTKEY_RINGBUFFER_SIZE * sizeof(int),
 			  GFP_KERNEL);
 	if (ret)
 		return ret;
@@ -825,7 +824,7 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 
 	while (call_fext_func(device, FUNC_BUTTONS, OP_GET_EVENTS,
 			      0x0, 0x0) != 0 &&
-	       i++ < MAX_HOTKEY_RINGBUFFER_SIZE)
+	       i++ < HOTKEY_RINGBUFFER_SIZE)
 		; /* No action, result is discarded */
 	acpi_handle_debug(device->handle, "Discarded %i ringbuffer entries\n",
 			  i);
@@ -941,7 +940,7 @@ static void acpi_fujitsu_laptop_notify(struct acpi_device *device, u32 event)
 
 	while ((irb = call_fext_func(device, FUNC_BUTTONS, OP_GET_EVENTS,
 				     0x0, 0x0)) != 0 &&
-	       i++ < MAX_HOTKEY_RINGBUFFER_SIZE) {
+	       i++ < HOTKEY_RINGBUFFER_SIZE) {
 		scancode = irb & 0x4ff;
 		if (sparse_keymap_entry_from_scancode(priv->input, scancode))
 			acpi_fujitsu_laptop_press(device, scancode);
-- 
2.16.2

Re: [PATCH 6/7] platform/x86: fujitsu-laptop: More accurately represent the hotkey ring buffer managed by firmware

[Andy Shevchenko]

On Tue, Feb 27, 2018 at 11:15 PM, Michał Kępień <kernel@kempniu.pl> wrote:
> The MAX_HOTKEY_RINGBUFFER_SIZE constant is set to 100, which allows up
> to 100 hotkey events to be drained from the firmware ring buffer upon
> module load.  However, no known firmware is capable of holding more than
> 16 hotkey events in its internal ring buffer:

> The RINGBUFFERSIZE constant is set to 40, which allows the module to
> queue up to 40 hotkey events for delivery to user space.  As this value
> seems arbitrarily chosen and 16 should be more than enough for any
> practical use case, merge the two aforementioned constants into one
> (HOTKEY_RINGBUFFER_SIZE) in order to simplify code and more accurately
> represent the underlying data structure.

> +#define HOTKEY_RINGBUFFER_SIZE         16

This need the comment or a

BUILD_BUG_ON(!is_power_of_2(...));


> -       ret = kfifo_alloc(&priv->fifo, RINGBUFFERSIZE * sizeof(int),
> +       ret = kfifo_alloc(&priv->fifo, HOTKEY_RINGBUFFER_SIZE * sizeof(int),
>                           GFP_KERNEL);

>         while (call_fext_func(device, FUNC_BUTTONS, OP_GET_EVENTS,
>                               0x0, 0x0) != 0 &&
> -              i++ < MAX_HOTKEY_RINGBUFFER_SIZE)
> +              i++ < HOTKEY_RINGBUFFER_SIZE)
>                 ; /* No action, result is discarded */

This looks horrible.
Can it be redone

do {
 if (call...() == 0)
  break;
} while (i++ < ...);

?

>         while ((irb = call_fext_func(device, FUNC_BUTTONS, OP_GET_EVENTS,
>                                      0x0, 0x0)) != 0 &&
> -              i++ < MAX_HOTKEY_RINGBUFFER_SIZE) {
> +              i++ < HOTKEY_RINGBUFFER_SIZE) {

Ditto.

>                 scancode = irb & 0x4ff;
>                 if (sparse_keymap_entry_from_scancode(priv->input, scancode))
>                         acpi_fujitsu_laptop_press(device, scancode);


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/7] platform/x86: fujitsu-laptop: More accurately represent the hotkey ring buffer managed by firmware

[Michał Kępień]

> On Tue, Feb 27, 2018 at 11:15 PM, Michał Kępień <kernel@kempniu.pl> wrote:
> > The MAX_HOTKEY_RINGBUFFER_SIZE constant is set to 100, which allows up
> > to 100 hotkey events to be drained from the firmware ring buffer upon
> > module load.  However, no known firmware is capable of holding more than
> > 16 hotkey events in its internal ring buffer:
> 
> > The RINGBUFFERSIZE constant is set to 40, which allows the module to
> > queue up to 40 hotkey events for delivery to user space.  As this value
> > seems arbitrarily chosen and 16 should be more than enough for any
> > practical use case, merge the two aforementioned constants into one
> > (HOTKEY_RINGBUFFER_SIZE) in order to simplify code and more accurately
> > represent the underlying data structure.
> 
> > +#define HOTKEY_RINGBUFFER_SIZE         16
> 
> This need the comment or a
> 
> BUILD_BUG_ON(!is_power_of_2(...));

Okay, I will probably take the comment route in v2.

> > -       ret = kfifo_alloc(&priv->fifo, RINGBUFFERSIZE * sizeof(int),
> > +       ret = kfifo_alloc(&priv->fifo, HOTKEY_RINGBUFFER_SIZE * sizeof(int),
> >                           GFP_KERNEL);
> 
> >         while (call_fext_func(device, FUNC_BUTTONS, OP_GET_EVENTS,
> >                               0x0, 0x0) != 0 &&
> > -              i++ < MAX_HOTKEY_RINGBUFFER_SIZE)
> > +              i++ < HOTKEY_RINGBUFFER_SIZE)
> >                 ; /* No action, result is discarded */
> 
> This looks horrible.

It sure does!  Hence patch 7/7, which does the following:

-	while (call_fext_func(device, FUNC_BUTTONS, OP_GET_EVENTS,
-			      0x0, 0x0) != 0 &&
+	while (fext_buttons(device, OP_GET_EVENTS, 0x0, 0x0) != 0 &&
	       i++ < HOTKEY_RINGBUFFER_SIZE)

In other words, patch 6/7 is just a stopover on the way to shorten
current module code:

-	while (call_fext_func(device, FUNC_BUTTONS, 0x1, 0x0, 0x0) != 0 &&
-	       i++ < MAX_HOTKEY_RINGBUFFER_SIZE)
+	while (fext_buttons(device, OP_GET_EVENTS, 0x0, 0x0) != 0 &&
+	       i++ < HOTKEY_RINGBUFFER_SIZE)

> >         while ((irb = call_fext_func(device, FUNC_BUTTONS, OP_GET_EVENTS,
> >                                      0x0, 0x0)) != 0 &&
> > -              i++ < MAX_HOTKEY_RINGBUFFER_SIZE) {
> > +              i++ < HOTKEY_RINGBUFFER_SIZE) {
> 
> Ditto.

Similarly, patch 7/7 does the following:

-	while ((irb = call_fext_func(device, FUNC_BUTTONS, OP_GET_EVENTS,
-				     0x0, 0x0)) != 0 &&
+	while ((irb = fext_buttons(device, OP_GET_EVENTS, 0x0, 0x0)) != 0 &&
	       i++ < HOTKEY_RINGBUFFER_SIZE) {

The diff against current module code is thus:

-	while ((irb = call_fext_func(device,
-				     FUNC_BUTTONS, 0x1, 0x0, 0x0)) != 0 &&
-	       i++ < MAX_HOTKEY_RINGBUFFER_SIZE) {
+	while ((irb = fext_buttons(device, OP_GET_EVENTS, 0x0, 0x0)) != 0 &&
+	       i++ < HOTKEY_RINGBUFFER_SIZE) {

-- 
Best regards,
Michał Kępień

[PATCH 7/7] platform/x86: fujitsu-laptop: Introduce fext_*() helper functions

[Michał Kępień]

Stop invoking call_fext_func() directly to improve code clarity and save
some horizontal space.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/fujitsu-laptop.c | 116 +++++++++++++++++-----------------
 1 file changed, 59 insertions(+), 57 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 46c9f4441c24..9ac9283fa707 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -184,6 +184,30 @@ static int call_fext_func(struct acpi_device *device,
 	return value;
 }
 
+static int fext_backlight(struct acpi_device *device,
+			  int op, int feature, int state)
+{
+	return call_fext_func(device, FUNC_BACKLIGHT, op, feature, state);
+}
+
+static int fext_buttons(struct acpi_device *device,
+			int op, int feature, int state)
+{
+	return call_fext_func(device, FUNC_BUTTONS, op, feature, state);
+}
+
+static int fext_flags(struct acpi_device *device,
+		      int op, int feature, int state)
+{
+	return call_fext_func(device, FUNC_FLAGS, op, feature, state);
+}
+
+static int fext_leds(struct acpi_device *device,
+		     int op, int feature, int state)
+{
+	return call_fext_func(device, FUNC_LEDS, op, feature, state);
+}
+
 /* Hardware access for LCD brightness control */
 
 static int set_lcd_level(struct acpi_device *device, int level)
@@ -274,12 +298,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, OP_SET,
-				       FEAT_BACKLIGHT_POWER,
+			fext_backlight(fext, OP_SET, FEAT_BACKLIGHT_POWER,
 				       STATE_BACKLIGHT_OFF);
 		else
-			call_fext_func(fext, FUNC_BACKLIGHT, OP_SET,
-				       FEAT_BACKLIGHT_POWER,
+			fext_backlight(fext, OP_SET, FEAT_BACKLIGHT_POWER,
 				       STATE_BACKLIGHT_ON);
 	}
 
@@ -609,13 +631,11 @@ static int logolamp_set(struct led_classdev *cdev,
 	if (brightness < LED_FULL)
 		always = STATE_LED_OFF;
 
-	ret = call_fext_func(device, FUNC_LEDS, OP_SET,
-			     FEAT_LOGOLAMP_POWERON, poweron);
+	ret = fext_leds(device, OP_SET, FEAT_LOGOLAMP_POWERON, poweron);
 	if (ret < 0)
 		return ret;
 
-	return call_fext_func(device, FUNC_LEDS, OP_SET,
-			      FEAT_LOGOLAMP_ALWAYS, always);
+	return fext_leds(device, OP_SET, FEAT_LOGOLAMP_ALWAYS, always);
 }
 
 static enum led_brightness logolamp_get(struct led_classdev *cdev)
@@ -623,13 +643,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, OP_GET,
-			     FEAT_LOGOLAMP_ALWAYS, 0x0);
+	ret = fext_leds(device, OP_GET, FEAT_LOGOLAMP_ALWAYS, 0x0);
 	if (ret == STATE_LED_ON)
 		return LED_FULL;
 
-	ret = call_fext_func(device, FUNC_LEDS, OP_GET,
-			     FEAT_LOGOLAMP_POWERON, 0x0);
+	ret = fext_leds(device, OP_GET, FEAT_LOGOLAMP_POWERON, 0x0);
 	if (ret == STATE_LED_ON)
 		return LED_HALF;
 
@@ -642,11 +660,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, OP_SET,
-				      FEAT_KEYBOARD_LAMPS, STATE_LED_ON);
+		return fext_leds(device, OP_SET, FEAT_KEYBOARD_LAMPS,
+				 STATE_LED_ON);
 	else
-		return call_fext_func(device, FUNC_LEDS, OP_SET,
-				      FEAT_KEYBOARD_LAMPS, STATE_LED_OFF);
+		return fext_leds(device, OP_SET, FEAT_KEYBOARD_LAMPS,
+				 STATE_LED_OFF);
 }
 
 static enum led_brightness kblamps_get(struct led_classdev *cdev)
@@ -654,8 +672,7 @@ 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, OP_GET,
-			   FEAT_KEYBOARD_LAMPS, 0x0) == STATE_LED_ON)
+	if (fext_leds(device, OP_GET, FEAT_KEYBOARD_LAMPS, 0x0) == STATE_LED_ON)
 		brightness = LED_FULL;
 
 	return brightness;
@@ -667,11 +684,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, OP_SET_EXT,
-				      FEAT_RADIO_LED, STATE_RADIO_LED_ON);
+		return fext_flags(device, OP_SET_EXT, FEAT_RADIO_LED,
+				  STATE_RADIO_LED_ON);
 	else
-		return call_fext_func(device, FUNC_FLAGS, OP_SET_EXT,
-				      FEAT_RADIO_LED, STATE_RADIO_LED_OFF);
+		return fext_flags(device, OP_SET_EXT, FEAT_RADIO_LED,
+				  STATE_RADIO_LED_OFF);
 }
 
 static enum led_brightness radio_led_get(struct led_classdev *cdev)
@@ -679,8 +696,7 @@ 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, OP_GET_EXT,
-			   0x0, 0x0) & STATE_RADIO_LED_ON)
+	if (fext_flags(device, OP_GET_EXT, 0x0, 0x0) & STATE_RADIO_LED_ON)
 		brightness = LED_FULL;
 
 	return brightness;
@@ -692,13 +708,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, OP_GET, FEAT_ECO_LED, 0x0);
+	curr = fext_leds(device, OP_GET, FEAT_ECO_LED, 0x0);
 	if (brightness >= LED_FULL)
-		return call_fext_func(device, FUNC_LEDS, OP_SET,
-				      FEAT_ECO_LED, curr | STATE_ECO_LED_ON);
+		return fext_leds(device, OP_SET, FEAT_ECO_LED,
+				 curr | STATE_ECO_LED_ON);
 	else
-		return call_fext_func(device, FUNC_LEDS, OP_SET,
-				      FEAT_ECO_LED, curr & ~STATE_ECO_LED_ON);
+		return fext_leds(device, OP_SET, FEAT_ECO_LED,
+				 curr & ~STATE_ECO_LED_ON);
 }
 
 static enum led_brightness eco_led_get(struct led_classdev *cdev)
@@ -706,8 +722,7 @@ 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, OP_GET,
-			   FEAT_ECO_LED, 0x0) & STATE_ECO_LED_ON)
+	if (fext_leds(device, OP_GET, FEAT_ECO_LED, 0x0) & STATE_ECO_LED_ON)
 		brightness = LED_FULL;
 
 	return brightness;
@@ -719,8 +734,7 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
 	struct led_classdev *led;
 	int ret;
 
-	if (call_fext_func(device, FUNC_LEDS, OP_GET_CAPS,
-			   0x0, 0x0) & FEAT_LOGOLAMP_POWERON) {
+	if (fext_leds(device, OP_GET_CAPS, 0x0, 0x0) & FEAT_LOGOLAMP_POWERON) {
 		led = devm_kzalloc(&device->dev, sizeof(*led), GFP_KERNEL);
 		if (!led)
 			return -ENOMEM;
@@ -733,10 +747,8 @@ static int acpi_fujitsu_laptop_leds_register(struct acpi_device *device)
 			return ret;
 	}
 
-	if ((call_fext_func(device, FUNC_LEDS, OP_GET_CAPS,
-			    0x0, 0x0) & FEAT_KEYBOARD_LAMPS) &&
-	    (call_fext_func(device, FUNC_BUTTONS, OP_GET_CAPS,
-			    0x0, 0x0) == 0x0)) {
+	if ((fext_leds(device, OP_GET_CAPS, 0x0, 0x0) & FEAT_KEYBOARD_LAMPS) &&
+	    (fext_buttons(device, OP_GET_CAPS, 0x0, 0x0) == 0x0)) {
 		led = devm_kzalloc(&device->dev, sizeof(*led), GFP_KERNEL);
 		if (!led)
 			return -ENOMEM;
@@ -777,10 +789,8 @@ 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, OP_GET_CAPS,
-			    0x0, 0x0) & BIT(14)) &&
-	    (call_fext_func(device, FUNC_LEDS, OP_GET,
-			    FEAT_ECO_LED, 0x0) != UNSUPPORTED_CMD)) {
+	if ((fext_leds(device, OP_GET_CAPS, 0x0, 0x0) & BIT(14)) &&
+	    (fext_leds(device, OP_GET, FEAT_ECO_LED, 0x0) != UNSUPPORTED_CMD)) {
 		led = devm_kzalloc(&device->dev, sizeof(*led), GFP_KERNEL);
 		if (!led)
 			return -ENOMEM;
@@ -822,15 +832,13 @@ 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, OP_GET_EVENTS,
-			      0x0, 0x0) != 0 &&
+	while (fext_buttons(device, OP_GET_EVENTS, 0x0, 0x0) != 0 &&
 	       i++ < 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, OP_GET_CAPS,
-					       0x0, 0x0);
+	priv->flags_supported = fext_flags(device, 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. */
@@ -838,19 +846,16 @@ 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,
-						   OP_GET_EXT, 0x0, 0x0);
+		priv->flags_state = fext_flags(device, 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, OP_GET_CAPS,
-					0x0, 0x0));
+			 fext_buttons(device, 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, OP_GET,
-				   FEAT_BACKLIGHT_POWER,
+		if (fext_backlight(fext, OP_GET, FEAT_BACKLIGHT_POWER,
 				   0x0) == STATE_BACKLIGHT_OFF)
 			fujitsu_bl->bl_device->props.power = FB_BLANK_POWERDOWN;
 		else
@@ -935,11 +940,9 @@ 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,
-						   OP_GET_EXT, 0x0, 0x0);
+		priv->flags_state = fext_flags(device, OP_GET_EXT, 0x0, 0x0);
 
-	while ((irb = call_fext_func(device, FUNC_BUTTONS, OP_GET_EVENTS,
-				     0x0, 0x0)) != 0 &&
+	while ((irb = fext_buttons(device, OP_GET_EVENTS, 0x0, 0x0)) != 0 &&
 	       i++ < HOTKEY_RINGBUFFER_SIZE) {
 		scancode = irb & 0x4ff;
 		if (sparse_keymap_entry_from_scancode(priv->input, scancode))
@@ -956,8 +959,7 @@ 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, OP_GET_EVENTS,
-			    0x0, 0x0) & BIT(26)))
+	    (fext_flags(device, OP_GET_EVENTS, 0x0, 0x0) & BIT(26)))
 		sparse_keymap_report_event(priv->input, BIT(26), 1, true);
 }
 
-- 
2.16.2

Re: [PATCH 0/7] fujitsu-laptop: Consistent naming of constants

[Jonathan Woithe]

Hi Michal

On Tue, Feb 27, 2018 at 10:15:32PM +0100, Micha?? K??pie?? wrote:
> This patch series is an attempt to organize all the named constants used
> throughout fujitsu-laptop so that their names more clearly convey their
> purpose: a set of prefixes is introduced to "map" constant names to
> call_fext_func() parameters.

While fairly superficial in nature I think this patch set is worthwhile. 
Features have been progressively added to fujitsu-laptop over the last 10 or
so years from a number of sources but a consistent naming methodology for
constants has not emerged.  Having at least some consistency across the
constant names will help clarify the intent of the code.

> Some changes (like those in patches 4/7 and 5/7) may be perceived as
> bikeshedding, so please just think of them as proposals, not fixes.

Patches 4 and 5 don't bother me within the context of the patch series as a
whole.

> Finally, patch 7/7 again [1] proposes a set of helper functions which
> seem to be making quite a difference in terms of code readability in
> certain places (especially in long conditional expressions).  YMMV,
> though, feel free to disagree.

As before, I can't see any strong arguments one way or the other.  The
helper functions certainly save a line in many of the call sites, but
whether they provide significant advantages I cannot say (I personally have
no fundamental problems with either version).  I guess if pushed I'd
probably come down on the side of leaving things as they are: in principle
defining a bunch of thin wrapper functions to save one parameter isn't
something I tend to do since it doesn't seem worth it.  However, what has
changed since the last iteration of this idea is the use of identifiers
rather than numbers for many of call_fext_func()'s parameters, which adds
length to each call site.

If there is general agreement that using these functions is beneficial in
this context I certainly won't block it.

Regards
  jonathan

Re: [PATCH 0/7] fujitsu-laptop: Consistent naming of constants

[Darren Hart]

On Tue, Feb 27, 2018 at 10:15:32PM +0100, Michał Kępień wrote:
> This patch series is an attempt to organize all the named constants used
> throughout fujitsu-laptop so that their names more clearly convey their
> purpose: a set of prefixes is introduced to "map" constant names to
> call_fext_func() parameters.
> 
> The chosen constant naming patterns are a compromise between readability
> and enabling longer conditional expressions to fit into the 80-character
> line length limit.  Some changes (like those in patches 4/7 and 5/7) may
> be perceived as bikeshedding, so please just think of them as proposals,
> not fixes.
> 
> Finally, patch 7/7 again [1] proposes a set of helper functions which
> seem to be making quite a difference in terms of code readability in
> certain places (especially in long conditional expressions).  YMMV,
> though, feel free to disagree.
> 
> This patch series should be applied on top of for-next as it builds on
> the previous patch series I submitted.
> 
> [1] https://www.spinics.net/lists/platform-driver-x86/msg11633.html
> 
>  drivers/platform/x86/fujitsu-laptop.c | 228 +++++++++++++++++++---------------
>  1 file changed, 127 insertions(+), 101 deletions(-)

What I see here is an agreement on a simple integer listing for patch 1/7 per
Andy's latest reply. 7/7 appears to address Andy's concerns in 6/7. I agree they
KEY vs EVENT_HK is a bit of bikeshedding - but I don't have a preference and I
think overall the code is more readable for this series.

Based on feedback on a couple of the patches, a respin appears to be pending, so
I'm marking this series as "changes requested" and will await the v2.

Thanks,

-- 
Darren Hart
VMware Open Source Technology Center