[PATCHv2,04/56] omap/drm: drop unused dsi.configure_pins
diff mbox series

Message ID 20200224232126.3385250-5-sebastian.reichel@collabora.com
State New
Headers show
Series
  • drm/omap: Convert DSI code to use drm_mipi_dsi and drm_panel
Related show

Commit Message

Sebastian Reichel Feb. 24, 2020, 11:20 p.m. UTC
The panel-dsi-cm's ddata->pin_config is always NULL, so this
callback is never called. Instead the DSI encoder gets the pin
configuration directly from DT.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c | 11 -----------
 drivers/gpu/drm/omapdrm/dss/dsi.c               |  1 -
 drivers/gpu/drm/omapdrm/dss/omapdss.h           |  2 --
 3 files changed, 14 deletions(-)

Comments

Laurent Pinchart Feb. 24, 2020, 11:42 p.m. UTC | #1
Hi Sebastian,

Thank you for the patch.

On Tue, Feb 25, 2020 at 12:20:34AM +0100, Sebastian Reichel wrote:
> The panel-dsi-cm's ddata->pin_config is always NULL, so this
> callback is never called. Instead the DSI encoder gets the pin
> configuration directly from DT.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c | 11 -----------
>  drivers/gpu/drm/omapdrm/dss/dsi.c               |  1 -
>  drivers/gpu/drm/omapdrm/dss/omapdss.h           |  2 --
>  3 files changed, 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
> index 3484b5d4a91c..e7fe5d702337 100644
> --- a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
> +++ b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
> @@ -68,8 +68,6 @@ struct panel_drv_data {
>  	int width_mm;
>  	int height_mm;
>  
> -	struct omap_dsi_pin_config pin_config;
> -
>  	/* runtime variables */
>  	bool enabled;
>  
> @@ -623,15 +621,6 @@ static int dsicm_power_on(struct panel_drv_data *ddata)
>  		}
>  	}
>  
> -	if (ddata->pin_config.num_pins > 0) {
> -		r = src->ops->dsi.configure_pins(src, &ddata->pin_config);
> -		if (r) {
> -			dev_err(&ddata->pdev->dev,
> -				"failed to configure DSI pins\n");
> -			goto err_vddi;
> -		}
> -	}
> -
>  	r = src->ops->dsi.set_config(src, &dsi_config);
>  	if (r) {
>  		dev_err(&ddata->pdev->dev, "failed to configure DSI\n");
> diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c
> index 79ddfbfd1b58..8c39823a8295 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dsi.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
> @@ -4892,7 +4892,6 @@ static const struct omap_dss_device_ops dsi_ops = {
>  
>  		.enable_hs = dsi_vc_enable_hs,
>  
> -		.configure_pins = dsi_configure_pins,
>  		.set_config = dsi_set_config,
>  
>  		.enable_video_output = dsi_enable_video_output,
> diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> index cbbe10b2b60d..b0424daaceed 100644
> --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> @@ -292,8 +292,6 @@ struct omapdss_dsi_ops {

I think you can drop the definition of the omap_dsi_pin_config structure
earlier in this file too, as well as the OMAP_DSS_MAX_DSI_PINS macro.
With this fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  	/* bus configuration */
>  	int (*set_config)(struct omap_dss_device *dssdev,
>  			const struct omap_dss_dsi_config *cfg);
> -	int (*configure_pins)(struct omap_dss_device *dssdev,
> -			const struct omap_dsi_pin_config *pin_cfg);
>  
>  	void (*enable_hs)(struct omap_dss_device *dssdev, int channel,
>  			bool enable);
Sebastian Reichel Feb. 26, 2020, 9:28 p.m. UTC | #2
Hi Laurent,

On Tue, Feb 25, 2020 at 01:42:49AM +0200, Laurent Pinchart wrote:
> Hi Sebastian,
> 
> Thank you for the patch.
> 
> On Tue, Feb 25, 2020 at 12:20:34AM +0100, Sebastian Reichel wrote:
> > The panel-dsi-cm's ddata->pin_config is always NULL, so this
> > callback is never called. Instead the DSI encoder gets the pin
> > configuration directly from DT.
> > 
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > ---
> >  drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c | 11 -----------
> >  drivers/gpu/drm/omapdrm/dss/dsi.c               |  1 -
> >  drivers/gpu/drm/omapdrm/dss/omapdss.h           |  2 --
> >  3 files changed, 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
> > index 3484b5d4a91c..e7fe5d702337 100644
> > --- a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
> > +++ b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
> > @@ -68,8 +68,6 @@ struct panel_drv_data {
> >  	int width_mm;
> >  	int height_mm;
> >  
> > -	struct omap_dsi_pin_config pin_config;
> > -
> >  	/* runtime variables */
> >  	bool enabled;
> >  
> > @@ -623,15 +621,6 @@ static int dsicm_power_on(struct panel_drv_data *ddata)
> >  		}
> >  	}
> >  
> > -	if (ddata->pin_config.num_pins > 0) {
> > -		r = src->ops->dsi.configure_pins(src, &ddata->pin_config);
> > -		if (r) {
> > -			dev_err(&ddata->pdev->dev,
> > -				"failed to configure DSI pins\n");
> > -			goto err_vddi;
> > -		}
> > -	}
> > -
> >  	r = src->ops->dsi.set_config(src, &dsi_config);
> >  	if (r) {
> >  		dev_err(&ddata->pdev->dev, "failed to configure DSI\n");
> > diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c
> > index 79ddfbfd1b58..8c39823a8295 100644
> > --- a/drivers/gpu/drm/omapdrm/dss/dsi.c
> > +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
> > @@ -4892,7 +4892,6 @@ static const struct omap_dss_device_ops dsi_ops = {
> >  
> >  		.enable_hs = dsi_vc_enable_hs,
> >  
> > -		.configure_pins = dsi_configure_pins,
> >  		.set_config = dsi_set_config,
> >  
> >  		.enable_video_output = dsi_enable_video_output,
> > diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> > index cbbe10b2b60d..b0424daaceed 100644
> > --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> > +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> > @@ -292,8 +292,6 @@ struct omapdss_dsi_ops {
> 
> I think you can drop the definition of the omap_dsi_pin_config structure
> earlier in this file too, as well as the OMAP_DSS_MAX_DSI_PINS macro.
> With this fixed,

No, the struct is still used by the code setting up the pins from
DT.

-- Sebastian

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> >  	/* bus configuration */
> >  	int (*set_config)(struct omap_dss_device *dssdev,
> >  			const struct omap_dss_dsi_config *cfg);
> > -	int (*configure_pins)(struct omap_dss_device *dssdev,
> > -			const struct omap_dsi_pin_config *pin_cfg);
> >  
> >  	void (*enable_hs)(struct omap_dss_device *dssdev, int channel,
> >  			bool enable);
> 
> -- 
> Regards,
> 
> Laurent Pinchart
Laurent Pinchart Feb. 26, 2020, 9:36 p.m. UTC | #3
Hi Sebastian,

On Wed, Feb 26, 2020 at 10:28:19PM +0100, Sebastian Reichel wrote:
> On Tue, Feb 25, 2020 at 01:42:49AM +0200, Laurent Pinchart wrote:
> > On Tue, Feb 25, 2020 at 12:20:34AM +0100, Sebastian Reichel wrote:
> > > The panel-dsi-cm's ddata->pin_config is always NULL, so this
> > > callback is never called. Instead the DSI encoder gets the pin
> > > configuration directly from DT.
> > > 
> > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > > ---
> > >  drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c | 11 -----------
> > >  drivers/gpu/drm/omapdrm/dss/dsi.c               |  1 -
> > >  drivers/gpu/drm/omapdrm/dss/omapdss.h           |  2 --
> > >  3 files changed, 14 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
> > > index 3484b5d4a91c..e7fe5d702337 100644
> > > --- a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
> > > +++ b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
> > > @@ -68,8 +68,6 @@ struct panel_drv_data {
> > >  	int width_mm;
> > >  	int height_mm;
> > >  
> > > -	struct omap_dsi_pin_config pin_config;
> > > -
> > >  	/* runtime variables */
> > >  	bool enabled;
> > >  
> > > @@ -623,15 +621,6 @@ static int dsicm_power_on(struct panel_drv_data *ddata)
> > >  		}
> > >  	}
> > >  
> > > -	if (ddata->pin_config.num_pins > 0) {
> > > -		r = src->ops->dsi.configure_pins(src, &ddata->pin_config);
> > > -		if (r) {
> > > -			dev_err(&ddata->pdev->dev,
> > > -				"failed to configure DSI pins\n");
> > > -			goto err_vddi;
> > > -		}
> > > -	}
> > > -
> > >  	r = src->ops->dsi.set_config(src, &dsi_config);
> > >  	if (r) {
> > >  		dev_err(&ddata->pdev->dev, "failed to configure DSI\n");
> > > diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c
> > > index 79ddfbfd1b58..8c39823a8295 100644
> > > --- a/drivers/gpu/drm/omapdrm/dss/dsi.c
> > > +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
> > > @@ -4892,7 +4892,6 @@ static const struct omap_dss_device_ops dsi_ops = {
> > >  
> > >  		.enable_hs = dsi_vc_enable_hs,
> > >  
> > > -		.configure_pins = dsi_configure_pins,
> > >  		.set_config = dsi_set_config,
> > >  
> > >  		.enable_video_output = dsi_enable_video_output,
> > > diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> > > index cbbe10b2b60d..b0424daaceed 100644
> > > --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> > > +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> > > @@ -292,8 +292,6 @@ struct omapdss_dsi_ops {
> > 
> > I think you can drop the definition of the omap_dsi_pin_config structure
> > earlier in this file too, as well as the OMAP_DSS_MAX_DSI_PINS macro.
> > With this fixed,
> 
> No, the struct is still used by the code setting up the pins from
> DT.

Indeed, my bad. I think I'd pass the unsigned int num_pins and const int
*pins to dsi_configure_pins() directly to drop the structure, but that
can be done in a subsequent patch (maybe it is already :-)).

> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

This tag holds.

> > >  	/* bus configuration */
> > >  	int (*set_config)(struct omap_dss_device *dssdev,
> > >  			const struct omap_dss_dsi_config *cfg);
> > > -	int (*configure_pins)(struct omap_dss_device *dssdev,
> > > -			const struct omap_dsi_pin_config *pin_cfg);
> > >  
> > >  	void (*enable_hs)(struct omap_dss_device *dssdev, int channel,
> > >  			bool enable);
Sebastian Reichel Feb. 26, 2020, 10:25 p.m. UTC | #4
Hi Laurent,

On Wed, Feb 26, 2020 at 11:36:30PM +0200, Laurent Pinchart wrote:
> > > I think you can drop the definition of the omap_dsi_pin_config structure
> > > earlier in this file too, as well as the OMAP_DSS_MAX_DSI_PINS macro.
> > > With this fixed,
> > 
> > No, the struct is still used by the code setting up the pins from
> > DT.
> 
> Indeed, my bad. I think I'd pass the unsigned int num_pins and const int
> *pins to dsi_configure_pins() directly to drop the structure, but that
> can be done in a subsequent patch (maybe it is already :-)).

I added quite some cleanups at the end of the series, but there is
still quite a few cleanups possible within the DSI encoder
(including this one). Cleaning up dsi.c takes some time and rebasing
this code gets annoying.

After this series the cleanups mostly are internal to dsi.c and
should reduce merge conflict probability.  Also I feel a bit
uncomfortable, since we currently have no DSI video mode user.
Nikolaus Schaller is working on adding support for such a system, so
it would be nice to get that supported first making it possible to
do easy bisecting for issues introduced by refactoring. (This is not
specifically about dsi_configure_pins of course)

> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> This tag holds.

Thanks.

-- Sebastian
Tomi Valkeinen March 25, 2020, 12:45 p.m. UTC | #5
On 25/02/2020 01:20, Sebastian Reichel wrote:
> The panel-dsi-cm's ddata->pin_config is always NULL, so this
> callback is never called. Instead the DSI encoder gets the pin
> configuration directly from DT.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>   drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c | 11 -----------
>   drivers/gpu/drm/omapdrm/dss/dsi.c               |  1 -
>   drivers/gpu/drm/omapdrm/dss/omapdss.h           |  2 --
>   3 files changed, 14 deletions(-)

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

  Tomi

Patch
diff mbox series

diff --git a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
index 3484b5d4a91c..e7fe5d702337 100644
--- a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
+++ b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
@@ -68,8 +68,6 @@  struct panel_drv_data {
 	int width_mm;
 	int height_mm;
 
-	struct omap_dsi_pin_config pin_config;
-
 	/* runtime variables */
 	bool enabled;
 
@@ -623,15 +621,6 @@  static int dsicm_power_on(struct panel_drv_data *ddata)
 		}
 	}
 
-	if (ddata->pin_config.num_pins > 0) {
-		r = src->ops->dsi.configure_pins(src, &ddata->pin_config);
-		if (r) {
-			dev_err(&ddata->pdev->dev,
-				"failed to configure DSI pins\n");
-			goto err_vddi;
-		}
-	}
-
 	r = src->ops->dsi.set_config(src, &dsi_config);
 	if (r) {
 		dev_err(&ddata->pdev->dev, "failed to configure DSI\n");
diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c
index 79ddfbfd1b58..8c39823a8295 100644
--- a/drivers/gpu/drm/omapdrm/dss/dsi.c
+++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
@@ -4892,7 +4892,6 @@  static const struct omap_dss_device_ops dsi_ops = {
 
 		.enable_hs = dsi_vc_enable_hs,
 
-		.configure_pins = dsi_configure_pins,
 		.set_config = dsi_set_config,
 
 		.enable_video_output = dsi_enable_video_output,
diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
index cbbe10b2b60d..b0424daaceed 100644
--- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
+++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
@@ -292,8 +292,6 @@  struct omapdss_dsi_ops {
 	/* bus configuration */
 	int (*set_config)(struct omap_dss_device *dssdev,
 			const struct omap_dss_dsi_config *cfg);
-	int (*configure_pins)(struct omap_dss_device *dssdev,
-			const struct omap_dsi_pin_config *pin_cfg);
 
 	void (*enable_hs)(struct omap_dss_device *dssdev, int channel,
 			bool enable);