Re: [RFC v2 0/5] Common Display Framework

Hi Vikas,

On Tuesday 18 of December 2012 08:31:30 Vikas Sajjan wrote:

 Hi Laurent,

Thanks for the reply.

On 17 December 2012 20:55, Laurent Pinchart <

laurent.pinchart@ideasonboard.com> wrote:

quoted

Hi Vikas,

Sorry for the late reply. I now have more time to work on CDF, so
delays should be much shorter.

On Thursday 06 December 2012 10:51:15 Vikas Sajjan wrote:

quoted

Hi Laurent,

I was thinking of porting CDF to samsung EXYNOS 5250 platform, what
I

found

quoted

is that, the exynos display controller is MIPI DSI based controller.

But if I look at CDF patches, it has only support for MIPI DBI based

Display

quoted

controller.

So my question is, do we have any generic framework for MIPI DSI
based
display controller? basically I wanted to know, how to go about
porting

CDF

quoted

for such kind of display controller.

MIPI DSI support is not available yet. The only reason for that is
that I don't have any MIPI DSI hardware to write and test the code
with :-)

The common display framework should definitely support MIPI DSI. I
think the
existing MIPI DBI code could be used as a base, so the implementation
shouldn't be too high.

Yeah, i was also thinking in similar lines, below is my though for
MIPI

DSI support in CDF.

o   MIPI DSI support as part of CDF framework will expose

§  mipi_dsi_register_device(mpi_device) (will be called mach-xxx-dt.c
file )

§  mipi_dsi_register_driver(mipi_driver, bus ops) (will be called from
platform specific init driver call )

·    bus ops will be

o   read data

o   write data

o   write command

§  MIPI DSI will be registered as bus_register()

When MIPI DSI probe is called, it (e.g., Exynos or OMAP MIPI DSI) will
initialize the MIPI DSI HW IP.

 This probe will also parse the DT file for MIPI DSI based panel, add
the panel device (device_add() ) to kernel and register the display
entity with its control and  video ops with CDF.

I can give this a try.

I am currently in progress of reworking Exynos MIPI DSIM code and s6e8ax0 
LCD driver to use the v2 RFC of Common Display Framework. I have most of 
the work done, I have just to solve several remaining problems.

Best regards,
-- 
Tomasz Figa
Samsung Poland R&D Center
SW Solution Development, Linux Platform

Hi Tomasz,

On Friday 21 December 2012 11:00:52 Tomasz Figa wrote:

On Tuesday 18 of December 2012 08:31:30 Vikas Sajjan wrote:

quoted

On 17 December 2012 20:55, Laurent Pinchart wrote:

quoted

Hi Vikas,

Sorry for the late reply. I now have more time to work on CDF, so
delays should be much shorter.

On Thursday 06 December 2012 10:51:15 Vikas Sajjan wrote:

quoted

Hi Laurent,

I was thinking of porting CDF to samsung EXYNOS 5250 platform, what
I found is that, the exynos display controller is MIPI DSI based
controller.

But if I look at CDF patches, it has only support for MIPI DBI based
Display controller.

So my question is, do we have any generic framework for MIPI DSI
based display controller? basically I wanted to know, how to go about
porting CDF for such kind of display controller.

MIPI DSI support is not available yet. The only reason for that is
that I don't have any MIPI DSI hardware to write and test the code
with :-)

The common display framework should definitely support MIPI DSI. I
think the existing MIPI DBI code could be used as a base, so the
implementation shouldn't be too high.

Yeah, i was also thinking in similar lines, below is my though for
MIPI DSI support in CDF.

o   MIPI DSI support as part of CDF framework will expose
§  mipi_dsi_register_device(mpi_device) (will be called mach-xxx-dt.c
file )
§  mipi_dsi_register_driver(mipi_driver, bus ops) (will be called from
platform specific init driver call )
·    bus ops will be
o   read data
o   write data
o   write command
§  MIPI DSI will be registered as bus_register()

When MIPI DSI probe is called, it (e.g., Exynos or OMAP MIPI DSI) will
initialize the MIPI DSI HW IP.

This probe will also parse the DT file for MIPI DSI based panel, add
the panel device (device_add() ) to kernel and register the display
entity with its control and  video ops with CDF.

I can give this a try.

I am currently in progress of reworking Exynos MIPI DSIM code and s6e8ax0
LCD driver to use the v2 RFC of Common Display Framework. I have most of
the work done, I have just to solve several remaining problems.

Do you already have code that you can publish ? I'm particularly interested 
(and I think Tomi Valkeinen would be as well) in looking at the DSI operations 
you expose to DSI sinks (panels, transceivers, ...).

-- 
Regards,

Laurent Pinchart

Hi Laurent,

On Monday 24 of December 2012 15:12:28 Laurent Pinchart wrote:

Hi Tomasz,

On Friday 21 December 2012 11:00:52 Tomasz Figa wrote:

quoted

On Tuesday 18 of December 2012 08:31:30 Vikas Sajjan wrote:

quoted

On 17 December 2012 20:55, Laurent Pinchart wrote:

quoted

Hi Vikas,

Sorry for the late reply. I now have more time to work on CDF, so
delays should be much shorter.

On Thursday 06 December 2012 10:51:15 Vikas Sajjan wrote:

quoted

Hi Laurent,

I was thinking of porting CDF to samsung EXYNOS 5250 platform,
what
I found is that, the exynos display controller is MIPI DSI based
controller.

But if I look at CDF patches, it has only support for MIPI DBI
based
Display controller.

So my question is, do we have any generic framework for MIPI DSI
based display controller? basically I wanted to know, how to go
about
porting CDF for such kind of display controller.

MIPI DSI support is not available yet. The only reason for that is
that I don't have any MIPI DSI hardware to write and test the code
with :-)

The common display framework should definitely support MIPI DSI. I
think the existing MIPI DBI code could be used as a base, so the
implementation shouldn't be too high.

Yeah, i was also thinking in similar lines, below is my though for
MIPI DSI support in CDF.

o   MIPI DSI support as part of CDF framework will expose
§  mipi_dsi_register_device(mpi_device) (will be called
mach-xxx-dt.c
file )
§  mipi_dsi_register_driver(mipi_driver, bus ops) (will be called
from
platform specific init driver call )
·    bus ops will be
o   read data
o   write data
o   write command
§  MIPI DSI will be registered as bus_register()

When MIPI DSI probe is called, it (e.g., Exynos or OMAP MIPI DSI)
will
initialize the MIPI DSI HW IP.

This probe will also parse the DT file for MIPI DSI based panel, add
the panel device (device_add() ) to kernel and register the display
entity with its control and  video ops with CDF.

I can give this a try.

I am currently in progress of reworking Exynos MIPI DSIM code and
s6e8ax0 LCD driver to use the v2 RFC of Common Display Framework. I
have most of the work done, I have just to solve several remaining
problems.

Do you already have code that you can publish ? I'm particularly
interested (and I think Tomi Valkeinen would be as well) in looking at
the DSI operations you expose to DSI sinks (panels, transceivers, ...).

Well, I'm afraid this might be little below your expectations, but here's 
an initial RFC of the part defining just the DSI bus. I need a bit more 
time for patches for Exynos MIPI DSI master and s6e8ax0 LCD.

The implementation is very simple and heavily based on your MIPI DBI 
support and existing Exynos MIPI DSIM framework. Provided operation set is 
based on operation set used by Exynos s6e8ax0 LCD driver. Unfortunately 
this is my only source of information about MIPI DSI.

Best regards,
-- 
Tomasz Figa
Samsung Poland R&D Center
SW Solution Development, Linux Platform

From bad07d8bdce0ff76cbc81a9da597c0d01e5244f7 Mon Sep 17 00:00:00 2001
From: Tomasz Figa <redacted>
Date: Thu, 27 Dec 2012 12:36:15 +0100
Subject: [RFC] video: display: Add generic MIPI DSI bus

Signed-off-by: Tomasz Figa <redacted>
---
 drivers/video/display/Kconfig        |   4 +
 drivers/video/display/Makefile       |   1 +
 drivers/video/display/mipi-dsi-bus.c | 214 
+++++++++++++++++++++++++++++++++++
 include/video/display.h              |   1 +
 include/video/mipi-dsi-bus.h         |  98 ++++++++++++++++
 5 files changed, 318 insertions(+)
 create mode 100644 drivers/video/display/mipi-dsi-bus.c
 create mode 100644 include/video/mipi-dsi-bus.h

diff --git a/drivers/video/display/Kconfig b/drivers/video/display/Kconfig
index 13b6aaf..dbaff9d 100644
--- a/drivers/video/display/Kconfig
+++ b/drivers/video/display/Kconfig

@@ -9,6 +9,10 @@ config DISPLAY_MIPI_DBI
 	tristate
 	default n
 
+config DISPLAY_MIPI_DSI
+	tristate
+	default n
+
 config DISPLAY_PANEL_DPI
 	tristate "DPI (Parallel) Display Panels"
 	---help---

diff --git a/drivers/video/display/Makefile 

b/drivers/video/display/Makefile
index 482bec7..429b3ac8 100644

--- a/drivers/video/display/Makefile
+++ b/drivers/video/display/Makefile

@@ -1,5 +1,6 @@
 obj-$(CONFIG_DISPLAY_CORE) += display-core.o
 obj-$(CONFIG_DISPLAY_MIPI_DBI) += mipi-dbi-bus.o
+obj-$(CONFIG_DISPLAY_MIPI_DSI) += mipi-dsi-bus.o
 obj-$(CONFIG_DISPLAY_PANEL_DPI) += panel-dpi.o
 obj-$(CONFIG_DISPLAY_PANEL_R61505) += panel-r61505.o
 obj-$(CONFIG_DISPLAY_PANEL_R61517) += panel-r61517.o

diff --git a/drivers/video/display/mipi-dsi-bus.c 

b/drivers/video/display/mipi-dsi-bus.c
new file mode 100644
index 0000000..2998522

--- /dev/null
+++ b/drivers/video/display/mipi-dsi-bus.c

@@ -0,0 +1,214 @@
+/*
+ * MIPI DSI Bus
+ *
+ * Copyright (C) 2012 Samsung Electronics Co., Ltd.
+ * Contacts: Tomasz Figa <t.figa@samsung.com>
+ *
+ * Heavily based ond mipi-dbi-bus.c.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/device.h>
+#include <linux/export.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
+
+#include <video/mipi-dsi-bus.h>
+
+/* 
-----------------------------------------------------------------------------
+ * Bus operations
+ */
+
+int mipi_dsi_write_command(struct mipi_dsi_device *dev, unsigned int cmd,
+				const unsigned char *buf, unsigned int len)
+{
+	return dev->bus->ops->write_command(dev->bus, dev, cmd, buf, len);
+}
+EXPORT_SYMBOL_GPL(mipi_dsi_write_command);
+
+int mipi_dsi_read_command(struct mipi_dsi_device *dev, unsigned int cmd,
+		unsigned int addr, unsigned char *buf, unsigned int len)
+{
+	return dev->bus->ops->read_command(dev->bus, dev, cmd, addr, buf, 

len);
+}
+EXPORT_SYMBOL_GPL(mipi_dsi_read_command);
+
+/* 
-----------------------------------------------------------------------------
+ * Bus type
+ */
+
+static const struct mipi_dsi_device_id *
+mipi_dsi_match_id(const struct mipi_dsi_device_id *id,
+		  struct mipi_dsi_device *dev)
+{
+	while (id->name[0]) {
+		if (strcmp(dev->name, id->name) = 0) {
+			dev->id_entry = id;
+			return id;
+		}
+		id++;
+	}
+	return NULL;
+}
+
+static int mipi_dsi_match(struct device *_dev, struct device_driver 
*_drv)
+{
+	struct mipi_dsi_device *dev = to_mipi_dsi_device(_dev);
+	struct mipi_dsi_driver *drv = to_mipi_dsi_driver(_drv);
+
+	if (drv->id_table)
+		return mipi_dsi_match_id(drv->id_table, dev) != NULL;
+
+	return (strcmp(dev->name, _drv->name) = 0);
+}
+
+static ssize_t modalias_show(struct device *_dev, struct device_attribute 
*a,
+			     char *buf)
+{
+	struct mipi_dsi_device *dev = to_mipi_dsi_device(_dev);
+	int len = snprintf(buf, PAGE_SIZE, MIPI_DSI_MODULE_PREFIX "%s\n",
+			   dev->name);
+
+	return (len >= PAGE_SIZE) ? (PAGE_SIZE - 1) : len;
+}
+
+static struct device_attribute mipi_dsi_dev_attrs[] = {
+	__ATTR_RO(modalias),
+	__ATTR_NULL,
+};
+
+static int mipi_dsi_uevent(struct device *_dev, struct kobj_uevent_env 
*env)
+{
+	struct mipi_dsi_device *dev = to_mipi_dsi_device(_dev);
+
+	add_uevent_var(env, "MODALIAS=%s%s", MIPI_DSI_MODULE_PREFIX,
+		       dev->name);
+	return 0;
+}
+
+static const struct dev_pm_ops mipi_dsi_dev_pm_ops = {
+	.runtime_suspend = pm_generic_runtime_suspend,
+	.runtime_resume = pm_generic_runtime_resume,
+	.runtime_idle = pm_generic_runtime_idle,
+	.suspend = pm_generic_suspend,
+	.resume = pm_generic_resume,
+	.freeze = pm_generic_freeze,
+	.thaw = pm_generic_thaw,
+	.poweroff = pm_generic_poweroff,
+	.restore = pm_generic_restore,
+};
+
+static struct bus_type mipi_dsi_bus_type = {
+	.name		= "mipi-dsi",
+	.dev_attrs	= mipi_dsi_dev_attrs,
+	.match		= mipi_dsi_match,
+	.uevent		= mipi_dsi_uevent,
+	.pm		= &mipi_dsi_dev_pm_ops,
+};
+
+/* 
-----------------------------------------------------------------------------
+ * Device and driver (un)registration
+ */
+
+/**
+ * mipi_dsi_device_register - register a DSI device
+ * @dev: DSI device we're registering
+ */
+int mipi_dsi_device_register(struct mipi_dsi_device *dev,
+			      struct mipi_dsi_bus *bus)
+{
+	device_initialize(&dev->dev);
+
+	dev->bus = bus;
+	dev->dev.bus = &mipi_dsi_bus_type;
+	dev->dev.parent = bus->dev;
+
+	if (dev->id != -1)
+		dev_set_name(&dev->dev, "%s.%d", dev->name,  dev->id);
+	else
+		dev_set_name(&dev->dev, "%s", dev->name);
+
+	return device_add(&dev->dev);
+}
+EXPORT_SYMBOL_GPL(mipi_dsi_device_register);
+
+/**
+ * mipi_dsi_device_unregister - unregister a DSI device
+ * @dev: DSI device we're unregistering
+ */
+void mipi_dsi_device_unregister(struct mipi_dsi_device *dev)
+{
+	device_del(&dev->dev);
+	put_device(&dev->dev);
+}
+EXPORT_SYMBOL_GPL(mipi_dsi_device_unregister);
+
+static int mipi_dsi_drv_probe(struct device *_dev)
+{
+	struct mipi_dsi_driver *drv = to_mipi_dsi_driver(_dev->driver);
+	struct mipi_dsi_device *dev = to_mipi_dsi_device(_dev);
+
+	return drv->probe(dev);
+}
+
+static int mipi_dsi_drv_remove(struct device *_dev)
+{
+	struct mipi_dsi_driver *drv = to_mipi_dsi_driver(_dev->driver);
+	struct mipi_dsi_device *dev = to_mipi_dsi_device(_dev);
+
+	return drv->remove(dev);
+}
+
+/**
+ * mipi_dsi_driver_register - register a driver for DSI devices
+ * @drv: DSI driver structure
+ */
+int mipi_dsi_driver_register(struct mipi_dsi_driver *drv)
+{
+	drv->driver.bus = &mipi_dsi_bus_type;
+	if (drv->probe)
+		drv->driver.probe = mipi_dsi_drv_probe;
+	if (drv->remove)
+		drv->driver.remove = mipi_dsi_drv_remove;
+
+	return driver_register(&drv->driver);
+}
+EXPORT_SYMBOL_GPL(mipi_dsi_driver_register);
+
+/**
+ * mipi_dsi_driver_unregister - unregister a driver for DSI devices
+ * @drv: DSI driver structure
+ */
+void mipi_dsi_driver_unregister(struct mipi_dsi_driver *drv)
+{
+	driver_unregister(&drv->driver);
+}
+EXPORT_SYMBOL_GPL(mipi_dsi_driver_unregister);
+
+/* 
-----------------------------------------------------------------------------
+ * Init/exit
+ */
+
+static int __init mipi_dsi_init(void)
+{
+	return bus_register(&mipi_dsi_bus_type);
+}
+
+static void __exit mipi_dsi_exit(void)
+{
+	bus_unregister(&mipi_dsi_bus_type);
+}
+
+module_init(mipi_dsi_init);
+module_exit(mipi_dsi_exit)
+
+MODULE_AUTHOR("Tomasz Figa [off-list ref]");
+MODULE_DESCRIPTION("MIPI DSI Bus");
+MODULE_LICENSE("GPL");

diff --git a/include/video/display.h b/include/video/display.h
index 75ba270..f86ea6e 100644
--- a/include/video/display.h
+++ b/include/video/display.h

@@ -70,6 +70,7 @@ enum display_entity_stream_state {
 enum display_entity_interface_type {
 	DISPLAY_ENTITY_INTERFACE_DPI,
 	DISPLAY_ENTITY_INTERFACE_DBI,
+	DISPLAY_ENTITY_INTERFACE_DSI,
 };
 
 struct display_entity_interface_params {

diff --git a/include/video/mipi-dsi-bus.h b/include/video/mipi-dsi-bus.h
new file mode 100644
index 0000000..3efcb39
--- /dev/null
+++ b/include/video/mipi-dsi-bus.h

@@ -0,0 +1,98 @@
+/*
+ * MIPI DSI Bus
+ *
+ * Copyright (C) 2012 Samsung Electronics Co., Ltd.
+ * Contacts: Tomasz Figa <t.figa@samsung.com>
+ *
+ * Heavily based ond mipi-dbi-bus.h.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __MIPI_DSI_BUS_H__
+#define __MIPI_DSI_BUS_H__
+
+#include <linux/device.h>
+
+struct mipi_dsi_bus;
+struct mipi_dsi_device;
+
+struct mipi_dsi_bus_ops {
+	int (*write_command)(struct mipi_dsi_bus *bus,
+				struct mipi_dsi_device *dev, unsigned int cmd,
+				const unsigned char *buf, unsigned int len);
+	int (*read_command)(struct mipi_dsi_bus *bus,
+				struct mipi_dsi_device *dev, unsigned int cmd,
+				unsigned int addr, unsigned char *buf,
+				unsigned int len);
+};
+
+struct mipi_dsi_bus {
+	struct device *dev;
+	const struct mipi_dsi_bus_ops *ops;
+};
+
+#define MIPI_DSI_MODULE_PREFIX		"mipi-dsi:"
+#define MIPI_DSI_NAME_SIZE		32
+
+struct mipi_dsi_device_id {
+	char name[MIPI_DSI_NAME_SIZE];
+	__kernel_ulong_t driver_data	/* Data private to the driver */
+			__aligned(sizeof(__kernel_ulong_t));
+};
+
+struct mipi_dsi_device {
+	const char *name;
+	int id;
+	struct device dev;
+
+	const struct mipi_dsi_device_id *id_entry;
+	struct mipi_dsi_bus *bus;
+};
+
+#define to_mipi_dsi_device(d)	container_of(d, struct mipi_dsi_device, 

dev)
+
+int mipi_dsi_device_register(struct mipi_dsi_device *dev,
+			     struct mipi_dsi_bus *bus);
+void mipi_dsi_device_unregister(struct mipi_dsi_device *dev);
+
+struct mipi_dsi_driver {
+	int(*probe)(struct mipi_dsi_device *);
+	int(*remove)(struct mipi_dsi_device *);
+	struct device_driver driver;
+	const struct mipi_dsi_device_id *id_table;
+};
+
+#define to_mipi_dsi_driver(d)	container_of(d, struct mipi_dsi_driver, 
driver)
+
+int mipi_dsi_driver_register(struct mipi_dsi_driver *drv);
+void mipi_dsi_driver_unregister(struct mipi_dsi_driver *drv);
+
+static inline void *mipi_dsi_get_drvdata(const struct mipi_dsi_device 
*dev)
+{
+	return dev_get_drvdata(&dev->dev);
+}
+
+static inline void mipi_dsi_set_drvdata(struct mipi_dsi_device *dev,
+					void *data)
+{
+	dev_set_drvdata(&dev->dev, data);
+}
+
+/* module_mipi_dsi_driver() - Helper macro for drivers that don't do
+ * anything special in module init/exit.  This eliminates a lot of
+ * boilerplate.  Each module may only use this macro once, and
+ * calling it replaces module_init() and module_exit()
+ */
+#define module_mipi_dsi_driver(__mipi_dsi_driver) \
+	module_driver(__mipi_dsi_driver, mipi_dsi_driver_register, \
+			mipi_dsi_driver_unregister)
+
+int mipi_dsi_write_command(struct mipi_dsi_device *dev, unsigned int cmd,
+				const unsigned char *buf, unsigned int len);
+int mipi_dsi_read_command(struct mipi_dsi_device *dev, unsigned int cmd,
+		unsigned int addr, unsigned char *buf, unsigned int len);
+
+#endif /* __MIPI_DSI_BUS__ */
-- 
1.8.0.2

On 27 December 2012 20:13, Tomasz Figa [off-list ref] wrote:

As per the discussion what Laurent and Tomi had (
http://lists.freedesktop.org/archives/dri-devel/2012-December/032038.html),
both  DSI and DBI doesn't need to be registered as real bus. Please
refer to Tomi's pacthset for DSI support. (
git://gitorious.org/linux-omap-dss2/linux.git work/dss-dev-model-cdf )

anyways i am working on Exynos MIPI DSI as per the new CDF proposed by
Tomi and Laurent.

--
Thanks and Regards
 Vikas Sajjan

Hi Tomasz,

On Thursday 27 December 2012 15:43:34 Tomasz Figa wrote:

On Monday 24 of December 2012 15:12:28 Laurent Pinchart wrote:

quoted

On Friday 21 December 2012 11:00:52 Tomasz Figa wrote:

quoted

On Tuesday 18 of December 2012 08:31:30 Vikas Sajjan wrote:

quoted

On 17 December 2012 20:55, Laurent Pinchart wrote:

quoted

Hi Vikas,

Sorry for the late reply. I now have more time to work on CDF, so
delays should be much shorter.

On Thursday 06 December 2012 10:51:15 Vikas Sajjan wrote:

quoted

Hi Laurent,

I was thinking of porting CDF to samsung EXYNOS 5250 platform,
what I found is that, the exynos display controller is MIPI DSI
based controller.

But if I look at CDF patches, it has only support for MIPI DBI
based Display controller.

So my question is, do we have any generic framework for MIPI DSI
based display controller? basically I wanted to know, how to go
about porting CDF for such kind of display controller.

MIPI DSI support is not available yet. The only reason for that is
that I don't have any MIPI DSI hardware to write and test the code
with :-)

