Messages in this thread | | | Date | Mon, 8 Jul 2019 19:50:08 +0200 | From | Sam Ravnborg <> | Subject | Re: [PATCH v5 2/7] drm/panel: simple: Add ability to override typical timing |
| |
Hi Dough.
On Mon, Jul 01, 2019 at 09:39:24AM -0700, Doug Anderson wrote: > Hi, > > On Sun, Jun 30, 2019 at 1:22 PM Sam Ravnborg <sam@ravnborg.org> wrote: > > > > > @@ -91,6 +92,8 @@ struct panel_simple { > > > struct i2c_adapter *ddc; > > > > > > struct gpio_desc *enable_gpio; > > > + > > > + struct drm_display_mode override_mode; > > I fail to see where this poiter is assigned. > > In panel_simple_parse_override_mode(). Specifically: > > drm_display_mode_from_videomode(&vm, &panel->override_mode);
The above code-snippet is only called in the panel has specified display timings using display_timings - it is not called when display_mode is used. So override_mode is only assigned in some cases and not all cases. This needs to be fixed so we do not reference override_mode unless it is set.
> > > > @@ -152,6 +162,44 @@ static int panel_simple_get_fixed_modes(struct panel_simple *panel) > > > num++; > > > } > > > > > > + return num; > > > +} > > > + > > > +static int panel_simple_get_non_edid_modes(struct panel_simple *panel) > > > +{ > > > + struct drm_connector *connector = panel->base.connector; > > > + struct drm_device *drm = panel->base.drm; > > > + struct drm_display_mode *mode; > > > + bool has_override = panel->override_mode.type; > > This looks suspicious. > > panel->override_mode.type is an unsigned int that may have a number of > > bits set. > > So the above code implicitly convert a .type != 0 to a true. > > This can be expressed in a much more reader friendly way. > > You would suggest that I add a boolean field to a structure to > indicate whether an override mode is present? A simple bool has_override = panel->override_mode.type != 0; would do the trick here. Then there is no hidden conversion from int to a bool.
But as override_mode can be NULL something more needs to be done.
Sam
| |