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

[Marcus Folkesson <marcus.folkesson@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3274 bytes --]

Hello Javier,

On Wed, Apr 23, 2025 at 01:08:11AM +0200, Javier Martinez Canillas wrote:
> Marcus Folkesson <marcus.folkesson@gmail.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@suse.de>
> > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.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@redhat.com>
> 
> -- 
> Best regards,
> 
> Javier Martinez Canillas
> Core Platforms
> Red Hat
> 

Best regards,
Marcus Folkesson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]