linux-kernel - Re: [PATCH v6 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: <87wm2oj6re.fsf@ocarina.mail-host-address-is-not-set>
Date: Mon, 15 Dec 2025 12:59:01 +0100
From: Javier Martinez Canillas <javierm@...hat.com>
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>, Iker
 Pedrosa <ikerpedrosam@...il.com>
Cc: linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
 devicetree@...r.kernel.org
Subject: Re: [PATCH v6 2/3] drm: Add driver for Sitronix ST7920 LCD displays

Iker Pedrosa <ikerpedrosam@...il.com> writes:

Hello Iker,

> Add a new DRM/KMS driver for displays using the Sitronix ST7920
> controller connected via the SPI bus. This provides a standard
> framebuffer interface for these common monochrome LCDs.
>
> Signed-off-by: Iker Pedrosa <ikerpedrosam@...il.com>
> ---

[...]

> diff --git a/drivers/gpu/drm/sitronix/Makefile b/drivers/gpu/drm/sitronix/Makefile
> index bd139e5a6995fa026cc635b3c29782473d1efad7..2f064a518121bfee3cca73acd42589e8c54cd4d7 100644
> --- a/drivers/gpu/drm/sitronix/Makefile
> +++ b/drivers/gpu/drm/sitronix/Makefile
> @@ -1,3 +1,4 @@
>  obj-$(CONFIG_DRM_ST7571_I2C)		+= st7571-i2c.o
>  obj-$(CONFIG_DRM_ST7586)		+= st7586.o
>  obj-$(CONFIG_DRM_ST7735R)		+= st7735r.o
> +obj-$(CONFIG_DRM_ST7920))		+= st7920.o

You have two closing parenthesis here.

> diff --git a/drivers/gpu/drm/sitronix/st7920.c b/drivers/gpu/drm/sitronix/st7920.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..c1e38beedcc660f6db96ec10cd87b2d4e62ee05e
> --- /dev/null
> +++ b/drivers/gpu/drm/sitronix/st7920.c
> @@ -0,0 +1,868 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * DRM driver for Sitronix ST7920 LCD displays
> + *
> + * Copyright 2025 Iker Pedrosa <ikerpedrosam@...il.com>
> + *
> + */
> +
> +#include <linux/bitrev.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/spi/spi.h>
> +
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_client_setup.h>

This header was moved to drivers/gpu/drm/clients/ by commit b86711c6d6e2
("drm/client: Move public client header to clients/ subdirectory").

So it should be instead (and moved above the drm headers includes):

#include <drm/clients/drm_client_setup.h>

> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_damage_helper.h>
> +#include <drm/drm_drv.h>
> +#include <drm/drm_fbdev_shmem.h>
> +#include <drm/drm_framebuffer.h>
> +#include <drm/drm_gem_atomic_helper.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_gem_shmem_helper.h>
> +#include <drm/drm_plane.h>

You need a #include <drm/drm_print.h> here too since you are using
helpers declared in that header file.

> +#include <drm/drm_probe_helper.h>
> +
> +#define DRIVER_NAME	"sitronix_st7920"
> +#define DRIVER_DESC	"DRM driver for Sitronix ST7920 LCD displays"
> +#define DRIVER_DATE	"20250723"

This isn't used anymore, the struct drm_driver doesn't have a .date field
anymore since commit cb2e1c2136f7 ("drm: remove driver date from struct
drm_driver and all drivers").

There are also some checkpatch warnings, please fix those. Remember to run
the checkpatch.pl script using the --strict option.

Other than these small comments, the driver looks good to me. Once you fix
them, feel free to add to your series:

Reviewed-by: Javier Martinez Canillas <javierm@...hat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat