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

[Thierry Reding]

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 at samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park at 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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20131122/f9a36ecc/attachment-0001.pgp>