Hi Mauro,
Thank you for the review.
On Wednesday 05 March 2014 17:10:06 Mauro Carvalho Chehab wrote:
> Em Tue, 11 Feb 2014 13:38:51 +0100 Laurent Pinchart escreveu:
> > On Tuesday 11 February 2014 12:17:01 Paul Bolle wrote:
> > > Commit d632dfefd36f ("[media] v4l: omap4iss: Add support for OMAP4
> > > camera interface - Build system") added a Kconfig entry for
> > > VIDEO_OMAP4_DEBUG. But nothing uses that symbol.
> > >
> > > This entry was apparently copied from a similar entry for "OMAP 3
> > > Camera debug messages". The OMAP 3 entry is used to set the DEBUG
> > > compiler flag, which enables calls of dev_dbg().
> > >
> > > So add a Makefile line to do that for omap4iss too.
> > >
> > > Signed-off-by: Paul Bolle <pebo...@tiscali.nl>
> >
> > Acked-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> >
> > and applied to my tree.
> >
> > > ---
> > > 0) v1 was called "[media] v4l: omap4iss: Remove VIDEO_OMAP4_DEBUG". This
> > > versions implements Laurent's alternative (which is much better).
> >
> > Thanks :-)
> >
> > > 1) Still untested.
> > >
> > > drivers/staging/media/omap4iss/Makefile | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/staging/media/omap4iss/Makefile
> > > b/drivers/staging/media/omap4iss/Makefile index a716ce9..f46c6bd 100644
> > > --- a/drivers/staging/media/omap4iss/Makefile
> > > +++ b/drivers/staging/media/omap4iss/Makefile
> > > @@ -1,5 +1,7 @@
> > >
> > > # Makefile for OMAP4 ISS driver
> > >
> > > +ccflags-$(CONFIG_VIDEO_OMAP4_DEBUG) += -DDEBUG
> > > +
>
> This seems to be a very bad idea on my eyes. It is just creating an alias to
> an already existing Kconfig symbol with no good reason.
>
> To be fair, there are also two other drivers doing this:
>
>
drivers/media/platform/omap3isp/Makefile:ccflags-$(CONFIG_VIDEO_OMAP3_DEBUG
> ) += -DDEBUG
> drivers/media/platform/ti-vpe/Makefile:ccflags-$(CONFIG_VIDEO_TI_VPE_DEBUG)
> += -DDEBUG
>
> It sounds better do to a s/CONFIG_DEBUG/CONFIG_VIDEO_whatever_DEBUG/
> inside each of those driver, or to just remove this, and document that
> one should enable CONFIG_DEBUG if they want to debug v4l2 drivers.
Please note that -DDEBUG is equivalent to '#define DEBUG', not to '#define
CONFIG_DEBUG'. 'DEBUG' needs to be defined for dev_dbg() to have any effect.
Furthermore, there's not CONFIG_DEBUG as far as I know.
An alternative to this would be to add
#ifdef CONFIG_VIDEO_OMAP3_DEBUG
#define DEBUG
#endif
at the beginning of each source file of the driver. That doesn't seem very
practical to me though.
> > > omap4-iss-objs += \
> > >
> > > iss.o iss_csi2.o iss_csiphy.o iss_ipipeif.o iss_ipipe.o iss_resizer.o
> > >
> > > iss_video.o
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
