On Sat, Oct 21, 2017 at 05:26:33PM +0530, Meghana Madhyastha wrote:

Move the helper functions enable_backlight and disable_backlight from tinydrm-helpers.c to backlight.h as static inline functions so that they can be used by other drivers.

Signed-off-by: Meghana Madhyastha meghana.madhyastha@gmail.com

Reviewed-by: Sean Paul seanpaul@chromium.org

---

Changes in v14:

• None

drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 55 -------------------------- drivers/gpu/drm/tinydrm/mipi-dbi.c | 5 ++- include/drm/tinydrm/tinydrm-helpers.h | 2 - include/linux/backlight.h | 30 ++++++++++++++ 4 files changed, 33 insertions(+), 59 deletions(-)

diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c index bd6cce0..a42dee6 100644 --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c @@ -276,61 +276,6 @@ struct backlight_device *tinydrm_of_find_backlight(struct device *dev) } EXPORT_SYMBOL(tinydrm_of_find_backlight);

-/**

• • tinydrm_enable_backlight - Enable backlight helper
• • @backlight: Backlight device
• • Returns:
• • Zero on success, negative error code on failure.
• */

-int tinydrm_enable_backlight(struct backlight_device *backlight) -{

• unsigned int old_state;
• int ret;
• if (!backlight)

• return 0;
  

• old_state = backlight->props.state;
• backlight->props.state &= ~BL_CORE_FBBLANK;
• DRM_DEBUG_KMS("Backlight state: 0x%x -> 0x%x\n", old_state,

•       backlight->props.state);
  

• ret = backlight_update_status(backlight);
• if (ret)

• DRM_ERROR("Failed to enable backlight %d\n", ret);
  

• return ret;

-} -EXPORT_SYMBOL(tinydrm_enable_backlight);

-/**

• • tinydrm_disable_backlight - Disable backlight helper
• • @backlight: Backlight device
• • Returns:
• • Zero on success, negative error code on failure.
• */

-int tinydrm_disable_backlight(struct backlight_device *backlight) -{

• unsigned int old_state;
• int ret;
• if (!backlight)

• return 0;
  

• old_state = backlight->props.state;
• backlight->props.state |= BL_CORE_FBBLANK;
• DRM_DEBUG_KMS("Backlight state: 0x%x -> 0x%x\n", old_state,

•       backlight->props.state);
  

• ret = backlight_update_status(backlight);
• if (ret)

• DRM_ERROR("Failed to disable backlight %d\n", ret);
  

• return ret;

-} -EXPORT_SYMBOL(tinydrm_disable_backlight);

#if IS_ENABLED(CONFIG_SPI)

/** diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c index d43e992..2561549 100644 --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c @@ -12,6 +12,7 @@ #include <drm/drm_gem_framebuffer_helper.h> #include <drm/tinydrm/mipi-dbi.h> #include <drm/tinydrm/tinydrm-helpers.h> +#include <linux/backlight.h> #include <linux/debugfs.h> #include <linux/dma-buf.h> #include <linux/gpio/consumer.h> @@ -280,7 +281,7 @@ void mipi_dbi_pipe_enable(struct drm_simple_display_pipe *pipe, if (fb) fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0);

• tinydrm_enable_backlight(mipi->backlight);

• backlight_enable(mipi->backlight);

} EXPORT_SYMBOL(mipi_dbi_pipe_enable);

@@ -319,7 +320,7 @@ void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe) mipi->enabled = false;

if (mipi->backlight)

• tinydrm_disable_backlight(mipi->backlight);
  

• backlight_disable(mipi->backlight);
  

  else mipi_dbi_blank(mipi);

} diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h index d554ded..f54fae0 100644 --- a/include/drm/tinydrm/tinydrm-helpers.h +++ b/include/drm/tinydrm/tinydrm-helpers.h @@ -47,8 +47,6 @@ 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); -int tinydrm_enable_backlight(struct backlight_device *backlight); -int tinydrm_disable_backlight(struct backlight_device *backlight);

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); diff --git a/include/linux/backlight.h b/include/linux/backlight.h index 5f2fd61..b88fabb 100644 --- a/include/linux/backlight.h +++ b/include/linux/backlight.h @@ -129,6 +129,36 @@ static inline int backlight_update_status(struct backlight_device *bd) return ret; }

+/**

• • backlight_enable - Enable backlight
• • @bd: the backlight device to enable
• */

+static inline int backlight_enable(struct backlight_device *bd) +{

• if (!bd)

• return 0;
  

• bd->props.power = FB_BLANK_UNBLANK;
• bd->props.state &= ~BL_CORE_FBBLANK;
• return backlight_update_status(bd);

+}

+/**

• • backlight_disable - Disable backlight
• • @bd: the backlight device to disable
• */

+static inline int backlight_disable(struct backlight_device *bd) +{

• if (!bd)

• return 0;
  

• bd->props.power = FB_BLANK_POWERDOWN;
• bd->props.state |= BL_CORE_FBBLANK;
• return backlight_update_status(bd);

+}

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); -- 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@googlegroups.com. To post to this group, send email to outreachy-kernel@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/203e216d74645b795687a2978.... For more options, visit https://groups.google.com/d/optout.

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@gmail.com

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); 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_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..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(&bd->dev);
  

+}

As I mentioned in my last review, I don't think we need this function.

