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

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

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
>
>> I can also imagine scenarios when dynamic virtual channel (re-)association
>> can be useful and DSI specs are not against it AFAIK.
> How is that going to work? There's no hotplug support or similar in DSI,
> so why would anyone want to reassign virtual channels.
>
> Supposing even that we wanted to support this eventually, I think a more
> appropriate solution would be to completely remove the device and add a
> new one, because that also takes care of keeping the channel number
> embedded within the struct mipi_dsi_device up to date.
>
>> reg property means device is at fixed virtual channel.
>> DSI specs says nothing about it IMHO.
> Without that fixed virtual channel number we can't use the device in the
> first place. How are you going to address the device if you don't know
> its virtual channel?
>
>>>> +	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.
> I don't know what an HSI transfer is. The transfer function is something
> that will be called by the peripheral's device driver, and that driver
> will know the state of the device, so I don't think the DSI host should
> care.
It should be HS, ie high-speed. But as I wrote before we can remove it
for now.
It can be added again in the future if I will have a real use case.
>
>>>> +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.
And enabling/disabling transmission is also quite common for
DSI hosts.

>
> On a side-note, I think we should rename .set_stream() to something more
> DSI specific, such as:
>
> 	enum mipi_dsi_mode {
> 		MIPI_DSI_COMMAND_MODE,
> 		MIPI_DSI_VIDEO_MODE,
> 	};
>
> 	int (*set_mode)(struct mipi_dsi_host *host, enum mipi_dsi_mode mode);
Yes, that looks better.
>
>>>> +#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 :)
Anyway I can move it to driver :)
>
> 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.

Andrzej
>
> Thierry