Re: [PATCH v4 3/5] dt-bindings: media: add cat24c208 bindings

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

 



On 11/18/22 16:20, Rob Herring wrote:
> +Bartosz
> 
> On Fri, Nov 18, 2022 at 4:34 AM Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>>
>> On 11/16/22 21:07, Rob Herring wrote:
>>> On Fri, Nov 11, 2022 at 02:29:04PM +0100, Erling Ljunggren wrote:
>>>> Add devicetree bindings for new cat24c208 EDID EEPROM driver.
>>>>
>>>> Signed-off-by: Erling Ljunggren <hljunggr@xxxxxxxxx>
>>>> ---
>>>>  .../bindings/media/i2c/onnn,cat24c208.yaml    | 46 +++++++++++++++++++
>>>>  1 file changed, 46 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/onnn,cat24c208.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/onnn,cat24c208.yaml b/Documentation/devicetree/bindings/media/i2c/onnn,cat24c208.yaml
>>>> new file mode 100644
>>>> index 000000000000..492eecb3ab7c
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/media/i2c/onnn,cat24c208.yaml
>>>> @@ -0,0 +1,46 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/media/i2c/onnn,cat24c208.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: ON Semiconductor CAT24C208 EDID EEPROM driver
>>>> +
>>>> +maintainers:
>>>> +  - Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
>>>> +
>>>> +description: |
>>>> +  CAT24C208 is a dual port i2c EEPROM designed for EDID storage.
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: onnn,cat24c208
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  input-connector:
>>>> +    description: |
>>>> +      Phandle to the video input connector, used to find
>>>> +      the HPD gpio and the connector label, both optional.
>>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>>
>>> The binding and driver feel the wrong way around to me. It seems
>>> like you should have a driver for the connector and it needs HPD GPIO,
>>> label, and EEPROM. The driver instead looks mostly like an EEPROM driver
>>> that hooks into a few connector properties.
>>
>> A device like this is typically used next to an HDMI receiver: the DDC
>> lines and the HPD line are connected to the EDID EEPROM, and the video
>> is handled by the HDMI receiver.
>>
>> Most HDMI receivers will have the EDID part integrated into the chip itself
>> (see e.g. the adv7604 driver), but that doesn't have to be the case. The EDID
>> can be completely separate, it doesn't matter for the receiver part.
>>
>> In our specific use-case there isn't even an HDMI receiver since the HDMI
>> video is passed through and this EDID EEPROM is used to help debug HDMI
>> issues by presenting custom EDIDs, similar to something like this:
>>
>> https://www.amazon.com/dp/B0722NVQHX
>>
>> The HPD line is controlled by the EDID part since it has to be low if there
>> is no EDID or pulled low for at least 100ms if the EDID is being modified.
> 
> There is no HPD control on the EEPROM itself. So HPD does not belong
> in the EEPROM node. That is the fundamental problem with the binding.

Perhaps there is a terminology issue here: the input-connector phandle
points to a connector (an hdmi-connector in our case, but it can also
be a dvi-connector for example). The driver gets the HPD gpio from
the connector node. Perhaps the example should be extended with the
hdmi-connector node?

Or do you mean that the hdmi-connector node should point to the EDID
driver? In other words, a new property has to be added there to support
a reference to an EDID device (the hdmi connector bindings are currently written
for HDMI output and never used with HDMI input drivers).

Regards,

	Hans

> 
> We've always started bindings without connector nodes and just shove
> properties into whatever node we do have. Then things evolve to be
> more complicated with different h/w and that doesn't work. Start with
> a connector even if you think it is overkill.
> 
>>> Reading the datasheet, I don't see anything special about accessing the
>>> EEPROM from the host (DSP) side. Wouldn't the default at24 driver work?
>>> It exposes regmap and nvmem.
>>
>> No. It is not a regular EEPROM, it is dedicated to store EDIDs. It has to
>> correctly toggle the HPD line and inform other drivers (specifically HDMI CEC)
>> of EDID updates.
>>
>> I don't see how the at24 could work: besides the eeprom i2c address it has to
>> deal with two additional i2c devices: the segment address and the config
>> address of the device itself. Writing to the eeprom from the host side requires
>> a write to the segment address followed by a write to the eeprom part itself,
>> and that's not something the at24 can do. And it is also something very specific
>> to the VESA E-DDC standard (freely downloadable from vesa.org).
> 
> The addressing is different on the DSP (as the datasheet calls it)
> side compared to the DDC side which has the EDID_SEL signal. Linux
> only sees the DSP side, right? Looking at it a bit more, it looks like
> the segment addressing is different from at24 which uses an i2c
> address per segment. It is similar to ee1004 SPD where the segment is
> selected by a write to a 2nd i2c address.
> 
>> Note that historically the first EDID EEPROMs most likely did work like the
>> at24, but as EDID sizes grew beyond 256 bytes the E-DDC standard was created
>> and that departed from the normal EEPROMs.
> 
> What happens if/when you have more than 1 part to support? Why not
> provide a regmap or nvmem to the EDID storage and then the backend
> device can be anything. I could imagine we could have a s/w
> implementation.
> 
> Anyways, I don't really care if you do any of that now, but I still
> think the binding is backwards. It's the connector you are managing,
> not just an EEPROM, so you should bind to the connector and from there
> gather all the resources you need to do that (EEPROM, HPD).
> 
> Rob




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux