Re: [PATCH v8 1/6] video: add display_timing and videomode

[Steffen Trumtrar <s.trumtrar@pengutronix.de>]

On Tue, Nov 13, 2012 at 11:41:59AM +0100, Thierry Reding wrote:
> On Mon, Nov 12, 2012 at 04:37:01PM +0100, Steffen Trumtrar wrote:
> [...]
> > diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> > index d08d799..2a23b18 100644
> > --- a/drivers/video/Kconfig
> > +++ b/drivers/video/Kconfig
> > @@ -33,6 +33,12 @@ config VIDEO_OUTPUT_CONTROL
> >  	  This framework adds support for low-level control of the video 
> >  	  output switch.
> >  
> > +config DISPLAY_TIMING
> 
> DISPLAY_TIMINGS?
> 
> >  #video output switch sysfs driver
> >  obj-$(CONFIG_VIDEO_OUTPUT_CONTROL) += output.o
> > +obj-$(CONFIG_DISPLAY_TIMING) += display_timing.o
> 
> display_timings.o?
> 
> > +obj-$(CONFIG_VIDEOMODE) += videomode.o
> > diff --git a/drivers/video/display_timing.c b/drivers/video/display_timing.c
> 
> display_timings.c?
> 

I originally had that and changed it by request to the singular form.
(Can't find the mail atm). And I think this fits better with all the other drivers.

> > +int videomode_from_timing(struct display_timings *disp, struct videomode *vm,
> > +			  unsigned int index)
> 
> I find the indexing API a bit confusing. But that's perhaps just a
> matter of personal preference.
> 
> Also the ordering of arguments seems a little off. I find it more
> natural to have the destination pointer in the first argument, similar
> to the memcpy() function, so this would be:
> 
> int videomode_from_timing(struct videomode *vm, struct display_timings *disp,
> 			  unsigned int index);
> 
> Actually, when reading videomode_from_timing() I'd expect the argument
> list to be:
> 
> int videomode_from_timing(struct videomode *vm, struct display_timing *timing);
> 
> Am I the only one confused by this?
> 

I went with the of_xxx-functions that have fname(from_node, to_property)
and personally prefer it this way. Therefore I'd like to keep it as is.

> > diff --git a/include/linux/display_timing.h b/include/linux/display_timing.h
> 
> display_timings.h?
> 
> > +/* placeholder function until ranges are really needed 
> 
> The above line has trailing whitespace. Also the block comment should
> have the opening /* on a separate line.
> 

Okay.

> > + * the index parameter should then be used to select one of [min typ max]
> 
> If index is supposed to select min, typ or max, then maybe an enum would
> be a better candidate? Or alternatively provide separate accessors, like
> display_timing_get_{minimum,typical,maximum}().
> 

Hm, I'm not so sure about this one. I'd prefer the enum.

> > + */
> > +static inline u32 display_timing_get_value(struct timing_entry *te,
> > +					   unsigned int index)
> > +{
> > +	return te->typ;
> > +}
> > +
> > +static inline struct display_timing *display_timings_get(struct display_timings *disp,
> > +							 unsigned int index)
> > +{
> > +	if (disp->num_timings > index)
> > +		return disp->timings[index];
> > +	else
> > +		return NULL;
> > +}
> > +
> > +void timings_release(struct display_timings *disp);
> 
> This function no longer exists.
> 
Right.

Steffen

> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |