Hi Peter. Good with more panel drivers. Some comments in the following, please do not blindly follow them but check that this is OK. 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. > +#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. > + > + const struct drm_display_mode *default_mode; > + const struct drm_display_mode *mode; Assigned but never used. Drop the assignment and the variable? > +}; > + > +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); > + > + 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. > + > + 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. > + > + 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. > + > + 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. > +} > + > +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() > + > + 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? > + osd101t2587_panel_disable(&osd101t2587->base); > +} > + > +static struct mipi_dsi_driver osd101t2587_panel_driver = { > + .driver = { > + .name = "panel-osd-osd101t2587-53ts", > + .of_match_table = osd101t2587_of_match, > + }, > + .probe = osd101t2587_panel_probe, > + .remove = osd101t2587_panel_remove, > + .shutdown = osd101t2587_panel_shutdown, > +}; > +module_mipi_dsi_driver(osd101t2587_panel_driver); > + > +MODULE_AUTHOR("Peter Ujfalusi <peter.ujfalusi@xxxxxx>"); > +MODULE_DESCRIPTION("OSD101T2587-53TS DSI panel"); > +MODULE_LICENSE("GPL v2"); > -- > Peter > > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki > > _______________________________________________ > dri-devel mailing list > dri-devel@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/dri-devel
