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

DSI host can usefully be modelled as their own uclass.

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

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