Re: [PATCH] fb: backlight: add driver for iPAQ micro backlight

[Lee Jones]

On Sat, 12 Jul 2014, Linus Walleij wrote:

> This adds a driver for the backlight controlled by the microcontroller
> on the Compaq iPAQ series of handheld computers: h3100, h3600
> and h3700.
> 
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
>  drivers/video/backlight/Kconfig         |  9 ++++
>  drivers/video/backlight/Makefile        |  1 +
>  drivers/video/backlight/ipaq_micro_bl.c | 89 +++++++++++++++++++++++++++++++++
>  3 files changed, 99 insertions(+)
>  create mode 100644 drivers/video/backlight/ipaq_micro_bl.c
> 
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index 5d449059a556..cc153f55d9f9 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -207,6 +207,15 @@ config BACKLIGHT_GENERIC
>  	  known as the Corgi backlight driver. If you have a Sharp Zaurus
>  	  SL-C7xx, SL-Cxx00 or SL-6000x say y.
>  
> +config BACKLIGHT_IPAQ_MICRO
> +	tristate "iPAQ microcontroller backlight driver"
> +	depends on MFD_IPAQ_MICRO
> +	default y
> +	help
> +	  Say y to enable the backlight driver for Compaq iPAQ handheld
> +	  computers. Say yes if you have one of the h3100/h3600/h3700
> +	  machines.
> +
>  config BACKLIGHT_LM3533
>  	tristate "Backlight Driver for LM3533"
>  	depends on BACKLIGHT_CLASS_DEVICE
> diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
> index bb820024f346..a9ea34a39cad 100644
> --- a/drivers/video/backlight/Makefile
> +++ b/drivers/video/backlight/Makefile
> @@ -36,6 +36,7 @@ obj-$(CONFIG_BACKLIGHT_GENERIC)		+= generic_bl.o
>  obj-$(CONFIG_BACKLIGHT_GPIO)		+= gpio_backlight.o
>  obj-$(CONFIG_BACKLIGHT_HP680)		+= hp680_bl.o
>  obj-$(CONFIG_BACKLIGHT_HP700)		+= jornada720_bl.o
> +obj-$(CONFIG_BACKLIGHT_IPAQ_MICRO)	+= ipaq_micro_bl.o
>  obj-$(CONFIG_BACKLIGHT_LM3533)		+= lm3533_bl.o
>  obj-$(CONFIG_BACKLIGHT_LM3630A)		+= lm3630a_bl.o
>  obj-$(CONFIG_BACKLIGHT_LM3639)		+= lm3639_bl.o
> diff --git a/drivers/video/backlight/ipaq_micro_bl.c b/drivers/video/backlight/ipaq_micro_bl.c
> new file mode 100644
> index 000000000000..b7ec8c453b61
> --- /dev/null
> +++ b/drivers/video/backlight/ipaq_micro_bl.c
> @@ -0,0 +1,89 @@
> +/*
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * iPAQ microcontroller backlight support
> + * Author : Linus Walleij <linus.walleij@xxxxxxxxxx>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +#include <linux/backlight.h>
> +#include <linux/mfd/ipaq-micro.h>
> +#include <linux/fb.h>
> +#include <linux/err.h>
> +
> +static int micro_bl_update_status(struct backlight_device *bd)
> +{
> +	struct ipaq_micro *micro = dev_get_drvdata(&bd->dev);
> +	int intensity = bd->props.brightness;
> +	struct ipaq_micro_msg msg = {
> +		.id = MSG_BACKLIGHT,
> +		.tx_len = 3,
> +	};
> +
> +	if (bd->props.power != FB_BLANK_UNBLANK)
> +		intensity = 0;
> +	if (bd->props.state & BL_CORE_FBBLANK)
> +		intensity = 0;
> +	if (bd->props.state & BL_CORE_SUSPENDED)
> +		intensity = 0;

You can rid 2 lines of code here without sacrificing any clarity.

> +	msg.tx_data[0] = 0x01;
> +	msg.tx_data[1] = intensity > 0 ? 1 : 0;

A little comment to instantly clarify what [0] and [1] data segments
are would be great.

> +	msg.tx_data[2] = intensity;
> +	return ipaq_micro_tx_msg_sync(micro, &msg);
> +}
> +
> +static const struct backlight_ops micro_bl_ops = {
> +	.options = BL_CORE_SUSPENDRESUME,
> +	.update_status  = micro_bl_update_status,
> +};
> +
> +static struct backlight_properties micro_bl_props = {
> +	.type = BACKLIGHT_RAW,
> +	.max_brightness = 255,
> +	.power = FB_BLANK_UNBLANK,
> +	.brightness = 64,
> +};
> +
> +static int micro_backlight_probe(struct platform_device *pdev)
> +{
> +	struct backlight_device *bd;
> +	struct ipaq_micro *micro = dev_get_drvdata(pdev->dev.parent);
> +
> +	bd = devm_backlight_device_register(
> +		&pdev->dev,
> +		"ipaq-micro-backlight",
> +		&pdev->dev,
> +		micro,
> +		&micro_bl_ops,
> +		&micro_bl_props);

This formatting is odd for no particular reason.

What's wrong with the more conventional:

	bd = devm_backlight_device_register(&pdev->dev, "ipaq-micro-backlight",
					    &pdev->dev, micro, &micro_bl_ops,
					    &micro_bl_props);

> +	if (IS_ERR(bd))
> +		return PTR_ERR(bd);

Nit: Would prefer this to be split with a '\n'.

> +	platform_set_drvdata(pdev, bd);
> +	backlight_update_status(bd);
> +	dev_info(&pdev->dev, "iPAQ micro backlight driver\n");

Remove this line please.

> +	return 0;
> +}
> +
> +static int micro_backlight_remove(struct platform_device *pdev)
> +{
> +	return 0;
> +}

No need to supply this if it doesn't do anything.

> +struct platform_driver micro_backlight_device_driver = {
> +	.driver = {
> +		.name    = "ipaq-micro-backlight",
> +	},
> +	.probe   = micro_backlight_probe,
> +	.remove  = micro_backlight_remove,
> +};
> +module_platform_driver(micro_backlight_device_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("driver for iPAQ Atmel micro backlight");
> +MODULE_ALIAS("platform:ipaq-micro-backlight");

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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/