dri-devel Archive on lore.kernel.org
 help / color / mirror / Atom feed
From: Jani Nikula <jani.nikula@intel.com>
To: dri-devel@lists.freedesktop.org
Cc: Neil Armstrong <neil.armstrong@linaro.org>,
	Zheyu Ma <zheyuma97@gmail.com>, Robert Foss <rfoss@kernel.org>,
	Martyn Welch <martyn.welch@collabora.co.uk>,
	Jonas Karlman <jonas@kwiboo.se>,
	Jani Nikula <jani.nikula@intel.com>,
	Peter Senna Tschudin <peter.senna@gmail.com>,
	Yuan Can <yuancan@huawei.com>,
	Jernej Skrabec <jernej.skrabec@gmail.com>,
	Ian Ray <ian.ray@ge.com>,
	Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
	Andrzej Hajda <andrzej.hajda@intel.com>,
	Martin Donnelly <martin.donnelly@ge.com>
Subject: [PATCH v2] drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: switch to drm_do_get_edid()
Date: Fri,  6 Oct 2023 12:54:17 +0300	[thread overview]
Message-ID: <20231006095417.109983-1-jani.nikula@intel.com> (raw)
In-Reply-To: <20230901102400.552254-1-jani.nikula@intel.com>

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

v2: Fix build

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>
Tested-by: Ian Ray <ian.ray@ge.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..e93083bbec9d 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);
 }
 
 static int ge_b850v3_lvds_get_modes(struct drm_connector *connector)
-- 
2.39.2


      parent reply	other threads:[~2023-10-06  9:54 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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:52 ` Jani Nikula
2023-09-08  9:13   ` EXT: " Ian Ray
2023-09-12 12:17     ` Jani Nikula
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 ` Jani Nikula [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20231006095417.109983-1-jani.nikula@intel.com \
    --to=jani.nikula@intel.com \
    --cc=Laurent.pinchart@ideasonboard.com \
    --cc=andrzej.hajda@intel.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=ian.ray@ge.com \
    --cc=jernej.skrabec@gmail.com \
    --cc=jonas@kwiboo.se \
    --cc=martin.donnelly@ge.com \
    --cc=martyn.welch@collabora.co.uk \
    --cc=neil.armstrong@linaro.org \
    --cc=peter.senna@gmail.com \
    --cc=rfoss@kernel.org \
    --cc=yuancan@huawei.com \
    --cc=zheyuma97@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox