[PATCH] TDA1997x: enable EDID support

[PATCH] TDA1997x: enable EDID support

[Krzysztof Hałasa]

Without this patch, the TDA19971 chip's EDID is inactive.

Signed-off-by: Krzysztof Halasa <khalasa@piap.pl>

--- a/drivers/media/i2c/tda1997x.c
+++ b/drivers/media/i2c/tda1997x.c
@@ -2233,6 +2233,7 @@ static int tda1997x_core_init(struct v4l2_subdev *sd)
 	/* get initial HDMI status */
 	state->hdmi_status = io_read(sd, REG_HDMI_FLAGS);
 
+	io_write(sd, REG_EDID_ENABLE, EDID_ENABLE_A_EN | EDID_ENABLE_B_EN);
 	return 0;
 }
 

-- 
Krzysztof Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa

Re: [PATCH] TDA1997x: enable EDID support

[Hans Verkuil]

Hi Krzysztof,

On 07/06/2021 12:56, Krzysztof Hałasa wrote:
> Without this patch, the TDA19971 chip's EDID is inactive.

Was this wrong from the very beginning? How can this ever have been tested
without an EDID?

If it broke in a later patch, then please add a Fixes tag.

Regards,

	Hans

> 
> Signed-off-by: Krzysztof Halasa <khalasa@piap.pl>
> 
> --- a/drivers/media/i2c/tda1997x.c
> +++ b/drivers/media/i2c/tda1997x.c
> @@ -2233,6 +2233,7 @@ static int tda1997x_core_init(struct v4l2_subdev *sd)
>  	/* get initial HDMI status */
>  	state->hdmi_status = io_read(sd, REG_HDMI_FLAGS);
>  
> +	io_write(sd, REG_EDID_ENABLE, EDID_ENABLE_A_EN | EDID_ENABLE_B_EN);
>  	return 0;
>  }
>  
> 

Re: [PATCH] TDA1997x: enable EDID support

[Krzysztof Hałasa]

Hi Hans,

Hans Verkuil <hverkuil@xs4all.nl> writes:

>> Without this patch, the TDA19971 chip's EDID is inactive.
>
> Was this wrong from the very beginning? How can this ever have been tested
> without an EDID?

It seems so. I suspect it might have worked in tests because this
register isn't cleared on reboot. I.e., setting it once after power up
makes it work to the next power up.
Or, maybe, the HDMI signal source didn't need EDID.

I'm looking at the previous version of this driver from Gateworks and it
contains:

     /* Configure EDID
      *
      * EDID_ENABLE bits:
      *  7 - nack_off
      *  6 - edid_only
      *  1 - edid_b_en
      *  0 - edid_a_en
      */
     reg = io_read(REG_EDID_ENABLE);
     if (!tda1997x->internal_edid)
         reg &= ~0x83; /* EDID Nack ON */
     else
         reg |= 0x83;  /* EDID Nack OFF */
     io_write(REG_EDID_ENABLE, reg);

Not sure what the "non-internal" EDID could be - a separate I2C EEPROM
chip? I'm using this on Gateworks' GW54xx boards and I can't see any
such EEPROM in the vicinity of the TDA19971, but I don't know how it is
wired - perhaps Tim has some idea?
-- 
Krzysztof Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa

Re: [PATCH] TDA1997x: enable EDID support

[Tim Harvey]

On Mon, Jun 7, 2021 at 4:56 AM Krzysztof Hałasa <khalasa@piap.pl> wrote:
>
> Hi Hans,
>
> Hans Verkuil <hverkuil@xs4all.nl> writes:
>
> >> Without this patch, the TDA19971 chip's EDID is inactive.
> >
> > Was this wrong from the very beginning? How can this ever have been tested
> > without an EDID?
>
> It seems so. I suspect it might have worked in tests because this
> register isn't cleared on reboot. I.e., setting it once after power up
> makes it work to the next power up.
> Or, maybe, the HDMI signal source didn't need EDID.
>

Krzysztof,

