From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [RFC v2 PATCH] mipi-dsi-bus: add MIPI DSI bus support Date: Wed, 27 Nov 2013 11:54:03 +0100 Message-ID: <20131127105401.GB9639@ulmo.nvidia.com> References: <1384791923-11424-1-git-send-email-a.hajda@samsung.com> <20131122174127.GA30591@ulmo.nvidia.com> <52936745.1070209@samsung.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0103952332==" Return-path: Received: from mail-bk0-f45.google.com (mail-bk0-f45.google.com [209.85.214.45]) by gabe.freedesktop.org (Postfix) with ESMTP id 47BE6FAFD0 for ; Wed, 27 Nov 2013 02:54:47 -0800 (PST) Received: by mail-bk0-f45.google.com with SMTP id mx13so3114237bkb.18 for ; Wed, 27 Nov 2013 02:54:46 -0800 (PST) In-Reply-To: <52936745.1070209@samsung.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces@lists.freedesktop.org Errors-To: dri-devel-bounces@lists.freedesktop.org To: Andrzej Hajda Cc: tomi.valkeinen@ti.com, Kyungmin Park , Thierry Reding , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============0103952332== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="UHN/qo2QbUvPLonB" Content-Disposition: inline --UHN/qo2QbUvPLonB Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 =3D bus; > >> + dev->dev.bus =3D &mipi_dsi_bus_type; > >> + dev->dev.parent =3D bus->dev; > >> + dev->dev.type =3D &mipi_dsi_dev_type; > >> + > >> + if (dev->id !=3D -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 =3D...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[] =3D { seq };\ > >> + BUILD_BUG_ON_MSG(ARRAY_SIZE(d) > 64, "DCS sequence too long for stac= k");\ > >> + mipi_dsi_dcs_write(dev, channel, d, ARRAY_SIZE(d));\ > >> +}) > >> + > >> +#define mipi_dsi_dcs_write_static_seq(dev, channel, seq...) \ > >> +({\ > >> + static const u8 d[] =3D { 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. >=20 > [1] http://lists.freedesktop.org/archives/dri-devel/2013-January/034212.h= tml > [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 =2Etransfer() 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 --UHN/qo2QbUvPLonB Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJSlc9JAAoJEN0jrNd/PrOh5NwP/imiV8p2YZzYFpvU0N2qzdsi S38LwEndmzAh+n+DFu4Qd2qcaMDV/bq/q+fKG/+cQpeFnILlmdPWdkOdfwWU9WAS LTra30hBEOKUz4fWPEVoHogSSPUWTET317lLq5rCNtUL6or6l3aBomcBT8ZHCq4Q tNyCfrqrVdcXRBPlwOu2vPW5PggpBVNyu/VLewsYFk9n8dg1Nh//9+0o00cQBLdN Dfionx1FY2+qn2T+D5cSq7xwYvMmklCg0ingLRvK4fusaUYZETMxpUCdcgj05+34 tJ8vV9kqLqPshC7ouJjZJ85kqTUTIaBePY15PyGBlIsZrVeUtaX/9W37fcw91bSi cYXEB943N6d3Jymz6FmAThGJIm4iXC9ni2q/1wnL5BU+HUZJ6swcwWR458zdjW5n KhkS0Gx8hYhzbmxhp6mmUjUydjjeWu2QXfbXlvtr0r26oI/ALttnwipCQ77v954G lGUtxXA636J1hhbH+xP2WpNeZlG9prPMTeBlW9dHQGNSYaJpbr+u5ERuXJYvrcFO Arv6yVY+9n34G80C81B67kXWLBD0EITZXy57OyG1od58eLNdfho71spAlUpHx/pJ kDI7SRLttP7Li78n2tUIB+alBJccjJ31lk5BgmSNBGj/9dtUKLbFFMgT6F3niGJQ 9jIoPl3DN8pwOBZOiJbA =CIG9 -----END PGP SIGNATURE----- --UHN/qo2QbUvPLonB-- --===============0103952332== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel --===============0103952332==--