[PATCH v2 08/10] drm/ofdrm: Add CRTC state

[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 <tzimmermann at suse.de>
> ---
>  drivers/gpu/drm/tiny/ofdrm.c | 62 ++++++++++++++++++++++++++++++++++--
>  

Reviewed-by: Javier Martinez Canillas <javierm at 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 ?

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat