Re: [PATCH v3 50/56] drm/omap: dsi: simplify pin config

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

Hi Tomi and Sebastian,

Thank you for the patch.

On Thu, Nov 05, 2020 at 02:03:27PM +0200, Tomi Valkeinen wrote:
> From: Sebastian Reichel <sebastian.reichel@collabora.com>
> 
> Simplify DSI pin config, which always originates from DT
> nowadays. With the code being fully contained in the DSI
> encoder, we can drop the public structure.
> 
> Since the function is no longer exposed, it now directly
> takes the private DSI data pointer. This drop a pointless

s/drop/drops/

> conversion and means the pins can be configured earlier.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/dsi.c     | 33 +++++++++------------------
>  drivers/gpu/drm/omapdrm/dss/omapdss.h | 15 ------------
>  2 files changed, 11 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c
> index f47d7e3bb631..76e4f607d8cf 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dsi.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
> @@ -3568,12 +3568,9 @@ static void dsi_proto_timings(struct dsi_data *dsi)
>  	}
>  }
>  
> -static int dsi_configure_pins(struct omap_dss_device *dssdev,
> -		const struct omap_dsi_pin_config *pin_cfg)
> +static int dsi_configure_pins(struct dsi_data *dsi,
> +		int num_pins, const u32 *pins)
>  {
> -	struct dsi_data *dsi = to_dsi_data(dssdev);
> -	int num_pins;
> -	const int *pins;
>  	struct dsi_lane_config lanes[DSI_MAX_NR_LANES];
>  	int num_lanes;
>  	int i;
> @@ -3586,9 +3583,6 @@ static int dsi_configure_pins(struct omap_dss_device *dssdev,
>  		DSI_LANE_DATA4,
>  	};
>  
> -	num_pins = pin_cfg->num_pins;
> -	pins = pin_cfg->pins;
> -
>  	if (num_pins < 4 || num_pins > dsi->num_lanes_supported * 2
>  			|| num_pins % 2 != 0)
>  		return -EINVAL;
> @@ -3600,7 +3594,7 @@ static int dsi_configure_pins(struct omap_dss_device *dssdev,
>  
>  	for (i = 0; i < num_pins; i += 2) {
>  		u8 lane, pol;
> -		int dx, dy;
> +		u32 dx, dy;

Is this change needed ?

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

>  
>  		dx = pins[i];
>  		dy = pins[i + 1];
> @@ -5481,9 +5475,8 @@ static int dsi_probe_of(struct dsi_data *dsi)
>  	struct property *prop;
>  	u32 lane_arr[10];
>  	int len, num_pins;
> -	int r, i;
> +	int r;
>  	struct device_node *ep;
> -	struct omap_dsi_pin_config pin_cfg;
>  
>  	ep = of_graph_get_endpoint_by_regs(node, 0, 0);
>  	if (!ep)
> @@ -5511,11 +5504,7 @@ static int dsi_probe_of(struct dsi_data *dsi)
>  		goto err;
>  	}
>  
> -	pin_cfg.num_pins = num_pins;
> -	for (i = 0; i < num_pins; ++i)
> -		pin_cfg.pins[i] = (int)lane_arr[i];
> -
> -	r = dsi_configure_pins(&dsi->output, &pin_cfg);
> +	r = dsi_configure_pins(dsi, num_pins, lane_arr);
>  	if (r) {
>  		dev_err(dsi->dev, "failed to configure pins");
>  		goto err;
> @@ -5728,6 +5717,12 @@ static int dsi_probe(struct platform_device *pdev)
>  	dsi->host.ops = &omap_dsi_host_ops;
>  	dsi->host.dev = &pdev->dev;
>  
> +	r = dsi_probe_of(dsi);
> +	if (r) {
> +		DSSERR("Invalid DSI DT data\n");
> +		goto err_pm_disable;
> +	}
> +
>  	r = mipi_dsi_host_register(&dsi->host);
>  	if (r < 0) {
>  		dev_err(&pdev->dev, "failed to register DSI host: %d\n", r);
> @@ -5738,12 +5733,6 @@ static int dsi_probe(struct platform_device *pdev)
>  	if (r)
>  		goto err_dsi_host_unregister;
>  
> -	r = dsi_probe_of(dsi);
> -	if (r) {
> -		DSSERR("Invalid DSI DT data\n");
> -		goto err_uninit_output;
> -	}
> -
>  	r = component_add(&pdev->dev, &dsi_component_ops);
>  	if (r)
>  		goto err_uninit_output;
> diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> index a1236b8ef7ea..4a0826c8fed5 100644
> --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> @@ -243,21 +243,6 @@ struct omap_overlay_manager_info {
>  	struct omap_dss_cpr_coefs cpr_coefs;
>  };
>  
> -/* 22 pins means 1 clk lane and 10 data lanes */
> -#define OMAP_DSS_MAX_DSI_PINS 22
> -
> -struct omap_dsi_pin_config {
> -	int num_pins;
> -	/*
> -	 * pin numbers in the following order:
> -	 * clk+, clk-
> -	 * data1+, data1-
> -	 * data2+, data2-
> -	 * ...
> -	 */
> -	int pins[OMAP_DSS_MAX_DSI_PINS];
> -};
> -
>  struct omap_dss_writeback_info {
>  	u32 paddr;
>  	u32 p_uv_addr;

-- 
Regards,

Laurent Pinchart