[PATCH] dma: sh: inherit debug options from the subsystem for sh

[PATCH] dma: sh: inherit debug options from the subsystem for sh

[Wolfram Sang]

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
+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
-- 
2.0.0

Re: [PATCH] dma: sh: inherit debug options from the subsystem for sh

[Geert Uytterhoeven]

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

Re: [PATCH] dma: sh: inherit debug options from the subsystem for sh

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart

Re: [PATCH] dma: sh: inherit debug options from the subsystem for sh

[Vinod Koul]

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!

-- 
~Vinod

> 
> > +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
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 

Re: [PATCH] dma: sh: inherit debug options from the subsystem for sh

[Wolfram Sang]

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

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


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

Re: [PATCH] dma: sh: inherit debug options from the subsystem for sh

[Vinod Koul]

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

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

-- 
~Vinod

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

Re: [PATCH] dma: sh: inherit debug options from the subsystem for sh

[Wolfram Sang]

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

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)?


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

Re: [PATCH] dma: sh: inherit debug options from the subsystem for sh

[Laurent Pinchart]

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

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.

-- 
Regards,

Laurent Pinchart

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] dma: sh: inherit debug options from the subsystem for sh

[Wolfram Sang]

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


> > > 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.


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

Re: [PATCH] dma: sh: inherit debug options from the subsystem for sh

[Vinod Koul]

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

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


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

Re: [PATCH] dma: sh: inherit debug options from the subsystem for sh

[Vinod Koul]

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

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.

-- 
~Vinod

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

Re: [PATCH] dma: sh: inherit debug options from the subsystem for sh

[Laurent Pinchart]

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

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.

-- 
Regards,

Laurent Pinchart

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] dma: sh: inherit debug options from the subsystem for sh

[Simon Horman]

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.