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

[Javier Martinez Canillas]

Marcus Folkesson <marcus.folkesson at 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 at suse.de>
> Signed-off-by: Marcus Folkesson <marcus.folkesson at 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)))

[...]

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 at redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat