| Message ID | 1384791923-11424-1-git-send-email-a.hajda@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Nov 18, 2013 at 05:25:23PM +0100, Andrzej Hajda wrote: > MIPI DSI bus allows to model DSI hosts > and DSI devices using Linux bus. This looks somewhat terse. Any chance you could be more verbose about what exactly this does? You could mention for instance that it allows DSI devices to be instantiated from device tree and manually. That a Linux bus type is provided and device drivers can use that to bind to DSI devices. > Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > --- > Hi, > > This is my implementation of mipi dsi bus. > The first time it was posted as a part of CDF infrastructure [1], > but if it can be merged independently I will be glad. > > I have not addressed yet Bert's comments, but I think it should > be easy, once we have agreement how to implement it. > > There are also working drivers which uses this bus: > - Exynos DSI master, > - s6e8aa0 panel. > I will post them later. > > [1] http://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg45252.html > > Changes: > v2: > - set_power callback removed (its role is passed to RUNTIME_PM), > - changed transfer ops parameters to struct, > - changed source location, > - minor fixes > > Regards > Andrzej > > --- > drivers/gpu/drm/Kconfig | 4 + > drivers/gpu/drm/Makefile | 2 + > drivers/gpu/drm/drm_mipi_dsi.c | 344 +++++++++++++++++++++++++++++++++++++++++ > include/drm/drm_mipi_dsi.h | 154 ++++++++++++++++++ > 4 files changed, 504 insertions(+) > create mode 100644 drivers/gpu/drm/drm_mipi_dsi.c > create mode 100644 include/drm/drm_mipi_dsi.h This seems to be missing a device tree binding document. That could probably be rather small since there's not a lot there right now. I volunteer to draft that document if you don't have the time to do that. > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index f864275..58a603d 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -20,6 +20,10 @@ menuconfig DRM > details. You should also select and configure AGP > (/dev/agpgart) support if it is available for your platform. > > +config DRM_MIPI_DSI > + tristate > + default y Shouldn't this remain off by default? And only be selected by drivers that actually need it. I think that DSI host drivers could select this symbol and DSI peripheral drivers can depend on it. That way you'll automatically be able to only select peripheral drivers if there's at least one DSI host driver enabled. > diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c [...] > +/* > + * MIPI DSI Bus > + * > + * Copyright (C) 2012, Samsung Electronics, Co., Ltd. Perhaps this should now be "2012-2013"? > + * Andrzej Hajda <a.hajda@samsung.com> > + * > + * 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/of_device.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/slab.h> > +#include <linux/pm.h> > +#include <linux/pm_runtime.h> Can these please be ordered alphabetically? > +/* ----------------------------------------------------------------------------- > + * Bus operations > + */ Nitpick: I'm not a huge fan of these separators. They have a tendency to get in the way when people add new functions and then all of a sudden they no longer fit into that category... > +int mipi_dsi_init(struct mipi_dsi_device *dev) > +{ [...] > +} > +EXPORT_SYMBOL_GPL(mipi_dsi_init); > + > +int mipi_dsi_set_stream(struct mipi_dsi_device *dev, bool on) > +{ [...] > +} > +EXPORT_SYMBOL_GPL(mipi_dsi_set_stream); These are missing documentation. It's not clear at all what they are supposed to do. > +int mipi_dsi_dcs_write(struct mipi_dsi_device *dev, int channel, const u8 *data, unsigned int for channel, please. And const void * for data so that the same type is used consistently. > +static struct device_attribute mipi_dsi_dev_attrs[] = { static const? I also believe these now need to be done using attribute groups. Look at commit d06262e58546 (driver-core: platform: convert bus code to use dev_groups) for an example of how to do that. > +static const struct dev_pm_ops mipi_dsi_dev_pm_ops = { > + .runtime_suspend = pm_generic_runtime_suspend, > + .runtime_resume = pm_generic_runtime_resume, > + .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, > +}; Can we please stick to one style for these structures? I personally prefer the one you used for mipi_dsi_dev_pm_ops because the other one falls apart when you need to initialize a new field that exceeds the size of the indentation that you've chosen. It also wastes screen space and doesn't really make the code more readable in my opinion. > +void mipi_dsi_dev_release(struct device *_dev) > +{ > + struct mipi_dsi_device *dev = to_mipi_dsi_device(_dev); > + > + kfree(dev); > +} > + > +static struct device_type mipi_dsi_dev_type = { > + .release = mipi_dsi_dev_release, > +}; Also, is there any reason why you've switched to the mipi_dsi_dev prefix instead of mipi_dsi_device all of a sudden? > +/** > + * 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; > + dev->dev.type = &mipi_dsi_dev_type; > + > + if (dev->id != -1) Does that case actually make sense in the context of DSI? There's a defined hierarchy in DSI, so shouldn't we construct the names in a more hierarchical way? I'm not sure if the ID is useful at all, perhaps it should really be the virtual channel? The patch that I proposed used the following to make up the device name: dev_set_name(&dsi->dev, "%s.%u", dev_name(host->dev), dsi->channel); That has the advantage of reflecting the bus topology. It seems like your code was copied mostly from platform_device, for which there's no well-defined topology and therefore having an ID makes sense to differentiate between multiple instances of the same device. But for DSI any host/bus can only have a single device with a given channel, so that makes for a pretty good for unique name. > +/** > + * 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); That's actually device_unregister(). > +} > +EXPORT_SYMBOL_GPL(mipi_dsi_device_unregister); In general DRM doesn't use GPL-only symbols in order to make it easier to port the code to other operating systems. > +struct mipi_dsi_device *of_mipi_dsi_register_device(struct mipi_dsi_bus *bus, > + struct device_node *node) > +{ > + struct mipi_dsi_device *d = NULL; Perhaps "dev" instead of "d", as the former is used everywhere else. > + int ret; > + > + d = kzalloc(sizeof(*d), GFP_KERNEL); > + if (!d) > + return ERR_PTR(-ENOMEM); > + > + ret = of_modalias_node(node, d->name, sizeof(d->name)); > + if (ret) { > + dev_err(bus->dev, "modalias failure on %s\n", node->full_name); > + goto err_free; > + } > + > + d->dev.of_node = of_node_get(node); > + if (!d->dev.of_node) > + goto err_free; > + > + ret = mipi_dsi_device_register(d, bus); > + > + if (!ret) > + return d; > + > + of_node_put(node); > +err_free: > + kfree(d); > + > + return ERR_PTR(ret); > +} The cleanup looks somewhat weird here. I think the more canonical form would be: ret = mipi_dsi_device_register(dev, bus); if (ret) goto err_put; return dev; err_put: of_node_put(node); err_free: kfree(dev); return ERR_PTR(ret); Also I think we'll need to have a reg property and parse that to allow a device tree node to be matched to a virtual channel. > +static int __init mipi_dsi_bus_init(void) > +{ > + return bus_register(&mipi_dsi_bus_type); > +} > + > +static void __exit mipi_dsi_bus_exit(void) > +{ > + bus_unregister(&mipi_dsi_bus_type); > +} > + > +module_init(mipi_dsi_bus_init); > +module_exit(mipi_dsi_bus_exit) I don't think module_init() will work here. The reason is that the probe order for built-in drivers is determined primarily by link-order, so if any DSI device drivers end up linked in earlier than the DSI bus infrastructure, then the bus won't have been initialized yet when that driver is probed and registering a device or bus with it will fail. > +MODULE_LICENSE("GPL v2"); I think this needs to be "GPL and additional rights" to be used within DRM. > diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h [...] > +#ifndef __DRM_MIPI_DSI_H__ > +#define __DRM_MIPI_DSI_H__ > + > +#include <linux/device.h> > +#include <video/videomode.h> > + > +struct mipi_dsi_bus; > +struct mipi_dsi_device; > + > +struct mipi_dsi_msg { > + u8 channel; > + u8 type; > + > + size_t tx_len; > + const void *tx_buf; > + > + size_t rx_len; > + void *rx_buf; > +}; > + > +struct mipi_dsi_bus_ops { > + int (*init)(struct mipi_dsi_bus *bus, struct mipi_dsi_device *dev); > + int (*set_stream)(struct mipi_dsi_bus *bus, struct mipi_dsi_device *dev, > + bool on); Again, these should really be documented. I can somewhat guess what .init() is supposed to do. But I have no idea what .set_stream() is, though I think I've seen that in CDF patches. Perhaps that's a relic from when this was based on CDF? > + ssize_t (*transfer)(struct mipi_dsi_bus *bus, > + struct mipi_dsi_device *dev, > + struct mipi_dsi_msg *msg); Why do we need the dev parameter? There's already a channel field in the mipi_dsi_msg structure. Isn't that all that the transfer function needs to know about the device? > +#define DSI_MODE_VIDEO (1 << 0) > +#define DSI_MODE_VIDEO_BURST (1 << 1) > +#define DSI_MODE_VIDEO_SYNC_PULSE (1 << 2) > +#define DSI_MODE_VIDEO_AUTO_VERT (1 << 3) > +#define DSI_MODE_VIDEO_HSE (1 << 4) > +#define DSI_MODE_VIDEO_HFP (1 << 5) > +#define DSI_MODE_VIDEO_HBP (1 << 6) > +#define DSI_MODE_VIDEO_HSA (1 << 7) > +#define DSI_MODE_VSYNC_FLUSH (1 << 8) > +#define DSI_MODE_EOT_PACKET (1 << 9) It should be documented how these are used. > +enum mipi_dsi_pixel_format { > + DSI_FMT_RGB888, > + DSI_FMT_RGB666, > + DSI_FMT_RGB666_PACKED, > + DSI_FMT_RGB565, > +}; > + > +struct mipi_dsi_interface_params { > + enum mipi_dsi_pixel_format format; > + unsigned long mode; I assume this is a bitmask of DSI_MODE_*. In that case perhaps "modes" would be a more accurate name? > + unsigned long hs_clk_freq; > + unsigned long esc_clk_freq; How are these interface parameters? The frequencies can certainly vary depending on the display mode, so why would a device specify that as a parameter? > + unsigned char data_lanes; > + unsigned char cmd_allow; What's cmd_allow? > +}; Any reason these parameters cannot be moved to the mipi_dsi_device structure? > +struct mipi_dsi_bus { I'd prefer this to be called mipi_dsi_host, because that's the name used everywhere in the specification. > + struct device *dev; > + const struct mipi_dsi_bus_ops *ops; > +}; > + > +#define MIPI_DSI_MODULE_PREFIX "mipi-dsi:" > +#define MIPI_DSI_NAME_SIZE 32 Do we really need this? Can't we allocate the name dynamically... > +struct mipi_dsi_device_id { > + char name[MIPI_DSI_NAME_SIZE]; ... or use const char * here? > + __kernel_ulong_t driver_data /* Data private to the driver */ > + __aligned(sizeof(__kernel_ulong_t)); I don't think the explicit aligned attribute is necessary here. > +}; > + > +struct mipi_dsi_device { > + char name[MIPI_DSI_NAME_SIZE]; > + int id; > + struct device dev; > + > + const struct mipi_dsi_device_id *id_entry; > + struct mipi_dsi_bus *bus; > + struct videomode vm; Why is the videomode part of this structure? What if a device supports more than a single mode? > +int of_mipi_dsi_register_devices(struct mipi_dsi_bus *bus); Perhaps there ought to be a dummy implementation for this in case OF isn't selected? In fact, I think we should be hiding all those details from users. They shouldn't bother about manually registering with OF. Instead this should be mipi_dsi_register_bus/host()... > +void mipi_dsi_unregister_devices(struct mipi_dsi_bus *bus); ... and this mipi_dsi_unregister_bus/host(). That way registration with OF (and instantiation of devices from DT) can be done automatically as needed. > + > +/* 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() > + */ The correct style for block comments for kernel-doc is: /** * module_mipi_dsi_driver() - ... * ... */ > +#define mipi_dsi_dcs_write_seq(dev, channel, seq...) \ > +({\ > + const u8 d[] = { seq };\ > + BUILD_BUG_ON_MSG(ARRAY_SIZE(d) > 64, "DCS sequence too long for stack");\ > + mipi_dsi_dcs_write(dev, channel, d, ARRAY_SIZE(d));\ > +}) > + > +#define mipi_dsi_dcs_write_static_seq(dev, channel, seq...) \ > +({\ > + static const u8 d[] = { seq };\ > + mipi_dsi_dcs_write(dev, channel, d, ARRAY_SIZE(d));\ > +}) Can we drop these please? I'd rather have drivers construct buffers explicitly than use these. Thierry
Hi Thierry, Thanks for the review. On 11/22/2013 06:41 PM, Thierry Reding wrote: > On Mon, Nov 18, 2013 at 05:25:23PM +0100, Andrzej Hajda wrote: >> MIPI DSI bus allows to model DSI hosts >> and DSI devices using Linux bus. > This looks somewhat terse. Any chance you could be more verbose about > what exactly this does? You could mention for instance that it allows > DSI devices to be instantiated from device tree and manually. That a > Linux bus type is provided and device drivers can use that to bind to > DSI devices. OK. > >> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> >> --- >> Hi, >> >> This is my implementation of mipi dsi bus. >> The first time it was posted as a part of CDF infrastructure [1], >> but if it can be merged independently I will be glad. >> >> I have not addressed yet Bert's comments, but I think it should >> be easy, once we have agreement how to implement it. >> >> There are also working drivers which uses this bus: >> - Exynos DSI master, >> - s6e8aa0 panel. >> I will post them later. >> >> [1] http://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg45252.html >> >> Changes: >> v2: >> - set_power callback removed (its role is passed to RUNTIME_PM), >> - changed transfer ops parameters to struct, >> - changed source location, >> - minor fixes >> >> Regards >> Andrzej >> >> --- >> drivers/gpu/drm/Kconfig | 4 + >> drivers/gpu/drm/Makefile | 2 + >> drivers/gpu/drm/drm_mipi_dsi.c | 344 +++++++++++++++++++++++++++++++++++++++++ >> include/drm/drm_mipi_dsi.h | 154 ++++++++++++++++++ >> 4 files changed, 504 insertions(+) >> create mode 100644 drivers/gpu/drm/drm_mipi_dsi.c >> create mode 100644 include/drm/drm_mipi_dsi.h > This seems to be missing a device tree binding document. That could > probably be rather small since there's not a lot there right now. I > volunteer to draft that document if you don't have the time to do > that. > >> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig >> index f864275..58a603d 100644 >> --- a/drivers/gpu/drm/Kconfig >> +++ b/drivers/gpu/drm/Kconfig >> @@ -20,6 +20,10 @@ menuconfig DRM >> details. You should also select and configure AGP >> (/dev/agpgart) support if it is available for your platform. >> >> +config DRM_MIPI_DSI >> + tristate >> + default y > Shouldn't this remain off by default? And only be selected by drivers > that actually need it. I think that DSI host drivers could select this > symbol and DSI peripheral drivers can depend on it. That way you'll > automatically be able to only select peripheral drivers if there's at > least one DSI host driver enabled. Yes, of course. I just forgot to set it back properly after last tests. > >> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c > [...] >> +/* >> + * MIPI DSI Bus >> + * >> + * Copyright (C) 2012, Samsung Electronics, Co., Ltd. > Perhaps this should now be "2012-2013"? Yes, thanks. > >> + * Andrzej Hajda <a.hajda@samsung.com> >> + * >> + * 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/of_device.h> >> +#include <linux/module.h> >> +#include <linux/mutex.h> >> +#include <linux/slab.h> >> +#include <linux/pm.h> >> +#include <linux/pm_runtime.h> > Can these please be ordered alphabetically? OK > >> +/* ----------------------------------------------------------------------------- >> + * Bus operations >> + */ > Nitpick: I'm not a huge fan of these separators. They have a tendency to > get in the way when people add new functions and then all of a sudden > they no longer fit into that category... > >> +int mipi_dsi_init(struct mipi_dsi_device *dev) >> +{ > [...] >> +} >> +EXPORT_SYMBOL_GPL(mipi_dsi_init); >> + >> +int mipi_dsi_set_stream(struct mipi_dsi_device *dev, bool on) >> +{ > [...] >> +} >> +EXPORT_SYMBOL_GPL(mipi_dsi_set_stream); > These are missing documentation. It's not clear at all what they are > supposed to do. > >> +int mipi_dsi_dcs_write(struct mipi_dsi_device *dev, int channel, const u8 *data, > unsigned int for channel, please. And const void * for data so that the > same type is used consistently. > >> +static struct device_attribute mipi_dsi_dev_attrs[] = { > static const? > > I also believe these now need to be done using attribute groups. Look at > commit d06262e58546 (driver-core: platform: convert bus code to use > dev_groups) for an example of how to do that. >> +static const struct dev_pm_ops mipi_dsi_dev_pm_ops = { >> + .runtime_suspend = pm_generic_runtime_suspend, >> + .runtime_resume = pm_generic_runtime_resume, >> + .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, >> +}; > Can we please stick to one style for these structures? I personally > prefer the one you used for mipi_dsi_dev_pm_ops because the other one > falls apart when you need to initialize a new field that exceeds the > size of the indentation that you've chosen. It also wastes screen space > and doesn't really make the code more readable in my opinion. OK, for all above. > >> +void mipi_dsi_dev_release(struct device *_dev) >> +{ >> + struct mipi_dsi_device *dev = to_mipi_dsi_device(_dev); >> + >> + kfree(dev); >> +} >> + >> +static struct device_type mipi_dsi_dev_type = { >> + .release = mipi_dsi_dev_release, >> +}; > Also, is there any reason why you've switched to the mipi_dsi_dev prefix > instead of mipi_dsi_device all of a sudden? Ups, will be fixed. > >> +/** >> + * 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; >> + dev->dev.type = &mipi_dsi_dev_type; >> + >> + if (dev->id != -1) > Does that case actually make sense in the context of DSI? There's a > defined hierarchy in DSI, so shouldn't we construct the names in a more > hierarchical way? I'm not sure if the ID is useful at all, perhaps it > should really be the virtual channel? > > The patch that I proposed used the following to make up the device name: > > dev_set_name(&dsi->dev, "%s.%u", dev_name(host->dev), dsi->channel); > > That has the advantage of reflecting the bus topology. > > It seems like your code was copied mostly from platform_device, for > which there's no well-defined topology and therefore having an ID makes > sense to differentiate between multiple instances of the same device. > But for DSI any host/bus can only have a single device with a given > channel, so that makes for a pretty good for unique name. Code was based on mipi_dbi_bus.c from CDF. I am not sure about hardwiring devices to virtual channels. There could be devices which uses more than one virtual channel, in fact exynos-drm docs suggests that such hardware exists. I can also imagine scenarios when dynamic virtual channel (re-)association can be useful and DSI specs are not against it AFAIK. Anyway the whole 'multiple devs on one DSI master' thing seems to me quite unspecified. > >> +/** >> + * 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); > That's actually device_unregister(). > >> +} >> +EXPORT_SYMBOL_GPL(mipi_dsi_device_unregister); > In general DRM doesn't use GPL-only symbols in order to make it easier > to port the code to other operating systems. > >> +struct mipi_dsi_device *of_mipi_dsi_register_device(struct mipi_dsi_bus *bus, >> + struct device_node *node) >> +{ >> + struct mipi_dsi_device *d = NULL; > Perhaps "dev" instead of "d", as the former is used everywhere else. > >> + int ret; >> + >> + d = kzalloc(sizeof(*d), GFP_KERNEL); >> + if (!d) >> + return ERR_PTR(-ENOMEM); >> + >> + ret = of_modalias_node(node, d->name, sizeof(d->name)); >> + if (ret) { >> + dev_err(bus->dev, "modalias failure on %s\n", node->full_name); >> + goto err_free; >> + } >> + >> + d->dev.of_node = of_node_get(node); >> + if (!d->dev.of_node) >> + goto err_free; >> + >> + ret = mipi_dsi_device_register(d, bus); >> + >> + if (!ret) >> + return d; >> + >> + of_node_put(node); >> +err_free: >> + kfree(d); >> + >> + return ERR_PTR(ret); >> +} > The cleanup looks somewhat weird here. I think the more canonical form > would be: > > ret = mipi_dsi_device_register(dev, bus); > if (ret) > goto err_put; > > return dev; > > err_put: > of_node_put(node); > err_free: > kfree(dev); > > return ERR_PTR(ret); OK for all above > > Also I think we'll need to have a reg property and parse that to allow a > device tree node to be matched to a virtual channel. reg property means device is at fixed virtual channel. DSI specs says nothing about it IMHO. >> +static int __init mipi_dsi_bus_init(void) >> +{ >> + return bus_register(&mipi_dsi_bus_type); >> +} >> + >> +static void __exit mipi_dsi_bus_exit(void) >> +{ >> + bus_unregister(&mipi_dsi_bus_type); >> +} >> + >> +module_init(mipi_dsi_bus_init); >> +module_exit(mipi_dsi_bus_exit) > I don't think module_init() will work here. The reason is that the probe > order for built-in drivers is determined primarily by link-order, so if > any DSI device drivers end up linked in earlier than the DSI bus > infrastructure, then the bus won't have been initialized yet when that > driver is probed and registering a device or bus with it will fail. > >> +MODULE_LICENSE("GPL v2"); > I think this needs to be "GPL and additional rights" to be used within > DRM. > >> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h > [...] >> +#ifndef __DRM_MIPI_DSI_H__ >> +#define __DRM_MIPI_DSI_H__ >> + >> +#include <linux/device.h> >> +#include <video/videomode.h> >> + >> +struct mipi_dsi_bus; >> +struct mipi_dsi_device; >> + >> +struct mipi_dsi_msg { >> + u8 channel; >> + u8 type; >> + >> + size_t tx_len; >> + const void *tx_buf; >> + >> + size_t rx_len; >> + void *rx_buf; >> +}; >> + >> +struct mipi_dsi_bus_ops { >> + int (*init)(struct mipi_dsi_bus *bus, struct mipi_dsi_device *dev); >> + int (*set_stream)(struct mipi_dsi_bus *bus, struct mipi_dsi_device *dev, >> + bool on); > Again, these should really be documented. I can somewhat guess what > .init() is supposed to do. But I have no idea what .set_stream() is, > though I think I've seen that in CDF patches. Perhaps that's a relic > from when this was based on CDF? OK, beside documenting I will add drivers for dsi and panel in the next iteration to show it in use. > >> + ssize_t (*transfer)(struct mipi_dsi_bus *bus, >> + struct mipi_dsi_device *dev, >> + struct mipi_dsi_msg *msg); > Why do we need the dev parameter? There's already a channel field in the > mipi_dsi_msg structure. Isn't that all that the transfer function needs > to know about the device? For simple HSI transfers, yes. In case transfer would depend on dev state it would be useful, but I have no use case yet, so it could be removed. > >> +#define DSI_MODE_VIDEO (1 << 0) >> +#define DSI_MODE_VIDEO_BURST (1 << 1) >> +#define DSI_MODE_VIDEO_SYNC_PULSE (1 << 2) >> +#define DSI_MODE_VIDEO_AUTO_VERT (1 << 3) >> +#define DSI_MODE_VIDEO_HSE (1 << 4) >> +#define DSI_MODE_VIDEO_HFP (1 << 5) >> +#define DSI_MODE_VIDEO_HBP (1 << 6) >> +#define DSI_MODE_VIDEO_HSA (1 << 7) >> +#define DSI_MODE_VSYNC_FLUSH (1 << 8) >> +#define DSI_MODE_EOT_PACKET (1 << 9) > It should be documented how these are used. OK > >> +enum mipi_dsi_pixel_format { >> + DSI_FMT_RGB888, >> + DSI_FMT_RGB666, >> + DSI_FMT_RGB666_PACKED, >> + DSI_FMT_RGB565, >> +}; >> + >> +struct mipi_dsi_interface_params { >> + enum mipi_dsi_pixel_format format; >> + unsigned long mode; > I assume this is a bitmask of DSI_MODE_*. In that case perhaps "modes" > would be a more accurate name? Yes > >> + unsigned long hs_clk_freq; >> + unsigned long esc_clk_freq; > How are these interface parameters? The frequencies can certainly vary > depending on the display mode, so why would a device specify that as a > parameter? > >> + unsigned char data_lanes; >> + unsigned char cmd_allow; > What's cmd_allow? For sure it requires documentation:) In this particular case it is a number of lines where DSI-master allows to send commands in video mode. > >> +}; > Any reason these parameters cannot be moved to the mipi_dsi_device > structure? I agree, it is again heritage of CDF. > >> +struct mipi_dsi_bus { > I'd prefer this to be called mipi_dsi_host, because that's the name used > everywhere in the specification. > >> + struct device *dev; >> + const struct mipi_dsi_bus_ops *ops; >> +}; >> + >> +#define MIPI_DSI_MODULE_PREFIX "mipi-dsi:" >> +#define MIPI_DSI_NAME_SIZE 32 > Do we really need this? Can't we allocate the name dynamically... > >> +struct mipi_dsi_device_id { >> + char name[MIPI_DSI_NAME_SIZE]; > ... or use const char * here? > >> + __kernel_ulong_t driver_data /* Data private to the driver */ >> + __aligned(sizeof(__kernel_ulong_t)); > I don't think the explicit aligned attribute is necessary here. > >> +}; >> + >> +struct mipi_dsi_device { >> + char name[MIPI_DSI_NAME_SIZE]; >> + int id; >> + struct device dev; >> + >> + const struct mipi_dsi_device_id *id_entry; >> + struct mipi_dsi_bus *bus; >> + struct videomode vm; > Why is the videomode part of this structure? What if a device supports > more than a single mode? This is the current video mode. If we want to change mode, we call: ops->set_stream(false); dev->vm =...new mode ops->set_stream(true); > >> +int of_mipi_dsi_register_devices(struct mipi_dsi_bus *bus); > Perhaps there ought to be a dummy implementation for this in case OF > isn't selected? > > In fact, I think we should be hiding all those details from users. They > shouldn't bother about manually registering with OF. Instead this should > be mipi_dsi_register_bus/host()... OK > >> +void mipi_dsi_unregister_devices(struct mipi_dsi_bus *bus); > ... and this mipi_dsi_unregister_bus/host(). That way registration with > OF (and instantiation of devices from DT) can be done automatically as > needed. > >> + >> +/* 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() >> + */ > The correct style for block comments for kernel-doc is: > > /** > * module_mipi_dsi_driver() - ... > * ... > */ > >> +#define mipi_dsi_dcs_write_seq(dev, channel, seq...) \ >> +({\ >> + const u8 d[] = { seq };\ >> + BUILD_BUG_ON_MSG(ARRAY_SIZE(d) > 64, "DCS sequence too long for stack");\ >> + mipi_dsi_dcs_write(dev, channel, d, ARRAY_SIZE(d));\ >> +}) >> + >> +#define mipi_dsi_dcs_write_static_seq(dev, channel, seq...) \ >> +({\ >> + static const u8 d[] = { seq };\ >> + mipi_dsi_dcs_write(dev, channel, d, ARRAY_SIZE(d));\ >> +}) > Can we drop these please? I'd rather have drivers construct buffers > explicitly than use these. I like those two. It removes lots of boiler-plate code. Please compare implementations of s6e8aa panel drivers without it [1] and with it [2], look for calls to dsi_dcs_write. [1] http://lists.freedesktop.org/archives/dri-devel/2013-January/034212.html [2] https://linuxtv.org/patch/20215/ Andrzej > > Thierry
On Mon, Nov 25, 2013 at 04:05:41PM +0100, Andrzej Hajda wrote: [...] > >> +/** > >> + * 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; > >> + dev->dev.type = &mipi_dsi_dev_type; > >> + > >> + if (dev->id != -1) > > Does that case actually make sense in the context of DSI? There's a > > defined hierarchy in DSI, so shouldn't we construct the names in a more > > hierarchical way? I'm not sure if the ID is useful at all, perhaps it > > should really be the virtual channel? > > > > The patch that I proposed used the following to make up the device name: > > > > dev_set_name(&dsi->dev, "%s.%u", dev_name(host->dev), dsi->channel); > > > > That has the advantage of reflecting the bus topology. > > > > It seems like your code was copied mostly from platform_device, for > > which there's no well-defined topology and therefore having an ID makes > > sense to differentiate between multiple instances of the same device. > > But for DSI any host/bus can only have a single device with a given > > channel, so that makes for a pretty good for unique name. > Code was based on mipi_dbi_bus.c from CDF. > I am not sure about hardwiring devices to virtual channels. > There could be devices which uses more than one virtual channel, > in fact exynos-drm docs suggests that such hardware exists. In that case, why not make them two logically separate devices within the kernel. We need the channel number so that the device can be addressed in the first place, so I don't see what's wrong with using that number in the device's name. The whole point of this patch is to add MIPI DSI bus infrastructure, and the virtual channel is one of the fundamental aspects of that bus, so I think we need to make it an integral part of the implementation. > I can also imagine scenarios when dynamic virtual channel (re-)association > can be useful and DSI specs are not against it AFAIK. How is that going to work? There's no hotplug support or similar in DSI, so why would anyone want to reassign virtual channels. Supposing even that we wanted to support this eventually, I think a more appropriate solution would be to completely remove the device and add a new one, because that also takes care of keeping the channel number embedded within the struct mipi_dsi_device up to date. > reg property means device is at fixed virtual channel. > DSI specs says nothing about it IMHO. Without that fixed virtual channel number we can't use the device in the first place. How are you going to address the device if you don't know its virtual channel? > >> + ssize_t (*transfer)(struct mipi_dsi_bus *bus, > >> + struct mipi_dsi_device *dev, > >> + struct mipi_dsi_msg *msg); > > Why do we need the dev parameter? There's already a channel field in the > > mipi_dsi_msg structure. Isn't that all that the transfer function needs > > to know about the device? > For simple HSI transfers, yes. In case transfer would depend on dev state > it would be useful, but I have no use case yet, so it could be removed. I don't know what an HSI transfer is. The transfer function is something that will be called by the peripheral's device driver, and that driver will know the state of the device, so I don't think the DSI host should care. > >> +struct mipi_dsi_device { > >> + char name[MIPI_DSI_NAME_SIZE]; > >> + int id; > >> + struct device dev; > >> + > >> + const struct mipi_dsi_device_id *id_entry; > >> + struct mipi_dsi_bus *bus; > >> + struct videomode vm; > > Why is the videomode part of this structure? What if a device supports > > more than a single mode? > This is the current video mode. If we want to change mode, > we call: > ops->set_stream(false); > dev->vm =...new mode > ops->set_stream(true); I don't think that needs to be part of the DSI device. Rather it seems to me that whatever object type is implemented by the DSI device driver should have subsystem-specific code to do this. For example, if a DSI device is a bridge, then it will implement a drm_bridge object, and in that case the drm_bridge_funcs.mode_set() would be the equivalent of this. On a side-note, I think we should rename .set_stream() to something more DSI specific, such as: enum mipi_dsi_mode { MIPI_DSI_COMMAND_MODE, MIPI_DSI_VIDEO_MODE, }; int (*set_mode)(struct mipi_dsi_host *host, enum mipi_dsi_mode mode); > >> +#define mipi_dsi_dcs_write_seq(dev, channel, seq...) \ > >> +({\ > >> + const u8 d[] = { seq };\ > >> + BUILD_BUG_ON_MSG(ARRAY_SIZE(d) > 64, "DCS sequence too long for stack");\ > >> + mipi_dsi_dcs_write(dev, channel, d, ARRAY_SIZE(d));\ > >> +}) > >> + > >> +#define mipi_dsi_dcs_write_static_seq(dev, channel, seq...) \ > >> +({\ > >> + static const u8 d[] = { seq };\ > >> + mipi_dsi_dcs_write(dev, channel, d, ARRAY_SIZE(d));\ > >> +}) > > Can we drop these please? I'd rather have drivers construct buffers > > explicitly than use these. > I like those two. It removes lots of boiler-plate code. Please compare > implementations > of s6e8aa panel drivers without it [1] and with it [2], look for > calls to dsi_dcs_write. > > [1] http://lists.freedesktop.org/archives/dri-devel/2013-January/034212.html > [2] https://linuxtv.org/patch/20215/ Actually I much prefer the first version that doesn't use the macros. It's much more explicit and therefore obvious what's happening. I can understand that these might be convenient for some use-cases, but I don't think that warrants them being part of the core. You can always add them in specific drivers if you really want to. Once more people start using this infrastructure and these macros start to appear as a common pattern we can still move them into the core header to avoid the duplication. Anyway, it seems like there are still a few things that need discussion, so in an attempt to push this forward perhaps a good idea would be to drop all the contentious parts and focus on getting the basic infra- structure to work. The crucial parts will be to have a standard way of instantiating devices from device tree. As far as I can tell, the only disagreement we have on that matter is whether or not to name the devices according to their bus number. I hope I've been able to convince you above. Do you think it would be possible to remove the .set_stream() and .transfer() operations (and related defines) for now? I'm not asking for you to drop them, just to move them to a separate patch so that the basic infrastructure patch can be merged early and we can get started to port drivers to use this as soon as possible. Thierry
On 11/27/2013 11:54 AM, Thierry Reding wrote: > On Mon, Nov 25, 2013 at 04:05:41PM +0100, Andrzej Hajda wrote: > [...] >>>> +/** >>>> + * 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; >>>> + dev->dev.type = &mipi_dsi_dev_type; >>>> + >>>> + if (dev->id != -1) >>> Does that case actually make sense in the context of DSI? There's a >>> defined hierarchy in DSI, so shouldn't we construct the names in a more >>> hierarchical way? I'm not sure if the ID is useful at all, perhaps it >>> should really be the virtual channel? >>> >>> The patch that I proposed used the following to make up the device name: >>> >>> dev_set_name(&dsi->dev, "%s.%u", dev_name(host->dev), dsi->channel); >>> >>> That has the advantage of reflecting the bus topology. >>> >>> It seems like your code was copied mostly from platform_device, for >>> which there's no well-defined topology and therefore having an ID makes >>> sense to differentiate between multiple instances of the same device. >>> But for DSI any host/bus can only have a single device with a given >>> channel, so that makes for a pretty good for unique name. >> Code was based on mipi_dbi_bus.c from CDF. >> I am not sure about hardwiring devices to virtual channels. >> There could be devices which uses more than one virtual channel, >> in fact exynos-drm docs suggests that such hardware exists. > In that case, why not make them two logically separate devices within > the kernel. We need the channel number so that the device can be > addressed in the first place, so I don't see what's wrong with using > that number in the device's name. > > The whole point of this patch is to add MIPI DSI bus infrastructure, and > the virtual channel is one of the fundamental aspects of that bus, so I > think we need to make it an integral part of the implementation. There are already devices which IMO do not fit to this model, for example TC358710 [1] can work as DSI hub, which I suppose uses two or three virtual channels of the main DSI-host and performs "automatic virtual channel translation". [1] http://www.toshiba.com/taec/components/ProdBrief/10F02_TC358710_ProdBrief.pdf > >> I can also imagine scenarios when dynamic virtual channel (re-)association >> can be useful and DSI specs are not against it AFAIK. > How is that going to work? There's no hotplug support or similar in DSI, > so why would anyone want to reassign virtual channels. > > Supposing even that we wanted to support this eventually, I think a more > appropriate solution would be to completely remove the device and add a > new one, because that also takes care of keeping the channel number > embedded within the struct mipi_dsi_device up to date. > >> reg property means device is at fixed virtual channel. >> DSI specs says nothing about it IMHO. > Without that fixed virtual channel number we can't use the device in the > first place. How are you going to address the device if you don't know > its virtual channel? > >>>> + ssize_t (*transfer)(struct mipi_dsi_bus *bus, >>>> + struct mipi_dsi_device *dev, >>>> + struct mipi_dsi_msg *msg); >>> Why do we need the dev parameter? There's already a channel field in the >>> mipi_dsi_msg structure. Isn't that all that the transfer function needs >>> to know about the device? >> For simple HSI transfers, yes. In case transfer would depend on dev state >> it would be useful, but I have no use case yet, so it could be removed. > I don't know what an HSI transfer is. The transfer function is something > that will be called by the peripheral's device driver, and that driver > will know the state of the device, so I don't think the DSI host should > care. It should be HS, ie high-speed. But as I wrote before we can remove it for now. It can be added again in the future if I will have a real use case. > >>>> +struct mipi_dsi_device { >>>> + char name[MIPI_DSI_NAME_SIZE]; >>>> + int id; >>>> + struct device dev; >>>> + >>>> + const struct mipi_dsi_device_id *id_entry; >>>> + struct mipi_dsi_bus *bus; >>>> + struct videomode vm; >>> Why is the videomode part of this structure? What if a device supports >>> more than a single mode? >> This is the current video mode. If we want to change mode, >> we call: >> ops->set_stream(false); >> dev->vm =...new mode >> ops->set_stream(true); > I don't think that needs to be part of the DSI device. Rather it seems > to me that whatever object type is implemented by the DSI device driver > should have subsystem-specific code to do this. > > For example, if a DSI device is a bridge, then it will implement a > drm_bridge object, and in that case the drm_bridge_funcs.mode_set() > would be the equivalent of this. I think mode setting is common for most DSI-hosts, at least for all having video mode. And enabling/disabling transmission is also quite common for DSI hosts. > > On a side-note, I think we should rename .set_stream() to something more > DSI specific, such as: > > enum mipi_dsi_mode { > MIPI_DSI_COMMAND_MODE, > MIPI_DSI_VIDEO_MODE, > }; > > int (*set_mode)(struct mipi_dsi_host *host, enum mipi_dsi_mode mode); Yes, that looks better. > >>>> +#define mipi_dsi_dcs_write_seq(dev, channel, seq...) \ >>>> +({\ >>>> + const u8 d[] = { seq };\ >>>> + BUILD_BUG_ON_MSG(ARRAY_SIZE(d) > 64, "DCS sequence too long for stack");\ >>>> + mipi_dsi_dcs_write(dev, channel, d, ARRAY_SIZE(d));\ >>>> +}) >>>> + >>>> +#define mipi_dsi_dcs_write_static_seq(dev, channel, seq...) \ >>>> +({\ >>>> + static const u8 d[] = { seq };\ >>>> + mipi_dsi_dcs_write(dev, channel, d, ARRAY_SIZE(d));\ >>>> +}) >>> Can we drop these please? I'd rather have drivers construct buffers >>> explicitly than use these. >> I like those two. It removes lots of boiler-plate code. Please compare >> implementations >> of s6e8aa panel drivers without it [1] and with it [2], look for >> calls to dsi_dcs_write. >> >> [1] http://lists.freedesktop.org/archives/dri-devel/2013-January/034212.html >> [2] https://linuxtv.org/patch/20215/ > Actually I much prefer the first version that doesn't use the macros. > It's much more explicit and therefore obvious what's happening. I can > understand that these might be convenient for some use-cases, but I > don't think that warrants them being part of the core. You can always > add them in specific drivers if you really want to. Once more people > start using this infrastructure and these macros start to appear as a > common pattern we can still move them into the core header to avoid the > duplication. Hmm, grep -rP '--include=*.h' '\.\.\.\)' shows lots of similar macros :) Anyway I can move it to driver :) > > Anyway, it seems like there are still a few things that need discussion, > so in an attempt to push this forward perhaps a good idea would be to > drop all the contentious parts and focus on getting the basic infra- > structure to work. The crucial parts will be to have a standard way of > instantiating devices from device tree. As far as I can tell, the only > disagreement we have on that matter is whether or not to name the > devices according to their bus number. I hope I've been able to convince > you above. > > Do you think it would be possible to remove the .set_stream() and > .transfer() operations (and related defines) for now? I'm not asking for > you to drop them, just to move them to a separate patch so that the > basic infrastructure patch can be merged early and we can get started to > port drivers to use this as soon as possible. I would like to have DSI bus and working DSI host and panel, so I would like to replace set_stream at least with set_mode. I hope to send next iteration at the beginning of the next week. Andrzej > > Thierry
On Fri, Nov 29, 2013 at 04:28:45PM +0100, Andrzej Hajda wrote: > On 11/27/2013 11:54 AM, Thierry Reding wrote: > > On Mon, Nov 25, 2013 at 04:05:41PM +0100, Andrzej Hajda wrote: > > [...] > >>>> +/** > >>>> + * 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; > >>>> + dev->dev.type = &mipi_dsi_dev_type; > >>>> + > >>>> + if (dev->id != -1) > >>> Does that case actually make sense in the context of DSI? There's a > >>> defined hierarchy in DSI, so shouldn't we construct the names in a more > >>> hierarchical way? I'm not sure if the ID is useful at all, perhaps it > >>> should really be the virtual channel? > >>> > >>> The patch that I proposed used the following to make up the device name: > >>> > >>> dev_set_name(&dsi->dev, "%s.%u", dev_name(host->dev), dsi->channel); > >>> > >>> That has the advantage of reflecting the bus topology. > >>> > >>> It seems like your code was copied mostly from platform_device, for > >>> which there's no well-defined topology and therefore having an ID makes > >>> sense to differentiate between multiple instances of the same device. > >>> But for DSI any host/bus can only have a single device with a given > >>> channel, so that makes for a pretty good for unique name. > >> Code was based on mipi_dbi_bus.c from CDF. > >> I am not sure about hardwiring devices to virtual channels. > >> There could be devices which uses more than one virtual channel, > >> in fact exynos-drm docs suggests that such hardware exists. > > In that case, why not make them two logically separate devices within > > the kernel. We need the channel number so that the device can be > > addressed in the first place, so I don't see what's wrong with using > > that number in the device's name. > > > > The whole point of this patch is to add MIPI DSI bus infrastructure, and > > the virtual channel is one of the fundamental aspects of that bus, so I > > think we need to make it an integral part of the implementation. > There are already devices which IMO do not fit to this model, for example > TC358710 [1] can work as DSI hub, which I suppose uses two or three virtual > channels of the main DSI-host and performs "automatic virtual channel > translation". > > [1] > http://www.toshiba.com/taec/components/ProdBrief/10F02_TC358710_ProdBrief.pdf It's not apparent from that product brief whether or not the hub itself can be addressed. But that doesn't really matter either. If it can be addressed, it will respond to one of the four possible virtual channels while forwarding everything destined at another virtual channel to one of the downstream ports. If the hub cannot be addressed, then all downstream ports still need to be addressed using a valid virtual channel number. In order to do so we still need that virtual channel to be associated with each peripheral, otherwise we don't know who to send packets to. > >>>> +struct mipi_dsi_device { > >>>> + char name[MIPI_DSI_NAME_SIZE]; > >>>> + int id; > >>>> + struct device dev; > >>>> + > >>>> + const struct mipi_dsi_device_id *id_entry; > >>>> + struct mipi_dsi_bus *bus; > >>>> + struct videomode vm; > >>> Why is the videomode part of this structure? What if a device supports > >>> more than a single mode? > >> This is the current video mode. If we want to change mode, > >> we call: > >> ops->set_stream(false); > >> dev->vm =...new mode > >> ops->set_stream(true); > > I don't think that needs to be part of the DSI device. Rather it seems > > to me that whatever object type is implemented by the DSI device driver > > should have subsystem-specific code to do this. > > > > For example, if a DSI device is a bridge, then it will implement a > > drm_bridge object, and in that case the drm_bridge_funcs.mode_set() > > would be the equivalent of this. > I think mode setting is common for most DSI-hosts, at least > for all having video mode. I agree, but I don't think we need to make it an integral part of the DSI peripheral or DSI host structures. Most of the drivers for a DSI host or DSI peripheral will have some subsystem specific way to deal with that anyway, so I don't think we're doing us a favour by adding an extra layer here. > >>>> +#define mipi_dsi_dcs_write_seq(dev, channel, seq...) \ > >>>> +({\ > >>>> + const u8 d[] = { seq };\ > >>>> + BUILD_BUG_ON_MSG(ARRAY_SIZE(d) > 64, "DCS sequence too long for stack");\ > >>>> + mipi_dsi_dcs_write(dev, channel, d, ARRAY_SIZE(d));\ > >>>> +}) > >>>> + > >>>> +#define mipi_dsi_dcs_write_static_seq(dev, channel, seq...) \ > >>>> +({\ > >>>> + static const u8 d[] = { seq };\ > >>>> + mipi_dsi_dcs_write(dev, channel, d, ARRAY_SIZE(d));\ > >>>> +}) > >>> Can we drop these please? I'd rather have drivers construct buffers > >>> explicitly than use these. > >> I like those two. It removes lots of boiler-plate code. Please compare > >> implementations > >> of s6e8aa panel drivers without it [1] and with it [2], look for > >> calls to dsi_dcs_write. > >> > >> [1] http://lists.freedesktop.org/archives/dri-devel/2013-January/034212.html > >> [2] https://linuxtv.org/patch/20215/ > > Actually I much prefer the first version that doesn't use the macros. > > It's much more explicit and therefore obvious what's happening. I can > > understand that these might be convenient for some use-cases, but I > > don't think that warrants them being part of the core. You can always > > add them in specific drivers if you really want to. Once more people > > start using this infrastructure and these macros start to appear as a > > common pattern we can still move them into the core header to avoid the > > duplication. > Hmm, > grep -rP '--include=*.h' '\.\.\.\)' > shows lots of similar macros :) The large majority of those are wrappers around printk(), so they have a completely different purpose. > Anyway I can move it to driver :) I agree that it can be useful to have shortcuts to common operations, and I'm open to move these back into core headers if indeed they prove to be the best way to do that. > > Anyway, it seems like there are still a few things that need discussion, > > so in an attempt to push this forward perhaps a good idea would be to > > drop all the contentious parts and focus on getting the basic infra- > > structure to work. The crucial parts will be to have a standard way of > > instantiating devices from device tree. As far as I can tell, the only > > disagreement we have on that matter is whether or not to name the > > devices according to their bus number. I hope I've been able to convince > > you above. > > > > Do you think it would be possible to remove the .set_stream() and > > .transfer() operations (and related defines) for now? I'm not asking for > > you to drop them, just to move them to a separate patch so that the > > basic infrastructure patch can be merged early and we can get started to > > port drivers to use this as soon as possible. > I would like to have DSI bus and working DSI host and panel, so > I would like to replace set_stream at least with set_mode. > > I hope to send next iteration at the beginning of the next week. Okay, I think I could live with those if they are modified as discussed so far. But I really think we should get rid of the struct videomode in struct mipi_dsi_device and encode the virtual channels in device names. Thierry
On 2013-11-27 12:54, Thierry Reding wrote: >> I am not sure about hardwiring devices to virtual channels. >> There could be devices which uses more than one virtual channel, >> in fact exynos-drm docs suggests that such hardware exists. > > In that case, why not make them two logically separate devices within > the kernel. We need the channel number so that the device can be > addressed in the first place, so I don't see what's wrong with using > that number in the device's name. > > The whole point of this patch is to add MIPI DSI bus infrastructure, and > the virtual channel is one of the fundamental aspects of that bus, so I > think we need to make it an integral part of the implementation. (I speak here more in the context of OMAP display subsystem and CDF, and this might not be totally applicable to DRM). In my opinion, DSI shouldn't be though of in the same way as other buses. In most of the cases, there's just one DSI peripheral connected. This peripheral may answer to multiple DSI VC IDs, though. I don't like the idea of having to split one device driver into multiple drivers, just to manage multiple DSI VC IDs. In some rare cases (I've never seen one in production) there may be a DSI hub, and one or two DSI peripherals behind it. But the hub is not really a hub, but a router, and the router requires configuration. The case here is not really one DSI bus with two or more peripherals, but two or more independent 1-to-1 DSI buses. I have never seen a pure hub, i.e. something that would just forward/broadcast the DSI packet to two or more DSI peripherals. So I think we should consider DSI as a one-to-one link, and let the DSI peripheral manage the VC IDs as it wants. I don't see any benefit in trying to create a similar linux bus for DSI as we have for, say, i2c or spi. >> I can also imagine scenarios when dynamic virtual channel (re-)association >> can be useful and DSI specs are not against it AFAIK. > > How is that going to work? There's no hotplug support or similar in DSI, > so why would anyone want to reassign virtual channels. > > Supposing even that we wanted to support this eventually, I think a more > appropriate solution would be to completely remove the device and add a > new one, because that also takes care of keeping the channel number > embedded within the struct mipi_dsi_device up to date. > >> reg property means device is at fixed virtual channel. >> DSI specs says nothing about it IMHO. > > Without that fixed virtual channel number we can't use the device in the > first place. How are you going to address the device if you don't know > its virtual channel? The DSI peripheral driver must know the VC IDs. Often they are hardcoded values, and they can be defined in the driver code. I don't see why the DSI host driver would need to know the VC IDs, as it can't really do anything independently with the peripheral anyway. All the transactions should be started by the DSI peripheral driver. Tomi
On Thu, Dec 05, 2013 at 04:37:39PM +0200, Tomi Valkeinen wrote: > On 2013-11-27 12:54, Thierry Reding wrote: > > >> I am not sure about hardwiring devices to virtual channels. > >> There could be devices which uses more than one virtual channel, > >> in fact exynos-drm docs suggests that such hardware exists. > > > > In that case, why not make them two logically separate devices within > > the kernel. We need the channel number so that the device can be > > addressed in the first place, so I don't see what's wrong with using > > that number in the device's name. > > > > The whole point of this patch is to add MIPI DSI bus infrastructure, and > > the virtual channel is one of the fundamental aspects of that bus, so I > > think we need to make it an integral part of the implementation. > > (I speak here more in the context of OMAP display subsystem and CDF, and > this might not be totally applicable to DRM). > > In my opinion, DSI shouldn't be though of in the same way as other buses. > > In most of the cases, there's just one DSI peripheral connected. This > peripheral may answer to multiple DSI VC IDs, though. I don't like the > idea of having to split one device driver into multiple drivers, just to > manage multiple DSI VC IDs. If they respond to multiple VCs, then I suppose they would be logically separate devices, even if only a single physical device. What would be the point of addressing them individually if they are just the same device? > In some rare cases (I've never seen one in production) there may be a > DSI hub, and one or two DSI peripherals behind it. But the hub is not > really a hub, but a router, and the router requires configuration. The > case here is not really one DSI bus with two or more peripherals, but > two or more independent 1-to-1 DSI buses. From the CPUs perspective there's still only one bus. Or perhaps to put it another way, there is a single address space, because that's what's implied by how the virtual channel number is defined. So you would still represent the DSI hub as one device with a specific VC and the two peripherals behind it with different VC numbers. The VC number space simply doesn't allow much else to be done. > I have never seen a pure hub, i.e. something that would just > forward/broadcast the DSI packet to two or more DSI peripherals. > > So I think we should consider DSI as a one-to-one link, and let the DSI > peripheral manage the VC IDs as it wants. But doing so would prevent us from supporting setups where we have two separate peripherals with different VC numbers. > I don't see any benefit in trying to create a similar linux bus for > DSI as we have for, say, i2c or spi. > > Without that fixed virtual channel number we can't use the device in the > > first place. How are you going to address the device if you don't know > > its virtual channel? > > The DSI peripheral driver must know the VC IDs. Often they are hardcoded > values, and they can be defined in the driver code. When you say "often", can you make a guarantee that it will always be the case? I don't think so. I can easily imagine somebody making the VC configurable via straps, which would come in handy if you wanted to use the same IC multiple times within the same design. In that case you still need some way to specify the VC on a per-board basis, either in DT or "platform data". > I don't see why the DSI host driver would need to know the VC IDs, as > it can't really do anything independently with the peripheral anyway. > All the transactions should be started by the DSI peripheral driver. There are things that are standardized in DSI. The core could for instance try to probe peripherals by reading the DDB. But even that aside, we'll still need some way to store a device's VC number somewhere. Even if the DSI host doesn't have any explicit knowledge about it, you still need to pass it along when you want to send a message to a device. So when you start a transaction from within a device driver, you still need to get the VC number from somewhere. The proposal is to store that number in struct mipi_dsi_device. It's a logical choice because it is something that characterises a device. It is also a property of every device, so by storing it within a common structure gives drivers a standard way of accessing it instead of having each driver come up with it's own way to store it. Thierry
On 2013-12-06 14:54, Thierry Reding wrote: > On Thu, Dec 05, 2013 at 04:37:39PM +0200, Tomi Valkeinen wrote: >> On 2013-11-27 12:54, Thierry Reding wrote: >> >>>> I am not sure about hardwiring devices to virtual channels. >>>> There could be devices which uses more than one virtual channel, >>>> in fact exynos-drm docs suggests that such hardware exists. >>> >>> In that case, why not make them two logically separate devices within >>> the kernel. We need the channel number so that the device can be >>> addressed in the first place, so I don't see what's wrong with using >>> that number in the device's name. >>> >>> The whole point of this patch is to add MIPI DSI bus infrastructure, and >>> the virtual channel is one of the fundamental aspects of that bus, so I >>> think we need to make it an integral part of the implementation. >> >> (I speak here more in the context of OMAP display subsystem and CDF, and >> this might not be totally applicable to DRM). >> >> In my opinion, DSI shouldn't be though of in the same way as other buses. >> >> In most of the cases, there's just one DSI peripheral connected. This >> peripheral may answer to multiple DSI VC IDs, though. I don't like the >> idea of having to split one device driver into multiple drivers, just to >> manage multiple DSI VC IDs. > > If they respond to multiple VCs, then I suppose they would be logically > separate devices, even if only a single physical device. What would be > the point of addressing them individually if they are just the same > device? No idea. But I have worked with a device, that used VC0 for the device's configuration registers, VC1 for buffer commands (the device had a framebuffer), VC2 and VC3 for panels connected to that device. Of course, that kind of devices are probably (?) rare, so it may not be a big issue. >> In some rare cases (I've never seen one in production) there may be a >> DSI hub, and one or two DSI peripherals behind it. But the hub is not >> really a hub, but a router, and the router requires configuration. The >> case here is not really one DSI bus with two or more peripherals, but >> two or more independent 1-to-1 DSI buses. > > From the CPUs perspective there's still only one bus. Or perhaps to put > it another way, there is a single address space, because that's what's > implied by how the virtual channel number is defined. > > So you would still represent the DSI hub as one device with a specific > VC and the two peripherals behind it with different VC numbers. The VC > number space simply doesn't allow much else to be done. > >> I have never seen a pure hub, i.e. something that would just >> forward/broadcast the DSI packet to two or more DSI peripherals. >> >> So I think we should consider DSI as a one-to-one link, and let the DSI >> peripheral manage the VC IDs as it wants. > > But doing so would prevent us from supporting setups where we have two > separate peripherals with different VC numbers. No it wouldn't. We could still communicate with the extra peripherals via the hub device. What I was trying to say is that we shouldn't think or model DSI with multiple devices as multiple devices connected to the same DSI bus. Instead, it should be seen as a tree of one-to-one links (as it is in the HW). For the sake of discussion, let's consider a simple DSI hub setup: SoC DSI -> DSI Hub -> DSI panel 1 -> DSI panel 2 The hub would use, say VC0 for hub configuration, and it'd route VC2 to panel 1 and VC3 to panel2. Both panels would use VC0, so the hub would translate the VC ID accordingly. How would you represent this in Linux? How about the case where the DSI hub uses i2c for control (DSI used only for video data), and the messages to DSI panels are sent via i2c? >> I don't see any benefit in trying to create a similar linux bus for >> DSI as we have for, say, i2c or spi. > >>> Without that fixed virtual channel number we can't use the device in the >>> first place. How are you going to address the device if you don't know >>> its virtual channel? >> >> The DSI peripheral driver must know the VC IDs. Often they are hardcoded >> values, and they can be defined in the driver code. > > When you say "often", can you make a guarantee that it will always be > the case? I don't think so. I can easily imagine somebody making the VC > configurable via straps, which would come in handy if you wanted to use > the same IC multiple times within the same design. Of course I can't guarantee things like that. But I have never seen a DSI device that can be configured in such a way. Have you? > In that case you still need some way to specify the VC on a per-board > basis, either in DT or "platform data". I'm not saying we should prevent that kind of device from working. But if all the known devices use a fixed VC ID scheme, maybe there's not much point in requiring to enter the VC ID manually in all the dts files. If there's a device with configurable VC ID, it can still be supported by custom DT properties. >> I don't see why the DSI host driver would need to know the VC IDs, as >> it can't really do anything independently with the peripheral anyway. >> All the transactions should be started by the DSI peripheral driver. > > There are things that are standardized in DSI. The core could for > instance try to probe peripherals by reading the DDB. That's not part of DSI. It's part of DCS. Not all devices support DCS, or they may support only some parts of it. Also the devices usually need regulator setup, resetting via gpios, or other similar setup which makes automatic probing not an option. > But even that aside, we'll still need some way to store a device's VC > number somewhere. Even if the DSI host doesn't have any explicit > knowledge about it, you still need to pass it along when you want to > send a message to a device. So when you start a transaction from within > a device driver, you still need to get the VC number from somewhere. Yep. > The proposal is to store that number in struct mipi_dsi_device. It's a > logical choice because it is something that characterises a device. It > is also a property of every device, so by storing it within a common > structure gives drivers a standard way of accessing it instead of having > each driver come up with it's own way to store it. Yep, that sounds fine to me if mipi_dsi_device is not a linux device (but it is in this patch). I'll try to summarize my view on mipi dsi: DSI is a video bus. It can be used for control, but it's main purpose is as a video data bus. Video has rather strict timing requirements, which means control messages have to be done in a controlled manner. DSI devices quite often have another control bus, usually i2c. I guess the reason to support i2c in addition to DSI is the timing restrictions on the DSI control bus. If the DSI device is represented as a linux DSI device on a linux DSI bus, supporting the i2c is difficult. It is possible to have multiple DSI devices behind a single DSI connection, but it is not a generic solution, and requires one to use the DSI peripherals in a very controlled manner to make sure no DSI device is blocking the other ones for too long. I have not seen these in production, and while I'm just guessing, the reason may be that is so difficult to make them work well. The only standard part of DSI, in my experience, is the DSI packet layer (maybe there's a better term for it). Anything else is often custom. For this reason I think the DSI peripheral driver has to be in full control of the device, and the DSI bus cannot really do anything by itself. There's not much "bus" in DSI in my opinion. No probing, one-to-one links, very limited "address space", all the cases I know have just one DSI device... So... While having a Linux DSI bus, etc. would feel elegant and nice, I just feel it's not easy and not worth it. What we have in omapdss is far from perfect, but it has been working quite nicely. DSI is considered just as a data bus, with extra functionality for sending control messages. A DSI device is either a platform device if it requires no control or the control is done via DSI, or it's a device of the control bus like i2c. All that said, I may be mentally stuck in the old models I've been using for a long time, so maybe a different approach for DSI is good. We just need to make sure the existing devices can be supported. Tomi
On Mon, Dec 09, 2013 at 01:34:06PM +0200, Tomi Valkeinen wrote: > On 2013-12-06 14:54, Thierry Reding wrote: > > On Thu, Dec 05, 2013 at 04:37:39PM +0200, Tomi Valkeinen wrote: > >> On 2013-11-27 12:54, Thierry Reding wrote: > >> > >>>> I am not sure about hardwiring devices to virtual channels. > >>>> There could be devices which uses more than one virtual channel, > >>>> in fact exynos-drm docs suggests that such hardware exists. > >>> > >>> In that case, why not make them two logically separate devices within > >>> the kernel. We need the channel number so that the device can be > >>> addressed in the first place, so I don't see what's wrong with using > >>> that number in the device's name. > >>> > >>> The whole point of this patch is to add MIPI DSI bus infrastructure, and > >>> the virtual channel is one of the fundamental aspects of that bus, so I > >>> think we need to make it an integral part of the implementation. > >> > >> (I speak here more in the context of OMAP display subsystem and CDF, and > >> this might not be totally applicable to DRM). > >> > >> In my opinion, DSI shouldn't be though of in the same way as other buses. > >> > >> In most of the cases, there's just one DSI peripheral connected. This > >> peripheral may answer to multiple DSI VC IDs, though. I don't like the > >> idea of having to split one device driver into multiple drivers, just to > >> manage multiple DSI VC IDs. > > > > If they respond to multiple VCs, then I suppose they would be logically > > separate devices, even if only a single physical device. What would be > > the point of addressing them individually if they are just the same > > device? > > No idea. But I have worked with a device, that used VC0 for the device's > configuration registers, VC1 for buffer commands (the device had a > framebuffer), VC2 and VC3 for panels connected to that device. Well, VC2 and VC3 certainly sound like logically separate devices to me. If they are only connected to the device it sounds like they aren't part of the device at all. One could even argue that a device's configuration and the framebuffer are logically separate and therefore it wouldn't be a problem to address them as separate devices. > >> So I think we should consider DSI as a one-to-one link, and let the DSI > >> peripheral manage the VC IDs as it wants. > > > > But doing so would prevent us from supporting setups where we have two > > separate peripherals with different VC numbers. > > No it wouldn't. We could still communicate with the extra peripherals > via the hub device. What I was trying to say is that we shouldn't think > or model DSI with multiple devices as multiple devices connected to the > same DSI bus. Instead, it should be seen as a tree of one-to-one links > (as it is in the HW). But even if you have a tree of one-to-one links, you still need some way to address the individual nodes in the tree. The VC ID is the only way by which you can address a node. I don't see how you can possible send packets to more than one node if you keep sending packets to the same address. Where does the missing information magically come from? > For the sake of discussion, let's consider a simple DSI hub setup: > > SoC DSI -> DSI Hub -> DSI panel 1 > -> DSI panel 2 > > The hub would use, say VC0 for hub configuration, and it'd route VC2 to > panel 1 and VC3 to panel2. Both panels would use VC0, so the hub would > translate the VC ID accordingly. > > How would you represent this in Linux? You keep saying that devices use various VC IDs (VC0 for hub config, VC2 and VC3 for panels 1 and 2 in this case). But those are exactly the addresses. You've got to have some way within the kernel to store those. Given the limited address space of DSI there's no way to accurately represent the hierarchy of the above in the bus/device numbering. But that doesn't mean you can't assign addresses (VC IDs) to the devices. In fact you've given examples yourself. > How about the case where the DSI hub uses i2c for control (DSI used > only for video data), and the messages to DSI panels are sent via i2c? Well you still have to send the video data somewhere, right? Each video packet still has a destination VC ID, independent of whether that same bus is used for configuration or not. Of course it's more difficult to represent these devices than if they were accessed using only a single bus. Perhaps it could help to look at Linux devices not so much as the equivalent of a physical device, but rather as a representation of the logical functionality of that device. In your example above, the DSI hub would be a single physical device. But it also has two logically separate functions. It can be configured and it passes through video to one or more panels. Similarily for the panels, where each has an I2C control input and a DSI video input. The I2C portion can be a device separate from the DSI portion. That way your problem becomes one of interconnecting two "devices". You could for instance pass a handle to the control portion to the DSI portion and let the driver for that handle control via I2C. In DT terms that would be something like this: i2c { dsihub: dsi-hub@2f { }; panel1: panel@30 { }; panel2: panel@31 { }; }; dsi { hub@0 { control = <&dsihub>; }; panel@2 { control = <&panel1>; }; panel@3 { control = <&panel2>; }; }; > >> The DSI peripheral driver must know the VC IDs. Often they are hardcoded > >> values, and they can be defined in the driver code. > > > > When you say "often", can you make a guarantee that it will always be > > the case? I don't think so. I can easily imagine somebody making the VC > > configurable via straps, which would come in handy if you wanted to use > > the same IC multiple times within the same design. > > Of course I can't guarantee things like that. But I have never seen a > DSI device that can be configured in such a way. Have you? I've seen plenty of I2C devices that do so. The reason is usually so that the same IC can be used on the same bus multiple times. Why would DSI be any different? If in your example above, panel 1 and panel 2 are identical panels, they would use the same hardcoded VC IDs, right? So you couldn't address them separately. In such a case, if I were the panel vendor I'd make the VC ID selectable (via straps for example). No, I've never seen a DSI device like it. But I still think that we should be prepared, especially if it doesn't add any significant overhead. Every other bus with potentially multiple peripheral needs addressing and so does DSI. Why wouldn't we want to associate that address with a device on that bus? > > In that case you still need some way to specify the VC on a per-board > > basis, either in DT or "platform data". > > I'm not saying we should prevent that kind of device from working. But > if all the known devices use a fixed VC ID scheme, maybe there's not > much point in requiring to enter the VC ID manually in all the dts > files. If there's a device with configurable VC ID, it can still be > supported by custom DT properties. But we loose any possibility of ever making that code generic. Even if the DSI specification doesn't go into the details (it even explicitly chickens out at that point), it still defines that each DSI bus can have up to 4 peripherals. So by definition the 2 bit VC is something that every DSI host and every DSI peripheral needs to support. There is no reason to represent it using custom properties. It is completely generic. > > The proposal is to store that number in struct mipi_dsi_device. It's a > > logical choice because it is something that characterises a device. It > > is also a property of every device, so by storing it within a common > > structure gives drivers a standard way of accessing it instead of having > > each driver come up with it's own way to store it. > > Yep, that sounds fine to me if mipi_dsi_device is not a linux device > (but it is in this patch). What's that got to do with anything? > I'll try to summarize my view on mipi dsi: > > DSI is a video bus. It can be used for control, but it's main purpose is > as a video data bus. Video has rather strict timing requirements, which > means control messages have to be done in a controlled manner. > > DSI devices quite often have another control bus, usually i2c. I guess > the reason to support i2c in addition to DSI is the timing restrictions > on the DSI control bus. If the DSI device is represented as a linux DSI > device on a linux DSI bus, supporting the i2c is difficult. The same is true of the reverse. If the DSI device is represented as a Linux I2C device on a Linux I2C bus, supporting the DSI is difficult. That's just something we have to deal with. > It is possible to have multiple DSI devices behind a single DSI > connection, but it is not a generic solution, and requires one to use > the DSI peripherals in a very controlled manner to make sure no DSI > device is blocking the other ones for too long. I have not seen these in > production, and while I'm just guessing, the reason may be that is so > difficult to make them work well. > > The only standard part of DSI, in my experience, is the DSI packet layer > (maybe there's a better term for it). Anything else is often custom. For > this reason I think the DSI peripheral driver has to be in full control > of the device, and the DSI bus cannot really do anything by itself. The same is true of I2C. There's no probing or standard registers across devices. But it's still a bus that can have multiple peripherals and a way to address them. Every peripheral has an associated device, which drivers can use to have the I2C master send messages to the correct peripheral. > There's not much "bus" in DSI in my opinion. No probing, one-to-one > links, There are one-to-many links as well. You've in fact provided an example of one such device above to prove another point. > very limited "address space", all the cases I know have just one > DSI device... Most of that's true for I2C or SPI as well. SPI even needs sideband signalling to address more than a single slave. All the case that I know have only a single DSI device as well. I'm beginning to wonder why I even bother with all this. > So... While having a Linux DSI bus, etc. would feel elegant and nice, I > just feel it's not easy and not worth it. Really the only reason why I've been pushing for this is because Laurent wanted me to represent DSI panels as children of a DSI bus in DT for the simple-panel bindings. The only solution to do this somewhat generically is to have a DSI bus binding and that sort of implies a DSI bus implementation. The particular use-case that I care about works without any of this as well. > What we have in omapdss is far from perfect, but it has been working > quite nicely. DSI is considered just as a data bus, with extra > functionality for sending control messages. A DSI device is either a > platform device if it requires no control or the control is done via > DSI, or it's a device of the control bus like i2c. So in fact you do consider DSI a control *and* data bus, but you're side-stepping the issue by hiding things within the drivers. But you still need to hook up the DSI device to an I2C master and a DSI host before you can use it. The reason why you can hide that is probably because it isn't generic and you couldn't reuse the peripheral DSI drivers on a different DSI host. > All that said, I may be mentally stuck in the old models I've been using > for a long time, so maybe a different approach for DSI is good. We just > need to make sure the existing devices can be supported. I don't see how other devices would be broken. For one you can easily keep your existing code. We're adding new API here so there's no need at all to migrate to it and no way for it to break existing functionality. But it will obviously be some work to move to a generic "framework", if you can call it that. The driver will likely need some major rewrite, which I think will be the case anyway because this will be a DRM API and you'll have to move to DRM to use it. Thierry
On 2013-12-09 15:10, Thierry Reding wrote: >> No idea. But I have worked with a device, that used VC0 for the device's >> configuration registers, VC1 for buffer commands (the device had a >> framebuffer), VC2 and VC3 for panels connected to that device. > > Well, VC2 and VC3 certainly sound like logically separate devices to me. > If they are only connected to the device it sounds like they aren't part > of the device at all. Yes, the two panels are obviously separate devices. > One could even argue that a device's configuration and the framebuffer > are logically separate and therefore it wouldn't be a problem to address > them as separate devices. True. But one could also argue that handling them with separate linux devices just causes unnecessary complexity. >>>> So I think we should consider DSI as a one-to-one link, and let the DSI >>>> peripheral manage the VC IDs as it wants. >>> >>> But doing so would prevent us from supporting setups where we have two >>> separate peripherals with different VC numbers. >> >> No it wouldn't. We could still communicate with the extra peripherals >> via the hub device. What I was trying to say is that we shouldn't think >> or model DSI with multiple devices as multiple devices connected to the >> same DSI bus. Instead, it should be seen as a tree of one-to-one links >> (as it is in the HW). > > But even if you have a tree of one-to-one links, you still need some way > to address the individual nodes in the tree. The VC ID is the only way > by which you can address a node. I don't see how you can possible send > packets to more than one node if you keep sending packets to the same > address. Where does the missing information magically come from? From the DSI hub. In the example case below, let's say the DSI panel 1 driver is told to send a configuration packet to the panel. The panel driver would call a dsi_transmit op, giving as arguments the VC number 0, and the packet data. That call would go to the DSI hub driver. It knows how it routes the packets (the routing configuration is either hardcoded or passed via DT data), and then calls dsi_transmit op on SoC DSI, with VC number 2 and the packet data. >> For the sake of discussion, let's consider a simple DSI hub setup: >> >> SoC DSI -> DSI Hub -> DSI panel 1 >> -> DSI panel 2 >> >> The hub would use, say VC0 for hub configuration, and it'd route VC2 to >> panel 1 and VC3 to panel2. Both panels would use VC0, so the hub would >> translate the VC ID accordingly. >> >> How would you represent this in Linux? > > You keep saying that devices use various VC IDs (VC0 for hub config, VC2 > and VC3 for panels 1 and 2 in this case). But those are exactly the > addresses. You've got to have some way within the kernel to store those. > > Given the limited address space of DSI there's no way to accurately > represent the hierarchy of the above in the bus/device numbering. But > that doesn't mean you can't assign addresses (VC IDs) to the devices. In > fact you've given examples yourself. Yes, but I guess the difference in our views is that I see the VC IDs as "link local" and routing is done by the hubs as they see fit. In other words, if I'm not mistaken, you'd have a Linux DSI bus with three devices in the bus, each having its own VC ID, whereas I'd have a DSI "link" between the SoC and the hub, without any general information about the VC IDs used, and two additional DSI links, from the DSI hub to each panel. >> How about the case where the DSI hub uses i2c for control (DSI used >> only for video data), and the messages to DSI panels are sent via i2c? > > Well you still have to send the video data somewhere, right? Each video > packet still has a destination VC ID, independent of whether that same > bus is used for configuration or not. Yes, sure. And actually the DSI could also be used for control, in addition to i2c. DSI is much faster, so it could be used to send tables or such, but i2c would be used later for backlight and other similar adjustments. > Of course it's more difficult to represent these devices than if they > were accessed using only a single bus. Perhaps it could help to look at > Linux devices not so much as the equivalent of a physical device, but > rather as a representation of the logical functionality of that device. > > In your example above, the DSI hub would be a single physical device. > But it also has two logically separate functions. It can be configured > and it passes through video to one or more panels. Similarily for the > panels, where each has an I2C control input and a DSI video input. > > The I2C portion can be a device separate from the DSI portion. That way > your problem becomes one of interconnecting two "devices". You could for > instance pass a handle to the control portion to the DSI portion and let > the driver for that handle control via I2C. In DT terms that would be > something like this: > > i2c { > dsihub: dsi-hub@2f { > }; > > panel1: panel@30 { > }; > > panel2: panel@31 { > }; > }; > > dsi { > hub@0 { > control = <&dsihub>; > }; > > panel@2 { > control = <&panel1>; > }; > > panel@3 { > control = <&panel2>; > }; > }; Hmm, well. The panels here should should be inside the hub node for DSI case. They are connected to the hub, not the SoC's DSI, and need the hub to be powered up and configured to work. And the hub should contain two output DSI busses, each having one panel with VC ID 0. Probably something like: dsi { hub@0 { dsi@0 { panel@0 { }; }; dsi@1 { panel@0 { }; }; }; }; That kind of ruins the idea of representing the panels as DSI devices with VC IDs of 2 and 3. I don't know how that should be managed then... But this DT example shows how DSI is really a one-to-one bus in the hardware. >>>> The DSI peripheral driver must know the VC IDs. Often they are hardcoded >>>> values, and they can be defined in the driver code. >>> >>> When you say "often", can you make a guarantee that it will always be >>> the case? I don't think so. I can easily imagine somebody making the VC >>> configurable via straps, which would come in handy if you wanted to use >>> the same IC multiple times within the same design. >> >> Of course I can't guarantee things like that. But I have never seen a >> DSI device that can be configured in such a way. Have you? > > I've seen plenty of I2C devices that do so. The reason is usually so > that the same IC can be used on the same bus multiple times. Why would > DSI be any different? If in your example above, panel 1 and panel 2 are > identical panels, they would use the same hardcoded VC IDs, right? So > you couldn't address them separately. In such a case, if I were the > panel vendor I'd make the VC ID selectable (via straps for example). With DSI you always have a hub, which can translate the addresses. With my example above, both panels are identical and they have the same VC ID of 0. > No, I've never seen a DSI device like it. But I still think that we > should be prepared, especially if it doesn't add any significant > overhead. Every other bus with potentially multiple peripheral needs > addressing and so does DSI. Why wouldn't we want to associate that > address with a device on that bus? > >>> In that case you still need some way to specify the VC on a per-board >>> basis, either in DT or "platform data". >> >> I'm not saying we should prevent that kind of device from working. But >> if all the known devices use a fixed VC ID scheme, maybe there's not >> much point in requiring to enter the VC ID manually in all the dts >> files. If there's a device with configurable VC ID, it can still be >> supported by custom DT properties. > > But we loose any possibility of ever making that code generic. Even if > the DSI specification doesn't go into the details (it even explicitly > chickens out at that point), it still defines that each DSI bus can have > up to 4 peripherals. So by definition the 2 bit VC is something that > every DSI host and every DSI peripheral needs to support. There is no > reason to represent it using custom properties. It is completely > generic. Well, afaik, the VC ID is really just data in the packet. There's nothing for the HW to support. I mean, the DSI host hardware doesn't need to understand anything about the VC ID. This is the case on OMAP, the hardware has no configuration related to the VC ID. Maybe other hardware does. Of course, that's no reason not to make VC handling generic if it makes sense otherwise to do so. >>> The proposal is to store that number in struct mipi_dsi_device. It's a >>> logical choice because it is something that characterises a device. It >>> is also a property of every device, so by storing it within a common >>> structure gives drivers a standard way of accessing it instead of having >>> each driver come up with it's own way to store it. >> >> Yep, that sounds fine to me if mipi_dsi_device is not a linux device >> (but it is in this patch). > > What's that got to do with anything? See below. A linux device implies a Linux DSI bus, and I don't agree that having a DSI bus is a good thing. >> I'll try to summarize my view on mipi dsi: >> >> DSI is a video bus. It can be used for control, but it's main purpose is >> as a video data bus. Video has rather strict timing requirements, which >> means control messages have to be done in a controlled manner. >> >> DSI devices quite often have another control bus, usually i2c. I guess >> the reason to support i2c in addition to DSI is the timing restrictions >> on the DSI control bus. If the DSI device is represented as a linux DSI >> device on a linux DSI bus, supporting the i2c is difficult. > > The same is true of the reverse. If the DSI device is represented as a > Linux I2C device on a Linux I2C bus, supporting the DSI is difficult. > That's just something we have to deal with. No it's not, if DSI is not considered as a full blown control bus, but just a data bus. We (omapdss and CDF at least) anyway need a method to deal with the video data paths. I.e. define that this component outputs video data which goes to that component, and have some kind of API to control that particular part of the whole video pipeline. So we have that for DSI, and if the DSI panel is controlled via i2c, the i2c device can easily use that "video data API" to manage the DSI side. >> It is possible to have multiple DSI devices behind a single DSI >> connection, but it is not a generic solution, and requires one to use >> the DSI peripherals in a very controlled manner to make sure no DSI >> device is blocking the other ones for too long. I have not seen these in >> production, and while I'm just guessing, the reason may be that is so >> difficult to make them work well. >> >> The only standard part of DSI, in my experience, is the DSI packet layer >> (maybe there's a better term for it). Anything else is often custom. For >> this reason I think the DSI peripheral driver has to be in full control >> of the device, and the DSI bus cannot really do anything by itself. > > The same is true of I2C. There's no probing or standard registers across > devices. But it's still a bus that can have multiple peripherals and a > way to address them. Every peripheral has an associated device, which > drivers can use to have the I2C master send messages to the correct > peripheral. Right, but with i2c, the all the devices are actually connected to the host. That is not the case with DSI. It is normal to have multiple i2c devices on a bus. It's rare (never seen such) to have multiple DSI devices behind one DSI connector. >> There's not much "bus" in DSI in my opinion. No probing, one-to-one >> links, > > There are one-to-many links as well. You've in fact provided an example > of one such device above to prove another point. No, my example has three one-to-one links. The physical DSI buses are always one-to-one. Maybe we could represent DSI as both a physical one-to-one link tree, matching the hardware, and a logical bus, matching how it's used. But that starts to sound horribly complex for a one-to-one video bus, when all the known boards have just a single DSI device, and supporting multiple devices would be very tricky in any case. >> very limited "address space", all the cases I know have just one >> DSI device... > > Most of that's true for I2C or SPI as well. SPI even needs sideband > signalling to address more than a single slave. At least the i2c address space is huge compared to DSI =). And it's common to have multiple i2c or spi devices behind one bus. And all the i2c and spi devices are actually connected to the host, which is not the case with DSI. > All the case that I know have only a single DSI device as well. I'm > beginning to wonder why I even bother with all this. I have similar thoughts. I haven't even seen new DSI devices used on the platforms I work on for some time, I guess DSI is out of fashion. So don't consider me arguing here as "nack", I just want to bring out the issues and thoughts I've encountered with DSI. >> So... While having a Linux DSI bus, etc. would feel elegant and nice, I >> just feel it's not easy and not worth it. > > Really the only reason why I've been pushing for this is because Laurent > wanted me to represent DSI panels as children of a DSI bus in DT for the > simple-panel bindings. The only solution to do this somewhat generically > is to have a DSI bus binding and that sort of implies a DSI bus > implementation. I also think it's good to have DSI panels as children of the DSI host. But that doesn't mean a bus implementation. You can as well have platform devices as panels, and they may be children of the DSI host. That's what I do on omap. That allows me to describe the HW correctly in the DT data, but keep the linux driver side simple. > The particular use-case that I care about works without any of this as > well. > >> What we have in omapdss is far from perfect, but it has been working >> quite nicely. DSI is considered just as a data bus, with extra >> functionality for sending control messages. A DSI device is either a >> platform device if it requires no control or the control is done via >> DSI, or it's a device of the control bus like i2c. > > So in fact you do consider DSI a control *and* data bus, but you're > side-stepping the issue by hiding things within the drivers. But you > still need to hook up the DSI device to an I2C master and a DSI host > before you can use it. The reason why you can hide that is probably > because it isn't generic and you couldn't reuse the peripheral DSI > drivers on a different DSI host. Hmm, no, I don't see anything omap specific in our implementation. Well, ok, it's omap specific, but the DSI peripheral drivers just use generic DSI ops which could as well be implemented on any other platform. And I don't see it hiding anything. Can you be more specific? If there was a simple way to have a single Linux device sitting on two buses (DSI and I2C), I think it could make sense to have a Linux DSI bus and Linux DSI device. But afaik it's not simple, and as we anyway have a video "link" for DSI which can easily used for the control side, it much simpler not to have the DSI bus at all. Especially as the DSI bus would have just one device anyway. >> All that said, I may be mentally stuck in the old models I've been using >> for a long time, so maybe a different approach for DSI is good. We just >> need to make sure the existing devices can be supported. > > I don't see how other devices would be broken. For one you can easily > keep your existing code. We're adding new API here so there's no need at > all to migrate to it and no way for it to break existing functionality. Right. That's why I said in my original reply "(I speak here more in the context of OMAP display subsystem and CDF, and this might not be totally applicable to DRM).". I didn't mean current drivers would be broken, I meant the new code should have theoretical means to support the known existing hardware. > But it will obviously be some work to move to a generic "framework", if > you can call it that. The driver will likely need some major rewrite, > which I think will be the case anyway because this will be a DRM API and > you'll have to move to DRM to use it. Yep. I think the most important part is to have somewhat sane DT data with all the relevant information. It'd be nice to see an example of that kind of data there would be used with this patch. Tomi
On Mon, Dec 09, 2013 at 05:05:20PM +0200, Tomi Valkeinen wrote: > On 2013-12-09 15:10, Thierry Reding wrote: [...] > > But even if you have a tree of one-to-one links, you still need some way > > to address the individual nodes in the tree. The VC ID is the only way > > by which you can address a node. I don't see how you can possible send > > packets to more than one node if you keep sending packets to the same > > address. Where does the missing information magically come from? > > From the DSI hub. > > In the example case below, let's say the DSI panel 1 driver is told to > send a configuration packet to the panel. The panel driver would call a > dsi_transmit op, giving as arguments the VC number 0, and the packet data. > > That call would go to the DSI hub driver. It knows how it routes the > packets (the routing configuration is either hardcoded or passed via DT > data), and then calls dsi_transmit op on SoC DSI, with VC number 2 and > the packet data. So it is the DSI hub driver that translates VC 0 to VC 2? How does it know that VC 0 should be VC 2 but not VC 3? Does the panel 2 driver pass in a different VC as panel 1? > >> For the sake of discussion, let's consider a simple DSI hub setup: > >> > >> SoC DSI -> DSI Hub -> DSI panel 1 > >> -> DSI panel 2 > >> > >> The hub would use, say VC0 for hub configuration, and it'd route VC2 to > >> panel 1 and VC3 to panel2. Both panels would use VC0, so the hub would > >> translate the VC ID accordingly. > >> > >> How would you represent this in Linux? > > > > You keep saying that devices use various VC IDs (VC0 for hub config, VC2 > > and VC3 for panels 1 and 2 in this case). But those are exactly the > > addresses. You've got to have some way within the kernel to store those. > > > > Given the limited address space of DSI there's no way to accurately > > represent the hierarchy of the above in the bus/device numbering. But > > that doesn't mean you can't assign addresses (VC IDs) to the devices. In > > fact you've given examples yourself. > > Yes, but I guess the difference in our views is that I see the VC IDs as > "link local" and routing is done by the hubs as they see fit. In other > words, if I'm not mistaken, you'd have a Linux DSI bus with three > devices in the bus, each having its own VC ID, whereas I'd have a DSI > "link" between the SoC and the hub, without any general information > about the VC IDs used, and two additional DSI links, from the DSI hub to > each panel. It's really the same thing. If you define VC IDs in a link local manner you don't need them at all. What's the point in having them if you can only reach a single device anyway. From a software perspective we can always only describe devices from the viewpoint where things leave software control. In this case the last point of control is the DSI host. Therefore if we want to send a packet to any device connected to the DSI host (whether directly or through a hub), we need to pass some VC ID to the DSI host as a destination. That number needs to be known up front. So either you hide that information in some software framework, such as you seem to be doing given your example above, and make the panel driver call into the hub driver and have the hub driver translate the VC ID to a different one. The alternative would be to pass the correct address to the DSI host directly. Either way, though, whatever is written into the registers of the DSI host will end up the same, won't it? You always address peripherals from the DSI host's viewpoint. > The panels here should should be inside the hub node for DSI case. They > are connected to the hub, not the SoC's DSI, and need the hub to be > powered up and configured to work. And the hub should contain two output > DSI busses, each having one panel with VC ID 0. Probably something like: > > dsi { > hub@0 { > dsi@0 { > panel@0 { > }; > }; > > dsi@1 { > panel@0 { > }; > }; > }; > }; > > That kind of ruins the idea of representing the panels as DSI devices > with VC IDs of 2 and 3. I don't know how that should be managed then... Why does that ruin it? You still address panel@0 using VC 2 and panel@1 using VC 3, don't you? Therefore I see no reason why they shouldn't be represented as DSI devices with their respective VC IDs. > But this DT example shows how DSI is really a one-to-one bus in the > hardware. How is that one-to-one? The DSI hub connects to two peripherals, that's one-to-two. > >>>> The DSI peripheral driver must know the VC IDs. Often they are hardcoded > >>>> values, and they can be defined in the driver code. > >>> > >>> When you say "often", can you make a guarantee that it will always be > >>> the case? I don't think so. I can easily imagine somebody making the VC > >>> configurable via straps, which would come in handy if you wanted to use > >>> the same IC multiple times within the same design. > >> > >> Of course I can't guarantee things like that. But I have never seen a > >> DSI device that can be configured in such a way. Have you? > > > > I've seen plenty of I2C devices that do so. The reason is usually so > > that the same IC can be used on the same bus multiple times. Why would > > DSI be any different? If in your example above, panel 1 and panel 2 are > > identical panels, they would use the same hardcoded VC IDs, right? So > > you couldn't address them separately. In such a case, if I were the > > panel vendor I'd make the VC ID selectable (via straps for example). > > With DSI you always have a hub, which can translate the addresses. With > my example above, both panels are identical and they have the same VC ID > of 0. No. If they had the same VC ID how do you address them differently? When the transactions originate at the DSI host, you have to tell it where they should go. If you pass them the same VC ID they will go to the same device. They have to. What you say above could only be done if the hub actually had the possibility to initiate transactions of its own. But in that case it becomes a DSI host itself, rather than a simple hub. > >>> In that case you still need some way to specify the VC on a per-board > >>> basis, either in DT or "platform data". > >> > >> I'm not saying we should prevent that kind of device from working. But > >> if all the known devices use a fixed VC ID scheme, maybe there's not > >> much point in requiring to enter the VC ID manually in all the dts > >> files. If there's a device with configurable VC ID, it can still be > >> supported by custom DT properties. > > > > But we loose any possibility of ever making that code generic. Even if > > the DSI specification doesn't go into the details (it even explicitly > > chickens out at that point), it still defines that each DSI bus can have > > up to 4 peripherals. So by definition the 2 bit VC is something that > > every DSI host and every DSI peripheral needs to support. There is no > > reason to represent it using custom properties. It is completely > > generic. > > Well, afaik, the VC ID is really just data in the packet. There's > nothing for the HW to support. I mean, the DSI host hardware doesn't > need to understand anything about the VC ID. This is the case on OMAP, > the hardware has no configuration related to the VC ID. Maybe other > hardware does. Well, at least a hub would need to interpret the VC IDs. Take this example: DSI host --> DSI hub --> peripheral 1 (VC ID 2) | --> peripheral 2 (VC ID 3) If DSI host initiates a transaction addressed at VC 2, the DSI host will receive the packet, parse the VC and forward it to peripheral 1. In the same way, when the DSI hub receives a packet addressed at VC 3, it'll have to forward it to peripheral 2. So a hub would probably need to have some mechanism to configure which downstream port corresponds to which VC ID. Upon initialization you need to program some registers to make the hub do the right thing. > >> It is possible to have multiple DSI devices behind a single DSI > >> connection, but it is not a generic solution, and requires one to use > >> the DSI peripherals in a very controlled manner to make sure no DSI > >> device is blocking the other ones for too long. I have not seen these in > >> production, and while I'm just guessing, the reason may be that is so > >> difficult to make them work well. > >> > >> The only standard part of DSI, in my experience, is the DSI packet layer > >> (maybe there's a better term for it). Anything else is often custom. For > >> this reason I think the DSI peripheral driver has to be in full control > >> of the device, and the DSI bus cannot really do anything by itself. > > > > The same is true of I2C. There's no probing or standard registers across > > devices. But it's still a bus that can have multiple peripherals and a > > way to address them. Every peripheral has an associated device, which > > drivers can use to have the I2C master send messages to the correct > > peripheral. > > Right, but with i2c, the all the devices are actually connected to the > host. That is not the case with DSI. > > It is normal to have multiple i2c devices on a bus. It's rare (never > seen such) to have multiple DSI devices behind one DSI connector. But you can have repeaters and such for I2C as well. Either way, I don't see how the physical connections are related to whether DSI should or should not be modeled as a Linux bus. PCI is modeled as a bus and can easily originate from a single root complex. > >> There's not much "bus" in DSI in my opinion. No probing, one-to-one > >> links, > > > > There are one-to-many links as well. You've in fact provided an example > > of one such device above to prove another point. > > No, my example has three one-to-one links. The physical DSI buses are > always one-to-one. You're splitting hairs. The hub is one-to-many by definition. You have a single connection two the hub and two downstream ports. Even if the wires to the peripherals downstream are separate, there's still logic within the hub to switch packets this way or that. You have an packet stream that's demuxed using the VC ID. > Maybe we could represent DSI as both a physical one-to-one link tree, > matching the hardware, and a logical bus, matching how it's used. We don't have to represent it exactly matching the hardware. What we should be doing is represent it in a way that makes it convenient to use. > But that starts to sound horribly complex for a one-to-one video bus, > when all the known boards have just a single DSI device, and supporting > multiple devices would be very tricky in any case. The original proposal isn't at all complex. It's actually fairly simple. The bulk of the code is boilerplate to allow devices and drivers to be registered. And we don't even need to consider all the tricky cases. I'd very much like to avoid that in fact and rather get something that we can put to use now. > >> So... While having a Linux DSI bus, etc. would feel elegant and nice, I > >> just feel it's not easy and not worth it. > > > > Really the only reason why I've been pushing for this is because Laurent > > wanted me to represent DSI panels as children of a DSI bus in DT for the > > simple-panel bindings. The only solution to do this somewhat generically > > is to have a DSI bus binding and that sort of implies a DSI bus > > implementation. > > I also think it's good to have DSI panels as children of the DSI host. > But that doesn't mean a bus implementation. You can as well have > platform devices as panels, and they may be children of the DSI host. > That's what I do on omap. That allows me to describe the HW correctly in > the DT data, but keep the linux driver side simple. Oh well, I guess you could say the same about I2C and SPI. I don't think we can reach agreement here. > > The particular use-case that I care about works without any of this as > > well. > > > >> What we have in omapdss is far from perfect, but it has been working > >> quite nicely. DSI is considered just as a data bus, with extra > >> functionality for sending control messages. A DSI device is either a > >> platform device if it requires no control or the control is done via > >> DSI, or it's a device of the control bus like i2c. > > > > So in fact you do consider DSI a control *and* data bus, but you're > > side-stepping the issue by hiding things within the drivers. But you > > still need to hook up the DSI device to an I2C master and a DSI host > > before you can use it. The reason why you can hide that is probably > > because it isn't generic and you couldn't reuse the peripheral DSI > > drivers on a different DSI host. > > Hmm, no, I don't see anything omap specific in our implementation. Well, > ok, it's omap specific, but the DSI peripheral drivers just use generic > DSI ops which could as well be implemented on any other platform. > > And I don't see it hiding anything. Can you be more specific? From a quick look what omapdss seems to be doing is allocate and assign virtual channels dynamically at runtime. I suppose that's a good way to solve the issue. It might become an issue if you have devices that have a fixed mapping where you can't program the routing. > If there was a simple way to have a single Linux device sitting on two > buses (DSI and I2C), I think it could make sense to have a Linux DSI bus > and Linux DSI device. But afaik it's not simple, and as we anyway have a > video "link" for DSI which can easily used for the control side, But how do we have that? How is it represented? There is no generic framework that allows this. It's specifically what this patch series is all about. We need a way to instantiate a device and have it hooked up to a DSI host such that drivers can use the DSI host to send commands to the peripherals. Perhaps you have that in omapdss, the rest of us don't have that. We can obviously duplicate what you've done to a large extent, but the goal was to come up with something generic. If generic means that it'll take endless amounts of discussions, I don't think I want generic for now. I'd rather focus on something that works for my use-cases and get back to generic later on. > it much simpler not to have the DSI bus at all. Especially as the DSI > bus would have just one device anyway. You keep saying that, and at the same time you keep bringing forth arguments why it's important to support things such as DSI hubs. Can we settle on one way or another? > > But it will obviously be some work to move to a generic "framework", if > > you can call it that. The driver will likely need some major rewrite, > > which I think will be the case anyway because this will be a DRM API and > > you'll have to move to DRM to use it. > > Yep. I think the most important part is to have somewhat sane DT data > with all the relevant information. It'd be nice to see an example of > that kind of data there would be used with this patch. Well the simplest case, which I think will probably cover about 99% of use-cases would look something like this: /* DSI host */ dsi@54300000 { #address-cells = <1>; #size-cells = <0>; /* DSI peripheral */ panel@0 { reg = <0>; }; }; Which in that case has a hard-coded VC 0 for the panel. Other more complex use-cases could look like this: dsi@54300000 { #address-cells = <1>; #size-cells = <0>; hub@0 { panel@2 { reg = <2>; }; panel@3 { reg = <3>; }; }; }; Shouldn't that be able to cover pretty much any scenario in existence? Thierry
On 2013-12-09 18:10, Thierry Reding wrote: Btw, about single linux device handling multiple VC IDs: I noticed that the DSI spec has an example, in which a DSI peripheral receives interlaced video, and the video packets containing even field have VC ID 0 and packets for odd field have VC ID 1. I'm not sure how relevant interlaced video is, but I think there's an example where having separate linux devices for each VC ID would be somewhat clumsy. >> That call would go to the DSI hub driver. It knows how it routes the >> packets (the routing configuration is either hardcoded or passed via DT >> data), and then calls dsi_transmit op on SoC DSI, with VC number 2 and >> the packet data. > > So it is the DSI hub driver that translates VC 0 to VC 2? How does it > know that VC 0 should be VC 2 but not VC 3? Does the panel 2 driver pass > in a different VC as panel 1? I'm not sure what you mean. The routing information is passed via DT data and then configured to the hub HW, or is hardcoded in the hub hardware. > It's really the same thing. If you define VC IDs in a link local manner > you don't need them at all. What's the point in having them if you can > only reach a single device anyway. You have to define the VC IDs somewhere in link-local manner for the HW to work. The panels in my example respond to VC ID 0, even if from SW (DSI host's) point of view they are addressed with VC ID 2 or 3. Both VC ID 0 and VC IDs 2 and 3 need to be programmed to the hub and possibly to the panel. > From a software perspective we can always only describe devices from the > viewpoint where things leave software control. In this case the last > point of control is the DSI host. Therefore if we want to send a packet > to any device connected to the DSI host (whether directly or through a > hub), we need to pass some VC ID to the DSI host as a destination. That > number needs to be known up front. > > So either you hide that information in some software framework, such as > you seem to be doing given your example above, and make the panel driver > call into the hub driver and have the hub driver translate the VC ID to > a different one. > > The alternative would be to pass the correct address to the DSI host > directly. Either way, though, whatever is written into the registers of > the DSI host will end up the same, won't it? Yes. > You always address peripherals from the DSI host's viewpoint. Yes. Except, of course, if the hub can be accessed with i2c also, in which case you need some other addressing mechanism. >> The panels here should should be inside the hub node for DSI case. They >> are connected to the hub, not the SoC's DSI, and need the hub to be >> powered up and configured to work. And the hub should contain two output >> DSI busses, each having one panel with VC ID 0. Probably something like: >> >> dsi { >> hub@0 { >> dsi@0 { >> panel@0 { >> }; >> }; >> >> dsi@1 { >> panel@0 { >> }; >> }; >> }; >> }; >> >> That kind of ruins the idea of representing the panels as DSI devices >> with VC IDs of 2 and 3. I don't know how that should be managed then... > > Why does that ruin it? You still address panel@0 using VC 2 and panel@1 > using VC 3, don't you? Therefore I see no reason why they shouldn't be > represented as DSI devices with their respective VC IDs. The DT data should reflect the hardware. In this case both panels are addressed by the DSI hub with VC ID 0. That information has to be somewhere, and logical place for it is to define the panels as having VC ID 0. The fact that the DSI host can send a packet to VC ID 3, and the hub routes it to the second panel, is then part of the hub's operation. >> But this DT example shows how DSI is really a one-to-one bus in the >> hardware. > > How is that one-to-one? The DSI hub connects to two peripherals, that's > one-to-two. Well, we keep going around this subject. I'm speaking from the HW perspective. DSI link is a point to point link. Each DSI link is totally independent, having its own speed and configuration. The two output ports on the hub are as independent as two DSI hosts on a SoC. Any routing that happens between DSI links is totally up to the hub. I agree that from SW perspective we can say the DSI hub connects to two peripherals, addressed with VC2 and VC3. But we can't ignore the HW side, as that information is required to program the hardware settings properly (e.g. the routing configuration in the hub). >> With DSI you always have a hub, which can translate the addresses. With >> my example above, both panels are identical and they have the same VC ID >> of 0. > > No. If they had the same VC ID how do you address them differently? When > the transactions originate at the DSI host, you have to tell it where > they should go. If you pass them the same VC ID they will go to the same > device. They have to. I was speaking from HW perspective. Both panels respond to VC ID 0. The hub manages the translation. > What you say above could only be done if the hub actually had the > possibility to initiate transactions of its own. But in that case it > becomes a DSI host itself, rather than a simple hub. Well, what is a "DSI host"? From HW perspective, the hub is a DSI host. Or actually two DSI hosts, one for each panel. It buffers the packets it receives from the SoC, which can come via DSI, i2c, or any other means, and then sends them forward. > Well, at least a hub would need to interpret the VC IDs. Take this > example: > > DSI host --> DSI hub --> peripheral 1 (VC ID 2) > | > --> peripheral 2 (VC ID 3) > > If DSI host initiates a transaction addressed at VC 2, the DSI host will > receive the packet, parse the VC and forward it to peripheral 1. In the > same way, when the DSI hub receives a packet addressed at VC 3, it'll > have to forward it to peripheral 2. > > So a hub would probably need to have some mechanism to configure which > downstream port corresponds to which VC ID. Upon initialization you need > to program some registers to make the hub do the right thing. Right, if there's routing happening, the VC ID needs to be parsed. >> It is normal to have multiple i2c devices on a bus. It's rare (never >> seen such) to have multiple DSI devices behind one DSI connector. > > But you can have repeaters and such for I2C as well. Either way, I don't > see how the physical connections are related to whether DSI should or > should not be modeled as a Linux bus. PCI is modeled as a bus and can > easily originate from a single root complex. True. So, how would a hub be modeled? We'd still need to model the real hardware topology, right? That's what PCI does also, if I'm not mistaken. Would we have one DSI bus for each DSI master on the SoC? And the hub would be a device on that bus, and the panels would be children of that hub device? How are the logical VC ID (i.e. from SW perspective) and real VC ID (HW perspective) present there? Then, how does it work if we have an external DSI encoder, which uses i2c for control and DPI for input video data, and outputs DSI to a panel? Do we have a DSI bus somewhere? If we don't, how does the DSI panel driver, connected to that DSI encoder, work? I guess in that case the external encoder driver would register a DSI bus. So generally speaking, an external encoder that outputs DSI would register a DSI bus except if it is a DSI receiver itself, and it can route DSI packets. >> Maybe we could represent DSI as both a physical one-to-one link tree, >> matching the hardware, and a logical bus, matching how it's used. > > We don't have to represent it exactly matching the hardware. What we > should be doing is represent it in a way that makes it convenient to > use. True, but here we need to know that a panel is connected to this hub's that port, and the panel responds to VC ID x, and the hub routes VC ID y to VC ID x. We need that information somewhere to configure the HW. >> But that starts to sound horribly complex for a one-to-one video bus, >> when all the known boards have just a single DSI device, and supporting >> multiple devices would be very tricky in any case. > > The original proposal isn't at all complex. It's actually fairly simple. > The bulk of the code is boilerplate to allow devices and drivers to be > registered. And we don't even need to consider all the tricky cases. I'd > very much like to avoid that in fact and rather get something that we > can put to use now. Agreed. If it works for all people concerned, and gives us a generic DSI support for simple cases, I think we can just forget about DSI hubs, routing, VC IDs etc, and get it merged. > From a quick look what omapdss seems to be doing is allocate and assign > virtual channels dynamically at runtime. I suppose that's a good way to > solve the issue. It might become an issue if you have devices that have > a fixed mapping where you can't program the routing. It's not really like that. It's actually rather confusing, partly due to the HW. The virtual channels allocated there are actually "configuration sets", they have nothing to do with the virtual channel ids sent over the DSI bus. I've no idea why the HW people used such confusing naming. That's why I usually use the term "VC ID" when I refer to the ID sent over the DSI bus. The panel drivers define the VC ID themselves, however they see fit. >> If there was a simple way to have a single Linux device sitting on two >> buses (DSI and I2C), I think it could make sense to have a Linux DSI bus >> and Linux DSI device. But afaik it's not simple, and as we anyway have a >> video "link" for DSI which can easily used for the control side, > > But how do we have that? How is it represented? There is no generic > framework that allows this. It's specifically what this patch series is > all about. We need a way to instantiate a device and have it hooked up > to a DSI host such that drivers can use the DSI host to send commands to > the peripherals. > > Perhaps you have that in omapdss, the rest of us don't have that. We can > obviously duplicate what you've done to a large extent, but the goal was > to come up with something generic. If generic means that it'll take > endless amounts of discussions, I don't think I want generic for now. > I'd rather focus on something that works for my use-cases and get back > to generic later on. As I said, I'm not nacking the patch. If it works for you, it's fine. I just want to raise the issues I have encountered and though about. If we ever want to support all kinds of DSI configurations, my opinion is that this kind of bus approach is difficult, whereas a simpler model as used by omapdss works. Both can be generic code. Then again, I've not worked much with linux buses, so I may be missing something and it's actually easy to support different DSI scenarios with a linux bus. >> it much simpler not to have the DSI bus at all. Especially as the DSI >> bus would have just one device anyway. > > You keep saying that, and at the same time you keep bringing forth > arguments why it's important to support things such as DSI hubs. Can we > settle on one way or another? Well, I keep saying that because I've been thinking that in the case we have a DSI hub, we would have multiple Linux DSI buses. One for SoC, and more for the DSI hub. If so, each of the buses have just one device. If we can have one logical DSI bus, then it's different. And I haven't said it's important to support DSI hubs. I've said I have not seen such in production, possibly because they are very difficult to use well and not worth the trouble. I wouldn't be surprised if there never will such a device supported in Linux kernel. (that's purely subjective, of course, and no, I make no guarantees that tomorrow somebody won't be asking for DSI hub support). My opinion is that as long as the DT data is good enough, we could as well forget the hubs and multiple devices on the Linux driver side, and consider DSI as a point-to-point video data link. If the DT is fine, we could later extend the drivers if there's need for hubs. If it still makes sense to have a Linux DSI bus which only ever contains one device, then there should be a Linux bus for it. In my experience, it's easier without such a bus. >>> But it will obviously be some work to move to a generic "framework", if >>> you can call it that. The driver will likely need some major rewrite, >>> which I think will be the case anyway because this will be a DRM API and >>> you'll have to move to DRM to use it. >> >> Yep. I think the most important part is to have somewhat sane DT data >> with all the relevant information. It'd be nice to see an example of >> that kind of data there would be used with this patch. > > Well the simplest case, which I think will probably cover about 99% of > use-cases would look something like this: > > /* DSI host */ > dsi@54300000 { > #address-cells = <1>; > #size-cells = <0>; > > /* DSI peripheral */ > panel@0 { > reg = <0>; > }; > }; Yes, I think this looks fine. What I have in omapdss's current DT work tree is basically the same, except I use the V4L2 style video ports, so that I can construct longer video pipelines. If longer pipelines are not needed, the port information is not needed as it can be deduced from the above data (if it's ever needed). > Which in that case has a hard-coded VC 0 for the panel. Other more > complex use-cases could look like this: > > dsi@54300000 { > #address-cells = <1>; > #size-cells = <0>; > > hub@0 { > panel@2 { > reg = <2>; > }; > > panel@3 { > reg = <3>; > }; > }; > }; > > Shouldn't that be able to cover pretty much any scenario in existence? This I think needs more information, as the routing needs to be defined. But I think we can ignore the hub side, no one is using those at the moment. Tomi
On Tue, Dec 10, 2013 at 11:19:54AM +0200, Tomi Valkeinen wrote: > On 2013-12-09 18:10, Thierry Reding wrote: > > Btw, about single linux device handling multiple VC IDs: I noticed that > the DSI spec has an example, in which a DSI peripheral receives > interlaced video, and the video packets containing even field have VC ID > 0 and packets for odd field have VC ID 1. I'm not sure how relevant > interlaced video is, but I think there's an example where having > separate linux devices for each VC ID would be somewhat clumsy. Ugh... that's pretty bad. > >> That call would go to the DSI hub driver. It knows how it routes the > >> packets (the routing configuration is either hardcoded or passed via DT > >> data), and then calls dsi_transmit op on SoC DSI, with VC number 2 and > >> the packet data. > > > > So it is the DSI hub driver that translates VC 0 to VC 2? How does it > > know that VC 0 should be VC 2 but not VC 3? Does the panel 2 driver pass > > in a different VC as panel 1? > > I'm not sure what you mean. The routing information is passed via DT > data and then configured to the hub HW, or is hardcoded in the hub hardware. Okay, that's what I meant. > > It's really the same thing. If you define VC IDs in a link local manner > > you don't need them at all. What's the point in having them if you can > > only reach a single device anyway. > > You have to define the VC IDs somewhere in link-local manner for the HW > to work. The panels in my example respond to VC ID 0, even if from SW > (DSI host's) point of view they are addressed with VC ID 2 or 3. Both VC > ID 0 and VC IDs 2 and 3 need to be programmed to the hub and possibly to > the panel. In that case I think maybe a better thing to do would be to dynamically assign the VC ID's perhaps using some enumeration algorithm. Or perhaps DSI devices are assigned fixed numbers as defined by software (DT) and these are then used to program hubs. > > From a software perspective we can always only describe devices from the > > viewpoint where things leave software control. In this case the last > > point of control is the DSI host. Therefore if we want to send a packet > > to any device connected to the DSI host (whether directly or through a > > hub), we need to pass some VC ID to the DSI host as a destination. That > > number needs to be known up front. > > > > So either you hide that information in some software framework, such as > > you seem to be doing given your example above, and make the panel driver > > call into the hub driver and have the hub driver translate the VC ID to > > a different one. > > > > The alternative would be to pass the correct address to the DSI host > > directly. Either way, though, whatever is written into the registers of > > the DSI host will end up the same, won't it? > > Yes. > > > You always address peripherals from the DSI host's viewpoint. > > Yes. Except, of course, if the hub can be accessed with i2c also, in > which case you need some other addressing mechanism. Well, you address DSI peripherals from the DSI host's viewpoint. If they have an additional I2C configuration interface then that isn't directly related to DSI. Even if both devices are physically the same, the Linux representation is a logical one, so you can very well have two types of logical devices referring to the same physical device. > >> The panels here should should be inside the hub node for DSI case. They > >> are connected to the hub, not the SoC's DSI, and need the hub to be > >> powered up and configured to work. And the hub should contain two output > >> DSI busses, each having one panel with VC ID 0. Probably something like: > >> > >> dsi { > >> hub@0 { > >> dsi@0 { > >> panel@0 { > >> }; > >> }; > >> > >> dsi@1 { > >> panel@0 { > >> }; > >> }; > >> }; > >> }; > >> > >> That kind of ruins the idea of representing the panels as DSI devices > >> with VC IDs of 2 and 3. I don't know how that should be managed then... > > > > Why does that ruin it? You still address panel@0 using VC 2 and panel@1 > > using VC 3, don't you? Therefore I see no reason why they shouldn't be > > represented as DSI devices with their respective VC IDs. > > The DT data should reflect the hardware. In this case both panels are > addressed by the DSI hub with VC ID 0. That information has to be > somewhere, and logical place for it is to define the panels as having VC > ID 0. It seems to me like for these cases we'd need some sort of automatic enumeration (as I mentioned above) or some mechanism to translate from lower busses to upper busses. Perhaps something like this would work: dsi { hub@0 { /* * means the hub has VC ID 0 on the primary bus * which translates to global VC ID 0 */ reg = <0>; panel@0 { reg = <0>; /* link-local */ assigned-addresses = <2>; }; panel@1 { reg = <1>; /* link-local */ assigned-addresses = <3>; }; }; }; Where the assigned-addresses property is even somewhat standardized and used within the PCI DT bindings. That way a hub could easily use the reg property to find out about the local port that it needs to map the assigned VC ID to. The assigned VC ID is of course read from the assigned-addresses property. > > What you say above could only be done if the hub actually had the > > possibility to initiate transactions of its own. But in that case it > > becomes a DSI host itself, rather than a simple hub. > > Well, what is a "DSI host"? From HW perspective, the hub is a DSI host. > Or actually two DSI hosts, one for each panel. It buffers the packets it > receives from the SoC, which can come via DSI, i2c, or any other means, > and then sends them forward. Can it initiate transactions itself? Based on I2C messages that it receives? If so I think that would qualify it as a DSI host. If on the other hand it only routes DSI packets to downstream ports based on registers configured via I2C, then it is not a DSI host, because it cannot create DSI packets itself. > >> It is normal to have multiple i2c devices on a bus. It's rare (never > >> seen such) to have multiple DSI devices behind one DSI connector. > > > > But you can have repeaters and such for I2C as well. Either way, I don't > > see how the physical connections are related to whether DSI should or > > should not be modeled as a Linux bus. PCI is modeled as a bus and can > > easily originate from a single root complex. > > True. > > So, how would a hub be modeled? We'd still need to model the real > hardware topology, right? That's what PCI does also, if I'm not mistaken. Well, with PCI you have a whole lot more address space that can actually reflect the topology. That's completely absent in DSI, so I don't think we can really describe it accurately. > Would we have one DSI bus for each DSI master on the SoC? And the hub > would be a device on that bus, and the panels would be children of that > hub device? > > How are the logical VC ID (i.e. from SW perspective) and real VC ID (HW > perspective) present there? I think the above example should clarify this. > Then, how does it work if we have an external DSI encoder, which uses > i2c for control and DPI for input video data, and outputs DSI to a > panel? Do we have a DSI bus somewhere? If we don't, how does the DSI > panel driver, connected to that DSI encoder, work? In that case I think the external DSI encoder would have to be considered the DSI host, since it will be the one who initiates transactions. A panel connected to it will be a simple DSI peripheral on that host's bus. > I guess in that case the external encoder driver would register a DSI > bus. So generally speaking, an external encoder that outputs DSI would > register a DSI bus except if it is a DSI receiver itself, and it can > route DSI packets. Yes. Although I think in the latter case it will typically only route but not create packets of its own. Doing both seems to me like it would be rather difficult to implement. > >> If there was a simple way to have a single Linux device sitting on two > >> buses (DSI and I2C), I think it could make sense to have a Linux DSI bus > >> and Linux DSI device. But afaik it's not simple, and as we anyway have a > >> video "link" for DSI which can easily used for the control side, > > > > But how do we have that? How is it represented? There is no generic > > framework that allows this. It's specifically what this patch series is > > all about. We need a way to instantiate a device and have it hooked up > > to a DSI host such that drivers can use the DSI host to send commands to > > the peripherals. > > > > Perhaps you have that in omapdss, the rest of us don't have that. We can > > obviously duplicate what you've done to a large extent, but the goal was > > to come up with something generic. If generic means that it'll take > > endless amounts of discussions, I don't think I want generic for now. > > I'd rather focus on something that works for my use-cases and get back > > to generic later on. > > As I said, I'm not nacking the patch. If it works for you, it's fine. > > I just want to raise the issues I have encountered and though about. If > we ever want to support all kinds of DSI configurations, my opinion is > that this kind of bus approach is difficult, whereas a simpler model as > used by omapdss works. Both can be generic code. > > Then again, I've not worked much with linux buses, so I may be missing > something and it's actually easy to support different DSI scenarios with > a linux bus. Okay, so we've talked about many of these issues now and I appreciate you providing all that feedback. In an attempt to move things forward can I propose that we start off with something simple so we can get started and gather some experience with the matter. The most difficult part will be the DT bindings. I've sent a proposal a while ago[0]. It's explicitly very terse and I don't think it would prevent any of the more advanced setups involving hubs in the future. Basically it says you can have a DSI host and up to four children attached to it. And it describes that you use the reg property to describe the VC ID. If we don't have a hub in there, then the global and link-local VC IDs will be the same anyway, so it should be safe from that point of view. If we ever get something that's more complicated we will have to revise the binding documentation and formalize much of what we have discussed in this thread. It would be great if you could have a look and see if you have any concerns with it. As for the code, I suggest we proceed with what Andrzej has proposed for now. I know that you don't see this as a good solution, but I don't think we'll find a common ground here and now, so we may as well proceed with what we have. If it really turns out to cause problems later on I'm sure we can rework it to meet everyone's needs. The infrastructure in Andrzej's patch is only a kernel internal API, so we can change it in whatever way we want if there's a need. [0]: http://lists.freedesktop.org/archives/dri-devel/2013-December/049934.html > >> it much simpler not to have the DSI bus at all. Especially as the DSI > >> bus would have just one device anyway. > > > > You keep saying that, and at the same time you keep bringing forth > > arguments why it's important to support things such as DSI hubs. Can we > > settle on one way or another? > > Well, I keep saying that because I've been thinking that in the case we > have a DSI hub, we would have multiple Linux DSI buses. One for SoC, and > more for the DSI hub. If so, each of the buses have just one device. > > If we can have one logical DSI bus, then it's different. Yes, I think we should have a single logical bus per DSI host. All the link-local aspects should be hidden within drivers. I'm thinking that perhaps a DSI hub could be a special type of peripheral, much like a bridge in PCI, with a means to instantiate children of its own (yet still make them children of the DSI host bus) and providing a way to translate between link-local (physical) and logical VC IDs. > And I haven't said it's important to support DSI hubs. I've said I have > not seen such in production, possibly because they are very difficult to > use well and not worth the trouble. I wouldn't be surprised if there > never will such a device supported in Linux kernel. (that's purely > subjective, of course, and no, I make no guarantees that tomorrow > somebody won't be asking for DSI hub support). Okay, I like that mindset very much. No need to address something that we're not sure we'll ever even have to support. We should be careful not to make it impossible to support in the future, but given that in-kernel APIs are allowed to change when needed, we only need to make sure the DT is good enough to start with. > My opinion is that as long as the DT data is good enough, we could as > well forget the hubs and multiple devices on the Linux driver side, and > consider DSI as a point-to-point video data link. If the DT is fine, we > could later extend the drivers if there's need for hubs. I think the binding that I've posted doesn't preclude extension for things such as hubs. Again, it'd be of great help if you could take a look and raise any concerns you have about it. > >>> But it will obviously be some work to move to a generic "framework", if > >>> you can call it that. The driver will likely need some major rewrite, > >>> which I think will be the case anyway because this will be a DRM API and > >>> you'll have to move to DRM to use it. > >> > >> Yep. I think the most important part is to have somewhat sane DT data > >> with all the relevant information. It'd be nice to see an example of > >> that kind of data there would be used with this patch. > > > > Well the simplest case, which I think will probably cover about 99% of > > use-cases would look something like this: > > > > /* DSI host */ > > dsi@54300000 { > > #address-cells = <1>; > > #size-cells = <0>; > > > > /* DSI peripheral */ > > panel@0 { > > reg = <0>; > > }; > > }; > > Yes, I think this looks fine. What I have in omapdss's current DT work > tree is basically the same, except I use the V4L2 style video ports, so > that I can construct longer video pipelines. If longer pipelines are not > needed, the port information is not needed as it can be deduced from the > above data (if it's ever needed). Okay, good. That's actually a less abstract example from the DT binding, so I'm hopeful we can at least agree on that. =) > > Which in that case has a hard-coded VC 0 for the panel. Other more > > complex use-cases could look like this: > > > > dsi@54300000 { > > #address-cells = <1>; > > #size-cells = <0>; > > > > hub@0 { > > panel@2 { > > reg = <2>; > > }; > > > > panel@3 { > > reg = <3>; > > }; > > }; > > }; > > > > Shouldn't that be able to cover pretty much any scenario in existence? > > This I think needs more information, as the routing needs to be defined. > But I think we can ignore the hub side, no one is using those at the moment. Okay. Let's continue that discussion when somebody comes up with a setup that actually uses hubs. Thierry
Hi, On 2013-12-12 14:19, Thierry Reding wrote: > In that case I think maybe a better thing to do would be to dynamically > assign the VC ID's perhaps using some enumeration algorithm. Whether or not we can configure the VC IDs depends on the HW in question. Some allow setting VC ID, or the routing, some don't. So I don't think we can make any assumptions about this. > Can it initiate transactions itself? Based on I2C messages that it > receives? If so I think that would qualify it as a DSI host. If on the > other hand it only routes DSI packets to downstream ports based on > registers configured via I2C, then it is not a DSI host, because it > cannot create DSI packets itself. Yes, it can initiate DSI transactions. I agree with the above. > Okay, so we've talked about many of these issues now and I appreciate > you providing all that feedback. In an attempt to move things forward > can I propose that we start off with something simple so we can get > started and gather some experience with the matter. Agreed. > The most difficult part will be the DT bindings. I've sent a proposal a > while ago[0]. It's explicitly very terse and I don't think it would > prevent any of the more advanced setups involving hubs in the future. > Basically it says you can have a DSI host and up to four children Well. I don't know... On what level do we describe things in the DT? If a host and a DSI peripheral is described based on the physical link, then a DSI host can have only one child. If we have a DSI hub, it's a different thing as discussed, and we can ignore it for now. So the case where there could be multiple children, is if a single DSI peripheral accepts multiple VC IDs. In that case we'd have: dsi-host { #address-cells = <1>; #size-cells = <0>; peripheral@0 { compatible = "..."; reg = <0>; }; peripheral@1 { compatible = "..."; reg = <1>; }; }; I think you already expressed earlier that you think the above is ok, and we could as well have separate drivers if a DSI peripheral accepts multiple VC IDs. I'm still not sure about that. I think that's more of describing logical setup, not the hardware. But, then again, maybe that doesn't matter, as long as the real hardware setup can be deduced from the description. In the above case it can: we know DSI is one-to-one link on the wire level, which means the two peripherals described above must use that same link, and thus must be in the same DSI hardware peripheral. <DSI hub crap> I wanted to avoid the DSI hub discussions, but, here goes again. You can ignore this is you're too tired already =). If we supported DSI hubs, and the hub had two output DSI ports, and we had a peripheral answering to two VC IDs attached to the hub. In that case we would need separate dsi-host sub-nodes for the two DSI ports, instead of having the peripherals directly under the hub node, as you had in your reply. I.e. we could have something like: dsi-host { #address-cells = <1>; #size-cells = <0>; /* hub is controlled via VC ID 0 */ hub@0 { compatible = "..."; reg = <0>; }; /* video data is sent to VC ID 1 */ hub@1 { compatible = "..."; reg = <1>; /* port 0 of the hub */ dsi-host@0 { reg = <0>; /* panel answers to VC ID 0 */ panel@0 { reg = <0>; }; /* panel answers also to VC ID 1 */ panel@1 { reg = <1>; }; }; /* port 1 of the hub */ dsi-host@1 { reg = <0>; /* nothing attached to hub's second port in this case */ }; }; }; Well, that gets messy, and is probably rather unlikely scenario. But I think that shows (at least to me) that having separate nodes for each VC ID is problematic. I added the hub's peripherals to hub@1 node, but hub@0 could also make sense. Adding multiple values to reg field makes it a bit cleaner: dsi-host { #address-cells = <1>; #size-cells = <0>; /* hub answers to VC ID 0 and 1 */ hub@0 { compatible = "..."; reg = <0 1>; /* port 0 of the hub */ dsi-host@0 { reg = <0>; /* panel answers to VC ID 0 and 1 */ panel@0 { reg = <0 1>; }; }; /* port 1 of the hub */ dsi-host@1 { reg = <0>; /* nothing attached to hub's second port in this case */ }; }; }; I think we still need the "dsi-host" nodes in the hub, as the hub needs to know to which of the ports the panels are connected. The ports probably need individual configuration, and so on. </DSI hub crap> > attached to it. And it describes that you use the reg property to > describe the VC ID. If we don't have a hub in there, then the global and > link-local VC IDs will be the same anyway, so it should be safe from > that point of view. If we ever get something that's more complicated we > will have to revise the binding documentation and formalize much of what > we have discussed in this thread. > > It would be great if you could have a look and see if you have any > concerns with it. My only concern is what I said above. However, if all the devices we have only accept one VC ID, then it's not a real issue at the moment. Maybe it would be safest to state in the binding documentation that there can be only one peripheral, accepting one VC ID. That would work for all (?) our current cases, and prevent anyone from adding multiple peripherals without discussing this topic again. Although, to be honest, even if I totally want to ignore the DSI hubs, I would like to have an agreement how to handle a single device that accepts multiple VC IDs. It's not critical though, so if we don't have a clear "this one is the best"-option, I think it can also be left out. For the sake of discussion, the other ways I see to express multiple VC IDs would be: dsi-host { #address-cells = <1>; #size-cells = <0>; peripheral@0 { compatible = "..."; /* use reg property, but have multiple */ reg = < 0 1 >; }; }; dsi-host { peripheral { compatible = "..."; /* custom peripheral property, VC ID managed manually */ vcs = <0 1>; }; }; All of these three options have their pros, I think. But the last one feels most versatile, although it also needs extra code. The last one would also allow dynamically changing VC IDs by the driver. But, as I said, your proposed bindings look safe to me, if only one child is allowed. It's easy to convert that in the future to either of the two other bindings, if we so want. > As for the code, I suggest we proceed with what Andrzej has proposed for > now. I know that you don't see this as a good solution, but I don't > think we'll find a common ground here and now, so we may as well proceed > with what we have. If it really turns out to cause problems later on I'm > sure we can rework it to meet everyone's needs. The infrastructure in > Andrzej's patch is only a kernel internal API, so we can change it in > whatever way we want if there's a need. I'm fine with the bus approach. I think it works fine. I just see a bus as an overkill, if we ever (?) just have a single child. I would add a bus only if and when it's needed. But as you said, it's kernel internal, can be changed later if needed. > Yes, I think we should have a single logical bus per DSI host. All the > link-local aspects should be hidden within drivers. I'm thinking that > perhaps a DSI hub could be a special type of peripheral, much like a > bridge in PCI, with a means to instantiate children of its own (yet > still make them children of the DSI host bus) and providing a way to > translate between link-local (physical) and logical VC IDs. Yes, something like that sounds good to me. Tomi
On 12/12/2013 01:19 PM, Thierry Reding wrote: > On Tue, Dec 10, 2013 at 11:19:54AM +0200, Tomi Valkeinen wrote: >> On 2013-12-09 18:10, Thierry Reding wrote: >> >> Btw, about single linux device handling multiple VC IDs: I noticed that >> the DSI spec has an example, in which a DSI peripheral receives >> interlaced video, and the video packets containing even field have VC ID >> 0 and packets for odd field have VC ID 1. I'm not sure how relevant >> interlaced video is, but I think there's an example where having >> separate linux devices for each VC ID would be somewhat clumsy. > Ugh... that's pretty bad. I wonder if this scenario could not be solved just by allowing range of VCs per device, for example: dsi { #address-cells = <1>; #size-cells = <1>; panel_with_interleaved_vc@0 { reg = <0, 2>; }; }; Regards Andrzej
On 2013-12-13 13:22, Andrzej Hajda wrote: > On 12/12/2013 01:19 PM, Thierry Reding wrote: >> On Tue, Dec 10, 2013 at 11:19:54AM +0200, Tomi Valkeinen wrote: >>> On 2013-12-09 18:10, Thierry Reding wrote: >>> >>> Btw, about single linux device handling multiple VC IDs: I noticed that >>> the DSI spec has an example, in which a DSI peripheral receives >>> interlaced video, and the video packets containing even field have VC ID >>> 0 and packets for odd field have VC ID 1. I'm not sure how relevant >>> interlaced video is, but I think there's an example where having >>> separate linux devices for each VC ID would be somewhat clumsy. >> Ugh... that's pretty bad. > I wonder if this scenario could not be solved just > by allowing range of VCs per device, for example: > dsi { > #address-cells = <1>; > #size-cells = <1>; > panel_with_interleaved_vc@0 { > reg = <0, 2>; > }; > }; I don't think range is good, as then you can't have, say, VC IDs 0 and 2. But giving the VC IDs individually as I did in my recent reply, should do the same thing as above, without ranges. It's still open to me if that method is good or not. Tomi
On Fri, Dec 13, 2013 at 01:29:17PM +0200, Tomi Valkeinen wrote: > On 2013-12-13 13:22, Andrzej Hajda wrote: > > On 12/12/2013 01:19 PM, Thierry Reding wrote: > >> On Tue, Dec 10, 2013 at 11:19:54AM +0200, Tomi Valkeinen wrote: > >>> On 2013-12-09 18:10, Thierry Reding wrote: > >>> > >>> Btw, about single linux device handling multiple VC IDs: I noticed that > >>> the DSI spec has an example, in which a DSI peripheral receives > >>> interlaced video, and the video packets containing even field have VC ID > >>> 0 and packets for odd field have VC ID 1. I'm not sure how relevant > >>> interlaced video is, but I think there's an example where having > >>> separate linux devices for each VC ID would be somewhat clumsy. > >> Ugh... that's pretty bad. > > I wonder if this scenario could not be solved just > > by allowing range of VCs per device, for example: > > dsi { > > #address-cells = <1>; > > #size-cells = <1>; > > panel_with_interleaved_vc@0 { > > reg = <0, 2>; > > }; > > }; > > I don't think range is good, as then you can't have, say, VC IDs 0 and > 2. But giving the VC IDs individually as I did in my recent reply, > should do the same thing as above, without ranges. > > It's still open to me if that method is good or not. I think we can actually support both of those variants with the same binding. #address-cells = <1> and #size-cells = <0> means that every device has a single address, which matches Tomi's example: dsi { peripheral@0 { reg = <0, /* first VC ID */ 2>; /* second VC ID */ }; }; The difference to what Andrzej proposed is that we additionally have #size-cells = <1>, which implies each entry in reg can have a "size" as well: dsi { peripheral@0 { reg = <0 2>; /* VC IDs 0 and 1 */ }; }; While I could be wrong of course, I do think that the former is unlikely since it's much easier to support consecutive addresses than completely separate ones. Or perhaps it isn't. Either way I think the binding would support both of those cases. I'll see if I can come up with some wording to make that a part of the binding and perhaps mention that only one-to-one relationships are taken into account in the binding. Thierry
On 2013-12-13 14:06, Thierry Reding wrote: > On Fri, Dec 13, 2013 at 01:29:17PM +0200, Tomi Valkeinen wrote: >> On 2013-12-13 13:22, Andrzej Hajda wrote: >>> On 12/12/2013 01:19 PM, Thierry Reding wrote: >>>> On Tue, Dec 10, 2013 at 11:19:54AM +0200, Tomi Valkeinen wrote: >>>>> On 2013-12-09 18:10, Thierry Reding wrote: >>>>> >>>>> Btw, about single linux device handling multiple VC IDs: I noticed that >>>>> the DSI spec has an example, in which a DSI peripheral receives >>>>> interlaced video, and the video packets containing even field have VC ID >>>>> 0 and packets for odd field have VC ID 1. I'm not sure how relevant >>>>> interlaced video is, but I think there's an example where having >>>>> separate linux devices for each VC ID would be somewhat clumsy. >>>> Ugh... that's pretty bad. >>> I wonder if this scenario could not be solved just >>> by allowing range of VCs per device, for example: >>> dsi { >>> #address-cells = <1>; >>> #size-cells = <1>; >>> panel_with_interleaved_vc@0 { >>> reg = <0, 2>; >>> }; >>> }; >> >> I don't think range is good, as then you can't have, say, VC IDs 0 and >> 2. But giving the VC IDs individually as I did in my recent reply, >> should do the same thing as above, without ranges. >> >> It's still open to me if that method is good or not. > > I think we can actually support both of those variants with the same > binding. #address-cells = <1> and #size-cells = <0> means that every > device has a single address, which matches Tomi's example: > > dsi { > peripheral@0 { > reg = <0, /* first VC ID */ > 2>; /* second VC ID */ > }; > }; > > The difference to what Andrzej proposed is that we additionally have > #size-cells = <1>, which implies each entry in reg can have a "size" as > well: > > dsi { > peripheral@0 { > reg = <0 2>; /* VC IDs 0 and 1 */ > }; > }; > > While I could be wrong of course, I do think that the former is unlikely > since it's much easier to support consecutive addresses than completely > separate ones. Or perhaps it isn't. Either way I think the binding would > support both of those cases. Right, and even if #size-cells = <1>, you can have multiple distinct VC IDs: /* two ranges, 0-0 and 2-2 */ reg = <0 1 2 1>; If there's a simple to use of_xxx helper function to get the VC IDs in any case, I think we can easily support all those cases. But if we need to parse the reg ourselves, and have different code paths for #size-cells 0 and 1, I'd rather stick to the simplest model. No point in having lots of code lines to handle different #size-cells, as we most likely just one one VC ID. I do agree that consecutive VC IDs sounds much more probable. However, array of VC IDs work for all cases, but a (single) range doesn't. And we can only have 4 VC IDs, so the biggest array would be <0 1 2 3>. Tomi
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index f864275..58a603d 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -20,6 +20,10 @@ menuconfig DRM details. You should also select and configure AGP (/dev/agpgart) support if it is available for your platform. +config DRM_MIPI_DSI + tristate + default y + config DRM_USB tristate depends on DRM diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index cc08b84..6bab99c 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -19,6 +19,7 @@ drm-$(CONFIG_COMPAT) += drm_ioc32.o drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o drm-$(CONFIG_PCI) += ati_pcigart.o +drm-mipi-dsi-y := drm_mipi_dsi.o drm-usb-y := drm_usb.o drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o @@ -31,6 +32,7 @@ obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o CFLAGS_drm_trace_points.o := -I$(src) obj-$(CONFIG_DRM) += drm.o +obj-$(CONFIG_DRM_MIPI_DSI) += drm_mipi_dsi.o obj-$(CONFIG_DRM_USB) += drm_usb.o obj-$(CONFIG_DRM_TTM) += ttm/ obj-$(CONFIG_DRM_TDFX) += tdfx/ diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c new file mode 100644 index 0000000..ead35da --- /dev/null +++ b/drivers/gpu/drm/drm_mipi_dsi.c @@ -0,0 +1,344 @@ +/* + * MIPI DSI Bus + * + * Copyright (C) 2012, Samsung Electronics, Co., Ltd. + * Andrzej Hajda <a.hajda@samsung.com> + * + * 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/of_device.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/slab.h> +#include <linux/pm.h> +#include <linux/pm_runtime.h> +#include <video/mipi_display.h> +#include <drm/drm_mipi_dsi.h> + +/* ----------------------------------------------------------------------------- + * Bus operations + */ + +int mipi_dsi_init(struct mipi_dsi_device *dev) +{ + const struct mipi_dsi_bus_ops *ops = dev->bus->ops; + + if (!ops->init) + return -ENOSYS; + + return ops->init(dev->bus, dev); +} +EXPORT_SYMBOL_GPL(mipi_dsi_init); + +int mipi_dsi_set_stream(struct mipi_dsi_device *dev, bool on) +{ + const struct mipi_dsi_bus_ops *ops = dev->bus->ops; + + if (!ops->set_stream) + return -ENOSYS; + + return ops->set_stream(dev->bus, dev, on); +} +EXPORT_SYMBOL_GPL(mipi_dsi_set_stream); + +int mipi_dsi_dcs_write(struct mipi_dsi_device *dev, int channel, const u8 *data, + size_t len) +{ + const struct mipi_dsi_bus_ops *ops = dev->bus->ops; + struct mipi_dsi_msg msg = { + .channel = channel, + .tx_buf = data, + .tx_len = len + }; + + if (!ops->transfer) + return -ENOSYS; + + switch (len) { + case 0: + return -EINVAL; + case 1: + msg.type = MIPI_DSI_DCS_SHORT_WRITE; + break; + case 2: + msg.type = MIPI_DSI_DCS_SHORT_WRITE_PARAM; + break; + default: + msg.type = MIPI_DSI_DCS_LONG_WRITE; + } + + return ops->transfer(dev->bus, dev, &msg); +} +EXPORT_SYMBOL_GPL(mipi_dsi_dcs_write); + +ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dev, int channel, u8 cmd, + u8 *data, size_t len) +{ + const struct mipi_dsi_bus_ops *ops = dev->bus->ops; + struct mipi_dsi_msg msg = { + .channel = channel, + .type = MIPI_DSI_DCS_READ, + .tx_buf = &cmd, + .tx_len = 1, + .rx_buf = data, + .rx_len = len + }; + + if (!ops->transfer) + return -ENOSYS; + + return ops->transfer(dev->bus, dev, &msg); +} +EXPORT_SYMBOL_GPL(mipi_dsi_dcs_read); + +/* ----------------------------------------------------------------------------- + * 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 (of_driver_match_device(_dev, _drv)) + return 1; + + 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, + .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, +}; + +void mipi_dsi_dev_release(struct device *_dev) +{ + struct mipi_dsi_device *dev = to_mipi_dsi_device(_dev); + + kfree(dev); +} + +static struct device_type mipi_dsi_dev_type = { + .release = mipi_dsi_dev_release, +}; + + +/* ----------------------------------------------------------------------------- + * 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; + dev->dev.type = &mipi_dsi_dev_type; + + 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); + +struct mipi_dsi_device *of_mipi_dsi_register_device(struct mipi_dsi_bus *bus, + struct device_node *node) +{ + struct mipi_dsi_device *d = NULL; + int ret; + + d = kzalloc(sizeof(*d), GFP_KERNEL); + if (!d) + return ERR_PTR(-ENOMEM); + + ret = of_modalias_node(node, d->name, sizeof(d->name)); + if (ret) { + dev_err(bus->dev, "modalias failure on %s\n", node->full_name); + goto err_free; + } + + d->dev.of_node = of_node_get(node); + if (!d->dev.of_node) + goto err_free; + + ret = mipi_dsi_device_register(d, bus); + + if (!ret) + return d; + + of_node_put(node); +err_free: + kfree(d); + + return ERR_PTR(ret); +} + +int of_mipi_dsi_register_devices(struct mipi_dsi_bus *bus) +{ + struct device_node *n; + + for_each_child_of_node(bus->dev->of_node, n) + of_mipi_dsi_register_device(bus, n); + + return 0; +} +EXPORT_SYMBOL_GPL(of_mipi_dsi_register_devices); + +static int mipi_dsi_remove_device_fn(struct device *_dev, void *priv) +{ + struct mipi_dsi_device *dev = to_mipi_dsi_device(_dev); + + mipi_dsi_device_unregister(dev); + + return 0; +} + +void mipi_dsi_unregister_devices(struct mipi_dsi_bus *bus) +{ + device_for_each_child(bus->dev, bus, mipi_dsi_remove_device_fn); +} +EXPORT_SYMBOL_GPL(mipi_dsi_unregister_devices); +/* ----------------------------------------------------------------------------- + * Init/exit + */ + +static int __init mipi_dsi_bus_init(void) +{ + return bus_register(&mipi_dsi_bus_type); +} + +static void __exit mipi_dsi_bus_exit(void) +{ + bus_unregister(&mipi_dsi_bus_type); +} + +module_init(mipi_dsi_bus_init); +module_exit(mipi_dsi_bus_exit) + +MODULE_AUTHOR("Andrzej Hajda <a.hajda@samsung.com>"); +MODULE_DESCRIPTION("MIPI DSI Bus"); +MODULE_LICENSE("GPL v2"); diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h new file mode 100644 index 0000000..cdde75e --- /dev/null +++ b/include/drm/drm_mipi_dsi.h @@ -0,0 +1,154 @@ +/* + * MIPI DSI Bus + * + * Copyright (C) 2013, Samsung Electronics, Co., Ltd. + * Andrzej Hajda <a.hajda@samsung.com> + * + * 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 __DRM_MIPI_DSI_H__ +#define __DRM_MIPI_DSI_H__ + +#include <linux/device.h> +#include <video/videomode.h> + +struct mipi_dsi_bus; +struct mipi_dsi_device; + +struct mipi_dsi_msg { + u8 channel; + u8 type; + + size_t tx_len; + const void *tx_buf; + + size_t rx_len; + void *rx_buf; +}; + +struct mipi_dsi_bus_ops { + int (*init)(struct mipi_dsi_bus *bus, struct mipi_dsi_device *dev); + int (*set_stream)(struct mipi_dsi_bus *bus, struct mipi_dsi_device *dev, + bool on); + ssize_t (*transfer)(struct mipi_dsi_bus *bus, + struct mipi_dsi_device *dev, + struct mipi_dsi_msg *msg); +}; + +#define DSI_MODE_VIDEO (1 << 0) +#define DSI_MODE_VIDEO_BURST (1 << 1) +#define DSI_MODE_VIDEO_SYNC_PULSE (1 << 2) +#define DSI_MODE_VIDEO_AUTO_VERT (1 << 3) +#define DSI_MODE_VIDEO_HSE (1 << 4) +#define DSI_MODE_VIDEO_HFP (1 << 5) +#define DSI_MODE_VIDEO_HBP (1 << 6) +#define DSI_MODE_VIDEO_HSA (1 << 7) +#define DSI_MODE_VSYNC_FLUSH (1 << 8) +#define DSI_MODE_EOT_PACKET (1 << 9) + +enum mipi_dsi_pixel_format { + DSI_FMT_RGB888, + DSI_FMT_RGB666, + DSI_FMT_RGB666_PACKED, + DSI_FMT_RGB565, +}; + +struct mipi_dsi_interface_params { + enum mipi_dsi_pixel_format format; + unsigned long mode; + unsigned long hs_clk_freq; + unsigned long esc_clk_freq; + unsigned char data_lanes; + unsigned char cmd_allow; +}; + +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 { + char name[MIPI_DSI_NAME_SIZE]; + int id; + struct device dev; + + const struct mipi_dsi_device_id *id_entry; + struct mipi_dsi_bus *bus; + struct videomode vm; + struct mipi_dsi_interface_params params; +}; + +#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); +} + +int of_mipi_dsi_register_devices(struct mipi_dsi_bus *bus); +void mipi_dsi_unregister_devices(struct mipi_dsi_bus *bus); + +/* 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_init(struct mipi_dsi_device *dev); +int mipi_dsi_set_stream(struct mipi_dsi_device *dev, bool on); +int mipi_dsi_dcs_write(struct mipi_dsi_device *dev, int channel, const u8 *data, + size_t len); +ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dev, int channel, u8 cmd, + u8 *data, size_t len); + +#define mipi_dsi_dcs_write_seq(dev, channel, seq...) \ +({\ + const u8 d[] = { seq };\ + BUILD_BUG_ON_MSG(ARRAY_SIZE(d) > 64, "DCS sequence too long for stack");\ + mipi_dsi_dcs_write(dev, channel, d, ARRAY_SIZE(d));\ +}) + +#define mipi_dsi_dcs_write_static_seq(dev, channel, seq...) \ +({\ + static const u8 d[] = { seq };\ + mipi_dsi_dcs_write(dev, channel, d, ARRAY_SIZE(d));\ +}) + +#endif /* __DRM_MIPI_DSI__ */