[PATCH v4 0/5] Add the cat24c208 EDID EEPROM driver + new EDID capability

[PATCH v4 0/5] Add the cat24c208 EDID EEPROM driver + new EDID capability

[Erling Ljunggren]

This series adds support for the standalone cat24c208 EDID EEPROM i2c device.
Usually EDID support is part of an HDMI receiver, but this is a standalone EEPROM.

Note that EEPROMs for EDIDs are not regular EEPROM devices, these are dual port
devices that follow the VESA E-DDC standard.

Since this is a standalone device that does not capture any video a new
V4L2_CAP_EDID capability is introduced to represent such devices.
Note that such a device doesn't have to be an EEPROM, it can also be
implemented using a microcontroller, for example.

v4:
 - update driver and bindings to support a video input connector phandle
 - use connector phandle to get HPD gpio and input label

v3:
 - use old V4L2_CAP_ASYNCIO (0x02000000) capability bit
 - validate physical address of edid in driver
 - handle empty edid in driver
 - add cec notifier support to driver
 - update driver and bindings with hpd gpio support
 - removed references to "memory" in capability and docs
 - associate ioctls based on device direction

v2:
 - fix dt binding example
 - rename i2c client variables in data struct
 - fix include: of_device.h -> mod_devicetable.h
 - sorted makefile
 - used define EDID_OFFSET_EXT_FLAG instead of magic number
 - removed of_match_ptr
 - added bus_info
 - remove unneeded headers
 - add depends on OF to Kconfig

Erling Ljunggren (4):
  media: videodev2.h: add V4L2_CAP_EDID
  media: docs: Add V4L2_CAP_EDID
  dt-bindings: media: add cat24c208 bindings
  media: v4l2-dev: handle V4L2_CAP_EDID

Jonathan Selnes (1):
  media: i2c: cat24c208: driver for the cat24c208 EDID EEPROM

 .../bindings/media/i2c/onnn,cat24c208.yaml    |  46 ++
 .../userspace-api/media/v4l/biblio.rst        |  11 +
 .../media/v4l/vidioc-querycap.rst             |  11 +
 .../media/videodev2.h.rst.exceptions          |   1 +
 MAINTAINERS                                   |   7 +
 drivers/media/i2c/Kconfig                     |   9 +
 drivers/media/i2c/Makefile                    |   1 +
 drivers/media/i2c/cat24c208.c                 | 499 ++++++++++++++++++
 drivers/media/v4l2-core/v4l2-dev.c            |  15 +
 include/uapi/linux/videodev2.h                |   1 +
 10 files changed, 601 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/onnn,cat24c208.yaml
 create mode 100644 drivers/media/i2c/cat24c208.c

-- 
2.38.0

[PATCH v4 1/5] media: videodev2.h: add V4L2_CAP_EDID

[Erling Ljunggren]

Add capability flag to indicate that the device is an EDID-only device.

Signed-off-by: Erling Ljunggren <hljunggr@cisco.com>
---
 include/uapi/linux/videodev2.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 054fc8bbdb22..2ea95639f441 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -480,6 +480,7 @@ struct v4l2_capability {
 #define V4L2_CAP_META_CAPTURE		0x00800000  /* Is a metadata capture device */
 
 #define V4L2_CAP_READWRITE              0x01000000  /* read/write systemcalls */
+#define V4L2_CAP_EDID			0x02000000  /* Is an EDID-only device */
 #define V4L2_CAP_STREAMING              0x04000000  /* streaming I/O ioctls */
 #define V4L2_CAP_META_OUTPUT		0x08000000  /* Is a metadata output device */
 
-- 
2.38.0

[PATCH v4 2/5] media: docs: Add V4L2_CAP_EDID

[Erling Ljunggren]

Add documentation for the new edid capability.

Signed-off-by: Erling Ljunggren <hljunggr@cisco.com>
---
 Documentation/userspace-api/media/v4l/biblio.rst      | 11 +++++++++++
 .../userspace-api/media/v4l/vidioc-querycap.rst       | 11 +++++++++++
 .../userspace-api/media/videodev2.h.rst.exceptions    |  1 +
 3 files changed, 23 insertions(+)

diff --git a/Documentation/userspace-api/media/v4l/biblio.rst b/Documentation/userspace-api/media/v4l/biblio.rst
index 9cd18c153d19..5cbe41877a63 100644
--- a/Documentation/userspace-api/media/v4l/biblio.rst
+++ b/Documentation/userspace-api/media/v4l/biblio.rst
@@ -334,6 +334,17 @@ VESA DMT
 
 :author:    Video Electronics Standards Association (http://www.vesa.org)
 
+.. _vesaeddc:
+
+E-DDC
+====
+
+
+:title:     VESA Enhanced Display Data Channel (E-DDC) Standard
+:subtitle:  Version 1.3
+
+:author:    Video Electronics Standards Association (http://www.vesa.org)
+
 .. _vesaedid:
 
 EDID
diff --git a/Documentation/userspace-api/media/v4l/vidioc-querycap.rst b/Documentation/userspace-api/media/v4l/vidioc-querycap.rst
index 6c57b8428356..3d11d86d9cbf 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-querycap.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-querycap.rst
@@ -244,6 +244,17 @@ specification the ioctl returns an ``EINVAL`` error code.
       - 0x01000000
       - The device supports the :c:func:`read()` and/or
 	:c:func:`write()` I/O methods.
+    * - ``V4L2_CAP_EDID``
+      - 0x02000000
+      - The device stores the EDID for a video input, or retrieves the EDID for a video
+        output. It is a standalone EDID device, so no video streaming etc. will take place.
+
+        For a video input this is typically an eeprom that supports the
+        :ref:`VESA Enhanced Display Data Channel Standard <vesaeddc>`. It can be something
+        else as well, for example a micro controller.
+
+        For a video output this is typically read from an external device such as an
+        HDMI splitter accessed by a serial port.
     * - ``V4L2_CAP_STREAMING``
       - 0x04000000
       - The device supports the :ref:`streaming <mmap>` I/O method.
diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
index 9cbb7a0c354a..b1b1127d278c 100644
--- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
+++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
@@ -185,6 +185,7 @@ replace define V4L2_CAP_META_OUTPUT device-capabilities
 replace define V4L2_CAP_DEVICE_CAPS device-capabilities
 replace define V4L2_CAP_TOUCH device-capabilities
 replace define V4L2_CAP_IO_MC device-capabilities
+replace define V4L2_CAP_EDID device-capabilities
 
 # V4L2 pix flags
 replace define V4L2_PIX_FMT_PRIV_MAGIC :c:type:`v4l2_pix_format`
-- 
2.38.0

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

[Erling Ljunggren]

Add devicetree bindings for new cat24c208 EDID EEPROM driver.

Signed-off-by: Erling Ljunggren <hljunggr@cisco.com>
---
 .../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@xs4all.nl>
+
+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
+
+required:
+  - compatible
+  - reg
+  - input-connector
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        cat24c208@31 {
+            compatible = "onnn,cat24c208";
+            reg = <0x31>;
+            input-connector = <&hdmi_connector_in>;
+        };
+    };
-- 
2.38.0

[PATCH v4 4/5] media: i2c: cat24c208: driver for the cat24c208 EDID EEPROM

[Erling Ljunggren]

From: Jonathan Selnes <jonathansb1@gmail.com>

Support reading and writing the EDID EEPROM through the
v4l2 API.

Signed-off-by: Jonathan Selnes <jonathansb1@gmail.com>
Co-developed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Co-developed-by: Erling Ljunggren <hljunggr@cisco.com>
Signed-off-by: Erling Ljunggren <hljunggr@cisco.com>
---
 MAINTAINERS                   |   7 +
 drivers/media/i2c/Kconfig     |   9 +
 drivers/media/i2c/Makefile    |   1 +
 drivers/media/i2c/cat24c208.c | 499 ++++++++++++++++++++++++++++++++++
 4 files changed, 516 insertions(+)
 create mode 100644 drivers/media/i2c/cat24c208.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 7d9490a5c15a..407d76c42bab 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14912,6 +14912,13 @@ S:	Maintained
 T:	git git://linuxtv.org/media_tree.git
 F:	drivers/media/i2c/ov9734.c
 
+ON SEMICONDUCTOR CAT24C208 EDID EEPROM DRIVER
+M:	Hans Verkuil <hverkuil-cisco@xs4all.nl>
+L:	linux-media@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/media/i2c/onnn,cat24c208.yaml
+F:	drivers/media/i2c/cat24c208*
+
 ONENAND FLASH DRIVER
 M:	Kyungmin Park <kyungmin.park@samsung.com>
 L:	linux-mtd@lists.infradead.org
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 2b20aa6c37b1..2f5f9f058b48 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -1530,6 +1530,15 @@ endmenu
 menu "Miscellaneous helper chips"
 	visible if !MEDIA_HIDE_ANCILLARY_SUBDRV
 
+config VIDEO_CAT24C208
+	tristate "ON Semiconductor cat24c208 EDID EEPROM"
+	depends on VIDEO_DEV && I2C && OF
+	help
+	  Support for the ON Semiconductor CAT24C208 Dual Port EDID EEPROM.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called cat24c208.
+
 config VIDEO_I2C
 	tristate "I2C transport video support"
 	depends on VIDEO_DEV && I2C
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 3e1696963e7f..70e4360a21ba 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_VIDEO_APTINA_PLL) += aptina-pll.o
 obj-$(CONFIG_VIDEO_BT819) += bt819.o
 obj-$(CONFIG_VIDEO_BT856) += bt856.o
 obj-$(CONFIG_VIDEO_BT866) += bt866.o
+obj-$(CONFIG_VIDEO_CAT24C208) += cat24c208.o
 obj-$(CONFIG_VIDEO_CCS) += ccs/
 obj-$(CONFIG_VIDEO_CCS_PLL) += ccs-pll.o
 obj-$(CONFIG_VIDEO_CS3308) += cs3308.o
diff --git a/drivers/media/i2c/cat24c208.c b/drivers/media/i2c/cat24c208.c
new file mode 100644
index 000000000000..153cefcb5f68
--- /dev/null
+++ b/drivers/media/i2c/cat24c208.c
@@ -0,0 +1,499 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * HDMI i2c controlled EEPROM from ON Semiconductor or Catalyst Semiconductor
+ *
+ * Support for i2c based DDC EEPROM
+ *
+ * Copyright (C) 2021-2022 Cisco Systems, Inc. and/or its affiliates. All rights reserved.
+ */
+
+/*
+ * REF_01 - ON Semiconductor, cat24c208, Datasheet, URL : https://www.onsemi.com/pdf/datasheet/cat24c208-d.pdf
+ *	Revision 7, May 2018
+ */
+
+#include "linux/of.h"
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/mod_devicetable.h>
+#include <linux/gpio/consumer.h>
+#include <linux/slab.h>
+#include <linux/videodev2.h>
+#include <linux/workqueue.h>
+#include <linux/of.h>
+
+#include <media/cec-notifier.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-dv-timings.h>
+#include <media/v4l2-fh.h>
+#include <media/v4l2-ioctl.h>
+
+MODULE_DESCRIPTION("cat24c208 EDID EEPROM driver");
+MODULE_AUTHOR("Jonathan Selnes Bognaes <jonathansb1@gmail.com>");
+MODULE_LICENSE("GPL");
+
+/*
+ * CAT24C208 setup
+ */
+#define BYTES_PER_BLOCK		128
+#define EDID_OFFSET_EXT_FLAG	126
+#define MAX_WRITE_BYTES		16
+#define NUM_BLOCKS		4
+#define NUM_CLIENTS		3
+#define CONFIG_NB_BIT		BIT(0)
+#define CONFIG_AB0_BIT		BIT(1)
+#define CONFIG_AB1_BIT		BIT(2)
+#define CONFIG_WE_BIT		BIT(3)
+
+/*
+ * From the datasheet: REF_01
+ *
+ * The write cycle time is the time from a valid stop condition of a write
+ * sequence to the end of the internal program/erase cycle. During the write
+ * cycle, the bus interface circuits are disabled, SDA is allowed to remain
+ * high, and the device does not respond to its slave address.
+ */
+#define WRITE_CYCLE_TIME_US	5000
+
+/*
+ * CAT24C208 addresses
+ */
+#define CONFIG_I2C_ADDR		0x31
+#define EEPROM_I2C_ADDR		0x50
+#define SEGMENT_I2C_ADDR	0x30
+
+struct cat24c208_state {
+	struct i2c_client *dev_client;
+	struct i2c_client *data_client;
+	struct i2c_client *seg_client;
+	// V4L2 ioctl serialization
+	struct mutex lock;
+	struct cec_notifier *notifier;
+	struct delayed_work dwork_enable_hpd;
+
+	struct v4l2_device v4l2_dev;
+	struct video_device vdev;
+	struct gpio_desc *hpd_gpio;
+	const char *connector_name;
+
+	u8 edid_blocks;		// edid length can vary, one block = 128 bytes
+	u8 edid[BYTES_PER_BLOCK * NUM_BLOCKS]; // actual active edid data
+};
+
+static const struct v4l2_file_operations cat24c208_fops = {
+	.owner		= THIS_MODULE,
+	.open		= v4l2_fh_open,
+	.release	= v4l2_fh_release,
+	.unlocked_ioctl = video_ioctl2,
+};
+
+static int cat24c208_seg_write(struct cat24c208_state *state, u8 *data, u16 len, u8 seg)
+{
+	struct i2c_msg msg[] = {
+		{
+			.addr = state->seg_client->addr,	// Segment
+			.buf = &seg,
+			.len = 1,
+			.flags = 0,
+		},
+		{
+			.addr = state->data_client->addr,	// write data
+			.buf = data,
+			.len = len,
+			.flags = 0,
+		},
+	};
+	int err;
+
+	if (seg)
+		err = i2c_transfer(state->dev_client->adapter, msg, ARRAY_SIZE(msg));
+	else
+		err = i2c_transfer(state->dev_client->adapter, &msg[1], 1);
+
+	if (err < 0)
+		dev_err(&state->dev_client->dev, "Writing to 0x%x failed (segment %d)\n",
+			state->data_client->addr, seg);
+
+	usleep_range(WRITE_CYCLE_TIME_US, 2 * WRITE_CYCLE_TIME_US);
+	return err < 0 ? err : 0;
+}
+
+static int cat24c208_edid_read(struct cat24c208_state *state, u8 *data, u8 seg, u8 offset, u16 len)
+{
+	int err;
+
+	len *= BYTES_PER_BLOCK;
+	if (seg) {
+		struct i2c_msg msg[] = {
+			{
+				.addr = state->seg_client->addr,	// Segment
+				.buf = &seg,
+				.len = 1,
+				.flags = 0,
+			},
+			{
+				.addr = state->data_client->addr,	// read data
+				.buf = data,
+				.len = len,
+				.flags = I2C_M_RD,
+			},
+		};
+		err = i2c_transfer(state->dev_client->adapter, msg, ARRAY_SIZE(msg));
+	} else {
+		struct i2c_msg msg[] = {
+			{
+				.addr = state->data_client->addr,	// set offset
+				.buf = &offset,
+				.len = 1,
+				.flags = 0,
+			},
+			{
+				.addr = state->data_client->addr,	// read data
+				.buf = data,
+				.len = len,
+				.flags = I2C_M_RD,
+			},
+		};
+		err = i2c_transfer(state->dev_client->adapter, msg, ARRAY_SIZE(msg));
+	}
+
+	if (err < 0)
+		dev_err(&state->dev_client->dev, "Reading of EDID failed\n");
+	return err < 0 ? err : 0;
+}
+
+static int cat24c208_set_config(struct i2c_client *client)
+{
+	u8 buf[2] = { 0, CONFIG_NB_BIT };
+	struct i2c_msg msg = {
+		.addr = client->addr,
+		.buf = buf,
+		.len = sizeof(buf),
+		.flags = 0,
+	};
+	int err;
+
+	err = i2c_transfer(client->adapter, &msg, 1);
+	if (err < 0)
+		dev_err(&client->dev, "Could not set config register\n");
+
+	usleep_range(WRITE_CYCLE_TIME_US, 2 * WRITE_CYCLE_TIME_US);
+	return err < 0 ? err : 0;
+}
+
+static bool cat24c208_is_valid_edid(const u8 *block)
+{
+	static const u8 header_pattern[] = {
+		0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00
+	};
+
+	return !memcmp(block, header_pattern, sizeof(header_pattern));
+}
+
+static void cat24c208_delayed_work_release_hpd(struct work_struct *work)
+{
+	struct delayed_work *dwork = to_delayed_work(work);
+	struct cat24c208_state *state = container_of(dwork, struct cat24c208_state,
+						     dwork_enable_hpd);
+
+	gpiod_set_value(state->hpd_gpio, 1);
+}
+
+static int cat24c208_set_edid(struct file *file, void *fh, struct v4l2_edid *edid)
+{
+	struct cat24c208_state *state = video_drvdata(file);
+	u8 buf[MAX_WRITE_BYTES + 1];
+	u16 pa;
+	int err;
+	int seg;
+	int i;
+
+	memset(edid->reserved, 0, sizeof(edid->reserved));
+
+	if (edid->pad)
+		return -EINVAL;
+
+	if (edid->blocks > NUM_BLOCKS) {
+		edid->blocks = NUM_BLOCKS;
+		return -E2BIG;
+	}
+
+	if (edid->start_block)
+		return -EINVAL;
+
+	pa = v4l2_get_edid_phys_addr(edid->edid, edid->blocks * BYTES_PER_BLOCK, NULL);
+
+	err = v4l2_phys_addr_validate(pa, NULL, NULL);
+	if (err)
+		return err;
+
+	if (state->hpd_gpio) {
+		cancel_delayed_work_sync(&state->dwork_enable_hpd);
+		gpiod_set_value(state->hpd_gpio, 0);
+	}
+
+	if (edid->blocks == 0) {
+		cec_notifier_set_phys_addr(state->notifier, pa);
+		return 0;
+	}
+
+	state->edid_blocks = edid->blocks;
+	memcpy(state->edid, edid->edid, state->edid_blocks * BYTES_PER_BLOCK);
+
+	/* Write EDID to EEPROM */
+	for (i = 0; i < edid->blocks * BYTES_PER_BLOCK; i = i + MAX_WRITE_BYTES) {
+		if (i >= 2 * BYTES_PER_BLOCK) {
+			seg = 1;
+			buf[0] = i - BYTES_PER_BLOCK * 2;
+		} else {
+			seg = 0;
+			buf[0] = i;
+		}
+
+		memcpy(buf + 1, &edid->edid[i], MAX_WRITE_BYTES);
+		err = cat24c208_seg_write(state, buf, MAX_WRITE_BYTES + 1, seg);
+		if (err) {
+			dev_err(&state->dev_client->dev,
+				"Could not write EDID to EEPROM, i: %d\n", i);
+			return err;
+		}
+	}
+
+	cec_notifier_set_phys_addr(state->notifier, pa);
+
+	if (state->hpd_gpio)
+		schedule_delayed_work(&state->dwork_enable_hpd, 110 * HZ / 1000);
+
+	return 0;
+}
+
+static int cat24c208_get_edid(struct file *file, void *fh, struct v4l2_edid *edid)
+{
+	struct cat24c208_state *state = video_drvdata(file);
+
+	memset(edid->reserved, 0, sizeof(edid->reserved));
+
+	if (edid->pad != 0)
+		return -EINVAL;
+
+	if (edid->start_block == 0 && edid->blocks == 0) {
+		edid->blocks = state->edid_blocks;
+		return 0;
+	}
+
+	if (state->edid_blocks == 0)
+		return -ENODATA;
+
+	if (edid->start_block >= state->edid_blocks)
+		return -EINVAL;
+
+	if (edid->start_block + edid->blocks > state->edid_blocks)
+		edid->blocks = state->edid_blocks - edid->start_block;
+
+	memcpy(edid->edid, state->edid + edid->start_block * BYTES_PER_BLOCK,
+	       edid->blocks * BYTES_PER_BLOCK);
+
+	return 0;
+}
+
+static int cat24c208_get_input(struct file *file, void *priv, unsigned int *i)
+{
+	*i = 0;
+	return 0;
+}
+
+static int cat24c208_set_input(struct file *file, void *priv, unsigned int i)
+{
+	return i > 0 ? -EINVAL : 0;
+}
+
+static int cat24c208_enum_input(struct file *file, void *priv,
+				struct v4l2_input *inp)
+{
+	struct cat24c208_state *state = video_drvdata(file);
+
+	if (inp->index)
+		return -EINVAL;
+	strscpy(inp->name, state->connector_name, sizeof(inp->name));
+	inp->capabilities = 0;
+	inp->type = V4L2_INPUT_TYPE_CAMERA;
+	return 0;
+}
+
+static int cat24c208_querycap(struct file *file,
+			      void *priv, struct v4l2_capability *cap)
+{
+	struct cat24c208_state *state = video_drvdata(file);
+	struct i2c_client *client = state->dev_client;
+
+	strscpy(cap->driver, "cat24c208", sizeof(cap->driver));
+	strscpy(cap->card, "cat24c208 EDID EEPROM", sizeof(cap->card));
+	snprintf(cap->bus_info, sizeof(cap->bus_info),
+		 "I2C:%d-%04x", client->adapter->nr, client->addr);
+	return 0;
+}
+
+static const struct v4l2_ioctl_ops cat24c208_ioctl_ops = {
+	.vidioc_querycap	= cat24c208_querycap,
+	.vidioc_g_edid		= cat24c208_get_edid,
+	.vidioc_s_edid		= cat24c208_set_edid,
+	.vidioc_g_input		= cat24c208_get_input,
+	.vidioc_s_input		= cat24c208_set_input,
+	.vidioc_enum_input	= cat24c208_enum_input,
+};
+
+static void cat24c208_release(struct video_device *vdev)
+{
+	struct cat24c208_state *state = video_get_drvdata(vdev);
+
+	v4l2_device_unregister(&state->v4l2_dev);
+	mutex_destroy(&state->lock);
+	kfree(state);
+}
+
+static int cat24c208_probe(struct i2c_client *client)
+{
+	struct cat24c208_state *state;
+	struct v4l2_device *v4l2_dev;
+	struct device_node *input_connector = NULL;
+	int blocks;
+	int ret;
+
+	state = kzalloc(sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return -ENOMEM;
+
+	input_connector = of_parse_phandle(client->dev.of_node, "input-connector", 0);
+	if (!input_connector) {
+		dev_err(&client->dev, "Failed to get input_connector\n");
+		ret = -ENODEV;
+		goto free_state;
+	}
+
+	state->hpd_gpio = devm_gpiod_get_from_of_node(&client->dev, input_connector,
+						      "hpd-gpios", 0, GPIOD_OUT_LOW, "hpd");
+
+	state->connector_name = "HDMI IN";
+	ret = of_property_read_string_index(input_connector, "label", 0, &state->connector_name);
+
+	of_node_put(input_connector);
+
+	if (IS_ERR(state->hpd_gpio) && PTR_ERR(state->hpd_gpio) != -ENOENT) {
+		ret = PTR_ERR(state->hpd_gpio);
+		dev_err(&client->dev, "Failed to get hpd-gpio err: %d\n", ret);
+		goto free_state;
+	}
+
+	state->dev_client = client;
+	state->data_client = i2c_new_ancillary_device(client, "eeprom", EEPROM_I2C_ADDR);
+	if (IS_ERR(state->data_client)) {
+		ret = PTR_ERR(state->data_client);
+		goto free_state;
+	}
+	state->seg_client = i2c_new_ancillary_device(client, "segment", SEGMENT_I2C_ADDR);
+	if (IS_ERR(state->seg_client)) {
+		ret = PTR_ERR(state->seg_client);
+		goto unreg_i2c_first;
+	}
+
+	ret = cat24c208_set_config(client);
+	if (ret)
+		goto unreg_i2c_all;
+
+	if (cat24c208_edid_read(state, state->edid, 0, 0, 2) >= 0 &&
+	    cat24c208_is_valid_edid(state->edid)) {
+		unsigned int i;
+
+		blocks = 1 + state->edid[EDID_OFFSET_EXT_FLAG];
+		state->edid_blocks = blocks;
+		for (i = 2; i < blocks; i += 2) {
+			if (cat24c208_edid_read(state, state->edid + i * BYTES_PER_BLOCK,
+						i / 2, 0, (i + 1 >= blocks ? 1 : 2))) {
+				state->edid_blocks = i;
+				break;
+			}
+		}
+	}
+
+	v4l2_dev = &state->v4l2_dev;
+	strscpy(v4l2_dev->name, "cat24c208", sizeof(v4l2_dev->name));
+	ret = v4l2_device_register(&client->dev, v4l2_dev);
+	if (ret) {
+		dev_err(&client->dev, "v4l2_device_register failed: %d\n", ret);
+		goto unreg_i2c_all;
+	}
+
+	mutex_init(&state->lock);
+
+	state->notifier = cec_notifier_conn_register(&client->dev, NULL, NULL);
+	if (!state->notifier) {
+		dev_err(&client->dev, "Failed to get CEC notifier\n");
+		ret = -ENOMEM;
+		goto unreg_i2c_all;
+	}
+
+	INIT_DELAYED_WORK(&state->dwork_enable_hpd, cat24c208_delayed_work_release_hpd);
+
+	snprintf(state->vdev.name, sizeof(state->vdev.name),
+		 "cat24c208 %d-%d", client->adapter->nr, client->addr);
+
+	state->vdev.v4l2_dev = v4l2_dev;
+	state->vdev.fops = &cat24c208_fops;
+	state->vdev.ioctl_ops = &cat24c208_ioctl_ops;
+	state->vdev.lock = &state->lock;
+	state->vdev.release = cat24c208_release;
+	state->vdev.device_caps = V4L2_CAP_EDID;
+
+	video_set_drvdata(&state->vdev, state);
+	i2c_set_clientdata(client, state);
+	ret = video_register_device(&state->vdev, VFL_TYPE_VIDEO, -1);
+	if (ret != 0) {
+		dev_err(&client->dev, "Video registering failed: %d\n", ret);
+		goto unreg_v4l2_dev;
+	}
+	return 0;
+
+unreg_v4l2_dev:
+	v4l2_device_unregister(&state->v4l2_dev);
+	cec_notifier_conn_unregister(state->notifier);
+	cancel_delayed_work_sync(&state->dwork_enable_hpd);
+unreg_i2c_all:
+	i2c_unregister_device(state->seg_client);
+unreg_i2c_first:
+	i2c_unregister_device(state->data_client);
+free_state:
+	kfree(state);
+	return ret;
+}
+
+static int cat24c208_remove(struct i2c_client *client)
+{
+	struct cat24c208_state *state = i2c_get_clientdata(client);
+
+	cancel_delayed_work_sync(&state->dwork_enable_hpd);
+	cec_notifier_conn_unregister(state->notifier);
+	i2c_unregister_device(state->data_client);
+	i2c_unregister_device(state->seg_client);
+	video_unregister_device(&state->vdev);
+
+	return 0;
+}
+
+static const struct of_device_id cat24c208_of_match[] = {
+	{ .compatible = "onnn,cat24c208"},
+	{}
+};
+MODULE_DEVICE_TABLE(of, cat24c208_of_match);
+
+static struct i2c_driver cat24c208_driver = {
+	.driver = {
+		.name	= "cat24c208",
+		.of_match_table = cat24c208_of_match,
+	},
+	.probe_new	= cat24c208_probe,
+	.remove		= cat24c208_remove,
+};
+module_i2c_driver(cat24c208_driver);
-- 
2.38.0

[PATCH v4 5/5] media: v4l2-dev: handle V4L2_CAP_EDID

[Erling Ljunggren]

When the V4L2_CAP_EDID capability flag is set,
ioctls for enum inputs/outputs and get/set edid are automatically set.

Signed-off-by: Erling Ljunggren <hljunggr@cisco.com>
---
 drivers/media/v4l2-core/v4l2-dev.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index d00237ee4cae..e8222b9835e6 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -556,6 +556,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
 	bool is_rx = vdev->vfl_dir != VFL_DIR_TX;
 	bool is_tx = vdev->vfl_dir != VFL_DIR_RX;
 	bool is_io_mc = vdev->device_caps & V4L2_CAP_IO_MC;
+	bool is_edid =  vdev->device_caps & V4L2_CAP_EDID;
 
 	bitmap_zero(valid_ioctls, BASE_VIDIOC_PRIVATE);
 
@@ -778,6 +779,20 @@ static void determine_valid_ioctls(struct video_device *vdev)
 		SET_VALID_IOCTL(ops, VIDIOC_S_TUNER, vidioc_s_tuner);
 		SET_VALID_IOCTL(ops, VIDIOC_S_HW_FREQ_SEEK, vidioc_s_hw_freq_seek);
 	}
+	if (is_edid) {
+		SET_VALID_IOCTL(ops, VIDIOC_G_EDID, vidioc_g_edid);
+		if (is_tx) {
+			SET_VALID_IOCTL(ops, VIDIOC_G_OUTPUT, vidioc_g_output);
+			SET_VALID_IOCTL(ops, VIDIOC_S_OUTPUT, vidioc_s_output);
+			SET_VALID_IOCTL(ops, VIDIOC_ENUMOUTPUT, vidioc_enum_output);
+		}
+		if (is_rx) {
+			SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
+			SET_VALID_IOCTL(ops, VIDIOC_G_INPUT, vidioc_g_input);
+			SET_VALID_IOCTL(ops, VIDIOC_S_INPUT, vidioc_s_input);
+			SET_VALID_IOCTL(ops, VIDIOC_S_EDID, vidioc_s_edid);
+		}
+	}
 
 	bitmap_andnot(vdev->valid_ioctls, valid_ioctls, vdev->valid_ioctls,
 			BASE_VIDIOC_PRIVATE);
-- 
2.38.0

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

[Rob Herring]

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@cisco.com>
> ---
>  .../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@xs4all.nl>
> +
> +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.

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.

Rob

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

[Hans Verkuil]

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@cisco.com>
>> ---
>>  .../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@xs4all.nl>
>> +
>> +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.

> 
> 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).

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.