The common display framework should definitely support MIPI DSI. I
think the existing MIPI DBI code could be used as a base, so the
implementation shouldn't be too high.

Yeah, i was also thinking in similar lines, below is my though for
MIPI DSI support in CDF.

o   MIPI DSI support as part of CDF framework will expose
§  mipi_dsi_register_device(mpi_device) (will be called mach-xxx-dt.c
file )
§  mipi_dsi_register_driver(mipi_driver, bus ops) (will be called
from platform specific init driver call )
·    bus ops will be
o   read data
o   write data
o   write command
§  MIPI DSI will be registered as bus_register()

When MIPI DSI probe is called, it (e.g., Exynos or OMAP MIPI DSI)
will initialize the MIPI DSI HW IP.

This probe will also parse the DT file for MIPI DSI based panel, add
the panel device (device_add() ) to kernel and register the display
entity with its control and  video ops with CDF.

I can give this a try.

I am currently in progress of reworking Exynos MIPI DSIM code and
s6e8ax0 LCD driver to use the v2 RFC of Common Display Framework. I
have most of the work done, I have just to solve several remaining
problems.

Do you already have code that you can publish ? I'm particularly
interested (and I think Tomi Valkeinen would be as well) in looking at
the DSI operations you expose to DSI sinks (panels, transceivers, ...).

Well, I'm afraid this might be little below your expectations, but here's
an initial RFC of the part defining just the DSI bus. I need a bit more
time for patches for Exynos MIPI DSI master and s6e8ax0 LCD.

No worries. I was particularly interested in the DSI operations you needed to 
export, they seem pretty simple. Thank you for sharing the code.

The implementation is very simple and heavily based on your MIPI DBI
support and existing Exynos MIPI DSIM framework. Provided operation set is
based on operation set used by Exynos s6e8ax0 LCD driver. Unfortunately
this is my only source of information about MIPI DSI.

-- 
Regards,

Laurent Pinchart

On 01/08/2013 09:18 AM, Laurent Pinchart wrote:

On Thursday 27 December 2012 15:43:34 Tomasz Figa wrote:

quoted

quoted

 On Monday 24 of December 2012 15:12:28 Laurent Pinchart wrote:

quoted

 >  On Friday 21 December 2012 11:00:52 Tomasz Figa wrote:

quoted

 >  >  On Tuesday 18 of December 2012 08:31:30 Vikas Sajjan wrote:

quoted

 >  >  >  On 17 December 2012 20:55, Laurent Pinchart wrote:

quoted

 >  >  >  >  Hi Vikas,
 >  >  >  >  
 >  >  >  >  Sorry for the late reply. I now have more time to work on CDF, so
 >  >  >  >  delays should be much shorter.
 >  >  >  >  
 >  >  >  >  On Thursday 06 December 2012 10:51:15 Vikas Sajjan wrote:

quoted

 >  >  >  >  >  Hi Laurent,
 >  >  >  >  >  
 >  >  >  >  >  I was thinking of porting CDF to samsung EXYNOS 5250 platform,
 >  >  >  >  >  what I found is that, the exynos display controller is MIPI DSI
 >  >  >  >  >  based controller.
 >  >  >  >  >  
 >  >  >  >  >  But if I look at CDF patches, it has only support for MIPI DBI
 >  >  >  >  >  based Display controller.
 >  >  >  >  >  
 >  >  >  >  >  So my question is, do we have any generic framework for MIPI DSI
 >  >  >  >  >  based display controller? basically I wanted to know, how to go
 >  >  >  >  >  about porting CDF for such kind of display controller.

 >  >  >  >  
 >  >  >  >  MIPI DSI support is not available yet. The only reason for that is
 >  >  >  >  that I don't have any MIPI DSI hardware to write and test the code
 >  >  >  >  with:-)
 >  >  >  >  
 >  >  >  >  The common display framework should definitely support MIPI DSI. I
 >  >  >  >  think the existing MIPI DBI code could be used as a base, so the
 >  >  >  >  implementation shouldn't be too high.
 >  >  >  >  
 >  >  >  >  Yeah, i was also thinking in similar lines, below is my though for
 >  >  >  >  MIPI DSI support in CDF.

 >  >  >  
 >  >  >  o   MIPI DSI support as part of CDF framework will expose
 >  >  >  §  mipi_dsi_register_device(mpi_device) (will be called mach-xxx-dt.c
 >  >  >  file )
 >  >  >  §  mipi_dsi_register_driver(mipi_driver, bus ops) (will be called
 >  >  >  from platform specific init driver call )
 >  >  >  ·    bus ops will be
 >  >  >  o   read data
 >  >  >  o   write data
 >  >  >  o   write command
 >  >  >  §  MIPI DSI will be registered as bus_register()
 >  >  >  
 >  >  >  When MIPI DSI probe is called, it (e.g., Exynos or OMAP MIPI DSI)
 >  >  >  will initialize the MIPI DSI HW IP.
 >  >  >  
 >  >  >  This probe will also parse the DT file for MIPI DSI based panel, add
 >  >  >  the panel device (device_add() ) to kernel and register the display
 >  >  >  entity with its control and  video ops with CDF.
 >  >  >  
 >  >  >  I can give this a try.

 >  >  
 >  >  I am currently in progress of reworking Exynos MIPI DSIM code and
 >  >  s6e8ax0 LCD driver to use the v2 RFC of Common Display Framework. I
 >  >  have most of the work done, I have just to solve several remaining
 >  >  problems.

 >  
 >  Do you already have code that you can publish ? I'm particularly
 >  interested (and I think Tomi Valkeinen would be as well) in looking at
 >  the DSI operations you expose to DSI sinks (panels, transceivers, ...).

 
 Well, I'm afraid this might be little below your expectations, but here's
 an initial RFC of the part defining just the DSI bus. I need a bit more
 time for patches for Exynos MIPI DSI master and s6e8ax0 LCD.

No worries. I was particularly interested in the DSI operations you needed to
export, they seem pretty simple. Thank you for sharing the code.

FYI,
here is STE "DSI API":
http://www.igloocommunity.org/gitweb/?p=kernel/igloo-kernel.git;a=blob;f=include/video/mcde.h;hI9ce5cfecc9ad77593e761cdcc1624502f28432;hb=HEAD#l361

But it is not perfect. After a couple of products we realized that most 
panel drivers want an easy way to send a bunch of init commands in one 
go. So I think it should be an op for sending an array of commands at 
once. Something like

struct dsi_cmd {
     enum mipi_pkt_type type; /* MIPI DSI, DCS, SetPacketLen, ... */
     u8 cmd;
     int dataLen;
     u8 *data;
}
struct dsi_ops {
     int dsi_write(source, int num_cmds, struct dsi_cmd *cmds);
     ...
}

The rest of "DSI write API" could be made helpers on top of this one op. 
This grouping also allows driver to describe intent to send a bunch of 
commands together which might be of interest with mode set (if you need 
to synchronize a bunch of commands with a mode set, like setting smart 
panel rotation in synch with new framebuffer in dsi video mode).

