On Wed, Feb 09, 2022 at 01:12:45PM +0200, Jani Nikula wrote:
> On Wed, 09 Feb 2022, Sascha Hauer <s.hauer@xxxxxxxxxxxxxx> 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
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
