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

[Marc Gonzalez]

On 30/07/2024 11:30, Maxime Ripard wrote:
> On Tue, Jul 30, 2024 at 11:46:24AM GMT, Dmitry Baryshkov wrote:
>> On Tue, 30 Jul 2024 at 11:27, Maxime Ripard <mripard at kernel.org> wrote:
>>> 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
>>
>> No, this is not possible. I2C pins are repurposed if I2C_EN is low.
>> You can not call that an i2c bus anymore.
>>
>>>   - 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.
>>
>> This is a pretty strange configuration.  The I2C_EN pin isn't supposed
>> to be toggled dynamically. Anyway, if that happens, I'd use pinctrl /
>> hog to control the pin.
> 
> ACK. I still believe it would be valuable, but I don't really want to be
> part of that conversation anymore. Marc, do whatever you want.

OK.

I'll send the v4 sitting in my queue.

Regards