[PATCH] OMAP: board-files: fix i2c_bus for tfp410

[PATCH] OMAP: board-files: fix i2c_bus for tfp410

[Tomi Valkeinen]

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.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reported-by: Thomas Weber <thomas@tomweber.eu>
[for v3.5, v3.6 stable kernels]
Cc: stable@vger.kernel.org
---
 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(-)

diff --git a/arch/arm/mach-omap2/board-3430sdp.c b/arch/arm/mach-omap2/board-3430sdp.c
index 96cd369..09e1790 100644
--- a/arch/arm/mach-omap2/board-3430sdp.c
+++ b/arch/arm/mach-omap2/board-3430sdp.c
@@ -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 = {
diff --git a/arch/arm/mach-omap2/board-am3517evm.c b/arch/arm/mach-omap2/board-am3517evm.c
index e162897..f2a920a 100644
--- a/arch/arm/mach-omap2/board-am3517evm.c
+++ b/arch/arm/mach-omap2/board-am3517evm.c
@@ -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 = {
diff --git a/arch/arm/mach-omap2/board-cm-t35.c b/arch/arm/mach-omap2/board-cm-t35.c
index 376d26e..7ed0270 100644
--- a/arch/arm/mach-omap2/board-cm-t35.c
+++ b/arch/arm/mach-omap2/board-cm-t35.c
@@ -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 = {
diff --git a/arch/arm/mach-omap2/board-devkit8000.c b/arch/arm/mach-omap2/board-devkit8000.c
index 1fd161e..6f04f0f 100644
--- a/arch/arm/mach-omap2/board-devkit8000.c
+++ b/arch/arm/mach-omap2/board-devkit8000.c
@@ -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 = {
diff --git a/arch/arm/mach-omap2/board-omap3evm.c b/arch/arm/mach-omap2/board-omap3evm.c
index b9b776b..5631eb9 100644
--- a/arch/arm/mach-omap2/board-omap3evm.c
+++ b/arch/arm/mach-omap2/board-omap3evm.c
@@ -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 = {
diff --git a/arch/arm/mach-omap2/board-omap3stalker.c b/arch/arm/mach-omap2/board-omap3stalker.c
index 731235e..797be22 100644
--- a/arch/arm/mach-omap2/board-omap3stalker.c
+++ b/arch/arm/mach-omap2/board-omap3stalker.c
@@ -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 = {
diff --git a/include/video/omap-panel-tfp410.h b/include/video/omap-panel-tfp410.h
index 68c31d7..aef35e4 100644
--- a/include/video/omap-panel-tfp410.h
+++ b/include/video/omap-panel-tfp410.h
@@ -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;
 };
 
-- 
1.7.10.4

Re: [PATCH] OMAP: board-files: fix i2c_bus for tfp410

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2068 bytes --]

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 <tomi.valkeinen@ti.com>
> Reported-by: Thomas Weber <thomas@tomweber.eu>
> [for v3.5, v3.6 stable kernels]
> Cc: stable@vger.kernel.org

This format is peculiar. Usually people use:

Cc: stable@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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAP: board-files: fix i2c_bus for tfp410

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3323 bytes --]

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.valkeinen-l0cyMroinI0@public.gmane.org>
>> Reported-by: Thomas Weber <thomas-xePHB1Atl/Dsq35pWSNszA@public.gmane.org>
>> [for v3.5, v3.6 stable kernels]
>> Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> 
> This format is peculiar. Usually people use:
> 
> Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

Re: [PATCH] OMAP: board-files: fix i2c_bus for tfp410

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 4047 bytes --]

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.valkeinen@ti.com>
> >> Reported-by: Thomas Weber <thomas@tomweber.eu>
> >> [for v3.5, v3.6 stable kernels]
> >> Cc: stable@vger.kernel.org
> > 
> > This format is peculiar. Usually people use:
> > 
> > Cc: stable@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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAP: board-files: fix i2c_bus for tfp410

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 4382 bytes --]

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.valkeinen@ti.com>
>>>> Reported-by: Thomas Weber <thomas@tomweber.eu>
>>>> [for v3.5, v3.6 stable kernels]
>>>> Cc: stable@vger.kernel.org
>>>
>>> This format is peculiar. Usually people use:
>>>
>>> Cc: stable@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



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

tfp410 and i2c_bus_num (was: Re: [PATCH] OMAP: board-files: fix i2c_bus for tfp410)

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3029 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: tfp410 and i2c_bus_num

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 4359 bytes --]

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

