diff mbox

dma: sh: inherit debug options from the subsystem for sh

Message ID 1404033022-3014-1-git-send-email-wsa@the-dreams.de (mailing list archive)
State Rejected
Delegated to: Vinod Koul
Headers show

Commit Message

Wolfram Sang June 29, 2014, 9:10 a.m. UTC
From: Wolfram Sang <wsa+renesas@sang-engineering.com>

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/dma/sh/Makefile | 3 +++
 1 file changed, 3 insertions(+)

Comments

Geert Uytterhoeven June 29, 2014, 9:32 a.m. UTC | #1
Hi Wolfram,

On Sun, Jun 29, 2014 at 11:10 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
> --- a/drivers/dma/sh/Makefile
> +++ b/drivers/dma/sh/Makefile
> @@ -1,3 +1,6 @@
> +ccflags-$(CONFIG_DMADEVICES_DEBUG)  := -DDEBUG
> +ccflags-$(CONFIG_DMADEVICES_VDEBUG) += -DVERBOSE_DEBUG
> +
>  obj-$(CONFIG_SH_DMAE_BASE) += shdma-base.o shdma-of.o
>  obj-$(CONFIG_SH_DMAE) += shdma.o
>  shdma-y := shdmac.o

Thanks!

There are a few more subdirectories that could benefit from a similar change.
I'm wondering whether this can be fixed at the drivers/dma/Makefile level,
as these config options were clearly intended to apply to all DMA drivers?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Laurent Pinchart June 29, 2014, 7:56 p.m. UTC | #2
Hi Wolfram,

Thank you for the patch.

On Sunday 29 June 2014 11:10:05 Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/dma/sh/Makefile | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/dma/sh/Makefile b/drivers/dma/sh/Makefile
> index 1ce88b28cfc6..275297a89bd5 100644
> --- a/drivers/dma/sh/Makefile
> +++ b/drivers/dma/sh/Makefile
> @@ -1,3 +1,6 @@
> +ccflags-$(CONFIG_DMADEVICES_DEBUG)  := -DDEBUG

Isn't this discouraged in favour of using dynamic printk ? I've recently 
submitted a similar patch for the OMAP4 ISS driver and it got nacked.

> +ccflags-$(CONFIG_DMADEVICES_VDEBUG) += -DVERBOSE_DEBUG
> +
>  obj-$(CONFIG_SH_DMAE_BASE) += shdma-base.o shdma-of.o
>  obj-$(CONFIG_SH_DMAE) += shdma.o
>  shdma-y := shdmac.o
Vinod Koul June 30, 2014, 1:50 p.m. UTC | #3
On Sun, Jun 29, 2014 at 09:56:43PM +0200, Laurent Pinchart wrote:
> Hi Wolfram,
> 
> Thank you for the patch.
> 
> On Sunday 29 June 2014 11:10:05 Wolfram Sang wrote:
> > From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > 
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > ---
> >  drivers/dma/sh/Makefile | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/dma/sh/Makefile b/drivers/dma/sh/Makefile
> > index 1ce88b28cfc6..275297a89bd5 100644
> > --- a/drivers/dma/sh/Makefile
> > +++ b/drivers/dma/sh/Makefile
> > @@ -1,3 +1,6 @@
> > +ccflags-$(CONFIG_DMADEVICES_DEBUG)  := -DDEBUG
> 
> Isn't this discouraged in favour of using dynamic printk ? I've recently 
> submitted a similar patch for the OMAP4 ISS driver and it got nacked.
With dynamic debug, these flags dont make much sense unless you want something
to be printed *always* during boot.

unforuntely, this patch doesnt provide reason why we should do this here!
Wolfram Sang July 3, 2014, 8:48 a.m. UTC | #4
Hi,

> With dynamic debug, these flags dont make much sense unless you want something
> to be printed *always* during boot.

Yes, this was exactly what I wanted. So, for consistency reasons I
copied the existing mechanism over to sh. If it turns out that the
desired default is something different, I'll send patches for that.

Thanks,

   Wolfram
Vinod Koul July 3, 2014, 9:02 a.m. UTC | #5
On Thu, Jul 03, 2014 at 10:48:34AM +0200, Wolfram Sang wrote:
> Hi,
> 
> > With dynamic debug, these flags dont make much sense unless you want something
> > to be printed *always* during boot.
> 
> Yes, this was exactly what I wanted. So, for consistency reasons I
> copied the existing mechanism over to sh. If it turns out that the
> desired default is something different, I'll send patches for that.

Ah good, and while at it, Please *do* mention this in changelog!

also fix the subsystem name to dmaengine in patch title
Wolfram Sang July 10, 2014, 11:56 a.m. UTC | #6
On Thu, Jul 03, 2014 at 02:32:25PM +0530, Vinod Koul wrote:
> On Thu, Jul 03, 2014 at 10:48:34AM +0200, Wolfram Sang wrote:
> > Hi,
> > 
> > > With dynamic debug, these flags dont make much sense unless you want something
> > > to be printed *always* during boot.
> > 
> > Yes, this was exactly what I wanted. So, for consistency reasons I
> > copied the existing mechanism over to sh. If it turns out that the
> > desired default is something different, I'll send patches for that.
> 
> Ah good, and while at it, Please *do* mention this in changelog!
> 
> also fix the subsystem name to dmaengine in patch title

So, I assume you are okay with the change now that I gave the reason?
But should I fix the other subdirs, too? Or keep the change minimal
(= only for sh)?
Laurent Pinchart July 10, 2014, 12:25 p.m. UTC | #7
Hi Wolfram,

On Thursday 03 July 2014 10:48:34 Wolfram Sang wrote:
> Hi,
> 
> > With dynamic debug, these flags dont make much sense unless you want
> > something to be printed *always* during boot.
> 
> Yes, this was exactly what I wanted.

Why is that ? My understanding is that the purpose of the 
CONFIG_DMADEVICES_DEBUG option is to enable debug messages for debugging. As 
it requires recompiling the kernel, wouldn't dynamic printk be a better 
alternative ?

> So, for consistency reasons I copied the existing mechanism over to sh. If
> it turns out that the desired default is something different, I'll send
> patches for that.
Wolfram Sang July 10, 2014, 12:48 p.m. UTC | #8
> > > With dynamic debug, these flags dont make much sense unless you want
> > > something to be printed *always* during boot.
> > 
> > Yes, this was exactly what I wanted.
> 
> Why is that ? My understanding is that the purpose of the 
> CONFIG_DMADEVICES_DEBUG option is to enable debug messages for debugging. As 
> it requires recompiling the kernel, wouldn't dynamic printk be a better 
> alternative ?

I didn't mind recompiling once for the prize of a smaller kernel image
(only the debug strings I wanted are included).

But let's just drop this patch, it is not worth all this discussion.
Vinod Koul July 10, 2014, 3:09 p.m. UTC | #9
On Thu, Jul 10, 2014 at 02:25:58PM +0200, Laurent Pinchart wrote:
> Hi Wolfram,
> 
> On Thursday 03 July 2014 10:48:34 Wolfram Sang wrote:
> > Hi,
> > 
> > > With dynamic debug, these flags dont make much sense unless you want
> > > something to be printed *always* during boot.
> > 
> > Yes, this was exactly what I wanted.
> 
> Why is that ? My understanding is that the purpose of the 
> CONFIG_DMADEVICES_DEBUG option is to enable debug messages for debugging. As 
> it requires recompiling the kernel, wouldn't dynamic printk be a better 
> alternative ?
No it wont. Pls see the discussion.

Lots of dmanegine device gets used at boot, so for testing those scenarios it
will help to enable debug!
Vinod Koul July 10, 2014, 3:10 p.m. UTC | #10
On Thu, Jul 10, 2014 at 01:56:48PM +0200, Wolfram Sang wrote:
> On Thu, Jul 03, 2014 at 02:32:25PM +0530, Vinod Koul wrote:
> > On Thu, Jul 03, 2014 at 10:48:34AM +0200, Wolfram Sang wrote:
> > > Hi,
> > > 
> > > > With dynamic debug, these flags dont make much sense unless you want something
> > > > to be printed *always* during boot.
> > > 
> > > Yes, this was exactly what I wanted. So, for consistency reasons I
> > > copied the existing mechanism over to sh. If it turns out that the
> > > desired default is something different, I'll send patches for that.
> > 
> > Ah good, and while at it, Please *do* mention this in changelog!
> > 
> > also fix the subsystem name to dmaengine in patch title
> 
> So, I assume you are okay with the change now that I gave the reason?
> But should I fix the other subdirs, too? Or keep the change minimal
> (= only for sh)?

Yes with a right reason :)

And yes makes sense to do so for whole subsystem.
Laurent Pinchart July 10, 2014, 3:42 p.m. UTC | #11
Hi Vinod,

On Thursday 10 July 2014 20:39:29 Vinod Koul wrote:
> On Thu, Jul 10, 2014 at 02:25:58PM +0200, Laurent Pinchart wrote:
> > On Thursday 03 July 2014 10:48:34 Wolfram Sang wrote:
> > > Hi,
> > > 
> > > > With dynamic debug, these flags dont make much sense unless you want
> > > > something to be printed *always* during boot.
> > > 
> > > Yes, this was exactly what I wanted.
> > 
> > Why is that ? My understanding is that the purpose of the
> > CONFIG_DMADEVICES_DEBUG option is to enable debug messages for debugging.
> > As it requires recompiling the kernel, wouldn't dynamic printk be a
> > better alternative ?
> 
> No it wont. Pls see the discussion.
> 
> Lots of dmanegine device gets used at boot, so for testing those scenarios
> it will help to enable debug!

OK, that's the point I've missed. I agree with the patch then.
Simon Horman July 11, 2014, 8:32 a.m. UTC | #12
On Thu, Jul 10, 2014 at 05:42:13PM +0200, Laurent Pinchart wrote:
> Hi Vinod,
> 
> On Thursday 10 July 2014 20:39:29 Vinod Koul wrote:
> > On Thu, Jul 10, 2014 at 02:25:58PM +0200, Laurent Pinchart wrote:
> > > On Thursday 03 July 2014 10:48:34 Wolfram Sang wrote:
> > > > Hi,
> > > > 
> > > > > With dynamic debug, these flags dont make much sense unless you want
> > > > > something to be printed *always* during boot.
> > > > 
> > > > Yes, this was exactly what I wanted.
> > > 
> > > Why is that ? My understanding is that the purpose of the
> > > CONFIG_DMADEVICES_DEBUG option is to enable debug messages for debugging.
> > > As it requires recompiling the kernel, wouldn't dynamic printk be a
> > > better alternative ?
> > 
> > No it wont. Pls see the discussion.
> > 
> > Lots of dmanegine device gets used at boot, so for testing those scenarios
> > it will help to enable debug!
> 
> OK, that's the point I've missed. I agree with the patch then.

Hi,

in order to try and create a smoother path for changes in this area to land
I have pushed them to the shdma-for-v3.17 branch of my renesas tree
on kernel.org. It is merged into the devel and next branches of that tree
and should appear in linux-next in the near future.

My intention is to send a pull-request for this branch once it has sat
in next for a short time. I hope that this approach is useful to all parties.

--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/dma/sh/Makefile b/drivers/dma/sh/Makefile
index 1ce88b28cfc6..275297a89bd5 100644
--- a/drivers/dma/sh/Makefile
+++ b/drivers/dma/sh/Makefile
@@ -1,3 +1,6 @@ 
+ccflags-$(CONFIG_DMADEVICES_DEBUG)  := -DDEBUG
+ccflags-$(CONFIG_DMADEVICES_VDEBUG) += -DVERBOSE_DEBUG
+
 obj-$(CONFIG_SH_DMAE_BASE) += shdma-base.o shdma-of.o
 obj-$(CONFIG_SH_DMAE) += shdma.o
 shdma-y := shdmac.o