linux-kernel - Re: [PATCH v8 2/2] drm/panel: Add Sitronix ST7701 panel driver

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] [day] [month] [year] [list]

Date:   Thu, 24 Jan 2019 20:39:25 +0100
From:   Sam Ravnborg <sam@...nborg.org>
To:     Jagan Teki <jagan@...rulasolutions.com>
Cc:     Thierry Reding <thierry.reding@...il.com>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Michael Trimarchi <michael@...rulasolutions.com>,
        linux-amarula@...rulasolutions.com, linux-kernel@...r.kernel.org,
        dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v8 2/2] drm/panel: Add Sitronix ST7701 panel driver

Hi Jagan.

Thanks for being persistent and keep follow-up on this driver.

On Thu, Jan 24, 2019 at 11:58:44PM +0530, Jagan Teki wrote:
> ST7701 designed for small and medium sizes of TFT LCD display, is
> capable of supporting up to 480RGBX864 in resolution. It provides
> several system interfaces like MIPI/RGB/SPI.
> 
> Currently added support for Techstar TS8550B which is ST7701 based
> 480x854, 2-lane MIPI DSI LCD panel.
> 
> Driver now registering mipi_dsi device, but indeed it can extendable
> for RGB if any requirement trigger in future.
> 
> Signed-off-by: Jagan Teki <jagan@...rulasolutions.com>
> ---

> diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7701.c b/drivers/gpu/drm/panel/panel-sitronix-st7701.c
> new file mode 100644
> index 000000000000..499dd8a34a6d
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7701.c
> @@ -0,0 +1,429 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2019, Amarula Solutions.
> + * Author: Jagan Teki <jagan@...rulasolutions.com>
> + */
> +
> +#include <linux/backlight.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <drm/drmP.h>
For new drivers please do not use drmP.h.
We are slowly going away from this and in drm-misc.git
the drmP.h file is no longer needed for anything.


> +	st7701->backlight = devm_of_find_backlight(&dsi->dev);
> +	if (IS_ERR(st7701->backlight))
> +		return PTR_ERR(st7701->backlight);

> +static int st7701_dsi_remove(struct mipi_dsi_device *dsi)
> +{
> +	struct st7701 *st7701 = mipi_dsi_get_drvdata(dsi);
> +
> +	mipi_dsi_detach(dsi);
> +	drm_panel_remove(&st7701->panel);
> +
> +	if (st7701->backlight)
> +		put_device(&st7701->backlight->dev);
> +
> +	return 0;
> +}

Please address this comment from Noralf:
"This happens automatically on driver detach if devm_of_find_backlight()
is used"

In other words, no need to do anything with backlight in st7701_dsi_remove()

With these details fixed you can add back my:
Reviewed-by: Sam Ravnborg <sam@...nborg.org>