Re: tfp410 and i2c_bus_num

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 6579 bytes --]

Hi,

On Sat, Nov 17, 2012 at 07:41:36AM +0200, Tomi Valkeinen wrote:
> 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?

how are you toggling the gpio ? You said tfp410 isn't controlled at all
except for the power down gpio. Who provides the gpio ?

> >>>> 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.

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.

> 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.

> 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.

> 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 ?

> 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 same thing, which is reading edid
over an I2C link.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: tfp410 and i2c_bus_num

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 5468 bytes --]

(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 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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

Re: tfp410 and i2c_bus_num

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 7266 bytes --]

hi,

On Mon, Nov 19, 2012 at 08:38:21AM +0200, Tomi Valkeinen wrote:
> (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.

ok, fair enough.

> >> 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.

still avoids duplication :-)

> 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.

> 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.

> >> 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.

fair enough.

> >> 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.

> > 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.

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.

> > 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.

> > 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.

fair enough

> >> 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 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

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: tfp410 and i2c_bus_num

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 5342 bytes --]

On 2012-11-19 11:27, Felipe Balbi wrote:

>> If we had a separate, independent i2c-edid driver, we'd have to somehow
>> link the i2c-edid driver and tfp410 (or whatever driver would handle the
>> video link in that particular case) so that the i2c-edid driver would
>> know about hotplug events. We would also need to link the drivers so
>> that the edid can be associated with the video pipeline the tfp410 chip
>> is part of, and to the particular slot in the pipeline where the tfp410 is.
> 
> that should all be doable, just look at how v4l2 handles pipelining the
> video data (all in userland though) and you'll see it's definitely
> possible.

Afaik, v4l2 handles only the pipeline for video. This i2c-edid would not
be part of the video pipeline, so I don't see a connection to v4l2.

And I'm not saying it's not possible, or that the current way is good or
correct. I'm saying it's not easy, and definitely more complex than the
current tfp410 implementation. The amount of code related to this
feature would multiply.

If we had 10 different tfp410 like drivers, then obviously there'd be
more pressing reason to clean this up. But currently we have only one
place where this code is used.

>> I haven't tried writing such a driver, but it sounds much more complex
>> to me than doing one or two i2c reads in the tfp410 driver.
> 
> until you have e.g. tfp999, tfp534, tfp678 (hypothetical device numbers,
> they probably don't exist) which are all SW incompatible and you have to
> duplicate i2c reads in all of them. Whereas if you had a dedicated
> i2c-edid.c driver, you would only have to tell them where to get EDID
> structure from.
> 
> Also, if you have systems with different panel interfaces, they will use
> the same i2c-edid.c and just instantiate that class multiple times.

Yep. Althought the same could be done with a common edid library,
without an i2c-edid driver.

>>>> So we need a generic way to get the resolution information, and it makes
>>>> sense to have this in the components of the video path, not in a
>>>> separate driver which is not part of the video path as such.
>>>
>>> But the I2C bus isn't part of the video path anyway. It's a completely
>>> separate path which is dedicated to reading EDID. If you need a generic
>>
>> While reading EDID is totally separate from the video data itself, it is
>> not separate from the hotplug status of the cable. And the EDID needs to
>> be associated with the video path in question, of course.
> 
> yes, but that can't be done in another way right ? Try something like
> having a EDID provider and an EDID consumer. The provider will be
> i2c-edid.c or dvi-edid.c and the consumer will a generic layer which
> tfp410 would use to request certain attributes from the EDID structure.

TFP410 doesn't need the EDID for anything, it just provides it as raw
data. Parsing the EDID is done in other layers. But as you said, instead
of tfp410 fetching the EDID and providing it, tfp410 could get the EDID
from the i2c-edid driver and pass it forward.

> I think that would look a lot nicer as the underlying implementation for
> requesting EDID would be completely encapsulated from its users.

If with "users" you mean, for example, tfp410, then true. But tfp410
doesn't use EDID for anything, it just provides it. For the actual users
of the EDID data reading the EDID is encapsulated already.

>> No, reading edid is done via generic read_edid call. TFP410 uses i2c to
>> read edid, but it could do it in any way it wants.
> 
> it's not very well designed, then, if tfp410 still needs to fetch the
> i2c adapter. It still has to know about the underlying bus for reading
> EDID.

Well, yes. TFP410 is a RGB to DVI chip. It knows that the underlying bus
is DVI, and that there are i2c lines to read the EDID. There are no
other option what the busses could be. So I don't see any problem with
knowing the underlying bus.

>>> In fact current implementation is far worse than having an extra
>>> i2c-edid.c driver because it doesn't encapsulate EDID implementation
>>> from tfp410.
>>>
>>> Then moment you need to read EDID via DSI, you will likely have to pass
>>> a DSI handle to e.g. TFP410 so it can read EDID via DSI.
>>
>> TFP410 is a parallel RGB to DVI chip. It's not related to DSI. The DSI
>> case would be with some other chip.
> 
> it doesn't change what I said, however. Another chip will need to be
> passed in a DVI handle.

I'm sorry, but I don't understand what you're saying above...

If we have a DSI-2-DVI chip, then reading the EDID would be done by
sending a DSI command to the chip, which would return the EDID data.
Which would be passed forward when someone requests it. There would not
be anything in common with tfp410 and i2c implementation.

>> We could have a library with the code that does the one or two i2c
>> reads. And while I think it'd be good, we're talking about one or two
>> i2c reads. It wouldn't be a huge improvement.
> 
> fair enough... it looks like this is going nowhere, so best we come back
> to this later. No reason to block your patch.

Well, the patch is a fix for stable kernels, so even if we had a great
idea how to rewrite the dss device handling, it wouldn't affect this
patch =).

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

Re: tfp410 and i2c_bus_num

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 6515 bytes --]

Hi,

On Mon, Nov 19, 2012 at 01:09:42PM +0200, Tomi Valkeinen wrote:
> 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.

hehe, I was using an example of building a pipeline and adding whatever
devices you wanted to the pipeline ;-)

> 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.

> >> 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.

correct.

> >>>> 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.

Or whatever layer needs EDID would fetch it directly instead of going
through tfp410.

> > 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.

> >> 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.

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.

> >>> 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.

yeah, nevermind. If it's done by a DSI-2-DVI chip, then it already _has_
the DSI handle. my bad.

> Which would be passed forward when someone requests it. There would not
> be anything in common with tfp410 and i2c implementation.
> 
> >> We could have a library with the code that does the one or two i2c
> >> reads. And while I think it'd be good, we're talking about one or two
> >> i2c reads. It wouldn't be a huge improvement.
> > 
> > fair enough... it looks like this is going nowhere, so best we come back
> > to this later. No reason to block your patch.
> 
> Well, the patch is a fix for stable kernels, so even if we had a great
> idea how to rewrite the dss device handling, it wouldn't affect this
> patch =).

ok good ;-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: tfp410 and i2c_bus_num

[Tony Lindgren]

* Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org> [121119 04:25]:
> On Mon, Nov 19, 2012 at 01:09:42PM +0200, Tomi Valkeinen wrote:
> > On 2012-11-19 11:27, Felipe Balbi wrote:
> > > 
> > > 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

Re: tfp410 and i2c_bus_num

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 684 bytes --]

On 2012-11-21 19:57, Tony Lindgren wrote:
> * Felipe Balbi <balbi@ti.com> [121119 04:25]:
>> On Mon, Nov 19, 2012 at 01:09:42PM +0200, Tomi Valkeinen wrote:
>>> On 2012-11-19 11:27, Felipe Balbi wrote:
>>>>
>>>> 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.

Yes, I'll resend.

 Tomi




[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCHv2] OMAP: board-files: fix i2c_bus for tfp410

