On 12/10/2021 15:38, Tomi Valkeinen wrote:
> On 12/10/2021 16:23, Neil Armstrong wrote:
>
>>>> + struct drm_private_obj glob_obj;
>>>> +
>>>> struct drm_fb_helper *fbdev;
>>>> struct workqueue_struct *wq;
>>>> @@ -88,5 +105,9 @@ struct omap_drm_private {
>>>> void omap_debugfs_init(struct drm_minor *minor);
>>>> +struct omap_global_state *__must_check
>>>> +omap_get_global_state(struct drm_atomic_state *s);
>>>> +struct omap_global_state *
>>>> +omap_get_existing_global_state(struct omap_drm_private *priv);
>>>
>>> These could also be separated by empty lines. At least to my eyes it gets
>>> confusing if those declarations are not separated.
>>
>> Atomic states can be extremely confusing, and hard to track.
>> I checked and they do what they are documented for...
>>
>> The omap_get_existing_global_state() is the most confusing since the result
>> depends if
>> we are in an atomic transaction of not.
>
> So here I was just talking about the cosmetics, how the lines above look
> like. I have trouble seeing where the function declaration starts and where
> it ends without looking closely, as both lines of the declaration start at
> the first column, and there are no empty lines between the declarations.
Ok, it's a legacy of the 80chars max, will reformat.
>
> But now that you mention, yes, the states are confusing =). And this series
> is somewhat difficult. I think it's important for future maintainability to
> include explanations and comments in this series for the confusing parts
> (plane-overlay mapping and state handling, mostly).
Yep I added some hopefully useful comments explaining that.
Neil
>
> Tomi
