tfp410 and i2c_bus_num (was: Re: [PATCH] OMAP: board-files: fix i2c_bus for tfp410)
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 ? > >> 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). 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. > common panel framework perhaps we will. ok, good ;-) -- balbi signature.asc Description: 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 Reported-by: Thomas Weber [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: [PATCH] OMAP: board-files: fix i2c_bus for tfp410
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 > >> Reported-by: Thomas Weber > >> [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 ? > 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 ? > (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. -- balbi signature.asc Description: Digital signature
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 >> Reported-by: Thomas Weber >> [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
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).' > 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. > 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 > Reported-by: Thomas Weber > [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 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. -- balbi signature.asc Description: Digital signature
