On Wed, 21 Aug 2019 17:45:04 +0300 Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Boris,
Thank you for the patch.
On Thu, Aug 08, 2019 at 05:11:37PM +0200, Boris Brezillon wrote:
DRM bridges should not be operated independently. Let's rename the drm_bridge_xxx() helpers that take the first bridge of the chain and iterate over the whole chain into drm_bridge_chain_xx(). We also pass it the encoder containing the bridge chain instead of the dereferencing encoder->bridge.
I'm not sure about this. I do agree that the helpers operate on a chain, but I think they're use for calling them on any bridge, especially in conjunction with your work. The way I see it is that when a bridge in the chain needs a custom enable/disable sequence (flagged by some kind of flag in the bridge structure), the helpers will not automatically propagate the calls down the chain, and let the bridge call the pre/post enable/disable operations on the downstream bridge. This means that those bridges with special needs will have to call the helpers on the next bridge down the chain, and thus require keeping the ability to do so.
Also changed my mind on this one after the discussion we had. I kept the rename part but dropper the s/bridge/encoder/ change. This way, bridges that need to control the enable/disable sequence can use those helpers on a sub-chain (the chain starting at the bridge element just after them).
We could of course propose two sets of helpers, one taking a bridge pointer, and another one taking an encoder pointer, but I think it's a bit overkill. Especially if you consider my comments earlier in this series where I propose moving the custom sequence feature to bridges instead of encoders, I don't think this patch will be needed.
Agreed. As said above, I kept the rename part of this patch because I think it better reflects the fact that those helpers are acting on a bridge chain, and not a single bridge element.