Re: [Outreachy kernel] [PATCH v14 2/3] drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c

[Noralf Trønnes <noralf@xxxxxxxxxxx>]

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 <meghana.madhyastha@xxxxxxxxx>
> > > > > ---
> > > > > 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_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..53ab5a0 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 = 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel