On 26/06/2024 11:02, Jerome Brunet wrote:
On Wed 26 Jun 2024 at 07:41, Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> wrote:
On Tue, Jun 25, 2024 at 04:25:50PM GMT, Jerome Brunet wrote:
Add support for the Lincoln LCD197 1080x1920 DSI panel.
Signed-off-by: Jerome Brunet <jbrunet@xxxxxxxxxxxx>
---
drivers/gpu/drm/panel/Kconfig | 11 +
drivers/gpu/drm/panel/Makefile | 1 +
drivers/gpu/drm/panel/panel-lincoln-lcd197.c | 333 +++++++++++++++++++
3 files changed, 345 insertions(+)
create mode 100644 drivers/gpu/drm/panel/panel-lincoln-lcd197.c
[...]
+
+ mipi_dsi_dcs_write_seq(lcd->dsi, 0xB9, 0xFF, 0x83, 0x99);
- Please use lowercase hex instead
- Please consider switching to _multi() functions.
Could you be a bit more specific about these '_multi' function ?
I've looked at 'drm_mipi_dsi.h' and can't really make what you mean.
Maybe I'm not looking in the right place.
+ usleep_range(200, 300);
This will require new helper msm_dsi_usleep_range(ctx, 200, 300);
I don't really understand why I would need something else to just sleep
? Could you add some context please ?
Isn't 'msm_' usually something Qcom specific ?
+ mipi_dsi_dcs_write_seq(lcd->dsi, 0xB6, 0x92, 0x92);
+ mipi_dsi_dcs_write_seq(lcd->dsi, 0xCC, 0x00);
+ mipi_dsi_dcs_write_seq(lcd->dsi, 0xBF, 0x40, 0x41, 0x50, 0x49);
+ mipi_dsi_dcs_write_seq(lcd->dsi, 0xC6, 0xFF, 0xF9);
+ mipi_dsi_dcs_write_seq(lcd->dsi, 0xC0, 0x25, 0x5A);
+ mipi_dsi_dcs_write_seq(lcd->dsi, MIPI_DCS_SET_ADDRESS_MODE, 0x02);
+
+ err = mipi_dsi_dcs_exit_sleep_mode(lcd->dsi);
+ if (err < 0) {
+ dev_err(panel->dev, "failed to exit sleep mode: %d\n", err);
+ goto poweroff;
+ }
+ msleep(120);
+
+ err = mipi_dsi_dcs_read(lcd->dsi, MIPI_DCS_GET_DISPLAY_ID, display_id, 3);
This probably needs new _multi helper too.
+ if (err < 0) {
+ dev_err(panel->dev, "Failed to read display id: %d\n", err);
+ } else {
+ dev_dbg(panel->dev, "Display id: 0x%02x-0x%02x-0x%02x\n",
+ display_id[0], display_id[1], display_id[2]);
+ }
+
+ lcd->prepared = true;
Should not be required anymore.
The whole driver is heavily inspired by what is already in
drivers/gpu/drm/panel/ and a lot are doing something similar.
Maybe there has been a change since then and the existing have been
reworked yet. Would you mind pointing me that change if that is
the case ?
+
+ return 0;
+
+poweroff:
+ gpiod_set_value_cansleep(lcd->enable_gpio, 0);
+ gpiod_set_value_cansleep(lcd->reset_gpio, 1);
+ regulator_disable(lcd->supply);
+
+ return err;
+}
+
+
+static const struct drm_display_mode default_mode = {
+ .clock = 154002,
+ .hdisplay = 1080,
+ .hsync_start = 1080 + 20,
+ .hsync_end = 1080 + 20 + 6,
+ .htotal = 1080 + 204,
+ .vdisplay = 1920,
+ .vsync_start = 1920 + 4,
+ .vsync_end = 1920 + 4 + 4,
+ .vtotal = 1920 + 79,
+ .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
+};
+
+static int lincoln_lcd197_panel_get_modes(struct drm_panel *panel,
+ struct drm_connector *connector)
+{
+ struct drm_display_mode *mode;
+
+ mode = drm_mode_duplicate(connector->dev, &default_mode);
+ if (!mode) {
+ dev_err(panel->dev, "failed to add mode %ux%u@%u\n",
+ default_mode.hdisplay, default_mode.vdisplay,
+ drm_mode_vrefresh(&default_mode));
+ return -ENOMEM;
+ }
+
+ drm_mode_set_name(mode);
+ drm_mode_probed_add(connector, mode);
+ connector->display_info.width_mm = 79;
+ connector->display_info.height_mm = 125;
drm_connector_helper_get_modes_fixed()
Thanks for the hint
+
+ return 1;
+}
+
+
+static void lincoln_lcd197_panel_shutdown(struct mipi_dsi_device *dsi)
+{
+ struct lincoln_lcd197_panel *lcd = mipi_dsi_get_drvdata(dsi);
+
+ drm_panel_disable(&lcd->panel);
+ drm_panel_unprepare(&lcd->panel);
+}
I think the agreement was that there should be no need for the panel's
shutdown, the DRM driver should shutdown the panel.
I'm happy to drop that if there is such agreement. Again, most panel
drivers do implement that callback so I just did the same.
Could you point me to this 'agreement' please, so I can get a better
understanding of it ?
please rebase on linux-next or drm-misc-next and use the new introduced macros
dmitry pointed out.
Neil
