[PATCH] Platform: Brightness quirk for samsung laptop driver

[PATCH] Platform: Brightness quirk for samsung laptop driver

[David Herrmann]

From: Jason Stubbs <jasonbstubbs@gmail.com>

On some samsung laptops the brightness regulation works slightly different.
All SABI commands except for set_brightness work as expected. The behaviour
of set_brightness is as follows:

- Setting a new brightness will only step one level toward the new brightness
  level. For example, setting a level of 5 when the current level is 2 will
  result in a brightness level of 3.
- A spurious KEY_BRIGHTNESS_UP or KEY_BRIGHTNESS_DOWN event is also generated
  along with the change in brightness.
- Neither of the above two issues occur when changing from/to brightness level 0.

This patch adds detection and a non-intrusive workaround for the above issues.

Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
Hi

This patch is originally from Jason Stubbs. See here:
http://git.kernel.org/?p=linux/kernel/git/gregkh/patches.git;a=blob;f=platform-samsung_laptop-add-support-for-samsung-nc210-nc110.patch;hb=HEAD

I just wanted to ask whether there is some work going on here? My laptop model
needs this fix and I reviewed and tested it and it works fine. It would be nice
to see this upstream.

I removed the NC210/NC120 device addition as it is not part of this change. This
might be added in a separate patch.

My laptop is detected as "N150P/N210P/N220P" and needs this quirk, too. I
reviewed and tested it and it works fine.

As it was signed-off earlier and released as GPL, I thought I'd just resubmit
the patch. However, its not from me so a signed-off by Jason Stubbs would be
nice again.

Regards
David

 drivers/platform/x86/samsung-laptop.c |   43 +++++++++++++++++++++++++++++++++
 1 files changed, 43 insertions(+), 0 deletions(-)

diff --git a/drivers/platform/x86/samsung-laptop.c b/drivers/platform/x86/samsung-laptop.c
index d347116..4cceb60 100644
--- a/drivers/platform/x86/samsung-laptop.c
+++ b/drivers/platform/x86/samsung-laptop.c
@@ -226,6 +226,7 @@ static struct backlight_device *backlight_device;
 static struct mutex sabi_mutex;
 static struct platform_device *sdev;
 static struct rfkill *rfk;
+static bool has_stepping_quirk;
 
 static int force;
 module_param(force, bool, 0);
@@ -380,6 +381,17 @@ static void set_brightness(u8 user_brightness)
 {
 	u8 user_level = user_brightness - sabi_config->min_brightness;
 
+	if (has_stepping_quirk && user_level != 0) {
+		/*
+		 * short circuit if the specified level is what's already set
+		 * to prevent the screen from flickering needlessly
+		 */
+		if (user_brightness == read_brightness())
+			return;
+
+		sabi_set_command(sabi_config->commands.set_brightness, 0);
+	}
+
 	sabi_set_command(sabi_config->commands.set_brightness, user_level);
 }
 
@@ -388,6 +400,34 @@ static int get_brightness(struct backlight_device *bd)
 	return (int)read_brightness();
 }
 
+static void check_for_stepping_quirk(void)
+{
+	u8 initial_level = read_brightness();
+	u8 check_level;
+
+	/*
+	 * Some laptops (nc210/nc110 at the very least) exhibit the strange
+	 * behaviour of stepping toward rather than setting the brightness
+	 * except when changing to/from brightness level 0. This behaviour
+	 * is checked for here and worked around in set_brightness.
+	 */
+
+	if (initial_level <= 2)
+		check_level = initial_level + 2;
+	else
+		check_level = initial_level - 2;
+
+	has_stepping_quirk = false;
+	set_brightness(check_level);
+
+	if (read_brightness() != check_level) {
+		has_stepping_quirk = true;
+		pr_info("enabled workaround for brightness stepping quirk\n");
+	}
+
+	set_brightness(initial_level);
+}
+
 static int update_status(struct backlight_device *bd)
 {
 	set_brightness(bd->props.brightness);
@@ -774,6 +814,9 @@ static int __init samsung_init(void)
 		}
 	}
 
+	/* Check for stepping quirk */
+	check_for_stepping_quirk();
+
 	/* knock up a platform device to hang stuff off of */
 	sdev = platform_device_register_simple("samsung", -1, NULL, 0);
 	if (IS_ERR(sdev))
-- 
1.7.6

Re: [PATCH] Platform: Brightness quirk for samsung laptop driver

[Greg KH]

On Tue, Aug 23, 2011 at 06:05:27PM +0200, David Herrmann wrote:
> From: Jason Stubbs <jasonbstubbs@gmail.com>
> 
> On some samsung laptops the brightness regulation works slightly different.
> All SABI commands except for set_brightness work as expected. The behaviour
> of set_brightness is as follows:
> 
> - Setting a new brightness will only step one level toward the new brightness
>   level. For example, setting a level of 5 when the current level is 2 will
>   result in a brightness level of 3.
> - A spurious KEY_BRIGHTNESS_UP or KEY_BRIGHTNESS_DOWN event is also generated
>   along with the change in brightness.
> - Neither of the above two issues occur when changing from/to brightness level 0.
> 
> This patch adds detection and a non-intrusive workaround for the above issues.
> 
> Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
> ---
> Hi
> 
> This patch is originally from Jason Stubbs. See here:
> http://git.kernel.org/?p=linux/kernel/git/gregkh/patches.git;a=blob;f=platform-samsung_laptop-add-support-for-samsung-nc210-nc110.patch;hb=HEAD
> 
> I just wanted to ask whether there is some work going on here? My laptop model
> needs this fix and I reviewed and tested it and it works fine. It would be nice
> to see this upstream.

I have a different patch from Jason that should fix this properly, I'll
send it to Matthew later today.

thanks,

greg k-h

Re: [PATCH] Platform: Brightness quirk for samsung laptop driver

[Jason Stubbs]

On 08/25/2011 09:10 AM, Greg KH wrote:
> On Tue, Aug 23, 2011 at 06:05:27PM +0200, David Herrmann wrote:
>> From: Jason Stubbs<jasonbstubbs@gmail.com>
>>
>> On some samsung laptops the brightness regulation works slightly different.
>> All SABI commands except for set_brightness work as expected. The behaviour
>> of set_brightness is as follows:
>>
>> - Setting a new brightness will only step one level toward the new brightness
>>    level. For example, setting a level of 5 when the current level is 2 will
>>    result in a brightness level of 3.
>> - A spurious KEY_BRIGHTNESS_UP or KEY_BRIGHTNESS_DOWN event is also generated
>>    along with the change in brightness.
>> - Neither of the above two issues occur when changing from/to brightness level 0.
>>
>> This patch adds detection and a non-intrusive workaround for the above issues.
>>
>> Signed-off-by: David Herrmann<dh.herrmann@googlemail.com>
>> ---
>> Hi
>>
>> This patch is originally from Jason Stubbs. See here:
>> http://git.kernel.org/?p=linux/kernel/git/gregkh/patches.git;a=blob;f=platform-samsung_laptop-add-support-for-samsung-nc210-nc110.patch;hb=HEAD
>>
>> I just wanted to ask whether there is some work going on here? My laptop model
>> needs this fix and I reviewed and tested it and it works fine. It would be nice
>> to see this upstream.
>
> I have a different patch from Jason that should fix this properly, I'll
> send it to Matthew later today.

If you're talking about the min/max calculations patch, that fixes a 
separate issue to this patch. Both patches touch set_brightness() 
though, which is probably where the confusion came from.

The version of the patch in git doesn't apply on top of the min/max 
calcs patch that you've added to the gregkh-2.6 tree, but I've 
previously posted a version that does. You can find it at 
https://lkml.org/lkml/2011/5/13/150 or I can post it again.

Regards,
Jason Stubbs

Re: [PATCH] Platform: Brightness quirk for samsung laptop driver

[Greg KH]

On Thu, Aug 25, 2011 at 01:27:26PM +1000, Jason Stubbs wrote:
> On 08/25/2011 09:10 AM, Greg KH wrote:
> >On Tue, Aug 23, 2011 at 06:05:27PM +0200, David Herrmann wrote:
> >>From: Jason Stubbs<jasonbstubbs@gmail.com>
> >>
> >>On some samsung laptops the brightness regulation works slightly different.
> >>All SABI commands except for set_brightness work as expected. The behaviour
> >>of set_brightness is as follows:
> >>
> >>- Setting a new brightness will only step one level toward the new brightness
> >>   level. For example, setting a level of 5 when the current level is 2 will
> >>   result in a brightness level of 3.
> >>- A spurious KEY_BRIGHTNESS_UP or KEY_BRIGHTNESS_DOWN event is also generated
> >>   along with the change in brightness.
> >>- Neither of the above two issues occur when changing from/to brightness level 0.
> >>
> >>This patch adds detection and a non-intrusive workaround for the above issues.
> >>
> >>Signed-off-by: David Herrmann<dh.herrmann@googlemail.com>
> >>---
> >>Hi
> >>
> >>This patch is originally from Jason Stubbs. See here:
> >>http://git.kernel.org/?p=linux/kernel/git/gregkh/patches.git;a=blob;f=platform-samsung_laptop-add-support-for-samsung-nc210-nc110.patch;hb=HEAD
> >>
> >>I just wanted to ask whether there is some work going on here? My laptop model
> >>needs this fix and I reviewed and tested it and it works fine. It would be nice
> >>to see this upstream.
> >
> >I have a different patch from Jason that should fix this properly, I'll
> >send it to Matthew later today.
> 
> If you're talking about the min/max calculations patch, that fixes a
> separate issue to this patch. Both patches touch set_brightness()
> though, which is probably where the confusion came from.
> 
> The version of the patch in git doesn't apply on top of the min/max
> calcs patch that you've added to the gregkh-2.6 tree, but I've
> previously posted a version that does. You can find it at
> https://lkml.org/lkml/2011/5/13/150 or I can post it again.

Emailing it to me again would be great.

thanks,

greg k-h

[PATCH] Platform: Brightness quirk for samsung laptop driver

[Jason Stubbs]

From: Jason Stubbs <jasonbstubbs@gmail.com>

On some Samsung laptops the brightness regulation works slightly different.
All SABI commands except for set_brightness work as expected. The behaviour
of set_brightness is as follows:

- Setting a new brightness will only step one level toward the new brightness
  level. For example, setting a level of 5 when the current level is 2 will
  result in a brightness level of 3.
- A spurious KEY_BRIGHTNESS_UP or KEY_BRIGHTNESS_DOWN event is also generated
  along with the change in brightness.
- Neither of the above two issues occur when changing from/to brightness
  level 0.

This patch adds detection and a non-intrusive workaround for the above issues.

Signed-off-by: Jason Stubbs <jasonbstubbs@gmail.com>

---

diff --git a/drivers/platform/x86/samsung-laptop.c b/drivers/platform/x86/samsung-laptop.c
index ee68c1c..b193bf0 100644
--- a/drivers/platform/x86/samsung-laptop.c
+++ b/drivers/platform/x86/samsung-laptop.c
@@ -226,6 +226,7 @@ static struct backlight_device *backlight_device;
 static struct mutex sabi_mutex;
 static struct platform_device *sdev;
 static struct rfkill *rfk;
+static bool has_stepping_quirk;
 
 static int force;
 module_param(force, bool, 0);
@@ -382,6 +383,17 @@ static void set_brightness(u8 user_brightness)
 {
 	u8 user_level = user_brightness + sabi_config->min_brightness;
 
+	if (has_stepping_quirk && user_level != 0) {
+		/*
+		 * short circuit if the specified level is what's already set
+		 * to prevent the screen from flickering needlessly
+		 */
+		if (user_brightness == read_brightness())
+			return;
+
+		sabi_set_command(sabi_config->commands.set_brightness, 0);
+	}
+
 	sabi_set_command(sabi_config->commands.set_brightness, user_level);
 }
 
@@ -390,6 +402,34 @@ static int get_brightness(struct backlight_device *bd)
 	return (int)read_brightness();
 }
 
+static void check_for_stepping_quirk(void)
+{
+	u8 initial_level = read_brightness();
+	u8 check_level;
+
+	/*
+	 * Some laptops exhibit the strange behaviour of stepping toward
+	 * (rather than setting) the brightness except when changing to/from
+	 * brightness level 0. This behaviour is checked for here and worked
+	 * around in set_brightness.
+	 */
+
+	if (initial_level <= 2)
+		check_level = initial_level + 2;
+	else
+		check_level = initial_level - 2;
+
+	has_stepping_quirk = false;
+	set_brightness(check_level);
+
+	if (read_brightness() != check_level) {
+		has_stepping_quirk = true;
+		pr_info("enabled workaround for brightness stepping quirk\n");
+	}
+
+	set_brightness(initial_level);
+}
+
 static int update_status(struct backlight_device *bd)
 {
 	set_brightness(bd->props.brightness);
@@ -796,6 +836,9 @@ static int __init samsung_init(void)
 		}
 	}
 
+	/* Check for stepping quirk */
+	check_for_stepping_quirk();
+
 	/* knock up a platform device to hang stuff off of */
 	sdev = platform_device_register_simple("samsung", -1, NULL, 0);
 	if (IS_ERR(sdev))

Re: [PATCH] Platform: Brightness quirk for samsung laptop driver

[David Herrmann]

On Fri, Aug 26, 2011 at 11:58 AM, Jason Stubbs <jasonbstubbs@gmail.com> wrote:
> From: Jason Stubbs <jasonbstubbs@gmail.com>
>
> On some Samsung laptops the brightness regulation works slightly different.
> All SABI commands except for set_brightness work as expected. The behaviour
> of set_brightness is as follows:
>
> - Setting a new brightness will only step one level toward the new brightness
>  level. For example, setting a level of 5 when the current level is 2 will
>  result in a brightness level of 3.
> - A spurious KEY_BRIGHTNESS_UP or KEY_BRIGHTNESS_DOWN event is also generated
>  along with the change in brightness.
> - Neither of the above two issues occur when changing from/to brightness
>  level 0.
>
> This patch adds detection and a non-intrusive workaround for the above issues.
>
> Signed-off-by: Jason Stubbs <jasonbstubbs@gmail.com>
>
> ---
>
> diff --git a/drivers/platform/x86/samsung-laptop.c b/drivers/platform/x86/samsung-laptop.c
> index ee68c1c..b193bf0 100644
> --- a/drivers/platform/x86/samsung-laptop.c
> +++ b/drivers/platform/x86/samsung-laptop.c
> @@ -226,6 +226,7 @@ static struct backlight_device *backlight_device;
>  static struct mutex sabi_mutex;
>  static struct platform_device *sdev;
>  static struct rfkill *rfk;
> +static bool has_stepping_quirk;
>
>  static int force;
>  module_param(force, bool, 0);
> @@ -382,6 +383,17 @@ static void set_brightness(u8 user_brightness)
>  {
>        u8 user_level = user_brightness + sabi_config->min_brightness;
>
> +       if (has_stepping_quirk && user_level != 0) {
> +               /*
> +                * short circuit if the specified level is what's already set
> +                * to prevent the screen from flickering needlessly
> +                */
> +               if (user_brightness == read_brightness())
> +                       return;
> +
> +               sabi_set_command(sabi_config->commands.set_brightness, 0);
> +       }
> +
>        sabi_set_command(sabi_config->commands.set_brightness, user_level);
>  }
>
> @@ -390,6 +402,34 @@ static int get_brightness(struct backlight_device *bd)
>        return (int)read_brightness();
>  }
>
> +static void check_for_stepping_quirk(void)
> +{
> +       u8 initial_level = read_brightness();
> +       u8 check_level;
> +
> +       /*
> +        * Some laptops exhibit the strange behaviour of stepping toward
> +        * (rather than setting) the brightness except when changing to/from
> +        * brightness level 0. This behaviour is checked for here and worked
> +        * around in set_brightness.
> +        */
> +
> +       if (initial_level <= 2)
> +               check_level = initial_level + 2;
> +       else
> +               check_level = initial_level - 2;
> +
> +       has_stepping_quirk = false;
> +       set_brightness(check_level);
> +
> +       if (read_brightness() != check_level) {
> +               has_stepping_quirk = true;
> +               pr_info("enabled workaround for brightness stepping quirk\n");
> +       }
> +
> +       set_brightness(initial_level);
> +}
> +
>  static int update_status(struct backlight_device *bd)
>  {
>        set_brightness(bd->props.brightness);
> @@ -796,6 +836,9 @@ static int __init samsung_init(void)
>                }
>        }
>
> +       /* Check for stepping quirk */
> +       check_for_stepping_quirk();
> +
>        /* knock up a platform device to hang stuff off of */
>        sdev = platform_device_register_simple("samsung", -1, NULL, 0);
>        if (IS_ERR(sdev))
>
>

Thank you, Jason.
Tested-by: David Herrmann <dh.herrmann@googlemail.com>

And Greg, could you actually "git add" the new samsung patches in your
repository? You added them to "series" but didn't submit the actual
patch files. That would be great.

Thank you and cheers
David

Re: [PATCH] Platform: Brightness quirk for samsung laptop driver

[Greg KH]

On Fri, Aug 26, 2011 at 01:31:38PM +0200, David Herrmann wrote:
> On Fri, Aug 26, 2011 at 11:58 AM, Jason Stubbs <jasonbstubbs@gmail.com> wrote:
> > From: Jason Stubbs <jasonbstubbs@gmail.com>
> >
> > On some Samsung laptops the brightness regulation works slightly different.
> > All SABI commands except for set_brightness work as expected. The behaviour
> > of set_brightness is as follows:
> >
> > - Setting a new brightness will only step one level toward the new brightness
> >  level. For example, setting a level of 5 when the current level is 2 will
> >  result in a brightness level of 3.
> > - A spurious KEY_BRIGHTNESS_UP or KEY_BRIGHTNESS_DOWN event is also generated
> >  along with the change in brightness.
> > - Neither of the above two issues occur when changing from/to brightness
> >  level 0.
> >
> > This patch adds detection and a non-intrusive workaround for the above issues.
> >
> > Signed-off-by: Jason Stubbs <jasonbstubbs@gmail.com>
> >
> > ---
> >
> > diff --git a/drivers/platform/x86/samsung-laptop.c b/drivers/platform/x86/samsung-laptop.c
> > index ee68c1c..b193bf0 100644
> > --- a/drivers/platform/x86/samsung-laptop.c
> > +++ b/drivers/platform/x86/samsung-laptop.c
> > @@ -226,6 +226,7 @@ static struct backlight_device *backlight_device;
> >  static struct mutex sabi_mutex;
> >  static struct platform_device *sdev;
> >  static struct rfkill *rfk;
> > +static bool has_stepping_quirk;
> >
> >  static int force;
> >  module_param(force, bool, 0);
> > @@ -382,6 +383,17 @@ static void set_brightness(u8 user_brightness)
> >  {
> >        u8 user_level = user_brightness + sabi_config->min_brightness;
> >
> > +       if (has_stepping_quirk && user_level != 0) {
> > +               /*
> > +                * short circuit if the specified level is what's already set
> > +                * to prevent the screen from flickering needlessly
> > +                */
> > +               if (user_brightness == read_brightness())
> > +                       return;
> > +
> > +               sabi_set_command(sabi_config->commands.set_brightness, 0);
> > +       }
> > +
> >        sabi_set_command(sabi_config->commands.set_brightness, user_level);
> >  }
> >
> > @@ -390,6 +402,34 @@ static int get_brightness(struct backlight_device *bd)
> >        return (int)read_brightness();
> >  }
> >
> > +static void check_for_stepping_quirk(void)
> > +{
> > +       u8 initial_level = read_brightness();
> > +       u8 check_level;
> > +
> > +       /*
> > +        * Some laptops exhibit the strange behaviour of stepping toward
> > +        * (rather than setting) the brightness except when changing to/from
> > +        * brightness level 0. This behaviour is checked for here and worked
> > +        * around in set_brightness.
> > +        */
> > +
> > +       if (initial_level <= 2)
> > +               check_level = initial_level + 2;
> > +       else
> > +               check_level = initial_level - 2;
> > +
> > +       has_stepping_quirk = false;
> > +       set_brightness(check_level);
> > +
> > +       if (read_brightness() != check_level) {
> > +               has_stepping_quirk = true;
> > +               pr_info("enabled workaround for brightness stepping quirk\n");
> > +       }
> > +
> > +       set_brightness(initial_level);
> > +}
> > +
> >  static int update_status(struct backlight_device *bd)
> >  {
> >        set_brightness(bd->props.brightness);
> > @@ -796,6 +836,9 @@ static int __init samsung_init(void)
> >                }
> >        }
> >
> > +       /* Check for stepping quirk */
> > +       check_for_stepping_quirk();
> > +
> >        /* knock up a platform device to hang stuff off of */
> >        sdev = platform_device_register_simple("samsung", -1, NULL, 0);
> >        if (IS_ERR(sdev))
> >
> >
> 
> Thank you, Jason.
> Tested-by: David Herrmann <dh.herrmann@googlemail.com>
> 
> And Greg, could you actually "git add" the new samsung patches in your
> repository? You added them to "series" but didn't submit the actual
> patch files. That would be great.

Now done, sorry about that.

greg k-h

[PATCH] Platform: Samsung laptop DMI info for NC210/NC110

[Jason Stubbs]

From: Jason Stubbs <jasonbstubbs@gmail.com>

This patch just adds the DMI info for the samsung laptop driver to work with
the NC210/NC110. It needs the brightness quirk patch for proper support.

Signed-off-by: Jason Stubbs <jasonbstubbs@gmail.com>

---

diff --git a/drivers/platform/x86/samsung-laptop.c b/drivers/platform/x86/samsung-laptop.c
index b193bf0..bae1a97 100644
--- a/drivers/platform/x86/samsung-laptop.c
+++ b/drivers/platform/x86/samsung-laptop.c
@@ -728,6 +728,15 @@ static struct dmi_system_id __initdata samsung_dmi_table[] = {
 		},
 		.callback = dmi_check_cb,
 	},
+	{
+		.ident = "NC210/NC110",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "SAMSUNG ELECTRONICS CO., LTD."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "NC210/NC110"),
+			DMI_MATCH(DMI_BOARD_NAME, "NC210/NC110"),
+		},
+		.callback = dmi_check_cb,
+	},
 	{ },
 };
 MODULE_DEVICE_TABLE(dmi, samsung_dmi_table);