linux-kernel - Re: [v3 2/4] drm/panel: support for BOE tv101wum-nl6 wuxga dsi video mode panel

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190626072021.GA14541@ravnborg.org>
Date:   Wed, 26 Jun 2019 09:20:21 +0200
From:   Sam Ravnborg <sam@...nborg.org>
To:     Jitao Shi <jitao.shi@...iatek.com>
Cc:     Rob Herring <robh+dt@...nel.org>, Pawel Moll <pawel.moll@....com>,
        Mark Rutland <mark.rutland@....com>,
        Ian Campbell <ijc+devicetree@...lion.org.uk>,
        linux-pwm@...r.kernel.org, David Airlie <airlied@...ux.ie>,
        Matthias Brugger <matthias.bgg@...il.com>, stonea168@....com,
        dri-devel@...ts.freedesktop.org,
        Andy Yan <andy.yan@...k-chips.com>,
        Ajay Kumar <ajaykumar.rs@...sung.com>,
        Vincent Palatin <vpalatin@...omium.org>,
        cawa.cheng@...iatek.com,
        Russell King <rmk+kernel@....linux.org.uk>,
        Thierry Reding <treding@...dia.com>,
        devicetree@...r.kernel.org, linux-mediatek@...ts.infradead.org,
        yingjoe.chen@...iatek.com, eddie.huang@...iatek.com,
        linux-arm-kernel@...ts.infradead.org,
        Rahul Sharma <rahul.sharma@...sung.com>,
        srv_heupstream@...iatek.com, linux-kernel@...r.kernel.org,
        Sascha Hauer <kernel@...gutronix.de>,
        Sean Paul <seanpaul@...omium.org>
Subject: Re: [v3 2/4] drm/panel: support for BOE tv101wum-nl6 wuxga dsi video
 mode panel

Hi Jitao.

Driver looks good, one detail.

> +
> +struct panel_desc {
> +	const struct drm_display_mode *modes;
> +	unsigned int bpc;
> +
> +	/**
> +	 * @width: width (in millimeters) of the panel's active display area
> +	 * @height: height (in millimeters) of the panel's active display area
> +	 */
> +	struct {
> +		unsigned int width;
> +		unsigned int height;
> +	} size;
Maybe name these width_mm and height_mm.
Then they have the same name as where they are copied to,
and it is explicit documented that it is in mm.

The extra indirection with a struct is not needed in display_mode,
maybe drop it here too?
> +
> +	unsigned long mode_flags;
> +	enum mipi_dsi_pixel_format format;
> +	const struct panel_init_cmd *init_cmds;
> +	unsigned int lanes;
> +};
> +
...
> +static int boe_panel_unprepare(struct drm_panel *panel)
> +{
> +	struct boe_panel *boe = to_boe_panel(panel);
> +	int ret;
> +
> +	if (!boe->prepared)
> +		return 0;
> +
> +	ret = boe_panel_off(boe);
> +	if (ret < 0) {
> +		dev_err(panel->dev, "failed to set panel off: %d\n", ret);
> +		return ret;
> +	}
> +
> +	msleep(150);
> +	if (boe->enable_gpio)
> +		gpiod_set_value(boe->enable_gpio, 0);
Everywhere boe->enable_gpio is used it is checked like above.
Bot boe->enable_gpio in a mandatory property so it must be present.
The driver error out in probe if not present, so this check seems
redundandt?

Everything else looks really good.

With the above fixed / considered:
Reviewed-by: Sam Ravnborg <sam@...nborg.org>

	Sam