Thomas Zimmermann <tzimmermann@xxxxxxx> writes:
Hello Thomas,
> Hi Javier
>
> Am 05.10.23 um 15:38 schrieb Javier Martinez Canillas:
>> Thomas Zimmermann <tzimmermann@xxxxxxx> writes:
[...]
>>> +static int simpledrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
>>> + struct drm_atomic_state *state)
>>> +{
>>> + struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
>>> + struct drm_shadow_plane_state *new_shadow_plane_state =
>>> + to_drm_shadow_plane_state(new_plane_state);
>>> + struct drm_framebuffer *new_fb = new_plane_state->fb;
>>> + struct drm_crtc *new_crtc = new_plane_state->crtc;
>>> + struct drm_crtc_state *new_crtc_state = NULL;
>>> + struct drm_device *dev = plane->dev;
>>> + struct simpledrm_device *sdev = simpledrm_device_of_dev(dev);
>>> + int ret;
>>> +
>>> + if (new_crtc)
>>> + new_crtc_state = drm_atomic_get_new_crtc_state(state, new_crtc);
>>> +
>>> + ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
>>> + DRM_PLANE_NO_SCALING,
>>> + DRM_PLANE_NO_SCALING,
>>> + false, false);
>>
>> Same comment that with the ssd130x driver. I think that we should use the
>> drm_plane_helper_atomic_check() helper instead of open coding it in each
>
> I'm going to replace the call in simpledrm.
> drm_plane_helper_atomic_check() is useful to remove the entire
> atomic_check function from the driver; it does nothing apart from that.
> I've been called out before for such do-nothing helpers; deservedly so. [1]
>
The argument then is that drivers should open code *exactly* the same code
that the helper function already has just because they implement their own
.atomic_check callback?
And that the helper should only be used when is the .atomic_check callback
but not as a helper function?
I don't understand that rationale to be honest, but if there is one then
it should be very clear in the kernel-doc what functions are supposed to
be used only as callbacks and what functions can also be used as helpers.
> Best regards
> Thomas
>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
