| Message ID | 1523084813-858-2-git-send-email-abhinavk@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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 >
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 >> -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 13, 2018 at 01:59:29PM -0700, abhinavk@codeaurora.org wrote: > 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> > > > --- <snip /> > > > + 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. > Unless the userspace is opensource (I'm guessing it's not?), that's not something we need to code around. It sounds like this is mostly a moot point given Bjorn's comments on the v2. Sean <snip />
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> #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; + + 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) + 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; + } + + 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); + + 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); + 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; }
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(-)