Re: [PATCH v2] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/media

[Mauro Carvalho Chehab]

Em Wed, 12 Feb 2014 10:11:54 +0100
Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> escreveu:

> Hi Mauro,
> 
> Am Mittwoch, den 12.02.2014, 06:53 +0900 schrieb Mauro Carvalho Chehab:
> [...]
> > > diff --git a/include/media/of_graph.h b/include/media/of_graph.h
> > > new file mode 100644
> > > index 0000000..3bbeb60
> > > --- /dev/null
> > > +++ b/include/media/of_graph.h
> > > @@ -0,0 +1,46 @@
> > > +/*
> > > + * OF graph binding parsing helpers
> > > + *
> > > + * Copyright (C) 2012 - 2013 Samsung Electronics Co., Ltd.
> > > + * Author: Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>
> > > + *
> > > + * Copyright (C) 2012 Renesas Electronics Corp.
> > > + * Author: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of version 2 of the GNU General Public License as
> > > + * published by the Free Software Foundation.
> > > + */
> > > +#ifndef __LINUX_OF_GRAPH_H
> > > +#define __LINUX_OF_GRAPH_H
> > > +
> > > +#ifdef CONFIG_OF
> > 
> > As a matter of consistency, it would be better to test here for
> > CONFIG_OF_GRAPH instead, to reflect the same symbol that enables such
> > functions as used on Kconfig/Makefile.
> 
> Maybe I'm trying to be too clever for my own good, but my reasoning was
> as follows:
> 
> Suppose I newly use the of_graph_ helpers in a subsystem that does not
> yet select OF_GRAPH. In that case I'd rather get linking errors earlier
> rather than stubbed out functions that silently fail to parse the DT
> later.

I see your point, but, imagining that someone pushed a patch using those
symbols upstream, that would break compilation and git bisection, with 
will hurt everyone, and not only the very few of us that would actually
need the OF_GRAPH symbol for an specific driver.

Also, such push would mean that someone forgot to do his homework and
to test if the committed functionality is actually working.

So, it seems more fair that the one that did the mistake will be the one
that will suffer the consequences for his errors instead of applying a
penalty to everybody's else ;)

> Since there is
> config VIDEO_DEV
> 	select OF_GRAPH if OF
> already and the same should be added for other users of device tree
> graphs, I think stubbing out the functions only if OF is disabled should
> be enough.

Well, if you want to be sure that the graph will always be there if OF, then
you could do, instead:

config OF_GRAPH
	bool
	default OF

(that would actually make OF_GRAPH just an alias to OF - so we could just
use OF instead).

In any case, I think that we should use the same config name at Makefile, 
Kconfig and of_graph.h (either OF_GRAPH or just OF).

Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/