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

[Jani Nikula <jani.nikula@intel.com>]

On Mon, 25 Sep 2023, "Golani, Mitulkumar Ajitkumar" <mitulkumar.ajitkumar.golani@intel.com> wrote:
> Hi Jani,
>
> added comments in-line.
>
>> -----Original Message-----
>> From: Nikula, Jani <jani.nikula@intel.com>
>> Sent: Thursday, September 7, 2023 2:58 PM
>> To: dri-devel@lists.freedesktop.org
>> Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>;
>> Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@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@intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@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.

BR,
Jani.

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

-- 
Jani Nikula, Intel