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

[Javier Martinez Canillas]

Marcus Folkesson <marcus.folkesson@xxxxxxxxx> writes:

> Hello Javier,
>
> Thank you for your review and suggestions.
>
> On Tue, Apr 08, 2025 at 12:44:46PM +0200, Javier Martinez Canillas wrote:
>> Marcus Folkesson <marcus.folkesson@xxxxxxxxx> 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.
>> >
>> 
>> I would structure the driver differently. For example, what was done
>> for the Solomon SSD130X display controllers, that also support these
>> three interfaces:
>> 
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/solomon
>> 
>> Basically, it was split in a ssd130x.c module that's agnostic of the
>> transport interface and implements all the core logic for the driver.
>> 
>> And a set of different modules that have the interface specific bits:
>> ssd130x-i2c.c and ssd130x-spi.c.
>> 
>> That way, adding for example SPI support to your driver would be quite
>> trivial and won't require any refactoring. Specially since you already
>> are using regmap, which abstracts away the I2C interface bits.
>
> Yes, I had in mind to start looking into this after the initial version.
> The driver is writtin in this in mind, everything that is common for all
> interfaces is easy to move out.
>

Yes, I noticed that and the reason why I mentioned the file layout used in
the ssd130x driver. If was meant to only be an I2C driver then I think it
would be a good candidate for the tiny sub-dir (that is for small drivers
that can be implemented in a single file).

But as said, it's OK for me too if you start in tiny and then refactor it
to be moved to a sitronix vendor sub-dir.

[...]

>> > +static int st7571_set_pixel_format(struct st7571_device *st7571,
>> > +				   u32 pixel_format)
>> > +{
>> > +	switch (pixel_format) {
>> > +	case DRM_FORMAT_C1:
>> > +		return st7571_set_color_mode(st7571, ST7571_COLOR_MODE_BLACKWHITE);
>> > +	case DRM_FORMAT_C2:
>> > +		return st7571_set_color_mode(st7571, ST7571_COLOR_MODE_GRAY);
>> > +	default:
>> > +		return -EINVAL;
>> > +	}
>> 
>> These should be DRM_FORMAT_R1 and DRM_FORMAT_R2 and not C{1,2}. The former
>> is for displays have a single color (i.e: grey) while the latter is when a
>> pixel can have different color, whose values are defined by a CLUT table.
>> 
>
> I see.
> Does fbdev only works with CLUT formats? I get this error when I switch
> to DRM_FORMAT_R{1,2}:
>
> [drm] Initialized st7571 1.0.0 for 0-003f on minor 0
> st7571 0-003f: [drm] format C1   little-endian (0x20203143) not supported
> st7571 0-003f: [drm] No compatible format found
> st7571 0-003f: [drm] *ERROR* fbdev: Failed to setup emulation (ret=-22)
>
>

That's a god question, I don't really know...

But fbdev does support XRGB8888, which may be another good reason to add
it and make it the default format. Yes, it will cause an unnecessary pixel
format conversion but the I2C transport is so slow anyways that compute is
not the bottleneck when using these small displays.

>> ...
>> 
>> > +
>> > +static const uint32_t st7571_primary_plane_formats[] = {
>> > +	DRM_FORMAT_C1,
>> > +	DRM_FORMAT_C2,
>> > +};
>> > +
>> 
>> I would add a DRM_FORMAT_XRGB8888 format. This will allow your display to
>> be compatible with any user-space. Your st7571_fb_blit_rect() can then do
>> a pixel format conversion from XRGB8888 to the native pixel format.
>
> This were discussed in v2, but there were limitations in the helper
> functions that we currently have.
>

Indeed, will need a drm_fb_xrgb8888_to_gray2() for R2. There is already a
drm_fb_xrgb8888_to_mono() as mentioned that you can use for R1. 

> I will look into how this could be implemented in a generic way, but maybe that is
> something for a follow up patch?
>

Yes, it could be a follow-up patch. It just helps to have XRGB8888 support for
compatibility reasons (the fbdev issue you found is another example of this).

[...]

>> > +
>> > +static void st7571_remove(struct i2c_client *client)
>> > +{
>> > +	struct st7571_device *st7571 = i2c_get_clientdata(client);
>> > +
>> > +	drm_dev_unplug(&st7571->dev);
>> 
>> I think you are missing a drm_atomic_helper_shutdown() here.
>
> This is a change for v3. As the device has been unplugged already, it
> won't do anything, so I removed it.
>
> Isn't it right to do so?
>
>

It seems I was wrong on this and your implementation is correct. I talked
with Thomas yesterday and he clarified it to me.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat