Re: [RFC PATCH 2/3] drm/panel: sharp-lq101r1sx01: Use panel-common helpers

[Eric Anholt <eric@anholt.net>]

[-- Attachment #1.1: Type: text/plain, Size: 5380 bytes --]

Sean Paul <seanpaul@chromium.org> writes:

> Instead of duplicating common code from panel-simple, use the panel-common helpers.
>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/panel/Kconfig                   |  1 +
>  drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c | 79 +++++++------------------
>  2 files changed, 24 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index be8590724042..98db78d22a13 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -73,6 +73,7 @@ config DRM_PANEL_SHARP_LQ101R1SX01
>  	depends on OF
>  	depends on DRM_MIPI_DSI
>  	depends on BACKLIGHT_CLASS_DEVICE
> +	select DRM_PANEL_COMMON
>  	help
>  	  Say Y here if you want to enable support for Sharp LQ101R1SX01
>  	  TFT-LCD modules. The panel has a 2560x1600 resolution and uses
> diff --git a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
> index 3cce3ca19601..fb2bf67449ab 100644
> --- a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
> +++ b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
> @@ -6,11 +6,8 @@
>   * published by the Free Software Foundation.
>   */
>  
> -#include <linux/backlight.h>
> -#include <linux/gpio/consumer.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> -#include <linux/regulator/consumer.h>
>  
>  #include <drm/drmP.h>
>  #include <drm/drm_crtc.h>
> @@ -19,17 +16,17 @@
>  
>  #include <video/mipi_display.h>
>  
> +#include "panel-common.h"
> +
>  struct sharp_panel {
>  	struct drm_panel base;
> +	struct panel_common common;
> +
>  	/* the datasheet refers to them as DSI-LINK1 and DSI-LINK2 */
>  	struct mipi_dsi_device *link1;
>  	struct mipi_dsi_device *link2;
>  
> -	struct backlight_device *backlight;
> -	struct regulator *supply;
> -
>  	bool prepared;
> -	bool enabled;
>  
>  	const struct drm_display_mode *mode;
>  };
> @@ -93,17 +90,7 @@ static int sharp_panel_disable(struct drm_panel *panel)
>  {
>  	struct sharp_panel *sharp = to_sharp_panel(panel);
>  
> -	if (!sharp->enabled)
> -		return 0;
> -
> -	if (sharp->backlight) {
> -		sharp->backlight->props.power = FB_BLANK_POWERDOWN;
> -		backlight_update_status(sharp->backlight);
> -	}
> -
> -	sharp->enabled = false;
> -
> -	return 0;
> +	return panel_common_disable(&sharp->common, 0);
>  }
>  
>  static int sharp_panel_unprepare(struct drm_panel *panel)
> @@ -126,11 +113,13 @@ static int sharp_panel_unprepare(struct drm_panel *panel)
>  
>  	msleep(120);
>  
> -	regulator_disable(sharp->supply);
> +	err = panel_common_unprepare(&sharp->common, 0);
> +	if (err < 0)
> +		dev_err(panel->dev, "failed common unprepare: %d\n", err);
>  
>  	sharp->prepared = false;
>  
> -	return 0;
> +	return err;
>  }
>  
>  static int sharp_setup_symmetrical_split(struct mipi_dsi_device *left,
> @@ -176,17 +165,15 @@ static int sharp_panel_prepare(struct drm_panel *panel)
>  	if (sharp->prepared)
>  		return 0;
>  
> -	err = regulator_enable(sharp->supply);
> -	if (err < 0)
> -		return err;
> -
>  	/*
>  	 * According to the datasheet, the panel needs around 10 ms to fully
>  	 * power up. At least another 120 ms is required before exiting sleep
>  	 * mode to make sure the panel is ready. Throw in another 20 ms for
>  	 * good measure.
>  	 */
> -	msleep(150);
> +	err = panel_common_prepare(&sharp->common, 150);
> +	if (err < 0)
> +		return err;

Aside: I like how 120 + 20 = 150, because always round up your delays :)

>  
>  	err = mipi_dsi_dcs_exit_sleep_mode(sharp->link1);
>  	if (err < 0) {
> @@ -252,7 +239,7 @@ static int sharp_panel_prepare(struct drm_panel *panel)
>  	return 0;
>  
>  poweroff:
> -	regulator_disable(sharp->supply);
> +	panel_common_unprepare(&sharp->common, 0);
>  	return err;
>  }
>  
> @@ -260,17 +247,7 @@ static int sharp_panel_enable(struct drm_panel *panel)
>  {
>  	struct sharp_panel *sharp = to_sharp_panel(panel);
>  
> -	if (sharp->enabled)
> -		return 0;
> -
> -	if (sharp->backlight) {
> -		sharp->backlight->props.power = FB_BLANK_UNBLANK;
> -		backlight_update_status(sharp->backlight);
> -	}
> -
> -	sharp->enabled = true;
> -
> -	return 0;
> +	return panel_common_enable(&sharp->common, 0);
>  }
>  
>  static const struct drm_display_mode default_mode = {
> @@ -324,23 +301,15 @@ MODULE_DEVICE_TABLE(of, sharp_of_match);
>  
>  static int sharp_panel_add(struct sharp_panel *sharp)
>  {
> -	struct device_node *np;
>  	int err;
>  
>  	sharp->mode = &default_mode;
> +	sharp->prepared = false;
>  
> -	sharp->supply = devm_regulator_get(&sharp->link1->dev, "power");
> -	if (IS_ERR(sharp->supply))
> -		return PTR_ERR(sharp->supply);
> -
> -	np = of_parse_phandle(sharp->link1->dev.of_node, "backlight", 0);
> -	if (np) {
> -		sharp->backlight = of_find_backlight_by_node(np);
> -		of_node_put(np);
> -
> -		if (!sharp->backlight)
> -			return -EPROBE_DEFER;
> -	}
> +	err = panel_common_init(&sharp->link1->dev, &sharp->common, "supply",
> +				"gpio", "backlight");
> +	if (err < 0)
> +		return err;

Looks like you've incidentally changed the regulator name from "power"
to "supply".  Seems like it should be a called out in the commit
message, if intentional.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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