lkml.org 
[lkml]   [2008]   [Feb]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    SubjectRe: [PATCH/HP7XX] - Add combined LCD / BL driver for platform HP Jornada 7xx handhelds
    From
    Attempt #3 :
    Changes :
    ---------
    * removed JORN.. prefixes except from functions.
    * removed file location in head
    * fixed (hope) indent issues
    * do not exceed 80 chars per line
    * added extra checks if driver allocation fails
    * return -1 instead of (MAX + 1)
    ...and probably alot more.

    Patch:
    * builds
    * passes checkpatch.pl (only complaining about signoff-tag)

    Oh, and thanks for all the feedback people! (and for doing it nicely)
    Only thing not adress so far is richards mdelay question, need to do a quick test before I can answer that.

    /Kristoffer

    diff --git a/drivers/video/backlight/jornada720_bllcd.c b/drivers/video/backlight/jornada720_bllcd.c
    new file mode 100644
    index 0000000..0adbc49
    --- /dev/null
    +++ b/drivers/video/backlight/jornada720_bllcd.c
    @@ -0,0 +1,287 @@
    +/*
    + *
    + * Backlight and LCD Driver for HP Jornada 720
    + * Copyright (C) 2007 Kristoffer Ericson <Kristoffer.Ericson@gmail.com>
    + *
    + * This program is free software; you can redistribute it and/or modify
    + * it under the terms of the GNU General Public License version 2
    + * or any later version as published by the Free Software Foundation.
    + *
    + */
    +#include <linux/module.h>
    +#include <linux/kernel.h>
    +#include <linux/init.h>
    +#include <linux/backlight.h>
    +#include <linux/lcd.h>
    +#include <linux/delay.h>
    +#include <linux/platform_device.h>
    +#include <linux/fb.h>
    +#include <linux/device.h>
    +#include <asm/hardware.h>
    +#include <asm/arch/jornada720.h>
    +#include <video/s1d13xxxfb.h>
    +
    +MODULE_AUTHOR("Kristoffer Ericson <kristoffer.ericson@gmail.com>");
    +MODULE_DESCRIPTION("HP Jornada 710/720/728 Backlight/LCD Driver");
    +MODULE_LICENSE("GPL");
    +
    +/* Contrast */
    +#define LCD_MAX_CONTR 0xff
    +#define LCD_DEF_CONTR 0x80
    +/* Brightness */
    +#define BL_MAX_BRIGHT 0xff
    +#define BL_DEF_BRIGHT 0x19
    +
    +struct bllcd_device {
    + struct backlight_device *bl_device;
    + struct lcd_device *lcd_device;
    +};
    +
    +/*
    + * BACKLIGHT HANDLING ROUTINES
    + */
    +static int jornada_bl_get_brightness(struct backlight_device *dev)
    +{
    + int ret;
    +
    + /* check if backlight is on */
    + if (!(PPSR & PPC_LDD1))
    + return BL_MAX_BRIGHT;
    +
    + jornada_ssp_start();
    + if (jornada_ssp_inout(GETBRIGHTNESS) == -ETIMEDOUT) {
    + printk(KERN_ERR "bl :get brightness timeout\n");
    + ret = -1;
    + } else
    + ret = jornada_ssp_inout(TXDUMMY);
    +
    + jornada_ssp_end();
    +
    + /* 0 is max brightness */
    + return BL_MAX_BRIGHT - ret;
    +}
    +
    +static int jornada_bl_update_status(struct backlight_device *dev)
    +{
    + int ret = 0;
    + int value;
    +
    + jornada_ssp_start();
    +
    + if (dev->props.power != FB_BLANK_UNBLANK ||
    + dev->props.fb_blank != FB_BLANK_UNBLANK) {
    + ret = jornada_ssp_inout(BRIGHTNESSOFF);
    + if (ret == -ETIMEDOUT) {
    + printk(KERN_ERR "bl : brightness off timeout\n");
    + /* backlight off */
    + PPSR &= ~PPC_LDD1;
    + PPDR |= PPC_LDD1;
    + }
    + } else { /* backlight on */
    + PPSR |= PPC_LDD1;
    + /* send setbrightness cmd */
    + ret = jornada_ssp_inout(SETBRIGHTNESS);
    + if (ret != TXDUMMY) {
    + printk(KERN_ERR "bl :set brightness timeout\n");
    + } else {
    + /* cmd accepted */
    + value = BL_MAX_BRIGHT - dev->props.brightness;
    + if (jornada_ssp_byte(value) == TXDUMMY)
    + ret = value;
    + else
    + printk(KERN_ERR "bl :set brightness failed\n");
    + }
    + }
    +
    + jornada_ssp_end();
    +
    + return ret;
    +}
    +
    +/*
    + * LCD HANDLING FUNCTIONS
    + */
    +static int jornada_lcd_set_contrast(struct lcd_device *pdev, int contrast)
    +{
    + int ret = 0;
    +
    + jornada_ssp_start();
    +
    + ret = jornada_ssp_inout(SETCONTRAST);
    +
    + if (ret == -ETIMEDOUT)
    + printk(KERN_ERR "lcd :set contrast timeout\n");
    + else
    + ret = jornada_ssp_byte(contrast);
    +
    + jornada_ssp_end();
    +
    + return ret;
    +}
    +
    +static int jornada_lcd_set_power(struct lcd_device *pdev, int power)
    +{
    + if (power != FB_BLANK_UNBLANK) {
    + /* turn off LCD */
    + PPSR &= ~PPC_LDD2;
    + PPDR |= PPC_LDD2;
    + } else {
    + /* turn on LCD */
    + PPSR |= PPC_LDD2;
    + }
    +
    + return 0;
    +}
    +
    +static int jornada_lcd_get_power(struct lcd_device *pdev)
    +{
    + if (PPSR & PPC_LDD2)
    + return FB_BLANK_UNBLANK;
    + else
    + return FB_BLANK_POWERDOWN;
    +}
    +
    +static int jornada_lcd_get_contrast(struct lcd_device *pdev)
    +{
    + int ret;
    +
    + /* Don't set contrast on off powerd LCD */
    + if (jornada_lcd_get_power(pdev) != FB_BLANK_UNBLANK)
    + return 0;
    +
    + jornada_ssp_start();
    +
    + ret = jornada_ssp_inout(GETCONTRAST);
    + if (ret != -ETIMEDOUT)
    + ret = jornada_ssp_inout(TXDUMMY);
    + else {
    + printk(KERN_ERR "lcd :get contrast timeout\n");
    + ret = -1;
    + }
    +
    + jornada_ssp_end();
    +
    + return ret;
    +}
    +
    +static struct backlight_ops jornada_bl_ops = {
    + .get_brightness = jornada_bl_get_brightness,
    + .update_status = jornada_bl_update_status,
    +};
    +
    +static struct lcd_ops jornada_lcd_ops = {
    + .get_contrast = jornada_lcd_get_contrast,
    + .set_contrast = jornada_lcd_set_contrast,
    + .get_power = jornada_lcd_get_power,
    + .set_power = jornada_lcd_set_power,
    +};
    +
    +static int jornada_bl_probe(struct platform_device *pdev)
    +{
    + struct bllcd_device *bllcd;
    +
    + bllcd = kzalloc(sizeof(*bllcd), GFP_KERNEL);
    + if (bllcd == NULL)
    + return -ENOMEM;
    +
    + /* bl driver - name must match fb driver name */
    + bllcd->bl_device = backlight_device_register(S1D_DEVICENAME,
    + &pdev->dev, NULL, &jornada_bl_ops);
    +
    + if (IS_ERR(bllcd->bl_device)) {
    + kfree(bllcd);
    + printk(KERN_ERR "bl :failed to register device\n");
    + return -ENODEV;
    + }
    +
    + /* lcd driver */
    + bllcd->lcd_device = lcd_device_register(S1D_DEVICENAME,
    + &pdev->dev, NULL, &jornada_lcd_ops);
    + if (IS_ERR(bllcd->lcd_device)) {
    + backlight_device_unregister(bllcd->bl_device);
    + kfree(bllcd);
    + printk(KERN_ERR "lcd :failed to register device\n");
    + return -ENODEV;
    + }
    +
    + jornada_lcd_set_contrast(bllcd->lcd_device, LCD_DEF_CONTR);
    + jornada_lcd_set_power(bllcd->lcd_device, FB_BLANK_UNBLANK);
    +
    + msleep(100);
    +
    + bllcd->bl_device->props.power = FB_BLANK_UNBLANK;
    + bllcd->bl_device->props.brightness = BL_DEF_BRIGHT;
    + jornada_bl_update_status(bllcd->bl_device);
    +
    + platform_set_drvdata(pdev, bllcd);
    + printk(KERN_INFO "HP Jornada 7xx Backlight/LCD driver activated\n");
    +
    + return 0;
    +}
    +
    +static int jornada_bl_remove(struct platform_device *pdev)
    +{
    + struct bllcd_device *bllcd = platform_get_drvdata(pdev);
    +
    + bllcd->bl_device->props.power = FB_BLANK_POWERDOWN;
    + bllcd->bl_device->props.brightness = 0;
    + bllcd->bl_device->props.max_brightness = 0;
    +
    + backlight_device_unregister(bllcd->bl_device);
    + lcd_device_unregister(bllcd->lcd_device);
    +
    + return 0;
    +}
    +
    +static int jornada_bl_suspend(struct platform_device *pdev, pm_message_t state)
    +{
    + struct bllcd_device *bllcd = platform_get_drvdata(pdev);
    +
    + bllcd->bl_device->props.power = FB_BLANK_POWERDOWN;
    + bllcd->bl_device->props.brightness = 0;
    + bllcd->bl_device->props.max_brightness = 0;
    + jornada_bl_update_status(bllcd->bl_device);
    +
    + jornada_lcd_set_power(bllcd->lcd_device, FB_BLANK_POWERDOWN);
    +
    + return 0;
    +}
    +
    +static int jornada_bl_resume(struct platform_device *pdev)
    +{
    + struct bllcd_device *bllcd = platform_get_drvdata(pdev);
    +
    + bllcd->bl_device->props.power = FB_BLANK_UNBLANK;
    + bllcd->bl_device->props.brightness = BL_DEF_BRIGHT;
    + bllcd->bl_device->props.max_brightness = BL_MAX_BRIGHT;
    + jornada_bl_update_status(bllcd->bl_device);
    +
    + jornada_lcd_set_power(bllcd->lcd_device, FB_BLANK_UNBLANK);
    +
    + return 0;
    +}
    +
    +static struct platform_driver jornada_bl_driver = {
    + .probe = jornada_bl_probe,
    + .remove = jornada_bl_remove,
    +#ifdef CONFIG_PM
    + .suspend = jornada_bl_suspend,
    + .resume = jornada_bl_resume,
    +#endif
    + .driver = {
    + .name = "jornada_bllcd",
    + },
    +};
    +
    +static int __init jornada_bl_init(void)
    +{
    + return platform_driver_register(&jornada_bl_driver);
    +}
    +
    +static void __exit jornada_bl_exit(void)
    +{
    + platform_driver_unregister(&jornada_bl_driver);
    +}
    +
    +module_init(jornada_bl_init);
    +module_exit(jornada_bl_exit);




    On Wed, 6 Feb 2008 16:44:36 +0000
    Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
    > On Wed, Feb 06, 2008 at 05:33:41PM +0100, Kristoffer Ericson wrote:
    > > Oki, here is attempt #2 (btw, new mail or just keep thread?)
    >
    > Probably better to keep the thread. More comments 8)
    >
    > > +struct jornada_bllcd_device {
    > > + struct backlight_device *jorn_backlight_device;
    > > + struct lcd_device *jorn_lcd_device;
    >
    > We're inside a jornada file. Do these "jorn_" prefixes add anything
    > useful that the reader doesn't already know?
    >
    > > +static int jornada_bl_update_status(struct backlight_device *dev)
    > > +{
    > > + int ret = 0;
    > > +
    > > + jornada_ssp_start();
    > > +
    > > + if (dev->props.power != FB_BLANK_UNBLANK ||
    > > + dev->props.fb_blank != FB_BLANK_UNBLANK) {
    > > + ret = jornada_ssp_inout(BRIGHTNESSOFF);
    > > + if (ret == -ETIMEDOUT)
    > > + printk(KERN_ERR "jornada720_bl: \
    > > + BrightnessOff timeout\n");
    > > + /* backlight off */
    > > + PPSR &= ~PPC_LDD1;
    > > + PPDR |= PPC_LDD1;
    >
    > Look at the above 5 lines. What are your expectations if ret != -ETIMEDOUT?
    > If C were python you'd have one behaviour due to the indent...
    >
    > > +
    > > + } else { /* backlight on */
    >
    > Too many spaces.
    >
    > > + PPSR |= PPC_LDD1;
    > > + if ((jornada_ssp_inout(SETBRIGHTNESS)) == TXDUMMY) {
    > > + /* send brightness value (0 is max, 255 lowest) */
    > > + if (jornada_ssp_byte(JORNADA_BL_MAX_BRIGHTNESS -
    > > + dev->props.brightness) != -ETIMEDOUT)
    > > + ret = (JORNADA_BL_MAX_BRIGHTNESS -
    > > + dev->props.brightness);
    > > + } else
    > > + printk(KERN_ERR "jornada720_bl: \
    > > + SetBrightness timeout\n");
    > > + }
    >
    > If this corresponds with the "} else {" line, it should have one tab. Plus
    > the above indented code section should probably have one less indentation.
    >
    > > + jornada_ssp_end();
    > > +
    > > + return ret;
    > > +}
    > > +
    > > +/*
    > > + * LCD HANDLING FUNCTIONS
    > > + */
    > > +static int jornada_lcd_set_contrast(struct lcd_device *pdev, int contrast)
    > > +{
    > > + int ret = 0;
    > > +
    > > + jornada_ssp_start();
    > > +
    > > + ret = jornada_ssp_inout(SETCONTRAST);
    > > +
    > > + if (ret == -ETIMEDOUT)
    > > + printk(KERN_ERR "jornada_lcd: failure to set contrast\n");
    > > + else
    >
    > Space before 'else' not required.
    >
    > > + ret = jornada_ssp_byte(contrast);
    > > +
    > > + jornada_ssp_end();
    > > +
    > > + return ret;
    > > +}
    > > +
    > > +static int jornada_lcd_set_power(struct lcd_device *pdev, int power)
    > > +{
    > > + if (power != FB_BLANK_UNBLANK) {
    > > + /* turn off LCD */
    > > + PPSR &= ~PPC_LDD2;
    > > + PPDR |= PPC_LDD2;
    > > + } else {
    > > + /* turn on LCD */
    > > + PPSR |= PPC_LDD2;
    > > + }
    >
    > Wrongly placed }
    >
    > > +
    > > + return 0;
    > > +}
    > > +
    > > +static int jornada_lcd_get_power(struct lcd_device *pdev)
    > > +{
    > > + if (PPSR & PPC_LDD2)
    > > + return FB_BLANK_UNBLANK;
    > > + else
    > > + return FB_BLANK_POWERDOWN;
    > > +}
    > > +
    > > +static int jornada_lcd_get_contrast(struct lcd_device *pdev)
    > > +{
    > > + int ret;
    > > +
    > > + /* Don't set contrast on off powerd LCD */
    > > + if (jornada_lcd_get_power(pdev) != FB_BLANK_UNBLANK)
    > > + return 0;
    > > +
    > > + jornada_ssp_start();
    > > +
    > > + ret = jornada_ssp_inout(GETCONTRAST);
    > > + if (ret != -ETIMEDOUT)
    > > + ret = jornada_ssp_inout(TXDUMMY);
    > > + else {
    > > + printk(KERN_ERR "jornada_lcd: getcontrast failed!\n");
    > > + ret = JORNADA_BL_MAX_BRIGHTNESS + 1;
    > > + }
    > > +
    > > + jornada_ssp_end();
    > > +
    > > + return ret;
    > > +}
    > > +
    > > +static struct backlight_ops jornada_bl_ops = {
    > > + .get_brightness = jornada_bl_get_brightness,
    > > + .update_status = jornada_bl_update_status,
    > > +};
    > > +
    > > +static struct lcd_ops jornada_lcd_ops = {
    > > + .get_contrast = jornada_lcd_get_contrast,
    > > + .set_contrast = jornada_lcd_set_contrast,
    > > + .get_power = jornada_lcd_get_power,
    > > + .set_power = jornada_lcd_set_power,
    > > +};
    > > +
    > > +static int jornada_bl_probe(struct platform_device *pdev)
    > > +{
    > > + struct jornada_bllcd_device *bllcd;
    > > +
    > > + bllcd = kzalloc(sizeof(*bllcd), GFP_KERNEL);
    > > + if (bllcd == NULL)
    > > + return -ENOMEM;
    > > +
    > > + /* bl driver - name must match fb driver name */
    > > + bllcd->jorn_backlight_device = backlight_device_register(S1D_DEVICENAME,
    > > + &pdev->dev, NULL, &jornada_bl_ops);
    > > +
    > > + if (IS_ERR(bllcd->jorn_backlight_device)) {
    > > + kfree(bllcd);
    > > + return PTR_ERR(bllcd->jorn_backlight_device);
    >
    > Access after free. Bad bad bad.
    >
    > > + }
    > > +
    > > + /* lcd driver */
    > > + bllcd->jorn_lcd_device = lcd_device_register(S1D_DEVICENAME,
    > > + &pdev->dev, NULL, &jornada_lcd_ops);
    > > + if (IS_ERR(bllcd->jorn_lcd_device)) {
    > > + kfree(bllcd);
    > > + return PTR_ERR(bllcd->jorn_lcd_device);
    >
    > Access after free. Bad bad bad.
    >
    > > + }
    > > +
    > > + jornada_lcd_set_contrast(bllcd->jorn_lcd_device,
    > > + JORNADA_LCD_DEFAULT_CONTRAST);
    > > + jornada_lcd_set_power(bllcd->jorn_lcd_device,
    > > + FB_BLANK_UNBLANK);
    > > +
    > > + msleep(100);
    > > +
    > > + bllcd->jorn_backlight_device->props.power
    > > + = FB_BLANK_UNBLANK;
    > > + bllcd->jorn_backlight_device->props.brightness
    > > + = JORNADA_BL_DEFAULT_BRIGHTNESS;
    >
    > If you get rid of the "jorn_" prefix, can these be on one line?
    >
    > > + jornada_bl_update_status(bllcd->jorn_backlight_device);
    > > +
    > > + platform_set_drvdata(pdev, bllcd);
    > > + printk(KERN_INFO "HP Jornada 7xx Backlight/LCD driver activated\n");
    > > +
    > > + return 0;
    > > +}
    > > +
    > > +static int jornada_bl_remove(struct platform_device *pdev)
    > > +{
    > > + struct jornada_bllcd_device *bllcd = platform_get_drvdata(pdev);
    > > +
    > > + bllcd->jorn_backlight_device->props.power = FB_BLANK_POWERDOWN;
    > > + bllcd->jorn_backlight_device->props.brightness = 0;
    > > + bllcd->jorn_backlight_device->props.max_brightness = 0;
    > > +
    > > + backlight_device_unregister(bllcd->jorn_backlight_device);
    > > + lcd_device_unregister(bllcd->jorn_lcd_device);
    >
    > Leaves bllcd allocated - memory leak.
    >
    > > +
    > > + return 0;
    > > +}
    > > +
    > > +static int jornada_bl_suspend(struct platform_device *pdev, pm_message_t state)
    > > +{
    > > + struct jornada_bllcd_device *bllcd = platform_get_drvdata(pdev);
    > > +
    > > + bllcd->jorn_backlight_device->props.power = FB_BLANK_POWERDOWN;
    > > + bllcd->jorn_backlight_device->props.brightness = 0;
    > > + bllcd->jorn_backlight_device->props.max_brightness = 0;
    > > + jornada_bl_update_status(bllcd->jorn_backlight_device);
    > > +
    > > + jornada_lcd_set_power(bllcd->jorn_lcd_device, FB_BLANK_POWERDOWN);
    > > +
    > > + return 0;
    > > +}
    > > +
    > > +static int jornada_bl_resume(struct platform_device *pdev)
    > > +{
    > > + struct jornada_bllcd_device *bllcd = platform_get_drvdata(pdev);
    > > +
    > > + bllcd->jorn_backlight_device->props.power
    > > + = FB_BLANK_UNBLANK;
    > > + bllcd->jorn_backlight_device->props.brightness
    > > + = JORNADA_BL_DEFAULT_BRIGHTNESS;
    > > + bllcd->jorn_backlight_device->props.max_brightness
    > > + = JORNADA_BL_MAX_BRIGHTNESS;
    > > + jornada_bl_update_status(bllcd->jorn_backlight_device);
    > > +
    > > + jornada_lcd_set_power(bllcd->jorn_lcd_device, FB_BLANK_UNBLANK);
    > > +
    > > + return 0;
    > > +}
    > > +
    > > +static struct platform_driver jornada_bl_driver = {
    > > + .probe = jornada_bl_probe,
    > > + .remove = jornada_bl_remove,
    > > +#ifdef CONFIG_PM
    > > + .suspend = jornada_bl_suspend,
    > > + .resume = jornada_bl_resume,
    > > +#endif
    > > + .driver = {
    > > + .name = "jornada_bllcd",
    >
    > a tab, not 4 spaces.
    >
    [unhandled content-type:application/octet-stream]
    \
     
     \ /
      Last update: 2008-02-06 20:25    [W:0.055 / U:90.732 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site