[U-Boot] [ v2 02/10] video: stm32: stm32_ltdc: add bridge to display controller

[Anatolij Gustschin <agust@denx.de>]

On Fri, 13 Jul 2018 14:11:10 +0200
Yannick Fertré yannick.fertre at st.com wrote:

> Manage a bridge insert between the display controller & a panel.
> 
> Signed-off-by: Yannick Fertré <yannick.fertre@st.com>
> ---
>  drivers/video/stm32/stm32_ltdc.c | 154 +++++++++++++++++++++++----------------
>  1 file changed, 92 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/video/stm32/stm32_ltdc.c b/drivers/video/stm32/stm32_ltdc.c
> index dc6c889..4a1db5e 100644
> --- a/drivers/video/stm32/stm32_ltdc.c
> +++ b/drivers/video/stm32/stm32_ltdc.c
...
> @@ -330,96 +328,128 @@ static int stm32_ltdc_probe(struct udevice *dev)
>  	struct video_uc_platdata *uc_plat = dev_get_uclass_platdata(dev);
>  	struct video_priv *uc_priv = dev_get_uclass_priv(dev);
>  	struct stm32_ltdc_priv *priv = dev_get_priv(dev);
> -	struct udevice *panel;
> +#ifdef CONFIG_VIDEO_BRIDGE
> +	struct udevice *bridge = NULL;
> +#endif

Please drop ifdef here. See below.

...
> -	rate = clk_set_rate(&pclk, priv->timing.pixelclock.typ);
> -	if (rate < 0) {
> -		debug("%s: fail to set pixel clock %d hz %d hz\n",
> -		      __func__, priv->timing.pixelclock.typ, rate);
> -		return rate;
> +#ifdef CONFIG_VIDEO_BRIDGE
> +	ret = uclass_get_device(UCLASS_VIDEO_BRIDGE, 0, &bridge);
> +	if (ret)
> +		debug("No video bridge, or no backlight on bridge\n");
> +
> +	if (bridge) {
> +		ret = video_bridge_attach(bridge);
> +		if (ret) {
> +			dev_err(dev, "fail to attach bridge\n");
> +			return ret;
> +		}
>  	}
> -
> -	debug("%s: Set pixel clock req %d hz get %d hz\n", __func__,
> -	      priv->timing.pixelclock.typ, rate);
> -
> +#endif

Can we do without ifdefs and use IS_ENABLED() instead ?
E.g. like:

	if (IS_ENABLED(CONFIG_VIDEO_BRIDGE)) {
		ret = uclass_get_device(UCLASS_VIDEO_BRIDGE, 0, &bridge);
		...
	}

...
> +#ifdef CONFIG_VIDEO_BRIDGE
> +	if (bridge) {
> +		ret = video_bridge_set_backlight(bridge, 80);
> +		if (ret) {
> +			dev_err(dev, "fail to set backlight\n");
> +			return ret;
> +		}
> +	} else {
> +		ret = panel_enable_backlight(panel);
> +		if (ret) {
> +			dev_err(dev, "panel %s enable backlight error %d\n",
> +				panel->name, ret);
> +			return ret;
> +		}
> +	}
> +#else
> +	ret = panel_enable_backlight(panel);
> +	if (ret) {
> +		dev_err(dev, "panel %s enable backlight error %d\n",
> +			panel->name, ret);
> +		return ret;
> +	}
> +#endif

Please reduce code duplication:

        if (!bridge) {
                ret = panel_enable_backlight(panel);
                if (ret) {
                        dev_err(dev, "panel %s enable backlight error %d\n",
                                panel->name, ret);
                        return ret;
                }
        } else if (IS_ENABLED(CONFIG_VIDEO_BRIDGE)) {
                ret = video_bridge_set_backlight(bridge, 80);
                if (ret) {
                        dev_err(dev, "fail to set backlight\n");
                        return ret;
                }
        }

--
Anatolij