Backlight: move per-device data out of backlight_properties
Data such as current brightness belongs to a device and should not
be part of a structure shared between several devices.
Signed-off-by: Dmitry Torokhov <[email protected]>
---
drivers/video/backlight/backlight.c | 37 +++++++++---------------------------
drivers/video/backlight/lcd.c | 2 -
include/linux/backlight.h | 21 +++++++++++---------
include/linux/lcd.h | 4 +--
4 files changed, 25 insertions(+), 39 deletions(-)
Index: work/include/linux/backlight.h
===================================================================
--- work.orig/include/linux/backlight.h
+++ work/include/linux/backlight.h
@@ -26,15 +26,8 @@ struct backlight_properties {
return 0 if not, !=0 if it is. If NULL, backlight always matches the fb. */
int (*check_fb)(struct fb_info *);
- /* Current User requested brightness (0 - max_brightness) */
- int brightness;
/* Maximal value for brightness (read-only) */
int max_brightness;
- /* Current FB Power mode (0: full on, 1..3: power saving
- modes; 4: full off), see FB_BLANK_XXX */
- int power;
- /* FB Blanking active? (values as for power) */
- int fb_blank;
};
struct backlight_device {
@@ -43,15 +36,25 @@ struct backlight_device {
points to something in the body of that driver, it is also invalid. */
struct mutex mutex;
/* If this is NULL, the backing module is unloaded */
- struct backlight_properties *props;
+ const struct backlight_properties *props;
+
/* The framebuffer notifier block */
struct notifier_block fb_notif;
+
+ /* Current User requested brightness (0 - max_brightness) */
+ int brightness;
+ /* Current FB Power mode (0: full on, 1..3: power saving
+ modes; 4: full off), see FB_BLANK_XXX */
+ int power;
+ /* FB Blanking active? (values as for power) */
+ int fb_blank;
+
/* The class device structure */
struct class_device class_dev;
};
extern struct backlight_device *backlight_device_register(const char *name,
- void *devdata, struct backlight_properties *bp);
+ void *devdata, const struct backlight_properties *bp);
extern void backlight_device_unregister(struct backlight_device *bd);
#define to_backlight_device(obj) container_of(obj, struct backlight_device, class_dev)
Index: work/drivers/video/backlight/backlight.c
===================================================================
--- work.orig/drivers/video/backlight/backlight.c
+++ work/drivers/video/backlight/backlight.c
@@ -16,18 +16,9 @@
static ssize_t backlight_show_power(struct class_device *cdev, char *buf)
{
- int rc;
struct backlight_device *bd = to_backlight_device(cdev);
- rc = mutex_lock_interruptible(&bd->mutex);
- if (rc)
- return rc;
-
- rc = bd->props ? sprintf(buf, "%d\n", bd->props->power) : -ENXIO;
-
- mutex_unlock(&bd->mutex);
-
- return rc;
+ return sprintf(buf, "%d\n", bd->power);
}
static ssize_t backlight_store_power(struct class_device *cdev, const char *buf, size_t count)
@@ -49,7 +40,7 @@ static ssize_t backlight_store_power(str
if (bd->props) {
pr_debug("backlight: set power to %d\n", power);
- bd->props->power = power;
+ bd->power = power;
if (bd->props->update_status)
bd->props->update_status(bd);
rc = count;
@@ -63,18 +54,9 @@ static ssize_t backlight_store_power(str
static ssize_t backlight_show_brightness(struct class_device *cdev, char *buf)
{
- int rc;
struct backlight_device *bd = to_backlight_device(cdev);
- rc = mutex_lock_interruptible(&bd->mutex);
- if (rc)
- return rc;
-
- rc = bd->props ? sprintf(buf, "%d\n", bd->props->brightness) : -ENXIO;
-
- mutex_unlock(&bd->mutex);
-
- return rc;
+ return sprintf(buf, "%d\n", bd->brightness);
}
static ssize_t backlight_store_brightness(struct class_device *cdev, const char *buf, size_t count)
@@ -100,7 +82,7 @@ static ssize_t backlight_store_brightnes
else {
pr_debug("backlight: set brightness to %d\n",
brightness);
- bd->props->brightness = brightness;
+ bd->brightness = brightness;
if (bd->props->update_status)
bd->props->update_status(bd);
rc = count;
@@ -191,7 +173,7 @@ static int fb_notifier_callback(struct n
if (bd->props) {
if (!bd->props->check_fb ||
bd->props->check_fb(evdata->info)) {
- bd->props->fb_blank = *(int *)evdata->data;
+ bd->fb_blank = *(int *)evdata->data;
if (likely(bd->props && bd->props->update_status))
bd->props->update_status(bd);
}
@@ -213,8 +195,9 @@ static int fb_notifier_callback(struct n
* Creates and registers new backlight class_device. Returns either an
* ERR_PTR() or a pointer to the newly allocated device.
*/
-struct backlight_device *backlight_device_register(const char *name, void *devdata,
- struct backlight_properties *bp)
+struct backlight_device *
+backlight_device_register(const char *name, void *devdata,
+ const struct backlight_properties *bp)
{
int err;
struct backlight_device *new_bd;
@@ -264,8 +247,8 @@ void backlight_device_unregister(struct
mutex_lock(&bd->mutex);
if (bd->props && bd->props->update_status) {
- bd->props->brightness = 0;
- bd->props->power = 0;
+ bd->brightness = 0;
+ bd->power = 0;
bd->props->update_status(bd);
}
Index: work/drivers/video/backlight/lcd.c
===================================================================
--- work.orig/drivers/video/backlight/lcd.c
+++ work/drivers/video/backlight/lcd.c
@@ -186,7 +186,7 @@ static int fb_notifier_callback(struct n
* or a pointer to the newly allocated device.
*/
struct lcd_device *lcd_device_register(const char *name, void *devdata,
- struct lcd_properties *lp)
+ const struct lcd_properties *lp)
{
int err;
struct lcd_device *new_ld;
Index: work/include/linux/lcd.h
===================================================================
--- work.orig/include/linux/lcd.h
+++ work/include/linux/lcd.h
@@ -38,7 +38,7 @@ struct lcd_device {
points to something in the body of that driver, it is also invalid. */
struct mutex mutex;
/* If this is NULL, the backing module is unloaded */
- struct lcd_properties *props;
+ const struct lcd_properties *props;
/* The framebuffer notifier block */
struct notifier_block fb_notif;
/* The class device structure */
@@ -46,7 +46,7 @@ struct lcd_device {
};
extern struct lcd_device *lcd_device_register(const char *name,
- void *devdata, struct lcd_properties *lp);
+ void *devdata, const struct lcd_properties *lp);
extern void lcd_device_unregister(struct lcd_device *ld);
#define to_lcd_device(obj) container_of(obj, struct lcd_device, class_dev)
On Fri, 2006-08-11 at 01:03 -0400, Dmitry Torokhov wrote:
> plain text document attachment (backlight-move-data.patch)
> Backlight: move per-device data out of backlight_properties
>
> Data such as current brightness belongs to a device and should not
> be part of a structure shared between several devices.
I agree there's an issue to address here. Looking at this patch very
quickly, it breaks all the existing backlight drivers as they know about
the variables in struct backlight_properties and all their references
need to be updated e.g.: corgi_bl.c:
if (bd->props->power != FB_BLANK_UNBLANK)
intensity = 0;
if (bd->props->fb_blank != FB_BLANK_UNBLANK)
intensity = 0;
Thinking about this, ideally, struct backlight_properties would be left
containing the backlight properties in but become part of struct
backlight_device (and allocated with it). The drivers would provide a
new struct backlight_ops instead of the properties struct at present and
the function pointers would move to that structure.
Your other patches looked ok at a quick glance. I'll aim to test them
against the corgi driver over the weekend and I look at reworking this
one, unless you want beat me to it :)
Cheers,
Richard
On 8/11/06, Richard Purdie <[email protected]> wrote:
> On Fri, 2006-08-11 at 01:03 -0400, Dmitry Torokhov wrote:
> > plain text document attachment (backlight-move-data.patch)
> > Backlight: move per-device data out of backlight_properties
> >
> > Data such as current brightness belongs to a device and should not
> > be part of a structure shared between several devices.
>
> I agree there's an issue to address here. Looking at this patch very
> quickly, it breaks all the existing backlight drivers as they know about
> the variables in struct backlight_properties and all their references
> need to be updated e.g.: corgi_bl.c:
>
> if (bd->props->power != FB_BLANK_UNBLANK)
> intensity = 0;
> if (bd->props->fb_blank != FB_BLANK_UNBLANK)
> intensity = 0;
>
Oops, I had them all updated but apparently lost that change. I'll fix
it and resend.
> Thinking about this, ideally, struct backlight_properties would be left
> containing the backlight properties in but become part of struct
> backlight_device (and allocated with it).
Why would you want to separate properties into a structure? You don't
normally pass a set of properties around so I am not sure why would we
need this...
--
Dmitry
On Fri, 2006-08-11 at 08:27 -0400, Dmitry Torokhov wrote:
> On 8/11/06, Richard Purdie <[email protected]> wrote:
> > Thinking about this, ideally, struct backlight_properties would be left
> > containing the backlight properties in but become part of struct
> > backlight_device (and allocated with it).
>
> Why would you want to separate properties into a structure? You don't
> normally pass a set of properties around so I am not sure why would we
> need this...
The structure would just end up being optimised away by the compiler so
would just serve to keep the properties themselves separate from the
device data. I'm not so bothered about that but don't really want a
struct backlight_properties around which just contains what would be
better called something like struct backlight_ops. The backlight core
has changed a fair bit and the names are starting to lose meaning.
Richard
On 8/11/06, Richard Purdie <[email protected]> wrote:
> On Fri, 2006-08-11 at 08:27 -0400, Dmitry Torokhov wrote:
> > On 8/11/06, Richard Purdie <[email protected]> wrote:
> > > Thinking about this, ideally, struct backlight_properties would be left
> > > containing the backlight properties in but become part of struct
> > > backlight_device (and allocated with it).
> >
> > Why would you want to separate properties into a structure? You don't
> > normally pass a set of properties around so I am not sure why would we
> > need this...
>
> The structure would just end up being optimised away by the compiler so
> would just serve to keep the properties themselves separate from the
> device data. I'm not so bothered about that but don't really want a
> struct backlight_properties around which just contains what would be
> better called something like struct backlight_ops. The backlight core
> has changed a fair bit and the names are starting to lose meaning.
>
There is some constant shared data, such as max_brightness, etc, that
is still residing in backlist_poperties so _ops would not be quite
correct.
--
Dmitry