Re: [PATCH 1/3] drm/panel: Add display_timing support

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2877 bytes --]

On Tue, Mar 03, 2015 at 12:49:43PM +0100, Philipp Zabel wrote:
> Hi Thierry,
> 
> Am Montag, den 23.02.2015, 15:04 +0100 schrieb Philipp Zabel:
> > Hi Thierry,
> > 
> > do you have any further thoughts on this?
> 
> [...]
> > > Sorry for taking so long to get back on this. I generally like the idea,
> > > though a couple of things are unclear to me.
> > > 
> > > In struct display_timing we have:
> > > 
> > > 	struct timing_entry hactive;
> > > 	...
> > > 	struct timing_entry vactive;
> > > 
> > > I wonder if that really makes sense. Are there really datasheets that
> > > provide a valid range for the number of horizontal and vertical active
> > > pixels?
> > 
> > I could send a patch to change hactive/vactive to a single value
> > and change Documentation/devicetree/bindings/video/display-timing.txt
> > to clarify ranges are not allowed for these properties until somebody
> > digs out a panel that actually needs this.
> > Adding Steffen to Cc: in case there was a reason other than symmetry.
> 
> That seems not to be the case so far.
> 
> [...]
> > > >  /**
> > > >   * struct drm_panel_funcs - perform operations on a given panel
> > > > @@ -38,6 +39,8 @@ struct drm_panel;
> > > >   * @enable: enable panel (turn on back light, etc.)
> > > >   * @get_modes: add modes to the connector that the panel is attached to and
> > > >   * return the number of modes added
> > > > + * @get_timings: copy display timings into the provided array and return
> > > > + * the number of display timings available
> > > >   *
> > > >   * The .prepare() function is typically called before the display controller
> > > >   * starts to transmit video data. Panel drivers can use this to turn the panel
> > > > @@ -68,6 +71,8 @@ struct drm_panel_funcs {
> > > >  	int (*prepare)(struct drm_panel *panel);
> > > >  	int (*enable)(struct drm_panel *panel);
> > > >  	int (*get_modes)(struct drm_panel *panel);
> > > > +	int (*get_timings)(struct drm_panel *panel, unsigned int num_timings,
> > > > +			   struct display_timing *timings);
> > > 
> > > Also are there even panels that support more than one set of timings?
> > > I've never heard of panels that are actually able to do more than one
> > > mode, so I'm wondering if num_timings isn't overdoing it a little here.
> > > I guess it doesn't hurt all that much, though.
> > 
> > Would you prefer
> > 	struct display_timing *(*get_timing)(struct drm_panel *panel);
> > ?
> 
> I'd like to resend this. Please let me know if you want me to change
> this function prototype.

I have no objections to keeping the current prototype. It's something we
can always fixup if we want to. Also keeping the symmetry with min/max
values for hactive and vactive is okay in my opinion.

Were there any other remaining points? If not I'll just apply this as
is.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel