[PATCH v2 0/7] tda998x: allow use with bridge based devices

Tue Jul 31 11:15:37 UTC 2018

On Tue, Jul 31, 2018 at 12:43:34PM +0200, Peter Rosin wrote:
> On 2018-07-31 11:23, Russell King - ARM Linux wrote:
> > On Tue, Jul 31, 2018 at 09:53:57AM +0200, Peter Rosin wrote:
> >> On 2018-07-31 09:41, Russell King - ARM Linux wrote:
> >>> On Tue, Jul 31, 2018 at 07:44:24AM +0200, Peter Rosin wrote:
> >>>> On 2018-07-30 18:41, Russell King - ARM Linux wrote:
> >>>>> Hi,
> >>>>>
> >>>>> This is a re-posting of the series I responded to Peter Rosin's May
> >>>>> posting, with a few bugs fixed, and the bridge registered outside of
> >>>>> the component helper.  This should allow Peter to use the driver
> >>>>> while maintaining armada drm and tilcdc support.
> >>>>>
> >>>>> No comments (other than 0-day test results) were received on the
> >>>>> previous posting.
> >>>>
> >>>> I of course meant to comment on this! I didn't because there was a rush
> >>>> before vacation, then vacation, then a heap of important stuff that had
> >>>> piled up. This one simply had too low priority to make a significant bleep
> >>>> on the radar. Sorry for the silence...
> >>>>
> >>>> However, now that I do try to test, I get conflicts as I try to apply the
> >>>> patches. I'm wondering what this was based on? I've tried next-20180730
> >>>> drm-misc/drm-misc-next from some minutes ago.
> >>>
> >>> They're based on 4.17.
> >>
> >> Right, meanwhile I massaged the patches into a combo of some local patches,
> >> next-20180730 and drm-misc-next, and tested that. The conflict was trivial
> >> once I had a closer look...
> >>
> >> And it seems to work (with atmel-hlcdc), so you can add
> >>
> >> Tested-by: Peter Rosin <peda at axentia.se>
> > 
> > Thanks, I've added your attributation, and fixed patch 2 as you pointed
> > out.
> > 
> > I have a four more tda998x patches if you're willing to test.  These
> > follow on from this set - I've tweaked patch 2 in this follow on set to
> > cater for the removal of "_mode_" in
> > drm_mode_connector_update_edid_property.
> 
> Sure thing. Those patches work for me too. So, feel free to add
> 
> Tested-by: Peter Rosin <peda at axentia.se>
> 
> However, a couple of notes:
> 
> 1) I never succeeded in reading the edid, so I'm using a built-in edid.
> 
> I noticed the patch adding the nxp,calib gpio and got my hopes up, but
> if I just add that to the DT I get
> 
> tda998x 2-0070: found TDA19988
> gpio-21 (nxp,calib): gpiod_direction_output: tried to set a GPIO tied to an IRQ as output
> tda9950 2-0034: TDA9950 CEC interface, hardware version 3.3
> 
> Grepping the source, that comes from gpiod_direction_output, which
> fails with -EIO after that message, so that can't be working...
> 
> (Side note on that gpio; the way I read the patch I should specify
> the gpio as GPIO_ACTIVE_HIGH, but that seems backwards? Isn't the
> INT pin on the tda19988 active low?)

See below...

> ...and if I try to evade that by commenting out the interrupt stuff
> from the DT, I instead get
> 
> tda998x 2-0070: found TDA19988
> tda9950 2-0034: driver requires an interrupt
> 
> Are we supposed to wire the INT pin to two different pins on the CPU, or
> what?

It's a problem with how some GPIO/IRQ drivers in Linux model GPIOs
used as interrupts.  There is a school of thought that if an IRQ is
requested, then the GPIO associated with it should be requested and
forced as an input.  Not every GPIO/IRQ driver implements this, and
the Dove driver does not.  So, TDA998x with CEC works there.

That's fine and dandy when you have "sensible" hardware, but the TDA998x
uses the INT pin for more than just signalling an interrupt.  It is also
used as a calibration mechanism for the CEC controller's free-running
clock.  The CEC block requires various registers set, and then the INT
pin being pulled low for exactly 10ms and then raised by the CPU.  It
uses this pulse to count the number of cycles of the internal FRO to
calculate the divider necessary to get the CEC bus clock.

The GPIO is "active high" because we drive it in "true" sense:

        local_irq_disable();
        gpiod_set_value(calib, 0);
        mdelay(10);
        gpiod_set_value(calib, 1);
        local_irq_enable();

Note that the GPIO sense is independent of the IRQ trigger sense.

Some TDA998x seem to function without calibration, but it depends on the
temperature and supply voltage to the device, and the margins on other
devices on the CEC bus.

The TDA998x and CEC implement what's required for CEC to work, which
requires a GPIO/IRQ driver that does not subscribe to the above
mentioned school of thought.

However, this should only cause the CEC part to fail, and should not be
an issue for reading the EDID.

EDID reading works in two stages with the TDA998x - there is no direct
access to the EEPROM in the HDMI sink.  Instead, the TDA998x is
requested to read a block from the HDMI sink, and the received data
then appears in the TDA998x registers.  The interrupt _is_ optional
for the TDA998x non-CEC part, and when present will be used to deal with
hotplug and EDID reading.  It is required for the CEC functionality
though, so if there's no interrupt present in DT, the TDA9950 CEC
controller driver will fail, but the TDA998x should continue to work.

If you have tried without an interrupt, and EDID reading still doesn't
work, it suggests there's an electrical problem with the DDC bus between
the TDA998x and the HDMI sink.

> Is edid reading know to work on 19988? The code suggests so, but maybe
> it hasn't been tested? Or maybe it regressed?

I've been using the TDA998x pretty much constantly and EDID reading
does work.  I've not had any reports that it fails on Juno nor with
the TI hardware either, and I'm sure that I would've had some reports
if it didnt' work there.

There is an issue I'm aware of with the Synopsis DesignWare I2C
controller - attempts to read more than 16 bytes result in the I2C
master hardware crapping out, but in that case you'll get:

  "transfer terminated early - interrupt latency too high"

in the kernel log - the problem there is that the hardware
_automatically_ performs an I2C bus stop if the TX FIFO empties
during a read, which means a high interrupt latency causes only the
first TX FIFO-depth of data to be received.  The driver _used_ to
report success with garbage in the receive buffer after the first
16 bytes, now it gracefully fails and reports the condition to the
kernel log.

So, failure to read the EDID can have multiple reasons - it'll need
some investigation - is it the TDA998x failing to read the EDID from
the sink, or is it a failure to correctly read the EDID out of the
TDA998x due to some bug in the I2C driver interacting with a large
I2C receive request?

> 2) The lowest resolution on my monitor is 800x600, so I suppose I
> can't really test 4/4?

No matter, it can stay as "experimental" for the time being.  However,
I should probably arrange for the mode validation to reject pixel
doubled modes while we don't support them.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up