Hello Javier, On Wed, Apr 23, 2025 at 01:08:11AM +0200, Javier Martinez Canillas wrote: > Marcus Folkesson <[email protected]> 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 <[email protected]> > > Signed-off-by: Marcus Folkesson <[email protected]> > > --- > > [...] > > > +#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 <[email protected]>
>
> --
> Best regards,
>
> Javier Martinez Canillas
> Core Platforms
> Red Hat
>
Best regards,
Marcus Folkesson
signature.asc
Description: PGP signature
