Re: [PATCH] backlight control for Frontpath ProGear HX1050+

[Richard Purdie <rpurdie@rpsys.net>]

On Tue, 2007-02-06 at 14:30 +0100, Marcin Juszkiewicz wrote:
> From: Marcin Juszkiewicz <openembedded@hrw.one.pl>
> 
> Add control of LCD backlight for Frontpath ProGear HX1050+.
> Patch is based on http://downloads.sf.net/progear/progear-lcd-0.2.tar.gz
> driver by M Schacht.
> 
> Signed-Off-By: Marcin Juszkiewicz <openembedded@hrw.one.pl>

This is very similar to corgi_bl which I have some changes I was
considering to make it a bit more efficient/clean. The same changes
could apply to this driver:

> +static int progearbl_intensity;
> +static struct backlight_properties progearbl_data;

Not sure you need the above line?

> +static struct backlight_device *progear_backlight_device;

You shouldn't need this structure pointer (see below)

> +static int progearbl_get_intensity(struct backlight_device *bd)
> +{
> +       return progearbl_intensity;
> +}

Can you read PMU_LPCR and return it (minus HW_LEVEL_MIN) here? corgi_bl
only stores this as it can't read back from the hardware. Either
approach is probably ok (although if the register can be updated by
other means, it should be read).

> +static int progearbl_set_intensity(struct backlight_device *bd)
> +{
> +       progearbl_send_intensity(progear_backlight_device);
> +       return 0;
> +}

You can use progearbl_send_intensity(bd); here or maybe lose the
function all together?

> +static struct backlight_properties progearbl_data = {
> +       .owner = THIS_MODULE,
> +       .get_brightness = progearbl_get_intensity,
> +       .update_status = progearbl_set_intensity,

progearbl_send_intensity?

> +       pci_read_config_byte(sb_dev, SB_MPS1, &temp);
> +       pci_write_config_byte(sb_dev, SB_MPS1, temp | 0x20);
> +
> +       progear_backlight_device = backlight_device_register("progear-bl",
> +			   &pdev->dev, NULL, &progearbl_data);
> +       if (IS_ERR(progear_backlight_device))
> +               return PTR_ERR(progear_backlight_device);

platform_set_drvdata(pdev, progear_backlight_device);

> +       progearbl_data.power = FB_BLANK_UNBLANK;
> +       progearbl_data.brightness = HW_LEVEL_MAX - HW_LEVEL_MIN;
> +       progearbl_data.max_brightness = HW_LEVEL_MAX - HW_LEVEL_MIN;
> +       progearbl_send_intensity(progear_backlight_device);
> +
> +       return 0;
> +}
> +
> +static int progearbl_remove(struct platform_device *dev)
> +{
> +       backlight_device_unregister(progear_backlight_device);

struct backlight_device *bd = platform_get_drvdata(pdev);
backlight_device_unregister(bd);

> +       return 0;
> +}

All pretty minor though and if you change the above, it can have an

Acked-by: Richard Purdie <rpurdie@rpsys.net>

-- 
Richard