| Message ID | 15b81a58eddd5d3fbfa418293cf1c817ef46423a.1548236066.git-series.maxime.ripard@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/sun4i: dsi: Add burst mode support | expand |
Hi Maxime / Konstantin. Nice welstructured and small driver. Please see a few comments below Some of the comments in the following apply to a lot of the existing panel drivers as well. But lets see if we can get new drivers to be even better. Sam On Wed, Jan 23, 2019 at 04:54:24PM +0100, Maxime Ripard wrote: > From: Konstantin Sudakov <k.sudakov@integrasources.com> > > The Rondo RB070D30 panel is a MIPI-DSI panel based on a Fitipower EK79007 > controller and a 1024x600 panel. > > Signed-off-by: Konstantin Sudakov <k.sudakov@integrasources.com> > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > --- > drivers/gpu/drm/panel/Kconfig | 9 +- > drivers/gpu/drm/panel/Makefile | 1 +- > drivers/gpu/drm/panel/panel-rondo-rb070d30.c | 258 ++++++++++++++++++++- > 3 files changed, 268 insertions(+) > create mode 100644 drivers/gpu/drm/panel/panel-rondo-rb070d30.c > > diff --git a/drivers/gpu/drm/panel/panel-rondo-rb070d30.c b/drivers/gpu/drm/panel/panel-rondo-rb070d30.c > new file mode 100644 > index 000000000000..4f8e63f367b1 > --- /dev/null > +++ b/drivers/gpu/drm/panel/panel-rondo-rb070d30.c > @@ -0,0 +1,258 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (C) 2018, Bridge Systems BV > + * Copyright (C) 2018, Bootlin > + * Copyright (C) 2017, Free Electrons > + * > + * This file based on panel-ilitek-ili9881c.c > + */ > + > +#include <linux/backlight.h> > +#include <linux/delay.h> > +#include <linux/device.h> > +#include <linux/err.h> > +#include <linux/errno.h> > +#include <linux/fb.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > + > +#include <linux/gpio/consumer.h> > +#include <linux/regulator/consumer.h> > + > +#include <drm/drm_mipi_dsi.h> > +#include <drm/drmP.h> > +#include <drm/drm_panel.h> Please do not use drmP.h in new drivers. We are trying to get rid of it. > + > +#include <video/mipi_display.h> > +#include <video/of_display_timing.h> > +#include <video/videomode.h> > + > +struct rb070d30_panel { > + struct drm_panel panel; > + struct mipi_dsi_device *dsi; > + struct backlight_device *backlight; > + struct regulator *supply; > + > + struct { > + struct gpio_desc *power; > + struct gpio_desc *reset; > + struct gpio_desc *updn; > + struct gpio_desc *shlr; > + } gpios; > +}; > + > +static inline struct rb070d30_panel *panel_to_rb070d30_panel(struct drm_panel *panel) > +{ > + return container_of(panel, struct rb070d30_panel, panel); > +} > + > +static int rb070d30_panel_prepare(struct drm_panel *panel) > +{ > + struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel); > + int ret; > + > + ret = regulator_enable(ctx->supply); > + if (ret < 0) { > + dev_err(&ctx->dsi->dev, "Failed to enable supply: %d\n", ret); Use DRM_DEV_ERROR(...) to have consistent error message for the drm drivers. This is general for the whole file. > + return ret; > + } > + > + /* Reset */ > + msleep(20); > + gpiod_set_value(ctx->gpios.power, 1); > + msleep(20); > + gpiod_set_value(ctx->gpios.reset, 1); > + msleep(20); > + return 0; > +} To verify the above pointer to a datasheet would be nice. > + > +static int rb070d30_panel_unprepare(struct drm_panel *panel) > +{ > + struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel); > + > + gpiod_set_value(ctx->gpios.power, 0); > + gpiod_set_value(ctx->gpios.reset, 0); > + regulator_disable(ctx->supply); > + > + return 0; > +} There is sometimes timing constrains after deassert reset.. The order is not strictly reverse of prepare - is that on purpose? > + > +static int rb070d30_panel_enable(struct drm_panel *panel) > +{ > + struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel); > + int ret; > + > + ret = mipi_dsi_dcs_exit_sleep_mode(ctx->dsi); > + if (ret) > + return ret; > + > + ret = backlight_enable(ctx->backlight); > + if (ret) > + goto out; > + > + return 0; > + > +out: > + mipi_dsi_dcs_enter_sleep_mode(ctx->dsi); > + return ret; > +} > + > +static int rb070d30_panel_disable(struct drm_panel *panel) > +{ > + struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel); > + > + backlight_disable(ctx->backlight); > + return mipi_dsi_dcs_enter_sleep_mode(ctx->dsi); > +} > + > +/* Default timings */ > +static const struct drm_display_mode default_mode = { > + .clock = 51206, > + .hdisplay = 1024, > + .hsync_start = 1024 + 160, > + .hsync_end = 1024 + 160 + 80, > + .htotal = 1024 + 160 + 80 + 80, > + .vdisplay = 600, > + .vsync_start = 600 + 12, > + .vsync_end = 600 + 12 + 10, > + .vtotal = 600 + 12 + 10 + 13, > + .vrefresh = 60, > +}; height and width missing here. Seems better to add them here than hiding in code below. Is it on purpose that no flags are specified? > + > +static int rb070d30_panel_get_modes(struct drm_panel *panel) > +{ > + struct drm_connector *connector = panel->connector; > + struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel); > + struct drm_display_mode *mode; > + static const u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24; > + > + mode = drm_mode_duplicate(panel->drm, &default_mode); > + if (!mode) { > + dev_err(&ctx->dsi->dev, "failed to add mode %ux%ux@%u\n", > + default_mode.hdisplay, default_mode.vdisplay, > + default_mode.vrefresh); Use" DRM_DEV_ERROR(&ctx->dsi->dev, "failed to add mode" DRM_MODE_FMT, DRM_MODE_ARG(mode)); > + return -EINVAL; > + } > + > + drm_mode_set_name(mode); > + > + mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED; > + drm_mode_probed_add(connector, mode); > + > + panel->connector->display_info.bpc = 8; > + panel->connector->display_info.width_mm = 154; > + panel->connector->display_info.height_mm = 85; See comment on height above. Same goes for bpc > + drm_display_info_set_bus_formats(&connector->display_info, > + &bus_format, 1); > + > + return 1; > +} > + > +static const struct drm_panel_funcs rb070d30_panel_funcs = { > + .get_modes = rb070d30_panel_get_modes, > + .prepare = rb070d30_panel_prepare, > + .enable = rb070d30_panel_enable, > + .disable = rb070d30_panel_disable, > + .unprepare = rb070d30_panel_unprepare, > +}; > + > +static int rb070d30_panel_dsi_probe(struct mipi_dsi_device *dsi) > +{ > + struct device_node *np; > + struct rb070d30_panel *ctx; > + int ret; > + > + ctx = devm_kzalloc(&dsi->dev, sizeof(*ctx), GFP_KERNEL); > + if (!ctx) > + return -ENOMEM; > + > + ctx->supply = devm_regulator_get(&dsi->dev, "vcc-lcd"); > + if (IS_ERR(ctx->supply)) > + return PTR_ERR(ctx->supply); > + > + mipi_dsi_set_drvdata(dsi, ctx); > + ctx->dsi = dsi; > + > + drm_panel_init(&ctx->panel); > + ctx->panel.dev = &dsi->dev; > + ctx->panel.funcs = &rb070d30_panel_funcs; > + > + ctx->gpios.reset = devm_gpiod_get(&dsi->dev, "reset", GPIOD_OUT_LOW); > + if (IS_ERR(ctx->gpios.reset)) { > + dev_err(&dsi->dev, "Couldn't get our reset GPIO\n"); > + return PTR_ERR(ctx->gpios.reset); > + } > + > + ctx->gpios.power = devm_gpiod_get(&dsi->dev, "power", GPIOD_OUT_LOW); > + if (IS_ERR(ctx->gpios.power)) { > + dev_err(&dsi->dev, "Couldn't get our power GPIO\n"); > + return PTR_ERR(ctx->gpios.power); > + } > + > + ctx->gpios.updn = devm_gpiod_get(&dsi->dev, "updn", GPIOD_OUT_LOW); > + if (IS_ERR(ctx->gpios.updn)) { > + dev_err(&dsi->dev, "Couldn't get our updn GPIO\n"); > + return PTR_ERR(ctx->gpios.updn); > + } This gpio is never used, it is only read from DT > + > + ctx->gpios.shlr = devm_gpiod_get(&dsi->dev, "shlr", GPIOD_OUT_LOW); > + if (IS_ERR(ctx->gpios.shlr)) { > + dev_err(&dsi->dev, "Couldn't get our shlr GPIO\n"); > + return PTR_ERR(ctx->gpios.shlr); > + } This gpio is never used, it is only read from DT > + > + np = of_parse_phandle(dsi->dev.of_node, "backlight", 0); > + if (np) { > + ctx->backlight = of_find_backlight_by_node(np); > + of_node_put(np); > + > + if (!ctx->backlight) > + return -EPROBE_DEFER; > + } Use devm_of_find_backlight() > + > + ret = drm_panel_add(&ctx->panel); > + if (ret < 0) > + goto free_backlight; > + > + dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST | MIPI_DSI_MODE_LPM; > + dsi->format = MIPI_DSI_FMT_RGB888; > + dsi->lanes = 4; > + > + return mipi_dsi_attach(dsi); > + > +free_backlight: > + backlight_put(ctx->backlight); If devm_of_find_backlight() is used this can go. > + return ret; > +} > + > +static int rb070d30_panel_dsi_remove(struct mipi_dsi_device *dsi) > +{ > + struct rb070d30_panel *ctx = mipi_dsi_get_drvdata(dsi); > + > + mipi_dsi_detach(dsi); > + drm_panel_remove(&ctx->panel); > + backlight_put(ctx->backlight); If devm_of_find_backlight() is used this can go. > + > + return 0; > +} > + > +static const struct of_device_id rb070d30_panel_of_match[] = { > + { .compatible = "rondo,rb070d30" }, > + { /* sentinel */ }, > +}; > +MODULE_DEVICE_TABLE(of, rb070d30_panel_of_match); > + > +static struct mipi_dsi_driver rb070d30_panel_driver = { > + .probe = rb070d30_panel_dsi_probe, > + .remove = rb070d30_panel_dsi_remove, > + .driver = { > + .name = "panel-rondo-rb070d30", > + .of_match_table = rb070d30_panel_of_match, > + }, > +}; > +module_mipi_dsi_driver(rb070d30_panel_driver); > + > +MODULE_AUTHOR("Boris Brezillon <boris.brezillon@bootlin.com>"); > +MODULE_AUTHOR("Konstantin Sudakov <k.sudakov@integrasources.com>"); > +MODULE_DESCRIPTION("Rondo RB070D30 Panel Driver"); > +MODULE_LICENSE("GPL"); > -- > git-series 0.9.1 > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hello, Sam! Thank you for the comments. >> +#include <drm/drmP.h> > Please do not use drmP.h in new drivers. We are trying to get rid of it. It can be replaced by these headers: #include <drm/drm_connector.h> #include <drm/drm_modes.h> #include <uapi/linux/media-bus-format.h> > Use DRM_DEV_ERROR(...) to have consistent error message for the drm drivers. > This is general for the whole file. Good idea, I think. So, this header is needed: #include <drm/drm_print.h> >> + /* Reset */ >> + msleep(20); >> + gpiod_set_value(ctx->gpios.power, 1); >> + msleep(20); >> + gpiod_set_value(ctx->gpios.reset, 1); >> + msleep(20); >To verify the above pointer to a datasheet would be nice. I looked the datasheet again, looks like the reset should be first. But I didn't find information about timings. It's needed to be checked on hardware. The power pin is named "STBYB" originally. > height and width missing here. Seems better to add them here than hiding in code below. > Is it on purpose that no flags are specified? I sure, this algorithm was copied from the ilitek driver. The default_mode (drm_display_mode) variable and panel->connector->display_info (drm_display_info) variable has a different types, I'm not sure about this point. The bpc is not not applicable for the drm_display_mode. >> + mode = drm_mode_duplicate(panel->drm, &default_mode); >> + if (!mode) { >> + dev_err(&ctx->dsi->dev, "failed to add mode %ux%ux@%u\n", >> + default_mode.hdisplay, default_mode.vdisplay, >> + default_mode.vrefresh); > Use" > DRM_DEV_ERROR(&ctx->dsi->dev, > "failed to add mode" DRM_MODE_FMT, > DRM_MODE_ARG(mode)); Not sure because the mode is NULL. May the default_mode be used for print? >> + ctx->gpios.updn = devm_gpiod_get(&dsi->dev, "updn", GPIOD_OUT_LOW); >> + if (IS_ERR(ctx->gpios.updn)) { >> + dev_err(&dsi->dev, "Couldn't get our updn GPIO\n"); >> + return PTR_ERR(ctx->gpios.updn); >> + } > This gpio is never used, it is only read from DT The gpio is initialized with low state. The state may be inverted by DT. The same for the "shlr". It is a vertical / horizontal inversion. > Use devm_of_find_backlight() I agree with you. The lastly, I think the rondo should be replaced by ronbo, because the company name is Ronbo Electronics Ltd. http://www.ronboe.com/about.html >Суббота, 26 января 2019, 22:39 +07:00 от Sam Ravnborg <sam@ravnborg.org>: > >Hi Maxime / Konstantin. > >Nice welstructured and small driver. >Please see a few comments below > >Some of the comments in the following apply to a lot of >the existing panel drivers as well. >But lets see if we can get new drivers to be even better. > >Sam > >On Wed, Jan 23, 2019 at 04:54:24PM +0100, Maxime Ripard wrote: >> From: Konstantin Sudakov < k.sudakov@integrasources.com > >> >> The Rondo RB070D30 panel is a MIPI-DSI panel based on a Fitipower EK79007 >> controller and a 1024x600 panel. >> >> Signed-off-by: Konstantin Sudakov < k.sudakov@integrasources.com > >> Signed-off-by: Maxime Ripard < maxime.ripard@bootlin.com > >> --- >> drivers/gpu/drm/panel/Kconfig | 9 +- >> drivers/gpu/drm/panel/Makefile | 1 +- >> drivers/gpu/drm/panel/panel-rondo-rb070d30.c | 258 ++++++++++++++++++++- >> 3 files changed, 268 insertions(+) >> create mode 100644 drivers/gpu/drm/panel/panel-rondo-rb070d30.c >> >> diff --git a/drivers/gpu/drm/panel/panel-rondo-rb070d30.c b/drivers/gpu/drm/panel/panel-rondo-rb070d30.c >> new file mode 100644 >> index 000000000000..4f8e63f367b1 >> --- /dev/null >> +++ b/drivers/gpu/drm/panel/panel-rondo-rb070d30.c >> @@ -0,0 +1,258 @@ >> +// SPDX-License-Identifier: GPL-2.0+ >> +/* >> + * Copyright (C) 2018, Bridge Systems BV >> + * Copyright (C) 2018, Bootlin >> + * Copyright (C) 2017, Free Electrons >> + * >> + * This file based on panel-ilitek-ili9881c.c >> + */ >> + >> +#include <linux/backlight.h> >> +#include <linux/delay.h> >> +#include <linux/device.h> >> +#include <linux/err.h> >> +#include <linux/errno.h> >> +#include <linux/fb.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> + >> +#include <linux/gpio/consumer.h> >> +#include <linux/regulator/consumer.h> >> + >> +#include <drm/drm_mipi_dsi.h> >> +#include <drm/drmP.h> >> +#include <drm/drm_panel.h> >Please do not use drmP.h in new drivers. We are trying to get rid of it. > >> + >> +#include <video/mipi_display.h> >> +#include <video/of_display_timing.h> >> +#include <video/videomode.h> >> + >> +struct rb070d30_panel { >> + struct drm_panel panel; >> + struct mipi_dsi_device *dsi; >> + struct backlight_device *backlight; >> + struct regulator *supply; >> + >> + struct { >> + struct gpio_desc *power; >> + struct gpio_desc *reset; >> + struct gpio_desc *updn; >> + struct gpio_desc *shlr; >> + } gpios; >> +}; >> + >> +static inline struct rb070d30_panel *panel_to_rb070d30_panel(struct drm_panel *panel) >> +{ >> + return container_of(panel, struct rb070d30_panel, panel); >> +} >> + >> +static int rb070d30_panel_prepare(struct drm_panel *panel) >> +{ >> + struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel); >> + int ret; >> + >> + ret = regulator_enable(ctx->supply); >> + if (ret < 0) { >> + dev_err(&ctx->dsi->dev, "Failed to enable supply: %d\n", ret); >Use DRM_DEV_ERROR(...) to have consistent error message for the drm drivers. >This is general for the whole file. > >> + return ret; >> + } >> + >> + /* Reset */ >> + msleep(20); >> + gpiod_set_value(ctx->gpios.power, 1); >> + msleep(20); >> + gpiod_set_value(ctx->gpios.reset, 1); >> + msleep(20); >> + return 0; >> +} >To verify the above pointer to a datasheet would be nice. > > >> + >> +static int rb070d30_panel_unprepare(struct drm_panel *panel) >> +{ >> + struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel); >> + >> + gpiod_set_value(ctx->gpios.power, 0); >> + gpiod_set_value(ctx->gpios.reset, 0); >> + regulator_disable(ctx->supply); >> + >> + return 0; >> +} >There is sometimes timing constrains after deassert reset.. >The order is not strictly reverse of prepare - is that on purpose? > > >> + >> +static int rb070d30_panel_enable(struct drm_panel *panel) >> +{ >> + struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel); >> + int ret; >> + >> + ret = mipi_dsi_dcs_exit_sleep_mode(ctx->dsi); >> + if (ret) >> + return ret; >> + >> + ret = backlight_enable(ctx->backlight); >> + if (ret) >> + goto out; >> + >> + return 0; >> + >> +out: >> + mipi_dsi_dcs_enter_sleep_mode(ctx->dsi); >> + return ret; >> +} >> + >> +static int rb070d30_panel_disable(struct drm_panel *panel) >> +{ >> + struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel); >> + >> + backlight_disable(ctx->backlight); >> + return mipi_dsi_dcs_enter_sleep_mode(ctx->dsi); >> +} >> + >> +/* Default timings */ >> +static const struct drm_display_mode default_mode = { >> + .clock = 51206, >> + .hdisplay = 1024, >> + .hsync_start = 1024 + 160, >> + .hsync_end = 1024 + 160 + 80, >> + .htotal = 1024 + 160 + 80 + 80, >> + .vdisplay = 600, >> + .vsync_start = 600 + 12, >> + .vsync_end = 600 + 12 + 10, >> + .vtotal = 600 + 12 + 10 + 13, >> + .vrefresh = 60, >> +}; >height and width missing here. Seems better to add them here than hiding in code below. >Is it on purpose that no flags are specified? > >> + >> +static int rb070d30_panel_get_modes(struct drm_panel *panel) >> +{ >> + struct drm_connector *connector = panel->connector; >> + struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel); >> + struct drm_display_mode *mode; >> + static const u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24; >> + >> + mode = drm_mode_duplicate(panel->drm, &default_mode); >> + if (!mode) { >> + dev_err(&ctx->dsi->dev, "failed to add mode %ux%ux@%u\n", >> + default_mode.hdisplay, default_mode.vdisplay, >> + default_mode.vrefresh); >Use" >DRM_DEV_ERROR(&ctx->dsi->dev, >"failed to add mode" DRM_MODE_FMT, >DRM_MODE_ARG(mode)); >> + return -EINVAL; >> + } >> + >> + drm_mode_set_name(mode); >> + >> + mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED; >> + drm_mode_probed_add(connector, mode); >> + >> + panel->connector->display_info.bpc = 8; >> + panel->connector->display_info.width_mm = 154; >> + panel->connector->display_info.height_mm = 85; >See comment on height above. >Same goes for bpc > >> + drm_display_info_set_bus_formats(&connector->display_info, >> + &bus_format, 1); >> + >> + return 1; >> +} >> + >> +static const struct drm_panel_funcs rb070d30_panel_funcs = { >> + .get_modes = rb070d30_panel_get_modes, >> + .prepare = rb070d30_panel_prepare, >> + .enable = rb070d30_panel_enable, >> + .disable = rb070d30_panel_disable, >> + .unprepare = rb070d30_panel_unprepare, >> +}; >> + >> +static int rb070d30_panel_dsi_probe(struct mipi_dsi_device *dsi) >> +{ >> + struct device_node *np; >> + struct rb070d30_panel *ctx; >> + int ret; >> + >> + ctx = devm_kzalloc(&dsi->dev, sizeof(*ctx), GFP_KERNEL); >> + if (!ctx) >> + return -ENOMEM; >> + >> + ctx->supply = devm_regulator_get(&dsi->dev, "vcc-lcd"); >> + if (IS_ERR(ctx->supply)) >> + return PTR_ERR(ctx->supply); >> + >> + mipi_dsi_set_drvdata(dsi, ctx); >> + ctx->dsi = dsi; >> + >> + drm_panel_init(&ctx->panel); >> + ctx->panel.dev = &dsi->dev; >> + ctx->panel.funcs = &rb070d30_panel_funcs; >> + >> + ctx->gpios.reset = devm_gpiod_get(&dsi->dev, "reset", GPIOD_OUT_LOW); >> + if (IS_ERR(ctx->gpios.reset)) { >> + dev_err(&dsi->dev, "Couldn't get our reset GPIO\n"); >> + return PTR_ERR(ctx->gpios.reset); >> + } >> + >> + ctx->gpios.power = devm_gpiod_get(&dsi->dev, "power", GPIOD_OUT_LOW); >> + if (IS_ERR(ctx->gpios.power)) { >> + dev_err(&dsi->dev, "Couldn't get our power GPIO\n"); >> + return PTR_ERR(ctx->gpios.power); >> + } >> + >> + ctx->gpios.updn = devm_gpiod_get(&dsi->dev, "updn", GPIOD_OUT_LOW); >> + if (IS_ERR(ctx->gpios.updn)) { >> + dev_err(&dsi->dev, "Couldn't get our updn GPIO\n"); >> + return PTR_ERR(ctx->gpios.updn); >> + } >This gpio is never used, it is only read from DT > > >> + >> + ctx->gpios.shlr = devm_gpiod_get(&dsi->dev, "shlr", GPIOD_OUT_LOW); >> + if (IS_ERR(ctx->gpios.shlr)) { >> + dev_err(&dsi->dev, "Couldn't get our shlr GPIO\n"); >> + return PTR_ERR(ctx->gpios.shlr); >> + } >This gpio is never used, it is only read from DT > >> + >> + np = of_parse_phandle(dsi->dev.of_node, "backlight", 0); >> + if (np) { >> + ctx->backlight = of_find_backlight_by_node(np); >> + of_node_put(np); >> + >> + if (!ctx->backlight) >> + return -EPROBE_DEFER; >> + } >Use devm_of_find_backlight() > >> + >> + ret = drm_panel_add(&ctx->panel); >> + if (ret < 0) >> + goto free_backlight; >> + >> + dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST | MIPI_DSI_MODE_LPM; >> + dsi->format = MIPI_DSI_FMT_RGB888; >> + dsi->lanes = 4; >> + >> + return mipi_dsi_attach(dsi); >> + >> +free_backlight: >> + backlight_put(ctx->backlight); >If devm_of_find_backlight() is used this can go. > >> + return ret; >> +} >> + >> +static int rb070d30_panel_dsi_remove(struct mipi_dsi_device *dsi) >> +{ >> + struct rb070d30_panel *ctx = mipi_dsi_get_drvdata(dsi); >> + >> + mipi_dsi_detach(dsi); >> + drm_panel_remove(&ctx->panel); >> + backlight_put(ctx->backlight); >If devm_of_find_backlight() is used this can go. > >> + >> + return 0; >> +} >> + >> +static const struct of_device_id rb070d30_panel_of_match[] = { >> + { .compatible = "rondo,rb070d30" }, >> + { /* sentinel */ }, >> +}; >> +MODULE_DEVICE_TABLE(of, rb070d30_panel_of_match); >> + >> +static struct mipi_dsi_driver rb070d30_panel_driver = { >> + .probe = rb070d30_panel_dsi_probe, >> + .remove = rb070d30_panel_dsi_remove, >> + .driver = { >> + .name = "panel-rondo-rb070d30", >> + .of_match_table = rb070d30_panel_of_match, >> + }, >> +}; >> +module_mipi_dsi_driver(rb070d30_panel_driver); >> + >> +MODULE_AUTHOR("Boris Brezillon < boris.brezillon@bootlin.com >"); >> +MODULE_AUTHOR("Konstantin Sudakov < k.sudakov@integrasources.com >"); >> +MODULE_DESCRIPTION("Rondo RB070D30 Panel Driver"); >> +MODULE_LICENSE("GPL"); >> -- >> git-series 0.9.1 >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Konstantin > >> + ctx->gpios.updn = devm_gpiod_get(&dsi->dev, "updn", GPIOD_OUT_LOW); > >> + if (IS_ERR(ctx->gpios.updn)) { > >> + dev_err(&dsi->dev, "Couldn't get our updn GPIO\n"); > >> + return PTR_ERR(ctx->gpios.updn); > >> + } > > This gpio is never used, it is only read from DT > The gpio is initialized with low state. The state may be inverted by DT. The same for the "shlr". > It is a vertical / horizontal inversion. Ohh, then it makes sense again. Consider adding a comment so this becomes more obvious Sam
Hi Sam, Thanks for the review, I'll address the points left out. On Sat, Jan 26, 2019 at 04:39:46PM +0100, Sam Ravnborg wrote: > > + return ret; > > + } > > + > > + /* Reset */ > > + msleep(20); > > + gpiod_set_value(ctx->gpios.power, 1); > > + msleep(20); > > + gpiod_set_value(ctx->gpios.reset, 1); > > + msleep(20); > > + return 0; > > +} > To verify the above pointer to a datasheet would be nice. Unfortunately, it's not publicly available :/ > > + > > +static int rb070d30_panel_unprepare(struct drm_panel *panel) > > +{ > > + struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel); > > + > > + gpiod_set_value(ctx->gpios.power, 0); > > + gpiod_set_value(ctx->gpios.reset, 0); > > + regulator_disable(ctx->supply); > > + > > + return 0; > > +} > > There is sometimes timing constrains after deassert reset.. > The order is not strictly reverse of prepare - is that on purpose? You're right about the order. I couldn't find any delay after a reset though in the datasheet. > > +/* Default timings */ > > +static const struct drm_display_mode default_mode = { > > + .clock = 51206, > > + .hdisplay = 1024, > > + .hsync_start = 1024 + 160, > > + .hsync_end = 1024 + 160 + 80, > > + .htotal = 1024 + 160 + 80 + 80, > > + .vdisplay = 600, > > + .vsync_start = 600 + 12, > > + .vsync_end = 600 + 12 + 10, > > + .vtotal = 600 + 12 + 10 + 13, > > + .vrefresh = 60, > > +}; > height and width missing here. Seems better to add them here than hiding in code below. > Is it on purpose that no flags are specified? > > > + > > +static int rb070d30_panel_get_modes(struct drm_panel *panel) > > +{ > > + struct drm_connector *connector = panel->connector; > > + struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel); > > + struct drm_display_mode *mode; > > + static const u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24; > > + > > + mode = drm_mode_duplicate(panel->drm, &default_mode); > > + if (!mode) { > > + dev_err(&ctx->dsi->dev, "failed to add mode %ux%ux@%u\n", > > + default_mode.hdisplay, default_mode.vdisplay, > > + default_mode.vrefresh); > Use" > DRM_DEV_ERROR(&ctx->dsi->dev, > "failed to add mode" DRM_MODE_FMT, > DRM_MODE_ARG(mode)); > > + return -EINVAL; > > + } > > + > > + drm_mode_set_name(mode); > > + > > + mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED; > > + drm_mode_probed_add(connector, mode); > > + > > + panel->connector->display_info.bpc = 8; > > + panel->connector->display_info.width_mm = 154; > > + panel->connector->display_info.height_mm = 85; > See comment on height above. > Same goes for bpc Sorry, I'm not sure to follow you here. bpc and height are both set? Thanks! Maxime
Hi Maxime. > > > + } > > > + > > > + drm_mode_set_name(mode); > > > + > > > + mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED; > > > + drm_mode_probed_add(connector, mode); > > > + > > > + panel->connector->display_info.bpc = 8; > > > + panel->connector->display_info.width_mm = 154; > > > + panel->connector->display_info.height_mm = 85; > > See comment on height above. > > Same goes for bpc > > Sorry, I'm not sure to follow you here. bpc and height are both set? I assumed that if we had specified height and width in display_mode then we did not have to do it here. But I may be wrong. And for bpc I was plain wrong. Sam
Hi Sam, On Tue, Jan 29, 2019 at 04:48:33PM +0100, Sam Ravnborg wrote: > > > > + } > > > > + > > > > + drm_mode_set_name(mode); > > > > + > > > > + mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED; > > > > + drm_mode_probed_add(connector, mode); > > > > + > > > > + panel->connector->display_info.bpc = 8; > > > > + panel->connector->display_info.width_mm = 154; > > > > + panel->connector->display_info.height_mm = 85; > > > See comment on height above. > > > Same goes for bpc > > > > Sorry, I'm not sure to follow you here. bpc and height are both set? > > I assumed that if we had specified height and width in display_mode > then we did not have to do it here. But I may be wrong. It looks like it's not done by the core, but panel-simple simply copies it, so I'll do it as well. Thanks for the suggestion! Maxime
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig index 3f3537719beb..3164fa824a63 100644 --- a/drivers/gpu/drm/panel/Kconfig +++ b/drivers/gpu/drm/panel/Kconfig @@ -138,6 +138,15 @@ config DRM_PANEL_RAYDIUM_RM68200 Say Y here if you want to enable support for Raydium RM68200 720x1280 DSI video mode panel. +config DRM_PANEL_RONDO_RB070D30 + tristate "Rondo Electronics RB070D30 panel" + depends on OF + depends on DRM_MIPI_DSI + depends on BACKLIGHT_CLASS_DEVICE + help + Say Y here if you want to enable support for Rondo Electronics + RB070D30 1024x600 DSI panel. + config DRM_PANEL_SAMSUNG_S6D16D0 tristate "Samsung S6D16D0 DSI video mode panel" depends on OF diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile index 4396658a7996..4fe4cf1bfdb5 100644 --- a/drivers/gpu/drm/panel/Makefile +++ b/drivers/gpu/drm/panel/Makefile @@ -12,6 +12,7 @@ obj-$(CONFIG_DRM_PANEL_ORISETECH_OTM8009A) += panel-orisetech-otm8009a.o obj-$(CONFIG_DRM_PANEL_PANASONIC_VVX10F034N00) += panel-panasonic-vvx10f034n00.o obj-$(CONFIG_DRM_PANEL_RASPBERRYPI_TOUCHSCREEN) += panel-raspberrypi-touchscreen.o obj-$(CONFIG_DRM_PANEL_RAYDIUM_RM68200) += panel-raydium-rm68200.o +obj-$(CONFIG_DRM_PANEL_RONDO_RB070D30) += panel-rondo-rb070d30.o obj-$(CONFIG_DRM_PANEL_SAMSUNG_LD9040) += panel-samsung-ld9040.o obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6D16D0) += panel-samsung-s6d16d0.o obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E3HA2) += panel-samsung-s6e3ha2.o diff --git a/drivers/gpu/drm/panel/panel-rondo-rb070d30.c b/drivers/gpu/drm/panel/panel-rondo-rb070d30.c new file mode 100644 index 000000000000..4f8e63f367b1 --- /dev/null +++ b/drivers/gpu/drm/panel/panel-rondo-rb070d30.c @@ -0,0 +1,258 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (C) 2018, Bridge Systems BV + * Copyright (C) 2018, Bootlin + * Copyright (C) 2017, Free Electrons + * + * This file based on panel-ilitek-ili9881c.c + */ + +#include <linux/backlight.h> +#include <linux/delay.h> +#include <linux/device.h> +#include <linux/err.h> +#include <linux/errno.h> +#include <linux/fb.h> +#include <linux/kernel.h> +#include <linux/module.h> + +#include <linux/gpio/consumer.h> +#include <linux/regulator/consumer.h> + +#include <drm/drm_mipi_dsi.h> +#include <drm/drmP.h> +#include <drm/drm_panel.h> + +#include <video/mipi_display.h> +#include <video/of_display_timing.h> +#include <video/videomode.h> + +struct rb070d30_panel { + struct drm_panel panel; + struct mipi_dsi_device *dsi; + struct backlight_device *backlight; + struct regulator *supply; + + struct { + struct gpio_desc *power; + struct gpio_desc *reset; + struct gpio_desc *updn; + struct gpio_desc *shlr; + } gpios; +}; + +static inline struct rb070d30_panel *panel_to_rb070d30_panel(struct drm_panel *panel) +{ + return container_of(panel, struct rb070d30_panel, panel); +} + +static int rb070d30_panel_prepare(struct drm_panel *panel) +{ + struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel); + int ret; + + ret = regulator_enable(ctx->supply); + if (ret < 0) { + dev_err(&ctx->dsi->dev, "Failed to enable supply: %d\n", ret); + return ret; + } + + /* Reset */ + msleep(20); + gpiod_set_value(ctx->gpios.power, 1); + msleep(20); + gpiod_set_value(ctx->gpios.reset, 1); + msleep(20); + return 0; +} + +static int rb070d30_panel_unprepare(struct drm_panel *panel) +{ + struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel); + + gpiod_set_value(ctx->gpios.power, 0); + gpiod_set_value(ctx->gpios.reset, 0); + regulator_disable(ctx->supply); + + return 0; +} + +static int rb070d30_panel_enable(struct drm_panel *panel) +{ + struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel); + int ret; + + ret = mipi_dsi_dcs_exit_sleep_mode(ctx->dsi); + if (ret) + return ret; + + ret = backlight_enable(ctx->backlight); + if (ret) + goto out; + + return 0; + +out: + mipi_dsi_dcs_enter_sleep_mode(ctx->dsi); + return ret; +} + +static int rb070d30_panel_disable(struct drm_panel *panel) +{ + struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel); + + backlight_disable(ctx->backlight); + return mipi_dsi_dcs_enter_sleep_mode(ctx->dsi); +} + +/* Default timings */ +static const struct drm_display_mode default_mode = { + .clock = 51206, + .hdisplay = 1024, + .hsync_start = 1024 + 160, + .hsync_end = 1024 + 160 + 80, + .htotal = 1024 + 160 + 80 + 80, + .vdisplay = 600, + .vsync_start = 600 + 12, + .vsync_end = 600 + 12 + 10, + .vtotal = 600 + 12 + 10 + 13, + .vrefresh = 60, +}; + +static int rb070d30_panel_get_modes(struct drm_panel *panel) +{ + struct drm_connector *connector = panel->connector; + struct rb070d30_panel *ctx = panel_to_rb070d30_panel(panel); + struct drm_display_mode *mode; + static const u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24; + + mode = drm_mode_duplicate(panel->drm, &default_mode); + if (!mode) { + dev_err(&ctx->dsi->dev, "failed to add mode %ux%ux@%u\n", + default_mode.hdisplay, default_mode.vdisplay, + default_mode.vrefresh); + return -EINVAL; + } + + drm_mode_set_name(mode); + + mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED; + drm_mode_probed_add(connector, mode); + + panel->connector->display_info.bpc = 8; + panel->connector->display_info.width_mm = 154; + panel->connector->display_info.height_mm = 85; + drm_display_info_set_bus_formats(&connector->display_info, + &bus_format, 1); + + return 1; +} + +static const struct drm_panel_funcs rb070d30_panel_funcs = { + .get_modes = rb070d30_panel_get_modes, + .prepare = rb070d30_panel_prepare, + .enable = rb070d30_panel_enable, + .disable = rb070d30_panel_disable, + .unprepare = rb070d30_panel_unprepare, +}; + +static int rb070d30_panel_dsi_probe(struct mipi_dsi_device *dsi) +{ + struct device_node *np; + struct rb070d30_panel *ctx; + int ret; + + ctx = devm_kzalloc(&dsi->dev, sizeof(*ctx), GFP_KERNEL); + if (!ctx) + return -ENOMEM; + + ctx->supply = devm_regulator_get(&dsi->dev, "vcc-lcd"); + if (IS_ERR(ctx->supply)) + return PTR_ERR(ctx->supply); + + mipi_dsi_set_drvdata(dsi, ctx); + ctx->dsi = dsi; + + drm_panel_init(&ctx->panel); + ctx->panel.dev = &dsi->dev; + ctx->panel.funcs = &rb070d30_panel_funcs; + + ctx->gpios.reset = devm_gpiod_get(&dsi->dev, "reset", GPIOD_OUT_LOW); + if (IS_ERR(ctx->gpios.reset)) { + dev_err(&dsi->dev, "Couldn't get our reset GPIO\n"); + return PTR_ERR(ctx->gpios.reset); + } + + ctx->gpios.power = devm_gpiod_get(&dsi->dev, "power", GPIOD_OUT_LOW); + if (IS_ERR(ctx->gpios.power)) { + dev_err(&dsi->dev, "Couldn't get our power GPIO\n"); + return PTR_ERR(ctx->gpios.power); + } + + ctx->gpios.updn = devm_gpiod_get(&dsi->dev, "updn", GPIOD_OUT_LOW); + if (IS_ERR(ctx->gpios.updn)) { + dev_err(&dsi->dev, "Couldn't get our updn GPIO\n"); + return PTR_ERR(ctx->gpios.updn); + } + + ctx->gpios.shlr = devm_gpiod_get(&dsi->dev, "shlr", GPIOD_OUT_LOW); + if (IS_ERR(ctx->gpios.shlr)) { + dev_err(&dsi->dev, "Couldn't get our shlr GPIO\n"); + return PTR_ERR(ctx->gpios.shlr); + } + + np = of_parse_phandle(dsi->dev.of_node, "backlight", 0); + if (np) { + ctx->backlight = of_find_backlight_by_node(np); + of_node_put(np); + + if (!ctx->backlight) + return -EPROBE_DEFER; + } + + ret = drm_panel_add(&ctx->panel); + if (ret < 0) + goto free_backlight; + + dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST | MIPI_DSI_MODE_LPM; + dsi->format = MIPI_DSI_FMT_RGB888; + dsi->lanes = 4; + + return mipi_dsi_attach(dsi); + +free_backlight: + backlight_put(ctx->backlight); + return ret; +} + +static int rb070d30_panel_dsi_remove(struct mipi_dsi_device *dsi) +{ + struct rb070d30_panel *ctx = mipi_dsi_get_drvdata(dsi); + + mipi_dsi_detach(dsi); + drm_panel_remove(&ctx->panel); + backlight_put(ctx->backlight); + + return 0; +} + +static const struct of_device_id rb070d30_panel_of_match[] = { + { .compatible = "rondo,rb070d30" }, + { /* sentinel */ }, +}; +MODULE_DEVICE_TABLE(of, rb070d30_panel_of_match); + +static struct mipi_dsi_driver rb070d30_panel_driver = { + .probe = rb070d30_panel_dsi_probe, + .remove = rb070d30_panel_dsi_remove, + .driver = { + .name = "panel-rondo-rb070d30", + .of_match_table = rb070d30_panel_of_match, + }, +}; +module_mipi_dsi_driver(rb070d30_panel_driver); + +MODULE_AUTHOR("Boris Brezillon <boris.brezillon@bootlin.com>"); +MODULE_AUTHOR("Konstantin Sudakov <k.sudakov@integrasources.com>"); +MODULE_DESCRIPTION("Rondo RB070D30 Panel Driver"); +MODULE_LICENSE("GPL");