Hi Laurent,
On Mon, Oct 08, 2012 at 10:52:04PM +0200, Laurent Pinchart wrote:
> 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
> :-)
>
Okay. No error checking it is then.
> > > > + 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.
>
You're right. That should be made clear in the code.
> > > > +{
> > > > + 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
>
>
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 |
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
