| Message ID | 6692fbce07fbc03ad8785e6e6fe81fad4354e657.1694078430.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/edid: split out drm_eld.[ch], add some SAD helpers | expand |
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? Regards, Mitul > } > break; > } > -- > 2.39.2
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 >
> -----Original Message----- > From: Nikula, Jani <jani.nikula@intel.com> > Sent: 26 September 2023 13:14 > To: Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@intel.com>; > dri-devel@lists.freedesktop.org > Cc: intel-gfx@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@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. Sure. Other changes looks good to me. Reviewed-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com> > > BR, > Jani. > > > > > Regards, > > Mitul > >> } > >> break; > >> } > >> -- > >> 2.39.2 > > > > -- > Jani Nikula, Intel
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]; } break; }
It's arguably easier on the eyes, and drops a set of parenthesis too. 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(-)