Hi Douglas,
> > > void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
> >
> > If you, or someone else, could r-b or ack the pending patches to remove
> > this function, this part of the patch would no longer be needed.
>
> OK. I will likely be able to take a look next week. Given that I'm
> helping Philip bringup a board with ps8640 it looks like your patch
> series will be quite relevant! I guess it would be good to figure out
> what would be the best order to land them. In my case we need this fix
> to be easy to pick back to fix the behavior on the Chrome OS 5.4 tree.
> My fix is easy to pick back, but perhaps yours is as well. Of course
> we could also just make a local divergent change in our tree if need
> be, too.
I do not mind the order - so whatever works for you guys.
The only concern here is that we should not gain new users.
>
> > > {
> > > struct drm_encoder *encoder;
> > > - struct drm_bridge *iter;
> > >
> > > if (!bridge)
> > > return;
> > >
> > > encoder = bridge->encoder;
> > > - list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> > > - if (iter->funcs->pre_enable)
> > > - iter->funcs->pre_enable(iter);
> > > -
> > > - if (iter == bridge)
> > > - break;
> > > + list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> > > + if (bridge->funcs->pre_enable)
> > > + bridge->funcs->pre_enable(bridge);
> > > }
> > > }
> > > EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
> > > @@ -684,26 +680,30 @@ void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge,
> > > struct drm_atomic_state *old_state)
> > > {
> > > struct drm_encoder *encoder;
> > > + struct drm_bridge *iter;
> > s/iter/bridge/ would make the patch simpler
> > And then the bridge argument could be last_bridge or something.
> > This would IMO increase readability of the code and make the patch smaller.
>
> Yeah, I debated this too. I was trying to match
> drm_bridge_chain_disable() and in my mind keeping the two functions
> matching is more important than keeping this patch small.
Well, drm_bridge_chain_disable() is about to be deleted. So that the
wrong one to look at.
> Certainly I
> could add another patch in the series to rename "bridge" to
> "last_bridge" and "iter" to "bridge" in that function, but that
> defeats the goal of reducing churn... ...and clearly whoever wrote
> drm_bridge_chain_disable() liked "iter" better. :-P
>
> In any case, I'll change it as you say if everyone likes it better,
> but otherwise I'll leave it as I have it.
>
>
> > > if (!bridge)
> > > return;
> > >
> > > encoder = bridge->encoder;
> > > - list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> > > - if (bridge->funcs->atomic_post_disable) {
> > > + list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> > > + if (iter->funcs->atomic_post_disable) {
> > > struct drm_bridge_state *old_bridge_state;
> > >
> > > old_bridge_state =
> > > drm_atomic_get_old_bridge_state(old_state,
> > > - bridge);
> > > + iter);
> > > if (WARN_ON(!old_bridge_state))
> > > return;
> > >
> > > - bridge->funcs->atomic_post_disable(bridge,
> > > - old_bridge_state);
> > > - } else if (bridge->funcs->post_disable) {
> > > - bridge->funcs->post_disable(bridge);
> > > + iter->funcs->atomic_post_disable(iter,
> > > + old_bridge_state);
> > > + } else if (iter->funcs->post_disable) {
> > > + iter->funcs->post_disable(iter);
> > > }
> > > +
> > > + if (iter == bridge)
> > > + break;
> > I cannot see why this is needed, we are at the end of the list here
> > anyway.
I see, please include this change in your changelog and add it to the
documentation in drm_bridge_h.
Sam
