Re: [RFC 0/2] drm/dsi: DSI for devices with different control bus

[Thierry Reding]

On Thu, Aug 20, 2015 at 09:46:14AM +0530, Archit Taneja wrote:
> Hi Thierry, Lucas,
> 
> 
> On 08/19/2015 08:32 PM, Thierry Reding wrote:
> >On Wed, Aug 19, 2015 at 04:52:24PM +0200, Lucas Stach wrote:
> >>Am Mittwoch, den 19.08.2015, 16:34 +0200 schrieb Thierry Reding:
> >>>On Wed, Aug 19, 2015 at 04:17:08PM +0200, Lucas Stach wrote:
> >>>>Hi Thierry, Archit,
> >>>>
> >>[...]
> >>>>>Perhaps a better way would be to invert this relationship. According to
> >>>>>your proposal we'd have to have DT like this:
> >>>>>
> >>>>>	i2c@... {
> >>>>>		...
> >>>>>
> >>>>>		dsi-device@... {
> >>>>>			...
> >>>>>			dsi-bus = <&dsi>;
> >>>>>			...
> >>>>>		};
> >>>>>
> >>>>>		...
> >>>>>	};
> >>>>>
> >>>>>	dsi@... {
> >>>>>		...
> >>>>>	};
> >>>>>
> >>>>>Inversing the relationship would become something like this:
> >>>>>
> >>>>>	i2c@... {
> >>>>>		...
> >>>>>	};
> >>>>>
> >>>>>	dsi@... {
> >>>>>		...
> >>>>>
> >>>>>		peripheral@... {
> >>>>>			...
> >>>>>			i2c-bus = <&i2c>;
> >>>>>			...
> >>>>>		};
> >>>>>
> >>>>>		...
> >>>>>	};
> >>>>>
> >>>>>Both of those aren't fundamentally different, and they both have the
> >>>>>disavantage of lacking ways to transport configuration data that the
> >>>>>other bus needs to instantiate the dummy device (such as the reg
> >>>>>property for example, denoting the I2C slave address or the DSI VC).
> >>>>>
> >>>>>So how about we create two devices in the device tree and fuse them at
> >>>>>the driver level:
> >>>>>
> >>>>>	i2c@... {
> >>>>>		...
> >>>>>
> >>>>>		i2cdsi: dsi-device@... {
> >>>>>			...
> >>>>>		};
> >>>>>
> >>>>>		...
> >>>>>	};
> >>>>>
> >>>>>	dsi@... {
> >>>>>		...
> >>>>>
> >>>>>		peripheral@... {
> >>>>>			...
> >>>>>			control = <&i2cdsi>;
> >>>>>			...
> >>>>>		};
> >>>>>
> >>>>>		...
> >>>>>	};
> >>>>>
> >>>>>This way we'll get both an I2C device and a DSI device that we can fully
> >>>>>describe using the standard device tree bindings. At driver time we can
> >>>>>get the I2C device from the phandle in the control property of the DSI
> >>>>>device and use it to execute I2C transactions.
> >>>>>
> >>>>I don't really like to see that you are inventing yet-another-way to
> >>>>handle devices connected to multiple buses.
> >>>>
> >>>>Devicetree is structured along the control buses, even if the devices
> >>>>are connected to multiple buses, in the DT they are always children of
> >>>>the bus that is used to control their registers from the CPUs
> >>>>perspective. So a DSI encoder that is controlled through i2c is clearly
> >>>>a child of the i2c master controller and only of that one.
> >>>
> >>>I think that's a flawed interpretation of what's going on here. The
> >>>device in fact has two interfaces: one is I2C, the other is DSI. In my
> >>>opinion that's reason enough to represent it as two logical devices.
> >>>
> >>Does it really have 2 control interfaces that are used at the same time?
> >>Or is the DSI connection a passive data bus if the register control
> >>happens through i2c?
> >
> >The interfaces may not be used at the same time, and the DSI interface
> >may even be crippled, but the device is still addressable from the DSI
> >host controller, if for nothing else than for routing the video stream.
> >
> >>>>If you need to model connections between devices that are not reflected
> >>>>through the control bus hierarchy you should really consider using the
> >>>>standardized of-graph bindings.
> >>>>(Documentation/devicetree/bindings/graph.txt)
> >>>
> >>>The problem is that the original proposal would instantiate a dummy
> >>>device, so it wouldn't be represented in DT at all. So unless you do add
> >>>two logical devices to DT (one for each bus interface), you don't have
> >>>anything to glue together with an OF graph.
> >>>
> >>I see that the having dummy device is the least desirable solution. But
> >>if there is only one control bus to the device I think it should be one
> >>device sitting beneath the control bus.
> >>
> >>You can then use of-graph to model the data path between the DSI encoder
> >>and device.
> >
> >But you will be needing a device below the DSI host controller to
> >represent that endpoint of the connection. The DSI host controller
> >itself is in no way connected to the I2C adapter. You would have to
> >add some sort of quirk to the DSI controller binding to allow it to
> 
> Thanks for the review.
> 
> I implemented this to support ADV7533 DSI to HDMI encoder chip, which
> has a DSI video bus and an i2c control bus.
> 
> There weren't any quirks as such in the device tree when I tried to
> implement this. The DT seems to manage fine without a node
> corresponding to a mipi_dsi_device:
> 
> i2c_adap@.. {
> 	adv7533@.. {
> 
> 		port {
> 			adv_in: endpoint {
> 				remote-endpoint = <&dsi_out>;
> 			};
> 		};
> 	};
> };
> 
> dsi_host@.. {
> 	...
> 	...
> 
> 	port {
> 		dsi_out: endpoint {
> 			remote-endpoint = <&adv_in>;
> 		}
> 	};
> };
> 
> It's the i2c driver's job to parse the graph and retrieve the
> phandle to the dsi host. Using this, it can proceed with
> registering itself to this host using the new dsi funcs. This
> patch does the same for the adv7533 i2c driver:
> 
> http://www.spinics.net/lists/dri-devel/msg86840.html
> 
> >hook up with a control endpoint. And then you'll need more quirks
> >to describe what kind of DSI device this is.
> 
> Could you explain what you meant by this? I.e. describing the kind
> of DSI device?

