linux-kernel - Re: [PATCH v4 2/3] drm: Add driver for Sitronix ST7920 LCD displays

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]

Message-ID: <50a4b4a9-afd4-42a3-9740-5e48de312f1d@kernel.org>
Date: Mon, 17 Nov 2025 11:57:29 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Iker Pedrosa <ikerpedrosam@...il.com>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>,
 Javier Martinez Canillas <javierm@...hat.com>
Cc: linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
 devicetree@...r.kernel.org
Subject: Re: [PATCH v4 2/3] drm: Add driver for Sitronix ST7920 LCD displays

On 31/10/2025 10:05, Iker Pedrosa wrote:
> +
> +static void st7920_hw_reset(struct st7920_device *st7920)
> +{
> +	if (!st7920->reset_gpio)
> +		return;
> +
> +	gpiod_set_value_cansleep(st7920->reset_gpio, 0);
> +	usleep_range(15, 20);
> +	gpiod_set_value_cansleep(st7920->reset_gpio, 1);

This is odd. Why are you keeping device in reset state?

I don't think you tested your driver with DTS.

> +	msleep(40);
> +}
> +

> +
> +	regmap = devm_regmap_init_spi(spi, &st7920_spi_regmap_config);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	st7920 = devm_drm_dev_alloc(dev, &st7920_drm_driver,
> +				    struct st7920_device, drm);
> +	if (IS_ERR(st7920))
> +		return PTR_ERR(st7920);
> +
> +	drm = &st7920->drm;
> +
> +	st7920->dev = dev;
> +	st7920->regmap = regmap;
> +	st7920->spi = spi;
> +	st7920->device_info = device_get_match_data(dev);
> +	st7920->width = st7920->device_info->default_width;
> +	st7920->height = st7920->device_info->default_height;
> +
> +	st7920->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);

So you keep device in reset state, then why do you toggle it in
st7920_hw_reset()? Remember that this is the logical, not line level.

> +	if (IS_ERR(st7920->reset_gpio)) {
> +		ret = PTR_ERR(st7920->reset_gpio);
> +		dev_err(dev, "Unable to retrieve reset GPIO: %d\n", ret);

return dev_err_probe, didn't you get comment on that?

> +		return ret;
> +	}
> +
> +	spi_set_drvdata(spi, st7920);
> +
> +	ret = st7920_init_modeset(st7920);
> +	if (ret)
> +		return ret;
> +
> +	ret = drm_dev_register(drm, 0);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "DRM device register failed\n");
> +
> +	drm_client_setup(drm, NULL);
> +

Best regards,
Krzysztof