Received-SPF: pass (google.com: domain of linux-kernel+bounces-82404-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240223223958.3887423-1-hsinyi@chromium.org> <20240223223958.3887423-2-hsinyi@chromium.org>
In-Reply-To: <20240223223958.3887423-2-hsinyi@chromium.org>
From: Doug Anderson <dianders@chromium.org>
Date: Mon, 26 Feb 2024 14:29:05 -0800
Message-ID: <CAD=FV=XZJgFhufr8EkUfBYK+00Kb6R3dHob=EY8XUZ8NJmSChA@mail.gmail.com>
Subject: Re: [PATCH 1/2] drm_edid: Add a function to get EDID base block
To: Hsin-Yi Wang <hsinyi@chromium.org>
Cc: Neil Armstrong <neil.armstrong@linaro.org>, Jessica Zhang <quic_jesszhan@quicinc.com>, 
	Sam Ravnborg <sam@ravnborg.org>, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, 
	Maxime Ripard <mripard@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de>, 
	David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>, dri-devel@lists.freedesktop.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi,

On Fri, Feb 23, 2024 at 2:40=E2=80=AFPM Hsin-Yi Wang <hsinyi@chromium.org> =
wrote:
>
> @@ -2770,58 +2770,63 @@ static u32 edid_extract_panel_id(const struct edi=
d *edid)
>  }
>
>  /**
> - * drm_edid_get_panel_id - Get a panel's ID through DDC
> - * @adapter: I2C adapter to use for DDC
> + * drm_edid_get_panel_id - Get a panel's ID from EDID base block
> + * @base_bock: EDID base block that contains panel ID.

s/base_bock/base_block, as identified by:

scripts/kernel-doc -v drivers/gpu/drm/drm_edid.c | less 2>&1

drivers/gpu/drm/drm_edid.c:2787: warning: Function parameter or struct
member 'base_block' not described in 'drm_edid_get_panel_id'
drivers/gpu/drm/drm_edid.c:2787: warning: Excess function parameter
'base_bock' description in 'drm_edid_get_panel_id'


>   *
> - * This function reads the first block of the EDID of a panel and (assum=
ing
> + * This function uses the first block of the EDID of a panel and (assumi=
ng
>   * that the EDID is valid) extracts the ID out of it. The ID is a 32-bit=
 value
>   * (16 bits of manufacturer ID and 16 bits of per-manufacturer ID) that'=
s
>   * supposed to be different for each different modem of panel.
>   *
> + * Return: A 32-bit ID that should be different for each make/model of p=
anel.
> + *         See the functions drm_edid_encode_panel_id() and
> + *         drm_edid_decode_panel_id() for some details on the structure =
of this
> + *         ID.
> + */
> +u32 drm_edid_get_panel_id(void *base_block)
> +{
> +       return edid_extract_panel_id(base_block);
> +}
> +EXPORT_SYMBOL(drm_edid_get_panel_id);
> +
> +/**
> + * drm_edid_get_base_block - Get a panel's EDID base block
> + * @adapter: I2C adapter to use for DDC
> + *
> + * This function returns the first block of the EDID of a panel.
> + *
>   * This function is intended to be used during early probing on devices =
where
>   * more than one panel might be present. Because of its intended use it =
must
> - * assume that the EDID of the panel is correct, at least as far as the =
ID
> - * is concerned (in other words, we don't process any overrides here).
> + * assume that the EDID of the panel is correct, at least as far as the =
base
> + * block is concerned (in other words, we don't process any overrides he=
re).
>   *
>   * NOTE: it's expected that this function and drm_do_get_edid() will bot=
h
>   * be read the EDID, but there is no caching between them. Since we're o=
nly
>   * reading the first block, hopefully this extra overhead won't be too b=
ig.
>   *
> - * Return: A 32-bit ID that should be different for each make/model of p=
anel.
> - *         See the functions drm_edid_encode_panel_id() and
> - *         drm_edid_decode_panel_id() for some details on the structure =
of this
> - *         ID.
> + * Caller should free the base block after use.

Don't you need a "Return:" clause here to document what you're returning?


Other than the kernel-doc nits, this looks fine to me.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

It'll probably need at least an Ack from someone else in the DRM
community before it can land, though, since this is touching a core
file.


-Doug