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

[Marcus Folkesson]

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.

> 
> > Reviewed-by: Thomas Zimmermann <tzimmrmann@xxxxxxx>
> > Signed-off-by: Marcus Folkesson <marcus.folkesson@xxxxxxxxx>
> > ---
> >  drivers/gpu/drm/tiny/Kconfig      |  11 +
> >  drivers/gpu/drm/tiny/Makefile     |   1 +
> >  drivers/gpu/drm/tiny/st7571-i2c.c | 721 ++++++++++++++++++++++++++++++++++++++
> 
> I personally think that the tiny sub-directory is slowly becoming a
> dumping ground for small drivers. Instead, maybe we should create a
> drivers/gpu/drm/sitronix/ sub-dir and put all Sitronix drivers there?
> 
> So far we have drivers in tiny for: ST7735R, ST7586 and ST7571 with
> your driver. And also have a few more Sitronix drivers in the panel
> sub-directory (although those likely should remain there).
> 
> I have a ST7565S and plan to write a driver for it. And I know someone
> who is working on a ST7920 driver. That would be 5 Sitronix drivers and
> the reason why I think that a dedicated sub-dir would be more organized.
> 
> Maybe there's even common code among these drivers and could be reused?
> 
> Just a thought though, it's OK to keep your driver as-is and we could do
> refactor / move drivers around as follow-up if agreed that is desirable.

That sounds like a good idea.

[...]

> 
> > +#define drm_to_st7571(_dev) container_of(_dev, struct st7571_device, dev)
> > +
> 
> I usually prefer these to be static inline functions instead of a
> macro. That way you get type checking and the end result should be
> basically the same.

I agree, I will change this to a static inline function.

> 
> > +struct st7571_device {
> > +	struct drm_device dev;
> > +
> > +	struct drm_plane primary_plane;
> > +	struct drm_crtc crtc;
> > +	struct drm_encoder encoder;
> > +	struct drm_connector connector;
> > +
> > +	struct drm_display_mode mode;
> > +
> > +	struct i2c_client *client;
> > +	struct regmap *regmap;
> > +	bool ignore_nak;
> > +
> 
> I know you mentioned that the chip sometimes nacks some I2C messages but
> maybe we want to better understand why that is the case before adding a
> flag like this?
> 
> In particular, I see in the "6.4 MICROPROCESSOR INTERFACE" section of the
> datasheet the following note:
> 
> "By connecting SDA_OUT to SDA_IN externally, the SDA line becomes fully
> I2C interface compatible. Separating acknowledge-output from serial data
> input is advantageous for chip-on-glass (COG) applications. In COG
> applications, the ITO resistance and the pull-up resistor will form a
> voltage  divider, which affects acknowledge-signal level. Larger ITO
> resistance will raise the acknowledged-signal level and system cannot
> recognize this level as a valid logic “0” level. By separating SDA_IN from
> SDA_OUT, the IC can be used in a mode that ignores the acknowledge-bit.
> For applications which check acknowledge-bit, it is necessary to minimize
> the ITO resistance of the SDA_OUT trace to guarantee a valid low level."

This has completely flown under the radar, thank you for pointing it out.

I will put the text from the datasheet together with ignore_nak.

> 
> ...
> 
> > +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)


> ...
> 
> > +
> > +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.

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


[...]
> > +
> > +static const struct drm_plane_helper_funcs st7571_primary_plane_helper_funcs = {
> > +	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
> > +	.atomic_check = st7571_primary_plane_helper_atomic_check,
> > +	.atomic_update = st7571_primary_plane_helper_atomic_update,
> > +};
> 
> Maybe you want an .atomic_disable callback that clears your screen ?

Good point, yes, I will add that.

> 
> 
> > +
> > +/*
> > + * CRTC
> > + */
> > +
> > +static const struct drm_crtc_helper_funcs st7571_crtc_helper_funcs = {
> > +	.atomic_check = drm_crtc_helper_atomic_check,
> 
> I think you could have an .mode_valid callback that just checks the fixed mode.

Got it.

> 
> > +/*
> > + * Encoder
> > + */
> > +
> > +static const struct drm_encoder_funcs st7571_encoder_funcs = {
> > +	.destroy = drm_encoder_cleanup,
> > +};
> 
> I recommend to have an encoder .atomic_{en,dis}able callbacks to init and turn 
> off your display respectively. That way, the driver can call st7571_lcd_init()
> only when the display is going to be used instead of at probe time.

I will look into this as well.

> 
> ...
> 
> > +static enum drm_mode_status st7571_mode_config_mode_valid(struct drm_device *dev,
> > +						       const struct drm_display_mode *mode)
> > +{
> > +	struct st7571_device *st7571 = drm_to_st7571(dev);
> > +
> > +	return drm_crtc_helper_mode_valid_fixed(&st7571->crtc, mode, &st7571->mode);
> > +}
> 
> The fact that you are calling a drm_crtc_helper here is an indication that probably
> this should be done in a struct drm_crtc_helper_funcs .mode_valid callback instead,
> as mentioned above.

I will move it to drm_crtc_helper_funcs.

> 
> > +
> > +static const struct drm_mode_config_funcs st7571_mode_config_funcs = {
> > +	.fb_create = drm_gem_fb_create_with_dirty,
> > +	.mode_valid = st7571_mode_config_mode_valid,
> 
> And that way you could just drop this handler.

Yep, thanks.

> 
> > +	.atomic_check = drm_atomic_helper_check,
> > +	.atomic_commit = drm_atomic_helper_commit,
> > +};
> > +
> 
> ...
> 
> > +static int st7571_probe(struct i2c_client *client)
> > +{
> > +	struct st7571_device *st7571;
> > +	struct drm_device *dev;
> > +	int ret;
> > +
> > +	st7571 = devm_drm_dev_alloc(&client->dev, &st7571_driver,
> > +				    struct st7571_device, dev);
> > +	if (IS_ERR(st7571))
> > +		return PTR_ERR(st7571);
> > +
> > +	dev = &st7571->dev;
> > +	st7571->client = client;
> > +	i2c_set_clientdata(client, st7571);
> > +
> > +	ret = st7571_parse_dt(st7571);
> > +	if (ret)
> > +		return ret;
> > +
> > +	st7571->mode = st7571_mode(st7571);
> > +
> > +	/*
> > +	 * The chip nacks some messages but still works as expected.
> > +	 * If the adapter does not support protocol mangling do
> > +	 * not set the I2C_M_IGNORE_NAK flag at the expense * of possible
> > +	 * cruft in the logs.
> > +	 */
> > +	if (i2c_check_functionality(client->adapter, I2C_FUNC_PROTOCOL_MANGLING))
> > +		st7571->ignore_nak = true;
> > +
> > +	st7571->regmap = devm_regmap_init(&client->dev, &st7571_regmap_bus,
> > +					   client, &st7571_regmap_config);
> > +	if (IS_ERR(st7571->regmap)) {
> > +		dev_err(&client->dev, "Failed to initialize regmap\n");
> 
> If you use dev_err_probe(), you can give some indication to users about
> what failed. It prints messages in the /sys/kernel/debug/devices_deferred
> debugfs entry.

Got it, thanks.

> 
> > +
> > +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?


> 
> And also a struct i2c_driver .shutdown callback to call to
> drm_atomic_helper_shutdown() as well.
> 
> -- 
> Best regards,
> 
> Javier Martinez Canillas
> Core Platforms
> Red Hat
> 

Best regards,
Marcus Folkesson

Attachment: signature.asc
Description: PGP signature