Hi Sam, On 15/02/2019 20.07, Sam Ravnborg wrote: > Hi Peter. > > Good with more panel drivers. > Some comments in the following, please do not blindly follow them but > check that this is OK. First of all, thank you for the review and sorry for the delay! > > Sam > > On Fri, Feb 15, 2019 at 04:03:15PM +0200, Peter Ujfalusi via dri-devel wrote: >> The panel is similar to OSD101T2045-53TS (which is handled by panel-simple) >> with one big difference: osd101t2587-53ts needs MIPI_DSI_TURN_ON_PERIPHERAL >> message to be sent from the host to be operational and thus can not be >> handled by panel-simple. >> >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@xxxxxx> >> --- >> +++ b/drivers/gpu/drm/panel/panel-osd-osd101t2587-53ts.c >> @@ -0,0 +1,284 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com >> + * Author: Peter Ujfalusi <peter.ujfalusi@xxxxxx> >> + */ >> + >> +#include <linux/backlight.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/regulator/consumer.h> >> + >> +#include <drm/drmP.h> > Please do not use drmP.h in new drivers - we try to get rid of this file. OK. > >> +#include <drm/drm_crtc.h> >> +#include <drm/drm_mipi_dsi.h> >> +#include <drm/drm_panel.h> >> + >> +#include <video/mipi_display.h> >> + >> +struct osd101t2587_panel { >> + struct drm_panel base; >> + struct mipi_dsi_device *dsi; >> + >> + struct backlight_device *backlight; >> + struct regulator *supply; >> + >> + bool prepared; >> + bool enabled; >> + >> + ktime_t earliest_wake; > earliest_wake is not used, and can be deleted. Yes, it is leftover. >> + >> + const struct drm_display_mode *default_mode; >> + const struct drm_display_mode *mode; > Assigned but never used. Drop the assignment and the variable? Hrm, you are right. > >> +}; >> + >> +static inline struct osd101t2587_panel *to_osd101t2587_panel(struct drm_panel *panel) >> +{ >> + return container_of(panel, struct osd101t2587_panel, base); >> +} >> + >> +static int osd101t2587_panel_disable(struct drm_panel *panel) >> +{ >> + struct osd101t2587_panel *osd101t2587 = to_osd101t2587_panel(panel); >> + int ret; >> + >> + if (!osd101t2587->enabled) >> + return 0; >> + >> + if (osd101t2587->backlight) { >> + osd101t2587->backlight->props.power = FB_BLANK_POWERDOWN; >> + osd101t2587->backlight->props.state |= BL_CORE_FBBLANK; >> + backlight_update_status(osd101t2587->backlight); >> + } > backlight_disable(osd101t2587->backlight); OK >> + >> + ret = mipi_dsi_shutdown_peripheral(osd101t2587->dsi); >> + >> + osd101t2587->enabled = false; >> + >> + return ret; >> +} >> + >> +static int osd101t2587_panel_unprepare(struct drm_panel *panel) >> +{ >> + struct osd101t2587_panel *osd101t2587 = to_osd101t2587_panel(panel); >> + >> + if (!osd101t2587->prepared) >> + return 0; >> + >> + regulator_disable(osd101t2587->supply); >> + osd101t2587->prepared = false; >> + >> + return 0; >> +} >> + >> +static int osd101t2587_panel_prepare(struct drm_panel *panel) >> +{ >> + struct osd101t2587_panel *osd101t2587 = to_osd101t2587_panel(panel); >> + int ret; >> + >> + if (osd101t2587->prepared) >> + return 0; >> + >> + ret = regulator_enable(osd101t2587->supply); >> + if (!ret) >> + osd101t2587->prepared = true; >> + >> + return ret; >> +} >> + >> +static int osd101t2587_panel_enable(struct drm_panel *panel) >> +{ >> + struct osd101t2587_panel *osd101t2587 = to_osd101t2587_panel(panel); >> + int ret; >> + >> + if (osd101t2587->enabled) >> + return 0; >> + >> + ret = mipi_dsi_turn_on_peripheral(osd101t2587->dsi); >> + if (ret) >> + return ret; >> + >> + if (osd101t2587->backlight) { >> + osd101t2587->backlight->props.power = FB_BLANK_UNBLANK; >> + osd101t2587->backlight->props.state &= ~BL_CORE_FBBLANK; >> + backlight_update_status(osd101t2587->backlight); >> + } > backlight_enable(osd101t2587->backlight); > can be used here. Yes, it can be used. > >> + >> + osd101t2587->enabled = true; >> + >> + return ret; >> +} >> + >> +static const struct drm_display_mode default_mode_osd101t2587 = { >> + .clock = 164400, >> + .hdisplay = 1920, >> + .hsync_start = 1920 + 152, >> + .hsync_end = 1920 + 152 + 52, >> + .htotal = 1920 + 152 + 52 + 20, >> + .vdisplay = 1200, >> + .vsync_start = 1200 + 24, >> + .vsync_end = 1200 + 24 + 6, >> + .vtotal = 1200 + 24 + 6 + 48, >> + .vrefresh = 60, >> + .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC, >> +}; >> + >> +static int osd101t2587_panel_get_modes(struct drm_panel *panel) >> +{ >> + struct osd101t2587_panel *osd101t2587 = to_osd101t2587_panel(panel); >> + struct drm_display_mode *mode; >> + >> + mode = drm_mode_duplicate(panel->drm, osd101t2587->default_mode); >> + if (!mode) { >> + dev_err(panel->drm->dev, "failed to add mode %ux%ux@%u\n", >> + osd101t2587->default_mode->hdisplay, >> + osd101t2587->default_mode->vdisplay, >> + osd101t2587->default_mode->vrefresh); >> + return -ENOMEM; >> + } >> + >> + drm_mode_set_name(mode); >> + >> + drm_mode_probed_add(panel->connector, mode); >> + >> + panel->connector->display_info.width_mm = 217; >> + panel->connector->display_info.height_mm = 136; >> + >> + return 1; >> +} >> + >> +static const struct drm_panel_funcs osd101t2587_panel_funcs = { >> + .disable = osd101t2587_panel_disable, >> + .unprepare = osd101t2587_panel_unprepare, >> + .prepare = osd101t2587_panel_prepare, >> + .enable = osd101t2587_panel_enable, >> + .get_modes = osd101t2587_panel_get_modes, >> +}; >> + >> +static const struct of_device_id osd101t2587_of_match[] = { >> + { >> + .compatible = "osd,osd101t2587-53ts", >> + .data = &default_mode_osd101t2587, >> + }, { } > Move { } to a separate line and use { /* sentinel */ } to make it look like other drivers. > > >> +}; >> +MODULE_DEVICE_TABLE(of, osd101t2587_of_match); >> + >> +static int osd101t2587_panel_add(struct osd101t2587_panel *osd101t2587) >> +{ >> + struct device *dev = &osd101t2587->dsi->dev; >> + struct device_node *np; >> + int ret; >> + >> + osd101t2587->mode = osd101t2587->default_mode; >> + >> + osd101t2587->supply = devm_regulator_get(dev, "power"); >> + if (IS_ERR(osd101t2587->supply)) >> + return PTR_ERR(osd101t2587->supply); >> + >> + np = of_parse_phandle(dev->of_node, "backlight", 0); >> + if (np) { >> + osd101t2587->backlight = of_find_backlight_by_node(np); >> + of_node_put(np); >> + >> + if (!osd101t2587->backlight) >> + return -EPROBE_DEFER; >> + } > Use devm_of_find_backlight() - this results in simpler code. Yes, it certainly makes the code cleaner. > >> + >> + drm_panel_init(&osd101t2587->base); >> + osd101t2587->base.funcs = &osd101t2587_panel_funcs; >> + osd101t2587->base.dev = &osd101t2587->dsi->dev; >> + >> + ret = drm_panel_add(&osd101t2587->base); >> + if (ret < 0) >> + goto put_backlight; >> + >> + return 0; >> + >> +put_backlight: >> + if (osd101t2587->backlight) >> + put_device(&osd101t2587->backlight->dev); > When using devm_of_find_backlight() this is not needed. OK. > >> + >> + return ret; >> +} >> + >> +static void osd101t2587_panel_del(struct osd101t2587_panel *osd101t2587) >> +{ >> + if (osd101t2587->base.dev) >> + drm_panel_remove(&osd101t2587->base); >> + >> + if (osd101t2587->backlight) >> + put_device(&osd101t2587->backlight->dev); > When you use devm_of_find_backlight() then this is done for you, so the above can be deleted. OK. >> +} >> + >> +static int osd101t2587_panel_probe(struct mipi_dsi_device *dsi) >> +{ >> + struct osd101t2587_panel *osd101t2587; >> + const struct of_device_id *id; >> + int ret; >> + >> + id = of_match_node(osd101t2587_of_match, dsi->dev.of_node); >> + if (!id) >> + return -ENODEV; >> + >> + dsi->lanes = 4; >> + dsi->format = MIPI_DSI_FMT_RGB888; >> + dsi->mode_flags = MIPI_DSI_MODE_VIDEO | >> + MIPI_DSI_MODE_VIDEO_BURST | >> + MIPI_DSI_MODE_VIDEO_SYNC_PULSE | >> + MIPI_DSI_MODE_EOT_PACKET; >> + >> + osd101t2587 = devm_kzalloc(&dsi->dev, sizeof(*osd101t2587), GFP_KERNEL); >> + if (!osd101t2587) >> + return -ENOMEM; >> + >> + mipi_dsi_set_drvdata(dsi, osd101t2587); >> + >> + osd101t2587->dsi = dsi; >> + osd101t2587->default_mode = id->data; >> + >> + ret = osd101t2587_panel_add(osd101t2587); >> + if (ret < 0) >> + return ret; >> + >> + return mipi_dsi_attach(dsi); >> +} >> + >> +static int osd101t2587_panel_remove(struct mipi_dsi_device *dsi) >> +{ >> + struct osd101t2587_panel *osd101t2587 = mipi_dsi_get_drvdata(dsi); >> + int ret; >> + >> + ret = osd101t2587_panel_disable(&osd101t2587->base); >> + if (ret < 0) >> + dev_err(&dsi->dev, "failed to disable panel: %d\n", ret); >> + > To trun off power supply call osd101t2587_panel_unprepare() here? > >> + ret = mipi_dsi_detach(dsi); >> + if (ret < 0) >> + dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", ret); >> + >> + osd101t2587_panel_del(osd101t2587); > Replace this with > drm_panel_remove(osd101t2587->base); > This should be fine and no need to disable backlight, this is done in osd101t2587_panel_disable() OK. >> + >> + return 0; >> +} >> + >> +static void osd101t2587_panel_shutdown(struct mipi_dsi_device *dsi) >> +{ >> + struct osd101t2587_panel *osd101t2587 = mipi_dsi_get_drvdata(dsi); >> + > Maybe call osd101t2587_panel_unprepare() here to turn off power supply? Make sense, in this order: osd101t2587_panel_disable(&osd101t2587->base); osd101t2587_panel_unprepare(&osd101t2587->base); But should the osd101t2587_panel_remove() do the same thing? or the osd101t2587_panel_disable() is redundant in the osd101t2587_panel_remove()? Regards, - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
