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