[Freedreno] [DPU PATCH 2/2] drm/panel: add backlight control support for truly panel
abhinavk at codeaurora.org
abhinavk at codeaurora.org
Fri Apr 13 20:59:29 UTC 2018
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 at 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
>>
More information about the Freedreno
mailing list