| Message ID | m3sg1uq6xu.fsf@t19.piap.pl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | TDA1997x: enable EDID support | expand |
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; > } > >
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?
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
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.
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
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.
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
--- 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; }
Without this patch, the TDA19971 chip's EDID is inactive. Signed-off-by: Krzysztof Halasa <khalasa@piap.pl>