[Outreachy kernel] [PATCH v13 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c
Rob Clark
robdclark at gmail.com
Fri Oct 13 23:25:03 UTC 2017
On Fri, Oct 13, 2017 at 4:25 PM, Sean Paul <seanpaul at chromium.org> wrote:
> On Fri, Oct 13, 2017 at 04:11:43PM +0530, Meghana Madhyastha wrote:
>> Rename tinydrm_of_find_backlight to backlight_get and move it
>> to linux/backlight.c so that it can be used by other drivers.
>
> [apologies if this has been brought up in previous versions, I haven't been
> following closely]
>
> I don't think "backlight_get" is a good name for this function. How about
> of_find_backlight_by_name (since there's already of_find_backlight_by_node)?
>
>>
>> Signed-off-by: Meghana Madhyastha <meghana.madhyastha at gmail.com>
>> ---
>> Changes in v13:
>> -Add backlight_put to backlight.h in this patch
>>
>> drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 40 --------------------------
>> drivers/gpu/drm/tinydrm/mi0283qt.c | 3 +-
>> drivers/video/backlight/backlight.c | 37 ++++++++++++++++++++++++
>> include/drm/tinydrm/tinydrm-helpers.h | 2 --
>> include/linux/backlight.h | 19 ++++++++++++
>> 5 files changed, 58 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> index a42dee6..cb1a01a 100644
>> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> @@ -236,46 +236,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);
>> -
>> #if IS_ENABLED(CONFIG_SPI)
>>
>> /**
>> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
>> index 7fd2691..a932185 100644
>> --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
>> +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
>> @@ -12,6 +12,7 @@
>> #include <drm/tinydrm/ili9341.h>
>> #include <drm/tinydrm/mipi-dbi.h>
>> #include <drm/tinydrm/tinydrm-helpers.h>
>> +#include <linux/backlight.h>
>> #include <linux/delay.h>
>> #include <linux/gpio/consumer.h>
>> #include <linux/module.h>
>> @@ -188,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 = backlight_get(dev);
>> if (IS_ERR(mipi->backlight))
>> return PTR_ERR(mipi->backlight);
>>
>> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
>> index 8049e76..c4e94d0 100644
>> --- a/drivers/video/backlight/backlight.c
>> +++ b/drivers/video/backlight/backlight.c
>> @@ -580,6 +580,43 @@ struct backlight_device *of_find_backlight_by_node(struct device_node *node)
>> EXPORT_SYMBOL(of_find_backlight_by_node);
>> #endif
>>
>> +/**
>> + * backlight_get - Get backlight device
>> + * @dev: Device
>> + *
>> + * This function looks for a property named 'backlight' on the DT node
>> + * connected to @dev and looks up the backlight device.
>> + *
>> + * Call backlight_put() to drop the reference on the backlight device.
>> + *
>> + * Returns:
>> + * A pointer to the backlight device if found.
>> + * Error pointer -EPROBE_DEFER if the DT property is set, but no backlight
>> + * device is found.
>> + * NULL if there's no backlight property.
>> + */
>> +struct backlight_device *backlight_get(struct device *dev)
>> +{
>> + struct backlight_device *bd = NULL;
>> + struct device_node *np;
>> +
>> + if (!dev)
>> + return NULL;
>> +
>> + if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
>
> I'm not really used to seeing IS_ENABLED(CONFIG_BLAH) inline. The common
> patterns seems to be wrapping the actual implementation in #if
> IS_ENABLED(CONFIG_BLAH) and then sticking a stub implementation in the #else.
drive-by comment, but I actually prefer 'if (IS_ENABLED(CONFIG_FOO))
..' when possible.. it keeps the code compiled by the front-end of the
compiler in more configs so less likely to bitrot, and the compiler is
plenty good at DCE to eliminate the code when disabled in later stages
of the compiler..
possibly exceptions are things that are called frequently where you
want to inline the CONFIG_FOO=n stub so it can be eliminated without
calling into another object file (at least until LTO gets good), but I
guess that is not the case here..
BR,
-R
> I see below that you already have a stub if backlight is not enabled, so expand
> that #if to include CONFIG_OF as well.
>
>> + np = of_parse_phandle(dev->of_node, "backlight", 0);
>> + if (np) {
>> + bd = of_find_backlight_by_node(np);
>> + of_node_put(np);
>> + if (!bd)
>> + return ERR_PTR(-EPROBE_DEFER);
>> + }
>> + }
>> +
>> + return bd;
>> +}
>> +EXPORT_SYMBOL(backlight_get);
>> +
>> static void __exit backlight_class_exit(void)
>> {
>> class_destroy(backlight_class);
>> diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
>> index f54fae0..0a4ddbc 100644
>> --- a/include/drm/tinydrm/tinydrm-helpers.h
>> +++ b/include/drm/tinydrm/tinydrm-helpers.h
>> @@ -46,8 +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);
>> -
>> size_t tinydrm_spi_max_transfer_size(struct spi_device *spi, size_t max_len);
>> bool tinydrm_spi_bpw_supported(struct spi_device *spi, u8 bpw);
>> int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz,
>> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
>> index b88fabb..987a6d7 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);
>> +}
>
> I'm not convinced this function needs to exist.
>
>> +
>> 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);
>> @@ -202,4 +212,13 @@ of_find_backlight_by_node(struct device_node *node)
>> }
>> #endif
>>
>> +#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
>> +struct backlight_device *backlight_get(struct device *dev);
>> +#else
>> +static inline struct backlight_device *backlight_get(struct device *dev)
>> +{
>> + return NULL;
>> +}
>> +#endif
>> +
>> #endif
>> --
>> 2.7.4
>>
>> --
>> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
>> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe at googlegroups.com.
>> To post to this group, send email to outreachy-kernel at googlegroups.com.
>> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/e2ef05bf46438427d08c6a61fe5058156800db95.1507890285.git.meghana.madhyastha%40gmail.com.
>> For more options, visit https://groups.google.com/d/optout.
>
> --
> Sean Paul, Software Engineer, Google / Chromium OS
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
More information about the dri-devel
mailing list