Re: [U-Boot] [PATCH v4 05/15] dm: Add a dsi host uclass

Hi,

On Fri, 13 Sep 2019 at 02:48, Yannick Fertré yannick.fertre@st.com wrote:

DSI host can usefully be modelled as their own uclass.

What is DSI?

Signed-off-by: Yannick Fertré yannick.fertre@st.com

arch/sandbox/dts/sandbox.dts | 6 ++- configs/sandbox_defconfig | 1 + drivers/video/Kconfig | 7 ++++ drivers/video/Makefile | 2 + drivers/video/dsi-host-uclass.c | 39 +++++++++++++++++++ drivers/video/sandbox_dsi_host.c | 83 ++++++++++++++++++++++++++++++++++++++++ include/dm/uclass-id.h | 1 + include/dsi_host.h | 57 +++++++++++++++++++++++++++ test/dm/Makefile | 1 + test/dm/dsi_host.c | 58 ++++++++++++++++++++++++++++ 10 files changed, 254 insertions(+), 1 deletion(-) create mode 100644 drivers/video/dsi-host-uclass.c create mode 100644 drivers/video/sandbox_dsi_host.c create mode 100644 include/dsi_host.h create mode 100644 test/dm/dsi_host.c

diff --git a/arch/sandbox/dts/sandbox.dts b/arch/sandbox/dts/sandbox.dts index 16a33db..f1637c8 100644 --- a/arch/sandbox/dts/sandbox.dts +++ b/arch/sandbox/dts/sandbox.dts @@ -25,6 +25,11 @@ compatible = "google,cros-ec-sandbox"; };

•   dsi_host: dsi_host {
  

•           compatible = "sandbox,dsi-host";
  

•           status = "okay";
  

•   };
  

•   ethrawbus {
            compatible = "sandbox,eth-raw-bus";
            skip-localhost = <0>;
  

@@ -63,7 +68,6 @@ compatible = "sandbox,spi"; cs-gpios = <0>, <&gpio_a 0>; };

};

#include "sandbox.dtsi" diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig index 7355e3a..90f3e26 100644 --- a/configs/sandbox_defconfig +++ b/configs/sandbox_defconfig @@ -205,6 +205,7 @@ CONFIG_CONSOLE_ROTATION=y CONFIG_CONSOLE_TRUETYPE=y CONFIG_CONSOLE_TRUETYPE_CANTORAONE=y CONFIG_VIDEO_SANDBOX_SDL=y +CONFIG_VIDEO_DSI_HOST_SANDBOX=y CONFIG_OSD=y CONFIG_SANDBOX_OSD=y CONFIG_W1=y diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig index 36f666e..554b7db 100644 --- a/drivers/video/Kconfig +++ b/drivers/video/Kconfig @@ -686,6 +686,13 @@ config VIDEO_DW_HDMI rather requires a SoC-specific glue driver to call it), it can not be enabled from the configuration menu.

+config VIDEO_DSI_HOST_SANDBOX

•   bool "Enable sandbox for dsi host"
  

What is DSI?

•   depends on SANDBOX
  

•   help
  

•     Enable support for sandbox dsi host device used for testing
  

•     purposes.
  

Should have help here explaining what DSI is

config VIDEO_SIMPLE bool "Simple display driver for preconfigured display" help diff --git a/drivers/video/Makefile b/drivers/video/Makefile index 7df9b0b..5ed3590 100644 --- a/drivers/video/Makefile +++ b/drivers/video/Makefile @@ -11,6 +11,7 @@ obj-$(CONFIG_CONSOLE_ROTATION) += console_rotate.o obj-$(CONFIG_CONSOLE_TRUETYPE) += console_truetype.o fonts/ obj-$(CONFIG_DISPLAY) += display-uclass.o obj-$(CONFIG_DM_VIDEO) += backlight-uclass.o +obj-$(CONFIG_DM_VIDEO) += dsi-host-uclass.o obj-$(CONFIG_DM_VIDEO) += panel-uclass.o simple_panel.o obj-$(CONFIG_DM_VIDEO) += video-uclass.o vidconsole-uclass.o obj-$(CONFIG_DM_VIDEO) += video_bmp.o @@ -58,6 +59,7 @@ obj-$(CONFIG_VIDEO_MVEBU) += mvebu_lcd.o obj-$(CONFIG_VIDEO_MX3) += mx3fb.o videomodes.o obj-$(CONFIG_VIDEO_MXS) += mxsfb.o videomodes.o obj-$(CONFIG_VIDEO_OMAP3) += omap3_dss.o +obj-$(CONFIG_VIDEO_DSI_HOST_SANDBOX) += sandbox_dsi_host.o obj-$(CONFIG_VIDEO_SANDBOX_SDL) += sandbox_sdl.o obj-$(CONFIG_VIDEO_SIMPLE) += simplefb.o obj-$(CONFIG_VIDEO_TEGRA20) += tegra.o diff --git a/drivers/video/dsi-host-uclass.c b/drivers/video/dsi-host-uclass.c new file mode 100644 index 0000000..1db1f88 --- /dev/null +++ b/drivers/video/dsi-host-uclass.c @@ -0,0 +1,39 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Copyright (C) 2019 STMicroelectronics - All Rights Reserved
• • Author(s): Yannick Fertre yannick.fertre@st.com for STMicroelectronics.
• */

