Hi,
On Wed, Nov 21, 2012 at 01:37:08PM +0200, Tomi Valkeinen wrote:
> Hi,
>
> On 2012-11-20 17:54, Steffen Trumtrar wrote:
> > Add display_timing structure and the according helper functions. This allows
> > the description of a display via its supported timing parameters.
> >
> > Every timing parameter can be specified as a single value or a range
> > <min typ max>.
> >
> > Also, add helper functions to convert from display timings to a generic videomode
> > structure. This videomode can then be converted to the corresponding subsystem
> > mode representation (e.g. fb_videomode).
>
> Sorry for reviewing this so late.
>
> One thing I'd like to see is some explanation of the structs involved.
> For example, in this patch you present structs videomode, display_timing
> and display_timings without giving any hint what they represent.
>
> I'm not asking for you to write a long documentation, but perhaps the
> header files could include a few lines of comments above the structs,
> explaining the idea.
>
Okay. Will do.
> > +void display_timings_release(struct display_timings *disp)
> > +{
> > + if (disp->timings) {
> > + unsigned int i;
> > +
> > + for (i = 0; i < disp->num_timings; i++)
> > + kfree(disp->timings[i]);
> > + kfree(disp->timings);
> > + }
> > + kfree(disp);
> > +}
> > +EXPORT_SYMBOL_GPL(display_timings_release);
>
> Perhaps this will become clearer after reading the following patches,
> but it feels a bit odd to add a release function, without anything in
> this patch that would actually allocate the timings.
>
2/6 uses this function. And as this does not belong to the DT part, it
is added in this patch.
> > diff --git a/drivers/video/videomode.c b/drivers/video/videomode.c
> > new file mode 100644
> > index 0000000..e24f879
> > --- /dev/null
> > +++ b/drivers/video/videomode.c
> > @@ -0,0 +1,46 @@
> > +/*
> > + * generic display timing functions
> > + *
> > + * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@xxxxxxxxxxxxxx>, Pengutronix
> > + *
> > + * This file is released under the GPLv2
> > + */
> > +
> > +#include <linux/export.h>
> > +#include <linux/errno.h>
> > +#include <linux/display_timing.h>
> > +#include <linux/kernel.h>
> > +#include <linux/videomode.h>
> > +
> > +int videomode_from_timing(const struct display_timings *disp,
> > + struct videomode *vm, unsigned int index)
> > +{
> > + struct display_timing *dt;
> > +
> > + dt = display_timings_get(disp, index);
> > + if (!dt)
> > + return -EINVAL;
> > +
> > + vm->pixelclock = display_timing_get_value(&dt->pixelclock, 0);
> > + vm->hactive = display_timing_get_value(&dt->hactive, 0);
> > + vm->hfront_porch = display_timing_get_value(&dt->hfront_porch, 0);
> > + vm->hback_porch = display_timing_get_value(&dt->hback_porch, 0);
> > + vm->hsync_len = display_timing_get_value(&dt->hsync_len, 0);
> > +
> > + vm->vactive = display_timing_get_value(&dt->vactive, 0);
> > + vm->vfront_porch = display_timing_get_value(&dt->vfront_porch, 0);
> > + vm->vback_porch = display_timing_get_value(&dt->vback_porch, 0);
> > + vm->vsync_len = display_timing_get_value(&dt->vsync_len, 0);
>
> Shouldn't all these calls get the typical value, with index 1?
>
Yes. I omitted the indexing until now. So it didn't matter what value was used.
But I will integrate it in the next version.
> > +
> > + vm->vah = dt->vsync_pol_active;
> > + vm->hah = dt->hsync_pol_active;
> > + vm->de = dt->de_pol_active;
> > + vm->pixelclk_pol = dt->pixelclk_pol;
> > +
> > + vm->interlaced = dt->interlaced;
> > + vm->doublescan = dt->doublescan;
> > +
> > + return 0;
> > +}
> > +
> > +EXPORT_SYMBOL_GPL(videomode_from_timing);
> > diff --git a/include/linux/display_timing.h b/include/linux/display_timing.h
> > new file mode 100644
> > index 0000000..d5bf03f
> > --- /dev/null
> > +++ b/include/linux/display_timing.h
> > @@ -0,0 +1,70 @@
> > +/*
> > + * Copyright 2012 Steffen Trumtrar <s.trumtrar@xxxxxxxxxxxxxx>
> > + *
> > + * description of display timings
> > + *
> > + * This file is released under the GPLv2
> > + */
> > +
> > +#ifndef __LINUX_DISPLAY_TIMINGS_H
> > +#define __LINUX_DISPLAY_TIMINGS_H
> > +
> > +#include <linux/types.h>
>
> What is this needed for? u32? I don't see it defined in types.h
>
Yes, u32. What would be the right header for that if not types.h?
> > +
> > +struct timing_entry {
> > + u32 min;
> > + u32 typ;
> > + u32 max;
> > +};
> > +
> > +struct display_timing {
> > + struct timing_entry pixelclock;
> > +
> > + struct timing_entry hactive;
> > + struct timing_entry hfront_porch;
> > + struct timing_entry hback_porch;
> > + struct timing_entry hsync_len;
> > +
> > + struct timing_entry vactive;
> > + struct timing_entry vfront_porch;
> > + struct timing_entry vback_porch;
> > + struct timing_entry vsync_len;
> > +
> > + unsigned int vsync_pol_active;
> > + unsigned int hsync_pol_active;
> > + unsigned int de_pol_active;
> > + unsigned int pixelclk_pol;
> > + bool interlaced;
> > + bool doublescan;
> > +};
> > +
> > +struct display_timings {
> > + unsigned int num_timings;
> > + unsigned int native_mode;
> > +
> > + struct display_timing **timings;
> > +};
> > +
> > +/*
> > + * placeholder function until ranges are really needed
> > + * the index parameter should then be used to select one of [min typ max]
> > + */
> > +static inline u32 display_timing_get_value(const struct timing_entry *te,
> > + unsigned int index)
> > +{
> > + return te->typ;
> > +}
>
> Why did you opt for a placeholder here? It feels trivial to me to have
> support to get the min/typ/max value properly.
>
I will add that in the next version.
> > +static inline struct display_timing *display_timings_get(const struct
> > + display_timings *disp,
> > + unsigned int index)
> > +{
> > + if (disp->num_timings > index)
> > + return disp->timings[index];
> > + else
> > + return NULL;
> > +}
> > +
> > +void display_timings_release(struct display_timings *disp);
> > +
> > +#endif
> > diff --git a/include/linux/videomode.h b/include/linux/videomode.h
> > new file mode 100644
> > index 0000000..5d3e796
> > --- /dev/null
> > +++ b/include/linux/videomode.h
> > @@ -0,0 +1,40 @@
> > +/*
> > + * Copyright 2012 Steffen Trumtrar <s.trumtrar@xxxxxxxxxxxxxx>
> > + *
> > + * generic videomode description
> > + *
> > + * This file is released under the GPLv2
> > + */
> > +
> > +#ifndef __LINUX_VIDEOMODE_H
> > +#define __LINUX_VIDEOMODE_H
> > +
> > +#include <linux/display_timing.h>
>
> This is not needed, just add:
>
> struct display_timings;
>
Okay.
> > +struct videomode {
> > + u32 pixelclock;
> > + u32 refreshrate;
> > +
> > + u32 hactive;
> > + u32 hfront_porch;
> > + u32 hback_porch;
> > + u32 hsync_len;
> > +
> > + u32 vactive;
> > + u32 vfront_porch;
> > + u32 vback_porch;
> > + u32 vsync_len;
> > +
> > + u32 hah;
> > + u32 vah;
> > + u32 de;
> > + u32 pixelclk_pol;
> > +
> > + bool interlaced;
> > + bool doublescan;
> > +};
> > +
> > +int videomode_from_timing(const struct display_timings *disp,
> > + struct videomode *vm, unsigned int index);
> > +
>
> Are this and the few other functions above meant to be used from
> drivers? If so, some explanation of the parameters here would be nice.
> If they are just framework internal, they don't probably need that.
>
Okay. I will add some more documentation.
Regards,
Steffen
--
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 |
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
