[Intel-gfx] [PATCH 4/6] drm/edid: use a temp variable for sads to drop one level of dereferences

[Golani, Mitulkumar Ajitkumar]

Hi Jani,

added comments in-line.

> -----Original Message-----
> From: Nikula, Jani <jani.nikula at intel.com>
> Sent: Thursday, September 7, 2023 2:58 PM
> To: dri-devel at lists.freedesktop.org
> Cc: intel-gfx at lists.freedesktop.org; Nikula, Jani <jani.nikula at intel.com>;
> Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani at intel.com>
> Subject: [PATCH 4/6] drm/edid: use a temp variable for sads to drop one level
> of dereferences
> 
> It's arguably easier on the eyes, and drops a set of parenthesis too.

Please consider providing a bit more context in the commit message for better clarity.

> 
> Cc: Mitul Golani <mitulkumar.ajitkumar.golani at intel.com>
> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index
> 2025970816c9..fcdc2c314cde 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -5583,7 +5583,7 @@ static void drm_edid_to_eld(struct drm_connector
> *connector,  }
> 
>  static int _drm_edid_to_sad(const struct drm_edid *drm_edid,
> -			    struct cea_sad **sads)
> +			    struct cea_sad **psads)
>  {
>  	const struct cea_db *db;
>  	struct cea_db_iter iter;
> @@ -5592,19 +5592,21 @@ static int _drm_edid_to_sad(const struct
> drm_edid *drm_edid,
>  	cea_db_iter_edid_begin(drm_edid, &iter);
>  	cea_db_iter_for_each(db, &iter) {
>  		if (cea_db_tag(db) == CTA_DB_AUDIO) {
> +			struct cea_sad *sads;
>  			int j;
> 
>  			count = cea_db_payload_len(db) / 3; /* SAD is 3B */
> -			*sads = kcalloc(count, sizeof(**sads), GFP_KERNEL);
> -			if (!*sads)
> +			sads = kcalloc(count, sizeof(*sads), GFP_KERNEL);
> +			*psads = sads;
> +			if (!sads)
>  				return -ENOMEM;
>  			for (j = 0; j < count; j++) {
>  				const u8 *sad = &db->data[j * 3];
> 
> -				(*sads)[j].format = (sad[0] & 0x78) >> 3;
> -				(*sads)[j].channels = sad[0] & 0x7;
> -				(*sads)[j].freq = sad[1] & 0x7F;
> -				(*sads)[j].byte2 = sad[2];
> +				sads[j].format = (sad[0] & 0x78) >> 3;
> +				sads[j].channels = sad[0] & 0x7;
> +				sads[j].freq = sad[1] & 0x7F;
> +				sads[j].byte2 = sad[2];

Thanks for the code update. I noticed the use of magic values in this section, which can make the code less clear 
and harder to maintain. Would it be possible to define constants or use descriptive names instead of these magic 
values?

Regards,
Mitul
>  			}
>  			break;
>  		}
> --
> 2.39.2