Den 13.10.2017 22.25, skrev Sean Paul:
> 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)?
I came up with that name modelled after gpiod_get() and gpiod_put() and I
deliberately kept the of_ part out of the name like the gpio functions.
gpiod_get() checks OF, ACPI and platform for gpios and calling it
backlight_get() would keep the door open for other ways of connecting
backlight devices in the future, other than Device Tree.
I think of_find_backlight() would be better than *_by_name(), since
'backlight' is the common DT property name, so it wouldn't make much sense
to require every caller to pass in the same name.
Either way is fine with me.
Noralf.
>> 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.
>
> 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.