Re: [DPU PATCH 2/2] drm/panel: add backlight control support for truly panel

[Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>]

On Sat, Apr 07, 2018 at 12:06:53AM -0700, Abhinav Kumar wrote:
> Register truly panel as a backlight led device and
> provide methods to control its backlight operation.
> 
> Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
> ---
>  drivers/gpu/drm/panel/panel-truly-dual-dsi.c | 96 +++++++++++++++++++++++++++-
>  1 file changed, 94 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-truly-dual-dsi.c b/drivers/gpu/drm/panel/panel-truly-dual-dsi.c
> index 47891ee..5d0ef90 100644
> --- a/drivers/gpu/drm/panel/panel-truly-dual-dsi.c
> +++ b/drivers/gpu/drm/panel/panel-truly-dual-dsi.c
> @@ -14,6 +14,7 @@
>  #include <linux/gpio/consumer.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/pinctrl/consumer.h>
> +#include <linux/leds.h>

Includes should be alphabetical.

>  
>  #include <video/mipi_display.h>
>  #include <video/of_videomode.h>
> @@ -23,6 +24,9 @@
>  #include <drm/drm_panel.h>
>  #include <drm/drm_mipi_dsi.h>
>  
> +#define BL_NODE_NAME_SIZE 32
> +#define PRIM_DISPLAY_NODE 0
> +
>  struct truly_wqxga {
>  	struct device *dev;
>  	struct drm_panel panel;
> @@ -33,6 +37,8 @@ struct truly_wqxga {
>  	struct gpio_desc *mode_gpio;
>  
>  	struct backlight_device *backlight;
> +	/* WLED params */
> +	struct led_trigger *wled;
>  	struct videomode vm;
>  
>  	struct mipi_dsi_device *dsi[2];
> @@ -447,6 +453,83 @@ static void truly_wqxga_panel_del(struct truly_wqxga *ctx)
>  		put_device(&ctx->dsi[1]->dev);
>  }
>  
> +static int truly_backlight_device_update_status(struct backlight_device *bd)
> +{
> +	int brightness;
> +	int max_brightness;
> +	int rc = 0;
> +
extra line

> +	struct truly_wqxga *ctx = dev_get_drvdata(&bd->dev);
> +
> +	brightness = bd->props.brightness;
> +	max_brightness = bd->props.max_brightness;
> +
> +	if ((bd->props.power != FB_BLANK_UNBLANK) ||
> +		(bd->props.state & BL_CORE_FBBLANK) ||
> +		  (bd->props.state & BL_CORE_SUSPENDED))
> +		brightness = 0;
> +
> +	if (brightness > max_brightness)
> +		brightness = max_brightness;
> +
> +	/* Need to check WLED driver capability upstream */
> +	if (ctx && ctx->wled)

ctx can't be NULL, so no need to check for that. And if ctx->wled is null, it
doesn't seem like this function will do anything. So how about just not
registering the backlight if wled == NULL (if that's possible).

> +		led_trigger_event(ctx->wled, brightness);
> +
> +	return rc;
> +}
> +
> +static int truly_backlight_device_get_brightness(struct backlight_device *bd)
> +{
> +	return bd->props.brightness;
> +}
> +
> +static const struct backlight_ops truly_backlight_device_ops = {
> +	.update_status = truly_backlight_device_update_status,
> +	.get_brightness = truly_backlight_device_get_brightness,
> +};
> +
> +static int truly_backlight_setup(struct truly_wqxga *ctx)
> +{
> +	struct backlight_properties props;
> +	char bl_node_name[BL_NODE_NAME_SIZE];
> +
> +	if (!ctx) {
> +		dev_err(ctx->dev, "invalid context\n");
> +		return -EINVAL;
> +	}

This can't happen.

> +
> +	if (!ctx->backlight) {
> +		memset(&props, 0, sizeof(props));
> +		props.type = BACKLIGHT_RAW;
> +		props.power = FB_BLANK_UNBLANK;
> +		props.max_brightness = 4096;
> +
> +		snprintf(bl_node_name, BL_NODE_NAME_SIZE, "panel%u-backlight",
> +				 PRIM_DISPLAY_NODE);

Given that PRIM_DISPLAY_NODE is always 0, this seems like overkill for a pretty
generic name "panel0-backlight". So let's just call it "truly_backlight" in the
register call.

> +
> +		ctx->backlight =  backlight_device_register(bl_node_name,
> +				ctx->dev, ctx,
> +				&truly_backlight_device_ops, &props);
> +
> +		if (IS_ERR_OR_NULL(ctx->backlight)) {
> +			pr_err("Failed to register backlight\n");
> +			ctx->backlight = NULL;
> +			return -ENODEV;
> +		}
> +
> +		/* Register with the LED driver interface */
> +		led_trigger_register_simple("bkl-trigger", &ctx->wled);
> +
> +		if (!ctx->wled) {
> +			pr_err("backlight led registration failed\n");
> +			return -ENODEV;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int truly_wqxga_probe(struct mipi_dsi_device *dsi)
>  {
>  	struct device *dev = &dsi->dev;
> @@ -466,10 +549,11 @@ static int truly_wqxga_probe(struct mipi_dsi_device *dsi)
>  		secondary = of_find_mipi_dsi_device_by_node(np);
>  		of_node_put(np);
>  
> +		ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> +

Why move this?

>  		if (!secondary)
>  			return -EPROBE_DEFER;
>  
> -		ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
>  		if (!ctx) {
>  			put_device(&secondary->dev);
>  			return -ENOMEM;
> @@ -485,6 +569,12 @@ static int truly_wqxga_probe(struct mipi_dsi_device *dsi)
>  			put_device(&secondary->dev);
>  			return ret;
>  		}
> +
> +		ret = truly_backlight_setup(ctx);
> +		if (ret) {
> +			put_device(&secondary->dev);
> +			return ret;
> +		}
>  	}
>  
>  	ret = mipi_dsi_attach(dsi);
> @@ -504,8 +594,10 @@ static int truly_wqxga_remove(struct mipi_dsi_device *dsi)
>  	mipi_dsi_detach(dsi);
>  
>  	/* delete panel only for the DSI1 interface */
> -	if (ctx)
> +	if (ctx) {
>  		truly_wqxga_panel_del(ctx);
> +		kfree(ctx);
> +	}
>  
>  	return 0;
>  }
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno