[PATCH v3 2/2] drm/tinydrm: Add devres versions of drm_of_find_backlight

[Noralf Trønnes]

Den 29.09.2017 05.17, skrev Meghana Madhyastha:
> On Thu, Sep 28, 2017 at 06:19:35PM +0200, Noralf Trønnes wrote:
>> Den 28.09.2017 11.15, skrev Meghana Madhyastha:
>>> Add devm_drm_of_find_backlight and the corresponding release
>>> function because some drivers such as tinydrm use devres versions
>>> of functions for requiring device resources.
>>>
>>> Signed-off-by: Meghana Madhyastha <meghana.madhyastha at gmail.com>
>>> ---
>>> Changes in v3:
>>> -None
>>>
>>>   drivers/gpu/drm/drm_of.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>   include/drm/drm_of.h     |  2 ++
>>>   2 files changed, 50 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
>>> index d878d3a..238e8e5 100644
>>> --- a/drivers/gpu/drm/drm_of.c
>>> +++ b/drivers/gpu/drm/drm_of.c
>>> @@ -304,3 +304,51 @@ struct backlight_device *drm_of_find_backlight(struct device *dev)
>>>   	return backlight;
>>>   }
>>>   EXPORT_SYMBOL(drm_of_find_backlight);
>>> +
>>> +/**
>>> + * devm_drm_of_find_backlight_release - Release backlight device
>>> + * @dev: Device
>>> + *
>>> + * This is the release function corresponding to the devm_drm_of_find_backlight.
>>> + * Each devres entry is associated with a release function.
>>> + */
>> This is an internal function so no need for docs or exporting. I know
>> that some devm_ functions have explicit release functions, but I don't
>> think this is necessary since those users can just use
>> drm_of_find_backlight() directly instead.
> I have a question here. devm_drm_of_find_backlight_release is a
> wrapper around put_device which is passed as a parameter to
> devm_add_action in devm_drm_of_find_backlight. So isn't the function
> useful here ?

We need the function for devm_add_action(), but no one outside of this 
file needs
it. Hence the static definition.

We can't use put_device directly:
     ret = devm_add_action(dev, put_device,
                   &backlight->dev);
because devm_add_action expects an argument like this: void 
(*action)(void *)
and put_device is: void put_device(struct device *dev)
They differ in their argument type: void * vs. struct device *
The compiler would complain.

So we need a wrapper.

>>> +static void devm_drm_of_find_backlight_release(void *data)
>>> +{
>>> +	put_device(data);
>>> +}
>>> +EXPORT_SYMBOL(devm_drm_of_find_backlight_release);
>>> +
>>> +/**
>>> + * devm_drm_of_find_backlight - Find backlight device in device-tree
>>> + * devres version of the function
>>> + * @dev: Device
>>> + *
>>> + * This is the devres version of the function drm_of_find_backlight.
>>> + * Some drivers such as tinydrm use devres versions of functions for
>> No need to mention tinydrm here.
>>
>>> + * requiring device resources.
>>> + *
>>> + * Returns:
>>> + * NULL if there's no backlight property.
>>> + * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
>>> + * is found.
>>> + * If the backlight device is found, a pointer to the structure is returned.
>>> + */
>>> +struct backlight_device *devm_drm_of_find_backlight(struct device *dev)
>>> +{
>>> +	struct backlight_device *backlight;
>>> +	int ret;
>>> +
>>> +	backlight = drm_of_find_backlight(dev);
>>> +	if (IS_ERR_OR_NULL(backlight))
>>> +		return backlight;
>>> +
>>> +	ret = devm_add_action(dev, devm_drm_of_find_backlight_release,
>>> +			      &backlight->dev);
>>> +	if (ret) {
>>> +		put_device(&backlight->dev);
>>> +		return ERR_PTR(ret);
>>> +	}
>>> +
>>> +	return backlight;
>>> +}
>>> +EXPORT_SYMBOL(devm_drm_of_find_backlight);
>>> diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
>>> index e8fba5b..071fb3b 100644
>>> --- a/include/drm/drm_of.h
>>> +++ b/include/drm/drm_of.h
>>> @@ -30,7 +30,9 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
>>>   				struct drm_panel **panel,
>>>   				struct drm_bridge **bridge);
>>>   struct backlight_device *drm_of_find_backlight(struct device *dev);
>>> +struct backlight_device *devm_drm_of_find_backlight(struct device *dev);
>>>   #else
>> We need a dummy version of devm_drm_of_find_backlight() here that
>> returns NULL as in the previous patch.
>>
>>> +static void devm_drm_of_find_backlight_release(void *data);
>> And this isn't needed as explained above.
>>>   static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
>>>   						  struct device_node *port)
>>>   {
>> I'd appreciate if you could also switch mi0283qt over to this helper :-)
>>
>> Noralf.
>>
>>