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@xxxxxxxxxxx> >> Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx> >> --- >> 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@xxxxxxxxxxxxxxxxxxxxx/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@xxxxxxxxxxx> >> + * >> + * 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