[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_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.valkeinen@ti.com>
Reported-by: Thomas Weber <thomas@tomweber.eu>
Cc: Thomas Weber <thomas@tomweber.eu>
Cc: <stable@vger.kernel.org> # v3.5+
---
 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(-)

diff --git a/arch/arm/mach-omap2/board-3430sdp.c b/arch/arm/mach-omap2/board-3430sdp.c
index 96cd369..09e1790 100644
--- a/arch/arm/mach-omap2/board-3430sdp.c
+++ b/arch/arm/mach-omap2/board-3430sdp.c
@@ -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 = {
diff --git a/arch/arm/mach-omap2/board-am3517evm.c b/arch/arm/mach-omap2/board-am3517evm.c
index e162897..f2a920a 100644
--- a/arch/arm/mach-omap2/board-am3517evm.c
+++ b/arch/arm/mach-omap2/board-am3517evm.c
@@ -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 = {
diff --git a/arch/arm/mach-omap2/board-cm-t35.c b/arch/arm/mach-omap2/board-cm-t35.c
index 376d26e..7ed0270 100644
--- a/arch/arm/mach-omap2/board-cm-t35.c
+++ b/arch/arm/mach-omap2/board-cm-t35.c
@@ -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 = {
diff --git a/arch/arm/mach-omap2/board-devkit8000.c b/arch/arm/mach-omap2/board-devkit8000.c
index 1fd161e..6f04f0f 100644
--- a/arch/arm/mach-omap2/board-devkit8000.c
+++ b/arch/arm/mach-omap2/board-devkit8000.c
@@ -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 = {
diff --git a/arch/arm/mach-omap2/board-omap3evm.c b/arch/arm/mach-omap2/board-omap3evm.c
index b9b776b..5631eb9 100644
--- a/arch/arm/mach-omap2/board-omap3evm.c
+++ b/arch/arm/mach-omap2/board-omap3evm.c
@@ -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 = {
diff --git a/arch/arm/mach-omap2/board-omap3stalker.c b/arch/arm/mach-omap2/board-omap3stalker.c
index 731235e..797be22 100644
--- a/arch/arm/mach-omap2/board-omap3stalker.c
+++ b/arch/arm/mach-omap2/board-omap3stalker.c
@@ -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 = {
diff --git a/include/video/omap-panel-tfp410.h b/include/video/omap-panel-tfp410.h
index 68c31d7..aef35e4 100644
--- a/include/video/omap-panel-tfp410.h
+++ b/include/video/omap-panel-tfp410.h
@@ -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;
 };
 
-- 
1.7.10.4

Re: [PATCHv2] OMAP: board-files: fix i2c_bus for tfp410

[Thomas Weber]

Hello Tomi,

> 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_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.valkeinen@ti.com>
> Reported-by: Thomas Weber <thomas@tomweber.eu>
> Cc: Thomas Weber <thomas@tomweber.eu>
> Cc: <stable@vger.kernel.org> # v3.5+

I have tested the patch on Devkit8000.
Tested-by: Thomas Weber <thomas@tomweber.eu>

Thanks.

Thomas

Re: [PATCHv2] OMAP: board-files: fix i2c_bus for tfp410

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1759 bytes --]

Hi Tony,

On 2012-11-22 10:39, Tomi Valkeinen wrote:
> 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_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.valkeinen@ti.com>
> Reported-by: Thomas Weber <thomas@tomweber.eu>
> Cc: Thomas Weber <thomas@tomweber.eu>
> Cc: <stable@vger.kernel.org> # v3.5+
> ---
>  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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

Re: [PATCHv2] OMAP: board-files: fix i2c_bus for tfp410

[Tony Lindgren]

* Tomi Valkeinen <tomi.valkeinen@ti.com> [121130 01:33]:
> Hi Tony,
> 
> On 2012-11-22 10:39, Tomi Valkeinen wrote:
> > 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_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.valkeinen@ti.com>
> > Reported-by: Thomas Weber <thomas@tomweber.eu>
> > Cc: Thomas Weber <thomas@tomweber.eu>
> > Cc: <stable@vger.kernel.org> # v3.5+
> > ---
> >  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?

Applying now into omap-for-v3.8/fixes-for-merge-window.

Thanks,

Tony