Re: [PATCH v3 1/2] dt-bindings: display: bridge: add TI TDP158

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 16 Jul 2024 at 12:24, Maxime Ripard <mripard@xxxxxxxxxx> wrote:
>
> Hi,
>
> On Mon, Jul 15, 2024 at 07:38:34PM GMT, Dmitry Baryshkov wrote:
> > On Mon, 15 Jul 2024 at 17:42, Maxime Ripard <mripard@xxxxxxxxxx> wrote:
> > >
> > > On Mon, Jul 08, 2024 at 11:29:46PM GMT, Dmitry Baryshkov wrote:
> > > > On Mon, Jul 08, 2024 at 04:59:23PM GMT, Maxime Ripard wrote:
> > > > > On Mon, Jul 01, 2024 at 05:36:18PM GMT, Marc Gonzalez wrote:
> > > > > > On 01/07/2024 15:50, Maxime Ripard wrote:
> > > > > >
> > > > > > > On Thu, Jun 27, 2024 at 01:13:03PM GMT, Marc Gonzalez wrote:
> > > > > > >
> > > > > > >> TDP158 is an AC-coupled DVI / HDMI to TMDS level shifting Redriver.
> > > > > > >> It supports DVI 1.0, HDMI 1.4b and 2.0b.
> > > > > > >> It supports 4 TMDS channels, HPD, and a DDC interface.
> > > > > > >> It supports dual power supply rails (1.1V on VDD, 3.3V on VCC)
> > > > > > >> for power reduction. Several methods of power management are
> > > > > > >> implemented to reduce overall power consumption.
> > > > > > >> It supports fixed receiver EQ gain using I2C or pin strap to
> > > > > > >> compensate for different lengths input cable or board traces.
> > > > > > >>
> > > > > > >> Features
> > > > > > >>
> > > > > > >> - AC-coupled TMDS or DisplayPort dual-mode physical layer input
> > > > > > >> to HDMI 2.0b TMDS physical layer output supporting up to 6Gbps
> > > > > > >> data rate, compatible with HDMI 2.0b electrical parameters
> > > > > > >> - DisplayPort dual-mode standard version 1.1
> > > > > > >> - Programmable fixed receiver equalizer up to 15.5dB
> > > > > > >> - Global or independent high speed lane control, pre-emphasis
> > > > > > >> and transmit swing, and slew rate control
> > > > > > >> - I2C or pin strap programmable
> > > > > > >> - Configurable as a DisplayPort redriver through I2C
> > > > > > >> - Full lane swap on main lanes
> > > > > > >> - Low power consumption (200 mW at 6Gbps, 8 mW in shutdown)
> > > > > > >>
> > > > > > >> https://www.ti.com/lit/ds/symlink/tdp158.pdf
> > > > > > >>
> > > > > > >> Signed-off-by: Marc Gonzalez <mgonzalez@xxxxxxxxxx>
> > > > > > >> ---
> > > > > > >>  .../bindings/display/bridge/ti,tdp158.yaml         | 51 ++++++++++++++++++++++
> > > > > > >>  1 file changed, 51 insertions(+)
> > > > > > >>
> > > > > > >> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
> > > > > > >> new file mode 100644
> > > > > > >> index 0000000000000..21c8585c3bb2d
> > > > > > >> --- /dev/null
> > > > > > >> +++ b/Documentation/devicetree/bindings/display/bridge/ti,tdp158.yaml
> > > > > > >> @@ -0,0 +1,51 @@
> > > > > > >> +# SPDX-License-Identifier: GPL-2.0-only
> > > > > > >> +%YAML 1.2
> > > > > > >> +---
> > > > > > >> +$id: http://devicetree.org/schemas/display/bridge/ti,tdp158.yaml#
> > > > > > >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > >> +
> > > > > > >> +title: TI TDP158 HDMI to TMDS Redriver
> > > > > > >> +
> > > > > > >> +maintainers:
> > > > > > >> +  - Arnaud Vrac <avrac@xxxxxxxxxx>
> > > > > > >> +  - Pierre-Hugues Husson <phhusson@xxxxxxxxxx>
> > > > > > >> +
> > > > > > >> +properties:
> > > > > > >> +  compatible:
> > > > > > >> +    const: ti,tdp158
> > > > > > >> +
> > > > > > >> +  reg:
> > > > > > >> +    description: I2C address of the device
> > > > > > >> +
> > > > > > >> +  enable-gpios:
> > > > > > >> +    description: GPIO controlling bridge enable
> > > > > > >> +
> > > > > > >> +  vcc-supply:
> > > > > > >> +    description: Power supply 3.3V
> > > > > > >> +
> > > > > > >> +  vdd-supply:
> > > > > > >> +    description: Power supply 1.1V
> > > > > > >> +
> > > > > > >> +  ports:
> > > > > > >> +    $ref: /schemas/graph.yaml#/properties/ports
> > > > > > >> +
> > > > > > >> +    properties:
> > > > > > >> +      port@0:
> > > > > > >> +        $ref: /schemas/graph.yaml#/properties/port
> > > > > > >> +        description: Bridge input
> > > > > > >> +
> > > > > > >> +      port@1:
> > > > > > >> +        $ref: /schemas/graph.yaml#/properties/port
> > > > > > >> +        description: Bridge output
> > > > > > >> +
> > > > > > >> +    required:
> > > > > > >> +      - port@0
> > > > > > >> +      - port@1
> > > > > > >
> > > > > > > The device supports DVI, HDMI or DP input, with various requirements and
> > > > > > > capabilities depending on the input. Your binding doesn't address that.
> > > > > > >
> > > > > > > Similarly, it can do lane-swapping, so we should probably have a
> > > > > > > property to describe what mapping we want to use.
> > > > > > >
> > > > > > > The i2c register access (and the whole behaviour of the device) is
> > > > > > > constrained on the I2C_EN pin status, and you can't read it from the
> > > > > > > device, so it's also something we need to have in the DT.
> > > > > >
> > > > > > We are using the device in its default configuration.
> > > > > > (Power on via OE, then it works as expected)
> > > > >
> > > > > I know, but that doesn't really matter for a binding.
> > > > >
> > > > > > Can we leave any additional properties to be defined by whomever needs
> > > > > > them in the future?
> > > > >
> > > > > If you can guarantee that doing so would be backward compatible, sure.
> > > > > But that means being able to answer those questions with a reasonable
> > > > > plan.
> > > >
> > > > I think proposed bindings are generic enough to cover other possible
> > > > usecases in future.
> > >
> > > I don't think it is. The current binding is for a I2C device that
> > > shouldn't be accessed through I2C.
> > >
> > > It's working for now because the driver doesn't do any access, so it's
> > > all great, but as soon as we add support for the other case, then we'll
> > > have to add a property that states that while it's an i2c device, it
> > > shouldn't be accessed.
> > >
> > > And adding such a property is a compatibility-breaking change.
> >
> > Please correct me if I'm wrong. We have following usecases.
> >
> > 1. I2C_EN pulled low. TI158 is in the pin strap mode, it is not
> > connected to the I2C bus. A0, A1, SDA and SCL pins are used for
> > strapping the settings.
> >     board DT file should describe the bridge as a platform device
> > sitting directly under the root node.
>
> DT maintainers have required that reg is always present in the other
> sub-thread.

