Re: [PATCH] platform samsung-q10: use ACPI instead of direct EC calls

[Rafael J. Wysocki]

On Monday, July 08, 2013 10:34:56 PM Frederick van der Wyck wrote:
> On Mon, Jul 08, 2013 at 11:36:54AM -0400, Matthew Garrett wrote:
> 
> > you should be able to just use first_ec directly rather than  
> > probing yourself.
> 
> Thanks, I've made that change below.
> 
> > > +	for (i = 0; i < SAMSUNGQ10_BL_MAX_INTENSITY; i++) {
> > > +		status = acpi_evaluate_object(ec_handle, "_Q63", NULL, NULL);
> > 
> > The potential problem here is that there's no guarantee that these event
> > numbers are stable, and a firmware upgrade could change them. Of course,
> > that's also true of the EC registers, but we haven't had anyone complain
> > about the driver suddenly breaking so I'm not hugely enthusiastic about
> > replacing one fragile but seemingly working method with a fragile but
> > unproven one. 
> 
> That's true. On the other hand, imitating the action of the brightness keys at
> this higher level seems somewhat cleaner and has the advantage that the
> brightness setting is preserved on reboot (as the smi handler stores it in
> nvram). I did test that 63 and 64 are the appropriate event numbers on each of
> the models in the dmi table (although I only tested one firmware version per
> model).

I think we can make this change and if it breaks things for anyone, we'll
revert it.

Thanks,
Rafael


> Signed-off-by: Frederick van der Wyck <fvanderwyck@xxxxxxxxx>
> ---
>  drivers/platform/x86/Kconfig       |    2 +-
>  drivers/platform/x86/samsung-q10.c |   65 +++++++++++------------------------
>  2 files changed, 22 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 8577261..52aed6d 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -762,7 +762,7 @@ config INTEL_OAKTRAIL
>  
>  config SAMSUNG_Q10
>  	tristate "Samsung Q10 Extras"
> -	depends on SERIO_I8042
> +	depends on ACPI
>  	select BACKLIGHT_CLASS_DEVICE
>  	---help---
>  	  This driver provides support for backlight control on Samsung Q10
> diff --git a/drivers/platform/x86/samsung-q10.c b/drivers/platform/x86/samsung-q10.c
> index 1a90b62..9d6609d 100644
> --- a/drivers/platform/x86/samsung-q10.c
> +++ b/drivers/platform/x86/samsung-q10.c
> @@ -14,16 +14,12 @@
>  #include <linux/init.h>
>  #include <linux/platform_device.h>
>  #include <linux/backlight.h>
> -#include <linux/i8042.h>
>  #include <linux/dmi.h>
> +#include <acpi/acpi_drivers.h>
>  
> -#define SAMSUNGQ10_BL_MAX_INTENSITY      255
> -#define SAMSUNGQ10_BL_DEFAULT_INTENSITY  185
> +#define SAMSUNGQ10_BL_MAX_INTENSITY 7
>  
> -#define SAMSUNGQ10_BL_8042_CMD           0xbe
> -#define SAMSUNGQ10_BL_8042_DATA          { 0x89, 0x91 }
> -
> -static int samsungq10_bl_brightness;
> +static acpi_handle ec_handle;
>  
>  static bool force;
>  module_param(force, bool, 0);
> @@ -33,21 +29,26 @@ MODULE_PARM_DESC(force,
>  static int samsungq10_bl_set_intensity(struct backlight_device *bd)
>  {
>  
> -	int brightness = bd->props.brightness;
> -	unsigned char c[3] = SAMSUNGQ10_BL_8042_DATA;
> +	acpi_status status;
> +	int i;
>  
> -	c[2] = (unsigned char)brightness;
> -	i8042_lock_chip();
> -	i8042_command(c, (0x30 << 8) | SAMSUNGQ10_BL_8042_CMD);
> -	i8042_unlock_chip();
> -	samsungq10_bl_brightness = brightness;
> +	for (i = 0; i < SAMSUNGQ10_BL_MAX_INTENSITY; i++) {
> +		status = acpi_evaluate_object(ec_handle, "_Q63", NULL, NULL);
> +		if (ACPI_FAILURE(status))
> +			return -EIO;
> +	}
> +	for (i = 0; i < bd->props.brightness; i++) {
> +		status = acpi_evaluate_object(ec_handle, "_Q64", NULL, NULL);
> +		if (ACPI_FAILURE(status))
> +			return -EIO;
> +	}
>  
>  	return 0;
>  }
>  
>  static int samsungq10_bl_get_intensity(struct backlight_device *bd)
>  {
> -	return samsungq10_bl_brightness;
> +	return bd->props.brightness;
>  }
>  
>  static const struct backlight_ops samsungq10_bl_ops = {
> @@ -55,28 +56,6 @@ static const struct backlight_ops samsungq10_bl_ops = {
>  	.update_status	= samsungq10_bl_set_intensity,
>  };
>  
> -#ifdef CONFIG_PM_SLEEP
> -static int samsungq10_suspend(struct device *dev)
> -{
> -	return 0;
> -}
> -
> -static int samsungq10_resume(struct device *dev)
> -{
> -
> -	struct backlight_device *bd = dev_get_drvdata(dev);
> -
> -	samsungq10_bl_set_intensity(bd);
> -	return 0;
> -}
> -#else
> -#define samsungq10_suspend NULL
> -#define samsungq10_resume  NULL
> -#endif
> -
> -static SIMPLE_DEV_PM_OPS(samsungq10_pm_ops,
> -			  samsungq10_suspend, samsungq10_resume);
> -
>  static int samsungq10_probe(struct platform_device *pdev)
>  {
>  
> @@ -93,9 +72,6 @@ static int samsungq10_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, bd);
>  
> -	bd->props.brightness = SAMSUNGQ10_BL_DEFAULT_INTENSITY;
> -	samsungq10_bl_set_intensity(bd);
> -
>  	return 0;
>  }
>  
> @@ -104,9 +80,6 @@ static int samsungq10_remove(struct platform_device *pdev)
>  
>  	struct backlight_device *bd = platform_get_drvdata(pdev);
>  
> -	bd->props.brightness = SAMSUNGQ10_BL_DEFAULT_INTENSITY;
> -	samsungq10_bl_set_intensity(bd);
> -
>  	backlight_device_unregister(bd);
>  
>  	return 0;
> @@ -116,7 +89,6 @@ static struct platform_driver samsungq10_driver = {
>  	.driver		= {
>  		.name	= KBUILD_MODNAME,
>  		.owner	= THIS_MODULE,
> -		.pm	= &samsungq10_pm_ops,
>  	},
>  	.probe		= samsungq10_probe,
>  	.remove		= samsungq10_remove,
> @@ -172,6 +144,11 @@ static int __init samsungq10_init(void)
>  	if (!force && !dmi_check_system(samsungq10_dmi_table))
>  		return -ENODEV;
>  
> +	ec_handle = ec_get_handle();
> +
> +	if (!ec_handle)
> +		return -ENODEV;
> +
>  	samsungq10_device = platform_create_bundle(&samsungq10_driver,
>  						   samsungq10_probe,
>  						   NULL, 0, NULL, 0);
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/