Hi Sam,
On 23/02/2019 21.38, Sam Ravnborg wrote:
> Hi Peter.
>
> Driver looks to be in good shape now.
> With the few comments below addressed you can add my:
> Reviewed-by: Sam Ravnborg <sam@xxxxxxxxxxxx>
>
> Sam
>
> On Fri, Feb 22, 2019 at 03:16:18PM +0200, Peter Ujfalusi 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>
>> ---
>> drivers/gpu/drm/panel/Kconfig | 6 +
>> drivers/gpu/drm/panel/Makefile | 1 +
>> .../drm/panel/panel-osd-osd101t2587-53ts.c | 254 ++++++++++++++++++
>> 3 files changed, 261 insertions(+)
>> create mode 100644 drivers/gpu/drm/panel/panel-osd-osd101t2587-53ts.c
>>
>> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
>> index 3e070153ef21..351661920838 100644
>> --- a/drivers/gpu/drm/panel/Kconfig
>> +++ b/drivers/gpu/drm/panel/Kconfig
>> @@ -122,6 +122,12 @@ config DRM_PANEL_ORISETECH_OTM8009A
>> Say Y here if you want to enable support for Orise Technology
>> otm8009a 480x800 dsi 2dl panel.
>>
>> +config DRM_PANEL_OSD_OSD101T2587_53TS
>> + tristate "OSD OSD101T2587-53TS DSI 1920x1200 video mode panel"
>> + depends on OF
>> + depends on DRM_MIPI_DSI
>> + depends on BACKLIGHT_CLASS_DEVICE
> Please add a help-text
Sure, I forgot this.
>> +
>> +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;
>
> Logic is wrong here. regulator_enable() will return a negative value on error
> and 0 in the good case.
> So osd101t2587->prepared is set to true only in the error case, not in the good case.
It is good as it is:
'if (!0)' == 'if (1)'
'if (!-X)' == 'if (0)'
>> +
>> + ret = mipi_dsi_attach(dsi);
>> + if (ret)
>> + drm_panel_remove(&osd101t2587->base);
>
> I do not see panel-simple.c do a drm_panel_remove() if mipi_dsi_attach() fails.
> Maybe the driver core will call remove() is probe fails?
> Or maybe panel-simple() should call drm_panel_remove()
>
> Keep the above as is - I just wanted to express that this looks different
> from the panle-simple() driver.
I have a patch for panel-simple as well with the following commit message:
"drm/panel: simple: Fix panel_simple_dsi_probe
In case mipi_dsi_attach() fails remove the registered panel to avoid
added panel without corresponding device."
It has the same bug.
>> +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_warn(&dsi->dev, "failed to disable panel\n");
> This is already warned in lower layers and I think you could
> drop the dev_warn().
I think there is no warning from lower layer, but not sure as I never
hit this case.
>> +
>> + osd101t2587_panel_unprepare(&osd101t2587->base);
>> +
>> + drm_panel_remove(&osd101t2587->base);
>> +
>> + ret = mipi_dsi_detach(dsi);
>> + if (ret < 0)
>> + dev_warn(&dsi->dev, "failed to detach from DSI host\n");
> Add error code in logging.
OK
- Péter
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
