Hi Sean.
Nits below.
>
> + /**
> + * @atomic_disable:
> + *
...
> + *
> + * This callback is a variant of @disable that provides the atomic state
> + * to the driver. It takes priority over @disable during atomic commits.
> + *
> + * This hook is used only by atomic helpers. Atomic drivers don't need
> + * to implement it if there's no need to disable anything at the encoder
> + * level. To ensure that runtime PM handling (using either DPMS or the
> + * new "ACTIVE" property) works @atomic_disable must be the inverse of
> + * @atomic_enable.
> + */
> + void (*atomic_disable)(struct drm_encoder *encoder,
> + struct drm_atomic_state *state);
> +
> + /**
> + * @atomic_enable:
> + *
...
> + *
> + * This callback is a variant of @enable that provides the atomic state
> + * to the driver. It is called in place of @enable during atomic
> + * commits.
The wording in this paragrap is not the same as the similar paragraph
above.
One says "it takes priority over"
Another says "called in place of"
Maybe be a bit more explicit and say that "if atomic_{dis,en}able is
define then {dis,en}able is not called?
> + *
> + * This hook is used only by atomic helpers, for symmetry with @disable.
I do not get the "for symmetry with @disable."?
> + * Atomic drivers don't need to implement it if there's no need to
> + * enable anything at the encoder level. To ensure that runtime PM
> + * handling (using either DPMS or the new "ACTIVE" property) works
> + * @enable must be the inverse of @disable for atomic drivers.
Did you want to say "@atomic_enable must be the inverse of @atomic_disable for atomic drivers."?
> + */
> + void (*atomic_enable)(struct drm_encoder *encoder,
> + struct drm_atomic_state *state);
> +
> /**
> * @disable:
> *
> @@ -695,6 +740,9 @@ struct drm_encoder_helper_funcs {
> * handling (using either DPMS or the new "ACTIVE" property) works
> * @disable must be the inverse of @enable for atomic drivers.
> *
> + * For atomic drivers also consider @atomic_disable and save yourself
> + * from having to read the NOTE below!
Maybe, if this is so, say that atomic drivers shall alwyas use the
atomic_* variants?
And then add a TODO entry to make it so for the other atomic drivers?
> + *
> * NOTE:
> *
> * With legacy CRTC helpers there's a big semantic difference between
Sam
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
