Hi Steffen,
On Monday 08 October 2012 14:48:01 Steffen Trumtrar wrote:
> On Mon, Oct 08, 2012 at 02:13:50PM +0200, Laurent Pinchart wrote:
> > On Thursday 04 October 2012 19:59:20 Steffen Trumtrar wrote:
> > > Get videomode from devicetree in a format appropriate for the
> > > backend. drm_display_mode and fb_videomode are supported atm.
> > > Uses the display signal timings from of_display_timings
> > >
> > > Signed-off-by: Steffen Trumtrar <s.trumtrar@xxxxxxxxxxxxxx>
> > > ---
> > >
> > > drivers/of/Kconfig | 5 +
> > > drivers/of/Makefile | 1 +
> > > drivers/of/of_videomode.c | 212 +++++++++++++++++++++++++++++++++++
> > > include/linux/of_videomode.h | 41 ++++++++
> > > 4 files changed, 259 insertions(+)
> > > create mode 100644 drivers/of/of_videomode.c
> > > create mode 100644 include/linux/of_videomode.h
[snip]
> > > diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
> > > new file mode 100644
> > > index 0000000..76ac16e
> > > --- /dev/null
> > > +++ b/drivers/of/of_videomode.c
[snip]
> > > +int videomode_from_timing(struct display_timings *disp, struct
> > > videomode *vm,
> > > + int index)
> > > +{
> > > + struct signal_timing *st = NULL;
> > > +
> > > + if (!vm)
> > > + return -EINVAL;
> > > +
> >
> > What about making vm a mandatory argument ? It looks to me like a caller
> > bug if vm is NULL.
>
> The caller must provide the struct videomode, yes. Wouldn't the kernel hang
> itself with a NULL pointer exception, if I just work with it ?
The kernel would oops, clearly showing the caller that a non-null vm is needed
:-)
> > > + st = display_timings_get(disp, index);
> > > +
> >
> > You can remove the blank line.
> >
> > > + if (!st) {
> > > + pr_err("%s: no signal timings found\n", __func__);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + vm->pixelclock = signal_timing_get_value(&st->pixelclock, 0);
> > > + vm->hactive = signal_timing_get_value(&st->hactive, 0);
> > > + vm->hfront_porch = signal_timing_get_value(&st->hfront_porch, 0);
> > > + vm->hback_porch = signal_timing_get_value(&st->hback_porch, 0);
> > > + vm->hsync_len = signal_timing_get_value(&st->hsync_len, 0);
> > > +
> > > + vm->vactive = signal_timing_get_value(&st->vactive, 0);
> > > + vm->vfront_porch = signal_timing_get_value(&st->vfront_porch, 0);
> > > + vm->vback_porch = signal_timing_get_value(&st->vback_porch, 0);
> > > + vm->vsync_len = signal_timing_get_value(&st->vsync_len, 0);
> > > +
> > > + vm->vah = st->vsync_pol_active_high;
> > > + vm->hah = st->hsync_pol_active_high;
> > > + vm->interlaced = st->interlaced;
> > > + vm->doublescan = st->doublescan;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +int of_get_videomode(struct device_node *np, struct videomode *vm, int
> > > index)
> >
> > I wonder how to avoid abuse of this functions. It's a useful helper for
> > drivers that need to get a video mode once only, but would result in lower
> > performances if a driver calls it for every mode. Drivers must call
> > of_get_display_timing_list instead in that case and case the display
> > timings. I'm wondering whether we should really expose of_get_videomode.
>
> The intent was to let the driver decide. That way all the other overhead may
> be skipped.
My point is that driver writers might just call of_get_videomode() in a loop,
not knowing that it's expensive. I want to avoid that. We need to at least add
kerneldoc to the function stating that this shouldn't be done.
> > > +{
> > > + struct display_timings *disp;
> > > + int ret = 0;
> >
> > No need to assign ret to 0 here.
>
> Ah, yes. Unneeded in this case.
>
> > > +
> > > + disp = of_get_display_timing_list(np);
> > > +
> >
> > You can remove the blank line.
> >
> > > + if (!disp) {
> > > + pr_err("%s: no timings specified\n", __func__);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + if (index == OF_DEFAULT_TIMING)
> > > + index = disp->default_timing;
> > > +
> > > + ret = videomode_from_timing(disp, vm, index);
> > > +
> >
> > No need for a blank line.
> >
> > > + if (ret)
> > > + return ret;
> > > +
> > > + display_timings_release(disp);
> > > +
> > > + if (!vm) {
> > > + pr_err("%s: could not get videomode %d\n", __func__, index);
> > > + return -EINVAL;
> > > + }
> >
> > This can't happen. If vm is NULL the videomode_from_timing call above will
> > return -EINVAL, and this function will then return immediately without
> > reaching this code block.
>
> Okay.
>
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(of_get_videomode);
--
Regards,
Laurent Pinchart
--
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
