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 <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/tiny/ofdrm.c | 62 ++++++++++++++++++++++++++++++++++--
>>   
> 
> Reviewed-by: Javier Martinez Canillas <javierm@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