Hi

Am 26.07.22 um 15:36 schrieb Javier Martinez Canillas:
On 7/20/22 16:27, Thomas Zimmermann wrote:
Add a dedicated CRTC state to ofdrm to later store information for
palette updates.

Signed-off-by: Thomas Zimmermann <tzimmerm...@suse.de>
---
  drivers/gpu/drm/tiny/ofdrm.c | 62 ++++++++++++++++++++++++++++++++++--

Reviewed-by: Javier Martinez Canillas <javi...@redhat.com>

[...]

+static void ofdrm_crtc_reset(struct drm_crtc *crtc)
+{
+       struct ofdrm_crtc_state *ofdrm_crtc_state;
+
+       if (crtc->state) {
+               ofdrm_crtc_state_destroy(to_ofdrm_crtc_state(crtc->state));
+               crtc->state = NULL; /* must be set to NULL here */
+       }
+
+       ofdrm_crtc_state = kzalloc(sizeof(*ofdrm_crtc_state), GFP_KERNEL);
+       if (!ofdrm_crtc_state)
+               return;
+       __drm_atomic_helper_crtc_reset(crtc, &ofdrm_crtc_state->base);
+}
+

IMO this function is hard to read, I would instead write it as following:

static void ofdrm_crtc_reset(struct drm_crtc *crtc)
{
         struct ofdrm_crtc_state *ofdrm_crtc_state = 
kzalloc(sizeof(*ofdrm_crtc_state), GFP_KERNEL);

        if (!ofdrm_crtc_state)
                return;

         if (crtc->state) {
                 ofdrm_crtc_state_destroy(to_ofdrm_crtc_state(crtc->state));
                crtc->state = NULL; /* must be set to NULL here */
        }

         __drm_atomic_helper_crtc_reset(crtc, &ofdrm_crtc_state->base);
}

Also with that form I think that the crtc->state = NULL could just be dropped ?

I once had to add this line to a driver to make the DRM helpers work. But I cannot find any longer why. Maybe it's been resolved meanwhile.

Best regards
Thomas



--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Attachment: OpenPGP_signature
Description: OpenPGP digital signature

Reply via email to