[PATCH 10/11] drm/imx/tve: convert to struct drm_edid

[Jani Nikula]

On Mon, 20 May 2024, Dmitry Baryshkov <dmitry.baryshkov at linaro.org> wrote:
> On Tue, May 14, 2024 at 03:55:16PM +0300, Jani Nikula wrote:
>> Prefer the struct drm_edid based functions for reading the EDID and
>> updating the connector.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
>> 
>> ---
>> 
>> Cc: Philipp Zabel <p.zabel at pengutronix.de>
>> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> Cc: Maxime Ripard <mripard at kernel.org>
>> Cc: Thomas Zimmermann <tzimmermann at suse.de>
>> Cc: Shawn Guo <shawnguo at kernel.org>
>> Cc: Sascha Hauer <s.hauer at pengutronix.de>
>> Cc: Pengutronix Kernel Team <kernel at pengutronix.de>
>> Cc: Fabio Estevam <festevam at gmail.com>
>> Cc: imx at lists.linux.dev
>> Cc: linux-arm-kernel at lists.infradead.org
>> ---
>>  drivers/gpu/drm/imx/ipuv3/imx-tve.c | 14 ++++++--------
>>  1 file changed, 6 insertions(+), 8 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/imx/ipuv3/imx-tve.c b/drivers/gpu/drm/imx/ipuv3/imx-tve.c
>> index b49bddb85535..29f494bfff67 100644
>> --- a/drivers/gpu/drm/imx/ipuv3/imx-tve.c
>> +++ b/drivers/gpu/drm/imx/ipuv3/imx-tve.c
>> @@ -201,18 +201,16 @@ static int tve_setup_vga(struct imx_tve *tve)
>>  static int imx_tve_connector_get_modes(struct drm_connector *connector)
>>  {
>>  	struct imx_tve *tve = con_to_tve(connector);
>> -	struct edid *edid;
>> -	int ret = 0;
>> +	const struct drm_edid *drm_edid;
>> +	int ret;
>>  
>>  	if (!tve->ddc)
>>  		return 0;
>>  
>> -	edid = drm_get_edid(connector, tve->ddc);
>> -	if (edid) {
>> -		drm_connector_update_edid_property(connector, edid);
>> -		ret = drm_add_edid_modes(connector, edid);
>> -		kfree(edid);
>> -	}
>> +	drm_edid = drm_edid_read_ddc(connector, tve->ddc);
>> +	drm_edid_connector_update(connector, drm_edid);
>> +	ret = drm_edid_connector_add_modes(connector);
>> +	drm_edid_free(drm_edid);
>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>
> Nit: if you change two last lines, you can drop ret= assignment and use
> return drm_edid_connector_add_modes(connector).
>
> Maybe we shoud add cocci rule for such cases.

I think there was a cocci rule like that, but a lot of people (including
yours truly) preferred to keep the assignment, and the patches ended up
in bikeshedding, so the cocci was removed.

My argument is that it's not uncommon to keep adding and removing stuff
while the code evolves, and having to change the return statement is
boring and makes the diff harder to follow. It's a bit like that extra
comma at the end of initialization lists or enumeration definitions.

Others think the code should reflect current state and not prepare for
scenarios that might never arrive.

Both are correct, so it's perfect for never ending bikeshedding. ;)


BR,
Jani.


>
>>  
>>  	return ret;
>>  }
>> -- 
>> 2.39.2
>> 

-- 
Jani Nikula, Intel