Messages in this thread | | | Subject | Re: [RFC 0/2] drm/dsi: DSI for devices with different control bus | From | Archit Taneja <> | Date | Thu, 10 Sep 2015 11:45:35 +0530 |
| |
On 09/08/2015 03:57 PM, Andrzej Hajda wrote: > On 09/07/2015 01:46 PM, Archit Taneja wrote: >> Thierry, >> >> On 08/21/2015 11:39 AM, Archit Taneja wrote: >>> >>> >>> On 08/20/2015 05:18 PM, Thierry Reding wrote: >>>> 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. >>> >>> I agree with the VC part. It could be a DT entry within the i2c client >>> node, but that does make it seem like a quirk. The 'reg' way under the >>> DSI host is definitely better to populate the virtual channel. >>> >>>> >>>>> 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? >>> >>> I assumed we could either represent the device using an i2c driver, or >>> dsi, but not both. Hence, I came up with this approach. >>> >>>> >>>>>> 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. >>> >>> Okay. >>> >>> Although, I'm not sure about the HDMI encoder example. An HDMI >>> encoder would read off edid directly from the adapter(with an address >>> specified), it wouldn't need to create an i2c client device. Therefore, >>> an HDMI encoder wouldn't need to have a separate node for i2c in DT. >>> >>>> >>>>> 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. >>> >>> I think what the driver should 'really' be is a bit subjective, and can >>> vary based on what the buses are used for in the device. For the Toshiba >>> chip that Jani mentioned, it tends more towards a DSI driver. Whereas, >>> for an ADV75xx chip, it's closer to an I2C driver since only I2C can be >>> used to configure the chip registers. >>> >>> Although, I agree with the point you made about the DSI bus here: >>> >>> "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." >>> >>> The fact that the data on the DSI bus contains routing information (i.e, >>> virtual channel number) always gives it some 'control' aspect. >>> >>>> >>>>> 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. >>> >>> Inconsistent wasn't the right term. I should have used 'uncommon' :) >>> It's common for two chips of the same family to have a different set >>> optional properties in DT, but it's not common for two chips of the >>> same family to be represented by a different number of devices in >>> DT. >>> >>> I don't have an issue with the fused approach you suggested, as long >>> as people are okay with the DT parts. Especially the part of needing 2 >>> compatible strings in the DT. >> >> I implemented the ADV7533 driver with the approach you suggested above >> (2 drivers for 2 different components of the chip). I posted it out >> just a while back (with you in loop). >> >> The DT node with this apporach would look like this: >> >> https://github.com/boddob/linux/blob/c24cbf63a6998d00095c10122ce5e37b764c7dba/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi#L162 >> >> The main irritant with the '2 driver' approach is that we need to >> share the per-device driver data with them. For ADV7533, I've made >> the i2c driver allocate driver data (struct adv7511). >> >> The dsi driver gets the driver data in the following way: >> >> - The i2c driver sets the driver data as its client data using >> i2c_set_clientdata() >> - Parse the i2c-control phandle to get the corresponding i2c client. >> - Extract the adv7511 struct by getting i2c_get_clientdata() >> >> This way of getting the same driver data is a bit strange, but it >> works. For this, we do need to ensure that the dsi driver defers >> as long as the i2c driver isn't probed. >> >> I've now implemented both approaches for the driver. The first using >> a dummy dsi device, and this one using 2 drivers (with both being >> represented in DT). The advantage of the latter is that we don't need >> to create any dummy device stuff, the disadvantage is that DT is a bit >> uncommon. >> >> Can we now come to a conclusion on what approach is better? > > DSI by design is data bus which can be used additionally as a control bus, but > in this particular case it is purely data bus. So of-graph bindings seem to be > better choice. As already Lucas Stach said DT hierarchy should describe control > buses and of-graph bindings should describe data bus. Argument that hw has two > interfaces does not seem to be valid here - it has only one control interface. > The other one is only for data, representing every data interface using DT > hierarchy would lead to inflation of pseudo devices. > > On the other side I do not see dummy device approach ideal solution, I guess > lightweight framework providing DSI hosts detached from Linux device model could > work better here. > The only problem here is that it should coexist somehow with dsi bus used to > control devices. Anyway implementing it shouldn't be hard, question is if it > would be eventually accepted :) I guess we can live for now with dummy devs. > > Summarizing I would prefer version with dummy devices, as it seems more > compatible with DT design.
Thanks for the feedback. I'll spin a newer version of the dummy dsi dev patches after waiting for some more comments.
Archit
> -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
-- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
| |