Re: [PATCH v1 08/18] backlight: add backlight_is_blank()

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, May 14, 2020 at 09:46:46PM +0200, Sam Ravnborg wrote:
> Hi Daniel.
> 
> On Thu, May 14, 2020 at 09:41:16PM +0200, Daniel Vetter wrote:
> > On Thu, May 14, 2020 at 09:09:51PM +0200, Sam Ravnborg wrote:
> > > The backlight support has two properties that express the state:
> > > - power
> > > - state
> > > 
> > > It is un-documented and easy to get wrong.
> > > Add backlight_is_blank() helper to make it simpler for drivers
> > > to get the check of the state correct.
> > > 
> > > A lot of drivers also includes checks for fb_blank.
> > > This check is redundant when the state is checked
> > > as thus not needed in this helper function.
> > > Rolling out this helper to all relevant backlight drivers
> > > will eliminate almost all accesses to fb_blank.
> > > 
> > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > > Cc: Lee Jones <lee.jones@linaro.org>
> > > Cc: Daniel Thompson <daniel.thompson@linaro.org>
> > > Cc: Jingoo Han <jingoohan1@gmail.com>
> > > ---
> > >  include/linux/backlight.h | 17 +++++++++++++++++
> > >  1 file changed, 17 insertions(+)
> > > 
> > > diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> > > index b7839ea9d00a..e67e926de1e2 100644
> > > --- a/include/linux/backlight.h
> > > +++ b/include/linux/backlight.h
> > > @@ -165,6 +165,23 @@ static inline int backlight_disable(struct backlight_device *bd)
> > >  	return backlight_update_status(bd);
> > >  }
> > >  
> > > +/**
> > > + * backlight_is_blank - Return true if display is expected to be blank
> > > + * @bd: the backlight device
> > > + *
> > > + * Display is expected to be blank if any of these is true::
> > > + *
> > > + *   1) if power in not UNBLANK
> > > + *   2) if state indicate BLANK or SUSPENDED
> > > + *
> > > + * Returns true if display is expected to be blank, false otherwise.
> > > + */
> > > +static inline bool backlight_is_blank(struct backlight_device *bd)
> > > +{
> > > +	return bd->props.power != FB_BLANK_UNBLANK ||
> > > +	       bd->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK);
> > 
> > This definition here doesn't match backlight_enabled/disable() functions
> > we added. I think to avoid lots of pondering and surprises we should try
> > to make sure these are all matching, so that once we rolled them out
> > everywhere, we can just replace the complicated state with one flag.
> 
> Will add it in v2. When all user of fb_blank is dropped we can
> safely remove it then.
> And as you said on irc, the risk of introducing regressions is lower
> as we see some creative uses in the drivers today.
> I already did some kind of audit - but I may have missed something.

btw one pattern I've seen in a few places with a few greps I've just done
is maybe worth putting into a helper too:

backlight_force_enable(bl)
{
	if (bl->brightness == 0)
		bl->brightness = bl->max_brightness;
	backlight_enable(backlight);
}

Just in case you run out of ideas anytime soon :-)

Cheers, Daniel

> 
> 	Sam
> 
> > 
> > > +}
> > > +
> > >  extern struct backlight_device *backlight_device_register(const char *name,
> > >  	struct device *dev, void *devdata, const struct backlight_ops *ops,
> > >  	const struct backlight_properties *props);
> > > -- 
> > > 2.25.1
> > > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch