Sam, On Mon, Jul 8, 2019 at 10:50 AM Sam Ravnborg <sam@xxxxxxxxxxxx> wrote: > > 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@xxxxxxxxxxxx> 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. I'm afraid I'm not following you here. * override_mode is a structure that's directly part of "struct panel_simple". * The panel is allocated in panel_simple_probe() with devm_kzalloc(). * The "z" in kzalloc means that this memory will be zero-initialized. >From the points above, "override_mode" will always be set to something. If we didn't run "drm_display_mode_from_videomode(&vm, &panel->override_mode);" then we know the entire override_mode structure will be zero. While it took a while for me to get used to it, the kernel convention is to rely on zero-initialization and not to explicitly init things to zero. As an example of this being codified in the source, you can see that "checkpatch.pl" will yell at you for a similar thing: "do not initialise globals to 0". > > > @@ -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. I will change this to "panel->override_mode.type != 0" if you're really sure, but this seems both against the general Linux style feedback I've received over the years (though there is definitely not 100% consistency) and also against the local convention in this file. Examples in this file of treating ints as bools without an explicit "!= 0": * panel_simple_get_fixed_modes checks "if (panel->desc->bus_format)" * panel_simple_disable checks "if (p->desc->delay.disable)" * panel_simple_unprepare checks "if (p->desc->delay.unprepare)" * panel_simple_prepare checks "if (delay)" * panel_simple_enable checks "if (p->desc->delay.enable)" ...and, although slightly different, pointers in this file are checked for NULL vs. non-NULL without an explicit "== NULL". Of course just because all the other examples in the file do it one way doesn't mean that new code has to do it another way, but I wanted to be really sure you wanted me to go against the existing convention before changing this. > But as override_mode can be NULL something more needs to be done. I'm afraid I don't understand how override_mode can be NULL since it's not a pointer. Can you clarify? -Doug