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

[Golani, Mitulkumar Ajitkumar]

> -----Original Message-----
> From: Nikula, Jani <jani.nikula at intel.com>
> Sent: 26 September 2023 13:14
> To: Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani at intel.com>;
> dri-devel at lists.freedesktop.org
> Cc: intel-gfx at lists.freedesktop.org
> Subject: RE: [PATCH 4/6] drm/edid: use a temp variable for sads to drop one
> level of dereferences
> 
> On Mon, 25 Sep 2023, "Golani, Mitulkumar Ajitkumar"
> <mitulkumar.ajitkumar.golani at intel.com> wrote:
> > 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?
> 
> Yes, but that would be for a separate patch. The magic values aren't added
> here.
 
Sure. Other changes looks good to me.
Reviewed-by: Mitul Golani <mitulkumar.ajitkumar.golani at intel.com>
> 
> BR,
> Jani.
> 
> >
> > Regards,
> > Mitul
> >>                       }
> >>                       break;
> >>               }
> >> --
> >> 2.39.2
> >
> 
> --
> Jani Nikula, Intel