Describing the number of lanes, specifying the virtual channel, mode
flags, etc. You could probably set the number of lanes and mode flags
via the I2C driver, but especially the virtual channel cannot be set
because it isn't known to the I2C DT branch of the device.

> The dsi device created isn't really a dummy device as such. It's
> dummy in the sense that there isn't a real dsi driver associated
> with it. The dsi device is still used to attach to a mipi dsi host,
> the way normal dsi devices do.

I understand, but I don't see why it has to be instantiated by the I2C
driver, that's what I find backwards. There is already a standard way
for instantiating DSI devices, why not use it?

> >On the other hand if you properly instantiate the DSI device you can
> >easily write a driver for it, and the driver will set up the correct
> >properties as implied by the compatible string. Once you have that you
> >can easily hook it up to the I2C control interface in whatever way you
> >like, be that an OF graph or just a simple unidirectional link by
> >phandle.
> >
> 
> With the fused approach you suggested, we would have 2 drivers: one i2c
> and the other dsi. The i2c client driver would be more or less minimal,
> preparing an i2c_client device for the dsi driver to use. Is my
> understanding correct?

Correct. That's kind of similar to the way an HDMI encoder driver would
use an I2C adapter to query EDID. The i2c_client device would be a means
for the DSI driver to access the control interface.

> We can do without dummy dsi devices with this method. But, representing
> a device with 2 DT nodes seems a bit off. We'd also need to compatible
> strings for the same device, one for the i2c part, and the other for
> the dsi part.

I agree that this somewhat stretches the capabilities of device tree.
Another alternative I guess would be to not have a compatible string for
the I2C device at all (that's technically not valid, I guess) because we
really don't need an I2C driver for the device. What we really need is a
DSI driver with a means to talk over some I2C bus with some other part
of its device.

> From an adv75xx driver perspective, it should also support the ADV7511
> chip, which is a RGB/DPI to HDMI encoder. For adv7511, we don't need a
> DSI DT node. It would be a bit inconsistent to have the bindings
> require both DSI and I2C nodes for one chip, and only I2C node for the
> other, with both chips being supported by the same driver.

Why would that be inconsistent? That sounds like the most accurate
representation of the hardware to me.

Thierry

Attachment: signature.asc
Description: PGP signature