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@xxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxx>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxx>
>> ---
>> .../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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
