OMAP: board-files: fix i2c_bus for tfp410
Subsystems (5)
PATCH 1/1 OMAP: board-files: fix i2c_bus for tfp410
The i2c handling in tfp410 driver, which handles converting parallel RGB to DVI, was changed in 958f2717b84e88bf833d996997fda8f73276f2af. The 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 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.
--- arch/arm/mach-omap2/board-3430sdp.c | 1 + arch/arm/mach-omap2/board-am3517evm.c | 1 + arch/arm/mach-omap2/board-cm-t35.c | 1 + arch/arm/mach-omap2/board-devkit8000.c | 1 + arch/arm/mach-omap2/board-omap3evm.c | 1 + arch/arm/mach-omap2/board-omap3stalker.c | 1 + include/video/omap-panel-tfp410.h | 2 +- 7 files changed, 7 insertions(+), 1 deletion(-)
arch/arm/mach-omap2/board-3430sdp.c +1 -0
@@ -157,6 +157,7 @@ static struct omap_dss_device sdp3430_lcd_device = {static struct tfp410_platform_data dvi_panel = { .power_down_gpio =-1 , + .i2c_bus_num =-1 , };static struct omap_dss_device sdp3430_dvi_device = {
arch/arm/mach-omap2/board-am3517evm.c +1 -0
@@ -208,6 +208,7 @@ static struct omap_dss_device am3517_evm_tv_device = {static struct tfp410_platform_data dvi_panel = { .power_down_gpio =-1 , + .i2c_bus_num =-1 , };static struct omap_dss_device am3517_evm_dvi_device = {
arch/arm/mach-omap2/board-cm-t35.c +1 -0
@@ -243,6 +243,7 @@ static struct omap_dss_device cm_t35_lcd_device = {static struct tfp410_platform_data dvi_panel = { .power_down_gpio =CM_T35_DVI_EN_GPIO , + .i2c_bus_num =-1 , };static struct omap_dss_device cm_t35_dvi_device = {
arch/arm/mach-omap2/board-devkit8000.c +1 -0
@@ -139,6 +139,7 @@ static struct omap_dss_device devkit8000_lcd_device = {static struct tfp410_platform_data dvi_panel = { .power_down_gpio =-1 , + .i2c_bus_num =1 , };static struct omap_dss_device devkit8000_dvi_device = {
arch/arm/mach-omap2/board-omap3evm.c +1 -0
@@ -236,6 +236,7 @@ static struct omap_dss_device omap3_evm_tv_device = {static struct tfp410_platform_data dvi_panel = { .power_down_gpio =OMAP3EVM_DVI_PANEL_EN_GPIO , + .i2c_bus_num =-1 , };static struct omap_dss_device omap3_evm_dvi_device = {
arch/arm/mach-omap2/board-omap3stalker.c +1 -0
@@ -119,6 +119,7 @@ static struct omap_dss_device omap3_stalker_tv_device = {static struct tfp410_platform_data dvi_panel = { .power_down_gpio =DSS_ENABLE_GPIO , + .i2c_bus_num =-1 , };static struct omap_dss_device omap3_stalker_dvi_device = {
include/video/omap-panel-tfp410.h +1 -1
@@ -28,7 +28,7 @@ struct omap_dss_device;* @power_down_gpio: gpio number for PD pin (or -1 if not available) */ struct tfp410_platform_data { -u16 i2c_bus_num; +int i2c_bus_num;int power_down_gpio; };
Signature · 1 line
1.7.10.4
PATCH 1/1 OMAP: board-files: fix i2c_bus for tfp410
Hi,
Quoted 2 lines · Tomi Valkeinen
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).'
Quoted 2 lines
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.
Quoted 13 lines
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_busnumber 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.
Quoted 6 lines
The bug exists in v3.5+ kernels.
Signed-off-by Tomi Valkeinen <[email protected]>Reported-by Thomas Weber <[email protected]>[for v3.5, v3.6 stable kernels]Cc stable at vger.kernel.org
This format is peculiar. Usually people use:
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.
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121116/547da487/attachment.sig>
PATCH 1/1 OMAP: board-files: fix i2c_bus for tfp410
Quoted 10 lines · Felipe Balbi
Hi,
Quoted 2 lines · Tomi Valkeinen
The i2c handling in tfp410 driver, which handles converting parallel RGB to DVI, was changed in
958f2717b84e88bf833d996997fda8f73276f2af. Thecommit summary should be added in () after commit hash. This would look like:
'was changed in 958f271 (OMAPDSS: TFP410: pdata rewrite).'
Yep.
Quoted 4 lines
Quoted 2 lines
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.
Quoted 15 lines
Quoted 15 lines
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_busnumber 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.
Quoted 8 lines
Quoted 4 lines
Signed-off-by Tomi Valkeinen <[email protected]>Reported-by Thomas Weber <[email protected]>[for v3.5, v3.6 stable kernels]Cc stable at vger.kernel.orgThis format is peculiar. Usually people use:
Cc stable at 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...
Quoted 10 lines
To be fair, the whole
i2c_bus_numlooks 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 897 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121116/10ca9040/attachment.sig>
PATCH 1/1 OMAP: board-files: fix i2c_bus for tfp410
Hi,
Quoted 41 lines · Tomi Valkeinen
Quoted 10 lines · Felipe Balbi
Hi,
Quoted 2 lines · Tomi Valkeinen
The i2c handling in tfp410 driver, which handles converting parallel RGB to DVI, was changed in
958f2717b84e88bf833d996997fda8f73276f2af. Thecommit summary should be added in () after commit hash. This would look like:
'was changed in 958f271 (OMAPDSS: TFP410: pdata rewrite).'
Yep.
Quoted 4 lines
Quoted 2 lines
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.
Quoted 20 lines
Quoted 19 lines
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_busnumber 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 <[email protected]>Reported-by Thomas Weber <[email protected]>[for v3.5, v3.6 stable kernels]Cc stable at vger.kernel.org
Quoted 6 lines
Quoted 3 lines
This format is peculiar. Usually people use:
Cc stable at vger.kernel.org # v3.5 v3.6Yes, 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.
Quoted 11 lines
Quoted 10 lines
To be fair, the whole
i2c_bus_numlooks 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.
Quoted 6 lines
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 ?
Quoted 4 lines
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 ?
Quoted 6 lines
(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.
I understand... should've added that this part wasn't related to $SUBJECT.
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121116/3b9aca52/attachment.sig>
PATCH 1/1 OMAP: board-files: fix i2c_bus for tfp410
Quoted 69 lines · Felipe Balbi
Hi,
Quoted 41 lines · Tomi Valkeinen
Quoted 10 lines · Felipe Balbi
Hi,
Quoted 2 lines · Tomi Valkeinen
The i2c handling in tfp410 driver, which handles converting parallel RGB to DVI, was changed in
958f2717b84e88bf833d996997fda8f73276f2af. Thecommit summary should be added in () after commit hash. This would look like:
'was changed in 958f271 (OMAPDSS: TFP410: pdata rewrite).'
Yep.
Quoted 4 lines
Quoted 2 lines
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.
Quoted 20 lines
Quoted 19 lines
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_busnumber 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 <[email protected]>Reported-by Thomas Weber <[email protected]>[for v3.5, v3.6 stable kernels]Cc stable at vger.kernel.orgQuoted 6 lines
Quoted 3 lines
This format is peculiar. Usually people use:
Cc stable at vger.kernel.org # v3.5 v3.6Yes, 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.
Quoted 14 lines
Quoted 10 lines
To be fair, the whole
i2c_bus_numlooks 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.
Quoted 6 lines
Quoted 3 lines
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.
Quoted 9 lines
Quoted 4 lines
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 897 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121116/e1f3bd17/attachment-0001.sig>
tfp410 and i2c_bus_num (was: Re: [PATCH] OMAP: board-files: fix i2c_bus for tfp410)
Hi,
Quoted 18 lines · Tomi Valkeinen
Quoted 18 lines
Quoted 17 lines
Quoted 10 lines
To be fair, the whole
i2c_bus_numlooks 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.
Quoted 5 lines
Quoted 2 lines
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 ?
Quoted 14 lines
Quoted 9 lines
Quoted 4 lines
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.
Quoted 7 lines
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).
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.
Looks like even drm_edid.c should change, btw.
Quoted 1 line
common panel framework perhaps we will.
ok, good ;-)
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121116/5c18aeda/attachment-0001.sig>
tfp410 and i2c_bus_num
Quoted 21 lines · Felipe Balbi
Hi,
Quoted 18 lines · Tomi Valkeinen
Quoted 18 lines
Quoted 17 lines
Quoted 10 lines
To be fair, the whole
i2c_bus_numlooks 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.
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
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?
Quoted 22 lines
Quoted 14 lines
Quoted 9 lines
Quoted 4 lines
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.Quoted 7 lines
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
Quoted 6 lines
than that's wrong :-) If TFP410 isn't really using I2C it shouldn't need the whole
i2c_bus_numlogic. 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 =).
Quoted 2 lines
If you have a generic
i2c-edid.csimple 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.
Quoted 1 line
Looks like even
drm_edid.cshould change, btw.
Yes, DRM handles it in somewhat similar way.
Tomi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 897 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121117/43701d9c/attachment.sig>
tfp410 and i2c_bus_num
Hi,
Quoted 25 lines · Tomi Valkeinen
Quoted 24 lines · Felipe Balbi
Hi,
Quoted 21 lines · Tomi Valkeinen
Quoted 20 lines
Quoted 11 lines
Quoted 10 lines
To be fair, the whole
i2c_bus_numlooks 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.
Quoted 6 lines
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 ?
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
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?
how are you toggling the gpio ? You said tfp410 isn't controlled at all except for the power down gpio. Who provides the gpio ?
Quoted 29 lines
Quoted 22 lines
Quoted 14 lines
Quoted 9 lines
Quoted 4 lines
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.Quoted 7 lines
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
Quoted 6 lines
than that's wrong :-) If TFP410 isn't really using I2C it shouldn't need the whole
i2c_bus_numlogic. 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).
Quoted 4 lines
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.
Quoted 6 lines
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.
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.
Quoted 3 lines
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 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.
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.
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.
Quoted 3 lines
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 ?
Quoted 1 line
list as long as the current solution works =).
that's your choice.
Quoted 5 lines
Quoted 2 lines
If you have a generic
i2c-edid.csimple 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.
Quoted 3 lines
Quoted 1 line
Looks like even
drm_edid.cshould 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 same thing, which is reading edid over an I2C link.
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121118/82455579/attachment-0001.sig>
tfp410 and i2c_bus_num
(dropping Tony and stable)
Quoted 2 lines · Felipe Balbi
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.
Quoted 7 lines
Quoted 4 lines
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.cfor 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.
Quoted 7 lines
Quoted 6 lines
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.
Quoted 5 lines
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.
Quoted 3 lines
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.
Quoted 6 lines
Quoted 3 lines
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.
Quoted 3 lines
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.
Quoted 6 lines
In fact current implementation is far worse than having an extra
i2c-edid.cdriver 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.
Quoted 7 lines
What I'm suggesting, however, is that you have a layer hiding all that. Make your
i2c-edid.cdriver 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.
Quoted 5 lines
Quoted 3 lines
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.
Quoted 12 lines
Quoted 1 line
list as long as the current solution works =).
that's your choice.
Quoted 5 lines
Quoted 2 lines
If you have a generic
i2c-edid.csimple 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.
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
Looks like even
drm_edid.cshould 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 same thing, which is reading edid over an I2C link.
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.
Tomi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 897 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121119/5a69f037/attachment.sig>
tfp410 and i2c_bus_num
hi,
Quoted 9 lines · Tomi Valkeinen
(dropping Tony and stable)
Quoted 2 lines · Felipe Balbi
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.
ok, fair enough.
Quoted 10 lines
Quoted 7 lines
Quoted 4 lines
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.cfor 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.
still avoids duplication :-)
Quoted 6 lines
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.
Quoted 2 lines
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.
Quoted 17 lines
Quoted 7 lines
Quoted 6 lines
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.
Quoted 5 lines
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_edidcall with omapdss, and the caller doesn't care ifread_ediduses i2c or DSI, so it should just work.
Quoted 5 lines
Quoted 3 lines
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.
fair enough.
Quoted 10 lines
Quoted 6 lines
Quoted 3 lines
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.
I think that would look a lot nicer as the underlying implementation for requesting EDID would be completely encapsulated from its users.
Quoted 6 lines
Quoted 3 lines
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_edidcall. 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.
Quoted 9 lines
Quoted 6 lines
In fact current implementation is far worse than having an extra
i2c-edid.cdriver 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.
Quoted 10 lines
Quoted 7 lines
What I'm suggesting, however, is that you have a layer hiding all that. Make your
i2c-edid.cdriver 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.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
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.
fair enough
Quoted 16 lines
Quoted 16 lines
Quoted 1 line
list as long as the current solution works =).
that's your choice.
Quoted 5 lines
Quoted 2 lines
If you have a generic
i2c-edid.csimple 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.
Quoted 3 lines
Quoted 1 line
Looks like even
drm_edid.cshould change, btw.Yes, DRM handles it in somewhat similar way.
Quoted 7 lines
Quoted 3 lines
another reason to make a generic
i2c-edid.c. It make no sense to have two pieces of code doing exactly the same thing, which is reading edid over an I2C link.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.
cheers
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121119/abb12fce/attachment.sig>
tfp410 and i2c_bus_num
Quoted 10 lines · Felipe Balbi
Quoted 6 lines
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.
Quoted 11 lines
Quoted 2 lines
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.cdriver, 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.cand just instantiate that class multiple times.
Yep. Althought the same could be done with a common edid library, without an i2c-edid driver.
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
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
Quoted 8 lines
Quoted 3 lines
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.cordvi-edid.cand 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.
Quoted 2 lines
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.
Quoted 6 lines
Quoted 2 lines
No, reading edid is done via generic
read_edidcall. 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.
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
In fact current implementation is far worse than having an extra
i2c-edid.cdriver because it doesn't encapsulate EDID implementation from tfp410.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
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.
Quoted 6 lines
Quoted 3 lines
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 897 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121119/670e0ed9/attachment.sig>
tfp410 and i2c_bus_num
Hi,
Quoted 9 lines · Tomi Valkeinen
Quoted 7 lines · Felipe Balbi
Quoted 6 lines
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.
Quoted 6 lines
Quoted 3 lines
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.
hehe, I was using an example of building a pipeline and adding whatever devices you wanted to the pipeline ;-)
Quoted 8 lines
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.
fair enough, but I'd say there are two places: DSS and DRM.
Quoted 9 lines
Quoted 9 lines
Quoted 2 lines
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.cdriver, you would only have to tell them where to get EDID structure from.
Quoted 5 lines
Quoted 2 lines
Also, if you have systems with different panel interfaces, they will use the same
i2c-edid.cand just instantiate that class multiple times.Yep. Althought the same could be done with a common edid library, without an i2c-edid driver.
correct.
Quoted 16 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
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
Quoted 8 lines
Quoted 3 lines
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.cordvi-edid.cand the consumer will a generic layer which tfp410 would use to request certain attributes from the EDID structure.
Quoted 4 lines
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.
Or whatever layer needs EDID would fetch it directly instead of going through tfp410.
Quoted 6 lines
Quoted 2 lines
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.
fair enough.
Quoted 11 lines
Quoted 6 lines
Quoted 2 lines
No, reading edid is done via generic
read_edidcall. 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.
passing i2c_bus_num should be an indicator that something is wrong with your design. Note that not only TFP410 knows that it has an underlying i2c bus, it needs to know exactly which bus it needs to use. Also keep in mind that i2c bus numbers are assigned by the kernel and could be anything. Any changes to the order of adding i2c buses, might break your i2c_bus_num parameter.
Of course this is all hypothetical, but it shows that current design is fragile, IMHO.
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
Quoted 6 lines
In fact current implementation is far worse than having an extra
i2c-edid.cdriver 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.
Quoted 7 lines
Quoted 2 lines
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.
yeah, nevermind. If it's done by a DSI-2-DVI chip, then it already _has_ the DSI handle. my bad.
Quoted 7 lines
Which would be passed forward when someone requests it. There would not be anything in common with tfp410 and i2c implementation.
Quoted 4 lines
Quoted 3 lines
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.
Quoted 6 lines
Quoted 2 lines
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 =).
ok good ;-)
Signature · 8 lines
balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121119/600a6104/attachment-0001.sig>
Tony Lindgren
maintainertfp410 and i2c_bus_num
Quoted 11 lines · Felipe Balbi
Quoted 8 lines · Tomi Valkeinen
Quoted 3 lines · Felipe Balbi
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 =).
ok good ;-)
FYI I'm assuming Tomi will resend the patch as there were some changes requested early on in the thread.
Tony
tfp410 and i2c_bus_num
Quoted 7 lines · Tony Lindgren
Quoted 6 lines · Felipe Balbi
Quoted 5 lines · Tomi Valkeinen
Quoted 3 lines · Felipe Balbi
fair enough... it looks like this is going nowhere, so best we come back to this later. No reason to block your patch.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
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 =).
ok good ;-)
FYI I'm assuming Tomi will resend the patch as there were some changes requested early on in the thread.
Yes, I'll resend.
Tomi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 899 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121122/48da7514/attachment.sig>
PATCH v2 1/1 OMAP: board-files: fix i2c_bus for tfp410
In reply to Tomi ValkeinenThe i2c handling in tfp410 driver, which handles converting parallel RGB to DVI, was changed in 958f2717b84e88bf833d996997fda8f73276f2af (OMAPDSS: TFP410: pdata rewrite). The 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 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.
--- arch/arm/mach-omap2/board-3430sdp.c | 1 + arch/arm/mach-omap2/board-am3517evm.c | 1 + arch/arm/mach-omap2/board-cm-t35.c | 1 + arch/arm/mach-omap2/board-devkit8000.c | 1 + arch/arm/mach-omap2/board-omap3evm.c | 1 + arch/arm/mach-omap2/board-omap3stalker.c | 1 + include/video/omap-panel-tfp410.h | 2 +- 7 files changed, 7 insertions(+), 1 deletion(-)
arch/arm/mach-omap2/board-3430sdp.c +1 -0
@@ -157,6 +157,7 @@ static struct omap_dss_device sdp3430_lcd_device = {static struct tfp410_platform_data dvi_panel = { .power_down_gpio =-1 , + .i2c_bus_num =-1 , };static struct omap_dss_device sdp3430_dvi_device = {
arch/arm/mach-omap2/board-am3517evm.c +1 -0
@@ -208,6 +208,7 @@ static struct omap_dss_device am3517_evm_tv_device = {static struct tfp410_platform_data dvi_panel = { .power_down_gpio =-1 , + .i2c_bus_num =-1 , };static struct omap_dss_device am3517_evm_dvi_device = {
arch/arm/mach-omap2/board-cm-t35.c +1 -0
@@ -243,6 +243,7 @@ static struct omap_dss_device cm_t35_lcd_device = {static struct tfp410_platform_data dvi_panel = { .power_down_gpio =CM_T35_DVI_EN_GPIO , + .i2c_bus_num =-1 , };static struct omap_dss_device cm_t35_dvi_device = {
arch/arm/mach-omap2/board-devkit8000.c +1 -0
@@ -139,6 +139,7 @@ static struct omap_dss_device devkit8000_lcd_device = {static struct tfp410_platform_data dvi_panel = { .power_down_gpio =-1 , + .i2c_bus_num =1 , };static struct omap_dss_device devkit8000_dvi_device = {
arch/arm/mach-omap2/board-omap3evm.c +1 -0
@@ -236,6 +236,7 @@ static struct omap_dss_device omap3_evm_tv_device = {static struct tfp410_platform_data dvi_panel = { .power_down_gpio =OMAP3EVM_DVI_PANEL_EN_GPIO , + .i2c_bus_num =-1 , };static struct omap_dss_device omap3_evm_dvi_device = {
arch/arm/mach-omap2/board-omap3stalker.c +1 -0
@@ -119,6 +119,7 @@ static struct omap_dss_device omap3_stalker_tv_device = {static struct tfp410_platform_data dvi_panel = { .power_down_gpio =DSS_ENABLE_GPIO , + .i2c_bus_num =-1 , };static struct omap_dss_device omap3_stalker_dvi_device = {
include/video/omap-panel-tfp410.h +1 -1
@@ -28,7 +28,7 @@ struct omap_dss_device;* @power_down_gpio: gpio number for PD pin (or -1 if not available) */ struct tfp410_platform_data { -u16 i2c_bus_num; +int i2c_bus_num;int power_down_gpio; };
Signature · 1 line
1.7.10.4
Hello Tomi,
Quoted 18 lines
The i2c handling in tfp410 driver, which handles converting parallel RGB to DVI, was changed in
958f2717b84e88bf833d996997fda8f73276f2af(OMAPDSS: TFP410: pdata rewrite). The 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 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_busnumber 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.
Quoted 6 lines
The bug exists in v3.5+ kernels.
Signed-off-by Tomi Valkeinen <[email protected]>Reported-by Thomas Weber <[email protected]>Cc Thomas Weber <[email protected]>Cc <[email protected]> # v3.5+
I have tested the patch on Devkit8000.
Thanks.
Thomas
Hi Tony,
Quoted 24 lines · Tomi Valkeinen
The i2c handling in tfp410 driver, which handles converting parallel RGB to DVI, was changed in
958f2717b84e88bf833d996997fda8f73276f2af(OMAPDSS: TFP410: pdata rewrite). The 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 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_busnumber 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 <[email protected]>Reported-by Thomas Weber <[email protected]>Cc Thomas Weber <[email protected]>Cc <[email protected]> # v3.5+
Quoted 9 lines
--- arch/arm/mach-omap2/board-3430sdp.c | 1 + arch/arm/mach-omap2/board-am3517evm.c | 1 + arch/arm/mach-omap2/board-cm-t35.c | 1 + arch/arm/mach-omap2/board-devkit8000.c | 1 + arch/arm/mach-omap2/board-omap3evm.c | 1 + arch/arm/mach-omap2/board-omap3stalker.c | 1 + include/video/omap-panel-tfp410.h | 2 +- 7 files changed, 7 insertions(+), 1 deletion(-)
Did this make it into your v3.8 branch?
Tomi
Tony Lindgren
maintainerQuoted 37 lines · Tomi Valkeinen
Hi Tony,
Quoted 24 lines · Tomi Valkeinen
The i2c handling in tfp410 driver, which handles converting parallel RGB to DVI, was changed in
958f2717b84e88bf833d996997fda8f73276f2af(OMAPDSS: TFP410: pdata rewrite). The 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 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_busnumber 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 <[email protected]>Reported-by Thomas Weber <[email protected]>Cc Thomas Weber <[email protected]>Cc <[email protected]> # v3.5+Quoted 9 lines
--- arch/arm/mach-omap2/board-3430sdp.c | 1 + arch/arm/mach-omap2/board-am3517evm.c | 1 + arch/arm/mach-omap2/board-cm-t35.c | 1 + arch/arm/mach-omap2/board-devkit8000.c | 1 + arch/arm/mach-omap2/board-omap3evm.c | 1 + arch/arm/mach-omap2/board-omap3stalker.c | 1 + include/video/omap-panel-tfp410.h | 2 +- 7 files changed, 7 insertions(+), 1 deletion(-)
Quoted 1 line
Did this make it into your v3.8 branch?
Applying now into omap-for-v3.8/fixes-for-merge-window.
Thanks,
Tony