Marcus Folkesson <marcus.folkes...@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 <tzimmrm...@suse.de>
> Signed-off-by: Marcus Folkesson <marcus.folkes...@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 <javi...@redhat.com>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
