| Message ID | 2529718.glQX8guWfJ@amdc1227 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 27 December 2012 20:13, Tomasz Figa <t.figa@samsung.com> wrote: > 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: >> > On Tuesday 18 of December 2012 08:31:30 Vikas Sajjan wrote: >> > > On 17 December 2012 20:55, Laurent Pinchart wrote: >> > > > 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: >> > > > > 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 <t.figa@samsung.com> > Date: Thu, 27 Dec 2012 12:36:15 +0100 > Subject: [RFC] video: display: Add generic MIPI DSI bus > > Signed-off-by: Tomasz Figa <t.figa@samsung.com> > --- > 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); > +} > + 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. > +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 <t.figa@samsung.com>"); > +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 > > -- 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: > > On Friday 21 December 2012 11:00:52 Tomasz Figa wrote: > > > On Tuesday 18 of December 2012 08:31:30 Vikas Sajjan wrote: > > > > On 17 December 2012 20:55, Laurent Pinchart wrote: > > > > > 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: > > > > > > 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.
On 01/08/2013 09:18 AM, Laurent Pinchart wrote: > On Thursday 27 December 2012 15:43:34 Tomasz Figa wrote: >> > On Monday 24 of December 2012 15:12:28 Laurent Pinchart wrote: >>> > > On Friday 21 December 2012 11:00:52 Tomasz Figa wrote: >>>> > > > On Tuesday 18 of December 2012 08:31:30 Vikas Sajjan wrote: >>>>> > > > > On 17 December 2012 20:55, Laurent Pinchart wrote: >>>>>> > > > > > 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: >>>>>>> > > > > > > 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;h=499ce5cfecc9ad77593e761cdcc1624502f28432;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;h=499ce5cfecc9ad77593e761cdcc1624502f28432;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: > > On Thursday 27 December 2012 15:43:34 Tomasz Figa wrote: > >> > On Monday 24 of December 2012 15:12:28 Laurent Pinchart wrote: > >>> > > On Friday 21 December 2012 11:00:52 Tomasz Figa wrote: > >>>> > > > On Tuesday 18 of December 2012 08:31:30 Vikas Sajjan wrote: > >>>>> > > > > On 17 December 2012 20:55, Laurent Pinchart wrote: > >>>>>> > > > > > 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: > >>>>>>> > > > > > > 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;h=499ce5cfecc9ad77593e761cdcc1624502f28432;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;h=499ce5cfecc9ad77593e761cdcc1624502f28432;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,
On 01/08/2013 05:36 PM, Tomasz Figa wrote: > On Tuesday 08 of January 2013 11:12:26 Marcus Lorentzon wrote: >> On 01/08/2013 09:18 AM, Laurent Pinchart wrote: >>> On Thursday 27 December 2012 15:43:34 Tomasz Figa wrote: >>>>> On Monday 24 of December 2012 15:12:28 Laurent Pinchart wrote: >>>>>> > On Friday 21 December 2012 11:00:52 Tomasz Figa wrote: >>>>>>> > > On Tuesday 18 of December 2012 08:31:30 Vikas Sajjan > wrote: >>>>>>>> > > > On 17 December 2012 20:55, Laurent Pinchart wrote: >>>>>>>>> > > > > 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: >>>>>>>>>> > > > > > 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;h=499ce5cfecc9ad77593e761cdcc1624502f28432;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. >> 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: > > On Tuesday 08 of January 2013 11:12:26 Marcus Lorentzon wrote: [snip] > >> FYI, > >> here is STE "DSI API": > >> http://www.igloocommunity.org/gitweb/?p=kernel/igloo-kernel.git;a=blob;f > >> =include/video/mcde.h;h=499ce5cfecc9ad77593e761cdcc1624502f28432;hb=HEAD > >> #l361 Thank you. > >> 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. > > 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. > >> 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. > > 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. > > 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.
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;h=499ce5cfecc9ad77593e761cdcc1624502f28432;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.
On 02/02/2013 12:35 AM, Laurent Pinchart wrote: > Hi Marcus, > > On Tuesday 08 January 2013 18:08:19 Marcus Lorentzon wrote: >> On 01/08/2013 05:36 PM, Tomasz Figa wrote: >>> On Tuesday 08 of January 2013 11:12:26 Marcus Lorentzon wrote: [...] >>>> 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. >>> 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. >> 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. >>>> 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? >>> 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: >> Hi Marcus, >> >> On Tuesday 08 January 2013 18:08:19 Marcus Lorentzon wrote: >>> On 01/08/2013 05:36 PM, Tomasz Figa wrote: >>>> On Tuesday 08 of January 2013 11:12:26 Marcus Lorentzon wrote: > [...] >>>>> 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: > >> 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
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 <t.figa@samsung.com>"); +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,