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

[Maxime Ripard <mripard@kernel.org>]

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

On Wed, Jul 24, 2024 at 07:59:21PM GMT, Marc Gonzalez wrote:
> On 15/07/2024 16:40, Maxime Ripard wrote:
> > On Thu, Jul 04, 2024 at 07:04:41PM GMT, Marc Gonzalez wrote:
> >> On 01/07/2024 15:50, Maxime Ripard wrote:
> >>
> >>> 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.
> >>
> >> I think the purpose of the I2C_EN pin might have been misunderstood.
> >>
> >> I2C_EN is not meant to be toggled, ever, by anyone from this planet.
> > 
> > Toggled, probably not. Connected to a GPIO and the kernel has to assert
> > a level at boot, I've seen worse hardware design already.
> > 
> >> I2C_EN is a layout-time setting, decided by a board manufacturer:
> >>
> >> - If the TDP158 is fully configured once-and-for-all at layout-time,
> >> then no I2C bus is required, and I2C_EN is pulled down forever.
> >>
> >> - If the board manufacturer wants to keep open the possibility
> >> to adjust some parameters at run-time, then they must connect
> >> the device to an I2C bus, and I2C_EN is pulled up forever.
> > 
> > How do you express both cases in your current binding?
> 
> It's not that I'm ignoring your question.
> 
> It's that I don't understand what you're asking.

And that's fine, you just need to say so.

Generally speaking, you're focusing on the driver. The driver is not the
issue here. You can do whatever you want in the driver for all I care,
we can change that later on as we wish.

The binding however cannot change, so it *has* to ideally cover all 
possible situations the hardware can be used in, or at a minimum leave
the door open to support those without a compatibility breakage.

That's why I've been asking those questions, because so far the only
thing you've claimed is that "I can't test the driver for anything
else", but, again, whether there's a driver or not, or if it's
functional, is completely missing the point.

> SITUATION 1
> tdp158 is pin strapped.
> Device node is child of root node.
> Properties in proposed binding are valid (regulators and power-on pin)
> Can be supported via module_platform_driver.
> 
> SITUATION 2
> tdp158 is sitting on I2C bus.
> Device node is child of i2c bus node.
> (robh said missing reg prop would be flagged by the compiler)
> Properties in proposed binding are valid (regulators and power-on pin)
> Supported via module_i2c_driver.
> 
> If some settings-specific properties are added later, like skew,
> they would only be valid for the I2C programmable mode, obviously.

I think there's a couple more combinations:

  - The device is connected on an I2C bus, but I2C_EN is tied low
  - The device is connected on an I2C bus, but I2C_EN is connected to a
    GPIO and the kernel needs to assert its state at boot.

The GPIO case can be easily dealt with later on using an optional GPIO
in the binding, but the current binding infers the I2C_EN level from the
bus it's connected to, and I think we don't have a good way to deal with
cases that would break that assumption.

So I think we need an extra property to report the state of the i2c_en
pin (and would be mutually exclusive with the GPIO if we ever have to
support it).

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]