On Fri, 13 Jul 2018 14:11:10 +0200 Yannick Fertré yannick.fertre@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