Re: [PATCH v2 4/4] drm/panel: Add OSD101T2587-53TS driver

[Peter Ujfalusi]

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