| Message ID | 1404033022-3014-1-git-send-email-wsa@the-dreams.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d6460c2e17ec2a051e95e9cdade968f0986c82ff |
| Headers | show |
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 linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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!
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
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
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)?
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.
> > > 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.
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!
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.
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.
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 linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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