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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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);
>  

-- 
Regards,

Laurent Pinchart