Skip to contentSkip to conversation

Platform: Samsung Q10 backlight driver

PATCH Superseded Frederick van der Wyck 3 participants 9 messages Feb 21, 2011 – May 15, 2011 x86 platform drivers

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Frederick van der Wyck

author
#0 lore raw

PATCH v2 1/1 Platform: Samsung Q10 backlight driver

This adds backlight control on the Samsung Q10 laptop, which does not support the SABI interface. Also tested successfully on the Dell Latitude X200.

Signed-off-by Frederick van der Wyck <[email protected]>
---
 drivers/platform/x86/Kconfig       |    7 ++
 drivers/platform/x86/Makefile      |    1 +
 drivers/platform/x86/samsung-q10.c |  185 ++++++++++++++++++++++++++++++++++++
 3 files changed, 193 insertions(+), 0 deletions(-)
 create mode 100644 drivers/platform/x86/samsung-q10.c

I've made all the changes suggested by Greg and Dmitry in response to v1. Main changes:
(1) use i8042_command instead of port IO;
(2) DMI detect to auto-load and to restrict to supported hardware. Description of v1:
The Samsung Q10 is an old model, which does not support the SABI interface, so the samsung-laptop driver does not work. Backlight control is not exposed via ACPI either. This driver works in the same way as the SMI handler triggered by pressing the brightness function keys.

drivers/platform/x86/Kconfig +7 -0
index faec777..c390727 100644
@@ -639,4 +639,11 @@ config XO1_RFKILL
 	  Support for enabling/disabling the WLAN interface on the OLPC XO-1
 	  laptop.
 
+config SAMSUNG_Q10
+	tristate "Samsung Q10 Extras"
+	select BACKLIGHT_CLASS_DEVICE
+	---help---
+          This driver provides support for backlight control on Samsung Q10
+          and some other laptops, including Dell Latitude X200.
+
 endif # X86_PLATFORM_DEVICES
drivers/platform/x86/Makefile +1 -0
index 9950ccc..2cd3e52 100644
@@ -33,3 +33,4 @@ obj-$(CONFIG_INTEL_IPS)		+= intel_ips.o
 obj-$(CONFIG_GPIO_INTEL_PMIC)	+= intel_pmic_gpio.o
 obj-$(CONFIG_XO1_RFKILL)	+= xo1-rfkill.o
 obj-$(CONFIG_IBM_RTL)		+= ibm_rtl.o
+obj-$(CONFIG_SAMSUNG_Q10)	+= samsung-q10.o
drivers/platform/x86/samsung-q10.c new file +185 -0

185 lines not on this page · raw message

#1 lore raw

Hi Fredetick,

Quoted 3 lines · Frederick van der Wyck

This adds backlight control on the Samsung Q10 laptop, which does not support the SABI interface. Also tested successfully on the Dell Latitude X200.

Thanks for making the changes, looks mostly good now.

Review

Quoted 2 lines
(quoted diff) +2 -0
+
+static int force;

If it is a bool the call it so.

drivers/platform/x86/samsung-q10.c line 40

Quoted 11 lines
drivers/platform/x86/samsung-q10.c new file +11 -0
+module_param(force, bool, 0);
+MODULE_PARM_DESC(force,
+		"Disable the DMI check and force the driver to be loaded");
+
+static int samsungq10_bl_set_intensity(struct backlight_device *bd)
+{
+
+	int brightness = bd->props.brightness;
+	unsigned char c[3] = SAMSUNGQ10_BL_8042_DATA;
+
+	if (!samsungq10_suspended) {

Hmm, if samsungq10_bl_set_intensity() can indeed run simultaneously with suspend/resume then you'd need to protect it with spinlock or mutex.

Quoted 34 lines
drivers/platform/x86/samsung-q10.c new file +34 -0
+		c[2] = (unsigned char)brightness;
+		i8042_lock_chip();
+		i8042_command(c, (0x30 << 8) | SAMSUNGQ10_BL_8042_CMD);
+		i8042_unlock_chip();
+		samsungq10_bl_brightness = brightness;
+	}
+	return 0;
+}
+
+static int samsungq10_bl_get_intensity(struct backlight_device *bd)
+{
+	return samsungq10_bl_brightness;
+}
+
+static const struct backlight_ops samsungq10_bl_ops = {
+	.get_brightness = samsungq10_bl_get_intensity,
+	.update_status	= samsungq10_bl_set_intensity,
+};
+
+#ifdef CONFIG_PM
+static int samsungq10_suspend(struct device *dev)
+{
+	samsungq10_suspended = true;
+	return 0;
+}
+
+static int samsungq10_resume(struct device *dev)
+{
+
+	struct backlight_device *bd = dev_get_drvdata(dev);
+
+	samsungq10_suspended = false;
+	samsungq10_bl_set_intensity(bd);
+	return 0;

drivers/platform/x86/samsung-q10.c line 84

Quoted 10 lines
drivers/platform/x86/samsung-q10.c new file +10 -0
+}
+#else
+#define samsungq10_suspend NULL
+#define samsungq10_resume  NULL
+#endif
+
+static const struct dev_pm_ops samsungq10_pm_ops = {
+	.suspend	= samsungq10_suspend,
+	.resume		= samsungq10_resume,
+};

Don't you need to restore backlight when resuming from hibernate? I think you need to guard you methods with CONFIG_PM_SLEEP and then use SIMPLE_DEV_PM_OPS().

Quoted 28 lines
drivers/platform/x86/samsung-q10.c new file +28 -0
+
+static int __devinit samsungq10_probe(struct platform_device *pdev)
+{
+
+	struct backlight_properties props;
+	struct backlight_device *bd;
+
+	memset(&props, 0, sizeof(struct backlight_properties));
+	props.max_brightness = SAMSUNGQ10_BL_MAX_INTENSITY;
+	bd = backlight_device_register("samsung", &pdev->dev, NULL,
+				       &samsungq10_bl_ops, &props);
+	if (IS_ERR(bd))
+		return PTR_ERR(bd);
+
+	platform_set_drvdata(pdev, bd);
+
+	bd->props.brightness = SAMSUNGQ10_BL_DEFAULT_INTENSITY;
+	samsungq10_bl_set_intensity(bd);
+
+	return 0;
+}
+
+static int __devexit samsungq10_remove(struct platform_device *pdev)
+{
+
+	struct backlight_device *bd = platform_get_drvdata(pdev);
+
+	bd->props.brightness = SAMSUNGQ10_BL_DEFAULT_INTENSITY;

drivers/platform/x86/samsung-q10.c line 122

Quoted 10 lines
drivers/platform/x86/samsung-q10.c new file +10 -0
+	samsungq10_bl_set_intensity(bd);
+
+	backlight_device_unregister(bd);
+
+	return 0;
+}
+
+static struct platform_driver samsungq10_driver = {
+	.driver		= {
+		.name	= "samsung",

Samsung is way too generic for driver name, maybe call it KBUILD_MODNAME?

Thanks.

Signature · 1 line
Dmitry
author
#2 lore raw

Hi Dmitry,

Thanks for the further comments.

Quoted 1 line

If it is a bool the call it so.

Fixed.

Quoted 2 lines

Hmm, if samsungq10_bl_set_intensity() can indeed run simultaneously with suspend/resume then you'd need to protect it with spinlock or mutex.

I guess it's OK to assume samsungq10_bl_set_intensity() will not be called during suspend/resume so I've removed the check.

Quoted 3 lines

Don't you need to restore backlight when resuming from hibernate? I think you need to guard you methods with CONFIG_PM_SLEEP and then use SIMPLE_DEV_PM_OPS().

You're right, thanks, I've made the change.

Quoted 2 lines

Samsung is way too generic for driver name, maybe call it KBUILD_MODNAME?

I changed it from "samsung-q10" to "samsung" in reponse to Greg's feedback, but OK, I've made it KBUILD_MODNAME as you suggest.

Here's a new version of the patch.

Thanks,
Frederick

Signed-off-by Frederick van der Wyck <[email protected]>
---
 drivers/platform/x86/Kconfig       |    7 ++
 drivers/platform/x86/Makefile      |    1 +
 drivers/platform/x86/samsung-q10.c |  179 ++++++++++++++++++++++++++++++++++++
 3 files changed, 187 insertions(+), 0 deletions(-)
 create mode 100644 drivers/platform/x86/samsung-q10.c
drivers/platform/x86/Kconfig +7 -0
index faec777..c390727 100644
@@ -639,4 +639,11 @@ config XO1_RFKILL
 	  Support for enabling/disabling the WLAN interface on the OLPC XO-1
 	  laptop.
 
+config SAMSUNG_Q10
+	tristate "Samsung Q10 Extras"
+	select BACKLIGHT_CLASS_DEVICE
+	---help---
+          This driver provides support for backlight control on Samsung Q10
+          and some other laptops, including Dell Latitude X200.
+
 endif # X86_PLATFORM_DEVICES
drivers/platform/x86/Makefile +1 -0
index 9950ccc..2cd3e52 100644
@@ -33,3 +33,4 @@ obj-$(CONFIG_INTEL_IPS)		+= intel_ips.o
 obj-$(CONFIG_GPIO_INTEL_PMIC)	+= intel_pmic_gpio.o
 obj-$(CONFIG_XO1_RFKILL)	+= xo1-rfkill.o
 obj-$(CONFIG_IBM_RTL)		+= ibm_rtl.o
+obj-$(CONFIG_SAMSUNG_Q10)	+= samsung-q10.o
drivers/platform/x86/samsung-q10.c new file +179 -0

179 lines not on this page · raw message

author
#4 lore raw
Quoted 1 line · Matthew Garrett

Any idea if the same interface is present on the Q15/X300?

I don't know - I don't have one of those to try it, unfortunately. I wouldn't be surprised though.

author
#6 lore raw

Hi Matthew,

Quoted 3 lines

Hm. This patch makes both git-am and patch very unhappy, in that they fail to create samsung-q10.c despite it not already existing. How did you generate it?

I used git format-patch. I guess I must have done something wrong
- I'm sorry about that. Here's a version made using diff.

Signed-off-by Frederick van der Wyck <[email protected]>
drivers/platform/x86/Kconfig +7 -0
diff -uprN -X platform-drivers-x86/Documentation/dontdiff platform-drivers-x86-vanilla/drivers/platform/x86/Kconfig platform-drivers-x86/drivers/platform/x86/Kconfig
@@ -654,4 +654,11 @@ config XO1_RFKILL
 	  Support for enabling/disabling the WLAN interface on the OLPC XO-1
 	  laptop.
 
+config SAMSUNG_Q10
+	tristate "Samsung Q10 Extras"
+	select BACKLIGHT_CLASS_DEVICE
+	---help---
+          This driver provides support for backlight control on Samsung Q10
+          and some other laptops, including Dell Latitude X200.
+
 endif # X86_PLATFORM_DEVICES
drivers/platform/x86/Makefile +1 -0
diff -uprN -X platform-drivers-x86/Documentation/dontdiff platform-drivers-x86-vanilla/drivers/platform/x86/Makefile platform-drivers-x86/drivers/platform/x86/Makefile
@@ -34,3 +34,4 @@ obj-$(CONFIG_INTEL_IPS)		+= intel_ips.o
 obj-$(CONFIG_GPIO_INTEL_PMIC)	+= intel_pmic_gpio.o
 obj-$(CONFIG_XO1_RFKILL)	+= xo1-rfkill.o
 obj-$(CONFIG_IBM_RTL)		+= ibm_rtl.o
+obj-$(CONFIG_SAMSUNG_Q10)	+= samsung-q10.o
drivers/platform/x86/samsung-q10.c +179 -0

179 lines not on this page · raw message

author
#7 lore raw

Hi Matthew,

Quoted 8 lines · Frederick van der Wyck

Hi Matthew,

Quoted 3 lines

Hm. This patch makes both git-am and patch very unhappy, in that they fail to create samsung-q10.c despite it not already existing. How did you generate it?

I used git format-patch. I guess I must have done something wrong
- I'm sorry about that. Here's a version made using diff.

Has this patch been queued for 2.6.40 release? It missed out in 2.6.39 because there was a problem with it which meant it wouldn't apply, but the new version I sent fixed that. Here it is again.

Thanks,
Frederick

Signed-off-by Frederick van der Wyck <[email protected]>
---
 drivers/platform/x86/Kconfig       |    7 ++
 drivers/platform/x86/Makefile      |    1 +
 drivers/platform/x86/samsung-q10.c |  179 ++++++++++++++++++++++++++++++++++++
 3 files changed, 187 insertions(+), 0 deletions(-)
 create mode 100644 drivers/platform/x86/samsung-q10.c
drivers/platform/x86/Kconfig +7 -0
index 446f723..532a604 100644
@@ -762,4 +762,11 @@ config INTEL_OAKTRAIL
 	  enable/disable the Camera, WiFi, BT etc. devices. If in doubt, say Y
 	  here; it will only load on supported platforms.
 
+config SAMSUNG_Q10
+	tristate "Samsung Q10 Extras"
+	select BACKLIGHT_CLASS_DEVICE
+	---help---
+          This driver provides support for backlight control on Samsung Q10
+          and some other laptops, including Dell Latitude X200.
+
 endif # X86_PLATFORM_DEVICES
drivers/platform/x86/Makefile +1 -0
index 9058c72..fe577de 100644
@@ -43,3 +43,4 @@ obj-$(CONFIG_IBM_RTL)		+= ibm_rtl.o
 obj-$(CONFIG_SAMSUNG_LAPTOP)	+= samsung-laptop.o
 obj-$(CONFIG_INTEL_MFLD_THERMAL)	+= intel_mid_thermal.o
 obj-$(CONFIG_INTEL_OAKTRAIL)	+= intel_oaktrail.o
+obj-$(CONFIG_SAMSUNG_Q10)	+= samsung-q10.o
drivers/platform/x86/samsung-q10.c new file +179 -0

179 lines not on this page · raw message

author
#8 lore raw
In reply to Matthew Garrett

Hi Matthew,

Quoted 3 lines · Matthew Garrett

Hm. This patch makes both git-am and patch very unhappy, in that they fail to create samsung-q10.c despite it not already existing. How did you generate it?

Did you get the chance to try applying the new copy of the Samsung Q10 patch I sent in response to your message above? I guess you're waiting to do it along with the next round of patches, but please let me know if you tried and it still caused problems, so I can fix it for next time.

Thanks,
Frederick