Re: [PATCH 4/4] drm/panel: Add helper for simple panel connector

From: Noralf TrÃnnes
Date: Sat May 07 2016 - 08:47:10 EST

Den 07.05.2016 11:59, skrev Daniel Vetter:
On Fri, May 6, 2016 at 9:45 PM, Noralf TrÃnnes <noralf@xxxxxxxxxxx> wrote:
In the discussion following the "no more fbdev drivers" call, someone
pointed me to drm_panel. It had function hooks that I needed, so I built
tinydrm around it. If this is misusing drm_panel, it shouldn't be too
difficult to drop it.

Actually I could just do this:

struct tinydrm_funcs {
int (*prepare)(struct tinydrm_device *tdev);
int (*unprepare)(struct tinydrm_device *tdev);
int (*enable)(struct tinydrm_device *tdev);
int (*disable)(struct tinydrm_device *tdev);
int (*dirty)(struct drm_framebuffer *fb, void *vmem, unsigned flags,
unsigned color, struct drm_clip_rect *clips,
unsigned num_clips);

When it comes to the simple connector, one solution could be to extend
drm_simple_display_pipe to embed a connector with (optional) modes:

struct drm_simple_display_pipe {
- struct drm_connector *connector;
+ struct drm_connector connector;
+ const struct drm_display_mode *modes;
+ unsigned int num_modes;

And maybe optional detect and get_modes hooks:

struct drm_simple_display_pipe_funcs {
+ enum drm_connector_status detect(struct drm_connector *connector,
+ bool force);
+ int (*get_modes)(struct drm_connector *connector);

int drm_simple_display_pipe_init(struct drm_device *dev,
struct drm_simple_display_pipe *pipe,
struct drm_simple_display_pipe_funcs
const uint32_t *formats,
unsigned int format_count,
- struct drm_connector *connector);
+ int connector_type);
Tbh I really like that simple_display_pipe is split after the encoder.
This allows us to easily splice in a drm_bridge (which will register
the drm_connector itself) with very little work. And even if there's
not a full-blown bridge you might have different connectors and

Another solution is to create a simple connector with modes:

struct drm_simple_connector {
struct drm_connector connector;
const struct drm_display_mode *modes;
unsigned int num_modes;

struct drm_connector *drm_simple_connector_create(struct drm_device *dev,
const struct drm_display_mode *modes, unsigned int num_modes,
int connector_type);
Yeah, this makes more sense to me, but then we're kinda reinventing
something like simple-panel.c with this. Otoh right now with
smple_display_pipe it's not possible to wire up the panel callbacks
easily, so maybe it should be a drm_bridge. Or we just leave this code
in tinydrm and extract it when someone else has a need for it?

Yes, since there's no obvious use for such a simple controller I'll just put it in tinydrm.