| 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
| ||
|
Message-ID: <87v7quafou.fsf@minerva.mail-host-address-is-not-set>
Date: Wed, 23 Apr 2025 20:36:01 +0200
From: Javier Martinez Canillas <javierm@...hat.com>
To: Marcus Folkesson <marcus.folkesson@...il.com>, David Airlie
<airlied@...il.com>, Simona Vetter <simona@...ll.ch>, Maarten Lankhorst
<maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>
Cc: dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, Marcus Folkesson
<marcus.folkesson@...il.com>, Thomas Zimmermann <tzimmrmann@...e.de>
Subject: Re: [PATCH v5 2/3] drm/st7571-i2c: add support for Sitronix ST7571
LCD controller
Marcus Folkesson <marcus.folkesson@...il.com> writes:
Hello Marcus,
I tried to apply your patches to drm-misc and found some issues, so I will
have to ask you to do a final re-spin. Sorry about that...
> Sitronix ST7571 is a 4bit gray scale dot matrix LCD controller.
> The controller has a SPI, I2C and 8bit parallel interface, this
> driver is for the I2C interface only.
>
> Reviewed-by: Thomas Zimmermann <tzimmrmann@...e.de>
> Reviewed-by: Javier Martinez Canillas <javierm@...hat.com>
> Signed-off-by: Marcus Folkesson <marcus.folkesson@...il.com>
> ---
> drivers/gpu/drm/tiny/Kconfig | 11 +
> drivers/gpu/drm/tiny/Makefile | 1 +
> drivers/gpu/drm/tiny/st7571-i2c.c | 1007 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 1019 insertions(+)
>
> diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
> index 94cbdb1337c07f1628a33599a7130369b9d59d98..e4a55482e3bcd3f6851df1d322a14cbe1f96adfb 100644
> --- a/drivers/gpu/drm/tiny/Kconfig
> +++ b/drivers/gpu/drm/tiny/Kconfig
> @@ -232,6 +232,17 @@ config TINYDRM_ST7586
>
> If M is selected the module will be called st7586.
>
> +config DRM_ST7571_I2C
> + tristate "DRM support for Sitronix ST7571 display panels (I2C)"
> + depends on DRM && I2C && MMU
> + select DRM_GEM_SHMEM_HELPER
> + select DRM_KMS_HELPER
> + select REGMAP_I2C
> + help
> + DRM driver for Sitronix ST7571 panels controlled over I2C.
> +
> + if M is selected the module will be called st7571-i2c.
> +
checkpatch here complains about:
WARNING: please write a help paragraph that fully describes the config symbol with at least 4 lines
#144: FILE: drivers/gpu/drm/tiny/Kconfig:215:
+config DRM_ST7571_I2C
+ tristate "DRM support for Sitronix ST7571 display panels (I2C)"
+ depends on DRM && I2C && MMU
+ select DRM_GEM_SHMEM_HELPER
+ select DRM_KMS_HELPER
+ select REGMAP_I2C
+ help
+ DRM driver for Sitronix ST7571 panels controlled over I2C.
but honestly I think is just silly and your explanation is good enough so
you could ignore it if you want.
> config TINYDRM_ST7735R
> tristate "DRM support for Sitronix ST7715R/ST7735R display panels"
> depends on DRM && SPI
> diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile
> index 60816d2eb4ff93b87228ed8eadd60a0a33a1144b..eab7568c92c880cfdf7c2f0b9c4bfac4685dbe95 100644
> --- a/drivers/gpu/drm/tiny/Makefile
> +++ b/drivers/gpu/drm/tiny/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_DRM_GM12U320) += gm12u320.o
> obj-$(CONFIG_DRM_OFDRM) += ofdrm.o
> obj-$(CONFIG_DRM_PANEL_MIPI_DBI) += panel-mipi-dbi.o
> obj-$(CONFIG_DRM_SIMPLEDRM) += simpledrm.o
> +obj-$(CONFIG_DRM_ST7571_I2C) += st7571-i2c.o
this chunk doesn't apply on top of the drm-misc/drm-next branch [1] due
the simpledrm driver being moved out of the tiny directory. Please do a
rebase on top of that branch when posting a v6.
[1]: https://gitlab.freedesktop.org/drm/misc/kernel/-/tree/drm-misc-next
[...]
> +++ b/drivers/gpu/drm/tiny/st7571-i2c.c
Please run ./scripts/checkpatch.pl --strict -f -- drivers/gpu/drm/tiny/st7571-i2c.c
since checkpatch complains about some issues. Other than some style nits, it has some
good points IMO. In particular:
[...]
> +
> +static int st7571_fb_update_rect_grayscale(struct drm_framebuffer *fb, struct drm_rect *rect)
> +{
> + struct st7571_device *st7571 = drm_to_st7571(fb->dev);
> + uint32_t format = fb->format->format;
CHECK: Prefer kernel type 'u32' over 'uint32_t'
#523: FILE: drivers/gpu/drm/tiny/st7571-i2c.c:348:
+ uint32_t format = fb->format->format;
[...]
> +
> +static const uint64_t st7571_primary_plane_fmtmods[] = {
> + DRM_FORMAT_MOD_LINEAR,
> + DRM_FORMAT_MOD_INVALID
> +};
CHECK: Prefer kernel type 'u64' over 'uint64_t'
#611: FILE: drivers/gpu/drm/tiny/st7571-i2c.c:436:
+static const uint64_t st7571_primary_plane_fmtmods[] = {
[...]
> +static void st7571_reset(struct st7571_device *st7571)
> +{
> + gpiod_set_value_cansleep(st7571->reset, 1);
> + udelay(20);
CHECK: usleep_range is preferred over udelay; see function description of usleep_range() and udelay().
#993: FILE: drivers/gpu/drm/tiny/st7571-i2c.c:818:
+ udelay(20);
Specially since this is not in atomic context AFAIK. The Delay and sleep
mechanisms [2] doc has a good explanation about the different functions
provided by the Linux kernel for this.
Feel free to keep my R-B tag if you decide that some of the checkpatch
suggestions are applicable. The most important bit for me is the rebase,
to allow your patch to be cleanly applied on the drm-misc-next branch.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Powered by blists - more mailing lists