Re: [PATCH 2/2 v6] of: add generic videomode description

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Steffen Trumtrar <s.trumtrar@pengutronix.de>
Cc: devicetree-discuss@lists.ozlabs.org,
	Rob Herring <robherring2@gmail.com>,
	linux-fbdev@vger.kernel.org, dri-devel@lists.freedesktop.org,
	linux-media@vger.kernel.org,
	Tomi Valkeinen <tomi.valkeinen@ti.com>
Subject: Re: [PATCH 2/2 v6] of: add generic videomode description
Date: Mon, 08 Oct 2012 22:52:04 +0200	[thread overview]
Message-ID: <1737299.6PuzOm7XuT@avalon> (raw)
In-Reply-To: <20121008124801.GD20800@pengutronix.de>

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@pengutronix.de>
> > > ---
> > > 
> > >  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