linux-kernel - Re: [PATCH 1/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] [thread-next>] [day] [month] [year] [list]

Message-ID: <285eee30-58ab-4837-9fc4-ff5cd5118037@kernel.org>
Date: Thu, 7 Aug 2025 10:12:19 +0200
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 1/3] drm: Add driver for Sitronix ST7920 LCD displays

On 06/08/2025 14:48, Iker Pedrosa wrote:
> This adds a functional DRM driver for ST7920 that communicates with the
> display via the SPI bus.

Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

> 
> Signed-off-by: Iker Pedrosa <ikerpedrosam@...il.com>
> ---


...


> +#define BOTTOM_HORIZONTAL_ADDRESS	0x80
> +
> +#define CMD_SIZE			35
> +
> +const struct st7920_deviceinfo st7920_variants[] = {
> +	[ST7920_ID] = {
> +		.default_width = 128,
> +		.default_height = 64,
> +		.family_id = ST7920_FAMILY,

Don't add dead code. This cannot be anything else than ST7920_FAMILY.

Several places here can be simplified (and "possible" future code is not
an argument here - this patch must be correct, simple and stand on its
own because we do not write code just in case).


...

> +static int st7920_probe(struct spi_device *spi)
> +{
> +	struct st7920_device *st7920;
> +	struct regmap *regmap;
> +	struct device *dev = &spi->dev;
> +	struct drm_device *drm;
> +	int ret;
> +
> +	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 dev_err_probe(dev, PTR_ERR(st7920),
> +							 "Failed to allocate DRM device\n");

Misaligned but also this looks like ENOMEM error and such should never
have dev_err.



Best regards,
Krzysztof