On Wed, Feb 09, 2022 at 01:12:45PM +0200, Jani Nikula wrote:
On Wed, 09 Feb 2022, Sascha Hauer s.hauer@pengutronix.de wrote:
David, Daniel,
I'll need a word from you regarding this patch. It's needed in patch 22/23 in this series. vop2_crtc_atomic_enable() needs to control the mux which routes the display output to the different encoders. Which encoder is used is described in the of_graph port, so I need a way to identify the encoder in the device tree.
I think the question is how useful is this going to be in general. IMO we should not be adding members that are useful in a single driver only.
For example i915 wraps encoders with:
struct intel_encoder { struct drm_encoder base;
/* i915 specific stuff here*/};
So that we can add stuff of our own there. Of course, it does mean a bunch of overhead for the first time you need to do it. But adding driver specific stuff to struct drm_encoder adds overhead for everyone.
All that said, *I* don't know how useful the port member would be in drivers that use device tree. Maybe it's worth it.
I don't know either.
Right now the drm_encoder is directly embedded into the encoder drivers private data structures, like this:
struct rockchip_hdmi { struct drm_encoder encoder; ... };
I could change this to:
struct rockchip_encoder { struct device_node *port; struct drm_encoder encoder; }
and then
struct rockchip_hdmi { struct rockchip_encoder encoder; ... };
That would solve the issue without touching generic DRM code if that's preferred.
Sascha