+#include <common.h> +#include <dm.h> +#include <dsi_host.h>

+int dsi_host_init(struct udevice *dev,

•             struct mipi_dsi_device *device,
  

•             struct display_timing *timings,
  

•             unsigned int max_data_lanes,
  

•             const struct mipi_dsi_phy_ops *phy_ops)
  

+{

•   struct dsi_host_ops *ops = dsi_host_get_ops(dev);
  

•   if (!ops->init)
  

•           return -ENOSYS;
  

•   return ops->init(dev, device, timings, max_data_lanes, phy_ops);
  

+}

+int dsi_host_enable(struct udevice *dev) +{

•   struct dsi_host_ops *ops = dsi_host_get_ops(dev);
  

•   if (!ops->enable)
  

•           return -ENOSYS;
  

•   return ops->enable(dev);
  

+}

+UCLASS_DRIVER(dsi_host) = {

•   .id             = UCLASS_DSI_HOST,
  

•   .name           = "dsi_host",
  

+}; diff --git a/drivers/video/sandbox_dsi_host.c b/drivers/video/sandbox_dsi_host.c new file mode 100644 index 0000000..ee01ed1 --- /dev/null +++ b/drivers/video/sandbox_dsi_host.c @@ -0,0 +1,83 @@ +// SPDX-License-Identifier: GPL-2.0+ OR BSD-3-Clause +/*

• • Copyright (C) 2019, STMicroelectronics - All Rights Reserved
• */

+#include <common.h> +#include <display.h> +#include <dm.h> +#include <dsi_host.h>

Please add struct comment

+struct sandbox_dsi_host_priv {

•   struct mipi_dsi_device *device;
  

•   struct display_timing *timings;
  

•   unsigned int max_data_lanes;
  

•   const struct mipi_dsi_phy_ops *phy_ops;
  

+};

+static int sandbox_dsi_host_init(struct udevice *dev,

•                            struct mipi_dsi_device *device,
  

•                            struct display_timing *timings,
  

•                            unsigned int max_data_lanes,
  

•                            const struct mipi_dsi_phy_ops *phy_ops)
  

+{

•   struct sandbox_dsi_host_priv *priv = dev_get_priv(dev);
  

•   if (!device)
  

•           return -1;
  

•   if (!timings)
  

•           return -2;
  

•   if (max_data_lanes == 0)
  

•           return -3;
  

•   if (!phy_ops)
  

•           return -4;
  

•   if (!phy_ops->init || !phy_ops->get_lane_mbps ||
  

•       !phy_ops->post_set_mode)
  

•           return -5;
  

•   priv->max_data_lanes = max_data_lanes;
  

•   priv->phy_ops = phy_ops;
  

•   priv->timings = timings;
  

•   priv->device = device;
  

•   return 0;
  

+}

+static int sandbox_dsi_host_enable(struct udevice *dev) +{

•   struct sandbox_dsi_host_priv *priv = dev_get_priv(dev);
  

•   unsigned int lane_mbps;
  

•   int ret;
  

•   priv->phy_ops->init(priv->device);
  

•   ret = priv->phy_ops->get_lane_mbps(priv->device, priv->timings, 2,
  

•                                      MIPI_DSI_FMT_RGB888, &lane_mbps);
  

•   if (ret)
  

•           return -1;
  

•   priv->phy_ops->post_set_mode(priv->device, MIPI_DSI_MODE_VIDEO);
  

•   return 0;
  

+}

+struct dsi_host_ops sandbox_dsi_host_ops = {

•   .init = sandbox_dsi_host_init,
  

•   .enable = sandbox_dsi_host_enable,
  

+};

+static const struct udevice_id sandbox_dsi_host_ids[] = {

•   { .compatible = "sandbox,dsi-host"},
  

•   { }
  

+};

