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

["Noralf Trønnes" <noralf@tronnes.org>]

Den 07.05.2016 11:59, skrev Daniel Vetter:
> On Fri, May 6, 2016 at 9:45 PM, Noralf Trønnes <noralf@tronnes.org> 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
>> *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
> things.
>
>> 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.

Noralf.