[PATCH] backlight: Add of_find_backlight_by_node() function

[PATCH] backlight: Add of_find_backlight_by_node() function

[Thierry Reding]

This function finds the struct backlight_device for a given device tree
node. A dummy function is provided so that it safely compiles out if OF
support is disabled.

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 drivers/video/backlight/backlight.c | 17 +++++++++++++++++
 include/linux/backlight.h           | 10 ++++++++++
 2 files changed, 27 insertions(+)

diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 297db2f..0d1ed4f 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -370,6 +370,23 @@ void backlight_device_unregister(struct backlight_device *bd)
 }
 EXPORT_SYMBOL(backlight_device_unregister);
 
+#if IS_ENABLED(CONFIG_OF)
+static int of_parent_match(struct device *dev, void *data)
+{
+	return dev->parent && dev->parent->of_node = data;
+}
+
+struct backlight_device *of_find_backlight_by_node(struct device_node *node)
+{
+	struct device *dev;
+
+	dev = class_find_device(backlight_class, NULL, node, of_parent_match);
+
+	return dev ? to_backlight_device(dev) : NULL;
+}
+EXPORT_SYMBOL(of_find_backlight_by_node);
+#endif
+
 static void __exit backlight_class_exit(void)
 {
 	class_destroy(backlight_class);
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index 5ffc6dd..11840e9 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -134,4 +134,14 @@ struct generic_bl_info {
 	void (*kick_battery)(void);
 };
 
+#if IS_ENABLED(CONFIG_OF)
+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
+
 #endif
-- 
1.8.0

[PATCH] backlight: Add of_find_backlight_by_node() function

[Thierry Reding]

This function finds the struct backlight_device for a given device tree
node. A dummy function is provided so that it safely compiles out if OF
support is disabled.

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 drivers/video/backlight/backlight.c | 17 +++++++++++++++++
 include/linux/backlight.h           | 10 ++++++++++
 2 files changed, 27 insertions(+)

diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 297db2f..0d1ed4f 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -370,6 +370,23 @@ void backlight_device_unregister(struct backlight_device *bd)
 }
 EXPORT_SYMBOL(backlight_device_unregister);
 
+#if IS_ENABLED(CONFIG_OF)
+static int of_parent_match(struct device *dev, void *data)
+{
+	return dev->parent && dev->parent->of_node = data;
+}
+
+struct backlight_device *of_find_backlight_by_node(struct device_node *node)
+{
+	struct device *dev;
+
+	dev = class_find_device(backlight_class, NULL, node, of_parent_match);
+
+	return dev ? to_backlight_device(dev) : NULL;
+}
+EXPORT_SYMBOL(of_find_backlight_by_node);
+#endif
+
 static void __exit backlight_class_exit(void)
 {
 	class_destroy(backlight_class);
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index 5ffc6dd..11840e9 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -134,4 +134,14 @@ struct generic_bl_info {
 	void (*kick_battery)(void);
 };
 
+#if IS_ENABLED(CONFIG_OF)
+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
+
 #endif
-- 
1.8.0

Re: [PATCH] backlight: Add of_find_backlight_by_node() function

[Jingoo Han]

On Friday, November 09, 2012 11:05 PM Thierry Reding wrote
> 
> This function finds the struct backlight_device for a given device tree
> node. A dummy function is provided so that it safely compiles out if OF
> support is disabled.
> 
> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>

CC'ed Andrew Morton

Hi Thierry Reding,

The patch itself looks good.
Could you explain when this API is used?
Thank you.


Best regards,
Jingoo Han

> ---
>  drivers/video/backlight/backlight.c | 17 +++++++++++++++++
>  include/linux/backlight.h           | 10 ++++++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> index 297db2f..0d1ed4f 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -370,6 +370,23 @@ void backlight_device_unregister(struct backlight_device *bd)
>  }
>  EXPORT_SYMBOL(backlight_device_unregister);
> 
> +#if IS_ENABLED(CONFIG_OF)
> +static int of_parent_match(struct device *dev, void *data)
> +{
> +	return dev->parent && dev->parent->of_node = data;
> +}
> +
> +struct backlight_device *of_find_backlight_by_node(struct device_node *node)
> +{
> +	struct device *dev;
> +
> +	dev = class_find_device(backlight_class, NULL, node, of_parent_match);
> +
> +	return dev ? to_backlight_device(dev) : NULL;
> +}
> +EXPORT_SYMBOL(of_find_backlight_by_node);
> +#endif
> +
>  static void __exit backlight_class_exit(void)
>  {
>  	class_destroy(backlight_class);
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index 5ffc6dd..11840e9 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
> @@ -134,4 +134,14 @@ struct generic_bl_info {
>  	void (*kick_battery)(void);
>  };
> 
> +#if IS_ENABLED(CONFIG_OF)
> +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
> +
>  #endif
> --
> 1.8.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] backlight: Add of_find_backlight_by_node() function

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1254 bytes --]

On Thu, Nov 15, 2012 at 10:30:11AM +0900, Jingoo Han wrote:
> On Friday, November 09, 2012 11:05 PM Thierry Reding wrote
> > 
> > This function finds the struct backlight_device for a given device tree
> > node. A dummy function is provided so that it safely compiles out if OF
> > support is disabled.
> > 
> > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> 
> CC'ed Andrew Morton

Yes, the backlight subsystem isn't very well maintained, so I should
have added Andrew in the first place. Thanks.

> 
> Hi Thierry Reding,
> 
> The patch itself looks good.
> Could you explain when this API is used?
> Thank you.

I use this for the upcoming Tegra DRM driver in order to hook up the
backlight with the DRM driver via DT to allow switching off the
backlight when the corresponding DRM output is switched of using DPMS.
Basically what you have is something like this in the device tree:

	display {
		...

		backlight = <&backlight>;

		...
	}

Then you call something along these lines:

	np = of_parse_phandle(display, "backlight", 0);
	if (np) {
		backlight = of_find_backlight_by_node(np);
		of_node_put(np);
	}

And then use the standard backlight API on the returned pointer.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] backlight: Add of_find_backlight_by_node() function

[Jingoo Han]

On Thursday, November 15, 2012 3:52 PM Thierry Reding wrote
> On Thu, Nov 15, 2012 at 10:30:11AM +0900, Jingoo Han wrote:
> > On Friday, November 09, 2012 11:05 PM Thierry Reding wrote
> > >
> > > This function finds the struct backlight_device for a given device tree
> > > node. A dummy function is provided so that it safely compiles out if OF
> > > support is disabled.
> > >
> > > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> >
> > CC'ed Andrew Morton
> 
> Yes, the backlight subsystem isn't very well maintained, so I should
> have added Andrew in the first place. Thanks.
> 
> >
> > Hi Thierry Reding,
> >
> > The patch itself looks good.
> > Could you explain when this API is used?
> > Thank you.
> 
> I use this for the upcoming Tegra DRM driver in order to hook up the
> backlight with the DRM driver via DT to allow switching off the
> backlight when the corresponding DRM output is switched of using DPMS.
> Basically what you have is something like this in the device tree:
> 
> 	display {
> 		...
> 
> 		backlight = <&backlight>;
> 
> 		...
> 	}
> 
> Then you call something along these lines:
> 
> 	np = of_parse_phandle(display, "backlight", 0);
> 	if (np) {
> 		backlight = of_find_backlight_by_node(np);
> 		of_node_put(np);
> 	}
> 
> And then use the standard backlight API on the returned pointer.

OK, I see how this API can be called.
AS you mentioned, it will allow Tegra DRM driver to use
the backlight driver.

Acked-by: Jingoo Han <jg1.han@samsung.com>


Best regards,
Jingoo Han

> 
> Thierry

Re: [PATCH] backlight: Add of_find_backlight_by_node() function

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1802 bytes --]

On Thu, Nov 15, 2012 at 05:58:35PM +0900, Jingoo Han wrote:
> On Thursday, November 15, 2012 3:52 PM Thierry Reding wrote
> > On Thu, Nov 15, 2012 at 10:30:11AM +0900, Jingoo Han wrote:
> > > On Friday, November 09, 2012 11:05 PM Thierry Reding wrote
> > > >
> > > > This function finds the struct backlight_device for a given device tree
> > > > node. A dummy function is provided so that it safely compiles out if OF
> > > > support is disabled.
> > > >
> > > > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> > >
> > > CC'ed Andrew Morton
> > 
> > Yes, the backlight subsystem isn't very well maintained, so I should
> > have added Andrew in the first place. Thanks.
> > 
> > >
> > > Hi Thierry Reding,
> > >
> > > The patch itself looks good.
> > > Could you explain when this API is used?
> > > Thank you.
> > 
> > I use this for the upcoming Tegra DRM driver in order to hook up the
> > backlight with the DRM driver via DT to allow switching off the
> > backlight when the corresponding DRM output is switched of using DPMS.
> > Basically what you have is something like this in the device tree:
> > 
> > 	display {
> > 		...
> > 
> > 		backlight = <&backlight>;
> > 
> > 		...
> > 	}
> > 
> > Then you call something along these lines:
> > 
> > 	np = of_parse_phandle(display, "backlight", 0);
> > 	if (np) {
> > 		backlight = of_find_backlight_by_node(np);
> > 		of_node_put(np);
> > 	}
> > 
> > And then use the standard backlight API on the returned pointer.
> 
> OK, I see how this API can be called.
> AS you mentioned, it will allow Tegra DRM driver to use
> the backlight driver.
> 
> Acked-by: Jingoo Han <jg1.han@samsung.com>

Andrew,

any chance we could still get this in for the 3.8 merge window?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] backlight: Add of_find_backlight_by_node() function

[Andrew Morton]

On Fri,  9 Nov 2012 15:04:38 +0100
Thierry Reding <thierry.reding@avionic-design.de> wrote:

> This function finds the struct backlight_device for a given device tree
> node. A dummy function is provided so that it safely compiles out if OF
> support is disabled.
> 
> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> ---
>  drivers/video/backlight/backlight.c | 17 +++++++++++++++++
>  include/linux/backlight.h           | 10 ++++++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> index 297db2f..0d1ed4f 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -370,6 +370,23 @@ void backlight_device_unregister(struct backlight_device *bd)
>  }
>  EXPORT_SYMBOL(backlight_device_unregister);
>  
> +#if IS_ENABLED(CONFIG_OF)

Using IS_ENABLED() was odd.  We'll never support CONFIG_OF=m, so can't
we use plain old "#ifdef CONFIG_OF" here?

--- a/drivers/video/backlight/backlight.c~backlight-add-of_find_backlight_by_node-function-fix
+++ a/drivers/video/backlight/backlight.c
@@ -370,7 +370,7 @@ void backlight_device_unregister(struct 
 }
 EXPORT_SYMBOL(backlight_device_unregister);
 
-#if IS_ENABLED(CONFIG_OF)
+#ifdef CONFIG_OF
 static int of_parent_match(struct device *dev, void *data)
 {
 	return dev->parent && dev->parent->of_node = data;
--- a/include/linux/backlight.h~backlight-add-of_find_backlight_by_node-function-fix
+++ a/include/linux/backlight.h
@@ -134,7 +134,7 @@ struct generic_bl_info {
 	void (*kick_battery)(void);
 };
 
-#if IS_ENABLED(CONFIG_OF)
+#ifdef CONFIG_OF
 struct backlight_device *of_find_backlight_by_node(struct device_node *node);
 #else
 static inline struct backlight_device *
_


> +static int of_parent_match(struct device *dev, void *data)
> +{
> +	return dev->parent && dev->parent->of_node = data;
> +}
> +
> +struct backlight_device *of_find_backlight_by_node(struct device_node *node)
> +{
> +	struct device *dev;
> +
> +	dev = class_find_device(backlight_class, NULL, node, of_parent_match);
> +
> +	return dev ? to_backlight_device(dev) : NULL;
> +}
> +EXPORT_SYMBOL(of_find_backlight_by_node);

It's a global, exported-to-modules function.  We should document such
major interfaces.  Unless they are dead trivial, but I don't think this
one is that simple.  The semantics of the return value could be
explained, and callers should be told that of_find_backlight_by_node()
took a ref on the returned device, and that they need to run
put_device(retval->dev), if retval was not NULL.

And anything else which might be useful.

Re: [PATCH] backlight: Add of_find_backlight_by_node() function

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3055 bytes --]

