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] [day] [month] [year] [list]

Message-ID: <aAh9ckXlJcXyB3BA@gmail.com>
Date: Wed, 23 Apr 2025 07:41:06 +0200
From: Marcus Folkesson <marcus.folkesson@...il.com>
To: Javier Martinez Canillas <javierm@...hat.com>
Cc: 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>, dri-devel@...ts.freedesktop.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	Thomas Zimmermann <tzimmrmann@...e.de>
Subject: Re: [PATCH v4 2/3] drm/st7571-i2c: add support for Sitronix ST7571
 LCD controller

Hello Javier,

On Wed, Apr 23, 2025 at 01:08:11AM +0200, Javier Martinez Canillas wrote:
> 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)))

That looks better, I will change to use those macros.
> 
> [...]
> 
> 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:

Sure

> 
> /* 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 the display is 4bit grayscale it still expect each pixel to be
written as two bits. The mapping is as follows:
0 0 => White
0 1 => Light gray
1 0 => Dark gray
1 1 => Black

So here I write the data twice to get a black&white image for monochrome
formats.
This is not tested though, I only have a monochrome display to test
with.

I will extend the comment to explain this.

> 
> > +			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!

Thanks!

> 
> Reviewed-by: Javier Martinez Canillas <javierm@...hat.com>
> 
> -- 
> Best regards,
> 
> Javier Martinez Canillas
> Core Platforms
> Red Hat
> 

Best regards,
Marcus Folkesson

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)