From: Jason Stubbs <[email protected]>
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 <[email protected]>
---
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);
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.
On Wed, 20 Apr 2011 13:58:50 Jason Stubbs wrote:
> --- 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;
min_brightness is (and probably will always be) only 0 or 1 in the current
sabi_configs, so this patch doesn't actually change present behaviour. Should
there ever be a min_brightness > 1, however, this patch will be required or
user_brightness will end up going below 0 and wrapping around.
> 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;
The sign here is definitely wrong. If the user sets a brightness of 0 when
using a sabi_config with a min_brightness of 1, user_level will become -1 and
that is the value that will be passed to the hardware.
> @@ -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;
Without subtracting the min_brightness, the user is offered a range of 0
through 8 for both sabi_configs. For the min_brightness == 1 case and with
set_brightness() fixed, a user setting of 8 would actually set 9 in hardware.
Regards,
Jason Stubbs
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?
thanks,
greg k-h
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.
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?
Regards,
Jason Stubbs
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.
> 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.
thanks,
greg k-h
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.
> > 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?
Regards,
Jason Stubbs
From: Jason Stubbs <[email protected]>
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.
Currently, the driver is reporting a max brightness of 8 and doing the offset
the wrong way such that setting a brightness of 8 will set as 7 in hardware
while setting a brightness of 0 will attempt (and fail) to set as -1 in
hardware.
This patch fixes these calculations as well as a potential miscalculation due
to an assumption of min_brightness being either 0 or 1.
Signed-off-by: Jason Stubbs <[email protected]>
---
diff --git a/drivers/platform/x86/samsung-laptop.c b/drivers/platform/x86/samsung-laptop.c
index 01d5278..995b041 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;
if (user_level == read_brightness()) {
return;
@@ -799,7 +801,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);
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
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
From: Jason Stubbs <[email protected]>
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 <[email protected]>
--
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))
On Fri, May 13, 2011 at 08:44:25PM +1000, Jason Stubbs wrote:
> 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.
Ok, I'll drop this one, but can you resend the above referenced patch so
I can apply it?
I'll also update your last patch you sent as well.
Sorry for the delay in working on this, my access to a samsung laptop is
now gone, and I've been swamped with other stuff.
Any chance you want to take over maintaining this driver now yourself?
You seem to have a much better understanding of it in places than I do
at the moment, combined with a strong need to keep it working properly
:)
thanks,
greg k-h
From: Jason Stubbs <[email protected]>
The min_brightness value of the sabi_config is incorrectly used in brightness
calculations. For the config where min_brightness = 1 and max_brightness = 8,
the user visible range should be 0 to 7 with hardware being set in the range
of 1 to 8. What is actually happening is that the user visible range is 0 to
8 with hardware being set in the range of -1 to 7.
This patch fixes the above issue as well as a miscalculation that would occur
in the case of min_brightness > 1.
Signed-off-by: Jason Stubbs <[email protected]>
---
diff --git a/drivers/platform/x86/samsung-laptop.c b/drivers/platform/x86/samsung-laptop.c
index 4c78dbc..aad14ab 100644
--- a/drivers/platform/x86/samsung-laptop.c
+++ b/drivers/platform/x86/samsung-laptop.c
@@ -371,15 +371,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;
if (has_stepping_quirk && user_level != 0) {
/*
@@ -834,7 +836,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);
On Tue, 14 Jun 2011 09:55:49 Greg KH wrote:
> On Fri, May 13, 2011 at 08:44:25PM +1000, Jason Stubbs wrote:
> > 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.
>
> Ok, I'll drop this one, but can you resend the above referenced patch so
> I can apply it?
Somehow I managed to screw things up again as the nc210-nc110 patch isn't
against the min-max-calculations patch... Doesn't matter though. The patch I
just resent applies on top of the nc210-nc110 patch that you already have in
patches.git.
> Sorry for the delay in working on this, my access to a samsung laptop is
> now gone, and I've been swamped with other stuff.
No problem.
> Any chance you want to take over maintaining this driver now yourself?
> You seem to have a much better understanding of it in places than I do
> at the moment, combined with a strong need to keep it working properly
I'm not exactly sure what that entails, but I don't have access to any specs
and have never dealt with any hardware programming before. Even these patches
don't directly deal with the hardware so I'm not sure that I'm best suited to
being the maintainer. Having said that, I'm happy to try my hand at any bugs
that arise and review any patches..
Regards,
Jason Stubbs