* [patch 1/6] Convert to use default class device attributes
2006-08-11 5:03 [patch 0/6] Backlight & lcd fixes/cleanups Dmitry Torokhov
@ 2006-08-11 5:03 ` Dmitry Torokhov
2006-08-11 5:03 ` [patch 2/6] Fix error handling when registering new device Dmitry Torokhov
` (4 subsequent siblings)
5 siblings, 0 replies; 19+ messages in thread
From: Dmitry Torokhov @ 2006-08-11 5:03 UTC (permalink / raw)
To: Richard Purdie; +Cc: LKML
[-- Attachment #1: backlight-attributes.patch --]
[-- Type: text/plain, Size: 6119 bytes --]
Backlight: convert to use default class device attributes
Use provided by the driver core method of creating set of default
attributes for all devices belonging to a given class instead of
using homegrown code.
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---
drivers/video/backlight/backlight.c | 49 ++++++++----------------------------
drivers/video/backlight/lcd.c | 44 +++++++-------------------------
2 files changed, 22 insertions(+), 71 deletions(-)
Index: work/drivers/video/backlight/backlight.c
===================================================================
--- work.orig/drivers/video/backlight/backlight.c
+++ work/drivers/video/backlight/backlight.c
@@ -130,25 +130,19 @@ static void backlight_class_release(stru
kfree(bd);
}
-static struct class backlight_class = {
- .name = "backlight",
- .release = backlight_class_release,
+static struct class_device_attribute bl_class_device_attributes[] = {
+ __ATTR(power, 0644, backlight_show_power, backlight_store_power),
+ __ATTR(brightness, 0644,
+ backlight_show_brightness, backlight_store_brightness),
+ __ATTR(actual_brightness, 0444, backlight_show_actual_brightness, NULL),
+ __ATTR(max_brightness, 0444, backlight_show_max_brightness, NULL),
+ __ATTR_NULL
};
-#define DECLARE_ATTR(_name,_mode,_show,_store) \
-{ \
- .attr = { .name = __stringify(_name), .mode = _mode, .owner = THIS_MODULE }, \
- .show = _show, \
- .store = _store, \
-}
-
-static struct class_device_attribute bl_class_device_attributes[] = {
- DECLARE_ATTR(power, 0644, backlight_show_power, backlight_store_power),
- DECLARE_ATTR(brightness, 0644, backlight_show_brightness,
- backlight_store_brightness),
- DECLARE_ATTR(actual_brightness, 0444, backlight_show_actual_brightness,
- NULL),
- DECLARE_ATTR(max_brightness, 0444, backlight_show_max_brightness, NULL),
+static struct class backlight_class = {
+ .name = "backlight",
+ .release = backlight_class_release,
+ .class_dev_attrs = bl_class_device_attributes,
};
/* This callback gets called when something important happens inside a
@@ -193,7 +187,7 @@ static int fb_notifier_callback(struct n
struct backlight_device *backlight_device_register(const char *name, void *devdata,
struct backlight_properties *bp)
{
- int i, rc;
+ int rc;
struct backlight_device *new_bd;
pr_debug("backlight_device_alloc: name=%s\n", name);
@@ -222,19 +216,6 @@ error: kfree(new_bd);
if (unlikely(rc))
goto error;
- for (i = 0; i < ARRAY_SIZE(bl_class_device_attributes); i++) {
- rc = class_device_create_file(&new_bd->class_dev,
- &bl_class_device_attributes[i]);
- if (unlikely(rc)) {
- while (--i >= 0)
- class_device_remove_file(&new_bd->class_dev,
- &bl_class_device_attributes[i]);
- class_device_unregister(&new_bd->class_dev);
- /* No need to kfree(new_bd) since release() method was called */
- return ERR_PTR(rc);
- }
- }
-
return new_bd;
}
EXPORT_SYMBOL(backlight_device_register);
@@ -247,17 +228,11 @@ EXPORT_SYMBOL(backlight_device_register)
*/
void backlight_device_unregister(struct backlight_device *bd)
{
- int i;
-
if (!bd)
return;
pr_debug("backlight_device_unregister: name=%s\n", bd->class_dev.class_id);
- for (i = 0; i < ARRAY_SIZE(bl_class_device_attributes); i++)
- class_device_remove_file(&bd->class_dev,
- &bl_class_device_attributes[i]);
-
down(&bd->sem);
if (likely(bd->props && bd->props->update_status)) {
bd->props->brightness = 0;
Index: work/drivers/video/backlight/lcd.c
===================================================================
--- work.orig/drivers/video/backlight/lcd.c
+++ work/drivers/video/backlight/lcd.c
@@ -109,22 +109,17 @@ static void lcd_class_release(struct cla
kfree(ld);
}
-static struct class lcd_class = {
- .name = "lcd",
- .release = lcd_class_release,
+static struct class_device_attribute lcd_class_device_attributes[] = {
+ __ATTR(power, 0644, lcd_show_power, lcd_store_power),
+ __ATTR(contrast, 0644, lcd_show_contrast, lcd_store_contrast),
+ __ATTR(max_contrast, 0444, lcd_show_max_contrast, NULL),
+ __ATTR_NULL
};
-#define DECLARE_ATTR(_name,_mode,_show,_store) \
-{ \
- .attr = { .name = __stringify(_name), .mode = _mode, .owner = THIS_MODULE }, \
- .show = _show, \
- .store = _store, \
-}
-
-static struct class_device_attribute lcd_class_device_attributes[] = {
- DECLARE_ATTR(power, 0644, lcd_show_power, lcd_store_power),
- DECLARE_ATTR(contrast, 0644, lcd_show_contrast, lcd_store_contrast),
- DECLARE_ATTR(max_contrast, 0444, lcd_show_max_contrast, NULL),
+static struct class lcd_class = {
+ .name = "lcd",
+ .release = lcd_class_release,
+ .class_dev_attrs = lcd_class_device_attributes,
};
/* This callback gets called when something important happens inside a
@@ -164,7 +159,7 @@ static int fb_notifier_callback(struct n
struct lcd_device *lcd_device_register(const char *name, void *devdata,
struct lcd_properties *lp)
{
- int i, rc;
+ int rc;
struct lcd_device *new_ld;
pr_debug("lcd_device_register: name=%s\n", name);
@@ -193,19 +188,6 @@ error: kfree(new_ld);
if (unlikely(rc))
goto error;
- for (i = 0; i < ARRAY_SIZE(lcd_class_device_attributes); i++) {
- rc = class_device_create_file(&new_ld->class_dev,
- &lcd_class_device_attributes[i]);
- if (unlikely(rc)) {
- while (--i >= 0)
- class_device_remove_file(&new_ld->class_dev,
- &lcd_class_device_attributes[i]);
- class_device_unregister(&new_ld->class_dev);
- /* No need to kfree(new_ld) since release() method was called */
- return ERR_PTR(rc);
- }
- }
-
return new_ld;
}
EXPORT_SYMBOL(lcd_device_register);
@@ -218,17 +200,11 @@ EXPORT_SYMBOL(lcd_device_register);
*/
void lcd_device_unregister(struct lcd_device *ld)
{
- int i;
-
if (!ld)
return;
pr_debug("lcd_device_unregister: name=%s\n", ld->class_dev.class_id);
- for (i = 0; i < ARRAY_SIZE(lcd_class_device_attributes); i++)
- class_device_remove_file(&ld->class_dev,
- &lcd_class_device_attributes[i]);
-
down(&ld->sem);
ld->props = NULL;
up(&ld->sem);
^ permalink raw reply [flat|nested] 19+ messages in thread* [patch 2/6] Fix error handling when registering new device
2006-08-11 5:03 [patch 0/6] Backlight & lcd fixes/cleanups Dmitry Torokhov
2006-08-11 5:03 ` [patch 1/6] Convert to use default class device attributes Dmitry Torokhov
@ 2006-08-11 5:03 ` Dmitry Torokhov
2006-08-11 5:03 ` [patch 3/6] Get rid of excessive amount of likely()s Dmitry Torokhov
` (3 subsequent siblings)
5 siblings, 0 replies; 19+ messages in thread
From: Dmitry Torokhov @ 2006-08-11 5:03 UTC (permalink / raw)
To: Richard Purdie; +Cc: LKML
[-- Attachment #1: backlight-fix-error-handling.patch --]
[-- Type: text/plain, Size: 1690 bytes --]
Backlight: fix error handling when registering new device
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---
drivers/video/backlight/backlight.c | 8 +++++---
drivers/video/backlight/lcd.c | 8 +++++---
2 files changed, 10 insertions(+), 6 deletions(-)
Index: work/drivers/video/backlight/backlight.c
===================================================================
--- work.orig/drivers/video/backlight/backlight.c
+++ work/drivers/video/backlight/backlight.c
@@ -205,7 +205,7 @@ struct backlight_device *backlight_devic
rc = class_device_register(&new_bd->class_dev);
if (unlikely(rc)) {
-error: kfree(new_bd);
+ kfree(new_bd);
return ERR_PTR(rc);
}
@@ -213,8 +213,10 @@ error: kfree(new_bd);
new_bd->fb_notif.notifier_call = fb_notifier_callback;
rc = fb_register_client(&new_bd->fb_notif);
- if (unlikely(rc))
- goto error;
+ if (unlikely(rc)) {
+ class_device_unregister(&new_bd->class_dev);
+ return ERR_PTR(rc);
+ }
return new_bd;
}
Index: work/drivers/video/backlight/lcd.c
===================================================================
--- work.orig/drivers/video/backlight/lcd.c
+++ work/drivers/video/backlight/lcd.c
@@ -177,7 +177,7 @@ struct lcd_device *lcd_device_register(c
rc = class_device_register(&new_ld->class_dev);
if (unlikely(rc)) {
-error: kfree(new_ld);
+ kfree(new_ld);
return ERR_PTR(rc);
}
@@ -185,8 +185,10 @@ error: kfree(new_ld);
new_ld->fb_notif.notifier_call = fb_notifier_callback;
rc = fb_register_client(&new_ld->fb_notif);
- if (unlikely(rc))
- goto error;
+ if (unlikely(rc)) {
+ class_device_unregister(&new_ld->class_dev);
+ return ERR_PTR(rc);
+ }
return new_ld;
}
^ permalink raw reply [flat|nested] 19+ messages in thread* [patch 3/6] Get rid of excessive amount of likely()s
2006-08-11 5:03 [patch 0/6] Backlight & lcd fixes/cleanups Dmitry Torokhov
2006-08-11 5:03 ` [patch 1/6] Convert to use default class device attributes Dmitry Torokhov
2006-08-11 5:03 ` [patch 2/6] Fix error handling when registering new device Dmitry Torokhov
@ 2006-08-11 5:03 ` Dmitry Torokhov
2006-08-11 5:03 ` [patch 4/6] Remove "owner" from backlight_properties structure Dmitry Torokhov
` (2 subsequent siblings)
5 siblings, 0 replies; 19+ messages in thread
From: Dmitry Torokhov @ 2006-08-11 5:03 UTC (permalink / raw)
To: Richard Purdie; +Cc: LKML
[-- Attachment #1: backlight-cleanup.patch --]
[-- Type: text/plain, Size: 8966 bytes --]
Backlight: get rid of excessive amount of likely()s
There are no hot paths in the backlight core; do not litter the
code with likely()s and rely on compiler to do the right thing.
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---
drivers/video/backlight/backlight.c | 45 +++++++++++++++++-------------------
drivers/video/backlight/lcd.c | 37 ++++++++++++-----------------
2 files changed, 38 insertions(+), 44 deletions(-)
Index: work/drivers/video/backlight/backlight.c
===================================================================
--- work.orig/drivers/video/backlight/backlight.c
+++ work/drivers/video/backlight/backlight.c
@@ -20,7 +20,7 @@ static ssize_t backlight_show_power(stru
struct backlight_device *bd = to_backlight_device(cdev);
down(&bd->sem);
- if (likely(bd->props))
+ if (bd->props)
rc = sprintf(buf, "%d\n", bd->props->power);
up(&bd->sem);
@@ -41,10 +41,10 @@ static ssize_t backlight_store_power(str
return -EINVAL;
down(&bd->sem);
- if (likely(bd->props)) {
+ if (bd->props) {
pr_debug("backlight: set power to %d\n", power);
bd->props->power = power;
- if (likely(bd->props->update_status))
+ if (bd->props->update_status)
bd->props->update_status(bd);
rc = count;
}
@@ -59,7 +59,7 @@ static ssize_t backlight_show_brightness
struct backlight_device *bd = to_backlight_device(cdev);
down(&bd->sem);
- if (likely(bd->props))
+ if (bd->props)
rc = sprintf(buf, "%d\n", bd->props->brightness);
up(&bd->sem);
@@ -80,14 +80,14 @@ static ssize_t backlight_store_brightnes
return -EINVAL;
down(&bd->sem);
- if (likely(bd->props)) {
+ if (bd->props) {
if (brightness > bd->props->max_brightness)
rc = -EINVAL;
else {
pr_debug("backlight: set brightness to %d\n",
brightness);
bd->props->brightness = brightness;
- if (likely(bd->props->update_status))
+ if (bd->props->update_status)
bd->props->update_status(bd);
rc = count;
}
@@ -103,7 +103,7 @@ static ssize_t backlight_show_max_bright
struct backlight_device *bd = to_backlight_device(cdev);
down(&bd->sem);
- if (likely(bd->props))
+ if (bd->props)
rc = sprintf(buf, "%d\n", bd->props->max_brightness);
up(&bd->sem);
@@ -117,7 +117,7 @@ static ssize_t backlight_show_actual_bri
struct backlight_device *bd = to_backlight_device(cdev);
down(&bd->sem);
- if (likely(bd->props && bd->props->get_brightness))
+ if (bd->props && bd->props->get_brightness)
rc = sprintf(buf, "%d\n", bd->props->get_brightness(bd));
up(&bd->sem);
@@ -127,6 +127,7 @@ static ssize_t backlight_show_actual_bri
static void backlight_class_release(struct class_device *dev)
{
struct backlight_device *bd = to_backlight_device(dev);
+
kfree(bd);
}
@@ -153,7 +154,7 @@ static int fb_notifier_callback(struct n
unsigned long event, void *data)
{
struct backlight_device *bd;
- struct fb_event *evdata =(struct fb_event *)data;
+ struct fb_event *evdata = data;
/* If we aren't interested in this event, skip it immediately ... */
if (event != FB_EVENT_BLANK)
@@ -161,13 +162,14 @@ static int fb_notifier_callback(struct n
bd = container_of(self, struct backlight_device, fb_notif);
down(&bd->sem);
- if (bd->props)
+ if (bd->props) {
if (!bd->props->check_fb ||
bd->props->check_fb(evdata->info)) {
bd->props->fb_blank = *(int *)evdata->data;
if (likely(bd->props && bd->props->update_status))
bd->props->update_status(bd);
}
+ }
up(&bd->sem);
return 0;
}
@@ -187,35 +189,33 @@ static int fb_notifier_callback(struct n
struct backlight_device *backlight_device_register(const char *name, void *devdata,
struct backlight_properties *bp)
{
- int rc;
+ int err;
struct backlight_device *new_bd;
pr_debug("backlight_device_alloc: name=%s\n", name);
- new_bd = kmalloc(sizeof(struct backlight_device), GFP_KERNEL);
- if (unlikely(!new_bd))
+ new_bd = kzalloc(sizeof(struct backlight_device), GFP_KERNEL);
+ if (!new_bd)
return ERR_PTR(-ENOMEM);
init_MUTEX(&new_bd->sem);
new_bd->props = bp;
- memset(&new_bd->class_dev, 0, sizeof(new_bd->class_dev));
new_bd->class_dev.class = &backlight_class;
strlcpy(new_bd->class_dev.class_id, name, KOBJ_NAME_LEN);
class_set_devdata(&new_bd->class_dev, devdata);
- rc = class_device_register(&new_bd->class_dev);
- if (unlikely(rc)) {
+ err = class_device_register(&new_bd->class_dev);
+ if (err) {
kfree(new_bd);
- return ERR_PTR(rc);
+ return ERR_PTR(err);
}
- memset(&new_bd->fb_notif, 0, sizeof(new_bd->fb_notif));
new_bd->fb_notif.notifier_call = fb_notifier_callback;
- rc = fb_register_client(&new_bd->fb_notif);
- if (unlikely(rc)) {
+ err = fb_register_client(&new_bd->fb_notif);
+ if (err) {
class_device_unregister(&new_bd->class_dev);
- return ERR_PTR(rc);
+ return ERR_PTR(err);
}
return new_bd;
@@ -236,7 +236,7 @@ void backlight_device_unregister(struct
pr_debug("backlight_device_unregister: name=%s\n", bd->class_dev.class_id);
down(&bd->sem);
- if (likely(bd->props && bd->props->update_status)) {
+ if (bd->props && bd->props->update_status) {
bd->props->brightness = 0;
bd->props->power = 0;
bd->props->update_status(bd);
@@ -246,7 +246,6 @@ void backlight_device_unregister(struct
up(&bd->sem);
fb_unregister_client(&bd->fb_notif);
-
class_device_unregister(&bd->class_dev);
}
EXPORT_SYMBOL(backlight_device_unregister);
Index: work/drivers/video/backlight/lcd.c
===================================================================
--- work.orig/drivers/video/backlight/lcd.c
+++ work/drivers/video/backlight/lcd.c
@@ -16,14 +16,12 @@
static ssize_t lcd_show_power(struct class_device *cdev, char *buf)
{
- int rc;
+ int rc = -ENXIO;
struct lcd_device *ld = to_lcd_device(cdev);
down(&ld->sem);
- if (likely(ld->props && ld->props->get_power))
+ if (ld->props && ld->props->get_power)
rc = sprintf(buf, "%d\n", ld->props->get_power(ld));
- else
- rc = -ENXIO;
up(&ld->sem);
return rc;
@@ -43,7 +41,7 @@ static ssize_t lcd_store_power(struct cl
return -EINVAL;
down(&ld->sem);
- if (likely(ld->props && ld->props->set_power)) {
+ if (ld->props && ld->props->set_power) {
pr_debug("lcd: set power to %d\n", power);
ld->props->set_power(ld, power);
rc = count;
@@ -59,7 +57,7 @@ static ssize_t lcd_show_contrast(struct
struct lcd_device *ld = to_lcd_device(cdev);
down(&ld->sem);
- if (likely(ld->props && ld->props->get_contrast))
+ if (ld->props && ld->props->get_contrast)
rc = sprintf(buf, "%d\n", ld->props->get_contrast(ld));
up(&ld->sem);
@@ -80,7 +78,7 @@ static ssize_t lcd_store_contrast(struct
return -EINVAL;
down(&ld->sem);
- if (likely(ld->props && ld->props->set_contrast)) {
+ if (ld->props && ld->props->set_contrast) {
pr_debug("lcd: set contrast to %d\n", contrast);
ld->props->set_contrast(ld, contrast);
rc = count;
@@ -96,7 +94,7 @@ static ssize_t lcd_show_max_contrast(str
struct lcd_device *ld = to_lcd_device(cdev);
down(&ld->sem);
- if (likely(ld->props))
+ if (ld->props)
rc = sprintf(buf, "%d\n", ld->props->max_contrast);
up(&ld->sem);
@@ -130,7 +128,7 @@ static int fb_notifier_callback(struct n
unsigned long event, void *data)
{
struct lcd_device *ld;
- struct fb_event *evdata =(struct fb_event *)data;
+ struct fb_event *evdata = data;
/* If we aren't interested in this event, skip it immediately ... */
if (event != FB_EVENT_BLANK)
@@ -159,35 +157,33 @@ static int fb_notifier_callback(struct n
struct lcd_device *lcd_device_register(const char *name, void *devdata,
struct lcd_properties *lp)
{
- int rc;
+ int err;
struct lcd_device *new_ld;
pr_debug("lcd_device_register: name=%s\n", name);
- new_ld = kmalloc(sizeof(struct lcd_device), GFP_KERNEL);
- if (unlikely(!new_ld))
+ new_ld = kzalloc(sizeof(struct lcd_device), GFP_KERNEL);
+ if (!new_ld)
return ERR_PTR(-ENOMEM);
init_MUTEX(&new_ld->sem);
new_ld->props = lp;
- memset(&new_ld->class_dev, 0, sizeof(new_ld->class_dev));
new_ld->class_dev.class = &lcd_class;
strlcpy(new_ld->class_dev.class_id, name, KOBJ_NAME_LEN);
class_set_devdata(&new_ld->class_dev, devdata);
- rc = class_device_register(&new_ld->class_dev);
- if (unlikely(rc)) {
+ err = class_device_register(&new_ld->class_dev);
+ if (unlikely(err)) {
kfree(new_ld);
- return ERR_PTR(rc);
+ return ERR_PTR(err);
}
- memset(&new_ld->fb_notif, 0, sizeof(new_ld->fb_notif));
new_ld->fb_notif.notifier_call = fb_notifier_callback;
- rc = fb_register_client(&new_ld->fb_notif);
- if (unlikely(rc)) {
+ err = fb_register_client(&new_ld->fb_notif);
+ if (err) {
class_device_unregister(&new_ld->class_dev);
- return ERR_PTR(rc);
+ return ERR_PTR(err);
}
return new_ld;
@@ -212,7 +208,6 @@ void lcd_device_unregister(struct lcd_de
up(&ld->sem);
fb_unregister_client(&ld->fb_notif);
-
class_device_unregister(&ld->class_dev);
}
EXPORT_SYMBOL(lcd_device_unregister);
^ permalink raw reply [flat|nested] 19+ messages in thread* [patch 4/6] Remove "owner" from backlight_properties structure
2006-08-11 5:03 [patch 0/6] Backlight & lcd fixes/cleanups Dmitry Torokhov
` (2 preceding siblings ...)
2006-08-11 5:03 ` [patch 3/6] Get rid of excessive amount of likely()s Dmitry Torokhov
@ 2006-08-11 5:03 ` Dmitry Torokhov
2006-08-11 5:03 ` [patch 5/6] Convert to use mutexes instead of semaphores Dmitry Torokhov
2006-08-11 5:03 ` [patch 6/6] Move per-device data out of backlight_properties Dmitry Torokhov
5 siblings, 0 replies; 19+ messages in thread
From: Dmitry Torokhov @ 2006-08-11 5:03 UTC (permalink / raw)
To: Richard Purdie; +Cc: LKML
[-- Attachment #1: backlight-remove-owner.patch --]
[-- Type: text/plain, Size: 6741 bytes --]
Backlight: remove "owner" from backlight_properties structure
Nothing uses it and it is unlikely that it will ever be used -
backlight uses other means to ensure that nothing references
unloaded code.
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---
drivers/macintosh/via-pmu-backlight.c | 1 -
drivers/usb/misc/appledisplay.c | 1 -
drivers/video/aty/aty128fb.c | 1 -
drivers/video/aty/atyfb_base.c | 1 -
drivers/video/aty/radeon_backlight.c | 1 -
drivers/video/backlight/corgi_bl.c | 1 -
drivers/video/backlight/hp680_bl.c | 1 -
drivers/video/backlight/locomolcd.c | 1 -
drivers/video/nvidia/nv_backlight.c | 1 -
drivers/video/riva/fbdev.c | 1 -
include/linux/backlight.h | 3 ---
include/linux/lcd.h | 2 --
12 files changed, 15 deletions(-)
Index: work/drivers/macintosh/via-pmu-backlight.c
===================================================================
--- work.orig/drivers/macintosh/via-pmu-backlight.c
+++ work/drivers/macintosh/via-pmu-backlight.c
@@ -81,7 +81,6 @@ static int pmu_backlight_get_brightness(
}
static struct backlight_properties pmu_backlight_data = {
- .owner = THIS_MODULE,
.get_brightness = pmu_backlight_get_brightness,
.update_status = pmu_backlight_update_status,
.max_brightness = (FB_BACKLIGHT_LEVELS - 1),
Index: work/drivers/usb/misc/appledisplay.c
===================================================================
--- work.orig/drivers/usb/misc/appledisplay.c
+++ work/drivers/usb/misc/appledisplay.c
@@ -179,7 +179,6 @@ static int appledisplay_bl_get_brightnes
}
static struct backlight_properties appledisplay_bl_data = {
- .owner = THIS_MODULE,
.get_brightness = appledisplay_bl_get_brightness,
.update_status = appledisplay_bl_update_status,
.max_brightness = 0xFF
Index: work/drivers/video/aty/aty128fb.c
===================================================================
--- work.orig/drivers/video/aty/aty128fb.c
+++ work/drivers/video/aty/aty128fb.c
@@ -1792,7 +1792,6 @@ static int aty128_bl_get_brightness(stru
}
static struct backlight_properties aty128_bl_data = {
- .owner = THIS_MODULE,
.get_brightness = aty128_bl_get_brightness,
.update_status = aty128_bl_update_status,
.max_brightness = (FB_BACKLIGHT_LEVELS - 1),
Index: work/drivers/video/aty/atyfb_base.c
===================================================================
--- work.orig/drivers/video/aty/atyfb_base.c
+++ work/drivers/video/aty/atyfb_base.c
@@ -2191,7 +2191,6 @@ static int aty_bl_get_brightness(struct
}
static struct backlight_properties aty_bl_data = {
- .owner = THIS_MODULE,
.get_brightness = aty_bl_get_brightness,
.update_status = aty_bl_update_status,
.max_brightness = (FB_BACKLIGHT_LEVELS - 1),
Index: work/drivers/video/aty/radeon_backlight.c
===================================================================
--- work.orig/drivers/video/aty/radeon_backlight.c
+++ work/drivers/video/aty/radeon_backlight.c
@@ -134,7 +134,6 @@ static int radeon_bl_get_brightness(stru
}
static struct backlight_properties radeon_bl_data = {
- .owner = THIS_MODULE,
.get_brightness = radeon_bl_get_brightness,
.update_status = radeon_bl_update_status,
.max_brightness = (FB_BACKLIGHT_LEVELS - 1),
Index: work/drivers/video/backlight/corgi_bl.c
===================================================================
--- work.orig/drivers/video/backlight/corgi_bl.c
+++ work/drivers/video/backlight/corgi_bl.c
@@ -106,7 +106,6 @@ EXPORT_SYMBOL(corgibl_limit_intensity);
static struct backlight_properties corgibl_data = {
- .owner = THIS_MODULE,
.get_brightness = corgibl_get_intensity,
.update_status = corgibl_set_intensity,
};
Index: work/drivers/video/backlight/hp680_bl.c
===================================================================
--- work.orig/drivers/video/backlight/hp680_bl.c
+++ work/drivers/video/backlight/hp680_bl.c
@@ -96,7 +96,6 @@ static int hp680bl_get_intensity(struct
}
static struct backlight_properties hp680bl_data = {
- .owner = THIS_MODULE,
.max_brightness = HP680_MAX_INTENSITY,
.get_brightness = hp680bl_get_intensity,
.update_status = hp680bl_set_intensity,
Index: work/drivers/video/backlight/locomolcd.c
===================================================================
--- work.orig/drivers/video/backlight/locomolcd.c
+++ work/drivers/video/backlight/locomolcd.c
@@ -142,7 +142,6 @@ static int locomolcd_get_intensity(struc
}
static struct backlight_properties locomobl_data = {
- .owner = THIS_MODULE,
.get_brightness = locomolcd_get_intensity,
.update_status = locomolcd_set_intensity,
.max_brightness = 4,
Index: work/drivers/video/nvidia/nv_backlight.c
===================================================================
--- work.orig/drivers/video/nvidia/nv_backlight.c
+++ work/drivers/video/nvidia/nv_backlight.c
@@ -104,7 +104,6 @@ static int nvidia_bl_get_brightness(stru
}
static struct backlight_properties nvidia_bl_data = {
- .owner = THIS_MODULE,
.get_brightness = nvidia_bl_get_brightness,
.update_status = nvidia_bl_update_status,
.max_brightness = (FB_BACKLIGHT_LEVELS - 1),
Index: work/drivers/video/riva/fbdev.c
===================================================================
--- work.orig/drivers/video/riva/fbdev.c
+++ work/drivers/video/riva/fbdev.c
@@ -346,7 +346,6 @@ static int riva_bl_get_brightness(struct
}
static struct backlight_properties riva_bl_data = {
- .owner = THIS_MODULE,
.get_brightness = riva_bl_get_brightness,
.update_status = riva_bl_update_status,
.max_brightness = (FB_BACKLIGHT_LEVELS - 1),
Index: work/include/linux/backlight.h
===================================================================
--- work.orig/include/linux/backlight.h
+++ work/include/linux/backlight.h
@@ -17,9 +17,6 @@ struct fb_info;
/* This structure defines all the properties of a backlight
(usually attached to a LCD). */
struct backlight_properties {
- /* Owner module */
- struct module *owner;
-
/* Notify the backlight driver some property has changed */
int (*update_status)(struct backlight_device *);
/* Return the current backlight brightness (accounting for power,
Index: work/include/linux/lcd.h
===================================================================
--- work.orig/include/linux/lcd.h
+++ work/include/linux/lcd.h
@@ -16,8 +16,6 @@ struct fb_info;
/* This structure defines all the properties of a LCD flat panel. */
struct lcd_properties {
- /* Owner module */
- struct module *owner;
/* Get the LCD panel power status (0: full on, 1..3: controller
power on, flat panel power off, 4: full off), see FB_BLANK_XXX */
int (*get_power)(struct lcd_device *);
^ permalink raw reply [flat|nested] 19+ messages in thread* [patch 5/6] Convert to use mutexes instead of semaphores
2006-08-11 5:03 [patch 0/6] Backlight & lcd fixes/cleanups Dmitry Torokhov
` (3 preceding siblings ...)
2006-08-11 5:03 ` [patch 4/6] Remove "owner" from backlight_properties structure Dmitry Torokhov
@ 2006-08-11 5:03 ` Dmitry Torokhov
2006-08-11 12:58 ` Dmitry Torokhov
2006-08-11 5:03 ` [patch 6/6] Move per-device data out of backlight_properties Dmitry Torokhov
5 siblings, 1 reply; 19+ messages in thread
From: Dmitry Torokhov @ 2006-08-11 5:03 UTC (permalink / raw)
To: Richard Purdie; +Cc: LKML
[-- Attachment #1: backlight-sem-to-mutex.patch --]
[-- Type: text/plain, Size: 10469 bytes --]
Backlight: convert to use mutexes instead of semaphores
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---
drivers/video/backlight/backlight.c | 89 +++++++++++++++++++++++-------------
drivers/video/backlight/lcd.c | 73 +++++++++++++++++++++--------
include/linux/backlight.h | 2
include/linux/lcd.h | 2
4 files changed, 112 insertions(+), 54 deletions(-)
Index: work/drivers/video/backlight/backlight.c
===================================================================
--- work.orig/drivers/video/backlight/backlight.c
+++ work/drivers/video/backlight/backlight.c
@@ -16,20 +16,23 @@
static ssize_t backlight_show_power(struct class_device *cdev, char *buf)
{
- int rc = -ENXIO;
+ int rc;
struct backlight_device *bd = to_backlight_device(cdev);
- down(&bd->sem);
- if (bd->props)
- rc = sprintf(buf, "%d\n", bd->props->power);
- up(&bd->sem);
+ 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;
}
static ssize_t backlight_store_power(struct class_device *cdev, const char *buf, size_t count)
{
- int rc = -ENXIO;
+ int rc;
char *endp;
struct backlight_device *bd = to_backlight_device(cdev);
int power = simple_strtoul(buf, &endp, 0);
@@ -40,35 +43,43 @@ static ssize_t backlight_store_power(str
if (size != count)
return -EINVAL;
- down(&bd->sem);
+ rc = mutex_lock_interruptible(&bd->mutex);
+ if (rc)
+ return rc;
+
if (bd->props) {
pr_debug("backlight: set power to %d\n", power);
bd->props->power = power;
if (bd->props->update_status)
bd->props->update_status(bd);
rc = count;
- }
- up(&bd->sem);
+ } else
+ rc = -ENXIO;
+
+ mutex_unlock(&bd->mutex);
return rc;
}
static ssize_t backlight_show_brightness(struct class_device *cdev, char *buf)
{
- int rc = -ENXIO;
+ int rc;
struct backlight_device *bd = to_backlight_device(cdev);
- down(&bd->sem);
- if (bd->props)
- rc = sprintf(buf, "%d\n", bd->props->brightness);
- up(&bd->sem);
+ 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;
}
static ssize_t backlight_store_brightness(struct class_device *cdev, const char *buf, size_t count)
{
- int rc = -ENXIO;
+ int rc;
char *endp;
struct backlight_device *bd = to_backlight_device(cdev);
int brightness = simple_strtoul(buf, &endp, 0);
@@ -79,7 +90,10 @@ static ssize_t backlight_store_brightnes
if (size != count)
return -EINVAL;
- down(&bd->sem);
+ rc = mutex_lock_interruptible(&bd->mutex);
+ if (rc)
+ return rc;
+
if (bd->props) {
if (brightness > bd->props->max_brightness)
rc = -EINVAL;
@@ -91,21 +105,26 @@ static ssize_t backlight_store_brightnes
bd->props->update_status(bd);
rc = count;
}
- }
- up(&bd->sem);
+ } else
+ rc = -ENXIO;
+
+ mutex_unlock(&bd->mutex);
return rc;
}
static ssize_t backlight_show_max_brightness(struct class_device *cdev, char *buf)
{
- int rc = -ENXIO;
+ int rc;
struct backlight_device *bd = to_backlight_device(cdev);
- down(&bd->sem);
- if (bd->props)
- rc = sprintf(buf, "%d\n", bd->props->max_brightness);
- up(&bd->sem);
+ rc = mutex_lock_interruptible(&bd->mutex);
+ if (rc)
+ return rc;
+
+ rc = bd->props ? sprintf(buf, "%d\n", bd->props->max_brightness) : -ENXIO;
+
+ mutex_unlock(&bd->mutex);
return rc;
}
@@ -113,13 +132,19 @@ static ssize_t backlight_show_max_bright
static ssize_t backlight_show_actual_brightness(struct class_device *cdev,
char *buf)
{
- int rc = -ENXIO;
+ int rc;
struct backlight_device *bd = to_backlight_device(cdev);
- down(&bd->sem);
+ rc = mutex_lock_interruptible(&bd->mutex);
+ if (rc)
+ return rc;
+
if (bd->props && bd->props->get_brightness)
rc = sprintf(buf, "%d\n", bd->props->get_brightness(bd));
- up(&bd->sem);
+ else
+ rc = -ENXIO;
+
+ mutex_unlock(&bd->mutex);
return rc;
}
@@ -161,7 +186,8 @@ static int fb_notifier_callback(struct n
return 0;
bd = container_of(self, struct backlight_device, fb_notif);
- down(&bd->sem);
+
+ mutex_lock(&bd->mutex);
if (bd->props) {
if (!bd->props->check_fb ||
bd->props->check_fb(evdata->info)) {
@@ -170,7 +196,8 @@ static int fb_notifier_callback(struct n
bd->props->update_status(bd);
}
}
- up(&bd->sem);
+ mutex_unlock(&bd->mutex);
+
return 0;
}
@@ -198,7 +225,7 @@ struct backlight_device *backlight_devic
if (!new_bd)
return ERR_PTR(-ENOMEM);
- init_MUTEX(&new_bd->sem);
+ mutex_init(&new_bd->mutex);
new_bd->props = bp;
new_bd->class_dev.class = &backlight_class;
strlcpy(new_bd->class_dev.class_id, name, KOBJ_NAME_LEN);
@@ -235,7 +262,7 @@ void backlight_device_unregister(struct
pr_debug("backlight_device_unregister: name=%s\n", bd->class_dev.class_id);
- down(&bd->sem);
+ mutex_lock(&bd->mutex);
if (bd->props && bd->props->update_status) {
bd->props->brightness = 0;
bd->props->power = 0;
@@ -243,7 +270,7 @@ void backlight_device_unregister(struct
}
bd->props = NULL;
- up(&bd->sem);
+ mutex_unlock(&bd->mutex);
fb_unregister_client(&bd->fb_notif);
class_device_unregister(&bd->class_dev);
Index: work/drivers/video/backlight/lcd.c
===================================================================
--- work.orig/drivers/video/backlight/lcd.c
+++ work/drivers/video/backlight/lcd.c
@@ -16,20 +16,26 @@
static ssize_t lcd_show_power(struct class_device *cdev, char *buf)
{
- int rc = -ENXIO;
+ int rc;
struct lcd_device *ld = to_lcd_device(cdev);
- down(&ld->sem);
+ rc = mutex_lock_interruptible(&ld->mutex);
+ if (rc)
+ return rc;
+
if (ld->props && ld->props->get_power)
rc = sprintf(buf, "%d\n", ld->props->get_power(ld));
- up(&ld->sem);
+ else
+ rc = -ENXIO;
+
+ mutex_unlock(&ld->mutex);
return rc;
}
static ssize_t lcd_store_power(struct class_device *cdev, const char *buf, size_t count)
{
- int rc = -ENXIO;
+ int rc;
char *endp;
struct lcd_device *ld = to_lcd_device(cdev);
int power = simple_strtoul(buf, &endp, 0);
@@ -40,33 +46,44 @@ static ssize_t lcd_store_power(struct cl
if (size != count)
return -EINVAL;
- down(&ld->sem);
+ rc = mutex_lock_interruptible(&ld->mutex);
+ if (rc)
+ return rc;
+
if (ld->props && ld->props->set_power) {
pr_debug("lcd: set power to %d\n", power);
ld->props->set_power(ld, power);
rc = count;
- }
- up(&ld->sem);
+ } else
+ rc = -ENXIO;
+
+ mutex_unlock(&ld->mutex);
return rc;
}
static ssize_t lcd_show_contrast(struct class_device *cdev, char *buf)
{
- int rc = -ENXIO;
+ int rc;
struct lcd_device *ld = to_lcd_device(cdev);
- down(&ld->sem);
+ rc = mutex_lock_interruptible(&ld->mutex);
+ if (rc)
+ return rc;
+
if (ld->props && ld->props->get_contrast)
rc = sprintf(buf, "%d\n", ld->props->get_contrast(ld));
- up(&ld->sem);
+ else
+ rc = -ENXIO;
+
+ mutex_unlock(&ld->mutex);
return rc;
}
static ssize_t lcd_store_contrast(struct class_device *cdev, const char *buf, size_t count)
{
- int rc = -ENXIO;
+ int rc;
char *endp;
struct lcd_device *ld = to_lcd_device(cdev);
int contrast = simple_strtoul(buf, &endp, 0);
@@ -77,13 +94,18 @@ static ssize_t lcd_store_contrast(struct
if (size != count)
return -EINVAL;
- down(&ld->sem);
+ rc = mutex_lock_interruptible(&ld->mutex);
+ if (rc)
+ return rc;
+
if (ld->props && ld->props->set_contrast) {
pr_debug("lcd: set contrast to %d\n", contrast);
ld->props->set_contrast(ld, contrast);
rc = count;
- }
- up(&ld->sem);
+ } else
+ rc = -ENXIO;
+
+ mutex_unlock(&ld->mutex);
return rc;
}
@@ -93,10 +115,16 @@ static ssize_t lcd_show_max_contrast(str
int rc = -ENXIO;
struct lcd_device *ld = to_lcd_device(cdev);
- down(&ld->sem);
+ rc = mutex_lock_interruptible(&ld->mutex);
+ if (rc)
+ return rc;
+
if (ld->props)
rc = sprintf(buf, "%d\n", ld->props->max_contrast);
- up(&ld->sem);
+ else
+ rc = -ENXIO;
+
+ mutex_unlock(&ld->mutex);
return rc;
}
@@ -104,6 +132,7 @@ static ssize_t lcd_show_max_contrast(str
static void lcd_class_release(struct class_device *dev)
{
struct lcd_device *ld = to_lcd_device(dev);
+
kfree(ld);
}
@@ -135,11 +164,13 @@ static int fb_notifier_callback(struct n
return 0;
ld = container_of(self, struct lcd_device, fb_notif);
- down(&ld->sem);
+
+ mutex_lock(&ld->mutex);
if (ld->props)
if (!ld->props->check_fb || ld->props->check_fb(evdata->info))
ld->props->set_power(ld, *(int *)evdata->data);
- up(&ld->sem);
+ mutex_unlock(&ld->mutex);
+
return 0;
}
@@ -166,7 +197,7 @@ struct lcd_device *lcd_device_register(c
if (!new_ld)
return ERR_PTR(-ENOMEM);
- init_MUTEX(&new_ld->sem);
+ mutex_init(&new_ld->mutex);
new_ld->props = lp;
new_ld->class_dev.class = &lcd_class;
strlcpy(new_ld->class_dev.class_id, name, KOBJ_NAME_LEN);
@@ -203,9 +234,9 @@ void lcd_device_unregister(struct lcd_de
pr_debug("lcd_device_unregister: name=%s\n", ld->class_dev.class_id);
- down(&ld->sem);
+ mutex_lock(&ld->mutex);
ld->props = NULL;
- up(&ld->sem);
+ mutex_unlock(&ld->mutex);
fb_unregister_client(&ld->fb_notif);
class_device_unregister(&ld->class_dev);
Index: work/include/linux/backlight.h
===================================================================
--- work.orig/include/linux/backlight.h
+++ work/include/linux/backlight.h
@@ -41,7 +41,7 @@ struct backlight_device {
/* This protects the 'props' field. If 'props' is NULL, the driver that
registered this device has been unloaded, and if class_get_devdata()
points to something in the body of that driver, it is also invalid. */
- struct semaphore sem;
+ struct mutex mutex;
/* If this is NULL, the backing module is unloaded */
struct backlight_properties *props;
/* The framebuffer notifier block */
Index: work/include/linux/lcd.h
===================================================================
--- work.orig/include/linux/lcd.h
+++ work/include/linux/lcd.h
@@ -36,7 +36,7 @@ struct lcd_device {
/* This protects the 'props' field. If 'props' is NULL, the driver that
registered this device has been unloaded, and if class_get_devdata()
points to something in the body of that driver, it is also invalid. */
- struct semaphore sem;
+ struct mutex mutex;
/* If this is NULL, the backing module is unloaded */
struct lcd_properties *props;
/* The framebuffer notifier block */
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [patch 5/6] Convert to use mutexes instead of semaphores
2006-08-11 5:03 ` [patch 5/6] Convert to use mutexes instead of semaphores Dmitry Torokhov
@ 2006-08-11 12:58 ` Dmitry Torokhov
2006-08-11 13:16 ` Richard Purdie
0 siblings, 1 reply; 19+ messages in thread
From: Dmitry Torokhov @ 2006-08-11 12:58 UTC (permalink / raw)
To: Richard Purdie; +Cc: LKML, Michael Hanselmann, Antonino A. Daplas
On 8/11/06, Dmitry Torokhov <dtor@insightbb.com> wrote:
> Backlight: convert to use mutexes instead of semaphores
>
Apparently I missed that several drivers also use bd->sem so they need
to be converted too... But what is it with the drivers:
static void aty128_bl_set_power(struct fb_info *info, int power)
{
mutex_lock(&info->bl_mutex);
up(&info->bl_dev->sem);
info->bl_dev->props->power = power;
__aty128_bl_update_status(info->bl_dev);
down(&info->bl_dev->sem);
mutex_unlock(&info->bl_mutex);
}
Why we are doing up() before down()??? And it is in almost every
driver that uses backlight... Do I need more coffee? [CC-ing bunch of
people trying to get an answer...]
--
Dmitry
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [patch 5/6] Convert to use mutexes instead of semaphores
2006-08-11 12:58 ` Dmitry Torokhov
@ 2006-08-11 13:16 ` Richard Purdie
2006-08-11 13:34 ` Dmitry Torokhov
0 siblings, 1 reply; 19+ messages in thread
From: Richard Purdie @ 2006-08-11 13:16 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: LKML, Michael Hanselmann, Antonino A. Daplas
On Fri, 2006-08-11 at 08:58 -0400, Dmitry Torokhov wrote:
> On 8/11/06, Dmitry Torokhov <dtor@insightbb.com> wrote:
> > Backlight: convert to use mutexes instead of semaphores
> >
>
> Apparently I missed that several drivers also use bd->sem so they need
> to be converted too... But what is it with the drivers:
>
> static void aty128_bl_set_power(struct fb_info *info, int power)
> {
> mutex_lock(&info->bl_mutex);
> up(&info->bl_dev->sem);
> info->bl_dev->props->power = power;
> __aty128_bl_update_status(info->bl_dev);
> down(&info->bl_dev->sem);
> mutex_unlock(&info->bl_mutex);
> }
>
> Why we are doing up() before down()??? And it is in almost every
> driver that uses backlight... Do I need more coffee? [CC-ing bunch of
> people trying to get an answer...]
It looks totally wrong.
In the archives, there are a number of comments from me questioning
whether that driver needs to touch bl_dev->sem anyway (esp. given the
mutex as well). I never did find out what it was trying to protect
against...
Richard
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [patch 5/6] Convert to use mutexes instead of semaphores
2006-08-11 13:16 ` Richard Purdie
@ 2006-08-11 13:34 ` Dmitry Torokhov
2006-08-11 13:42 ` Michael Hanselmann
0 siblings, 1 reply; 19+ messages in thread
From: Dmitry Torokhov @ 2006-08-11 13:34 UTC (permalink / raw)
To: Richard Purdie; +Cc: LKML, Michael Hanselmann, Antonino A. Daplas
On 8/11/06, Richard Purdie <rpurdie@rpsys.net> wrote:
> On Fri, 2006-08-11 at 08:58 -0400, Dmitry Torokhov wrote:
> > On 8/11/06, Dmitry Torokhov <dtor@insightbb.com> wrote:
> > > Backlight: convert to use mutexes instead of semaphores
> > >
> >
> > Apparently I missed that several drivers also use bd->sem so they need
> > to be converted too... But what is it with the drivers:
> >
> > static void aty128_bl_set_power(struct fb_info *info, int power)
> > {
> > mutex_lock(&info->bl_mutex);
> > up(&info->bl_dev->sem);
> > info->bl_dev->props->power = power;
> > __aty128_bl_update_status(info->bl_dev);
> > down(&info->bl_dev->sem);
> > mutex_unlock(&info->bl_mutex);
> > }
> >
> > Why we are doing up() before down()??? And it is in almost every
> > driver that uses backlight... Do I need more coffee? [CC-ing bunch of
> > people trying to get an answer...]
>
> It looks totally wrong.
>
Ok, so that is not only me seeing things ;)
> In the archives, there are a number of comments from me questioning
> whether that driver needs to touch bl_dev->sem anyway (esp. given the
> mutex as well). I never did find out what it was trying to protect
> against...
I think it is prudent to protect assess to these data structures. For
example, it could possibly race with setting power through sysfs
attribute. Now for this particular driver the race window is
non-existent for all practical purposes, but it looks like atyfb_base
might be needig it (of course currentimplementation not only have
up/down mixed but also has AB-BA deadlock).
How about we add backlight_set_power(&bd, power) to the backlight core
to take care of proper locking for drivers?
--
Dmitry
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [patch 5/6] Convert to use mutexes instead of semaphores
2006-08-11 13:34 ` Dmitry Torokhov
@ 2006-08-11 13:42 ` Michael Hanselmann
2006-08-11 14:07 ` Dmitry Torokhov
0 siblings, 1 reply; 19+ messages in thread
From: Michael Hanselmann @ 2006-08-11 13:42 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Richard Purdie, LKML, Antonino A. Daplas
[-- Attachment #1: Type: text/plain, Size: 534 bytes --]
On Fri, Aug 11, 2006 at 09:34:44AM -0400, Dmitry Torokhov wrote:
> How about we add backlight_set_power(&bd, power) to the backlight core
> to take care of proper locking for drivers?
I've tried to add several functions to the backlight core
({s,g}et_{brightness,power}) and they were rejected. Thus all the
locking is spread over the drivers. I agree it's faulty right now.
It's still easier to move to backlight core functions than to fix all
the drivers.
Because I am responsible/wrote for the broken code, how should I
proceed?
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [patch 5/6] Convert to use mutexes instead of semaphores
2006-08-11 13:42 ` Michael Hanselmann
@ 2006-08-11 14:07 ` Dmitry Torokhov
2006-08-11 16:45 ` Richard Purdie
0 siblings, 1 reply; 19+ messages in thread
From: Dmitry Torokhov @ 2006-08-11 14:07 UTC (permalink / raw)
To: Michael Hanselmann; +Cc: Richard Purdie, LKML, Antonino A. Daplas
On 8/11/06, Michael Hanselmann <linux-kernel@hansmi.ch> wrote:
> On Fri, Aug 11, 2006 at 09:34:44AM -0400, Dmitry Torokhov wrote:
> > How about we add backlight_set_power(&bd, power) to the backlight core
> > to take care of proper locking for drivers?
>
> I've tried to add several functions to the backlight core
> ({s,g}et_{brightness,power}) and they were rejected. Thus all the
> locking is spread over the drivers. I agree it's faulty right now.
> It's still easier to move to backlight core functions than to fix all
> the drivers.
>
> Because I am responsible/wrote for the broken code, how should I
> proceed?
>
Well, I was reading some more of the drivers and I am also not sure if
such methods are needed in backlight core. Let's take atyfb_base.c -
it tries to manipulate backlight's power from atyfb_blank. But it is
normally called from fb_blank() which is then calls
fb_notifier_call_chain(FB_EVENT_BLANK, &event);
So on the end backlight device will get that event and will turn off
power anyway. Now, atyfb_blank is also called suring suspend/resume so
we probably should just add handling of FB_EVENT_SUSPEND and
FB_EVENT_RESUME to the backlight core.
Richard?
--
Dmitry
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [patch 5/6] Convert to use mutexes instead of semaphores
2006-08-11 14:07 ` Dmitry Torokhov
@ 2006-08-11 16:45 ` Richard Purdie
2006-08-11 17:20 ` Dmitry Torokhov
2006-08-29 20:54 ` Michael Hanselmann
0 siblings, 2 replies; 19+ messages in thread
From: Richard Purdie @ 2006-08-11 16:45 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Michael Hanselmann, LKML, Antonino A. Daplas
On Fri, 2006-08-11 at 10:07 -0400, Dmitry Torokhov wrote:
> On 8/11/06, Michael Hanselmann <linux-kernel@hansmi.ch> wrote:
> > On Fri, Aug 11, 2006 at 09:34:44AM -0400, Dmitry Torokhov wrote:
> > > How about we add backlight_set_power(&bd, power) to the backlight core
> > > to take care of proper locking for drivers?
A couple of patches were attempted for this but they didn't solve the
underlying races. The main reason was a lack of understanding of what
the existing backlight lock protects and trying to make it do two thinsg
at once.
> > I've tried to add several functions to the backlight core
> > ({s,g}et_{brightness,power}) and they were rejected. Thus all the
> > locking is spread over the drivers. I agree it's faulty right now.
> > It's still easier to move to backlight core functions than to fix all
> > the drivers.
If we can find a way to safely do the locking in the backlight core I
agree.
> > Because I am responsible/wrote for the broken code, how should I
> > proceed?
First, we need to define the potential problems. Dimitry mentioned: "For
example, it could possibly race with setting power through sysfs
attribute". This is not what the lock in the backlight core is for
though. To quote backlight.h:
/* This protects the 'props' field. If 'props' is NULL, the driver that
registered this device has been unloaded, and if class_get_devdata()
points to something in the body of that driver, it is also invalid.
*/
My previous patches have gone a long way to removing race issues. The
need for the existing lock comes from backlight_device_unregister()
which basically does:
class_device_remove_files()
bd->props->brightness = 0;
bd->props->power = 0;
bd->props->update_status(bd);
bd->props = NULL
fb_unregister_client()
class_device_unregister()
If we could guarantee that after class_device_unregister(), nothing was
still executing any of the show/store methods, we'd be fine (the
fb_notifier is safe). As I understand the class device and sysfs
attributes, we can't guarantee that though. I'd appreciate comments from
the device model people as I could be wrong about this. The owner field
also can't help us.
Dimitry's "Backlight: convert to use default class device attributes"
patch should really mean the class_device_unregister() call is moved to
earlier in the function to try and avoid races from the attributes but
it still doesn't guarantee anything.
If we could somehow sync class_device_unregister(), we could get rid of
that semaphore entirely.
Regardless, if we want to add locking for synchronising the attributes
into the core, we need a different lock. I did think the drivers would
be able to handle this themselves with locking inside update_status if
needed but I can see why certain drivers might not like that.
> Well, I was reading some more of the drivers and I am also not sure if
> such methods are needed in backlight core. Let's take atyfb_base.c -
> it tries to manipulate backlight's power from atyfb_blank. But it is
> normally called from fb_blank() which is then calls
> fb_notifier_call_chain(FB_EVENT_BLANK, &event);
> So on the end backlight device will get that event and will turn off
> power anyway. Now, atyfb_blank is also called suring suspend/resume so
> we probably should just add handling of FB_EVENT_SUSPEND and
> FB_EVENT_RESUME to the backlight core.
>
> Richard?
Think about the case where you have 2 framebuffers. The notification
call was left to pass to the driver as only it can work out which
framebuffer a given backlight is attached to.
Cheers,
Richard
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [patch 5/6] Convert to use mutexes instead of semaphores
2006-08-11 16:45 ` Richard Purdie
@ 2006-08-11 17:20 ` Dmitry Torokhov
2006-08-29 20:54 ` Michael Hanselmann
1 sibling, 0 replies; 19+ messages in thread
From: Dmitry Torokhov @ 2006-08-11 17:20 UTC (permalink / raw)
To: Richard Purdie; +Cc: Michael Hanselmann, LKML, Antonino A. Daplas
On 8/11/06, Richard Purdie <rpurdie@rpsys.net> wrote:
> On Fri, 2006-08-11 at 10:07 -0400, Dmitry Torokhov wrote:
> > On 8/11/06, Michael Hanselmann <linux-kernel@hansmi.ch> wrote:
> > > On Fri, Aug 11, 2006 at 09:34:44AM -0400, Dmitry Torokhov wrote:
> > > > How about we add backlight_set_power(&bd, power) to the backlight core
> > > > to take care of proper locking for drivers?
>
> A couple of patches were attempted for this but they didn't solve the
> underlying races. The main reason was a lack of understanding of what
> the existing backlight lock protects and trying to make it do two thinsg
> at once.
>
> > > I've tried to add several functions to the backlight core
> > > ({s,g}et_{brightness,power}) and they were rejected. Thus all the
> > > locking is spread over the drivers. I agree it's faulty right now.
> > > It's still easier to move to backlight core functions than to fix all
> > > the drivers.
>
> If we can find a way to safely do the locking in the backlight core I
> agree.
>
> > > Because I am responsible/wrote for the broken code, how should I
> > > proceed?
>
> First, we need to define the potential problems. Dimitry mentioned: "For
> example, it could possibly race with setting power through sysfs
> attribute". This is not what the lock in the backlight core is for
> though. To quote backlight.h:
>
> /* This protects the 'props' field. If 'props' is NULL, the driver that
> registered this device has been unloaded, and if class_get_devdata()
> points to something in the body of that driver, it is also invalid.
> */
Yes, you are right. As long as ->update_status() method serializes
access to the underlying hardware by itself we don't need locking in
backlight core. If we have several writes one of them will win but we
will never have kernel and hardware disagree about the state they are
in. So we can just remove references to baccklight's semaphore from
drivers.
>
> Dimitry's "Backlight: convert to use default class device attributes"
> patch should really mean the class_device_unregister() call is moved to
> earlier in the function to try and avoid races from the attributes but
> it still doesn't guarantee anything.
>
No, it was not the intent of the patch. I was just trying to remove
unneeded code and simplify error handling because driver core can do
that for us. The race window is still present and mutex is still
needed.
>
> > Well, I was reading some more of the drivers and I am also not sure if
> > such methods are needed in backlight core. Let's take atyfb_base.c -
> > it tries to manipulate backlight's power from atyfb_blank. But it is
> > normally called from fb_blank() which is then calls
> > fb_notifier_call_chain(FB_EVENT_BLANK, &event);
> > So on the end backlight device will get that event and will turn off
> > power anyway. Now, atyfb_blank is also called suring suspend/resume so
> > we probably should just add handling of FB_EVENT_SUSPEND and
> > FB_EVENT_RESUME to the backlight core.
> >
> > Richard?
>
> Think about the case where you have 2 framebuffers. The notification
> call was left to pass to the driver as only it can work out which
> framebuffer a given backlight is attached to.
>
I am not sure I follow... It would end up in the driver, like
FB_EVENT_BLANK ultimately does. RIght now FB_EVENT_SUSPEND and
FB_EVENT_RESUME are dropped by the blacklight core.
But it does not matter now - if we do not fiddle with backlight's
locks we can continue switching backlight off in drivers.
--
Dmitry
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [patch 5/6] Convert to use mutexes instead of semaphores
2006-08-11 16:45 ` Richard Purdie
2006-08-11 17:20 ` Dmitry Torokhov
@ 2006-08-29 20:54 ` Michael Hanselmann
1 sibling, 0 replies; 19+ messages in thread
From: Michael Hanselmann @ 2006-08-29 20:54 UTC (permalink / raw)
To: Richard Purdie; +Cc: Dmitry Torokhov, LKML, Antonino A. Daplas
On Fri, Aug 11, 2006 at 05:45:51PM +0100, Richard Purdie wrote:
> On Fri, 2006-08-11 at 10:07 -0400, Dmitry Torokhov wrote:
> > On 8/11/06, Michael Hanselmann <linux-kernel@hansmi.ch> wrote:
> > > Because I am responsible/wrote for the broken code, how should I
> > > proceed?
Somehow this got lost, sorry. The patch below fixes at least the
wrongly ordered up()/down() calls. I know there are outstanding with the
backlight code in general, but those issues aren't that easy to fix.
Is it okay? If yes, I'm going to send it to akpm.
Signed-off-by: Michael Hanselmann <linux-kernel@hansmi.ch>
---
diff -Nrup --exclude-from linux-exclude-from linux-2.6.18-rc5.orig/drivers/macintosh/via-pmu-backlight.c linux-2.6.18-rc5/drivers/macintosh/via-pmu-backlight.c
--- linux-2.6.18-rc5.orig/drivers/macintosh/via-pmu-backlight.c 2006-08-29 22:27:01.000000000 +0200
+++ linux-2.6.18-rc5/drivers/macintosh/via-pmu-backlight.c 2006-08-29 22:40:58.000000000 +0200
@@ -168,11 +168,11 @@ void __init pmu_backlight_init()
mutex_unlock(&info->bl_mutex);
}
- up(&bd->sem);
+ down(&bd->sem);
bd->props->brightness = level;
bd->props->power = FB_BLANK_UNBLANK;
bd->props->update_status(bd);
- down(&bd->sem);
+ up(&bd->sem);
mutex_lock(&pmac_backlight_mutex);
if (!pmac_backlight)
diff -Nrup --exclude-from linux-exclude-from linux-2.6.18-rc5.orig/drivers/video/aty/aty128fb.c linux-2.6.18-rc5/drivers/video/aty/aty128fb.c
--- linux-2.6.18-rc5.orig/drivers/video/aty/aty128fb.c 2006-08-29 22:27:01.000000000 +0200
+++ linux-2.6.18-rc5/drivers/video/aty/aty128fb.c 2006-08-29 22:41:24.000000000 +0200
@@ -1801,10 +1801,10 @@ static struct backlight_properties aty12
static void aty128_bl_set_power(struct fb_info *info, int power)
{
mutex_lock(&info->bl_mutex);
- up(&info->bl_dev->sem);
+ down(&info->bl_dev->sem);
info->bl_dev->props->power = power;
__aty128_bl_update_status(info->bl_dev);
- down(&info->bl_dev->sem);
+ up(&info->bl_dev->sem);
mutex_unlock(&info->bl_mutex);
}
@@ -1839,11 +1839,11 @@ static void aty128_bl_init(struct aty128
219 * FB_BACKLIGHT_MAX / MAX_LEVEL);
mutex_unlock(&info->bl_mutex);
- up(&bd->sem);
+ down(&bd->sem);
bd->props->brightness = aty128_bl_data.max_brightness;
bd->props->power = FB_BLANK_UNBLANK;
bd->props->update_status(bd);
- down(&bd->sem);
+ up(&bd->sem);
#ifdef CONFIG_PMAC_BACKLIGHT
mutex_lock(&pmac_backlight_mutex);
diff -Nrup --exclude-from linux-exclude-from linux-2.6.18-rc5.orig/drivers/video/aty/atyfb_base.c linux-2.6.18-rc5/drivers/video/aty/atyfb_base.c
--- linux-2.6.18-rc5.orig/drivers/video/aty/atyfb_base.c 2006-08-29 22:27:01.000000000 +0200
+++ linux-2.6.18-rc5/drivers/video/aty/atyfb_base.c 2006-08-29 22:41:47.000000000 +0200
@@ -2200,10 +2200,10 @@ static struct backlight_properties aty_b
static void aty_bl_set_power(struct fb_info *info, int power)
{
mutex_lock(&info->bl_mutex);
- up(&info->bl_dev->sem);
+ down(&info->bl_dev->sem);
info->bl_dev->props->power = power;
__aty_bl_update_status(info->bl_dev);
- down(&info->bl_dev->sem);
+ up(&info->bl_dev->sem);
mutex_unlock(&info->bl_mutex);
}
@@ -2234,11 +2234,11 @@ static void aty_bl_init(struct atyfb_par
0xFF * FB_BACKLIGHT_MAX / MAX_LEVEL);
mutex_unlock(&info->bl_mutex);
- up(&bd->sem);
+ down(&bd->sem);
bd->props->brightness = aty_bl_data.max_brightness;
bd->props->power = FB_BLANK_UNBLANK;
bd->props->update_status(bd);
- down(&bd->sem);
+ up(&bd->sem);
#ifdef CONFIG_PMAC_BACKLIGHT
mutex_lock(&pmac_backlight_mutex);
diff -Nrup --exclude-from linux-exclude-from linux-2.6.18-rc5.orig/drivers/video/aty/radeon_backlight.c linux-2.6.18-rc5/drivers/video/aty/radeon_backlight.c
--- linux-2.6.18-rc5.orig/drivers/video/aty/radeon_backlight.c 2006-08-29 22:27:01.000000000 +0200
+++ linux-2.6.18-rc5/drivers/video/aty/radeon_backlight.c 2006-08-29 22:39:23.000000000 +0200
@@ -195,11 +195,11 @@ void radeonfb_bl_init(struct radeonfb_in
217 * FB_BACKLIGHT_MAX / MAX_RADEON_LEVEL);
mutex_unlock(&rinfo->info->bl_mutex);
- up(&bd->sem);
+ down(&bd->sem);
bd->props->brightness = radeon_bl_data.max_brightness;
bd->props->power = FB_BLANK_UNBLANK;
bd->props->update_status(bd);
- down(&bd->sem);
+ up(&bd->sem);
#ifdef CONFIG_PMAC_BACKLIGHT
mutex_lock(&pmac_backlight_mutex);
diff -Nrup --exclude-from linux-exclude-from linux-2.6.18-rc5.orig/drivers/video/nvidia/nv_backlight.c linux-2.6.18-rc5/drivers/video/nvidia/nv_backlight.c
--- linux-2.6.18-rc5.orig/drivers/video/nvidia/nv_backlight.c 2006-08-29 22:27:01.000000000 +0200
+++ linux-2.6.18-rc5/drivers/video/nvidia/nv_backlight.c 2006-08-29 22:43:03.000000000 +0200
@@ -113,10 +113,10 @@ static struct backlight_properties nvidi
void nvidia_bl_set_power(struct fb_info *info, int power)
{
mutex_lock(&info->bl_mutex);
- up(&info->bl_dev->sem);
+ down(&info->bl_dev->sem);
info->bl_dev->props->power = power;
__nvidia_bl_update_status(info->bl_dev);
- down(&info->bl_dev->sem);
+ up(&info->bl_dev->sem);
mutex_unlock(&info->bl_mutex);
}
@@ -151,11 +151,11 @@ void nvidia_bl_init(struct nvidia_par *p
0x534 * FB_BACKLIGHT_MAX / MAX_LEVEL);
mutex_unlock(&info->bl_mutex);
- up(&bd->sem);
+ down(&bd->sem);
bd->props->brightness = nvidia_bl_data.max_brightness;
bd->props->power = FB_BLANK_UNBLANK;
bd->props->update_status(bd);
- down(&bd->sem);
+ up(&bd->sem);
#ifdef CONFIG_PMAC_BACKLIGHT
mutex_lock(&pmac_backlight_mutex);
diff -Nrup --exclude-from linux-exclude-from linux-2.6.18-rc5.orig/drivers/video/riva/fbdev.c linux-2.6.18-rc5/drivers/video/riva/fbdev.c
--- linux-2.6.18-rc5.orig/drivers/video/riva/fbdev.c 2006-08-29 22:27:01.000000000 +0200
+++ linux-2.6.18-rc5/drivers/video/riva/fbdev.c 2006-08-29 22:43:26.000000000 +0200
@@ -355,10 +355,10 @@ static struct backlight_properties riva_
static void riva_bl_set_power(struct fb_info *info, int power)
{
mutex_lock(&info->bl_mutex);
- up(&info->bl_dev->sem);
+ down(&info->bl_dev->sem);
info->bl_dev->props->power = power;
__riva_bl_update_status(info->bl_dev);
- down(&info->bl_dev->sem);
+ up(&info->bl_dev->sem);
mutex_unlock(&info->bl_mutex);
}
@@ -393,11 +393,11 @@ static void riva_bl_init(struct riva_par
0x534 * FB_BACKLIGHT_MAX / MAX_LEVEL);
mutex_unlock(&info->bl_mutex);
- up(&bd->sem);
+ down(&bd->sem);
bd->props->brightness = riva_bl_data.max_brightness;
bd->props->power = FB_BLANK_UNBLANK;
bd->props->update_status(bd);
- down(&bd->sem);
+ up(&bd->sem);
#ifdef CONFIG_PMAC_BACKLIGHT
mutex_lock(&pmac_backlight_mutex);
^ permalink raw reply [flat|nested] 19+ messages in thread
* [patch 6/6] Move per-device data out of backlight_properties
2006-08-11 5:03 [patch 0/6] Backlight & lcd fixes/cleanups Dmitry Torokhov
` (4 preceding siblings ...)
2006-08-11 5:03 ` [patch 5/6] Convert to use mutexes instead of semaphores Dmitry Torokhov
@ 2006-08-11 5:03 ` Dmitry Torokhov
2006-08-11 8:02 ` Richard Purdie
5 siblings, 1 reply; 19+ messages in thread
From: Dmitry Torokhov @ 2006-08-11 5:03 UTC (permalink / raw)
To: Richard Purdie; +Cc: LKML
[-- Attachment #1: backlight-move-data.patch --]
[-- Type: text/plain, Size: 6618 bytes --]
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 <dtor@mail.ru>
---
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)
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [patch 6/6] Move per-device data out of backlight_properties
2006-08-11 5:03 ` [patch 6/6] Move per-device data out of backlight_properties Dmitry Torokhov
@ 2006-08-11 8:02 ` Richard Purdie
2006-08-11 12:27 ` Dmitry Torokhov
0 siblings, 1 reply; 19+ messages in thread
From: Richard Purdie @ 2006-08-11 8:02 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: LKML
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [patch 6/6] Move per-device data out of backlight_properties
2006-08-11 8:02 ` Richard Purdie
@ 2006-08-11 12:27 ` Dmitry Torokhov
2006-08-11 12:55 ` Richard Purdie
0 siblings, 1 reply; 19+ messages in thread
From: Dmitry Torokhov @ 2006-08-11 12:27 UTC (permalink / raw)
To: Richard Purdie; +Cc: LKML
On 8/11/06, Richard Purdie <rpurdie@rpsys.net> 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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [patch 6/6] Move per-device data out of backlight_properties
2006-08-11 12:27 ` Dmitry Torokhov
@ 2006-08-11 12:55 ` Richard Purdie
2006-08-11 13:10 ` Dmitry Torokhov
0 siblings, 1 reply; 19+ messages in thread
From: Richard Purdie @ 2006-08-11 12:55 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: LKML
On Fri, 2006-08-11 at 08:27 -0400, Dmitry Torokhov wrote:
> On 8/11/06, Richard Purdie <rpurdie@rpsys.net> 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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [patch 6/6] Move per-device data out of backlight_properties
2006-08-11 12:55 ` Richard Purdie
@ 2006-08-11 13:10 ` Dmitry Torokhov
0 siblings, 0 replies; 19+ messages in thread
From: Dmitry Torokhov @ 2006-08-11 13:10 UTC (permalink / raw)
To: Richard Purdie; +Cc: LKML
On 8/11/06, Richard Purdie <rpurdie@rpsys.net> wrote:
> On Fri, 2006-08-11 at 08:27 -0400, Dmitry Torokhov wrote:
> > On 8/11/06, Richard Purdie <rpurdie@rpsys.net> 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
^ permalink raw reply [flat|nested] 19+ messages in thread