On Tue, 2007-02-06 at 14:30 +0100, Marcin Juszkiewicz wrote:
quoted text
> 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:

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

Not sure you need the above line?

quoted text
> +static struct backlight_device *progear_backlight_device;

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

quoted text
> +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).

quoted text
> +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?

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

progearbl_send_intensity?

quoted text
> +       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);

quoted text
> +       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);

quoted text
> +       return 0;
> +}

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

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

-- 
Richard


-
unsubscribe notice
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/