I also looked at the video source in Tomi's git tree 
(http://gitorious.org/linux-omap-dss2/linux/blobs/work/dss-dev-model-cdf/include/video/display.h). 
I think I would prefer a single "setup" op taking a "struct dsi_config" 
as argument. Then each DSI formatter/encoder driver could decide best 
way to set that up. We have something similar at 
http://www.igloocommunity.org/gitweb/?p=kernel/igloo-kernel.git;a=blob;f=include/video/mcde.h;hI9ce5cfecc9ad77593e761cdcc1624502f28432;hb=HEAD#l118

And I think I still prefer the dsi_bus in favor of the abstract video 
source. It just looks like a home made bus with bus-ops ... can't you do 
something similar using the normal driver framework? enable/disable 
looks like suspend/resume, register/unregister_vid_src is like 
bus_(un)register_device, ... the video source anyway seems unattached to 
the panel stuff with the find_video_source call.

/BR
/Marcus

On Tuesday 08 of January 2013 11:12:26 Marcus Lorentzon wrote:

On 01/08/2013 09:18 AM, Laurent Pinchart wrote:

quoted

On Thursday 27 December 2012 15:43:34 Tomasz Figa wrote:

quoted

quoted

 On Monday 24 of December 2012 15:12:28 Laurent Pinchart wrote:

quoted

 >  On Friday 21 December 2012 11:00:52 Tomasz Figa wrote:

quoted

 >  >  On Tuesday 18 of December 2012 08:31:30 Vikas Sajjan 

wrote:

quoted

quoted

quoted

quoted

quoted

quoted

 >  >  >  On 17 December 2012 20:55, Laurent Pinchart wrote:

quoted

 >  >  >  >  Hi Vikas,
 >  >  >  >  
 >  >  >  >  Sorry for the late reply. I now have more time to
 >  >  >  >  work on CDF, so
 >  >  >  >  delays should be much shorter.
 >  >  >  >  
 >  >  >  >  On Thursday 06 December 2012 10:51:15 Vikas Sajjan 

wrote:

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

 >  >  >  >  >  Hi Laurent,
 >  >  >  >  >  
 >  >  >  >  >  I was thinking of porting CDF to samsung
 >  >  >  >  >  EXYNOS 5250 platform,
 >  >  >  >  >  what I found is that, the exynos display
 >  >  >  >  >  controller is MIPI DSI
 >  >  >  >  >  based controller.
 >  >  >  >  >  
 >  >  >  >  >  But if I look at CDF patches, it has only
 >  >  >  >  >  support for MIPI DBI
 >  >  >  >  >  based Display controller.
 >  >  >  >  >  
 >  >  >  >  >  So my question is, do we have any generic
 >  >  >  >  >  framework for MIPI DSI
 >  >  >  >  >  based display controller? basically I wanted
 >  >  >  >  >  to know, how to go
 >  >  >  >  >  about porting CDF for such kind of display
 >  >  >  >  >  controller.

 >  >  >  >  
 >  >  >  >  MIPI DSI support is not available yet. The only
 >  >  >  >  reason for that is
 >  >  >  >  that I don't have any MIPI DSI hardware to write
 >  >  >  >  and test the code
 >  >  >  >  with:-)
 >  >  >  >  
 >  >  >  >  The common display framework should definitely
 >  >  >  >  support MIPI DSI. I
 >  >  >  >  think the existing MIPI DBI code could be used as
 >  >  >  >  a base, so the
 >  >  >  >  implementation shouldn't be too high.
 >  >  >  >  
 >  >  >  >  Yeah, i was also thinking in similar lines, below
 >  >  >  >  is my though for
 >  >  >  >  MIPI DSI support in CDF.

 >  >  >  
 >  >  >  o   MIPI DSI support as part of CDF framework will
 >  >  >  expose
 >  >  >  §  mipi_dsi_register_device(mpi_device) (will be
 >  >  >  called mach-xxx-dt.c
 >  >  >  file )
 >  >  >  §  mipi_dsi_register_driver(mipi_driver, bus ops)
 >  >  >  (will be called
 >  >  >  from platform specific init driver call )
 >  >  >  ·    bus ops will be
 >  >  >  o   read data
 >  >  >  o   write data
 >  >  >  o   write command
 >  >  >  §  MIPI DSI will be registered as bus_register()
 >  >  >  
 >  >  >  When MIPI DSI probe is called, it (e.g., Exynos or
 >  >  >  OMAP MIPI DSI)
 >  >  >  will initialize the MIPI DSI HW IP.
 >  >  >  
 >  >  >  This probe will also parse the DT file for MIPI DSI
 >  >  >  based panel, add
 >  >  >  the panel device (device_add() ) to kernel and
 >  >  >  register the display
 >  >  >  entity with its control and  video ops with CDF.
 >  >  >  
 >  >  >  I can give this a try.

 >  >  
 >  >  I am currently in progress of reworking Exynos MIPI DSIM
 >  >  code and
 >  >  s6e8ax0 LCD driver to use the v2 RFC of Common Display
 >  >  Framework. I
 >  >  have most of the work done, I have just to solve several
 >  >  remaining
 >  >  problems.

 >  
 >  Do you already have code that you can publish ? I'm
 >  particularly
 >  interested (and I think Tomi Valkeinen would be as well) in
 >  looking at
 >  the DSI operations you expose to DSI sinks (panels,
 >  transceivers, ...).

 
 Well, I'm afraid this might be little below your expectations, but
 here's an initial RFC of the part defining just the DSI bus. I
 need a bit more time for patches for Exynos MIPI DSI master and
 s6e8ax0 LCD.

No worries. I was particularly interested in the DSI operations you
needed to export, they seem pretty simple. Thank you for sharing the
code.

FYI,
here is STE "DSI API":
http://www.igloocommunity.org/gitweb/?p=kernel/igloo-kernel.git;a=blob;f
=include/video/mcde.h;hI9ce5cfecc9ad77593e761cdcc1624502f28432;hb=HEAD
#l361

But it is not perfect. After a couple of products we realized that most
panel drivers want an easy way to send a bunch of init commands in one
go. So I think it should be an op for sending an array of commands at
once. Something like

struct dsi_cmd {
     enum mipi_pkt_type type; /* MIPI DSI, DCS, SetPacketLen, ... */
     u8 cmd;
     int dataLen;
     u8 *data;
}
struct dsi_ops {
     int dsi_write(source, int num_cmds, struct dsi_cmd *cmds);
     ...
}

Yes, this should be flexible enough to cover most of (or even whole) DSI 
specification.

However I'm not sure whether the dsi_write name would be appropriate, 
since DSI packet types include also read and special transactions. So, 
according to DSI terminology, maybe dsi_transaction would be better?

The rest of "DSI write API" could be made helpers on top of this one op.
This grouping also allows driver to describe intent to send a bunch of
commands together which might be of interest with mode set (if you need
to synchronize a bunch of commands with a mode set, like setting smart
panel rotation in synch with new framebuffer in dsi video mode).

I also looked at the video source in Tomi's git tree
(http://gitorious.org/linux-omap-dss2/linux/blobs/work/dss-dev-model-cdf
/include/video/display.h). I think I would prefer a single "setup" op
taking a "struct dsi_config" as argument. Then each DSI
formatter/encoder driver could decide best way to set that up. We have
something similar at
http://www.igloocommunity.org/gitweb/?p=kernel/igloo-kernel.git;a=blob;f
=include/video/mcde.h;hI9ce5cfecc9ad77593e761cdcc1624502f28432;hb=HEAD
#l118

Yes, this would be definitely better, because such configuration changes 
usually come together (i.e. display connected, which needs complete 
reconfiguration of all parameters).

And I think I still prefer the dsi_bus in favor of the abstract video
source. It just looks like a home made bus with bus-ops ... can't you do
something similar using the normal driver framework? enable/disable
looks like suspend/resume, register/unregister_vid_src is like
bus_(un)register_device, ... the video source anyway seems unattached
to the panel stuff with the find_video_source call.

DSI needs specific power management. It's necessary to power up the panel 
first to make it wait for Tinit event and then enable DSI master to 
trigger such event. Only then rest of panel initialization can be 
completed.

Also, as discussed in previous posts, some panels might use DSI only for 
video data and another interface (I2C, SPI) for control data. In such case 
it would be impossible to represent such device in a reasonable way using 
current driver model.

Best regards,
-- 
Tomasz Figa
Samsung Poland R&D Center
SW Solution Development, Linux Platform

On 01/08/2013 05:36 PM, Tomasz Figa wrote:

On Tuesday 08 of January 2013 11:12:26 Marcus Lorentzon wrote:

quoted

On 01/08/2013 09:18 AM, Laurent Pinchart wrote:

quoted

On Thursday 27 December 2012 15:43:34 Tomasz Figa wrote:

quoted

quoted

  On Monday 24 of December 2012 15:12:28 Laurent Pinchart wrote:

quoted

  >   On Friday 21 December 2012 11:00:52 Tomasz Figa wrote:

quoted

  >   >   On Tuesday 18 of December 2012 08:31:30 Vikas Sajjan

wrote:

quoted

quoted

quoted

quoted

quoted

quoted

quoted

  >   >   >   On 17 December 2012 20:55, Laurent Pinchart wrote:

quoted

  >   >   >   >   Hi Vikas,
  >   >   >   >
  >   >   >   >   Sorry for the late reply. I now have more time to
  >   >   >   >   work on CDF, so
  >   >   >   >   delays should be much shorter.
  >   >   >   >
  >   >   >   >   On Thursday 06 December 2012 10:51:15 Vikas Sajjan

wrote:

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

  >   >   >   >   >   Hi Laurent,
  >   >   >   >   >
  >   >   >   >   >   I was thinking of porting CDF to samsung
  >   >   >   >   >   EXYNOS 5250 platform,
  >   >   >   >   >   what I found is that, the exynos display
  >   >   >   >   >   controller is MIPI DSI
  >   >   >   >   >   based controller.
  >   >   >   >   >
  >   >   >   >   >   But if I look at CDF patches, it has only
  >   >   >   >   >   support for MIPI DBI
  >   >   >   >   >   based Display controller.
  >   >   >   >   >
  >   >   >   >   >   So my question is, do we have any generic
  >   >   >   >   >   framework for MIPI DSI
  >   >   >   >   >   based display controller? basically I wanted
  >   >   >   >   >   to know, how to go
  >   >   >   >   >   about porting CDF for such kind of display
  >   >   >   >   >   controller.

  >   >   >   >
  >   >   >   >   MIPI DSI support is not available yet. The only
  >   >   >   >   reason for that is
  >   >   >   >   that I don't have any MIPI DSI hardware to write
  >   >   >   >   and test the code
  >   >   >   >   with:-)
  >   >   >   >
  >   >   >   >   The common display framework should definitely
  >   >   >   >   support MIPI DSI. I
  >   >   >   >   think the existing MIPI DBI code could be used as
  >   >   >   >   a base, so the
  >   >   >   >   implementation shouldn't be too high.
  >   >   >   >
  >   >   >   >   Yeah, i was also thinking in similar lines, below
  >   >   >   >   is my though for
  >   >   >   >   MIPI DSI support in CDF.

  >   >   >
  >   >   >   o   MIPI DSI support as part of CDF framework will
  >   >   >   expose
  >   >   >   §  mipi_dsi_register_device(mpi_device) (will be
  >   >   >   called mach-xxx-dt.c
  >   >   >   file )
  >   >   >   §  mipi_dsi_register_driver(mipi_driver, bus ops)
  >   >   >   (will be called
  >   >   >   from platform specific init driver call )
  >   >   >   ·    bus ops will be
  >   >   >   o   read data
  >   >   >   o   write data
  >   >   >   o   write command
  >   >   >   §  MIPI DSI will be registered as bus_register()
  >   >   >
  >   >   >   When MIPI DSI probe is called, it (e.g., Exynos or
  >   >   >   OMAP MIPI DSI)
  >   >   >   will initialize the MIPI DSI HW IP.
  >   >   >
  >   >   >   This probe will also parse the DT file for MIPI DSI
  >   >   >   based panel, add
  >   >   >   the panel device (device_add() ) to kernel and
  >   >   >   register the display
  >   >   >   entity with its control and  video ops with CDF.
  >   >   >
  >   >   >   I can give this a try.

  >   >
  >   >   I am currently in progress of reworking Exynos MIPI DSIM
  >   >   code and
  >   >   s6e8ax0 LCD driver to use the v2 RFC of Common Display
  >   >   Framework. I
  >   >   have most of the work done, I have just to solve several
  >   >   remaining
  >   >   problems.

  >
  >   Do you already have code that you can publish ? I'm
  >   particularly
  >   interested (and I think Tomi Valkeinen would be as well) in
  >   looking at
  >   the DSI operations you expose to DSI sinks (panels,
  >   transceivers, ...).

  Well, I'm afraid this might be little below your expectations, but
  here's an initial RFC of the part defining just the DSI bus. I
  need a bit more time for patches for Exynos MIPI DSI master and
  s6e8ax0 LCD.

No worries. I was particularly interested in the DSI operations you
needed to export, they seem pretty simple. Thank you for sharing the
code.

FYI,
here is STE "DSI API":
http://www.igloocommunity.org/gitweb/?p=kernel/igloo-kernel.git;a=blob;f
=include/video/mcde.h;hI9ce5cfecc9ad77593e761cdcc1624502f28432;hb=HEAD
#l361

But it is not perfect. After a couple of products we realized that most
panel drivers want an easy way to send a bunch of init commands in one
go. So I think it should be an op for sending an array of commands at
once. Something like

struct dsi_cmd {
      enum mipi_pkt_type type; /* MIPI DSI, DCS, SetPacketLen, ... */
      u8 cmd;
      int dataLen;
      u8 *data;
}
struct dsi_ops {
      int dsi_write(source, int num_cmds, struct dsi_cmd *cmds);
      ...
}

Yes, this should be flexible enough to cover most of (or even whole) DSI
specification.

However I'm not sure whether the dsi_write name would be appropriate,
since DSI packet types include also read and special transactions. So,
according to DSI terminology, maybe dsi_transaction would be better?

I think read should still be separate. At least on my HW read and write 
are quite different. But all "transactions" are very much the same in HW 
setup. The ... was dsi_write etc ;) Like set_max_packet_size should 
maybe be an ops. Since only the implementer of the "video source" will 
know what the max read return packet size for that DSI IP is. The panels 
don't know that. Maybe another ops to retrieve some info about the caps 
of the video source would help that. Then a helper could call that and 
then the dsi_write one.

quoted

And I think I still prefer the dsi_bus in favor of the abstract video
source. It just looks like a home made bus with bus-ops ... can't you do
something similar using the normal driver framework? enable/disable
looks like suspend/resume, register/unregister_vid_src is like
bus_(un)register_device, ... the video source anyway seems unattached
to the panel stuff with the find_video_source call.

DSI needs specific power management. It's necessary to power up the panel
first to make it wait for Tinit event and then enable DSI master to
trigger such event. Only then rest of panel initialization can be
completed.

I know, we have a very complex sequence for our HDMI encoder which uses 
sort of continuous DSI cmmand mode. And power/clock on sequences are 
tricky to get right in our current "CDF" API (mcde_display). But I fail 
to see how the current video source API is different from just using the 
bus/device APIs.

Also, as discussed in previous posts, some panels might use DSI only for
video data and another interface (I2C, SPI) for control data. In such case
it would be impossible to represent such device in a reasonable way using
current driver model.

I understand that you need to get hold of both the control and data bus 
device in the driver. (Toshiba DSI-LVDS bridge is a good example and 
commonly used "encoder" that can use both DSI and I2C control 
interface.) But the control bus you get from device probe, and I guess 
you could call bus_find_device_by_name(dsi_bus, "mydev") and return the 
"datadev" which will have access to dsi bus ops just as you call 
find_video_source("mysource") to access the "databus" ops directly with 
a logical device (display entity).
I'm not saying I would refuse to use video sources. I just think the two 
models are so similar so it would be worth exploring how a device model 
style API would look like and to compare against.

/BR
/Marcus

Hi Marcus,

On Tuesday 08 January 2013 18:08:19 Marcus Lorentzon wrote:

On 01/08/2013 05:36 PM, Tomasz Figa wrote:

quoted

On Tuesday 08 of January 2013 11:12:26 Marcus Lorentzon wrote:

[snip]

quoted

quoted

FYI,
here is STE "DSI API":
http://www.igloocommunity.org/gitweb/?p=kernel/igloo-kernel.git;a=blob;f
=include/video/mcde.h;hI9ce5cfecc9ad77593e761cdcc1624502f28432;hb=HEAD
#l361

Thank you.

quoted

quoted

But it is not perfect. After a couple of products we realized that most
panel drivers want an easy way to send a bunch of init commands in one
go. So I think it should be an op for sending an array of commands at
once. Something like

struct dsi_cmd {
      enum mipi_pkt_type type; /* MIPI DSI, DCS, SetPacketLen, ... */
      u8 cmd;
      int dataLen;
      u8 *data;
}

struct dsi_ops {
      int dsi_write(source, int num_cmds, struct dsi_cmd *cmds);
      ...
}

Do you have DSI IP(s) that can handle a list of commands ? Or would all DSI 
transmitter drivers need to iterate over the commands manually ? In the later 
case a lower-level API might be easier to implement in DSI transmitter 
drivers. Helper functions could provide the higher-level API you proposed.

quoted

Yes, this should be flexible enough to cover most of (or even whole) DSI
specification.

However I'm not sure whether the dsi_write name would be appropriate,
since DSI packet types include also read and special transactions. So,
according to DSI terminology, maybe dsi_transaction would be better?

Or dsi_transfer or dsi_xfer ? Does the DSI bus have a concept of transactions 
?

I think read should still be separate. At least on my HW read and write
are quite different. But all "transactions" are very much the same in HW
setup. The ... was dsi_write etc ;) Like set_max_packet_size should
maybe be an ops. Since only the implementer of the "video source" will
know what the max read return packet size for that DSI IP is. The panels
don't know that. Maybe another ops to retrieve some info about the caps
of the video source would help that. Then a helper could call that and
then the dsi_write one.

If panels (or helper functions) need information about the DSI transmitter 
capabilities, sure, we can add an op.

quoted

quoted

And I think I still prefer the dsi_bus in favor of the abstract video
source. It just looks like a home made bus with bus-ops ... can't you do
something similar using the normal driver framework? enable/disable
looks like suspend/resume, register/unregister_vid_src is like
bus_(un)register_device, ... the video source anyway seems unattached
to the panel stuff with the find_video_source call.

The Linux driver framework is based on control busses. DSI usually handles 
both control and video transfer, but the control and data busses can also be 
separate (think DPI + SPI/I2C for instance). In that case the panel will be a 
child of its control bus master, and will need a separate interface to access 
video data operations. As a separate video interface is thus needed, I think 
we should use it for DSI as well.

My initial proposal included a DBI bus (as I don't have any DSI hardware - DBI 
and DSI can be used interchangeably in this discussions, they both share the 
caracteristic of having a common control + data bus), and panels were children 
of the DBI bus master. The DBI bus API was only used for control, not for data 
transfers. Tomi then removed the DBI bus and moved the control operations to 
the video source, turning the DBI panels into platform devices. I still favor 
my initial approach, but I can agree to drop the DBI bus if there's a 
consensus on that. Video bus operations will be separate in any case.

quoted

DSI needs specific power management. It's necessary to power up the panel
first to make it wait for Tinit event and then enable DSI master to
trigger such event. Only then rest of panel initialization can be
completed.

I know, we have a very complex sequence for our HDMI encoder which uses
sort of continuous DSI cmmand mode. And power/clock on sequences are
tricky to get right in our current "CDF" API (mcde_display). But I fail
to see how the current video source API is different from just using the
bus/device APIs.

As mentioned above, the video source API handles video transfers, while the 
bus/device API handles control transfers. Operations such as "start the video 
stream" will thus be video source APIs. Operations such as "enable the DSI 
master", used to trigger the Tinit event (whatever that is :-)) at power up 
time would probably be DSI bus operations.

quoted

Also, as discussed in previous posts, some panels might use DSI only for
video data and another interface (I2C, SPI) for control data. In such case
it would be impossible to represent such device in a reasonable way using
current driver model.

I understand that you need to get hold of both the control and data bus
device in the driver. (Toshiba DSI-LVDS bridge is a good example and
commonly used "encoder" that can use both DSI and I2C control interface.)
But the control bus you get from device probe, and I guess you could call
bus_find_device_by_name(dsi_bus, "mydev") and return the "datadev" which
will have access to dsi bus ops just as you call
find_video_source("mysource") to access the "databus" ops directly with
a logical device (display entity).
I'm not saying I would refuse to use video sources. I just think the two
models are so similar so it would be worth exploring how a device model
style API would look like and to compare against.

I don't think we should use the Linux device model for data busses. It hasn't 
been designed for that use case, and definitely doesn't support devices that 
would be children of two separate masters (control and data). For shared bus 
devices such as DSI, having a DSI bus was my preference to start with, as 
mentioned above :-) However, even in that case, I think it would still make 
sense to use video sources to control the video operations. As usual the devil 
is in the details, so there will probably be some tricky problems we'll need 
to solve, but that will require coding the proposed solution.

-- 
Regards,

Laurent Pinchart

Hi Marcus,

On Tuesday 08 January 2013 11:12:26 Marcus Lorentzon wrote:

[snip]

I also looked at the video source in Tomi's git tree
(http://gitorious.org/linux-omap-dss2/linux/blobs/work/dss-dev-model-cdf/inc
lude/video/display.h). I think I would prefer a single "setup" op taking a
"struct dsi_config" as argument. Then each DSI formatter/encoder driver
could decide best way to set that up. We have something similar at
http://www.igloocommunity.org/gitweb/?p=kernel/igloo-kernel.git;a=blob;f=inc
lude/video/mcde.h;hI9ce5cfecc9ad77593e761cdcc1624502f28432;hb=HEAD#l118

A single setup function indeed seems easier, but I don't have enough 
experience with DSI to have a strong opinion on that. We'll have to compare 
implementations if there's a disagreement on this.

-- 
Regards,

Laurent Pinchart

On 02/02/2013 12:35 AM, Laurent Pinchart wrote:

Hi Marcus,

On Tuesday 08 January 2013 18:08:19 Marcus Lorentzon wrote:

quoted

On 01/08/2013 05:36 PM, Tomasz Figa wrote:

quoted

On Tuesday 08 of January 2013 11:12:26 Marcus Lorentzon wrote:

[...]

quoted

quoted

quoted

But it is not perfect. After a couple of products we realized that most
panel drivers want an easy way to send a bunch of init commands in one
go. So I think it should be an op for sending an array of commands at
once. Something like

struct dsi_cmd {
       enum mipi_pkt_type type; /* MIPI DSI, DCS, SetPacketLen, ... */
       u8 cmd;
       int dataLen;
       u8 *data;
}

struct dsi_ops {
       int dsi_write(source, int num_cmds, struct dsi_cmd *cmds);
       ...
}

Do you have DSI IP(s) that can handle a list of commands ? Or would all DSI
transmitter drivers need to iterate over the commands manually ? In the later
case a lower-level API might be easier to implement in DSI transmitter
drivers. Helper functions could provide the higher-level API you proposed.

The HW has a FIFO, so it can handle a few. Currently we use the low 
level type of call with one call per command. But we have found DSI 
command mode panels that don't accept any commands during the "update" 
(write start+continues). And so we must use a mutex/state machine to 
exclude any async calls to send DSI commands during update. But if you 
need to send more than one command per frame this will be hard (like 
CABC and backlight commands). It will be a ping pong between update and 
command calls. One option is to expose the mutex to the caller so it can 
make many calls before the next update grabs the mutex again.
So maybe we could create a helper that handle the op for list of 
commands and another op for single command that you actually have to 
implement.

quoted

quoted

Yes, this should be flexible enough to cover most of (or even whole) DSI
specification.

However I'm not sure whether the dsi_write name would be appropriate,
since DSI packet types include also read and special transactions. So,
according to DSI terminology, maybe dsi_transaction would be better?

Or dsi_transfer or dsi_xfer ? Does the DSI bus have a concept of transactions
?

No transactions. And I don't want to mix reads and writes. It should be 
similar to I2C and other stream control busses. So one read and one 
write should be fine. And then a bunch of helpers on top for callers to 
use, like one per major DSI packet type.

quoted

I think read should still be separate. At least on my HW read and write
are quite different. But all "transactions" are very much the same in HW
setup. The ... was dsi_write etc ;) Like set_max_packet_size should
maybe be an ops. Since only the implementer of the "video source" will
know what the max read return packet size for that DSI IP is. The panels
don't know that. Maybe another ops to retrieve some info about the caps
of the video source would help that. Then a helper could call that and
then the dsi_write one.

If panels (or helper functions) need information about the DSI transmitter
capabilities, sure, we can add an op.

Yes, a "video source" op for getting caps would be ok too. But so far 
the only limits I have found is the read/write sizes. But if anyone else 
has other limits, please list them so we could add them to this struct 
dsi_host_caps.

quoted

quoted

quoted

And I think I still prefer the dsi_bus in favor of the abstract video
source. It just looks like a home made bus with bus-ops ... can't you do
something similar using the normal driver framework? enable/disable
looks like suspend/resume, register/unregister_vid_src is like
bus_(un)register_device, ... the video source anyway seems unattached
to the panel stuff with the find_video_source call.

The Linux driver framework is based on control busses. DSI usually handles
both control and video transfer, but the control and data busses can also be
separate (think DPI + SPI/I2C for instance). In that case the panel will be a
child of its control bus master, and will need a separate interface to access
video data operations. As a separate video interface is thus needed, I think
we should use it for DSI as well.

My initial proposal included a DBI bus (as I don't have any DSI hardware - DBI
and DSI can be used interchangeably in this discussions, they both share the
caracteristic of having a common control + data bus), and panels were children
of the DBI bus master. The DBI bus API was only used for control, not for data
transfers. Tomi then removed the DBI bus and moved the control operations to
the video source, turning the DBI panels into platform devices. I still favor
my initial approach, but I can agree to drop the DBI bus if there's a
consensus on that. Video bus operations will be separate in any case.

As discussed at FOSDEM I will give DSI bus with full feature set a try.

BTW. Who got the action to ask Greg about devices with multiple 
parents/buses?

quoted

quoted

Also, as discussed in previous posts, some panels might use DSI only for
video data and another interface (I2C, SPI) for control data. In such case
it would be impossible to represent such device in a reasonable way using
current driver model.

I understand that you need to get hold of both the control and data bus
device in the driver. (Toshiba DSI-LVDS bridge is a good example and
commonly used "encoder" that can use both DSI and I2C control interface.)
But the control bus you get from device probe, and I guess you could call
bus_find_device_by_name(dsi_bus, "mydev") and return the "datadev" which
will have access to dsi bus ops just as you call
find_video_source("mysource") to access the "databus" ops directly with
a logical device (display entity).
I'm not saying I would refuse to use video sources. I just think the two
models are so similar so it would be worth exploring how a device model
style API would look like and to compare against.

I don't think we should use the Linux device model for data busses. It hasn't
been designed for that use case, and definitely doesn't support devices that
would be children of two separate masters (control and data). For shared bus
devices such as DSI, having a DSI bus was my preference to start with, as
mentioned above :-) However, even in that case, I think it would still make
sense to use video sources to control the video operations. As usual the devil
is in the details, so there will probably be some tricky problems we'll need
to solve, but that will require coding the proposed solution.

I will give it a try after asking Greg for guidelines.

/BR
/Marcus

On Monday 04 February 2013 03:35 PM, Marcus Lorentzon wrote:

On 02/02/2013 12:35 AM, Laurent Pinchart wrote:

quoted

Hi Marcus,

On Tuesday 08 January 2013 18:08:19 Marcus Lorentzon wrote:

quoted

On 01/08/2013 05:36 PM, Tomasz Figa wrote:

quoted

On Tuesday 08 of January 2013 11:12:26 Marcus Lorentzon wrote:

[...]

quoted

quoted

quoted

quoted

But it is not perfect. After a couple of products we realized that
most
panel drivers want an easy way to send a bunch of init commands in one
go. So I think it should be an op for sending an array of commands at
once. Something like

struct dsi_cmd {
       enum mipi_pkt_type type; /* MIPI DSI, DCS, SetPacketLen, ... */
       u8 cmd;
       int dataLen;
       u8 *data;
}

struct dsi_ops {
       int dsi_write(source, int num_cmds, struct dsi_cmd *cmds);
       ...
}

Do you have DSI IP(s) that can handle a list of commands ? Or would
all DSI
transmitter drivers need to iterate over the commands manually ? In
the later
case a lower-level API might be easier to implement in DSI transmitter
drivers. Helper functions could provide the higher-level API you
proposed.

The HW has a FIFO, so it can handle a few. Currently we use the low
level type of call with one call per command. But we have found DSI
command mode panels that don't accept any commands during the "update"
(write start+continues). And so we must use a mutex/state machine to
exclude any async calls to send DSI commands during update. But if you
need to send more than one command per frame this will be hard (like
CABC and backlight commands). It will be a ping pong between update and
command calls. One option is to expose the mutex to the caller so it can
make many calls before the next update grabs the mutex again.
So maybe we could create a helper that handle the op for list of
commands and another op for single command that you actually have to
implement.

fyi, the DSI IP on OMAP3+ SoCs also has a FIFO. It can provide 
interrupts after each command is pushed out, and also when the FIFO gets 
empty(all commands are pushed). The only thing to take care is to not 
overflow FIFO.

DSI video mode panels generally have a few dozen internal registers 
which need to be configured via DSI commands. It's more fast(and 
convenient) to configure a handful of internal registers in one shot, 
and then perform a single BTA to know from the panel whether the 
commands were received correctly.

Regards,
Archit

On 2013-02-04 12:05, Marcus Lorentzon wrote:

As discussed at FOSDEM I will give DSI bus with full feature set a
try.

Please do, but as a reminder I want to raise the issues I see with a DSI
bus:

- A device can be a child of only one bus. So if DSI is used only for
video, the device is a child of, say, i2c bus, and thus there's no DSI
bus. How to configure and use DSI in this case?

- If DSI is used for both control and video, we have two separate APIs
for the bus. What I mean here is that for the video-only case above, we
need a video-only-API for DSI. This API should contain all necessary
methods to configure DSI. But we need similar methods for the control
API also.

So, I hope you come up with some solution for this, but as I see it,
it's easily the most simple and clear option to have one video_source
style entity for the DSI bus itself, which is used for both control and
video.

 Tomi

On 02/08/2013 11:51 AM, Tomi Valkeinen wrote:

On 2013-02-04 12:05, Marcus Lorentzon wrote:

quoted

As discussed at FOSDEM I will give DSI bus with full feature set a
try.

Please do, but as a reminder I want to raise the issues I see with a DSI
bus:

- A device can be a child of only one bus. So if DSI is used only for
video, the device is a child of, say, i2c bus, and thus there's no DSI
bus. How to configure and use DSI in this case?

- If DSI is used for both control and video, we have two separate APIs
for the bus. What I mean here is that for the video-only case above, we
need a video-only-API for DSI. This API should contain all necessary
methods to configure DSI. But we need similar methods for the control
API also.

So, I hope you come up with some solution for this, but as I see it,
it's easily the most simple and clear option to have one video_source
style entity for the DSI bus itself, which is used for both control and
video.

Thanks, it is not that I'm totally against the video source stuff. And I 
share your concerns, none of the solutions are perfect. It just doesn't 
feel right to create this dummy source "device" without investigating 
the DSI bus route. But I will try to write up some history/problem 
description and ask Greg KH for guidance. If he has a strong opinion 
either way, there is not much more to discuss ;)

/BR
/Marcus