Re: [PATCH] i2c: exynos5: Move initialization code to subsys_initcall()
On 12/01/15 10:43, Joonyoung Shim wrote: +Cc Tomi Valkeinen, Hi Uwe, On 01/12/2015 04:50 PM, Uwe Kleine-König wrote: Hello, On Mon, Jan 12, 2015 at 11:53:02AM +0900, Joonyoung Shim wrote: This is required in order to ensure that core system devices such as voltage regulators attached via I2C are available early in boot. Deferred probing isn't an option? If so I suggest adding the reasoning in a comment to stop the next person converting it to that. (And if not, please fix accordingly to use deferred probing.) I couldn't get penguin logo since the commit 92b004d (video/logo: prevent use of logos after they have been freed) and i just tried old way because i missed the flow to move to deferred probing. Fb driver probe seems to be ran between fb_logo_late_init late_initcall and the freeing of the logos. Any ideas? Thierry mentioned on IRC that he encountered the same issue. And I encountered it also. So... I'd rather not revert the fix, as it's quite a nasty one, and it happens while console lock is held, so it looks like the machine just froze. But I don't know how it could be improved with the current kernel. We could make the logos non-initdata, but I don't much like that option. Or we could perhaps implement some new way to catch the freeing of initdata. Any other ideas? Tomi signature.asc Description: OpenPGP digital signature
Re: tfp410 and i2c_bus_num
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
Re: tfp410 and i2c_bus_num
(dropping Tony and stable) On 2012-11-18 13:34, Felipe Balbi wrote: how are you toggling the gpio ? You said tfp410 isn't controlled at all except for the power down gpio. Who provides the gpio ? It's a normal OMAP GPIO, going to TFP410. I use gpio_set_value() to set/unset it. Well, it's not that simple. TFP410 manages hot plug detection of the HDMI cable (although not implemented currently), so the we'd need to convey that information to the i2c-edid somehow. Which, of course, is doable, but increases complexity. I'd say it would decrease it since you would have a single copy of i2c-edid.c for DRM or DSS or any other panel/display framework. Well. Reading EDID via i2c is one or two i2c reads. There's nothing else to it. 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. 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. Another thing is that even if in this case the i2c and EDID reading are separate from the actual video path, that may not be the case for other display solutions. We could have a DSI panel which reports its resolution via DSI bus, or we could have an external DSI-to-HDMI chip, which reads the EDID via i2c from the monitor, but reports them back to the SoC via DSI bus. In this last case we would see just the DSI, not the I2C bus, so it's like I2C bus doesn't exist, so in fact we would have two possibilities: a) read EDID via I2C bus (standard HDMI) b) read EDID via DSI. Right. And currently reading edid is done via read_edid call with omapdss, and the caller doesn't care if read_edid uses i2c or DSI, so it should just work. b) doesn't exist today so we need to care about it until something like what you say shows up in the market. Until then, we care for EDID over I2C IMHO. There are chips such as I described, although not supported in the mainline. 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. way for that case you wanna cope with other methods for reading EDID, then you need a generic layer which doesn't care if it's I2C or DSI. Currently implementation is hardcoded to I2C anyway. 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. 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. What I'm suggesting, however, is that you have a layer hiding all that. Make your i2c-edid.c driver read EDID and report it to this generic layer, if some other driver in kernel needs the information, you should be calling into this generic edid layer to get the cached values. If you don't need that data in kernel, then just expose it through a set of standard sysfs files and teach Xorg about those. We need the data in the kernel. And while the above issues could perhaps be solved, all we do is read one or two 128 byte datablocks from the i2c device. I'm not sure if the extra complexity is worth it. At least it's not on the top of my todo When you have EDID over DSI, what will you do ? Write the code to read the EDID =). How that is done in practice depends on the chip in question, using chip specific DSI commands. list as long as the current solution works =). that's your choice. If you have a generic i2c-edid.c simple driver, I guess X could be changes to read those values from sysfs and take actions based on those. We handle the EDID inside the kernel, although I'm not sure if drm also exposes the EDID to userspace. I suppose it does, but haven't looked through the code. Looks like even drm_edid.c should change, btw. Yes, DRM handles it in somewhat similar way. another reason to make a generic i2c-edid.c. It make no sense to have two pieces of code doing exactly the
Re: [PATCH] OMAP: board-files: fix i2c_bus for tfp410
On 2012-11-16 15:51, Felipe Balbi wrote: Hi, On Fri, Nov 16, 2012 at 02:22:33PM +0200, Tomi Valkeinen wrote: The i2c handling in tfp410 driver, which handles converting parallel RGB to DVI, was changed in 958f2717b84e88bf833d996997fda8f73276f2af. The commit summary should be added in () after commit hash. This would look like: 'was changed in 958f271 (OMAPDSS: TFP410: pdata rewrite).' Yep. patch changed what value the driver considers as invalid/undefined. Before the patch 0 was the invalid value, but as 0 is a valid bus ^ missing comma (,) character here. Right. number, the patch changed this to -1. However, the fact was missed that many board files do not define the bus number at all, thus it's left to 0. This causes the driver to fail to get the i2c bus, exiting from the driver's probe with an error, meaning that the DVI output does not work for those boards. This patch fixes the issue by changing the i2c_bus number field in the driver's platform data from u16 to int, and setting the bus number to -1 in the board files for the boards that did not define the bus. The exception is devkit8000, for which the bus is set to 1, which is the correct bus for that board. The bug exists in v3.5+ kernels. Signed-off-by: Tomi Valkeinen tomi.valkei...@ti.com Reported-by: Thomas Weber tho...@tomweber.eu [for v3.5, v3.6 stable kernels] Cc: sta...@vger.kernel.org This format is peculiar. Usually people use: Cc: sta...@vger.kernel.org # v3.5 v3.6 Yes, I tried that. But my git send-email (1.7.10.4) rejects that line. I don't know if it's my setup, that particular git version, or what... To be fair, the whole i2c_bus_num looks like a big hackery introduced by the way panel drivers are written for OMAP DSS. TFP410 is an I2C client, not an OMAPDSS client. After a quick look at the driver, there's is no such thing as a DSS bus, so looks like you should have an I2C driver for TFP410 and the whole DSS stuff should be just a list of clients, but not a struct bus at all. The fact that you have to pass the I2C bus number down to the panel driver is already a big indication of how wrong this is, IMHO. Without going deeper in the dss device model problems, I would agree with you if this was about i2c panel, but this is not quite like that. A panel controlled via i2c would be an i2c device. But TFP410 is not controlled via i2c. It's not really controlled at all except via power-down gpio. TFP410 doesn't need the i2c to be functional at all. The i2c lines do not even touch TFP410 chip, so to be precise, the i2c lines should not be TFP410's concern. The i2c lines come from the monitor and go to OMAP's i2c pins. But TFP410 driver is a convenient place to manage them. (As a side note, TFP410 _could_ be controlled via i2c, if it would've been setup so in the board hardware. That would be totally different thing, though, than what's discussed here.). Anyway, this patch wasn't meant to fix dss device model problems. It's meant to fix a bug in the kernel. Tomi signature.asc Description: OpenPGP digital signature
Re: [PATCH] OMAP: board-files: fix i2c_bus for tfp410
On 2012-11-16 17:19, Felipe Balbi wrote: Hi, On Fri, Nov 16, 2012 at 04:27:01PM +0200, Tomi Valkeinen wrote: On 2012-11-16 15:51, Felipe Balbi wrote: Hi, On Fri, Nov 16, 2012 at 02:22:33PM +0200, Tomi Valkeinen wrote: The i2c handling in tfp410 driver, which handles converting parallel RGB to DVI, was changed in 958f2717b84e88bf833d996997fda8f73276f2af. The commit summary should be added in () after commit hash. This would look like: 'was changed in 958f271 (OMAPDSS: TFP410: pdata rewrite).' Yep. patch changed what value the driver considers as invalid/undefined. Before the patch 0 was the invalid value, but as 0 is a valid bus ^ missing comma (,) character here. Right. number, the patch changed this to -1. However, the fact was missed that many board files do not define the bus number at all, thus it's left to 0. This causes the driver to fail to get the i2c bus, exiting from the driver's probe with an error, meaning that the DVI output does not work for those boards. This patch fixes the issue by changing the i2c_bus number field in the driver's platform data from u16 to int, and setting the bus number to -1 in the board files for the boards that did not define the bus. The exception is devkit8000, for which the bus is set to 1, which is the correct bus for that board. The bug exists in v3.5+ kernels. Signed-off-by: Tomi Valkeinen tomi.valkei...@ti.com Reported-by: Thomas Weber tho...@tomweber.eu [for v3.5, v3.6 stable kernels] Cc: sta...@vger.kernel.org This format is peculiar. Usually people use: Cc: sta...@vger.kernel.org # v3.5 v3.6 Yes, I tried that. But my git send-email (1.7.10.4) rejects that line. I don't know if it's my setup, that particular git version, or what... weird... I never had that problem, since git 1.6.x, I have never seen that and I tend to upgrade rather frequently. I'm using 1.8.0 now and have sent a few patches to stable recently with no problems. To be fair, the whole i2c_bus_num looks like a big hackery introduced by the way panel drivers are written for OMAP DSS. TFP410 is an I2C client, not an OMAPDSS client. After a quick look at the driver, there's is no such thing as a DSS bus, so looks like you should have an I2C driver for TFP410 and the whole DSS stuff should be just a list of clients, but not a struct bus at all. The fact that you have to pass the I2C bus number down to the panel driver is already a big indication of how wrong this is, IMHO. Without going deeper in the dss device model problems, I would agree with you if this was about i2c panel, but this is not quite like that. A panel controlled via i2c would be an i2c device. But TFP410 is not controlled via i2c. It's not really controlled at all except via power-down gpio. TFP410 doesn't need the i2c to be functional at all. then why does it need the i2c adapter ? What is this power-down gpio ? Should that be hidden under gpiolib instead ? For the i2c, see below. Power-down GPIO is used to power down and up the tfp410 chip. The i2c lines do not even touch TFP410 chip, so to be precise, the i2c lines should not be TFP410's concern. The i2c lines come from the monitor and go to OMAP's i2c pins. But TFP410 driver is a convenient place to manage them. fair enough... but who's actually using those i2c lines ? OMAP is the I2C master, who's the slave ? It's something in the monitor, I assume... IIUC, this I2C bus goes over the HDMI wire ? Yes, the i2c goes over HDMI wire. OMAP is the master, monitor is the slave. You can see some more info from http://en.wikipedia.org/wiki/Display_Data_Channel under DDC2 section. It is used to read the EDID (http://en.wikipedia.org/wiki/Extended_display_identification_data) information from the monitor, which tells things like supported video timings etc. As for why the tfp410 driver handles the i2c... We don't have a better place. There's no driver for the monitor. Although in the future with common panel framework perhaps we will. Tomi signature.asc Description: OpenPGP digital signature
Re: tfp410 and i2c_bus_num
On 2012-11-16 20:21, Felipe Balbi wrote: Hi, On Fri, Nov 16, 2012 at 05:39:44PM +0200, Tomi Valkeinen wrote: To be fair, the whole i2c_bus_num looks like a big hackery introduced by the way panel drivers are written for OMAP DSS. TFP410 is an I2C client, not an OMAPDSS client. After a quick look at the driver, there's is no such thing as a DSS bus, so looks like you should have an I2C driver for TFP410 and the whole DSS stuff should be just a list of clients, but not a struct bus at all. The fact that you have to pass the I2C bus number down to the panel driver is already a big indication of how wrong this is, IMHO. Without going deeper in the dss device model problems, I would agree with you if this was about i2c panel, but this is not quite like that. A panel controlled via i2c would be an i2c device. But TFP410 is not controlled via i2c. It's not really controlled at all except via power-down gpio. TFP410 doesn't need the i2c to be functional at all. then why does it need the i2c adapter ? What is this power-down gpio ? Should that be hidden under gpiolib instead ? For the i2c, see below. Power-down GPIO is used to power down and up the tfp410 chip. that much I guessed ;-) Should it be hidden under gpiolib ? I have to say I have no idea what you mean... Hidden how? The i2c lines do not even touch TFP410 chip, so to be precise, the i2c lines should not be TFP410's concern. The i2c lines come from the monitor and go to OMAP's i2c pins. But TFP410 driver is a convenient place to manage them. fair enough... but who's actually using those i2c lines ? OMAP is the I2C master, who's the slave ? It's something in the monitor, I assume... IIUC, this I2C bus goes over the HDMI wire ? Yes, the i2c goes over HDMI wire. OMAP is the master, monitor is the slave. You can see some more info from http://en.wikipedia.org/wiki/Display_Data_Channel under DDC2 section. It is used to read the EDID (http://en.wikipedia.org/wiki/Extended_display_identification_data) information from the monitor, which tells things like supported video timings etc. As for why the tfp410 driver handles the i2c... We don't have a better place. There's no driver for the monitor. Although in the future with than that's wrong :-) If TFP410 isn't really using I2C it shouldn't need the whole i2c_bus_num logic. I'm far from fully understanding dss architecture but it looks like what you want is a generic 'i2c-edid.c' which just reads the edid structure during probe and caches the values and exposes them via sysfs ?!? (perhaps you also need a kernel API to read those values... I don't know; but that's also doable). Well, it's not that simple. TFP410 manages hot plug detection of the HDMI cable (although not implemented currently), so the we'd need to convey that information to the i2c-edid somehow. Which, of course, is doable, but increases complexity. Another thing is that even if in this case the i2c and EDID reading are separate from the actual video path, that may not be the case for other display solutions. We could have a DSI panel which reports its resolution via DSI bus, or we could have an external DSI-to-HDMI chip, which reads the EDID via i2c from the monitor, but reports them back to the SoC via DSI bus. 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. And while the above issues could perhaps be solved, all we do is read one or two 128 byte datablocks from the i2c device. I'm not sure if the extra complexity is worth it. At least it's not on the top of my todo list as long as the current solution works =). If you have a generic i2c-edid.c simple driver, I guess X could be changes to read those values from sysfs and take actions based on those. We handle the EDID inside the kernel, although I'm not sure if drm also exposes the EDID to userspace. Looks like even drm_edid.c should change, btw. Yes, DRM handles it in somewhat similar way. Tomi signature.asc Description: OpenPGP digital signature
