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

[Thierry Reding]

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
-------------- 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/20131202/c7d919f6/attachment.pgp>