If I2C_EN is pulled low, there is no reg, as there is no i2c bus
whatsoever. I2C pins are repurposed as pin straps, An pins are
repurposed as pin straps.

I think DT maintainers wanted reg for the 2.a. case - in other words
the bridge is present on the I2C bus, but it is not being programmed.

>
> > 2. I2C_EN pulled high. TI158 is in the I2C mode. It is connected to
> > the I2C bus, A0/A1 pins set the I2C bus address. The device is
> > controlled over the I2C bus
> >
> > 2.a. The same as 2, but the device is not controlled at all, default
> > settings are fine.
> >
> > The driver covers usecase 2.a. The bindings allow extending the driver
> > to the usecase 2 (e.g. via optional properties which specify
> > bord-specific settings)
> >
> > The usecase 1 is a completely separate topic, it requires a different
> > schema file, specifying no i2c address, only voltages supplies and
> > enable-gpios.
>
> I could have mis-unnderstood, but my understanding was that they were
> running it with I2C_EN tied low.

That was my initial assumption, but I think Arnoud pointed out that
the bridge is connected to I2C, just not controlled as defaults are
sane.

> Of course, that's one of the thing that is completely missing from the
> commit log, so who knows.

Or from the cover letter :-(

-- 
With best wishes
Dmitry



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux