On Fri, 01 Sep 2023, Jani Nikula <jani.nik...@intel.com> wrote:
> 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.ha...@intel.com>
> Cc: Ian Ray <ian....@ge.com>
> Cc: Jernej Skrabec <jernej.skra...@gmail.com>
> Cc: Jonas Karlman <jo...@kwiboo.se>
> Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> Cc: Martin Donnelly <martin.donne...@ge.com>
> Cc: Martyn Welch <martyn.we...@collabora.co.uk>
> Cc: Neil Armstrong <neil.armstr...@linaro.org>
> Cc: Peter Senna Tschudin <peter.se...@gmail.com>
> Cc: Robert Foss <rf...@kernel.org>
> Cc: Yuan Can <yuan...@huawei.com>
> Cc: Zheyu Ma <zheyum...@gmail.com>
> Signed-off-by: Jani Nikula <jani.nik...@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);
The last NULL param should be dropped, as noted by the build bot.
BR,
Jani.
> }
>
> static int ge_b850v3_lvds_get_modes(struct drm_connector *connector)
--
Jani Nikula, Intel Open Source Graphics Center
