| Message ID | 20240313154857.12949-4-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | backlight: Remove struct backlight_properties.fb_blank | expand |
Hi Thomas, On Wed, Mar 13, 2024 at 04:45:02PM +0100, Thomas Zimmermann wrote: > The backlight is on for fb_blank eq FB_BLANK_UNBLANK, or off for > any other value in fb_blank. But the field fb_blank in struct > backlight_properties is deprecated and should not be used any > longer. > > Replace the test for fb_blank in omap's backlight code with a > simple boolean parameter and push the test into the update_status > helper. Instead of reading fb_blank directly, decode the backlight > device's status with backlight_is_blank(). > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> My biased opinion is that the approach in this patch is a little bit better: https://lore.kernel.org/lkml/20230107-sam-video-backlight-drop-fb_blank-v1-13-1bd9bafb351f@ravnborg.org/ I never came around resending this series it seems. Sam
Hi Am 13.03.24 um 19:00 schrieb Sam Ravnborg: > Hi Thomas, > > On Wed, Mar 13, 2024 at 04:45:02PM +0100, Thomas Zimmermann wrote: >> The backlight is on for fb_blank eq FB_BLANK_UNBLANK, or off for >> any other value in fb_blank. But the field fb_blank in struct >> backlight_properties is deprecated and should not be used any >> longer. >> >> Replace the test for fb_blank in omap's backlight code with a >> simple boolean parameter and push the test into the update_status >> helper. Instead of reading fb_blank directly, decode the backlight >> device's status with backlight_is_blank(). >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > My biased opinion is that the approach in this patch is a little bit > better: > https://lore.kernel.org/lkml/20230107-sam-video-backlight-drop-fb_blank-v1-13-1bd9bafb351f@ravnborg.org/ > > I never came around resending this series it seems. Oh, that series has been around for over a year. I don't care about which patches go in as long as they remove the dependency on <linux/fb.h>. I saw that Dan has already r-b'ed the current patchset, but if you prefer I'll adopt yours. Best regards Thomas > > Sam
On Thu, Mar 14, 2024 at 09:16:15AM +0100, Thomas Zimmermann wrote: > Hi > > Am 13.03.24 um 19:00 schrieb Sam Ravnborg: > > Hi Thomas, > > > > On Wed, Mar 13, 2024 at 04:45:02PM +0100, Thomas Zimmermann wrote: > > > The backlight is on for fb_blank eq FB_BLANK_UNBLANK, or off for > > > any other value in fb_blank. But the field fb_blank in struct > > > backlight_properties is deprecated and should not be used any > > > longer. > > > > > > Replace the test for fb_blank in omap's backlight code with a > > > simple boolean parameter and push the test into the update_status > > > helper. Instead of reading fb_blank directly, decode the backlight > > > device's status with backlight_is_blank(). > > > > > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > > My biased opinion is that the approach in this patch is a little bit > > better: > > https://lore.kernel.org/lkml/20230107-sam-video-backlight-drop-fb_blank-v1-13-1bd9bafb351f@ravnborg.org/ > > > > I never came around resending this series it seems. > > Oh, that series has been around for over a year. I don't care about which > patches go in as long as they remove the dependency on <linux/fb.h>. I saw > that Dan has already r-b'ed the current patchset, but if you prefer I'll > adopt yours. Whatever works for you. It is trivial stuff and as long as we get it cleaned up that is fine. You are pushing for this now - so whatever is easiest for you. Sam
On Thu, Mar 14, 2024 at 09:16:15AM +0100, Thomas Zimmermann wrote: > Hi > > Am 13.03.24 um 19:00 schrieb Sam Ravnborg: > > Hi Thomas, > > > > On Wed, Mar 13, 2024 at 04:45:02PM +0100, Thomas Zimmermann wrote: > > > The backlight is on for fb_blank eq FB_BLANK_UNBLANK, or off for > > > any other value in fb_blank. But the field fb_blank in struct > > > backlight_properties is deprecated and should not be used any > > > longer. > > > > > > Replace the test for fb_blank in omap's backlight code with a > > > simple boolean parameter and push the test into the update_status > > > helper. Instead of reading fb_blank directly, decode the backlight > > > device's status with backlight_is_blank(). > > > > > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > > My biased opinion is that the approach in this patch is a little bit > > better: > > https://lore.kernel.org/lkml/20230107-sam-video-backlight-drop-fb_blank-v1-13-1bd9bafb351f@ravnborg.org/ > > > > I never came around resending this series it seems. > > Oh, that series has been around for over a year. I don't care about which > patches go in as long as they remove the dependency on <linux/fb.h>. I saw > that Dan has already r-b'ed the current patchset, but if you prefer I'll > adopt yours. I hadn't seen Sam's patch. It's a little bit more daring, but it's really nice code and I trust him. regards, dan carpenter
On Wed, Mar 13, 2024 at 04:45:02PM +0100, Thomas Zimmermann wrote: > The backlight is on for fb_blank eq FB_BLANK_UNBLANK, or off for > any other value in fb_blank. But the field fb_blank in struct > backlight_properties is deprecated and should not be used any > longer. > > Replace the test for fb_blank in omap's backlight code with a > simple boolean parameter and push the test into the update_status > helper. Instead of reading fb_blank directly, decode the backlight > device's status with backlight_is_blank(). > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/video/backlight/omap1_bl.c | 46 ++++++++++++++---------------- > 1 file changed, 21 insertions(+), 25 deletions(-) > > diff --git a/drivers/video/backlight/omap1_bl.c b/drivers/video/backlight/omap1_bl.c > index 84d148f385951..3fd8bbb7f5877 100644 > --- a/drivers/video/backlight/omap1_bl.c > +++ b/drivers/video/backlight/omap1_bl.c > @@ -9,7 +9,6 @@ > #include <linux/kernel.h> > #include <linux/init.h> > #include <linux/platform_device.h> > -#include <linux/fb.h> > #include <linux/backlight.h> > #include <linux/slab.h> > #include <linux/platform_data/omap1_bl.h> > @@ -20,7 +19,7 @@ > #define OMAPBL_MAX_INTENSITY 0xff > > struct omap_backlight { > - int powermode; > + bool power; The new name is hard to read in several of the conditionals below (which were previously "documented" by the comparisons to constants. This boolean effectively controls what we send to omapbl_send_enable(). On that basis I'd rather it was called something like "enabled". > int current_intensity; > > struct device *dev; > @@ -37,21 +36,14 @@ static inline void omapbl_send_enable(int enable) > omap_writeb(enable, OMAP_PWL_CLK_ENABLE); > } > > -static void omapbl_blank(struct omap_backlight *bl, int mode) > +static void omapbl_power(struct omap_backlight *bl, bool on) As above omapbl_enable would be better. > { > - switch (mode) { > - case FB_BLANK_NORMAL: > - case FB_BLANK_VSYNC_SUSPEND: > - case FB_BLANK_HSYNC_SUSPEND: > - case FB_BLANK_POWERDOWN: > - omapbl_send_intensity(0); > - omapbl_send_enable(0); > - break; > - > - case FB_BLANK_UNBLANK: > + if (on) { > omapbl_send_intensity(bl->current_intensity); > omapbl_send_enable(1); > - break; > + } else { > + omapbl_send_intensity(0); > + omapbl_send_enable(0); > } > } > > @@ -61,7 +53,7 @@ static int omapbl_suspend(struct device *dev) > struct backlight_device *bl_dev = dev_get_drvdata(dev); > struct omap_backlight *bl = bl_get_data(bl_dev); > > - omapbl_blank(bl, FB_BLANK_POWERDOWN); > + omapbl_power(bl, false); > return 0; > } > > @@ -70,33 +62,37 @@ static int omapbl_resume(struct device *dev) > struct backlight_device *bl_dev = dev_get_drvdata(dev); > struct omap_backlight *bl = bl_get_data(bl_dev); > > - omapbl_blank(bl, bl->powermode); > + omapbl_power(bl, bl->power); > return 0; > } > #endif > > -static int omapbl_set_power(struct backlight_device *dev, int state) > +static void omapbl_set_power(struct backlight_device *dev, bool on) May also like a new name... > { > struct omap_backlight *bl = bl_get_data(dev); > > - omapbl_blank(bl, state); > - bl->powermode = state; > - > - return 0; > + omapbl_power(bl, on); > + bl->power = on; > } > > static int omapbl_update_status(struct backlight_device *dev) > { > struct omap_backlight *bl = bl_get_data(dev); > + bool power; > > if (bl->current_intensity != dev->props.brightness) { > - if (bl->powermode == FB_BLANK_UNBLANK) > + if (bl->power) > omapbl_send_intensity(dev->props.brightness); > bl->current_intensity = dev->props.brightness; > } > > - if (dev->props.fb_blank != bl->powermode) > - omapbl_set_power(dev, dev->props.fb_blank); > + if (backlight_is_blank(dev)) > + power = false; > + else > + power = true; I'd be happy with: enable = !backlight_is_blank() Daniel.
diff --git a/drivers/video/backlight/omap1_bl.c b/drivers/video/backlight/omap1_bl.c index 84d148f385951..3fd8bbb7f5877 100644 --- a/drivers/video/backlight/omap1_bl.c +++ b/drivers/video/backlight/omap1_bl.c @@ -9,7 +9,6 @@ #include <linux/kernel.h> #include <linux/init.h> #include <linux/platform_device.h> -#include <linux/fb.h> #include <linux/backlight.h> #include <linux/slab.h> #include <linux/platform_data/omap1_bl.h> @@ -20,7 +19,7 @@ #define OMAPBL_MAX_INTENSITY 0xff struct omap_backlight { - int powermode; + bool power; int current_intensity; struct device *dev; @@ -37,21 +36,14 @@ static inline void omapbl_send_enable(int enable) omap_writeb(enable, OMAP_PWL_CLK_ENABLE); } -static void omapbl_blank(struct omap_backlight *bl, int mode) +static void omapbl_power(struct omap_backlight *bl, bool on) { - switch (mode) { - case FB_BLANK_NORMAL: - case FB_BLANK_VSYNC_SUSPEND: - case FB_BLANK_HSYNC_SUSPEND: - case FB_BLANK_POWERDOWN: - omapbl_send_intensity(0); - omapbl_send_enable(0); - break; - - case FB_BLANK_UNBLANK: + if (on) { omapbl_send_intensity(bl->current_intensity); omapbl_send_enable(1); - break; + } else { + omapbl_send_intensity(0); + omapbl_send_enable(0); } } @@ -61,7 +53,7 @@ static int omapbl_suspend(struct device *dev) struct backlight_device *bl_dev = dev_get_drvdata(dev); struct omap_backlight *bl = bl_get_data(bl_dev); - omapbl_blank(bl, FB_BLANK_POWERDOWN); + omapbl_power(bl, false); return 0; } @@ -70,33 +62,37 @@ static int omapbl_resume(struct device *dev) struct backlight_device *bl_dev = dev_get_drvdata(dev); struct omap_backlight *bl = bl_get_data(bl_dev); - omapbl_blank(bl, bl->powermode); + omapbl_power(bl, bl->power); return 0; } #endif -static int omapbl_set_power(struct backlight_device *dev, int state) +static void omapbl_set_power(struct backlight_device *dev, bool on) { struct omap_backlight *bl = bl_get_data(dev); - omapbl_blank(bl, state); - bl->powermode = state; - - return 0; + omapbl_power(bl, on); + bl->power = on; } static int omapbl_update_status(struct backlight_device *dev) { struct omap_backlight *bl = bl_get_data(dev); + bool power; if (bl->current_intensity != dev->props.brightness) { - if (bl->powermode == FB_BLANK_UNBLANK) + if (bl->power) omapbl_send_intensity(dev->props.brightness); bl->current_intensity = dev->props.brightness; } - if (dev->props.fb_blank != bl->powermode) - omapbl_set_power(dev, dev->props.fb_blank); + if (backlight_is_blank(dev)) + power = false; + else + power = true; + + if (power != bl->power) + omapbl_set_power(dev, power); return 0; } @@ -136,7 +132,7 @@ static int omapbl_probe(struct platform_device *pdev) if (IS_ERR(dev)) return PTR_ERR(dev); - bl->powermode = FB_BLANK_POWERDOWN; + bl->power = false; bl->current_intensity = 0; bl->pdata = pdata;
The backlight is on for fb_blank eq FB_BLANK_UNBLANK, or off for any other value in fb_blank. But the field fb_blank in struct backlight_properties is deprecated and should not be used any longer. Replace the test for fb_blank in omap's backlight code with a simple boolean parameter and push the test into the update_status helper. Instead of reading fb_blank directly, decode the backlight device's status with backlight_is_blank(). Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/video/backlight/omap1_bl.c | 46 ++++++++++++++---------------- 1 file changed, 21 insertions(+), 25 deletions(-)