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

[abhinavk-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org]

Hi Sean

Thanks for the comments.

Some replies inline.

On 2018-04-13 13:46, Sean Paul wrote:
> 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.
[Abhinav] Ok, will take care of this in v2.
> 
>> 
>>  #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
> 
[Abhinav] Ok, will remove it in v2.

>> +	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).
[Abhinav] Yes, will remove the NULL check.
We are registering the backlight in the backlight_setup(), this was more 
of
an additional check, to make sure ctx->wled is present before we 
trigger.
Not sure if we should register again here.
> 
>> +		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.
[Abhinav] Will remove it.
> 
>> +
>> +	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.
> 
[Abhinav] The reason for keeping it "panel0-backlight" is because 
userspace is using
this node name to write the backlight. Changing the name will need more 
changes in our
userspace.

>> +
>> +		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?
[Abhinav] Thanks for catching this, yes not required.
Will move it back.
> 
>>  		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
>> 
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno