Message-ID: <87cydn9bkx.fsf@minerva.mail-host-address-is-not-set>
Date: Tue, 08 Apr 2025 12:44:46 +0200
From: Javier Martinez Canillas <javierm@...hat.com>
To: Marcus Folkesson <marcus.folkesson@...il.com>, David Airlie
 <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, Maarten Lankhorst
 <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>,
 Thomas Zimmermann <tzimmermann@...e.de>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>
Cc: dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, Marcus Folkesson
 <marcus.folkesson@...il.com>, Thomas Zimmermann <tzimmrmann@...e.de>
Subject: Re: [PATCH v3 2/3] drm/st7571-i2c: add support for Sitronix ST7571
 LCD controller

Marcus Folkesson <marcus.folkesson@...il.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.
>

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.

> Reviewed-by: Thomas Zimmermann <tzimmrmann@...e.de>
> Signed-off-by: Marcus Folkesson <marcus.folkesson@...il.com>
> ---
>  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.

>  3 files changed, 733 insertions(+)
>
> diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
> index 94cbdb1337c07f1628a33599a7130369b9d59d98..33a69aea4232c5ca7a04b1fe18bb424e0fded697 100644
> --- a/drivers/gpu/drm/tiny/Kconfig
> +++ b/drivers/gpu/drm/tiny/Kconfig
> @@ -232,6 +232,17 @@ config TINYDRM_ST7586
>  
>  	  If M is selected the module will be called st7586.
>  
> +config DRM_ST7571_I2C
> +	tristate "DRM support for Sitronix ST7571 display panels (I2C)"
> +	depends on DRM && I2C
> +	select DRM_GEM_SHMEM_HELPER

DRM_GEM_SHMEM_HELPER depends on MMU and your driver is selecting it,
so it should also depend on MMU, i.e: depends on DRM && MMU && I2C.



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

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

...

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

...

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

...

> +static void st7571_primary_plane_helper_atomic_update(struct drm_plane *plane,
> +						   struct drm_atomic_state *state)
> +{
> +	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
> +	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
> +	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
> +	struct drm_framebuffer *fb = plane_state->fb;
> +	struct drm_atomic_helper_damage_iter iter;
> +	struct drm_device *dev = plane->dev;
> +	struct drm_rect damage;
> +	struct st7571_device *st7571 = drm_to_st7571(plane->dev);
> +	int ret, idx;
> +
> +	if (!fb)
> +		return; /* no framebuffer; plane is disabled */
> +
> +	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> +	if (ret)
> +		return;
> +
> +	if (!drm_dev_enter(dev, &idx))

Should do a drm_gem_fb_end_cpu_access() here before returning.

> +		return;
> +
> +	ret = st7571_set_pixel_format(st7571, fb->format->format);
> +	if (ret) {
> +		dev_err(dev->dev, "Failed to set pixel format: %d\n", ret);

And here I think you need to do both drm_gem_fb_end_cpu_access() and drm_dev_exit().

> +		return;
> +	}
> +
> +	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
> +	drm_atomic_for_each_plane_damage(&iter, &damage) {
> +		st7571_fb_blit_rect(fb, &shadow_plane_state->data[0], &damage);
> +	}
> +
> +	drm_dev_exit(idx);
> +	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
> +}
> +
> +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 ?


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

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

...

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

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

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

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

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

Powered by blists - more mailing lists