Message-ID: <c4669293-0d56-4bdd-9075-01281042b002@suse.de>
Date: Tue, 8 Apr 2025 13:01:53 +0200
From: Thomas Zimmermann <tzimmermann@...e.de>
To: Javier Martinez Canillas <javierm@...hat.com>,
 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>, 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, Thomas Zimmermann <tzimmrmann@...e.de>
Subject: Re: [PATCH v3 2/3] drm/st7571-i2c: add support for Sitronix ST7571
 LCD controller

Hi,

lots of good points in the review.

Am 08.04.25 um 12:44 schrieb Javier Martinez Canillas:
[...]
>> 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.

That sounds like a good idea. But the other existing drivers are based 
on mipi-dbi helpers, while this one isn't. Not sure if that's important 
somehow.

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

It would be a starting point for XRGB8888 on C1/R1. I always wanted to 
reimplement drm_fb_xrgb8888_to_mono() [1] with the generic _xfrm_ 
helpers. Once the generic helpers can do such low-bit formats, C2 would 
also work easily.

[1] 
https://elixir.bootlin.com/linux/v6.14-rc6/source/drivers/gpu/drm/drm_format_helper.c#L1114

Best regards
Thomas

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

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Powered by blists - more mailing lists