Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1590081 > unrolled thread

[PATCH v2 0/4] fujitsu_init() cleanup

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/4] fujitsu_init() cleanup Michał Kępień <kernel@kempniu.pl> - 2017-03-01 09:20 +0100
    [PATCH v2 1/4] platform/x86: fujitsu-laptop: register backlight device in a separate function Michał Kępień <kernel@kempniu.pl> - 2017-03-01 09:20 +0100
    [PATCH v2 2/4] platform/x86: fujitsu-laptop: do not use call_fext_func() for LCD blanking Michał Kępień <kernel@kempniu.pl> - 2017-03-01 09:20 +0100
    [PATCH v2 3/4] platform/x86: fujitsu-laptop: only register backlight device if FUJ02B1 is present Michał Kępień <kernel@kempniu.pl> - 2017-03-01 09:20 +0100
    Re: [PATCH v2 0/4] fujitsu_init() cleanup [resend due to vger error] Jonathan Woithe <jwoithe@just42.net> - 2017-03-02 04:50 +0100
    Re: [PATCH v2 0/4] fujitsu_init() cleanup Michał Kępień <kernel@kempniu.pl> - 2017-03-02 09:30 +0100
      Re: [PATCH v2 0/4] fujitsu_init() cleanup Jonathan Woithe <jwoithe@just42.net> - 2017-03-03 13:50 +0100
    Re: [PATCH v2 0/4] fujitsu_init() cleanup Jonathan Woithe <jwoithe@just42.net> - 2017-03-04 03:10 +0100
      Re: [PATCH v2 0/4] fujitsu_init() cleanup Jonathan Woithe <jwoithe@just42.net> - 2017-03-06 01:20 +0100
        Re: [PATCH v2 0/4] fujitsu_init() cleanup Michał Kępień <kernel@kempniu.pl> - 2017-03-06 06:00 +0100
          Re: [PATCH v2 0/4] fujitsu_init() cleanup Jonathan Woithe <jwoithe@just42.net> - 2017-03-06 06:10 +0100
            Re: [PATCH v2 0/4] fujitsu_init() cleanup Jonathan Woithe <jwoithe@just42.net> - 2017-03-06 10:00 +0100
              Re: [PATCH v2 0/4] fujitsu_init() cleanup Michał Kępień <kernel@kempniu.pl> - 2017-03-06 10:40 +0100
                Re: [PATCH v2 0/4] fujitsu_init() cleanup Michał Kępień <kernel@kempniu.pl> - 2017-03-06 19:50 +0100
                  Re: [PATCH v2 0/4] fujitsu_init() cleanup Jonathan Woithe <jwoithe@just42.net> - 2017-03-07 05:00 +0100
                    Re: [PATCH v2 0/4] fujitsu_init() cleanup Michał Kępień <kernel@kempniu.pl> - 2017-03-07 09:20 +0100

#1590081 — [PATCH v2 0/4] fujitsu_init() cleanup

These patches should make fujitsu_init() a bit more palatable.  No
changes are made to platform device code yet, for clarity these will be
posted in a separate series after this one gets applied.

Changes from v1:

  - Rebase on top of reworked Alan Jenkins' cleanup patch series.

  - Drop patch 1/4 from v1 as it has already been applied in reworked
    Alan Jenkins' cleanup patch series.

  - Patch 3/4 from v1 has been replaced with a completely different one
    (patch 2/4).  It needs to be tested on a relevant machine as it is
    based purely on a dump of a DSDT table (further details can be found
    in the patch itself).

  - Patch 3/4 in v2 is a rebased version of patch 8/10 from the reworked
    Alan Jenkins' cleanup patch series.  Patch 2/4 from v2 (the one
    mentioned in the previous bullet point) ensures this one can be
    safely applied without causing a NULL dereference under any
    circumstances.

 drivers/platform/x86/fujitsu-laptop.c | 113 +++++++++++++++++-----------------
 1 file changed, 56 insertions(+), 57 deletions(-)

-- 
2.12.0

[toc] | [next] | [standalone]

#1590082 — [PATCH v2 1/4] platform/x86: fujitsu-laptop: register backlight device in a separate function

Move code responsible for backlight device registration to a separate
function in order to simplify error handling and decrease indentation.
Simplify initialization of struct backlight_properties.  Use
KBUILD_MODNAME as device name to avoid repeating the same string literal
throughout the module.

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

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index e12cc3504d48..185c929898d9 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -685,6 +685,25 @@ static const struct dmi_system_id fujitsu_dmi_table[] __initconst = {
 
 /* ACPI device for LCD brightness control */
 
+static int fujitsu_backlight_register(void)
+{
+	struct backlight_properties props = {
+		.brightness = fujitsu_bl->brightness_level,
+		.max_brightness = fujitsu_bl->max_brightness - 1,
+		.type = BACKLIGHT_PLATFORM
+	};
+	struct backlight_device *bd;
+
+	bd = backlight_device_register(KBUILD_MODNAME, NULL, NULL,
+				       &fujitsu_bl_ops, &props);
+	if (IS_ERR(bd))
+		return PTR_ERR(bd);
+
+	fujitsu_bl->bl_device = bd;
+
+	return 0;
+}
+
 static int acpi_fujitsu_bl_add(struct acpi_device *device)
 {
 	int state = 0;
@@ -1192,7 +1211,7 @@ MODULE_DEVICE_TABLE(acpi, fujitsu_ids);
 
 static int __init fujitsu_init(void)
 {
-	int ret, max_brightness;
+	int ret;
 
 	if (acpi_disabled)
 		return -ENODEV;
@@ -1232,22 +1251,9 @@ static int __init fujitsu_init(void)
 	/* Register backlight stuff */
 
 	if (acpi_video_get_backlight_type() == acpi_backlight_vendor) {
-		struct backlight_properties props;
-
-		memset(&props, 0, sizeof(struct backlight_properties));
-		max_brightness = fujitsu_bl->max_brightness;
-		props.type = BACKLIGHT_PLATFORM;
-		props.max_brightness = max_brightness - 1;
-		fujitsu_bl->bl_device = backlight_device_register("fujitsu-laptop",
-								  NULL, NULL,
-								  &fujitsu_bl_ops,
-								  &props);
-		if (IS_ERR(fujitsu_bl->bl_device)) {
-			ret = PTR_ERR(fujitsu_bl->bl_device);
-			fujitsu_bl->bl_device = NULL;
+		ret = fujitsu_backlight_register();
+		if (ret)
 			goto fail_sysfs_group;
-		}
-		fujitsu_bl->bl_device->props.brightness = fujitsu_bl->brightness_level;
 	}
 
 	ret = platform_driver_register(&fujitsu_pf_driver);
-- 
2.12.0

[toc] | [prev] | [next] | [standalone]

#1590084 — [PATCH v2 2/4] platform/x86: fujitsu-laptop: do not use call_fext_func() for LCD blanking

fujitsu-laptop registers two ACPI drivers: one for ACPI device FUJ02B1
enabling backlight control and another for ACPI device FUJ02E3 which
handles various other stuff (hotkeys, LEDs, etc.)  Sadly, one of the
functions exposed by call_fext_func() (i.e. through FUJ02E3) allows
probing and controlling LCD blanking (which is a backlight-related
feature that should logically be handled by FUJ02B1) and thus entangles
these two ACPI drivers, which is ugly.

Reverse engineering the DSDT table of a Lifebook S7020 shows that
current LCD blanking state can be:

  - read from ACPI variable BLCT (0: LCD on, 1: LCD off),
  - set using ACPI method SBLC belonging to ACPI device FUJ02E3.

Based on this information, reimplement LCD blanking without using
call_fext_func():

  - read backlight power from BLCT in fujitsu_backlight_register(),
  - grab a handle to SBLC in fujitsu_backlight_register() and then use
    it for setting backlight power in bl_update_status().

Apart from untangling the two ACPI drivers, this also prevents bogus
"Unable to adjust backlight power" messages from appearing on machines
which do not support LCD blanking through FUJ02E3.

Finally, this change eliminates the need to define and use
FUNC_BACKLIGHT, so remove it.

Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
Jonathan, this *really* needs testing on relevant hardware.  After
applying this patch, you should be able to turn LCD backlight on and off
using /sys/class/backlight/fujitsu-laptop/bl_power.  Also, the value
returned by that attribute upon read should be in sync with actual
backlight state even right after loading the module (i.e. before writing
anything to bl_power).  Please let me know if any of the above is not
true and the module works correctly without this patch applied.

 drivers/platform/x86/fujitsu-laptop.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 185c929898d9..afcc451e21f6 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -92,7 +92,6 @@
 #define FUNC_FLAGS	0x1000
 #define FUNC_LEDS	0x1001
 #define FUNC_BUTTONS	0x1002
-#define FUNC_BACKLIGHT  0x1004
 
 /* FUNC interface - responses */
 #define UNSUPPORTED_CMD 0x80000000
@@ -143,6 +142,7 @@
 /* Device controlling the backlight and associated keys */
 struct fujitsu_bl {
 	acpi_handle acpi_handle;
+	acpi_handle blank_handle;
 	struct acpi_device *dev;
 	struct input_dev *input;
 	char phys[32];
@@ -463,15 +463,12 @@ static int bl_get_brightness(struct backlight_device *b)
 
 static int bl_update_status(struct backlight_device *b)
 {
+	bool blank = b->props.power == FB_BLANK_POWERDOWN;
 	int ret;
-	if (b->props.power == FB_BLANK_POWERDOWN)
-		ret = call_fext_func(FUNC_BACKLIGHT, 0x1, 0x4, 0x3);
-	else
-		ret = call_fext_func(FUNC_BACKLIGHT, 0x1, 0x4, 0x0);
-	if (ret != 0)
-		vdbg_printk(FUJLAPTOP_DBG_ERROR,
-			"Unable to adjust backlight power, error code %i\n",
-			ret);
+
+	if (fujitsu_bl->blank_handle)
+		acpi_execute_simple_method(fujitsu_bl->blank_handle, NULL,
+					   blank);
 
 	if (use_alt_lcd_levels)
 		ret = set_lcd_level_alt(b->props.brightness);
@@ -693,6 +690,15 @@ static int fujitsu_backlight_register(void)
 		.type = BACKLIGHT_PLATFORM
 	};
 	struct backlight_device *bd;
+	unsigned long long blank;
+	acpi_status status;
+
+	/* Sync backlight power status */
+	status = acpi_evaluate_integer(NULL, "\\BLCT", NULL, &blank);
+	if (ACPI_SUCCESS(status) && blank)
+		props.power = FB_BLANK_POWERDOWN;
+
+	acpi_get_handle(NULL, "\\_SB.FEXT.SBLC", &fujitsu_bl->blank_handle);
 
 	bd = backlight_device_register(KBUILD_MODNAME, NULL, NULL,
 				       &fujitsu_bl_ops, &props);
@@ -1272,14 +1278,6 @@ static int __init fujitsu_init(void)
 	if (ret)
 		goto fail_laptop1;
 
-	/* Sync backlight power status (needs FUJ02E3 device, hence deferred) */
-	if (acpi_video_get_backlight_type() == acpi_backlight_vendor) {
-		if (call_fext_func(FUNC_BACKLIGHT, 0x2, 0x4, 0x0) == 3)
-			fujitsu_bl->bl_device->props.power = FB_BLANK_POWERDOWN;
-		else
-			fujitsu_bl->bl_device->props.power = FB_BLANK_UNBLANK;
-	}
-
 	pr_info("driver " FUJITSU_DRIVER_VERSION " successfully loaded\n");
 
 	return 0;
-- 
2.12.0

[toc] | [prev] | [next] | [standalone]

#1590088 — [PATCH v2 3/4] platform/x86: fujitsu-laptop: only register backlight device if FUJ02B1 is present

As the backlight device registered by fujitsu-laptop relies on the
FUJ02B1 ACPI device being present, only register the backlight device
once that ACPI device is detected.

Suggested-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
Signed-off-by: Michał Kępień <kernel@kempniu.pl>
---
 drivers/platform/x86/fujitsu-laptop.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index afcc451e21f6..4fc14fbcfa8a 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -787,6 +787,12 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
 		fujitsu_bl->max_brightness = FUJITSU_LCD_N_LEVELS;
 	get_lcd_level();
 
+	if (acpi_video_get_backlight_type() == acpi_backlight_vendor) {
+		error = fujitsu_backlight_register();
+		if (error)
+			goto err_unregister_input_dev;
+	}
+
 	return 0;
 
 err_unregister_input_dev:
@@ -803,6 +809,7 @@ static int acpi_fujitsu_bl_remove(struct acpi_device *device)
 	struct fujitsu_bl *fujitsu_bl = acpi_driver_data(device);
 	struct input_dev *input = fujitsu_bl->input;
 
+	backlight_device_unregister(fujitsu_bl->bl_device);
 	input_unregister_device(input);
 
 	fujitsu_bl->acpi_handle = NULL;
@@ -1254,17 +1261,9 @@ static int __init fujitsu_init(void)
 	if (ret)
 		goto fail_platform_device2;
 
-	/* Register backlight stuff */
-
-	if (acpi_video_get_backlight_type() == acpi_backlight_vendor) {
-		ret = fujitsu_backlight_register();
-		if (ret)
-			goto fail_sysfs_group;
-	}
-
 	ret = platform_driver_register(&fujitsu_pf_driver);
 	if (ret)
-		goto fail_backlight;
+		goto fail_sysfs_group;
 
 	/* Register laptop driver */
 
@@ -1286,8 +1285,6 @@ static int __init fujitsu_init(void)
 	kfree(fujitsu_laptop);
 fail_laptop:
 	platform_driver_unregister(&fujitsu_pf_driver);
-fail_backlight:
-	backlight_device_unregister(fujitsu_bl->bl_device);
 fail_sysfs_group:
 	sysfs_remove_group(&fujitsu_bl->pf_device->dev.kobj,
 			   &fujitsu_pf_attribute_group);
@@ -1311,8 +1308,6 @@ static void __exit fujitsu_cleanup(void)
 
 	platform_driver_unregister(&fujitsu_pf_driver);
 
-	backlight_device_unregister(fujitsu_bl->bl_device);
-
 	sysfs_remove_group(&fujitsu_bl->pf_device->dev.kobj,
 			   &fujitsu_pf_attribute_group);
 
-- 
2.12.0

[toc] | [prev] | [next] | [standalone]

#1590832 — Re: [PATCH v2 0/4] fujitsu_init() cleanup [resend due to vger error]

On Wed, Mar 01, 2017 at 09:10:40AM +0100, Micha?? K??pie?? wrote:
> These patches should make fujitsu_init() a bit more palatable.  No
> changes are made to platform device code yet, for clarity these will be
> posted in a separate series after this one gets applied.

Thanks for these.  As for the other series you posted I will endeavour to
review and test within the next 48 hours.  I note the particular testing
requirements of patch 2/4.

Could you confirm which git tree you've based these off?

I'm resending this for the benefit of the lists, hoping they will work this
time.  On the first attempt (at 2 Mar 2017 09:09:12 +1030) I got this:

>    ----- The following addresses had permanent fatal errors -----
> <platform-driver-x86@vger.kernel.org>
> <linux-kernel@vger.kernel.org>
> 
>    ----- Transcript of session follows -----
> ... while talking to vger.kernel.org.:
> >>> MAIL From:<jwoithe@just42.net> SIZE=1474
> <<< 450 4.5.0 Interactive router timed out!
> 550 5.1.2
> <linux-kernel@vger.kernel.org>,<platform-driver-x86@vger.kernel.org>... Host
> unknown (Name server: bl-ckh-le.kernel.org.: host not found)

Regards
  jonathan

[toc] | [prev] | [next] | [standalone]

#1590910

> On Wed, Mar 01, 2017 at 09:10:40AM +0100, Micha?? K??pie?? wrote:
> > These patches should make fujitsu_init() a bit more palatable.  No
> > changes are made to platform device code yet, for clarity these will be
> > posted in a separate series after this one gets applied.
> 
> Thanks for these.  As for the other series you posted I will endeavour to
> review and test within the next 48 hours.  I note the particular testing
> requirements of patch 2/4.
> 
> Could you confirm which git tree you've based these off?

Sure, both series posted yesterday are based on dvhart/testing as that
is where Alan's reworked series is currently applied.  I posted them as
separate series for consistency with previous submissions and also
because they are independent, i.e. can be applied in any order.

-- 
Best regards,
Michał Kępień

[toc] | [prev] | [next] | [standalone]

#1591916

On Thu, Mar 02, 2017 at 08:19:38AM +0100, Micha?? K??pie?? wrote:
> > On Wed, Mar 01, 2017 at 09:10:40AM +0100, Micha?? K??pie?? wrote:
> > > These patches should make fujitsu_init() a bit more palatable.  No
> > > changes are made to platform device code yet, for clarity these will be
> > > posted in a separate series after this one gets applied.
> > 
> > Thanks for these.  As for the other series you posted I will endeavour to
> > review and test within the next 48 hours.  I note the particular testing
> > requirements of patch 2/4.
> > 
> > Could you confirm which git tree you've based these off?
> 
> Sure, both series posted yesterday are based on dvhart/testing as that
> is where Alan's reworked series is currently applied.

Great, thanks.

Unfortunately I got held up with some other urgent tasks towards the end of
this week so I will not make my estimate of 48 hours.  I should be able to
get to the review and testing over the weekend.  Apologies for the delay.

Regards
  jonathan

[toc] | [prev] | [next] | [standalone]

#1592378

Hi Michael

On Wed, Mar 01, 2017 at 09:10:40AM +0100, Micha?? K??pie?? wrote:
> These patches should make fujitsu_init() a bit more palatable.  No
> changes are made to platform device code yet, for clarity these will be
> posted in a separate series after this one gets applied.

I have a preliminary report.  The backlight functionality remains functional
on an S7020 across all four of the patches in this series and with the
additional 2-patch cleanup series applied.

With regard to patch 2/4 you wrote:
> Jonathan, this *really* needs testing on relevant hardware.  After
> applying this patch, you should be able to turn LCD backlight on and off
> using /sys/class/backlight/fujitsu-laptop/bl_power.  Also, the value
> returned by that attribute upon read should be in sync with actual
> backlight state even right after loading the module (i.e. before writing
> anything to bl_power).  Please let me know if any of the above is not
> true and the module works correctly without this patch applied.

With patch 2/4 applied:

 * It is possible to read bl_power

 * It is possible to write a value to bl_power and read that value back

 * Writing values to bl_power does not appear to affect the LCD panel in 
   any way.  That is, the backlight remains unchanged regardless of the 
   value written.

 * Behaviour is the same both under X and from the terminal.

Backing out patch 2/4 but with all others still in place, resulted in no
change in behaviour.  So while bl_power had no effect with patch 2/4 in
place, it seems that patch 2/4 is *not* the cause of this.

I shall run some more bl_power tests and complete a review of the code later
this weekend.

Regards
  jonathan

[toc] | [prev] | [next] | [standalone]

#1592865

Hi Michael

On Sat, Mar 04, 2017 at 12:17:23PM +1030, Jonathan Woithe wrote:
> On Wed, Mar 01, 2017 at 09:10:40AM +0100, Micha?? K??pie?? wrote:
> > These patches should make fujitsu_init() a bit more palatable.  No
> > changes are made to platform device code yet, for clarity these will be
> > posted in a separate series after this one gets applied.
> 
> I have a preliminary report.  The backlight functionality remains functional
> on an S7020 across all four of the patches in this series and with the
> additional 2-patch cleanup series applied.
> 
> With regard to patch 2/4 you wrote:
> > Jonathan, this *really* needs testing on relevant hardware.  After
> > applying this patch, you should be able to turn LCD backlight on and off
> > using /sys/class/backlight/fujitsu-laptop/bl_power.  Also, the value
> > returned by that attribute upon read should be in sync with actual
> > backlight state even right after loading the module (i.e. before writing
> > anything to bl_power).  Please let me know if any of the above is not
> > true and the module works correctly without this patch applied.
> 
> With patch 2/4 applied:
> 
>  * It is possible to read bl_power
> 
>  * It is possible to write a value to bl_power and read that value back
> 
>  * Writing values to bl_power does not appear to affect the LCD panel in 
>    any way.  That is, the backlight remains unchanged regardless of the 
>    value written.
> 
>  * Behaviour is the same both under X and from the terminal.
> 
> Backing out patch 2/4 but with all others still in place, resulted in no
> change in behaviour.  So while bl_power had no effect with patch 2/4 in
> place, it seems that patch 2/4 is *not* the cause of this.
> 
> I shall run some more bl_power tests and complete a review of the code later
> this weekend.

I have completed a review of the code in this patch series (patches 1-4 of
4) and can find no obvious problems.  There do not appear to be any
regressions introduced by this patch series.  As noted, patch 2/4 does not
provide working backlight power control on an S7020 but it may well be that
this has never been functional on the S7020 (I do not make use of bl_power
myself).

I can add that immediately after loading the driver the value returned by a
read of bl_power is 0.  As noted above, setting to 1 makes no difference to
the backlight, neither does returning it to 0.  A value of 0 would normally
indicate that it's on I think, which means that the initial read of the
backlight power state does not appear to be working either.  As for the
other behaviour, this does not change if patch 2/4 is omitted.

Unfortunately I ran out of time over the weekend to cross check the
behaviour of bl_power on the S7020 with an unpatched kernel (as mentioned, I
don't utilise bl_power routinely myself and therefore can't recall whether
it has worked on my hardware in the past).  For completeness I will try to
look at this sometime this week.  However, given the patch content and the
observation that omitting patch 2/4 makes no difference to the S7020
behaviour I am satisfied that at least on S7020 this patch series does not
introduce any regressions and represents a worthwhile clean up of the
driver's code.

I am happy to see this series applied in its entirety (including patch 2/4).

Tested-by: Jonathan Woithe <jwoithe@just42.net>
Reviewed-by: Jonathan Woithe <jwoithe@just42.net>

Regards
  jonathan

[toc] | [prev] | [next] | [standalone]

#1592930

Hi Jonathan,

Thanks for testing this series.

> Hi Michael
> 
> On Sat, Mar 04, 2017 at 12:17:23PM +1030, Jonathan Woithe wrote:
> > On Wed, Mar 01, 2017 at 09:10:40AM +0100, Micha?? K??pie?? wrote:
> > > These patches should make fujitsu_init() a bit more palatable.  No
> > > changes are made to platform device code yet, for clarity these will be
> > > posted in a separate series after this one gets applied.
> > 
> > I have a preliminary report.  The backlight functionality remains functional
> > on an S7020 across all four of the patches in this series and with the
> > additional 2-patch cleanup series applied.
> > 
> > With regard to patch 2/4 you wrote:
> > > Jonathan, this *really* needs testing on relevant hardware.  After
> > > applying this patch, you should be able to turn LCD backlight on and off
> > > using /sys/class/backlight/fujitsu-laptop/bl_power.  Also, the value
> > > returned by that attribute upon read should be in sync with actual
> > > backlight state even right after loading the module (i.e. before writing
> > > anything to bl_power).  Please let me know if any of the above is not
> > > true and the module works correctly without this patch applied.
> > 
> > With patch 2/4 applied:
> > 
> >  * It is possible to read bl_power
> > 
> >  * It is possible to write a value to bl_power and read that value back
> > 
> >  * Writing values to bl_power does not appear to affect the LCD panel in 
> >    any way.  That is, the backlight remains unchanged regardless of the 
> >    value written.
> > 
> >  * Behaviour is the same both under X and from the terminal.
> > 
> > Backing out patch 2/4 but with all others still in place, resulted in no
> > change in behaviour.  So while bl_power had no effect with patch 2/4 in
> > place, it seems that patch 2/4 is *not* the cause of this.
> > 
> > I shall run some more bl_power tests and complete a review of the code later
> > this weekend.
> 
> I have completed a review of the code in this patch series (patches 1-4 of
> 4) and can find no obvious problems.  There do not appear to be any
> regressions introduced by this patch series.  As noted, patch 2/4 does not
> provide working backlight power control on an S7020 but it may well be that
> this has never been functional on the S7020 (I do not make use of bl_power
> myself).
> 
> I can add that immediately after loading the driver the value returned by a
> read of bl_power is 0.  As noted above, setting to 1 makes no difference to
> the backlight, neither does returning it to 0.

Have you tried setting bl_power to 4?  Because that is the value of
FB_BLANK_POWERDOWN, which is the value the patch is supposed to handle.

> A value of 0 would normally
> indicate that it's on I think,

Yes, I believe so too as 0 corresponds to FB_BLANK_UNBLANK.

> which means that the initial read of the
> backlight power state does not appear to be working either.

So I assume you have some kind of external display connected and the LCD
backlight is off, correct?  Just curious at this point.

> As for the
> other behaviour, this does not change if patch 2/4 is omitted.

Commit 3a407086090b ("fujitsu-laptop: Add BL power, LED control and
radio state information") which introduced backlight control mentions it
was "tested on the S6420, P8010 & U810 platforms".  Not sure if
backlight control was tested on all these models and S7020 is not listed
here, though I still find it puzzling that it did not work in the first
place, i.e. without this series applied.  This patch emerged from
reading the DSDT table of a S7020, so I would expect backlight control
to at least work properly through the "officially exposed" interface,
i.e. FEXT.

> Unfortunately I ran out of time over the weekend to cross check the
> behaviour of bl_power on the S7020 with an unpatched kernel (as mentioned, I
> don't utilise bl_power routinely myself and therefore can't recall whether
> it has worked on my hardware in the past).  For completeness I will try to
> look at this sometime this week.  However, given the patch content and the
> observation that omitting patch 2/4 makes no difference to the S7020
> behaviour I am satisfied that at least on S7020 this patch series does not
> introduce any regressions and represents a worthwhile clean up of the
> driver's code.

I would be happy to hear from someone for whom bl_power works as
expected, though we really should not leave that backlight sync code
where it currently is, so I am happy this is the conclusion you came to.

> 
> I am happy to see this series applied in its entirety (including patch 2/4).
> 
> Tested-by: Jonathan Woithe <jwoithe@just42.net>
> Reviewed-by: Jonathan Woithe <jwoithe@just42.net>

Thanks,

-- 
Best regards,
Michał Kępień

[toc] | [prev] | [next] | [standalone]

#1592934

Hi Michael

On Mon, Mar 06, 2017 at 05:49:05AM +0100, Micha?? K??pie?? wrote:
> > > With regard to patch 2/4 you wrote:
> > > > Jonathan, this *really* needs testing on relevant hardware.  After
> > > > applying this patch, you should be able to turn LCD backlight on and off
> > > > using /sys/class/backlight/fujitsu-laptop/bl_power.  Also, the value
> > > > returned by that attribute upon read should be in sync with actual
> > > > backlight state even right after loading the module (i.e. before writing
> > > > anything to bl_power).  Please let me know if any of the above is not
> > > > true and the module works correctly without this patch applied.
> > > 
> > > With patch 2/4 applied:
> > > 
> > >  * It is possible to read bl_power
> > > 
> > >  * It is possible to write a value to bl_power and read that value back
> > > 
> > >  * Writing values to bl_power does not appear to affect the LCD panel in 
> > >    any way.  That is, the backlight remains unchanged regardless of the 
> > >    value written.
> > > 
> > >  * Behaviour is the same both under X and from the terminal.
> > > 
> > > Backing out patch 2/4 but with all others still in place, resulted in no
> > > change in behaviour.  So while bl_power had no effect with patch 2/4 in
> > > place, it seems that patch 2/4 is *not* the cause of this.
> > > 
> > > I shall run some more bl_power tests and complete a review of the code later
> > > this weekend.
> > 
> > I have completed a review of the code in this patch series (patches 1-4 of
> > 4) and can find no obvious problems.  There do not appear to be any
> > regressions introduced by this patch series.  As noted, patch 2/4 does not
> > provide working backlight power control on an S7020 but it may well be that
> > this has never been functional on the S7020 (I do not make use of bl_power
> > myself).
> > 
> > I can add that immediately after loading the driver the value returned by a
> > read of bl_power is 0.  As noted above, setting to 1 makes no difference to
> > the backlight, neither does returning it to 0.
> 
> Have you tried setting bl_power to 4?  Because that is the value of
> FB_BLANK_POWERDOWN, which is the value the patch is supposed to handle.

Oh no, I didn't try 4.  I should have.  I will try to squeeze in a test of
this tonight (time is short but the test won't take a lot of time).

> > A value of 0 would normally indicate that it's on I think,
> 
> Yes, I believe so too as 0 corresponds to FB_BLANK_UNBLANK.
> 
> > which means that the initial read of the
> > backlight power state does not appear to be working either.
> 
> So I assume you have some kind of external display connected and the LCD
> backlight is off, correct?  Just curious at this point.

No, I got myself horribly confused when I wrote that second bit (I'll blame
a lack of sleep).  Ignore it.  FYI all tests have been done without an
external monitor connected.

> > As for the
> > other behaviour, this does not change if patch 2/4 is omitted.
> 
> Commit 3a407086090b ("fujitsu-laptop: Add BL power, LED control and
> radio state information") which introduced backlight control mentions it
> was "tested on the S6420, P8010 & U810 platforms".  Not sure if
> backlight control was tested on all these models 

I vaguely recall that the person who contributed this commit did have access
to those three models, but I could be mistaken.

> and S7020 is not listed here,

I guess that's because the contributor didn't have an S7020 and therefore it
didn't appear in their commit message.  I can't recall whether I tested it
on an S7020 at the time; if I did perhaps I didn't see the point in
modifying the original commit message.  In retrospect that might have been
an error on my part.

> though I still find it puzzling that it did not work in the first
> place, i.e. without this series applied.  This patch emerged from
> reading the DSDT table of a S7020, so I would expect backlight control
> to at least work properly through the "officially exposed" interface,
> i.e. FEXT.

Let me try a value of 4 and see if that works.  As I said, I haven't ever
routinely used bl_power so at this stage I can't say for sure whether it's
worked or not.  Besides, I was stupidly using the wrong value when testing
it over the weekend (1 instead of 4) so this could all be a moot point.

> > Unfortunately I ran out of time over the weekend to cross check the
> > behaviour of bl_power on the S7020 with an unpatched kernel (as mentioned, I
> > don't utilise bl_power routinely myself and therefore can't recall whether
> > it has worked on my hardware in the past).  For completeness I will try to
> > look at this sometime this week.  However, given the patch content and the
> > observation that omitting patch 2/4 makes no difference to the S7020
> > behaviour I am satisfied that at least on S7020 this patch series does not
> > introduce any regressions and represents a worthwhile clean up of the
> > driver's code.
> 
> I would be happy to hear from someone for whom bl_power works as
> expected, though we really should not leave that backlight sync code
> where it currently is, so I am happy this is the conclusion you came to.

Indeed, the more testing the better.  I'll respond in a few hours with the
outcome of a test with "bl_power" set to 4.  Regardless of the outcome I
don't believe this issue should hold up the patch series for previously
stated reasons.

Regards
  jonathan

[toc] | [prev] | [next] | [standalone]

#1592996

Hi Michael

Some quick feedback.

On Mon, Mar 06, 2017 at 03:31:04PM +1030, Jonathan Woithe wrote:
> > > I can add that immediately after loading the driver the value returned by a
> > > read of bl_power is 0.  As noted above, setting to 1 makes no difference to
> > > the backlight, neither does returning it to 0.
> > 
> > Have you tried setting bl_power to 4?  Because that is the value of
> > FB_BLANK_POWERDOWN, which is the value the patch is supposed to handle.
> 
> Oh no, I didn't try 4.  I should have.  I will try to squeeze in a test of
> this tonight (time is short but the test won't take a lot of time).

With an unpatched 4.5 kernel, writing 4 (as opposed to 1, which I stupidly
tried earlier) to bl_power caused the backlight to turn off.  Writing 0
turned it back on again.

With patches 1-4/4 applied, writing 4 to bl_power did *NOT* turn the
backlight off.

With patch 2 reverted, writing 4 to bl_power turned the backlight off. 
Writing 0 to bl_power turned it back on again.

This means that patch 2/4 seems to prevent bl_power from operating as
expected on the S7020 hardware.  Without this patch (but with all the others
in place) bl_power works.

I am unlikely to have any more time to investigate this further tonight. 

In light of the above findings, what would you like to do?

Regards
  jonathan

[toc] | [prev] | [next] | [standalone]

#1593094

[Multipart message — attachments visible in raw view] — view raw

> Hi Michael
> 
> Some quick feedback.
> 
> On Mon, Mar 06, 2017 at 03:31:04PM +1030, Jonathan Woithe wrote:
> > > > I can add that immediately after loading the driver the value returned by a
> > > > read of bl_power is 0.  As noted above, setting to 1 makes no difference to
> > > > the backlight, neither does returning it to 0.
> > > 
> > > Have you tried setting bl_power to 4?  Because that is the value of
> > > FB_BLANK_POWERDOWN, which is the value the patch is supposed to handle.
> > 
> > Oh no, I didn't try 4.  I should have.  I will try to squeeze in a test of
> > this tonight (time is short but the test won't take a lot of time).
> 
> With an unpatched 4.5 kernel, writing 4 (as opposed to 1, which I stupidly
> tried earlier) to bl_power caused the backlight to turn off.  Writing 0
> turned it back on again.
> 
> With patches 1-4/4 applied, writing 4 to bl_power did *NOT* turn the
> backlight off.
> 
> With patch 2 reverted, writing 4 to bl_power turned the backlight off. 
> Writing 0 to bl_power turned it back on again.
> 
> This means that patch 2/4 seems to prevent bl_power from operating as
> expected on the S7020 hardware.  Without this patch (but with all the others
> in place) bl_power works.
> 
> I am unlikely to have any more time to investigate this further tonight. 
> 
> In light of the above findings, what would you like to do?

Thanks for testing, good that we caught this before the patch series was
applied.  I think it is reasonable to skip applying this version of the
series as at least patch 2/4 is faulty and breaks a working feature.

Moving on, though, as I do not have access to Fujitsu hardware on which
this feature works, I was hoping you could help me verify whether my
assumptions were reasonable in the first place.  

I attached a crude patch to this message.  I would like to understand
how the underlying ACPI variables behave when the FEXT interface is
used, so please apply this patch on top of dvhart/testing (i.e. without
this series applied).  After compiling, please load the module with
debugging enabled, then test backlight control once again by writing 4
and then 0 to bl_power (this should work).  Then please send me all the
messages spit out by the driver into dmesg.  This should shed some light
on the matter.

Thanks!

-- 
Best regards,
Michał Kępień

[toc] | [prev] | [next] | [standalone]

#1593590

> > Hi Michael
> > 
> > Some quick feedback.
> > 
> > On Mon, Mar 06, 2017 at 03:31:04PM +1030, Jonathan Woithe wrote:
> > > > > I can add that immediately after loading the driver the value returned by a
> > > > > read of bl_power is 0.  As noted above, setting to 1 makes no difference to
> > > > > the backlight, neither does returning it to 0.
> > > > 
> > > > Have you tried setting bl_power to 4?  Because that is the value of
> > > > FB_BLANK_POWERDOWN, which is the value the patch is supposed to handle.
> > > 
> > > Oh no, I didn't try 4.  I should have.  I will try to squeeze in a test of
> > > this tonight (time is short but the test won't take a lot of time).
> > 
> > With an unpatched 4.5 kernel, writing 4 (as opposed to 1, which I stupidly
> > tried earlier) to bl_power caused the backlight to turn off.  Writing 0
> > turned it back on again.
> > 
> > With patches 1-4/4 applied, writing 4 to bl_power did *NOT* turn the
> > backlight off.
> > 
> > With patch 2 reverted, writing 4 to bl_power turned the backlight off. 
> > Writing 0 to bl_power turned it back on again.
> > 
> > This means that patch 2/4 seems to prevent bl_power from operating as
> > expected on the S7020 hardware.  Without this patch (but with all the others
> > in place) bl_power works.
> > 
> > I am unlikely to have any more time to investigate this further tonight. 
> > 
> > In light of the above findings, what would you like to do?
> 
> Thanks for testing, good that we caught this before the patch series was
> applied.  I think it is reasonable to skip applying this version of the
> series as at least patch 2/4 is faulty and breaks a working feature.
> 
> Moving on, though, as I do not have access to Fujitsu hardware on which
> this feature works, I was hoping you could help me verify whether my
> assumptions were reasonable in the first place.  
> 
> I attached a crude patch to this message.  I would like to understand
> how the underlying ACPI variables behave when the FEXT interface is
> used, so please apply this patch on top of dvhart/testing (i.e. without
> this series applied).  After compiling, please load the module with
> debugging enabled, then test backlight control once again by writing 4
> and then 0 to bl_power (this should work).  Then please send me all the
> messages spit out by the driver into dmesg.  This should shed some light
> on the matter.

Actually, scratch that.  I just ordered a banged up S7020 for €15 to
avoid pestering you with experimental patches and hopefully make the
whole driver cleanup process a bit smoother.

Darren, Andy, please ignore this whole series for now.  I will post v3
once I figure out how to clean things up without breaking working
features.

-- 
Best regards,
Michał Kępień

[toc] | [prev] | [next] | [standalone]

#1593896

Hi Michael

On Mon, Mar 06, 2017 at 07:47:23PM +0100, Micha?? K??pie?? wrote:
> > > In light of the above findings, what would you like to do?
> > 
> > Thanks for testing, good that we caught this before the patch series was
> > applied.  I think it is reasonable to skip applying this version of the
> > series as at least patch 2/4 is faulty and breaks a working feature.
> > 
> > Moving on, though, as I do not have access to Fujitsu hardware on which
> > this feature works, I was hoping you could help me verify whether my
> > assumptions were reasonable in the first place.  
> > 
> > I attached a crude patch to this message.  I would like to understand
> > how the underlying ACPI variables behave when the FEXT interface is
> > used, so please apply this patch on top of dvhart/testing (i.e. without
> > this series applied).  After compiling, please load the module with
> > debugging enabled, then test backlight control once again by writing 4
> > and then 0 to bl_power (this should work).  Then please send me all the
> > messages spit out by the driver into dmesg.  This should shed some light
> > on the matter.

I have done this.  Writing 4 to bl_power did indeed turn the backlight off,
and 0 restored it.  Annotated output from dmesg is at the end of this
message.

> Actually, scratch that.  I just ordered a banged up S7020 for ???15 to
> avoid pestering you with experimental patches and hopefully make the
> whole driver cleanup process a bit smoother.

Ok, no problem.  Obviously I'm still happy to test as required.

> Darren, Andy, please ignore this whole series for now.  I will post v3
> once I figure out how to clean things up without breaking working
> features.

To clarify, I see no reason why the earlier 2-patch cleanup series can't go
in at this stage.  It's only this 4-part patch which needs revision in light
of recent findings.

Regards
  jonathan

Module loading

[ 3292.204366] input: Fujitsu FUJ02B1 as /devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A03:00/device:05/FUJ02B1:00/input/input19
[ 3292.208512] fujitsu_laptop: ACPI: Fujitsu FUJ02B1 [FJEX] (on)
[ 3292.209653] fujitsu_laptop: acpi_fujitsu_bl_add: auto-detected usealt as 0
[ 3292.209654] fujitsu_laptop: acpi_fujitsu_bl_add: config: [alt interface: 0], [adjust disable: 0]
[ 3292.209655] fujitsu_laptop: get_max_brightness: get max lcd level via RBLL
[ 3292.209701] fujitsu_laptop: get_lcd_level: get lcd level via GBLL
[ 3292.212086] input: Fujitsu FUJ02E3 as /devices/LNXSYSTM:00/LNXSYBUS:00/FUJ02E3:00/input/input20
[ 3292.213483] fujitsu_laptop: ACPI: Fujitsu FUJ02E3 [FEXT] (on)
[ 3292.214627] fujitsu_laptop: acpi_fujitsu_laptop_add: Invoking _INI
[ 3292.214740] fujitsu_laptop: call_fext_func: FUNC 0x1002 (args 0x1, 0x0, 0x0) returned 0x0
[ 3292.214741] fujitsu_laptop: acpi_fujitsu_laptop_add: Discarded 0 ringbuffer entries
[ 3292.214770] fujitsu_laptop: call_fext_func: FUNC 0x1000 (args 0x0, 0x0, 0x0) returned 0x80000000
[ 3292.214819] fujitsu_laptop: call_fext_func: FUNC 0x1002 (args 0x0, 0x0, 0x0) returned 0xf0001
[ 3292.214820] fujitsu_laptop: BTNI: [0xf0001]
[ 3292.216523] fujitsu_laptop: call_fext_func: FUNC 0x1001 (args 0x0, 0x0, 0x0) returned 0x0
[ 3292.216567] fujitsu_laptop: call_fext_func: FUNC 0x1001 (args 0x0, 0x0, 0x0) returned 0x0
[ 3292.216616] fujitsu_laptop: call_fext_func: FUNC 0x1002 (args 0x0, 0x0, 0x0) returned 0xf0001
[ 3292.216658] fujitsu_laptop: call_fext_func: FUNC 0x1001 (args 0x0, 0x0, 0x0) returned 0x0
[ 3292.216964] fujitsu_laptop: call_fext_func: FUNC 0x1004 (args 0x2, 0x4, 0x0) returned 0x0
[ 3292.216965] fujitsu_laptop: driver 0.6.0 successfully loaded

echo 4 > /sys/devices/virtual/backlight/fujitsu-laptop/bl_power

[ 3320.168775] fujitsu_laptop: call_fext_func: FUNC 0x1004 (args 0x1, 0x4, 0x3) returned 0x0
[ 3320.168779] fujitsu_laptop: bl_update_status: Backlight power set to 4
[ 3320.168793] fujitsu_laptop: bl_update_status: BLCT = 1
[ 3320.168800] fujitsu_laptop: bl_update_status: NGTM = 3
[ 3320.168805] fujitsu_laptop: bl_update_status: Got ACPI handle for SBLC
[ 3320.168808] fujitsu_laptop: set_lcd_level: set lcd level via SBLL [7]

echo 4 > /sys/devices/virtual/backlight/fujitsu-laptop/bl_power

[ 3322.774773] fujitsu_laptop: call_fext_func: FUNC 0x1004 (args 0x1, 0x4, 0x0) returned 0x0
[ 3322.774776] fujitsu_laptop: bl_update_status: Backlight power set to 0
[ 3322.774790] fujitsu_laptop: bl_update_status: BLCT = 0
[ 3322.774798] fujitsu_laptop: bl_update_status: NGTM = 0
[ 3322.774802] fujitsu_laptop: bl_update_status: Got ACPI handle for SBLC
[ 3322.774804] fujitsu_laptop: set_lcd_level: set lcd level via SBLL [7]

[toc] | [prev] | [next] | [standalone]

#1593969

> Hi Michael
> 
> On Mon, Mar 06, 2017 at 07:47:23PM +0100, Micha?? K??pie?? wrote:
> > > > In light of the above findings, what would you like to do?
> > > 
> > > Thanks for testing, good that we caught this before the patch series was
> > > applied.  I think it is reasonable to skip applying this version of the
> > > series as at least patch 2/4 is faulty and breaks a working feature.
> > > 
> > > Moving on, though, as I do not have access to Fujitsu hardware on which
> > > this feature works, I was hoping you could help me verify whether my
> > > assumptions were reasonable in the first place.  
> > > 
> > > I attached a crude patch to this message.  I would like to understand
> > > how the underlying ACPI variables behave when the FEXT interface is
> > > used, so please apply this patch on top of dvhart/testing (i.e. without
> > > this series applied).  After compiling, please load the module with
> > > debugging enabled, then test backlight control once again by writing 4
> > > and then 0 to bl_power (this should work).  Then please send me all the
> > > messages spit out by the driver into dmesg.  This should shed some light
> > > on the matter.
> 
> I have done this.  Writing 4 to bl_power did indeed turn the backlight off,
> and 0 restored it.  Annotated output from dmesg is at the end of this
> message.
> 
> > Actually, scratch that.  I just ordered a banged up S7020 for ???15 to
> > avoid pestering you with experimental patches and hopefully make the
> > whole driver cleanup process a bit smoother.
> 
> Ok, no problem.  Obviously I'm still happy to test as required.
> 
> > Darren, Andy, please ignore this whole series for now.  I will post v3
> > once I figure out how to clean things up without breaking working
> > features.
> 
> To clarify, I see no reason why the earlier 2-patch cleanup series can't go
> in at this stage.  It's only this 4-part patch which needs revision in light
> of recent findings.

Yes, this is what I meant.  As I mentioned earlier in this thread [1],
v2 of the 2-patch series for acpi_fujitsu_bl_notify() is independent
from this one.

[]

> 
> echo 4 > /sys/devices/virtual/backlight/fujitsu-laptop/bl_power
> 
> [ 3320.168775] fujitsu_laptop: call_fext_func: FUNC 0x1004 (args 0x1, 0x4, 0x3) returned 0x0
> [ 3320.168779] fujitsu_laptop: bl_update_status: Backlight power set to 4
> [ 3320.168793] fujitsu_laptop: bl_update_status: BLCT = 1
> [ 3320.168800] fujitsu_laptop: bl_update_status: NGTM = 3
> [ 3320.168805] fujitsu_laptop: bl_update_status: Got ACPI handle for SBLC
> [ 3320.168808] fujitsu_laptop: set_lcd_level: set lcd level via SBLL [7]
> 
> echo 4 > /sys/devices/virtual/backlight/fujitsu-laptop/bl_power
> 
> [ 3322.774773] fujitsu_laptop: call_fext_func: FUNC 0x1004 (args 0x1, 0x4, 0x0) returned 0x0
> [ 3322.774776] fujitsu_laptop: bl_update_status: Backlight power set to 0
> [ 3322.774790] fujitsu_laptop: bl_update_status: BLCT = 0
> [ 3322.774798] fujitsu_laptop: bl_update_status: NGTM = 0
> [ 3322.774802] fujitsu_laptop: bl_update_status: Got ACPI handle for SBLC
> [ 3322.774804] fujitsu_laptop: set_lcd_level: set lcd level via SBLL [7]

Okay, this is exactly what I feared.  I correctly inferred how BLCT
behaves from the DSDT dump, but it looks like it works in tandem with
NGTM, which can only be set using the FUNC interface.

This means that, to avoid code duplication, we will need to use
call_fext_func() from within the backlight driver.  If we decide to
split backlight-related code and the rest into two separate modules,
this means that the backlight module will depend on the laptop module.
This is not really bad in and of itself, but I was hoping we could do
better than that.  That being said, a part of me is also happy that we
will stick to the "official" interface instead of doing custom low-level
tricks.

Anyway, I will prepare a v3 that does not break backlight control.
Driver untangling will have to wait.

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

-- 
Best regards,
Michał Kępień

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web