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.
> 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.
> >> +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.
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);
> >> +#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.
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.
Thierry
Attachment:
pgpY20_jeggoX.pgp
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
