Re: [Outreachy kernel] [PATCH v14 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c
On Thu, Dec 07, 2017 at 05:01:21PM +0100, Noralf Trønnes wrote: > > Den 07.12.2017 16.44, skrev Daniel Thompson: > > On 06/12/17 10:43, Meghana Madhyastha wrote: > > > On Tue, Oct 24, 2017 at 06:45:34PM +0200, Noralf Trønnes wrote: > > > > > > > > Den 24.10.2017 17.42, skrev Sean Paul: > > > > > On Sat, Oct 21, 2017 at 05:27:33PM +0530, Meghana Madhyastha wrote: > > > > > > Rename tinydrm_of_find_backlight to of_find_backlight and move > > > > > > it to linux/backlight.c so that it can be used by other drivers. > > > > > > > > > > > > Signed-off-by: Meghana Madhyastha> > > > > > --- > > > > > > Changes in v14: > > > > > > -s/backlight_get/of_find_backlight/ > > > > > > > > > > > > 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_xrgb_to_gray8(u8 > > > > > > *dst, void *vaddr, struct drm_framebuffer *fb, > > > > > > } > > > > > > EXPORT_SYMBOL(tinydrm_xrgb_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..53ab5a0 100644 > > > > > > --- a/drivers/gpu/drm/tinydrm/mi0283qt.c > > > > > > +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c > > > > > > @@ -12,6 +12,7 @@ > > > > > > #include > > > > > > #include > > > > > > #include > > > > > > +#include > > > > > > #include > > > > > > #include > > > > > > #include > > > > > > @@ -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 = of_find_backlight(dev); > > > > > Sorry for the follow-up spam, but are you missing the > > > > > put_device somewhere? The > > > > > next patch uses devm_of_find_backlight. So AFAICT you're > > > > > either leaking a reference > > > > > here, or you're closing an additional reference in the next patch. > > > > > > > > This is my fault, put_device() has been missing all along, so > > > > Meghana is > > > > plugging that hole, > > > > in the next patch :-) > > > > > > > > Noralf. > > > > > > Is there anything else that needs to be done for this patch ? If not, > > > and it is ready, can this be acked ? It seems to have been stuck for > > >
Re: [Outreachy kernel] [PATCH v14 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c
Den 07.12.2017 16.44, skrev Daniel Thompson: On 06/12/17 10:43, Meghana Madhyastha wrote: On Tue, Oct 24, 2017 at 06:45:34PM +0200, Noralf Trønnes wrote: Den 24.10.2017 17.42, skrev Sean Paul: On Sat, Oct 21, 2017 at 05:27:33PM +0530, Meghana Madhyastha wrote: Rename tinydrm_of_find_backlight to of_find_backlight and move it to linux/backlight.c so that it can be used by other drivers. Signed-off-by: Meghana Madhyastha--- Changes in v14: -s/backlight_get/of_find_backlight/ 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_xrgb_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb, } EXPORT_SYMBOL(tinydrm_xrgb_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..53ab5a0 100644 --- a/drivers/gpu/drm/tinydrm/mi0283qt.c +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -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 = of_find_backlight(dev); Sorry for the follow-up spam, but are you missing the put_device somewhere? The next patch uses devm_of_find_backlight. So AFAICT you're either leaking a reference here, or you're closing an additional reference in the next patch. This is my fault, put_device() has been missing all along, so Meghana is plugging that hole, in the next patch :-) Noralf. Is there anything else that needs to be done for this patch ? If not, and it is ready, can this be acked ? It seems to have been stuck for some time now and I will definitely fix anything that needs to be fixed in this patch ASAP. I've lost track a little but have two questions: 1. Is there any pending feedback? If not a resend might be sensible. 2. Which maintainers are you expecting to pick up the patch? It looks to me like lots of people are be missing from the To: fields (compared to what I think get_maintainer.pl would give you) meaning the patches won't get picked up. I'm a bit confused since the individual patches are touching both the drm and backlight subsystem. Will this be merged through drm? I'd expect one patch to add the backlight stuff in that subsystem and then some to do the drm side. Noralf. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Outreachy kernel] [PATCH v14 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c
On 06/12/17 10:43, Meghana Madhyastha wrote: On Tue, Oct 24, 2017 at 06:45:34PM +0200, Noralf Trønnes wrote: Den 24.10.2017 17.42, skrev Sean Paul: On Sat, Oct 21, 2017 at 05:27:33PM +0530, Meghana Madhyastha wrote: Rename tinydrm_of_find_backlight to of_find_backlight and move it to linux/backlight.c so that it can be used by other drivers. Signed-off-by: Meghana Madhyastha--- Changes in v14: -s/backlight_get/of_find_backlight/ 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_xrgb_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb, } EXPORT_SYMBOL(tinydrm_xrgb_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..53ab5a0 100644 --- a/drivers/gpu/drm/tinydrm/mi0283qt.c +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -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 = of_find_backlight(dev); Sorry for the follow-up spam, but are you missing the put_device somewhere? The next patch uses devm_of_find_backlight. So AFAICT you're either leaking a reference here, or you're closing an additional reference in the next patch. This is my fault, put_device() has been missing all along, so Meghana is plugging that hole, in the next patch :-) Noralf. Is there anything else that needs to be done for this patch ? If not, and it is ready, can this be acked ? It seems to have been stuck for some time now and I will definitely fix anything that needs to be fixed in this patch ASAP. I've lost track a little but have two questions: 1. Is there any pending feedback? If not a resend might be sensible. 2. Which maintainers are you expecting to pick up the patch? It looks to me like lots of people are be missing from the To: fields (compared to what I think get_maintainer.pl would give you) meaning the patches won't get picked up. Daniel. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Outreachy kernel] [PATCH v14 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c
On Tue, Oct 24, 2017 at 06:45:34PM +0200, Noralf Trønnes wrote: > > Den 24.10.2017 17.42, skrev Sean Paul: > >On Sat, Oct 21, 2017 at 05:27:33PM +0530, Meghana Madhyastha wrote: > >>Rename tinydrm_of_find_backlight to of_find_backlight and move > >>it to linux/backlight.c so that it can be used by other drivers. > >> > >>Signed-off-by: Meghana Madhyastha> >>--- > >>Changes in v14: > >> -s/backlight_get/of_find_backlight/ > >> > >> 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_xrgb_to_gray8(u8 *dst, void *vaddr, > >>struct drm_framebuffer *fb, > >> } > >> EXPORT_SYMBOL(tinydrm_xrgb_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..53ab5a0 100644 > >>--- a/drivers/gpu/drm/tinydrm/mi0283qt.c > >>+++ b/drivers/gpu/drm/tinydrm/mi0283qt.c > >>@@ -12,6 +12,7 @@ > >> #include > >> #include > >> #include > >>+#include > >> #include > >> #include > >> #include > >>@@ -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 = of_find_backlight(dev); > >Sorry for the follow-up spam, but are you missing the put_device somewhere? > >The > >next patch uses devm_of_find_backlight. So AFAICT you're either leaking a > >reference > >here, or you're closing an additional reference in the next patch. > > This is my fault, put_device() has been missing all along, so Meghana is > plugging that hole, > in the next patch :-) > > Noralf. Is there anything else that needs to be done for this patch ? If not, and it is ready, can this be acked ? It seems to have been stuck for some time now and I will definitely fix anything that needs to be fixed in this patch ASAP. -Meghana > >Sean > > > >>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..76d46c3 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 > >>+/** > >>+ * of_find_backlight - 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
Re: Re: [Outreachy kernel] [PATCH v14 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c
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(>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. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Outreachy kernel] [PATCH v14 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c
On Tue, Oct 24, 2017 at 04:56:59PM +0100, Daniel Thompson wrote: > On 24/10/17 16:38, Sean Paul wrote: > > On Sat, Oct 21, 2017 at 05:27:33PM +0530, Meghana Madhyastha wrote: > > > Rename tinydrm_of_find_backlight to of_find_backlight and move > > > it to linux/backlight.c so that it can be used by other drivers. > > > > > > Signed-off-by: Meghana Madhyastha> > > --- > > > Changes in v14: > > > -s/backlight_get/of_find_backlight/ > > > > > > 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_xrgb_to_gray8(u8 *dst, void *vaddr, > > > struct drm_framebuffer *fb, > > > } > > > EXPORT_SYMBOL(tinydrm_xrgb_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..53ab5a0 100644 > > > --- a/drivers/gpu/drm/tinydrm/mi0283qt.c > > > +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c > > > @@ -12,6 +12,7 @@ > > > #include > > > #include > > > #include > > > +#include > > > #include > > > #include > > > #include > > > @@ -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 = of_find_backlight(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..76d46c3 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 > > > +/** > > > + * of_find_backlight - 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 *of_find_backlight(struct device *dev) > > > +{ > > > + struct backlight_device *bd = NULL; > > > + struct device_node *np; > > > + > > > + if (!dev) > > > + return NULL; > > > + > > > + if (IS_ENABLED(CONFIG_OF) && dev->of_node) { > > > + np = of_parse_phandle(dev->of_node,
Re: [Outreachy kernel] [PATCH v14 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c
Den 24.10.2017 17.42, skrev Sean Paul: On Sat, Oct 21, 2017 at 05:27:33PM +0530, Meghana Madhyastha wrote: Rename tinydrm_of_find_backlight to of_find_backlight and move it to linux/backlight.c so that it can be used by other drivers. Signed-off-by: Meghana Madhyastha--- Changes in v14: -s/backlight_get/of_find_backlight/ 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_xrgb_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb, } EXPORT_SYMBOL(tinydrm_xrgb_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..53ab5a0 100644 --- a/drivers/gpu/drm/tinydrm/mi0283qt.c +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -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 = of_find_backlight(dev); Sorry for the follow-up spam, but are you missing the put_device somewhere? The next patch uses devm_of_find_backlight. So AFAICT you're either leaking a reference here, or you're closing an additional reference in the next patch. This is my fault, put_device() has been missing all along, so Meghana is plugging that hole, in the next patch :-) Noralf. Sean 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..76d46c3 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 +/** + * of_find_backlight - 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 *of_find_backlight(struct device *dev) +{ + struct backlight_device *bd = NULL; + struct device_node *np; + + if (!dev) + return NULL; + + if (IS_ENABLED(CONFIG_OF) && dev->of_node) { + 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; +}
Re: [Outreachy kernel] [PATCH v14 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c
On 24/10/17 16:38, Sean Paul wrote: On Sat, Oct 21, 2017 at 05:27:33PM +0530, Meghana Madhyastha wrote: Rename tinydrm_of_find_backlight to of_find_backlight and move it to linux/backlight.c so that it can be used by other drivers. Signed-off-by: Meghana Madhyastha--- Changes in v14: -s/backlight_get/of_find_backlight/ 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_xrgb_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb, } EXPORT_SYMBOL(tinydrm_xrgb_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..53ab5a0 100644 --- a/drivers/gpu/drm/tinydrm/mi0283qt.c +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -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 = of_find_backlight(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..76d46c3 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 +/** + * of_find_backlight - 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 *of_find_backlight(struct device *dev) +{ + struct backlight_device *bd = NULL; + struct device_node *np; + + if (!dev) + return NULL; + + if (IS_ENABLED(CONFIG_OF) && dev->of_node) { + 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(of_find_backlight); + 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_xrgb_to_rgb565(u16 *dst,
Re: [Outreachy kernel] [PATCH v14 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c
On Sat, Oct 21, 2017 at 05:27:33PM +0530, Meghana Madhyastha wrote: > Rename tinydrm_of_find_backlight to of_find_backlight and move > it to linux/backlight.c so that it can be used by other drivers. > > Signed-off-by: Meghana Madhyastha> --- > Changes in v14: > -s/backlight_get/of_find_backlight/ > > 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_xrgb_to_gray8(u8 *dst, void *vaddr, > struct drm_framebuffer *fb, > } > EXPORT_SYMBOL(tinydrm_xrgb_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..53ab5a0 100644 > --- a/drivers/gpu/drm/tinydrm/mi0283qt.c > +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c > @@ -12,6 +12,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -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 = of_find_backlight(dev); Sorry for the follow-up spam, but are you missing the put_device somewhere? The next patch uses devm_of_find_backlight. So AFAICT you're either leaking a reference here, or you're closing an additional reference in the next patch. Sean > 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..76d46c3 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 > > +/** > + * of_find_backlight - 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 *of_find_backlight(struct device *dev) > +{ > + struct backlight_device *bd = NULL; > + struct device_node *np; > + > + if (!dev) > + return NULL; > + > + if (IS_ENABLED(CONFIG_OF) && dev->of_node) { > + 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; > +} >
Re: [Outreachy kernel] [PATCH v14 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c
On Sat, Oct 21, 2017 at 05:27:33PM +0530, Meghana Madhyastha wrote: > Rename tinydrm_of_find_backlight to of_find_backlight and move > it to linux/backlight.c so that it can be used by other drivers. > > Signed-off-by: Meghana Madhyastha> --- > Changes in v14: > -s/backlight_get/of_find_backlight/ > > 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_xrgb_to_gray8(u8 *dst, void *vaddr, > struct drm_framebuffer *fb, > } > EXPORT_SYMBOL(tinydrm_xrgb_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..53ab5a0 100644 > --- a/drivers/gpu/drm/tinydrm/mi0283qt.c > +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c > @@ -12,6 +12,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -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 = of_find_backlight(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..76d46c3 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 > > +/** > + * of_find_backlight - 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 *of_find_backlight(struct device *dev) > +{ > + struct backlight_device *bd = NULL; > + struct device_node *np; > + > + if (!dev) > + return NULL; > + > + if (IS_ENABLED(CONFIG_OF) && dev->of_node) { > + 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(of_find_backlight); > + > 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