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
