[PATCH] platform: add support for samsung nc210/nc110

[PATCH] platform: add support for samsung nc210/nc110

[Jason Stubbs]

From: Jason Stubbs <jasonbstubbs@gmail.com>

Note, I don't expect this patch to go in and would actually just like some
feedback on how to better handle the set_brightness problem. Note also that
this patch won't apply on top of my brightness min/max fixes patch and I'll
redo this one as necessary.

This patch adds support for the NC210/NC110 to the samsung-laptop driver.

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.

The changes in set_brightness() work around the above while attempting to
minimize flicker due to jumping to and from brightness level 0.

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 d347116..01d5278 100644
--- a/drivers/platform/x86/samsung-laptop.c
+++ b/drivers/platform/x86/samsung-laptop.c
@@ -380,6 +380,14 @@ static void set_brightness(u8 user_brightness)
 {
 	u8 user_level = user_brightness - sabi_config->min_brightness;
 
+	if (user_level == read_brightness()) {
+		return;
+	}
+
+	if (sabi_config->min_brightness == 0 && user_level != 0) {
+		sabi_set_command(sabi_config->commands.set_brightness, 0);
+	}
+
 	sabi_set_command(sabi_config->commands.set_brightness, user_level);
 }
 
@@ -666,6 +674,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);

[PATCH] platform: fix samsung brightness min/max calculations

[Jason Stubbs]

From: Jason Stubbs <jasonbstubbs@gmail.com>

For one of the SABI configs, the valid brightness levels are 1 through 8 with
0 being reserved for the BIOS. To make the range 0-based, the driver is meant
to offset values to/from userspace by 1 giving valid levels of 0 through 7.

This patch fixes the reporting of the maximum level and the interpretation of
the value passed from userspace as per the above logic. It also fixes the
assumption that min_brightness will be either 0 or 1.

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 d347116..ebf1b07 100644
--- a/drivers/platform/x86/samsung-laptop.c
+++ b/drivers/platform/x86/samsung-laptop.c
@@ -370,15 +370,17 @@ static u8 read_brightness(void)
 				  &sretval);
 	if (!retval) {
 		user_brightness = sretval.retval[0];
-		if (user_brightness != 0)
+		if (user_brightness > sabi_config->min_brightness)
 			user_brightness -= sabi_config->min_brightness;
+		else
+			user_brightness = 0;
 	}
 	return user_brightness;
 }
 
 static void set_brightness(u8 user_brightness)
 {
-	u8 user_level = user_brightness - sabi_config->min_brightness;
+	u8 user_level = user_brightness + sabi_config->min_brightness;
 
 	sabi_set_command(sabi_config->commands.set_brightness, user_level);
 }
@@ -782,7 +784,8 @@ static int __init samsung_init(void)
 	/* create a backlight device to talk to this one */
 	memset(&props, 0, sizeof(struct backlight_properties));
 	props.type = BACKLIGHT_PLATFORM;
-	props.max_brightness = sabi_config->max_brightness;
+	props.max_brightness = sabi_config->max_brightness -
+				sabi_config->min_brightness;
 	backlight_device = backlight_device_register("samsung", &sdev->dev,
 						     NULL, &backlight_ops,
 						     &props);

Re: [PATCH] platform: fix samsung brightness min/max calculations

[Greg KH]

On Thu, May 12, 2011 at 12:13:59PM +1000, Jason Stubbs wrote:
> On Wed, 11 May 2011 23:51:14 Greg KH wrote:
> > On Wed, May 11, 2011 at 02:47:49PM +1000, Jason Stubbs wrote:
> > > On Wed, 11 May 2011 08:39:08 Greg KH wrote:
> > > > On Thu, Apr 28, 2011 at 06:55:50PM +1000, Jason Stubbs wrote:
> > > > > Not sure of the "bump" process, so I'll just go over each part of the
> > > > > patch.
> > > > > 
> > > > > Note that all the changes only affect the sabi_config where
> > > > > min_brightness is 1 so you might not see in difference on your
> > > > > hardware.
> > > > 
> > > > This isn't needed anymore due to your other patch superseeding it,
> > > > right?
> > > 
> > > Actually, this one is irrelevant to the nc210/nc110 support as that
> > > laptop uses the "SwSmi@" sabi. It was just something I noticed while
> > > learning the code. The miscalculations will only affect any laptops that
> > > use the "SECLINUX" sabi but I tested it by setting min_brightness to 2
> > > for my laptop.
> > 
> > That's wierd, as that is the type of laptop I have here and it seems to
> > work just fine for me as-is.
> 
> Throwing a printk of user_level into set_brightness when min_brightness = 1,
> I get the following behaviour:
> 
> # cd /sys/class/backlight/samsung
> # for x in 8 7 2 1 0; do echo $x > brightness; done
> # dmesg | tail -n6
> user_level is 0x07
> user_level is 0x06
> user_level is 0x01
> user_level is 0x00
> user_level is 0xff
> samsung_laptop: SABI set command 0x11 failed with completion flag 0xaa and data 0xff
> 
> Essentially, setting brightness to the maximum actually sets it one less and
> setting it to zero does bad thngs.

Ah, ok, nice catch, thanks, I've queued up your patch now.

> > > The patch doesn't apply cleanly on top of the nc210/nc110 patch though as
> > > they both modify set_brightness(). It might apply with a higher fuzz
> > > factor as the changes don't actually clash. Should I redo the patch?
> > 
> > Please do.
> 
> Will send seperately. Doing this though, I found a problem with the
> nc210/nc110 patch in that (user_level == read_brightness()) check should
> actually be (user_brightness == read_brightness()). What should I do about
> this?

I don't know, as you seem to understand this better than I do at this
point, I'll trust your changes :)

thanks,

greg k-h

Re: [PATCH] platform: fix samsung brightness min/max calculations

[Jason Stubbs]

On Fri, 13 May 2011 02:44:02 Greg KH wrote:
> On Thu, May 12, 2011 at 12:13:59PM +1000, Jason Stubbs wrote:
> > On Wed, 11 May 2011 23:51:14 Greg KH wrote:
> > > On Wed, May 11, 2011 at 02:47:49PM +1000, Jason Stubbs wrote:
> > > > The patch doesn't apply cleanly on top of the nc210/nc110 patch
> > > > though as they both modify set_brightness(). It might apply with a
> > > > higher fuzz factor as the changes don't actually clash. Should I
> > > > redo the patch?
> > > 
> > > Please do.
> > 
> > Will send seperately. Doing this though, I found a problem with the
> > nc210/nc110 patch in that (user_level == read_brightness()) check should
> > actually be (user_brightness == read_brightness()). What should I do
> > about this?
> 
> I don't know, as you seem to understand this better than I do at this
> point, I'll trust your changes :)

I meant that patch A is broken but (working) patch B applies on top of patch
A so should I submit a patch C to fix patch A, submit a fixed patch A and
then resubmit a patch B against that, or... I'm just unsure of the
development process.

As far as I can tell, the patches aren't applied to any trees yet and are
just sitting in .../gregkh/patches.git. If that's the case, what I would like
is for fix-samsung-brightness-min-max-calculations.patch to be replaced with
the version in https://lkml.org/lkml/2011/4/20/1 and I resubmit a fixed (and
enhanced) add-support-for-samsung-nc210-nc110.patch that applies on top of
it. That way an enhancement patch won't hold up a bugfix patch should there
be any further issues.

I actually wasn't particularly happy with the nc210-n110 patch and was hoping
for some guidance but I think I've managed to implement the workaround
cleanly now so I'll post it in a sec. If you take it and the 2011/4/20/1 one,
great! Otherwise, please let me know what to do. (and sorry for the bother!)

Regards,
Jason Stubbs

[PATCH] platform: add support for samsung nc210/nc110

[Jason Stubbs]

From: Jason Stubbs <jasonbstubbs@gmail.com>

This patch adds support for the NC210/NC110 to the samsung-laptop driver.

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.

Along with adding the DMI checks for the NC210/NC110, 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 ebf1b07..89fd703 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 int 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 (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 = 0;
+	set_brightness(check_level);
+
+	if (read_brightness() != check_level) {
+		has_stepping_quirk = 1;
+		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);
@@ -668,6 +708,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);
@@ -776,6 +825,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))