[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <bb73744f-9b04-410b-b4ec-1c5071016d28@linaro.org>
Date: Wed, 26 Jun 2024 11:12:22 +0200
From: Neil Armstrong <neil.armstrong@...aro.org>
To: Jerome Brunet <jbrunet@...libre.com>,
Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc: Jessica Zhang <quic_jesszhan@...cinc.com>, Sam Ravnborg
<sam@...nborg.org>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH 3/3] drm/panel: add lincoln lcd197 support
On 26/06/2024 11:02, Jerome Brunet wrote:
> On Wed 26 Jun 2024 at 07:41, Dmitry Baryshkov <dmitry.baryshkov@...aro.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@...libre.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 ?
>
please rebase on linux-next or drm-misc-next and use the new introduced macros
dmitry pointed out.
Neil
Powered by blists - more mailing lists