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

[Thierry Reding <thierry.reding@avionic-design.de>]

[-- Attachment #1: Type: text/plain, Size: 1921 bytes --]

On Sun, Oct 07, 2012 at 03:38:33PM +0200, Laurent Pinchart wrote:
> Hi Steffen,
> 
> On Friday 05 October 2012 17:51:21 Steffen Trumtrar wrote:
> > On Thu, Oct 04, 2012 at 12:51:00PM -0600, Stephen Warren wrote:
> > > On 10/04/2012 11:59 AM, 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
> > > > 
> > > > +++ b/drivers/of/of_videomode.c
> > > > 
> > > > +int videomode_from_timing(struct display_timings *disp, struct
> > > > videomode *vm,
> > > > 
> > > > +	st = display_timings_get(disp, index);
> > > > +
> > > > +	if (!st) {
> > > 
> > > It's a little odd to leave a blank line between those two lines.
> > 
> > Hm, well okay. That can be remedied
> > 
> > > Only half of the code in this file seems OF-related; the routines to
> > > convert a timing to a videomode or drm display mode seem like they'd be
> > > useful outside device tree, so I wonder if putting them into
> > > of_videomode.c is the correct thing to do. Still, it's probably not a
> > > big deal.
> > 
> > I am not sure, what the appropriate way to do this is. I can split it up
> > (again).
> 
> I think it would make sense to move them to their respective subsystems.

I agree. While looking at integrating this for Tegra DRM, I came across
the issue that if I build DRM as a module, linking with this code will
fail. The reason for that was that it was that the code, itself builtin,
uses drm_mode_set_name(), which would be exported by the drm module. So
I had to modifiy the Kconfig entries to be "def_tristate DRM". That
obviously isn't very nice since the code can also be used without DRM.

Moving the subsystem specific conversion routines to the respective
subsystems should solve any of these issues.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]