Re: [PATCH v6 1/2] drm/tinydrm: Move tinydrm_of_find_backlight into drm_of.c

["Noralf Trønnes" <noralf@tronnes.org>]

Den 01.10.2017 15.34, skrev Meghana Madhyastha:
> On Sun, Oct 01, 2017 at 03:26:36PM +0200, Noralf Trønnes wrote:
>> Den 01.10.2017 06.14, skrev Meghana Madhyastha:
>>> On Sat, Sep 30, 2017 at 09:04:57PM +0200, Noralf Trønnes wrote:
>>>> Den 30.09.2017 19.12, skrev Meghana Madhyastha:
>>>>> Rename tinydrm_of_find_backlight to drm_of_find_backlight
>>>>> and move it into drm_of.c from tinydrm-helpers.c. This is
>>>>> because other drivers in the drm subsystem might need to call
>>>>> this function. In that case and otherwise, it is better from
>>>>> an organizational point of view to move it into drm_of.c along
>>>>> with the other _of.c functions.
>>>>>
>>>>> Signed-off-by: Meghana Madhyastha <meghana.madhyastha@gmail.com>
>>>>> ---
>>>>> Changes in v6:
>>>>> -Move function declarations to linux/backlight.h to resolve
>>>>>   module dependency issues.
>>>>>
>>>>>   drivers/gpu/drm/drm_of.c                       | 44 ++++++++++++++++++++++++++
>>>>>   drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 -----------------------
>>>>>   drivers/gpu/drm/tinydrm/mi0283qt.c             |  2 +-
>>>>>   include/drm/tinydrm/tinydrm-helpers.h          |  1 -
>>>>>   include/linux/backlight.h                      | 11 +++++++
>>>>>   5 files changed, 56 insertions(+), 42 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
>>>>> index 8dafbdf..d878d3a 100644
>>>>> --- a/drivers/gpu/drm/drm_of.c
>>>>> +++ b/drivers/gpu/drm/drm_of.c
>>>>> @@ -1,6 +1,7 @@
>>>>>   #include <linux/component.h>
>>>>>   #include <linux/export.h>
>>>>>   #include <linux/list.h>
>>>>> +#include <linux/backlight.h>
>>>>>   #include <linux/of_graph.h>
>>>>>   #include <drm/drmP.h>
>>>>>   #include <drm/drm_bridge.h>
>>>>> @@ -260,3 +261,46 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
>>>>>   	return ret;
>>>>>   }
>>>>>   EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge);
>>>>> +
>>>>> +/**
>>>>> + * drm_of_find_backlight - Find backlight device in device-tree
>>>>> + * @dev: Device
>>>>> + *
>>>>> + * This function looks for a DT node pointed to by a property named 'backlight'
>>>>> + * and uses of_find_backlight_by_node() to get the backlight device.
>>>>> + * Additionally if the brightness property is zero, it is set to
>>>>> + * max_brightness.
>>>>> + *
>>>>> + * Note: It is the responsibility of the caller to call put_device() when
>>>>> + * releasing the resource.
>>>>> + *
>>>>> + * 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 *drm_of_find_backlight(struct device *dev)
>>>>> +{
>>>>> +	struct backlight_device *backlight;
>>>>> +	struct device_node *np;
>>>>> +
>>>>> +	np = of_parse_phandle(dev->of_node, "backlight", 0);
>>>>> +	if (!np)
>>>>> +		return NULL;
>>>>> +
>>>>> +	backlight = of_find_backlight_by_node(np);
>>>>> +	of_node_put(np);
>>>>> +
>>>>> +	if (!backlight)
>>>>> +		return ERR_PTR(-EPROBE_DEFER);
>>>>> +
>>>>> +	if (!backlight->props.brightness) {
>>>>> +		backlight->props.brightness = backlight->props.max_brightness;
>>>>> +		DRM_DEBUG_KMS("Backlight brightness set to %d\n",
>>>>> +			      backlight->props.brightness);
>>>>> +	}
>>>>> +
>>>>> +	return backlight;
>>>>> +}
>>>>> +EXPORT_SYMBOL(drm_of_find_backlight);
>>>>> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>>>>> index bd6cce0..cd4c6a5 100644
>>>>> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>>>>> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>>>>> @@ -237,46 +237,6 @@ void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
>>>>>   EXPORT_SYMBOL(tinydrm_xrgb8888_to_gray8);
>>>>>   /**
>>>>> - * tinydrm_of_find_backlight - Find backlight device in device-tree
>>>>> - * @dev: Device
>>>>> - *
>>>>> - * This function looks for a DT node pointed to by a property named 'backlight'
>>>>> - * and uses of_find_backlight_by_node() to get the backlight device.
>>>>> - * Additionally if the brightness property is zero, it is set to
>>>>> - * max_brightness.
>>>>> - *
>>>>> - * 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 *tinydrm_of_find_backlight(struct device *dev)
>>>>> -{
>>>>> -	struct backlight_device *backlight;
>>>>> -	struct device_node *np;
>>>>> -
>>>>> -	np = of_parse_phandle(dev->of_node, "backlight", 0);
>>>>> -	if (!np)
>>>>> -		return NULL;
>>>>> -
>>>>> -	backlight = of_find_backlight_by_node(np);
>>>>> -	of_node_put(np);
>>>>> -
>>>>> -	if (!backlight)
>>>>> -		return ERR_PTR(-EPROBE_DEFER);
>>>>> -
>>>>> -	if (!backlight->props.brightness) {
>>>>> -		backlight->props.brightness = backlight->props.max_brightness;
>>>>> -		DRM_DEBUG_KMS("Backlight brightness set to %d\n",
>>>>> -			      backlight->props.brightness);
>>>>> -	}
>>>>> -
>>>>> -	return backlight;
>>>>> -}
>>>>> -EXPORT_SYMBOL(tinydrm_of_find_backlight);
>>>>> -
>>>>> -/**
>>>>>    * tinydrm_enable_backlight - Enable backlight helper
>>>>>    * @backlight: Backlight device
>>>>>    *
>>>>> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
>>>>> index 7e5bb7d..682b4db 100644
>>>>> --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
>>>>> +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
>>>>> @@ -189,7 +189,7 @@ static int mi0283qt_probe(struct spi_device *spi)
>>>>>   	if (IS_ERR(mipi->regulator))
>>>>>   		return PTR_ERR(mipi->regulator);
>>>>> -	mipi->backlight = tinydrm_of_find_backlight(dev);
>>>>> +	mipi->backlight = drm_of_find_backlight(dev);
>>>>>   	if (IS_ERR(mipi->backlight))
>>>>>   		return PTR_ERR(mipi->backlight);
>>>>> diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
>>>>> index d554ded..e40ef2d 100644
>>>>> --- a/include/drm/tinydrm/tinydrm-helpers.h
>>>>> +++ b/include/drm/tinydrm/tinydrm-helpers.h
>>>>> @@ -46,7 +46,6 @@ void tinydrm_xrgb8888_to_rgb565(u16 *dst, void *vaddr,
>>>>>   void tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
>>>>>   			       struct drm_clip_rect *clip);
>>>>> -struct backlight_device *tinydrm_of_find_backlight(struct device *dev);
>>>>>   int tinydrm_enable_backlight(struct backlight_device *backlight);
>>>>>   int tinydrm_disable_backlight(struct backlight_device *backlight);
>>>>> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
>>>>> index 5f2fd61..47a095e 100644
>>>>> --- a/include/linux/backlight.h
>>>>> +++ b/include/linux/backlight.h
>>>>> @@ -172,4 +172,15 @@ of_find_backlight_by_node(struct device_node *node)
>>>>>   }
>>>>>   #endif
>>>>> +#if defined(CONFIG_OF) && IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
>>>>> +struct backlight_device *
>>>>> +drm_of_find_backlight(struct device *dev);
>>>>> +#else
>>>>> +static inline struct backlight_device *
>>>>> +drm_of_find_backlight(struct device *dev)
>>>>> +{
>>>>> +	return NULL;
>>>>> +}
>>>>> +#endif
>>>>> +
>>>>>   #endif
>>>> This isn't what I meant, you only change the of_find_backlight_by_node()
>>>> declaration/definition. It should be like this:
>>>>
>>>> #if defined(CONFIG_OF) && IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
>>>> struct backlight_device *of_find_backlight_by_node(struct device_node
>>>> *node);
>>>> #else
>>>> static inline struct backlight_device *
>>>> of_find_backlight_by_node(struct device_node *node)
>>>> {
>>>>      return NULL;
>>>> }
>>>> #endif
>>> I see. So basically add the function declaration of drm_of_find_backlight
>>> and devm_drm_of_find_backlight in drm_of.h and this function declaration
>>> need not be inside the #ifdef CONFIG / #else part?
>>> So there need not be a dummy version of drm_of_find_backlight here
>>> because we are anyway taking care of that in of_find_backliht by node?
>>> Sorry for the many versions and thank you for the clarifications.
>> You need the dummy part in drm_of.h because you need to handle the
>> CONFIG_OF option here as well.
>>
>> If drm_of_find_backlight() was only 2-3 lines of code, you could have
>> made a static inline function in drm_of.h outside the #if/#else/#endif,
>> but since it's longer than that it should be in drm_of.c.
>>
>> CONFIG_OF decides if drm_of.c is built (as part of drm.o) or not:
>>
>> drivers/gpu/drm/Makefile:
>> drm-$(CONFIG_OF) += drm_of.o
>>
>> This means we need a function prototype declaration for the CONFIG_OF=y
>> case, and an inline function definition for the CONFIG_OF=n case.
>>
>> Noralf.
> I have one more question. In that case, why wouldn't my previous patch
> work ? (i.e  definition and declaration of drm_of_find_backlight
> in linux/backlight.h with if/else directives)

It will work, the compiler doesn't complain as long as you include
backlight.h and make sure drm_of.c is built. But that would result
in a complexity that humans would struggle to get right. And in this
case it's even 2 different subsystems with different maintainers...

So in order to succeed with complex projects, humans need some more
rules than what the compiler requires. And one of those are:
   A header file is usually connected to a source file.

backlight.h contains prototypes and definitions for backlight.c
drm_of.h contains prototypes and definitions for drm_of.c


> Regards,
> Meghana
>
>>> Regards,
>>> Meghana
>>>> And this change needs to be a patch on it's own. The backlight subsystem
>>>> has it's own maintainer that will review this change.
>>>> It's best to send him/them the whole patchset so they see the context.
>>>>
>>>> Noralf.
>>>>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel