[Outreachy kernel] [PATCH v14 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c

[Daniel Thompson]

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.