All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC] drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: switch to drm_do_get_edid()
@ 2023-09-01 10:24 Jani Nikula
  2023-09-01 14:45 ` kernel test robot
                   ` (4 more replies)
  0 siblings, 5 replies; 12+ messages in thread
From: Jani Nikula @ 2023-09-01 10:24 UTC (permalink / raw)
  To: dri-devel
  Cc: Neil Armstrong, Zheyu Ma, Robert Foss, Martyn Welch,
	Jonas Karlman, Jani Nikula, Peter Senna Tschudin, Yuan Can,
	Jernej Skrabec, Ian Ray, Laurent Pinchart, Andrzej Hajda,
	Martin Donnelly

The driver was originally added in commit fcfa0ddc18ed ("drm/bridge:
Drivers for megachips-stdpxxxx-ge-b850v3-fw (LVDS-DP++)"). I tried to
look up the discussion, but didn't find anyone questioning the EDID
reading part.

Why does it not use drm_get_edid() or drm_do_get_edid()?

I don't know where client->addr comes from, so I guess it could be
different from DDC_ADDR, rendering drm_get_edid() unusable.

There's also the comment:

	/* Yes, read the entire buffer, and do not skip the first
	 * EDID_LENGTH bytes.
	 */

But again, there's not a word on *why*.

Maybe we could just use drm_do_get_edid()? I'd like drivers to migrate
away from their own EDID parsing and validity checks, including stop
using drm_edid_block_valid(). (And long term switch to drm_edid_read(),
struct drm_edid, and friends, but this is the first step.)

Cc: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: Ian Ray <ian.ray@ge.com>
Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Martin Donnelly <martin.donnelly@ge.com>
Cc: Martyn Welch <martyn.welch@collabora.co.uk>
Cc: Neil Armstrong <neil.armstrong@linaro.org>
Cc: Peter Senna Tschudin <peter.senna@gmail.com>
Cc: Robert Foss <rfoss@kernel.org>
Cc: Yuan Can <yuancan@huawei.com>
Cc: Zheyu Ma <zheyuma97@gmail.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>

---

I haven't even tried to compile this, and I have no way to test
this. Apologies for the long Cc list; I'm hoping someone could explain
the existing code, and perhaps give this approach a spin.
---
 .../bridge/megachips-stdpxxxx-ge-b850v3-fw.c  | 57 +++----------------
 1 file changed, 9 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
index 460db3c8a08c..0d9eacf3d9b7 100644
--- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
+++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
@@ -65,12 +65,11 @@ struct ge_b850v3_lvds {
 
 static struct ge_b850v3_lvds *ge_b850v3_lvds_ptr;
 
-static u8 *stdp2690_get_edid(struct i2c_client *client)
+static int stdp2690_read_block(void *context, u8 *buf, unsigned int block, size_t len)
 {
+	struct i2c_client *client = context;
 	struct i2c_adapter *adapter = client->adapter;
-	unsigned char start = 0x00;
-	unsigned int total_size;
-	u8 *block = kmalloc(EDID_LENGTH, GFP_KERNEL);
+	unsigned char start = block * EDID_LENGTH;
 
 	struct i2c_msg msgs[] = {
 		{
@@ -81,53 +80,15 @@ static u8 *stdp2690_get_edid(struct i2c_client *client)
 		}, {
 			.addr	= client->addr,
 			.flags	= I2C_M_RD,
-			.len	= EDID_LENGTH,
-			.buf	= block,
+			.len	= len,
+			.buf	= buf,
 		}
 	};
 
-	if (!block)
-		return NULL;
+	if (i2c_transfer(adapter, msgs, 2) != 2)
+		return -1;
 
-	if (i2c_transfer(adapter, msgs, 2) != 2) {
-		DRM_ERROR("Unable to read EDID.\n");
-		goto err;
-	}
-
-	if (!drm_edid_block_valid(block, 0, false, NULL)) {
-		DRM_ERROR("Invalid EDID data\n");
-		goto err;
-	}
-
-	total_size = (block[EDID_EXT_BLOCK_CNT] + 1) * EDID_LENGTH;
-	if (total_size > EDID_LENGTH) {
-		kfree(block);
-		block = kmalloc(total_size, GFP_KERNEL);
-		if (!block)
-			return NULL;
-
-		/* Yes, read the entire buffer, and do not skip the first
-		 * EDID_LENGTH bytes.
-		 */
-		start = 0x00;
-		msgs[1].len = total_size;
-		msgs[1].buf = block;
-
-		if (i2c_transfer(adapter, msgs, 2) != 2) {
-			DRM_ERROR("Unable to read EDID extension blocks.\n");
-			goto err;
-		}
-		if (!drm_edid_block_valid(block, 1, false, NULL)) {
-			DRM_ERROR("Invalid EDID data\n");
-			goto err;
-		}
-	}
-
-	return block;
-
-err:
-	kfree(block);
-	return NULL;
+	return 0;
 }
 
 static struct edid *ge_b850v3_lvds_get_edid(struct drm_bridge *bridge,
@@ -137,7 +98,7 @@ static struct edid *ge_b850v3_lvds_get_edid(struct drm_bridge *bridge,
 
 	client = ge_b850v3_lvds_ptr->stdp2690_i2c;
 
-	return (struct edid *)stdp2690_get_edid(client);
+	return drm_do_get_edid(connector, stdp2690_read_block, client, NULL);
 }
 
 static int ge_b850v3_lvds_get_modes(struct drm_connector *connector)
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-10-06  9:54 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-01 10:24 [RFC] drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: switch to drm_do_get_edid() Jani Nikula
2023-09-01 14:45 ` kernel test robot
2023-09-01 14:52 ` Jani Nikula
2023-09-08  9:13   ` EXT: " Ian Ray
2023-09-12 12:17     ` Jani Nikula
2023-09-01 16:09 ` kernel test robot
2023-09-02  5:39 ` Peter Senna Tschudin
2023-09-04 10:04   ` Jani Nikula
2023-09-04 11:40     ` EXT: " Ian Ray
2023-09-04 13:09     ` Laurent Pinchart
2023-09-04 13:45     ` Peter Senna Tschudin
2023-10-06  9:54 ` [PATCH v2] " Jani Nikula

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.