Re: [PATCH 3/3] drm/panel: add lincoln lcd197 support

[Jerome Brunet <jbrunet@baylibre.com>]

On Wed 26 Jun 2024 at 07:41, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> 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@baylibre.com>
>> ---
>>  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 ? 

-- 
Jerome