Regards,

	Hans

> 
> Rob

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

[Rob Herring]

+Bartosz

On Fri, Nov 18, 2022 at 4:34 AM Hans Verkuil <hverkuil@xs4all.nl> 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@cisco.com>
> >> ---
> >>  .../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@xs4all.nl>
> >> +
> >> +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.

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

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

[Hans Verkuil]

On 11/18/22 16:20, Rob Herring wrote:
> +Bartosz
> 
> On Fri, Nov 18, 2022 at 4:34 AM Hans Verkuil <hverkuil@xs4all.nl> 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@cisco.com>
>>>> ---
>>>>  .../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@xs4all.nl>
>>>> +
>>>> +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

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

[Rob Herring]

On Fri, Nov 18, 2022 at 9:46 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 11/18/22 16:20, Rob Herring wrote:
> > +Bartosz
> >
> > On Fri, Nov 18, 2022 at 4:34 AM Hans Verkuil <hverkuil@xs4all.nl> 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@cisco.com>
> >>>> ---
> >>>>  .../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@xs4all.nl>
> >>>> +
> >>>> +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?

You do need to define a connector node. But 'hdmi-connector' is taken
and means an hdmi output (though maybe it got used by someone for an
input?). The input side has and does different things, so we should
define a new compatible rather than complicate our lives trying to
reuse and extend the current one. So you need 'hdmi-in-connector'
compatible or something like that. You can possibly add this to the
existing hdmi-connector schema depending on how much the existing
properties may apply. For example, ddc-i2c-bus points to an i2c slave
for a s/w EDID?

> Or do you mean that the hdmi-connector node should point to the EDID
> driver?

DT does not have drivers, but yes, the HDMI connector node should
point to the EEPROM h/w device.

Rob