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@xxxxxxx>
> ---
> drivers/gpu/drm/tiny/ofdrm.c | 62 ++++++++++++++++++++++++++++++++++--
>
Reviewed-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
[...]
> +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