Sean

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 *of_find_backlight(struct device *dev); +#else +static inline struct backlight_device *of_find_backlight(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@googlegroups.com. To post to this group, send email to outreachy-kernel@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/678c3adb07b729acf6e80119a.... For more options, visit https://groups.google.com/d/optout.

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 meghana.madhyastha@gmail.com

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); 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_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..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(&bd->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.

Sean

Daniel.

...

...

• 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 *of_find_backlight(struct device *dev); +#else +static inline struct backlight_device *of_find_backlight(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@googlegroups.com. To post to this group, send email to outreachy-kernel@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/678c3adb07b729acf6e80119a.... For more options, visit https://groups.google.com/d/optout.

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@gmail.com

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.

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;

+} +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_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..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(&bd->dev);
  

+}

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 *of_find_backlight(struct device *dev); +#else +static inline struct backlight_device *of_find_backlight(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@googlegroups.com. To post to this group, send email to outreachy-kernel@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/678c3adb07b729acf6e80119a.... For more options, visit https://groups.google.com/d/optout.

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@gmail.com

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.

-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 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_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..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(&bd->dev);
  

+}

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 *of_find_backlight(struct device *dev); +#else +static inline struct backlight_device *of_find_backlight(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@googlegroups.com. To post to this group, send email to outreachy-kernel@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/678c3adb07b729acf6e80119a.... For more options, visit https://groups.google.com/d/optout.

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@gmail.com

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.

Den 11.12.2017 15.58, skrev Meghana Madhyastha:

On Mon, Dec 11, 2017 at 03:12:06PM +0100, Noralf Trønnes wrote:

...

Den 11.12.2017 14.17, skrev Meghana Madhyastha:

...

On Sat, Dec 09, 2017 at 03:09:28PM +0100, Noralf Trønnes wrote:

...

Den 21.10.2017 13.55, skrev Meghana Madhyastha:

...

Changes in v14:

• s/backlight_get/of_find_backlight/ in patch 2/3
• Change commit message in patch 3/3 from requiring to acquiring

Meghana Madhyastha (3): drm/tinydrm: Move helper functions from tinydrm-helpers to backlight.h drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c drm/tinydrm: Add devres versions of of_find_backlight

I tried the patchset and this is what I got:

[    8.057792] Unable to handle kernel paging request at virtual address fffffe6b [    8.069118] pgd = 93817cf7 [    8.075704] [fffffe6b] *pgd=1bfd7861, *pte=00000000, *ppte=00000000 [    8.086047] Internal error: Oops: 37 [#1] ARM [    8.094285] Modules linked in: mi0283qt(+) mipi_dbi tinydrm backlight bcm2835_rng rng_core [    8.110535] CPU: 0 PID: 121 Comm: systemd-udevd Not tainted 4.15.0-rc2+ #2 [    8.121531] Hardware name: BCM2835 [    8.128964] task: 85dc6e21 task.stack: 4d8f2d19 [    8.137482] PC is at kobject_put+0x18/0x1dc [    8.145556] LR is at put_device+0x24/0x28 [    8.153312] pc : [<c076a1b4>]    lr : [<c048edb8>] psr: a0000013 [    8.163321] sp : d6d33c18  ip : d6d33c40  fp : d6d33c3c [    8.172238] r10: c0496d38  r9 : 00000000  r8 : d6e46680 [    8.181108] r7 : 00000004  r6 : d766bc00  r5 : d6d33c70  r4 : fffffe4b [    8.191237] r3 : bf0113b8  r2 : d766bd2c  r1 : d6e46710  r0 : fffffe4b [    8.201248] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM Segment none [    8.211869] Control: 00c5387d  Table: 16d20008  DAC: 00000051 [    8.221141] Process systemd-udevd (pid: 121, stack limit = 0xa0ab0c84) [    8.231264] Stack: (0xd6d33c18 to 0xd6d34000) [    8.239228] 3c00: bf011858 c049728c [    8.254539] 3c20: d7737010 d6e46700 d6d33c70 d766bc00 d6d33c4c d6d33c40 c048edb8 c076a1a8 [    8.270045] 3c40: d6d33c5c d6d33c50 bf0113dc c048eda0 d6d33c6c d6d33c60 c0496fc4 bf0113c4 [    8.285883] 3c60: d6d33ca4 d6d33c70 c04979f0 c0496fb4 d7737000 d6e46700 d6d33c9c d766bc00 [    8.301819] 3c80: 00000000 bf030100 c0d369dc c0d369e0 0000000e fffffdfb d6d33cb4 d6d33ca8 [    8.317917] 3ca0: c0497ad0 c0497858 d6d33cec d6d33cb8 c0493efc c0497a94 c057e738 c057d26c [    8.334043] 3cc0: d6d33cec d766bc00 d766bc34 bf030100 c0c4a5d8 00000001 d6e46464 00000028 [    8.350344] 3ce0: d6d33d0c d6d33cf0 c049414c c0493c4c 00000001 00000000 bf030100 c04940b0 [    8.367036] 3d00: d6d33d34 d6d33d10 c0492018 c04940bc d754f28c d76afbb0 d6dca534 bf030100 [    8.383997] 3d20: 00000000 d6dca500 d6d33d44 d6d33d38 c0493750 c0491fa8 d6d33d6c d6d33d48 [    8.401301] 3d40: c0493124 c0493734 bf02f4d6 d6d33d58 bf030100 bf033000 c0c04048 00000000 [    8.418938] 3d60: d6d33d84 d6d33d70 c0494954 c0493048 bf030140 bf033000 d6d33d94 d6d33d88 [    8.436692] 3d80: c04c8794 c04948b4 d6d33da4 d6d33d98 bf033020 c04c8748 d6d33e1c d6d33da8 [    8.454638] 3da0: c0101a30 bf03300c d6d33dcc d6d33db8 c01013f4 c03f7c64 c0221690 60000013 [    8.472918] 3dc0: d6d33e44 d6d33dd0 c010d04c c01013e0 d7401e40 014000c0 0000000c c0221770 [    8.491185] 3de0: d6d33e1c d6d33df0 c0221770 c021e29c 00000001 fc36740d bf030140 00000001 [    8.509455] 3e00: d6e46500 d6e46440 00000001 d6e46464 d6d33e44 d6d33e20 c0178628 c0101978 [    8.527727] 3e20: bf030140 d6e46440 d6d33e44 d6d33f40 00000001 bf030140 d6d33f1c d6d33e48 [    8.546000] 3e40: c0177810 c01785c8 bf03014c 00007fff bf030140 c0174ae8 d6d33f38 d6d33eb8 [    8.564264] 3e60: d6d33e9c b6d4f004 dcb059a4 c0c1619c bf030324 d6e46448 bf030188 dcad6000 [    8.582529] 3e80: 00000000 d6ca3e40 00000000 00000000 00000000 00000000 00000000 00000000 [    8.600797] 3ea0: 00000000 00000000 00000000 00000000 6e72656b 00006c65 00000000 00000000 [    8.619065] 3ec0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [    8.637319] 3ee0: 00000000 00000000 00000000 fc36740d 7fffffff 00000000 b6d4f004 0000000d [    8.655582] 3f00: 0000017b c0108044 d6d32000 00000000 d6d33fa4 d6d33f20 c0177f94 c0175d00 [    8.673835] 3f20: 7fffffff 00000000 00000003 00000000 00000000 dcad6000 0002f9f4 00000000 [    8.692093] 3f40: dcad6b80 dcad6000 0002f9f4 dcb05274 dcaf9e6a dcafa860 00003000 00003240 [    8.710350] 3f60: 00000000 00000000 00000000 00001e34 0000002e 0000002f 00000019 00000000 [    8.728612] 3f80: 00000013 00000000 00000000 1ef80900 00000000 00000000 00000000 d6d33fa8 [    8.746870] 3fa0: c0107e60 c0177f04 1ef80900 00000000 0000000d b6d4f004 00000000 b6d4f928 [    8.765132] 3fc0: 1ef80900 00000000 00000000 0000017b b6d4f004 00020000 00af6e18 00000000 [    8.783394] 3fe0: bec95200 bec951f0 b6d4709c b6ea3c40 60000010 0000000d 00000000 00000000 [    8.801685] [<c076a1b4>] (kobject_put) from [<c048edb8>] (put_device+0x24/0x28) [    8.814234] [<c048edb8>] (put_device) from [<bf0113dc>] (devm_backlight_put+0x24/0x28 [backlight]) [    8.833323] [<bf0113dc>] (devm_backlight_put [backlight]) from [<c0496fc4>] (devm_action_release+0x1c/0x20) [    8.853182] [<c0496fc4>] (devm_action_release) from [<c04979f0>] (release_nodes+0x1a4/0x1cc) [    8.871702] [<c04979f0>] (release_nodes) from [<c0497ad0>] (devres_release_all+0x48/0x54) [    8.889956] [<c0497ad0>] (devres_release_all) from [<c0493efc>] (driver_probe_device+0x2bc/0x470) [    8.908937] [<c0493efc>] (driver_probe_device) from [<c049414c>] (__driver_attach+0x9c/0x100) [    8.927557] [<c049414c>] (__driver_attach) from [<c0492018>] (bus_for_each_dev+0x7c/0xa0) [    8.945822] [<c0492018>] (bus_for_each_dev) from [<c0493750>] (driver_attach+0x28/0x30) [    8.963916] [<c0493750>] (driver_attach) from [<c0493124>] (bus_add_driver+0xe8/0x240) [    8.981929] [<c0493124>] (bus_add_driver) from [<c0494954>] (driver_register+0xac/0xf0) [    9.000048] [<c0494954>] (driver_register) from [<c04c8794>] (__spi_register_driver+0x58/0x6c) [    9.018823] [<c04c8794>] (__spi_register_driver) from [<bf033020>] (mi0283qt_spi_driver_init+0x20/0x1000 [mi0283qt]) [    9.039575] [<bf033020>] (mi0283qt_spi_driver_init [mi0283qt]) from [<c0101a30>] (do_one_initcall+0xc4/0x184) [    9.059727] [<c0101a30>] (do_one_initcall) from [<c0178628>] (do_init_module+0x6c/0x1fc) [    9.078036] [<c0178628>] (do_init_module) from [<c0177810>] (load_module+0x1b1c/0x2090) [    9.096248] [<c0177810>] (load_module) from [<c0177f94>] (SyS_finit_module+0x9c/0xcc) [    9.114284] [<c0177f94>] (SyS_finit_module) from [<c0107e60>] (ret_fast_syscall+0x0/0x54) [    9.132673] Code: e24cb004 e24dd00c e2504000 0a00005e (e5d43020) [    9.144181] ---[ end trace 149c05934b6a6dcc ]---

Is the reason possibly because we have omitted error checking on the return value of backlight_enable function ? tinydrm_enable_backlight and tinydrm_disable_baclight do this. Eg. ret = backlight_update_status(backlight); if (ret) DRM_ERROR("Failed to enable backlight %d\n", ret);

I'm not sure, just asking whether this could be a possible reason for the above trace.

The crash happens during probe. I guess you'll figure this out when you get some hw to test on.

I have set up the raspberry pi and have built and boot into the custom kernel but I am waiting for the panel to arrive. Meanwhile, any thoughts on error message ? Sorry for the trivial question, but I did not quite understand the crash message and how to replicate it.

of_find_backlight() can return an error pointer (-EPROBE_DEFER):

diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c index 4bb7bf3ee443..57370c5d51f0 100644 --- a/drivers/video/backlight/backlight.c +++ b/drivers/video/backlight/backlight.c @@ -635,8 +635,8 @@ struct backlight_device *devm_of_find_backlight(struct device *dev)         int ret;

bd = of_find_backlight(dev); -       if (!bd) -               return NULL; +       if (IS_ERR_OR_NULL(bd)) +               return bd;

ret = devm_add_action(dev, devm_backlight_put, bd);         if (ret) {

That solved the crash, but the backlight didn't turn on. I had to do this as well:

diff --git a/include/linux/backlight.h b/include/linux/backlight.h index 5c441d4c049c..6f9925f10a7c 100644 --- a/include/linux/backlight.h +++ b/include/linux/backlight.h @@ -139,6 +139,8 @@ static inline int backlight_enable(struct backlight_device *bd)         if (!bd)                 return 0;

+       if (!bd->props.brightness) +               bd->props.brightness = bd->props.max_brightness;         bd->props.power = FB_BLANK_UNBLANK;         bd->props.state &= ~BL_CORE_FBBLANK;

This is my backlight node[1]:

backlight: backlight {     compatible = "gpio-backlight";     gpios = <&gpio 18 0>; // GPIO_ACTIVE_HIGH };

Not certain that this is the "correct" solution, but I can't use the gpio-backlight property default-on, because that would turn on backlight before the pipeline is enabled.

You can dry-run the driver without a panel connected, it can work without being able to read from the controller.

Noralf.

[1] https://github.com/notro/tinydrm/blob/master/rpi-overlays/rpi-display-overla...

Den 11.12.2017 18.56, skrev Noralf Trønnes:

Den 11.12.2017 18.45, skrev Noralf Trønnes:

...

Den 11.12.2017 15.58, skrev Meghana Madhyastha:

...

On Mon, Dec 11, 2017 at 03:12:06PM +0100, Noralf Trønnes wrote:

...

Den 11.12.2017 14.17, skrev Meghana Madhyastha:

...

On Sat, Dec 09, 2017 at 03:09:28PM +0100, Noralf Trønnes wrote:

...

Den 21.10.2017 13.55, skrev Meghana Madhyastha: > Changes in v14: > - s/backlight_get/of_find_backlight/ in patch 2/3 > - Change commit message in patch 3/3 from requiring to acquiring > > Meghana Madhyastha (3): >    drm/tinydrm: Move helper functions from tinydrm-helpers to > backlight.h >    drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c >    drm/tinydrm: Add devres versions of of_find_backlight I tried the patchset and this is what I got:

[    8.057792] Unable to handle kernel paging request at virtual address fffffe6b

<snip> >>>>> [    9.144181] ---[ end trace 149c05934b6a6dcc ]--- >>>> Is the reason possibly because we have omitted error checking on the >>>> return value of backlight_enable function ? >>>> tinydrm_enable_backlight and >>>> tinydrm_disable_baclight do this. >>>> Eg. >>>> ret = backlight_update_status(backlight); >>>> if (ret) >>>>     DRM_ERROR("Failed to enable backlight %d\n", ret); >>>> >>>> I'm not sure, just asking whether this could be a possible reason >>>> for the above trace. >>> The crash happens during probe. >>> I guess you'll figure this out when you get some hw to test on. >> I have set up the raspberry pi and have built and boot into the >> custom kernel >> but I am waiting for the panel to arrive. Meanwhile, any thoughts on >> error message ? Sorry for the trivial question, but I did not quite >> understand the crash message and how to replicate it. > > of_find_backlight() can return an error pointer (-EPROBE_DEFER): > > diff --git a/drivers/video/backlight/backlight.c > b/drivers/video/backlight/backlight.c > index 4bb7bf3ee443..57370c5d51f0 100644 > --- a/drivers/video/backlight/backlight.c > +++ b/drivers/video/backlight/backlight.c > @@ -635,8 +635,8 @@ struct backlight_device > *devm_of_find_backlight(struct device *dev) >         int ret; > >         bd = of_find_backlight(dev); > -       if (!bd) > -               return NULL; > +       if (IS_ERR_OR_NULL(bd)) > +               return bd; > >         ret = devm_add_action(dev, devm_backlight_put, bd); >         if (ret) { > > That solved the crash, but the backlight didn't turn on. > I had to do this as well: > > diff --git a/include/linux/backlight.h b/include/linux/backlight.h > index 5c441d4c049c..6f9925f10a7c 100644 > --- a/include/linux/backlight.h > +++ b/include/linux/backlight.h > @@ -139,6 +139,8 @@ static inline int backlight_enable(struct > backlight_device *bd) >         if (!bd) >                 return 0; > > +       if (!bd->props.brightness) > +               bd->props.brightness = bd->props.max_brightness;

No, this is wrong, it should happen on probe, not every time it's enabled. So maybe it should be done in of_find_backlight() or something. I see that I'm currently doing it in tinydrm_of_find_backlight().

I'm not happy with this brightness hack of mine really.

Since I last looked at this I see that pwm_bl has gained the ability to start in a power off state (pwm_backlight_initial_power_state()). However the gpio_backlight driver doesn't do this. gpio_backlight has a 'default-on' property, but the problem is that it sets brightness, not power state. So the absense of the property sets brightness to zero, which makes the driver turn off backlight on probe. This seems to be fine, but then systemd-backlight comes along and restores brightness to 1, since that's what it was on shutdown. This turns on backlight and now I have a glaring white uninitialized panel waiting for the display driver to set it up.

This patch would solve my problem:

diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c index e470da95d806..54bb722e1ea3 100644 --- a/drivers/video/backlight/gpio_backlight.c +++ b/drivers/video/backlight/gpio_backlight.c @@ -142,7 +142,8 @@ static int gpio_backlight_probe(struct platform_device *pdev)                 return PTR_ERR(bl);         }

-       bl->props.brightness = gbl->def_value; +       bl->props.brightness = 1; +       bl->props.state = gbl->def_value ? 0 : BL_CORE_FBBLANK;         backlight_update_status(bl);

platform_set_drvdata(pdev, bl);

The problem is that this will most likely break 2 in-kernel users of gpio-backlight which doesn't set the 'default-on' property:   arch/arm/boot/dts/omap4-var-dvk-om44.dts   arm/boot/dts/imx27-eukrea-mbimxsd27-baseboard.dts

AFAICT they rely on systemd-backlight to turn on backlight by setting brightness to 1.

So maybe my hack is _the_ soulution after all, but I'm no expert on the backlight subsystem and it's corner cases.

Noralf.

...

bd->props.power = FB_BLANK_UNBLANK;         bd->props.state &= ~BL_CORE_FBBLANK;

This is my backlight node[1]:

backlight: backlight {     compatible = "gpio-backlight";     gpios = <&gpio 18 0>; // GPIO_ACTIVE_HIGH };

Not certain that this is the "correct" solution, but I can't use the gpio-backlight property default-on, because that would turn on backlight before the pipeline is enabled.

You can dry-run the driver without a panel connected, it can work without being able to read from the controller.

Noralf.

[1] https://github.com/notro/tinydrm/blob/master/rpi-overlays/rpi-display-overla...

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

Den 21.12.2017 14.05, skrev Daniel Vetter:

On Thu, Dec 21, 2017 at 11:52:43AM +0100, Noralf Trønnes wrote:

...

Den 11.12.2017 18.56, skrev Noralf Trønnes:

...

Den 11.12.2017 18.45, skrev Noralf Trønnes:

...

Den 11.12.2017 15.58, skrev Meghana Madhyastha:

...

On Mon, Dec 11, 2017 at 03:12:06PM +0100, Noralf Trønnes wrote:

...

Den 11.12.2017 14.17, skrev Meghana Madhyastha: > On Sat, Dec 09, 2017 at 03:09:28PM +0100, Noralf Trønnes wrote: >> Den 21.10.2017 13.55, skrev Meghana Madhyastha: >>> Changes in v14: >>> - s/backlight_get/of_find_backlight/ in patch 2/3 >>> - Change commit message in patch 3/3 from requiring to acquiring >>> >>> Meghana Madhyastha (3): >>>    drm/tinydrm: Move helper functions from >>> tinydrm-helpers to backlight.h >>>    drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c >>>    drm/tinydrm: Add devres versions of of_find_backlight >> I tried the patchset and this is what I got: >> >> [    8.057792] Unable to handle kernel paging >> request at virtual address >> fffffe6b

<snip> >>>>> [    9.144181] ---[ end trace 149c05934b6a6dcc ]--- >>>> Is the reason possibly because we have omitted error checking on the >>>> return value of backlight_enable function ? >>>> tinydrm_enable_backlight and >>>> tinydrm_disable_baclight do this. >>>> Eg. >>>> ret = backlight_update_status(backlight); >>>> if (ret) >>>>     DRM_ERROR("Failed to enable backlight %d\n", ret); >>>> >>>> I'm not sure, just asking whether this could be a possible reason >>>> for the above trace. >>> The crash happens during probe. >>> I guess you'll figure this out when you get some hw to test on. >> I have set up the raspberry pi and have built and boot into the >> custom kernel >> but I am waiting for the panel to arrive. Meanwhile, any thoughts on >> error message ? Sorry for the trivial question, but I did not quite >> understand the crash message and how to replicate it. > of_find_backlight() can return an error pointer (-EPROBE_DEFER): > > diff --git a/drivers/video/backlight/backlight.c > b/drivers/video/backlight/backlight.c > index 4bb7bf3ee443..57370c5d51f0 100644 > --- a/drivers/video/backlight/backlight.c > +++ b/drivers/video/backlight/backlight.c > @@ -635,8 +635,8 @@ struct backlight_device > *devm_of_find_backlight(struct device *dev) >         int ret; > >         bd = of_find_backlight(dev); > -       if (!bd) > -               return NULL; > +       if (IS_ERR_OR_NULL(bd)) > +               return bd; > >         ret = devm_add_action(dev, devm_backlight_put, bd); >         if (ret) { > > That solved the crash, but the backlight didn't turn on. > I had to do this as well: > > diff --git a/include/linux/backlight.h b/include/linux/backlight.h > index 5c441d4c049c..6f9925f10a7c 100644 > --- a/include/linux/backlight.h > +++ b/include/linux/backlight.h > @@ -139,6 +139,8 @@ static inline int backlight_enable(struct > backlight_device *bd) >         if (!bd) >                 return 0; > > +       if (!bd->props.brightness) > +               bd->props.brightness = bd->props.max_brightness; No, this is wrong, it should happen on probe, not every time it's enabled. So maybe it should be done in of_find_backlight() or something. I see that I'm currently doing it in tinydrm_of_find_backlight().

I'm not happy with this brightness hack of mine really.

Since I last looked at this I see that pwm_bl has gained the ability to start in a power off state (pwm_backlight_initial_power_state()). However the gpio_backlight driver doesn't do this. gpio_backlight has a 'default-on' property, but the problem is that it sets brightness, not power state. So the absense of the property sets brightness to zero, which makes the driver turn off backlight on probe. This seems to be fine, but then systemd-backlight comes along and restores brightness to 1, since that's what it was on shutdown. This turns on backlight and now I have a glaring white uninitialized panel waiting for the display driver to set it up.

This patch would solve my problem:

diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c index e470da95d806..54bb722e1ea3 100644 --- a/drivers/video/backlight/gpio_backlight.c +++ b/drivers/video/backlight/gpio_backlight.c @@ -142,7 +142,8 @@ static int gpio_backlight_probe(struct platform_device *pdev)                 return PTR_ERR(bl);         }

-       bl->props.brightness = gbl->def_value; +       bl->props.brightness = 1; +       bl->props.state = gbl->def_value ? 0 : BL_CORE_FBBLANK;         backlight_update_status(bl);

platform_set_drvdata(pdev, bl);

The problem is that this will most likely break 2 in-kernel users of gpio-backlight which doesn't set the 'default-on' property:   arch/arm/boot/dts/omap4-var-dvk-om44.dts   arm/boot/dts/imx27-eukrea-mbimxsd27-baseboard.dts

AFAICT they rely on systemd-backlight to turn on backlight by setting brightness to 1.

So maybe my hack is _the_ soulution after all, but I'm no expert on the backlight subsystem and it's corner cases.

Can we fix the dts instead?

Isn't Device Tree ABI, ie. new kernels should work backwards with existing dtb's? We will break that contract if we change gpio_backlight like I proposed. Another solution is to add a new DT property: 'default-off':

Optional properties:   - default-on: enable the backlight at boot.   - default-off: disable the backlight at boot by setting backlight in power     state off with brightness set to one.

Not sure how well that will fly with Rob, it smells like a hack.

Noralf.

Untangling the on/off vs brightness mess definitely sounds like a good idea, especially since some backlights have a minimal brightness (which doesn't match to off), because too low screws up the backlight and destroys the panel. -Daniel

Den 21.12.2017 15.08, skrev Daniel Vetter:

On Thu, Dec 21, 2017 at 2:44 PM, Noralf Trønnes noralf@tronnes.org wrote:

...

Den 21.12.2017 14.05, skrev Daniel Vetter:

...

On Thu, Dec 21, 2017 at 11:52:43AM +0100, Noralf Trønnes wrote:

...

Den 11.12.2017 18.56, skrev Noralf Trønnes:

...

Den 11.12.2017 18.45, skrev Noralf Trønnes:

...

Den 11.12.2017 15.58, skrev Meghana Madhyastha: > On Mon, Dec 11, 2017 at 03:12:06PM +0100, Noralf Trønnes wrote: >> Den 11.12.2017 14.17, skrev Meghana Madhyastha: >>> On Sat, Dec 09, 2017 at 03:09:28PM +0100, Noralf Trønnes wrote: >>>> Den 21.10.2017 13.55, skrev Meghana Madhyastha: >>>>> Changes in v14: >>>>> - s/backlight_get/of_find_backlight/ in patch 2/3 >>>>> - Change commit message in patch 3/3 from requiring to acquiring >>>>> >>>>> Meghana Madhyastha (3): >>>>> drm/tinydrm: Move helper functions from >>>>> tinydrm-helpers to backlight.h >>>>> drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c >>>>> drm/tinydrm: Add devres versions of of_find_backlight >>>> I tried the patchset and this is what I got: >>>> >>>> [ 8.057792] Unable to handle kernel paging >>>> request at virtual address >>>> fffffe6b

<snip> >>>>> [ 9.144181] ---[ end trace 149c05934b6a6dcc ]--- >>>> Is the reason possibly because we have omitted error checking on the >>>> return value of backlight_enable function ? >>>> tinydrm_enable_backlight and >>>> tinydrm_disable_baclight do this. >>>> Eg. >>>> ret = backlight_update_status(backlight); >>>> if (ret) >>>> DRM_ERROR("Failed to enable backlight %d\n", ret); >>>> >>>> I'm not sure, just asking whether this could be a possible reason >>>> for the above trace. >>> The crash happens during probe. >>> I guess you'll figure this out when you get some hw to test on. >> I have set up the raspberry pi and have built and boot into the >> custom kernel >> but I am waiting for the panel to arrive. Meanwhile, any thoughts on >> error message ? Sorry for the trivial question, but I did not quite >> understand the crash message and how to replicate it. > of_find_backlight() can return an error pointer (-EPROBE_DEFER): > > diff --git a/drivers/video/backlight/backlight.c > b/drivers/video/backlight/backlight.c > index 4bb7bf3ee443..57370c5d51f0 100644 > --- a/drivers/video/backlight/backlight.c > +++ b/drivers/video/backlight/backlight.c > @@ -635,8 +635,8 @@ struct backlight_device > *devm_of_find_backlight(struct device *dev) > int ret; > > bd = of_find_backlight(dev); > - if (!bd) > - return NULL; > + if (IS_ERR_OR_NULL(bd)) > + return bd; > > ret = devm_add_action(dev, devm_backlight_put, bd); > if (ret) { > > That solved the crash, but the backlight didn't turn on. > I had to do this as well: > > diff --git a/include/linux/backlight.h b/include/linux/backlight.h > index 5c441d4c049c..6f9925f10a7c 100644 > --- a/include/linux/backlight.h > +++ b/include/linux/backlight.h > @@ -139,6 +139,8 @@ static inline int backlight_enable(struct > backlight_device *bd) > if (!bd) > return 0; > > + if (!bd->props.brightness) > + bd->props.brightness = bd->props.max_brightness; No, this is wrong, it should happen on probe, not every time it's enabled. So maybe it should be done in of_find_backlight() or something. I see that I'm currently doing it in tinydrm_of_find_backlight().

I'm not happy with this brightness hack of mine really.

Since I last looked at this I see that pwm_bl has gained the ability to start in a power off state (pwm_backlight_initial_power_state()). However the gpio_backlight driver doesn't do this. gpio_backlight has a 'default-on' property, but the problem is that it sets brightness, not power state. So the absense of the property sets brightness to zero, which makes the driver turn off backlight on probe. This seems to be fine, but then systemd-backlight comes along and restores brightness to 1, since that's what it was on shutdown. This turns on backlight and now I have a glaring white uninitialized panel waiting for the display driver to set it up.

This patch would solve my problem:

diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c index e470da95d806..54bb722e1ea3 100644 --- a/drivers/video/backlight/gpio_backlight.c +++ b/drivers/video/backlight/gpio_backlight.c @@ -142,7 +142,8 @@ static int gpio_backlight_probe(struct platform_device *pdev) return PTR_ERR(bl); }

•   bl->props.brightness = gbl->def_value;
  

•   bl->props.brightness = 1;
  

•   bl->props.state = gbl->def_value ? 0 : BL_CORE_FBBLANK;
      backlight_update_status(bl);
  
      platform_set_drvdata(pdev, bl);
  
  

The problem is that this will most likely break 2 in-kernel users of gpio-backlight which doesn't set the 'default-on' property: arch/arm/boot/dts/omap4-var-dvk-om44.dts arm/boot/dts/imx27-eukrea-mbimxsd27-baseboard.dts

AFAICT they rely on systemd-backlight to turn on backlight by setting brightness to 1.

So maybe my hack is _the_ soulution after all, but I'm no expert on the backlight subsystem and it's corner cases.

Can we fix the dts instead?

Isn't Device Tree ABI, ie. new kernels should work backwards with existing dtb's? We will break that contract if we change gpio_backlight like I proposed.

It's only a regression if someone reports a bug :-)

...

Another solution is to add a new DT property: 'default-off':

Optional properties:

• default-on: enable the backlight at boot.
• default-off: disable the backlight at boot by setting backlight in power state off with brightness set to one.

Not sure how well that will fly with Rob, it smells like a hack.

tbh if we indeed go with "we can't touch this, it's baked in", which feels a bit silly, then default-off is the only reasonable thing to do really. At least for gpio-backlight, since atm the current implementation boils down to default-on == nothing. Or we just say the kernel doesn't follow the spec and fix it. I guess this is up to dt maintainers to decide.

I have tried the pwm backlight driver (pwm_bl) and it behaves as it should and systemd-backlight does it's job of preserving brightness across reboots without lighting up the display.

So it would be nice to have gpio_backlight adhere to the same rules.

Noralf.

On Thu, Dec 21, 2017 at 11:52:43AM +0100, Noralf Trønnes wrote:

Den 11.12.2017 18.56, skrev Noralf Trønnes:

...

Den 11.12.2017 18.45, skrev Noralf Trønnes:

...

Den 11.12.2017 15.58, skrev Meghana Madhyastha:

...

On Mon, Dec 11, 2017 at 03:12:06PM +0100, Noralf Trønnes wrote:

...

Den 11.12.2017 14.17, skrev Meghana Madhyastha:

...

On Sat, Dec 09, 2017 at 03:09:28PM +0100, Noralf Trønnes wrote: >Den 21.10.2017 13.55, skrev Meghana Madhyastha: >>Changes in v14: >>- s/backlight_get/of_find_backlight/ in patch 2/3 >>- Change commit message in patch 3/3 from requiring to acquiring >> >>Meghana Madhyastha (3): >>   drm/tinydrm: Move helper functions from tinydrm-helpers to >>backlight.h >>   drm/tinydrm: Move tinydrm_of_find_backlight to backlight.c >>   drm/tinydrm: Add devres versions of of_find_backlight >I tried the patchset and this is what I got: > >[    8.057792] Unable to handle kernel paging request at virtual >address >fffffe6b

<snip> >>>>>[    9.144181] ---[ end trace 149c05934b6a6dcc ]--- >>>>Is the reason possibly because we have omitted error checking on the >>>>return value of backlight_enable function ? >>>>tinydrm_enable_backlight and >>>>tinydrm_disable_baclight do this. >>>>Eg. >>>>ret = backlight_update_status(backlight); >>>>if (ret) >>>>    DRM_ERROR("Failed to enable backlight %d\n", ret); >>>> >>>>I'm not sure, just asking whether this could be a possible reason >>>>for the above trace. >>>The crash happens during probe. >>>I guess you'll figure this out when you get some hw to test on. >>I have set up the raspberry pi and have built and boot into the custom >>kernel >>but I am waiting for the panel to arrive. Meanwhile, any thoughts on >>error message ? Sorry for the trivial question, but I did not quite >>understand the crash message and how to replicate it. > >of_find_backlight() can return an error pointer (-EPROBE_DEFER): > >diff --git a/drivers/video/backlight/backlight.c >b/drivers/video/backlight/backlight.c >index 4bb7bf3ee443..57370c5d51f0 100644 >--- a/drivers/video/backlight/backlight.c >+++ b/drivers/video/backlight/backlight.c >@@ -635,8 +635,8 @@ struct backlight_device >*devm_of_find_backlight(struct device *dev) >        int ret; > >        bd = of_find_backlight(dev); >-       if (!bd) >-               return NULL; >+       if (IS_ERR_OR_NULL(bd)) >+               return bd; > >        ret = devm_add_action(dev, devm_backlight_put, bd); >        if (ret) { > >That solved the crash, but the backlight didn't turn on. >I had to do this as well: > >diff --git a/include/linux/backlight.h b/include/linux/backlight.h >index 5c441d4c049c..6f9925f10a7c 100644 >--- a/include/linux/backlight.h >+++ b/include/linux/backlight.h >@@ -139,6 +139,8 @@ static inline int backlight_enable(struct >backlight_device *bd) >        if (!bd) >                return 0; > >+       if (!bd->props.brightness) >+               bd->props.brightness = bd->props.max_brightness;

No, this is wrong, it should happen on probe, not every time it's enabled. So maybe it should be done in of_find_backlight() or something. I see that I'm currently doing it in tinydrm_of_find_backlight().

I'm not happy with this brightness hack of mine really.

Since I last looked at this I see that pwm_bl has gained the ability to start in a power off state (pwm_backlight_initial_power_state()). However the gpio_backlight driver doesn't do this. gpio_backlight has a 'default-on' property, but the problem is that it sets brightness, not power state. So the absense of the property sets brightness to zero, which makes the driver turn off backlight on probe. This seems to be fine, but then systemd-backlight comes along and restores brightness to 1, since that's what it was on shutdown. This turns on backlight and now I have a glaring white uninitialized panel waiting for the display driver to set it up.

This patch would solve my problem:

diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c index e470da95d806..54bb722e1ea3 100644 --- a/drivers/video/backlight/gpio_backlight.c +++ b/drivers/video/backlight/gpio_backlight.c @@ -142,7 +142,8 @@ static int gpio_backlight_probe(struct platform_device *pdev)                 return PTR_ERR(bl);         }

-       bl->props.brightness = gbl->def_value; +       bl->props.brightness = 1; +       bl->props.state = gbl->def_value ? 0 : BL_CORE_FBBLANK;         backlight_update_status(bl);

platform_set_drvdata(pdev, bl);

I have a few questions here.

So if I understood the problem correctly, you do not want the backlight brightness set to 1 by systemd-backlight because it sets the backlight on before the display driver is set up. My question is, how does pwm_bl avoid this ? Even if it starts off in a power off state, won't the same thing happen i.e won't systemd set the brightness and subsequently turn on the backlight ?

Also, I didn't understand your patch. What does gbl->def_value specify? bl->props.state represents the power state right ? Is your patch setting it to 0 prevent systemd-backlight from kicking in because we are dealing with the power property and not the brightness property ?

I was trying to understand some of the functions in gpio_backlight.c and was confused about one more thing. What is the difference between gpio_backlight_probe and gpio_backlight_probe_dt ?

I am also a little confused about how gpio backlight and pwm backlight are separate/different (because both would require the gpio pins). Is there any documentation here which I could refer to?

Thanks and regards, Meghana

The problem is that this will most likely break 2 in-kernel users of gpio-backlight which doesn't set the 'default-on' property:   arch/arm/boot/dts/omap4-var-dvk-om44.dts   arm/boot/dts/imx27-eukrea-mbimxsd27-baseboard.dts

AFAICT they rely on systemd-backlight to turn on backlight by setting brightness to 1.

So maybe my hack is _the_ soulution after all, but I'm no expert on the backlight subsystem and it's corner cases.

Noralf.

...

...

bd->props.power = FB_BLANK_UNBLANK;         bd->props.state &= ~BL_CORE_FBBLANK;

This is my backlight node[1]:

backlight: backlight {     compatible = "gpio-backlight";     gpios = <&gpio 18 0>; // GPIO_ACTIVE_HIGH };

Not certain that this is the "correct" solution, but I can't use the gpio-backlight property default-on, because that would turn on backlight before the pipeline is enabled.

You can dry-run the driver without a panel connected, it can work without being able to read from the controller.

Noralf.

[1] https://github.com/notro/tinydrm/blob/master/rpi-overlays/rpi-display-overla...

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel