diff mbox series

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

Message ID 20201105120333.947408-29-tomi.valkeinen@ti.com (mailing list archive)
State New, archived
Headers show
Series Convert DSI code to use drm_mipi_dsi and drm_panel | expand

Commit Message

Tomi Valkeinen Nov. 5, 2020, 12:03 p.m. UTC 
From: Sebastian Reichel <sebastian.reichel@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@collabora.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@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(-)

Comments

Laurent Pinchart Nov. 9, 2020, 9:57 a.m. UTC | #1 
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@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@collabora.com>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@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.

> +}
> +
>  static int dsi_enable_te(struct dsi_data *dsi, bool enable)
>  {
>  	dsi->te_enabled = enable;
> @@ -4812,9 +4833,10 @@ static const struct omap_dss_device_ops dsi_ops = {
>  	.connect = dsi_connect,
>  	.disconnect = dsi_disconnect,
>  	.enable = dsi_display_enable,
> +	.disable = dsi_display_disable,
>  
>  	.dsi = {
> -		.disable = dsi_display_disable,
> +		.ulps = dsi_ulps,
>  
>  		.set_config = dsi_set_config,
>  
> diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> index 43eba2ea1f96..0d82ba34ca89 100644
> --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> @@ -281,10 +281,9 @@ struct omap_dss_writeback_info {
>  };
>  
>  struct omapdss_dsi_ops {
> -	void (*disable)(struct omap_dss_device *dssdev, bool disconnect_lanes,
> -			bool enter_ulps);
> -
>  	/* bus configuration */
> +	void (*ulps)(struct omap_dss_device *dssdev, bool enable);
> +
>  	int (*set_config)(struct omap_dss_device *dssdev,
>  			const struct omap_dss_dsi_config *cfg);
>
Tomi Valkeinen Nov. 11, 2020, 2:05 p.m. UTC | #2 
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@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@collabora.com>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@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
diff mbox series

Patch

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);
+}
+
 static int dsi_enable_te(struct dsi_data *dsi, bool enable)
 {
 	dsi->te_enabled = enable;
@@ -4812,9 +4833,10 @@  static const struct omap_dss_device_ops dsi_ops = {
 	.connect = dsi_connect,
 	.disconnect = dsi_disconnect,
 	.enable = dsi_display_enable,
+	.disable = dsi_display_disable,
 
 	.dsi = {
-		.disable = dsi_display_disable,
+		.ulps = dsi_ulps,
 
 		.set_config = dsi_set_config,
 
diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
index 43eba2ea1f96..0d82ba34ca89 100644
--- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
+++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
@@ -281,10 +281,9 @@  struct omap_dss_writeback_info {
 };
 
 struct omapdss_dsi_ops {
-	void (*disable)(struct omap_dss_device *dssdev, bool disconnect_lanes,
-			bool enter_ulps);
-
 	/* bus configuration */
+	void (*ulps)(struct omap_dss_device *dssdev, bool enable);
+
 	int (*set_config)(struct omap_dss_device *dssdev,
 			const struct omap_dss_dsi_config *cfg);