Re: [PATCH v2] Platform: Samsung Q10 backlight driver

[Dmitry Torokhov]

Hi Fredetick,
On Mon, Feb 21, 2011 at 10:37:43PM +0000, Frederick van der Wyck wrote:
> 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.

> +
> +static int force;

If it is a bool the call it so.

> +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.

> +		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;
> +}
> +#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().

> +
> +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;
> +	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.

-- 
Dmitry
--
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/