Re: [PATCH v1 16/16] drm/panel: simple: use drm_panel infrastructure

Hi Sam,

Thank you for the patch.

On Sun, Aug 04, 2019 at 10:16:37PM +0200, Sam Ravnborg wrote:

Use drm_panel infrastrucute:

• drm_panel has guards for calling disable/enable twice

As stated in the review of the corresponding patch, I think those checks should be dropped, but not moved to the panel core.

• drm_panel has backlight support

This answers my first question in the review of 15/16 :-)

To use the drm_panel infrastructure use the drm_panel_* variants for prepare/enable/disable/unprepare.

Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Thierry Reding thierry.reding@gmail.com Cc: Sam Ravnborg sam@ravnborg.org

The change looks good overall,

Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

but this is pending an agreement on what to do with the multiple prepare/enable guards.

---

drivers/gpu/drm/panel/panel-simple.c | 73 +++++----------------------- 1 file changed, 11 insertions(+), 62 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index bff7578f84dd..c7eed34f2c9c 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -21,7 +21,6 @@

• DEALINGS IN THE SOFTWARE.

*/

-#include <linux/backlight.h> #include <linux/delay.h> #include <linux/gpio/consumer.h> #include <linux/module.h> @@ -98,13 +97,10 @@ struct panel_desc {

struct panel_simple { struct drm_panel base;

• bool prepared;

• bool enabled; bool no_hpd;

  const struct panel_desc *desc;

• struct backlight_device *backlight; struct regulator *supply; struct i2c_adapter *ddc;

@@ -232,20 +228,9 @@ static int panel_simple_disable(struct drm_panel *panel) { struct panel_simple *p = to_panel_simple(panel);

• if (!p->enabled)

• return 0;
  

• if (p->backlight) {

• p->backlight->props.power = FB_BLANK_POWERDOWN;
  

• p->backlight->props.state |= BL_CORE_FBBLANK;
  

• backlight_update_status(p->backlight);
  

• }

• if (p->desc->delay.disable) msleep(p->desc->delay.disable);

• p->enabled = false;

• return 0;

}

@@ -253,9 +238,6 @@ static int panel_simple_unprepare(struct drm_panel *panel) { struct panel_simple *p = to_panel_simple(panel);

• if (!p->prepared)

• return 0;
  

• gpiod_set_value_cansleep(p->enable_gpio, 0);

  regulator_disable(p->supply);

@@ -263,8 +245,6 @@ static int panel_simple_unprepare(struct drm_panel *panel) if (p->desc->delay.unprepare) msleep(p->desc->delay.unprepare);

• p->prepared = false;
• return 0;

}

@@ -274,9 +254,6 @@ static int panel_simple_prepare(struct drm_panel *panel) unsigned int delay; int err;

• if (p->prepared)

• return 0;
  

• err = regulator_enable(p->supply); if (err < 0) { dev_err(panel->dev, "failed to enable supply: %d\n", err);

@@ -291,8 +268,6 @@ static int panel_simple_prepare(struct drm_panel *panel) if (delay) msleep(delay);

• p->prepared = true;
• return 0;

}

@@ -300,20 +275,9 @@ static int panel_simple_enable(struct drm_panel *panel) { struct panel_simple *p = to_panel_simple(panel);

• if (p->enabled)

• return 0;
  

• if (p->desc->delay.enable) msleep(p->desc->delay.enable);

• if (p->backlight) {

• p->backlight->props.state &= ~BL_CORE_FBBLANK;
  

• p->backlight->props.power = FB_BLANK_UNBLANK;
  

• backlight_update_status(p->backlight);
  

• }

• p->enabled = true;

• return 0;

}

@@ -413,7 +377,7 @@ static void panel_simple_parse_panel_timing_node(struct device *dev,

static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) {

• struct device_node *backlight, *ddc;

• struct device_node *ddc; struct panel_simple *panel; struct display_timing dt; int err;

@@ -422,8 +386,6 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) if (!panel) return -ENOMEM;

• panel->enabled = false;

• panel->prepared = false; panel->desc = desc;

  panel->no_hpd = of_property_read_bool(dev->of_node, "no-hpd");

@@ -441,24 +403,13 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) return err; }

• backlight = of_parse_phandle(dev->of_node, "backlight", 0);

• if (backlight) {

• panel->backlight = of_find_backlight_by_node(backlight);
  

• of_node_put(backlight);
  

• if (!panel->backlight)
  

• 	return -EPROBE_DEFER;
  

• }

• ddc = of_parse_phandle(dev->of_node, "ddc-i2c-bus", 0); if (ddc) { panel->ddc = of_find_i2c_adapter_by_node(ddc); of_node_put(ddc);

• if (!panel->ddc) {
  

• 	err = -EPROBE_DEFER;
  

• 	goto free_backlight;
  

• }
  

• if (!panel->ddc)
  

• 	return -EPROBE_DEFER;
  

  }

  if (!of_get_display_timing(dev->of_node, "panel-timing", &dt))

@@ -468,6 +419,10 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) panel->base.dev = dev; panel->base.funcs = &panel_simple_funcs;

• err = drm_panel_of_backlight(&panel->base);
• if (err)

• goto free_ddc;
  

• err = drm_panel_add(&panel->base); if (err < 0) goto free_ddc;

@@ -479,9 +434,6 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) free_ddc: if (panel->ddc) put_device(&panel->ddc->dev); -free_backlight:

• if (panel->backlight)

• put_device(&panel->backlight->dev);
  

This looks weird, where

return err; } @@ -492,15 +444,12 @@ static int panel_simple_remove(struct device *dev)

drm_panel_remove(&panel->base);

• panel_simple_disable(&panel->base);
• panel_simple_unprepare(&panel->base);

• drm_panel_disable(&panel->base);

• drm_panel_unprepare(&panel->base);

  if (panel->ddc) put_device(&panel->ddc->dev);

• if (panel->backlight)

• put_device(&panel->backlight->dev);
  

• return 0;

}

@@ -508,8 +457,8 @@ static void panel_simple_shutdown(struct device *dev) { struct panel_simple *panel = dev_get_drvdata(dev);

• panel_simple_disable(&panel->base);
• panel_simple_unprepare(&panel->base);

• drm_panel_disable(&panel->base);
• drm_panel_unprepare(&panel->base);

}

static const struct drm_display_mode ampire_am_480272h3tmqw_t01h_mode = {