On 25/10/17 08:39, Sean Paul wrote:
diff --git a/include/linux/backlight.h b/include/linux/backlight.h index b88fabb..f98b684 100644 --- a/include/linux/backlight.h +++ b/include/linux/backlight.h @@ -159,6 +159,16 @@ static inline int backlight_disable(struct backlight_device *bd) return backlight_update_status(bd); } +/**
- backlight_put - Drop backlight reference
- @bd: the backlight device to put
- */
+static inline void backlight_put(struct backlight_device *bd) +{
- if (bd)
put_device(&bd->dev);+}
As I mentioned in my last review, I don't think we need this function.
Just to check... some DRM drivers do allow themselves to work if CONFIG_BACKLIGHT_CLASS_DEVICE isn't enabled (as discussed earlier in the thread ;-). Did you consider what happens when this happens.
Wrapped up with backlight_put() we are sure never to accidentally dereference bd in DRM de-init paths when CONFIG_BACKLIGHT_CLASS_DEVICVE if it is not set. With a raw put_device() all that conditional logic ends up on the driver.
Thanks for pointing that out, definitely a fair point.
I don't really mind putting the NULL check in the driver code. The return values of of_find_backlight are well documented, so it shouldn't be too much of a problem.
That said, this is your rodeo, so if you prefer to supply the put helper, that's A-Ok with me.
Well... I don't want to add something with no callers... but if something else in the patch set (or near future) uses it then I'm certainly happy to have it!
Daniel.