linux-kernel - Re: [PATCH v4 2/3] drm/st7571-i2c: add support for Sitronix ST7571 LCD controller

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: <871ptjbxr8.fsf@minerva.mail-host-address-is-not-set>
Date: Wed, 23 Apr 2025 01:08:11 +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 v4 2/3] drm/st7571-i2c: add support for Sitronix ST7571
 LCD controller

Marcus Folkesson <marcus.folkesson@...il.com> writes:

Hello Marcus,

> 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>
> Signed-off-by: Marcus Folkesson <marcus.folkesson@...il.com>
> ---

[...]

> +#define ST7571_SET_COLUMN_LSB(c)		(0x00 | ((c) & 0xf))
> +#define ST7571_SET_COLUMN_MSB(c)		(0x10 | ((c) >> 4))
> +#define ST7571_SET_COM0_LSB(x)			((x) & 0x7f)
> +#define ST7571_SET_COM0_MSB			(0x44)
> +#define ST7571_SET_COM_SCAN_DIR(d)		(0xc0 | (((d) << 3) & 0x8))

You could also use the GENMASK() and FIELD_PREP() macros for these, e.g:

#define ST7571_SET_COLUMN_LSB(c)		(0x00 | FIELD_PREP(GENMASK(3, 0), (c)))
#define ST7571_SET_COLUMN_MSB(c)		(0x10 | FIELD_PREP(GENMASK(3, 0), (c) >> 4))
#define ST7571_SET_COM0_LSB(x)			(FIELD_PREP(GENMASK(6, 0), (x)))
#define ST7571_SET_COM0_MSB			(0x44)
#define ST7571_SET_COM_SCAN_DIR(d)		(0xc0 | FIELD_PREP(GENMASK(3, 3), (d)))

[...]

Maybe a comment here that this function only exists due regmap expecting
both a .write and .read handler even for devices that are write only, e.g:

/* The st7571 driver does not read registers but regmap expects a .read */
> +static int st7571_regmap_read(void *context, const void *reg_buf,
> +			       size_t reg_size, void *val_buf, size_t val_size)
> +{
> +	return -EOPNOTSUPP;
> +}
> +

[...]

> +static int st7571_fb_update_rect_grayscale(struct drm_framebuffer *fb, struct drm_rect *rect)
> +{

[...]

> +	for (int y = rect->y1; y < rect->y2; y += ST7571_PAGE_HEIGHT) {
> +		for (int x = x1; x < x2; x++)
> +			row[x] = st7571_transform_xy(st7571->hwbuf, x, y);
> +
> +		st7571_set_position(st7571, rect->x1, y);
> +
> +		/* TODO: Investige why we can't write multiple bytes at once */
> +		for (int x = x1; x < x2; x++) {
> +			regmap_bulk_write(st7571->regmap, ST7571_DATA_MODE, row + x, 1);
> +
> +			/* Write monochrome formats twice */

Why this is needed ?

> +			if (format == DRM_FORMAT_R1 || format == DRM_FORMAT_XRGB8888)
> +				regmap_bulk_write(st7571->regmap, ST7571_DATA_MODE, row + x, 1);
> +		}
> +	}
> +
> +	return 0;
> +}
> +

The driver looks great to me now, thanks a lot for taking my comments into account!

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

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat