From: Sam Ravnborg <sam@ravnborg.org> To: Doug Anderson <dianders@chromium.org> Cc: "Thierry Reding" <thierry.reding@gmail.com>, "Heiko Stuebner" <heiko@sntech.de>, "Sean Paul" <seanpaul@chromium.org>, devicetree@vger.kernel.org, "Rob Herring" <robh+dt@kernel.org>, "David Airlie" <airlied@linux.ie>, "Jeffy Chen" <jeffy.chen@rock-chips.com>, dri-devel <dri-devel@lists.freedesktop.org>, LKML <linux-kernel@vger.kernel.org>, "open list:ARM/Rockchip SoC..." <linux-rockchip@lists.infradead.org>, "Boris Brezillon" <boris.brezillon@collabora.com>, "Laurent Pinchart" <laurent.pinchart@ideasonboard.com>, "Enric Balletbò" <enric.balletbo@collabora.com>, "Stéphane Marchesin" <marcheu@chromium.org>, "Ezequiel Garcia" <ezequiel@collabora.com>, "Matthias Kaehlcke" <mka@chromium.org> Subject: Re: [PATCH v5 2/7] drm/panel: simple: Add ability to override typical timing Date: Mon, 8 Jul 2019 19:50:08 +0200 [thread overview] Message-ID: <20190708175007.GA3511@ravnborg.org> (raw) In-Reply-To: <CAD=FV=V_wTD1xpkXRe-z2HsZ8QXKq7jmq8CsfhMnFxi-5XDJjw@mail.gmail.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Sam Ravnborg <sam@ravnborg.org> To: Doug Anderson <dianders@chromium.org> Cc: devicetree@vger.kernel.org, "David Airlie" <airlied@linux.ie>, "Jeffy Chen" <jeffy.chen@rock-chips.com>, LKML <linux-kernel@vger.kernel.org>, "Rob Herring" <robh+dt@kernel.org>, "open list:ARM/Rockchip SoC..." <linux-rockchip@lists.infradead.org>, "Thierry Reding" <thierry.reding@gmail.com>, "Sean Paul" <seanpaul@chromium.org>, dri-devel <dri-devel@lists.freedesktop.org>, "Boris Brezillon" <boris.brezillon@collabora.com>, "Enric Balletbò" <enric.balletbo@collabora.com>, "Stéphane Marchesin" <marcheu@chromium.org>, "Ezequiel Garcia" <ezequiel@collabora.com>, "Matthias Kaehlcke" <mka@chromium.org>, "Laurent Pinchart" <laurent.pinchart@ideasonboard.com> Subject: Re: [PATCH v5 2/7] drm/panel: simple: Add ability to override typical timing Date: Mon, 8 Jul 2019 19:50:08 +0200 [thread overview] Message-ID: <20190708175007.GA3511@ravnborg.org> (raw) In-Reply-To: <CAD=FV=V_wTD1xpkXRe-z2HsZ8QXKq7jmq8CsfhMnFxi-5XDJjw@mail.gmail.com> 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2019-07-08 17:50 UTC|newest] Thread overview: 102+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-04-01 17:17 [PATCH v5 0/7] drm/panel: simple: Add mode support to devicetree Douglas Anderson 2019-04-01 17:17 ` Douglas Anderson 2019-04-01 17:17 ` Douglas Anderson 2019-04-01 17:17 ` [PATCH v5 1/7] dt-bindings: Add panel-timing subnode to simple-panel Douglas Anderson 2019-04-01 17:17 ` Douglas Anderson 2019-04-06 6:06 ` Rob Herring 2019-04-06 6:06 ` Rob Herring 2019-04-08 9:10 ` Boris Brezillon 2019-04-08 10:32 ` Thierry Reding 2019-04-08 14:39 ` Doug Anderson 2019-04-08 14:39 ` Doug Anderson 2019-05-20 18:35 ` Doug Anderson 2019-06-28 23:47 ` Thierry Reding 2019-06-28 23:47 ` Thierry Reding 2019-06-30 20:02 ` Sam Ravnborg 2019-06-30 20:02 ` Sam Ravnborg 2019-07-01 16:59 ` Doug Anderson 2019-07-01 16:59 ` Doug Anderson 2019-04-01 17:17 ` [PATCH v5 2/7] drm/panel: simple: Add ability to override typical timing Douglas Anderson 2019-04-01 17:17 ` Douglas Anderson 2019-04-08 9:10 ` Boris Brezillon 2019-04-08 9:10 ` Boris Brezillon 2019-06-28 23:49 ` Thierry Reding 2019-06-28 23:49 ` Thierry Reding 2019-06-30 20:22 ` Sam Ravnborg 2019-06-30 20:22 ` Sam Ravnborg 2019-06-30 20:55 ` Sam Ravnborg 2019-06-30 20:55 ` Sam Ravnborg 2019-07-01 16:39 ` Doug Anderson 2019-07-08 17:56 ` Sam Ravnborg 2019-07-10 22:56 ` Doug Anderson 2019-07-11 19:16 ` Sean Paul 2019-07-11 19:16 ` Sean Paul 2019-07-01 16:39 ` Doug Anderson 2019-07-08 17:50 ` Sam Ravnborg [this message] 2019-07-08 17:50 ` Sam Ravnborg 2019-07-10 22:39 ` Doug Anderson 2019-07-10 22:39 ` Doug Anderson 2019-07-11 19:38 ` Sam Ravnborg 2019-04-01 17:17 ` [PATCH v5 3/7] arm64: dts: rockchip: Specify override mode for kevin panel Douglas Anderson 2019-04-01 17:17 ` Douglas Anderson 2019-04-01 17:17 ` Douglas Anderson 2019-07-11 21:30 ` Heiko Stübner 2019-07-11 21:30 ` Heiko Stübner 2019-07-11 21:30 ` Heiko Stübner 2019-04-01 17:17 ` [PATCH v5 4/7] drm/panel: simple: Use display_timing for Innolux n116bge Douglas Anderson 2019-04-01 17:17 ` Douglas Anderson 2019-06-28 23:50 ` Thierry Reding 2019-04-01 17:17 ` [PATCH v5 5/7] drm/panel: simple: Use display_timing for AUO b101ean01 Douglas Anderson 2019-04-01 17:17 ` Douglas Anderson 2019-06-28 23:50 ` Thierry Reding 2019-04-01 17:17 ` [PATCH v5 6/7] ARM: dts: rockchip: Specify rk3288-veyron-chromebook's display timings Douglas Anderson 2019-04-01 17:17 ` Douglas Anderson 2019-04-01 17:17 ` Douglas Anderson 2019-04-07 1:15 ` Urja Rannikko 2019-04-07 1:15 ` Urja Rannikko 2019-04-08 15:21 ` Doug Anderson 2019-04-08 15:21 ` Doug Anderson 2019-04-08 16:26 ` Urja Rannikko 2019-04-08 16:26 ` Urja Rannikko 2019-04-13 0:07 ` Doug Anderson 2019-04-13 0:07 ` Doug Anderson 2019-04-13 0:07 ` Doug Anderson 2019-07-11 21:27 ` Heiko Stübner 2019-07-11 21:27 ` Heiko Stübner 2019-07-11 21:27 ` Heiko Stübner 2019-07-11 21:52 ` Heiko Stübner 2019-07-11 21:52 ` Heiko Stübner 2019-07-11 21:52 ` Heiko Stübner 2019-04-01 17:17 ` [PATCH v5 7/7] ARM: dts: rockchip: Specify rk3288-veyron-minnie's " Douglas Anderson 2019-04-01 17:17 ` Douglas Anderson 2019-04-01 17:17 ` Douglas Anderson 2019-07-11 21:28 ` Heiko Stübner 2019-07-11 21:28 ` Heiko Stübner 2019-07-11 21:28 ` Heiko Stübner 2019-06-14 10:39 ` [PATCH v5 0/7] drm/panel: simple: Add mode support to devicetree Heiko Stuebner 2019-06-14 10:39 ` Heiko Stuebner 2019-06-14 10:39 ` Heiko Stuebner 2019-06-26 13:00 ` Sam Ravnborg 2019-06-26 13:00 ` Sam Ravnborg 2019-06-26 13:00 ` Sam Ravnborg 2019-06-26 14:41 ` Doug Anderson 2019-06-26 14:41 ` Doug Anderson 2019-06-26 14:41 ` Doug Anderson 2019-06-28 15:55 ` Doug Anderson 2019-06-28 15:55 ` Doug Anderson 2019-06-28 15:55 ` Doug Anderson 2019-06-28 16:10 ` Rob Herring 2019-06-28 16:10 ` Rob Herring 2019-06-28 16:10 ` Rob Herring 2019-06-28 17:13 ` Sam Ravnborg 2019-06-28 17:13 ` Sam Ravnborg 2019-06-28 17:13 ` Sam Ravnborg 2019-06-29 14:09 ` Heiko Stübner 2019-06-29 14:09 ` Heiko Stübner 2019-06-29 14:09 ` Heiko Stübner 2019-07-08 15:58 ` Doug Anderson 2019-07-08 15:58 ` Doug Anderson 2019-07-08 15:58 ` Doug Anderson 2019-07-11 19:35 ` Sam Ravnborg 2019-07-11 19:35 ` Sam Ravnborg 2019-07-11 19:35 ` Sam Ravnborg
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190708175007.GA3511@ravnborg.org \ --to=sam@ravnborg.org \ --cc=airlied@linux.ie \ --cc=boris.brezillon@collabora.com \ --cc=devicetree@vger.kernel.org \ --cc=dianders@chromium.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=enric.balletbo@collabora.com \ --cc=ezequiel@collabora.com \ --cc=heiko@sntech.de \ --cc=jeffy.chen@rock-chips.com \ --cc=laurent.pinchart@ideasonboard.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-rockchip@lists.infradead.org \ --cc=marcheu@chromium.org \ --cc=mka@chromium.org \ --cc=robh+dt@kernel.org \ --cc=seanpaul@chromium.org \ --cc=thierry.reding@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.