On 2012-11-19 11:27, Felipe Balbi wrote: >> If we had a separate, independent i2c-edid driver, we'd have to somehow >> link the i2c-edid driver and tfp410 (or whatever driver would handle the >> video link in that particular case) so that the i2c-edid driver would >> know about hotplug events. We would also need to link the drivers so >> that the edid can be associated with the video pipeline the tfp410 chip >> is part of, and to the particular slot in the pipeline where the tfp410 is. > > that should all be doable, just look at how v4l2 handles pipelining the > video data (all in userland though) and you'll see it's definitely > possible. Afaik, v4l2 handles only the pipeline for video. This i2c-edid would not be part of the video pipeline, so I don't see a connection to v4l2. And I'm not saying it's not possible, or that the current way is good or correct. I'm saying it's not easy, and definitely more complex than the current tfp410 implementation. The amount of code related to this feature would multiply. If we had 10 different tfp410 like drivers, then obviously there'd be more pressing reason to clean this up. But currently we have only one place where this code is used. >> I haven't tried writing such a driver, but it sounds much more complex >> to me than doing one or two i2c reads in the tfp410 driver. > > until you have e.g. tfp999, tfp534, tfp678 (hypothetical device numbers, > they probably don't exist) which are all SW incompatible and you have to > duplicate i2c reads in all of them. Whereas if you had a dedicated > i2c-edid.c driver, you would only have to tell them where to get EDID > structure from. > > Also, if you have systems with different panel interfaces, they will use > the same i2c-edid.c and just instantiate that class multiple times. Yep. Althought the same could be done with a common edid library, without an i2c-edid driver. >>>> So we need a generic way to get the resolution information, and it makes >>>> sense to have this in the components of the video path, not in a >>>> separate driver which is not part of the video path as such. >>> >>> But the I2C bus isn't part of the video path anyway. It's a completely >>> separate path which is dedicated to reading EDID. If you need a generic >> >> While reading EDID is totally separate from the video data itself, it is >> not separate from the hotplug status of the cable. And the EDID needs to >> be associated with the video path in question, of course. > > yes, but that can't be done in another way right ? Try something like > having a EDID provider and an EDID consumer. The provider will be > i2c-edid.c or dvi-edid.c and the consumer will a generic layer which > tfp410 would use to request certain attributes from the EDID structure. TFP410 doesn't need the EDID for anything, it just provides it as raw data. Parsing the EDID is done in other layers. But as you said, instead of tfp410 fetching the EDID and providing it, tfp410 could get the EDID from the i2c-edid driver and pass it forward. > I think that would look a lot nicer as the underlying implementation for > requesting EDID would be completely encapsulated from its users. If with "users" you mean, for example, tfp410, then true. But tfp410 doesn't use EDID for anything, it just provides it. For the actual users of the EDID data reading the EDID is encapsulated already. >> No, reading edid is done via generic read_edid call. TFP410 uses i2c to >> read edid, but it could do it in any way it wants. > > it's not very well designed, then, if tfp410 still needs to fetch the > i2c adapter. It still has to know about the underlying bus for reading > EDID. Well, yes. TFP410 is a RGB to DVI chip. It knows that the underlying bus is DVI, and that there are i2c lines to read the EDID. There are no other option what the busses could be. So I don't see any problem with knowing the underlying bus. >>> In fact current implementation is far worse than having an extra >>> i2c-edid.c driver because it doesn't encapsulate EDID implementation >>> from tfp410. >>> >>> Then moment you need to read EDID via DSI, you will likely have to pass >>> a DSI handle to e.g. TFP410 so it can read EDID via DSI. >> >> TFP410 is a parallel RGB to DVI chip. It's not related to DSI. The DSI >> case would be with some other chip. > > it doesn't change what I said, however. Another chip will need to be > passed in a DVI handle. I'm sorry, but I don't understand what you're saying above... If we have a DSI-2-DVI chip, then reading the EDID would be done by sending a DSI command to the chip, which would return the EDID data. Which would be passed forward when someone requests it. There would not be anything in common with tfp410 and i2c implementation. >> We could have a library with the code that does the one or two i2c >> reads. And while I think it'd be good, we're talking about one or two >> i2c reads. It wouldn't be a huge improvement. > > fair enough... it looks like this is going nowhere, so best we come back > to this later. No reason to block your patch. Well, the patch is a fix for stable kernels, so even if we had a great idea how to rewrite the dss device handling, it wouldn't affect this patch =). Tomi
Attachment:
signature.asc
Description: OpenPGP digital signature
