From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrzej Hajda Subject: Re: [RFC v2 PATCH] mipi-dsi-bus: add MIPI DSI bus support Date: Mon, 25 Nov 2013 16:05:41 +0100 Message-ID: <52936745.1070209@samsung.com> References: <1384791923-11424-1-git-send-email-a.hajda@samsung.com> <20131122174127.GA30591@ulmo.nvidia.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mailout3.w1.samsung.com (mailout3.w1.samsung.com [210.118.77.13]) by gabe.freedesktop.org (Postfix) with ESMTP id DFD50FA969 for ; Mon, 25 Nov 2013 07:05:44 -0800 (PST) Received: from eucpsbgm2.samsung.com (unknown [203.254.199.245]) by mailout3.w1.samsung.com (Oracle Communications Messaging Server 7u4-24.01(7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTP id <0MWT00CWXR9IE760@mailout3.w1.samsung.com> for dri-devel@lists.freedesktop.org; Mon, 25 Nov 2013 15:05:42 +0000 (GMT) In-reply-to: <20131122174127.GA30591@ulmo.nvidia.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces@lists.freedesktop.org Errors-To: dri-devel-bounces@lists.freedesktop.org To: Thierry Reding Cc: tomi.valkeinen@ti.com, Kyungmin Park , Thierry Reding , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org 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 >> Signed-off-by: Kyungmin Park >> --- >> 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 >> + * >> + * 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 >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include > 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 >> +#include