| Message ID | 20190902131546.4691-2-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/edid: Don't look for CEA data blocks in CEA ext block rev < 3 | expand |
Hi Ville, On Mon, 2 Sep 2019 16:15:46 +0300, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Let's make cea_db_offsets() a bit more convenient to use by > setting the start/end offsets to zero whenever the data block > collection isn't present. This makes it safe for the caller > to blindly iterate the data blocks even if there are none. > > Cc: Jean Delvare <jdelvare@suse.de> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/drm_edid.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 7b3072fc550b..e5905dc764c1 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -3690,6 +3690,9 @@ cea_revision(const u8 *cea) > static int > cea_db_offsets(const u8 *cea, int *start, int *end) > { > + *start = 0; > + *end = 0; > + > if (cea_revision(cea) < 3) > return -ENOTSUPP; > > @@ -4116,10 +4119,7 @@ static void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid) > if (cea_revision(cea) >= 3) { > int i, start, end; > > - if (cea_db_offsets(cea, &start, &end)) { > - start = 0; > - end = 0; > - } > + cea_db_offsets(cea, &start, &end); > > for_each_cea_db(cea, i, start, end) { > db = &cea[i]; Not sure if that's really needed. As it stands there's only one function which wants to continue after cea_db_offsets() fails, all others just bail out at that point. Now that cea_db_offsets() checks for revision >= 3, the construct above could simply become: int i, start, end; if (cea_db_offsets(cea, &start, &end) == 0) { for_each_cea_db(cea, i, start, end) { db = &cea[i]; which is IMHO more elegant and does not require zeroing start and end in cea_db_offsets().
On Tue, Sep 10, 2019 at 11:46:20AM +0200, Jean Delvare wrote: > Hi Ville, > > On Mon, 2 Sep 2019 16:15:46 +0300, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Let's make cea_db_offsets() a bit more convenient to use by > > setting the start/end offsets to zero whenever the data block > > collection isn't present. This makes it safe for the caller > > to blindly iterate the data blocks even if there are none. > > > > Cc: Jean Delvare <jdelvare@suse.de> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/drm_edid.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > > index 7b3072fc550b..e5905dc764c1 100644 > > --- a/drivers/gpu/drm/drm_edid.c > > +++ b/drivers/gpu/drm/drm_edid.c > > @@ -3690,6 +3690,9 @@ cea_revision(const u8 *cea) > > static int > > cea_db_offsets(const u8 *cea, int *start, int *end) > > { > > + *start = 0; > > + *end = 0; > > + > > if (cea_revision(cea) < 3) > > return -ENOTSUPP; > > > > @@ -4116,10 +4119,7 @@ static void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid) > > if (cea_revision(cea) >= 3) { > > int i, start, end; > > > > - if (cea_db_offsets(cea, &start, &end)) { > > - start = 0; > > - end = 0; > > - } > > + cea_db_offsets(cea, &start, &end); > > > > for_each_cea_db(cea, i, start, end) { > > db = &cea[i]; > > Not sure if that's really needed. As it stands there's only one > function which wants to continue after cea_db_offsets() fails, all > others just bail out at that point. Now that cea_db_offsets() checks > for revision >= 3, the construct above could simply become: > > int i, start, end; > > if (cea_db_offsets(cea, &start, &end) == 0) { > for_each_cea_db(cea, i, start, end) { > db = &cea[i]; > > which is IMHO more elegant and does not require zeroing start and end > in cea_db_offsets(). I dislike indentation. Also could perhaps even move the cea_db_offsets() into the for_each_cea_db() macro so that the caller doesn't have to care about these mundane details at all.
On Tue, 10 Sep 2019 12:48:42 +0300, Ville Syrjälä wrote: > On Tue, Sep 10, 2019 at 11:46:20AM +0200, Jean Delvare wrote: > > Hi Ville, > > > > On Mon, 2 Sep 2019 16:15:46 +0300, Ville Syrjala wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Let's make cea_db_offsets() a bit more convenient to use by > > > setting the start/end offsets to zero whenever the data block > > > collection isn't present. This makes it safe for the caller > > > to blindly iterate the data blocks even if there are none. > > > > > > Cc: Jean Delvare <jdelvare@suse.de> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/drm_edid.c | 8 ++++---- > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > > > index 7b3072fc550b..e5905dc764c1 100644 > > > --- a/drivers/gpu/drm/drm_edid.c > > > +++ b/drivers/gpu/drm/drm_edid.c > > > @@ -3690,6 +3690,9 @@ cea_revision(const u8 *cea) > > > static int > > > cea_db_offsets(const u8 *cea, int *start, int *end) > > > { > > > + *start = 0; > > > + *end = 0; > > > + > > > if (cea_revision(cea) < 3) > > > return -ENOTSUPP; > > > > > > @@ -4116,10 +4119,7 @@ static void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid) > > > if (cea_revision(cea) >= 3) { > > > int i, start, end; > > > > > > - if (cea_db_offsets(cea, &start, &end)) { > > > - start = 0; > > > - end = 0; > > > - } > > > + cea_db_offsets(cea, &start, &end); > > > > > > for_each_cea_db(cea, i, start, end) { > > > db = &cea[i]; > > > > Not sure if that's really needed. As it stands there's only one > > function which wants to continue after cea_db_offsets() fails, all > > others just bail out at that point. Now that cea_db_offsets() checks > > for revision >= 3, the construct above could simply become: > > > > int i, start, end; > > > > if (cea_db_offsets(cea, &start, &end) == 0) { > > for_each_cea_db(cea, i, start, end) { > > db = &cea[i]; > > > > which is IMHO more elegant and does not require zeroing start and end > > in cea_db_offsets(). > > I dislike indentation. It would be the same as currently, so it's not that bad. But well I'm not maintaining that piece of code so it's not my call anyway. > Also could perhaps even move the cea_db_offsets() > into the for_each_cea_db() macro so that the caller doesn't have to care > about these mundane details at all. If the macro stays readable, why not.
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 7b3072fc550b..e5905dc764c1 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -3690,6 +3690,9 @@ cea_revision(const u8 *cea) static int cea_db_offsets(const u8 *cea, int *start, int *end) { + *start = 0; + *end = 0; + if (cea_revision(cea) < 3) return -ENOTSUPP; @@ -4116,10 +4119,7 @@ static void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid) if (cea_revision(cea) >= 3) { int i, start, end; - if (cea_db_offsets(cea, &start, &end)) { - start = 0; - end = 0; - } + cea_db_offsets(cea, &start, &end); for_each_cea_db(cea, i, start, end) { db = &cea[i];