On Fri, Nov 16, 2012 at 01:16:45PM -0800, Andrew Morton wrote:
> On Fri,  9 Nov 2012 15:04:38 +0100
> Thierry Reding <thierry.reding@avionic-design.de> wrote:
> 
> > This function finds the struct backlight_device for a given device tree
> > node. A dummy function is provided so that it safely compiles out if OF
> > support is disabled.
> > 
> > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> > ---
> >  drivers/video/backlight/backlight.c | 17 +++++++++++++++++
> >  include/linux/backlight.h           | 10 ++++++++++
> >  2 files changed, 27 insertions(+)
> > 
> > diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> > index 297db2f..0d1ed4f 100644
> > --- a/drivers/video/backlight/backlight.c
> > +++ b/drivers/video/backlight/backlight.c
> > @@ -370,6 +370,23 @@ void backlight_device_unregister(struct backlight_device *bd)
> >  }
> >  EXPORT_SYMBOL(backlight_device_unregister);
> >  
> > +#if IS_ENABLED(CONFIG_OF)
> 
> Using IS_ENABLED() was odd.  We'll never support CONFIG_OF=m, so can't
> we use plain old "#ifdef CONFIG_OF" here?
> 
> --- a/drivers/video/backlight/backlight.c~backlight-add-of_find_backlight_by_node-function-fix
> +++ a/drivers/video/backlight/backlight.c
> @@ -370,7 +370,7 @@ void backlight_device_unregister(struct 
>  }
>  EXPORT_SYMBOL(backlight_device_unregister);
>  
> -#if IS_ENABLED(CONFIG_OF)
> +#ifdef CONFIG_OF
>  static int of_parent_match(struct device *dev, void *data)
>  {
>  	return dev->parent && dev->parent->of_node == data;
> --- a/include/linux/backlight.h~backlight-add-of_find_backlight_by_node-function-fix
> +++ a/include/linux/backlight.h
> @@ -134,7 +134,7 @@ struct generic_bl_info {
>  	void (*kick_battery)(void);
>  };
>  
> -#if IS_ENABLED(CONFIG_OF)
> +#ifdef CONFIG_OF
>  struct backlight_device *of_find_backlight_by_node(struct device_node *node);
>  #else
>  static inline struct backlight_device *
> _

Yes, that should work.

> > +static int of_parent_match(struct device *dev, void *data)
> > +{
> > +	return dev->parent && dev->parent->of_node == data;
> > +}
> > +
> > +struct backlight_device *of_find_backlight_by_node(struct device_node *node)
> > +{
> > +	struct device *dev;
> > +
> > +	dev = class_find_device(backlight_class, NULL, node, of_parent_match);
> > +
> > +	return dev ? to_backlight_device(dev) : NULL;
> > +}
> > +EXPORT_SYMBOL(of_find_backlight_by_node);
> 
> It's a global, exported-to-modules function.  We should document such
> major interfaces.  Unless they are dead trivial, but I don't think this
> one is that simple.  The semantics of the return value could be
> explained, and callers should be told that of_find_backlight_by_node()
> took a ref on the returned device, and that they need to run
> put_device(retval->dev), if retval was not NULL.
> 
> And anything else which might be useful.

Yes, I should have thought about that. I'll add some proper kerneldoc
and repost.

Thanks for reviewing,
Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] backlight: Add of_find_backlight_by_node() function

[Grant Likely]

On Wed,  7 Nov 2012 23:08:39 +0100, Thierry Reding <thierry.reding@avionic-design.de> wrote:
> This function finds the struct backlight_device for a given device tree
> node. A dummy function is provided so that it safely compiles out if OF
> support is disabled.
> 
> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>

Looks okay to me.

Reviewed-by: Grant Likely <grant.likely@secretlab.ca>
> ---
>  drivers/video/backlight/backlight.c | 17 +++++++++++++++++
>  include/linux/backlight.h           | 10 ++++++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> index 297db2f..0d1ed4f 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -370,6 +370,23 @@ void backlight_device_unregister(struct backlight_device *bd)
>  }
>  EXPORT_SYMBOL(backlight_device_unregister);
>  
> +#if IS_ENABLED(CONFIG_OF)
> +static int of_parent_match(struct device *dev, void *data)
> +{
> +	return dev->parent && dev->parent->of_node = data;
> +}
> +
> +struct backlight_device *of_find_backlight_by_node(struct device_node *node)
> +{
> +	struct device *dev;
> +
> +	dev = class_find_device(backlight_class, NULL, node, of_parent_match);
> +
> +	return dev ? to_backlight_device(dev) : NULL;
> +}
> +EXPORT_SYMBOL(of_find_backlight_by_node);
> +#endif
> +
>  static void __exit backlight_class_exit(void)
>  {
>  	class_destroy(backlight_class);
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index 5ffc6dd..11840e9 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
> @@ -134,4 +134,14 @@ struct generic_bl_info {
>  	void (*kick_battery)(void);
>  };
>  
> +#if IS_ENABLED(CONFIG_OF)
> +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
> +
>  #endif
> -- 
> 1.8.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.