[PATCH v3 28/56] drm/omap: dsi: untangle ulps ops from enable/disable

[Tomi Valkeinen]

On 09/11/2020 11:57, Laurent Pinchart wrote:
> Hi Tomi and Sebastian,
> 
> Thank you for the patch.
> 
> On Thu, Nov 05, 2020 at 02:03:05PM +0200, Tomi Valkeinen wrote:
>> From: Sebastian Reichel <sebastian.reichel at collabora.com>
>>
>> Create a custom function pointer for ULPS and use it instead of
>> reusing disable/enable functions for ULPS mode switch. This allows
>> us to use the common disable/enable functions pointers for DSI.
>>
>> Signed-off-by: Sebastian Reichel <sebastian.reichel at collabora.com>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ti.com>
>> ---
>>  .../gpu/drm/omapdrm/displays/panel-dsi-cm.c   |  8 ++--
>>  drivers/gpu/drm/omapdrm/dss/dsi.c             | 42 ++++++++++++++-----
>>  drivers/gpu/drm/omapdrm/dss/omapdss.h         |  5 +--
>>  3 files changed, 38 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
>> index 4be0c9dbcc43..78247dcb1848 100644
>> --- a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
>> +++ b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
>> @@ -233,7 +233,7 @@ static int dsicm_enter_ulps(struct panel_drv_data *ddata)
>>  	if (r)
>>  		goto err;
>>  
>> -	src->ops->dsi.disable(src, false, true);
>> +	src->ops->dsi.ulps(src, true);
>>  
>>  	ddata->ulps_enabled = true;
>>  
>> @@ -258,7 +258,7 @@ static int dsicm_exit_ulps(struct panel_drv_data *ddata)
>>  	if (!ddata->ulps_enabled)
>>  		return 0;
>>  
>> -	src->ops->enable(src);
>> +	src->ops->dsi.ulps(src, false);
>>  	ddata->dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
>>  
>>  	r = _dsicm_enable_te(ddata, ddata->te_enabled);
>> @@ -586,7 +586,7 @@ static int dsicm_power_on(struct panel_drv_data *ddata)
>>  
>>  	dsicm_hw_reset(ddata);
>>  
>> -	src->ops->dsi.disable(src, true, false);
>> +	src->ops->disable(src);
>>  err_regulators:
>>  	r = regulator_bulk_disable(DCS_REGULATOR_SUPPLY_NUM, ddata->supplies);
>>  	if (r)
>> @@ -612,7 +612,7 @@ static void dsicm_power_off(struct panel_drv_data *ddata)
>>  		dsicm_hw_reset(ddata);
>>  	}
>>  
>> -	src->ops->dsi.disable(src, true, false);
>> +	src->ops->disable(src);
>>  
>>  	r = regulator_bulk_disable(DCS_REGULATOR_SUPPLY_NUM, ddata->supplies);
>>  	if (r)
>> diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c
>> index d54b743c2b48..937362ade4b4 100644
>> --- a/drivers/gpu/drm/omapdrm/dss/dsi.c
>> +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
>> @@ -4055,13 +4055,10 @@ static void dsi_display_uninit_dsi(struct dsi_data *dsi, bool disconnect_lanes,
>>  	}
>>  }
>>  
>> -static void dsi_display_enable(struct omap_dss_device *dssdev)
>> +static void dsi_display_ulps_enable(struct dsi_data *dsi)
>>  {
>> -	struct dsi_data *dsi = to_dsi_data(dssdev);
>>  	int r;
>>  
>> -	DSSDBG("dsi_display_enable\n");
>> -
>>  	WARN_ON(!dsi_bus_is_locked(dsi));
>>  
>>  	mutex_lock(&dsi->lock);
>> @@ -4084,16 +4081,19 @@ static void dsi_display_enable(struct omap_dss_device *dssdev)
>>  	dsi_runtime_put(dsi);
>>  err_get_dsi:
>>  	mutex_unlock(&dsi->lock);
>> -	DSSDBG("dsi_display_enable FAILED\n");
>> +	DSSDBG("dsi_display_ulps_enable FAILED\n");
>>  }
>>  
>> -static void dsi_display_disable(struct omap_dss_device *dssdev,
>> -		bool disconnect_lanes, bool enter_ulps)
>> +static void dsi_display_enable(struct omap_dss_device *dssdev)
>>  {
>>  	struct dsi_data *dsi = to_dsi_data(dssdev);
>> +	DSSDBG("dsi_display_enable\n");
>> +	dsi_display_ulps_enable(dsi);
>> +}
>>  
>> -	DSSDBG("dsi_display_disable\n");
>> -
>> +static void dsi_display_ulps_disable(struct dsi_data *dsi,
>> +		bool disconnect_lanes, bool enter_ulps)
>> +{
>>  	WARN_ON(!dsi_bus_is_locked(dsi));
>>  
>>  	mutex_lock(&dsi->lock);
>> @@ -4110,6 +4110,27 @@ static void dsi_display_disable(struct omap_dss_device *dssdev,
>>  	mutex_unlock(&dsi->lock);
>>  }
>>  
>> +static void dsi_display_disable(struct omap_dss_device *dssdev)
>> +{
>> +	struct dsi_data *dsi = to_dsi_data(dssdev);
>> +
>> +	DSSDBG("dsi_display_disable\n");
>> +
>> +	dsi_display_ulps_disable(dsi, true, false);
>> +}
>> +
>> +static void dsi_ulps(struct omap_dss_device *dssdev, bool enable)
>> +{
>> +	struct dsi_data *dsi = to_dsi_data(dssdev);
>> +
>> +	DSSDBG("dsi_ulps\n");
>> +
>> +	if (enable)
>> +		dsi_display_ulps_disable(dsi, false, true);
>> +	else
>> +		dsi_display_ulps_enable(dsi);
> 
> The names are fairly confusing. I would expect
> dsi_display_ulps_disable() to disable ULPS mode.

It is fairly confusing naming. It's actually something like dsi_display_displaye_featuring_ulps.
I'll rename those to

_dsi_display_disable and _dsi_display_enable. So they're just lower level enable/disable functions,
which also handle ulps.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki