Hi Jani,
added comments in-line.
> -----Original Message-----
> From: Nikula, Jani <jani.nikula@xxxxxxxxx>
> Sent: Thursday, September 7, 2023 2:58 PM
> To: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Nikula, Jani <jani.nikula@xxxxxxxxx>;
> Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@xxxxxxxxx>
> 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@xxxxxxxxx>
> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> ---
> 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