+U_BOOT_DRIVER(sandbox_dsi_host) = {

•   .name                 = "sandbox-dsi-host",
  

•   .id                   = UCLASS_DSI_HOST,
  

•   .of_match             = sandbox_dsi_host_ids,
  

•   .ops                  = &sandbox_dsi_host_ops,
  

•   .priv_auto_alloc_size = sizeof(struct sandbox_dsi_host_priv),
  

+}; diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index d4d9610..06989c7 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -39,6 +39,7 @@ enum uclass_id { UCLASS_CPU, /* CPU, typically part of an SoC */ UCLASS_CROS_EC, /* Chrome OS EC */ UCLASS_DISPLAY, /* Display (e.g. DisplayPort, HDMI) */

•   UCLASS_DSI_HOST,        /* Dsi host device */
  

Do you have room to write out DSI in full?

    UCLASS_DMA,             /* Direct Memory Access */
    UCLASS_EFI,             /* EFI managed devices */
    UCLASS_ETH,             /* Ethernet device */

diff --git a/include/dsi_host.h b/include/dsi_host.h new file mode 100644 index 0000000..9ff226f --- /dev/null +++ b/include/dsi_host.h @@ -0,0 +1,57 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*

• • Copyright (C) 2019 STMicroelectronics - All Rights Reserved
• • Author(s): Yannick Fertre yannick.fertre@st.com for STMicroelectronics.
• */

+#ifndef _DSI_HOST_H +#define _DSI_HOST_H

+#include <mipi_dsi.h>

+struct dsi_host_ops {

•   /**
  

•    * init() - Enable the dsi_host
  

•    *
  

•    * @dev: dsi host device
  

add the rest of the args

•    * @return 0 if OK, -ve on error
  

•    */
  

•   int (*init)(struct udevice *dev,
  

•               struct mipi_dsi_device *device,
  

•               struct display_timing *timings,
  

•               unsigned int max_data_lanes,
  

•               const struct mipi_dsi_phy_ops *phy_ops);
  

•   /**
  

•    * enable() - Enable the dsi_host
  

•    *
  

•    * @dev: dsi host device
  

•    * @return 0 if OK, -ve on error
  

•    */
  

•   int (*enable)(struct udevice *dev);
  

How can I disable it?

+};

+#define dsi_host_get_ops(dev) ((struct dsi_host_ops *)(dev)->driver->ops)

+/**

• • dsi_host_init
• • @dev: dsi host device

missing args

• • @return 0 if OK, -ve on error
• */

+int dsi_host_init(struct udevice *dev,

•             struct mipi_dsi_device *device,
  

•             struct display_timing *timings,
  

•             unsigned int max_data_lanes,
  

•             const struct mipi_dsi_phy_ops *phy_ops);
  

+/**

• • dsi_host_enable
• • @dev: dsi host device
• • @return 0 if OK, -ve on error
• */

+int dsi_host_enable(struct udevice *dev);

+#endif diff --git a/test/dm/Makefile b/test/dm/Makefile index 55a7940..75c4cae 100644 --- a/test/dm/Makefile +++ b/test/dm/Makefile @@ -18,6 +18,7 @@ obj-$(CONFIG_BLK) += blk.o obj-$(CONFIG_BOARD) += board.o obj-$(CONFIG_DM_BOOTCOUNT) += bootcount.o obj-$(CONFIG_CLK) += clk.o clk_ccf.o +obj-$(CONFIG_DM_VIDEO) += dsi_host.o

Shouldn't this have its own CONFIG? Not all boards will need this

obj-$(CONFIG_DM_ETH) += eth.o obj-$(CONFIG_FIRMWARE) += firmware.o obj-$(CONFIG_DM_GPIO) += gpio.o diff --git a/test/dm/dsi_host.c b/test/dm/dsi_host.c new file mode 100644 index 0000000..59fcd55 --- /dev/null +++ b/test/dm/dsi_host.c @@ -0,0 +1,58 @@ +// SPDX-License-Identifier: GPL-2.0+ OR BSD-3-Clause +/*

• • Copyright (C) 2019 STMicroelectronics - All Rights Reserved
• • Author(s): Yannick Fertre yannick.fertre@st.com for STMicroelectronics.
• */

+#include <common.h> +#include <dm.h> +#include <dsi_host.h> +#include <asm/state.h> +#include <asm/test.h> +#include <dm/test.h> +#include <test/ut.h>

+static int dm_test_dsi_host_phy_init(void *priv_data) +{

•   return 0;
  

+}

+static void dm_test_dsi_host_phy_post_set_mode(void *priv_data,

•                                          unsigned long mode_flags)
  

+{ +}

+static int dm_test_dsi_host_phy_get_lane_mbps(void *priv_data,

•                                         struct display_timing *timings,
  

•                                         u32 lanes,
  

•                                         u32 format,
  

•                                         unsigned int *lane_mbps)
  

+{

•   return 0;
  

+}

+static const struct mipi_dsi_phy_ops dm_test_dsi_host_phy_ops = {

•   .init = dm_test_dsi_host_phy_init,
  

•   .get_lane_mbps = dm_test_dsi_host_phy_get_lane_mbps,
  

•   .post_set_mode = dm_test_dsi_host_phy_post_set_mode,
  

+};

+/* Test that dsi_host driver functions are called */ +static int dm_test_dsi_host(struct unit_test_state *uts) +{

•   struct udevice *dev;
  

•   struct mipi_dsi_device device;
  

•   struct display_timing timings;
  

•   unsigned int max_data_lanes = 4;
  

•   ut_assertok(uclass_first_device_err(UCLASS_DSI_HOST, &dev));
  

•   ut_assertok(dsi_host_init(dev, &device, &timings, max_data_lanes,
  

•                             &dm_test_dsi_host_phy_ops));
  

•   ut_assertok(dsi_host_enable(dev));
  

•   return 0;
  

+}

+DM_TEST(dm_test_dsi_host, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);

2.7.4

Apart from these nits it looks OK.

Regards, Simon