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

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 08.04.25 um 15:20 schrieb Marcus Folkesson:
[...]
>>
>>> +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)

For testing purposes, you can add the _R formats to the switch case at

https://elixir.bootlin.com/linux/v6.13.7/source/drivers/gpu/drm/drm_fb_helper.c#L1246

and see how it goes.

Best regards
Thomas

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

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