Re: [RFC v2 PATCH] mipi-dsi-bus: add MIPI DSI bus support

[Andrzej Hajda <a.hajda@samsung.com>]

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