On Thu, Feb 10, 2022 at 12:52:32PM +0100, Sascha Hauer wrote:
> 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.
FYI here is a patch for this solution.
Sascha
------------------------------8<--------------------------------