Most likely it was that the HDMI signal source I tested with didn't
need EDID. I primarily used a V-SG4K HMDI signal generator in my
testing and development of the driver
(http://www.marshall-usa.com/monitors/model/V-SG4K-HDI.php) which
definitely doesn't need it. Other devices I tested with were another
Gateworks board with HDMI out (which also didn't need EDID) and
occasionally a 1st gen Google Chromecast and Amazon Fire stick (which
I'm not sure about).

> I'm looking at the previous version of this driver from Gateworks and it
> contains:
>
>      /* Configure EDID
>       *
>       * EDID_ENABLE bits:
>       *  7 - nack_off
>       *  6 - edid_only
>       *  1 - edid_b_en
>       *  0 - edid_a_en
>       */
>      reg = io_read(REG_EDID_ENABLE);
>      if (!tda1997x->internal_edid)
>          reg &= ~0x83; /* EDID Nack ON */
>      else
>          reg |= 0x83;  /* EDID Nack OFF */
>      io_write(REG_EDID_ENABLE, reg);
>
> Not sure what the "non-internal" EDID could be - a separate I2C EEPROM
> chip? I'm using this on Gateworks' GW54xx boards and I can't see any
> such EEPROM in the vicinity of the TDA19971, but I don't know how it is
> wired - perhaps Tim has some idea?

Not sure where the source above is from (this was all so long ago) but
my guess is that 'internal_edid' meant an EDID had been provided via
software and the else case meant there was no EDID available. There is
no support on that chip for an external EEPROM.

Tim

Re: [PATCH] TDA1997x: enable EDID support

[Krzysztof Hałasa]

Tim,

Tim Harvey <tharvey@gateworks.com> writes:

>> I'm looking at the previous version of this driver from Gateworks and it
>> contains:
>>
>>      /* Configure EDID
>>       *
>>       * EDID_ENABLE bits:
>>       *  7 - nack_off
>>       *  6 - edid_only
>>       *  1 - edid_b_en
>>       *  0 - edid_a_en
>>       */
>>      reg = io_read(REG_EDID_ENABLE);
>>      if (!tda1997x->internal_edid)
>>          reg &= ~0x83; /* EDID Nack ON */
>>      else
>>          reg |= 0x83;  /* EDID Nack OFF */
>>      io_write(REG_EDID_ENABLE, reg);

> Not sure where the source above is from (this was all so long ago) but

That's your gateworks_fslc_3.14_1.0.x_ga branch (3.14 is the kernel and
1.0.x_ga is IIRC some Freescale versioning) :-)

> my guess is that 'internal_edid' meant an EDID had been provided via
> software and the else case meant there was no EDID available.
> There is no support on that chip for an external EEPROM.

Right. I guess the else meant it was available and &= ~83 meant no
EDID... Anyway one could simply drop a 24c02 or a similar chip directly
to SDA/SCL HDMI lines, that's what the monitor makers had been doing for
a long time. Then, I guess, you would need the internal_edid = 0
(otherwise the TDA chip would be fighting the EEPROM on the SDA line).
Not that I know of any such design using this driver, I guess we can
safely skip this part.
-- 
Krzysztof Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa

Re: [PATCH] TDA1997x: enable EDID support

[Hans Verkuil]

Hi Krzysztof,

On 08/06/2021 06:54, Krzysztof Hałasa wrote:
> Tim,
> 
> Tim Harvey <tharvey@gateworks.com> writes:
> 
>>> I'm looking at the previous version of this driver from Gateworks and it
>>> contains:
>>>
>>>      /* Configure EDID
>>>       *
>>>       * EDID_ENABLE bits:
>>>       *  7 - nack_off
>>>       *  6 - edid_only
>>>       *  1 - edid_b_en
>>>       *  0 - edid_a_en
>>>       */
>>>      reg = io_read(REG_EDID_ENABLE);
>>>      if (!tda1997x->internal_edid)
>>>          reg &= ~0x83; /* EDID Nack ON */
>>>      else
>>>          reg |= 0x83;  /* EDID Nack OFF */
>>>      io_write(REG_EDID_ENABLE, reg);
> 
>> Not sure where the source above is from (this was all so long ago) but
> 
> That's your gateworks_fslc_3.14_1.0.x_ga branch (3.14 is the kernel and
> 1.0.x_ga is IIRC some Freescale versioning) :-)
> 
>> my guess is that 'internal_edid' meant an EDID had been provided via
>> software and the else case meant there was no EDID available.
>> There is no support on that chip for an external EEPROM.
> 
> Right. I guess the else meant it was available and &= ~83 meant no
> EDID... Anyway one could simply drop a 24c02 or a similar chip directly
> to SDA/SCL HDMI lines, that's what the monitor makers had been doing for
> a long time. Then, I guess, you would need the internal_edid = 0
> (otherwise the TDA chip would be fighting the EEPROM on the SDA line).
> Not that I know of any such design using this driver, I guess we can
> safely skip this part.
> 

OK, I think the history is clear. Can you post a v2 with a Fixes tag and
comment a bit on why this was not caught before?

Regards,

	Hans

Re: [PATCH] TDA1997x: enable EDID support

[Krzysztof Hałasa]

Hans Verkuil <hverkuil@xs4all.nl> writes:

> OK, I think the history is clear. Can you post a v2 with a Fixes tag and
> comment a bit on why this was not caught before?

Sure, will do. That "Fixes" tag... since it's from the beginning (the
Gateworks' branch was never a part of the official tree), do I still
need it? It would have to point to the initial submission of this
driver.
-- 
Krzysztof Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa

Re: [PATCH] TDA1997x: enable EDID support

[Hans Verkuil]

On 08/06/2021 10:45, Krzysztof Hałasa wrote:
> Hans Verkuil <hverkuil@xs4all.nl> writes:
> 
>> OK, I think the history is clear. Can you post a v2 with a Fixes tag and
>> comment a bit on why this was not caught before?
> 
> Sure, will do. That "Fixes" tag... since it's from the beginning (the
> Gateworks' branch was never a part of the official tree), do I still
> need it? It would have to point to the initial submission of this
> driver.
> 

Yes, that's fine. It's been broken since the beginning, so the Fixes
tag will indicate that.

Regards,

	Hans