Re: [PATCH v2] ALSA: korg1212: cleanup of printk
On Wed, 2014-11-26 at 16:19 +0530, Sudip Mukherjee wrote: > On Tue, Nov 25, 2014 at 10:59:25AM -0800, Joe Perches wrote: > > On Tue, 2014-11-25 at 11:51 +0530, Sudip Mukherjee wrote: > > > problems with this way: > > > 1) macro name is little misleading - macro says printk, but we are using > > > dev_dbg > > > 2) if some one later wants to add something to this file, and doesnot > > > want to use the variable name korg1212 in his function. > > > > > > any suggestions ? > > > > Some code is so old and unlikely to be ever used > > again, it may be better to move it to an "ancient > > and crufty" folder and forget about it. > > > > This may be one of those. > > > > If the debugging was written in a more current style, > > it might look like this: (with various format/argument > > mismatches fixed, formatting changes, etc) > > ok. and can't we delete some of the messages like "DSP download is > complete." and then printing the statename. there are lots of messages > like that which is just printing the function name and the > statename .. Dunno. Maybe somebody cares about the state of the dsp download, but I doubt it as I suspect the hardware isn't used much. > and, even if i modify it a little and send it, most of the work has > been done by you. so how do i send the patch in your name or add your > name in the patch ? I have not done anything so my name should not be > there. you started it. I spend a couple minutes running a couple scripts. I don't have the hardware, I've never looked at that code before, do what you think appropriate. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] ALSA: korg1212: cleanup of printk
On Tue, Nov 25, 2014 at 10:59:25AM -0800, Joe Perches wrote:
> On Tue, 2014-11-25 at 11:51 +0530, Sudip Mukherjee wrote:
> > problems with this way:
> > 1) macro name is little misleading - macro says printk, but we are using
> > dev_dbg
> > 2) if some one later wants to add something to this file, and doesnot want
> > to use the variable name korg1212 in his function.
> >
> > any suggestions ?
>
> Some code is so old and unlikely to be ever used
> again, it may be better to move it to an "ancient
> and crufty" folder and forget about it.
>
> This may be one of those.
>
> If the debugging was written in a more current style,
> it might look like this: (with various format/argument
> mismatches fixed, formatting changes, etc)
ok. and can't we delete some of the messages like "DSP download is complete."
and then printing the statename. there are lots of messages like that which is
just printing the function name and the statename ..
and, even if i modify it a little and send it, most of the work has been done
by you. so how do i send the patch in your name or add your name in the patch ?
I have not done anything so my name should not be there.
thanks
sudip
>
> I don't have this card (does anyone?), so it's compiled
> but untested.
> ---
> sound/pci/korg1212/korg1212.c | 363
> +-
> 1 file changed, 184 insertions(+), 179 deletions(-)
>
> diff --git a/sound/pci/korg1212/korg1212.c b/sound/pci/korg1212/korg1212.c
> index 59d21c9..9dd7f96 100644
> --- a/sound/pci/korg1212/korg1212.c
> +++ b/sound/pci/korg1212/korg1212.c
> @@ -38,20 +38,9 @@
>
> #include
>
> -//
>
> -// Debug Stuff
> -//
>
> -#define K1212_DEBUG_LEVEL0
> -#if K1212_DEBUG_LEVEL > 0
> -#define K1212_DEBUG_PRINTK(fmt,args...) printk(KERN_DEBUG fmt,##args)
> -#else
> -#define K1212_DEBUG_PRINTK(fmt,...)
> -#endif
> -#if K1212_DEBUG_LEVEL > 1
> -#define K1212_DEBUG_PRINTK_VERBOSE(fmt,args...) printk(KERN_DEBUG
> fmt,##args)
> -#else
> -#define K1212_DEBUG_PRINTK_VERBOSE(fmt,...)
> -#endif
> +/* #define K1212_DEBUG_MEMORY_RANGES */
> +#define k1212_dbg(k1212, fmt, ...) \
> + dev_dbg((k1212)->card->dev, fmt, ##__VA_ARGS__)
>
> //
>
> // Record/Play Buffer Allocation Method. If K1212_LARGEALLOC is defined all
> @@ -530,12 +519,12 @@ static int snd_korg1212_Send1212Command(struct
> snd_korg1212 *korg1212,
> int rc = K1212_CMDRET_Success;
>
> if (!korg1212->outDoorbellPtr) {
> - K1212_DEBUG_PRINTK_VERBOSE("K1212_DEBUG: CardUninitialized\n");
> + k1212_dbg(korg1212, "CardUninitialized\n");
> return K1212_CMDRET_CardUninitialized;
> }
>
> - K1212_DEBUG_PRINTK("K1212_DEBUG: Card <- 0x%08x 0x%08x [%s]\n",
> -doorbellVal, mailBox0Val,
> stateName[korg1212->cardState]);
> + k1212_dbg(korg1212, "Card <- 0x%08x 0x%08x [%s]\n",
> + doorbellVal, mailBox0Val, stateName[korg1212->cardState]);
> for (retryCount = 0; retryCount < MAX_COMMAND_RETRIES; retryCount++)
> {
> writel(mailBox3Val, korg1212->mailbox3Ptr);
> writel(mailBox2Val, korg1212->mailbox2Ptr);
> @@ -562,7 +551,7 @@ static int snd_korg1212_Send1212Command(struct
> snd_korg1212 *korg1212,
> mailBox3Lo = readl(korg1212->mailbox3Ptr);
> if (mailBox3Lo & COMMAND_ACK_MASK) {
> if ((mailBox3Lo & DOORBELL_VAL_MASK) == (doorbellVal &
> DOORBELL_VAL_MASK)) {
> - K1212_DEBUG_PRINTK_VERBOSE("K1212_DEBUG: Card
> <- Success\n");
> + k1212_dbg(korg1212, "Card <- Success\n");
> rc = K1212_CMDRET_Success;
> break;
> }
> @@ -571,7 +560,7 @@ static int snd_korg1212_Send1212Command(struct
> snd_korg1212 *korg1212,
> korg1212->cmdRetryCount += retryCount;
>
> if (retryCount >= MAX_COMMAND_RETRIES) {
> - K1212_DEBUG_PRINTK_VERBOSE("K1212_DEBUG: Card <-
> NoAckFromCard\n");
> + k1212_dbg(korg1212, "Card <- NoAckFromCard\n");
> rc = K1212_CMDRET_NoAckFromCard;
> }
>
> @@ -612,8 +601,8 @@ static void snd_korg1212_timer_func(unsigned long data)
> korg1212->stop_pending_cnt = 0;
> korg1212->dsp_stop_is_processed = 1;
> wake_up(>wait);
> - K1212_DEBUG_PRINTK_VERBOSE("K1212_DEBUG: Stop ack'ed [%s]\n",
> -stateName[korg1212->cardState]);
> + k1212_dbg(korg1212, "Stop ack'ed [%s]\n",
> + stateName[korg1212->cardState]);
> } else {
>
Re: [PATCH v2] ALSA: korg1212: cleanup of printk
On Tue, Nov 25, 2014 at 10:59:25AM -0800, Joe Perches wrote:
On Tue, 2014-11-25 at 11:51 +0530, Sudip Mukherjee wrote:
problems with this way:
1) macro name is little misleading - macro says printk, but we are using
dev_dbg
2) if some one later wants to add something to this file, and doesnot want
to use the variable name korg1212 in his function.
any suggestions ?
Some code is so old and unlikely to be ever used
again, it may be better to move it to an ancient
and crufty folder and forget about it.
This may be one of those.
If the debugging was written in a more current style,
it might look like this: (with various format/argument
mismatches fixed, formatting changes, etc)
ok. and can't we delete some of the messages like DSP download is complete.
and then printing the statename. there are lots of messages like that which is
just printing the function name and the statename ..
and, even if i modify it a little and send it, most of the work has been done
by you. so how do i send the patch in your name or add your name in the patch ?
I have not done anything so my name should not be there.
thanks
sudip
I don't have this card (does anyone?), so it's compiled
but untested.
---
sound/pci/korg1212/korg1212.c | 363
+-
1 file changed, 184 insertions(+), 179 deletions(-)
diff --git a/sound/pci/korg1212/korg1212.c b/sound/pci/korg1212/korg1212.c
index 59d21c9..9dd7f96 100644
--- a/sound/pci/korg1212/korg1212.c
+++ b/sound/pci/korg1212/korg1212.c
@@ -38,20 +38,9 @@
#include asm/io.h
-//
-// Debug Stuff
-//
-#define K1212_DEBUG_LEVEL0
-#if K1212_DEBUG_LEVEL 0
-#define K1212_DEBUG_PRINTK(fmt,args...) printk(KERN_DEBUG fmt,##args)
-#else
-#define K1212_DEBUG_PRINTK(fmt,...)
-#endif
-#if K1212_DEBUG_LEVEL 1
-#define K1212_DEBUG_PRINTK_VERBOSE(fmt,args...) printk(KERN_DEBUG
fmt,##args)
-#else
-#define K1212_DEBUG_PRINTK_VERBOSE(fmt,...)
-#endif
+/* #define K1212_DEBUG_MEMORY_RANGES */
+#define k1212_dbg(k1212, fmt, ...) \
+ dev_dbg((k1212)-card-dev, fmt, ##__VA_ARGS__)
//
// Record/Play Buffer Allocation Method. If K1212_LARGEALLOC is defined all
@@ -530,12 +519,12 @@ static int snd_korg1212_Send1212Command(struct
snd_korg1212 *korg1212,
int rc = K1212_CMDRET_Success;
if (!korg1212-outDoorbellPtr) {
- K1212_DEBUG_PRINTK_VERBOSE(K1212_DEBUG: CardUninitialized\n);
+ k1212_dbg(korg1212, CardUninitialized\n);
return K1212_CMDRET_CardUninitialized;
}
- K1212_DEBUG_PRINTK(K1212_DEBUG: Card - 0x%08x 0x%08x [%s]\n,
-doorbellVal, mailBox0Val,
stateName[korg1212-cardState]);
+ k1212_dbg(korg1212, Card - 0x%08x 0x%08x [%s]\n,
+ doorbellVal, mailBox0Val, stateName[korg1212-cardState]);
for (retryCount = 0; retryCount MAX_COMMAND_RETRIES; retryCount++)
{
writel(mailBox3Val, korg1212-mailbox3Ptr);
writel(mailBox2Val, korg1212-mailbox2Ptr);
@@ -562,7 +551,7 @@ static int snd_korg1212_Send1212Command(struct
snd_korg1212 *korg1212,
mailBox3Lo = readl(korg1212-mailbox3Ptr);
if (mailBox3Lo COMMAND_ACK_MASK) {
if ((mailBox3Lo DOORBELL_VAL_MASK) == (doorbellVal
DOORBELL_VAL_MASK)) {
- K1212_DEBUG_PRINTK_VERBOSE(K1212_DEBUG: Card
- Success\n);
+ k1212_dbg(korg1212, Card - Success\n);
rc = K1212_CMDRET_Success;
break;
}
@@ -571,7 +560,7 @@ static int snd_korg1212_Send1212Command(struct
snd_korg1212 *korg1212,
korg1212-cmdRetryCount += retryCount;
if (retryCount = MAX_COMMAND_RETRIES) {
- K1212_DEBUG_PRINTK_VERBOSE(K1212_DEBUG: Card -
NoAckFromCard\n);
+ k1212_dbg(korg1212, Card - NoAckFromCard\n);
rc = K1212_CMDRET_NoAckFromCard;
}
@@ -612,8 +601,8 @@ static void snd_korg1212_timer_func(unsigned long data)
korg1212-stop_pending_cnt = 0;
korg1212-dsp_stop_is_processed = 1;
wake_up(korg1212-wait);
- K1212_DEBUG_PRINTK_VERBOSE(K1212_DEBUG: Stop ack'ed [%s]\n,
-stateName[korg1212-cardState]);
+ k1212_dbg(korg1212, Stop ack'ed [%s]\n,
+ stateName[korg1212-cardState]);
} else {
if (--korg1212-stop_pending_cnt 0) {
/* reprogram timer */
@@ -624,8 +613,8 @@ static void snd_korg1212_timer_func(unsigned
Re: [PATCH v2] ALSA: korg1212: cleanup of printk
On Wed, 2014-11-26 at 16:19 +0530, Sudip Mukherjee wrote: On Tue, Nov 25, 2014 at 10:59:25AM -0800, Joe Perches wrote: On Tue, 2014-11-25 at 11:51 +0530, Sudip Mukherjee wrote: problems with this way: 1) macro name is little misleading - macro says printk, but we are using dev_dbg 2) if some one later wants to add something to this file, and doesnot want to use the variable name korg1212 in his function. any suggestions ? Some code is so old and unlikely to be ever used again, it may be better to move it to an ancient and crufty folder and forget about it. This may be one of those. If the debugging was written in a more current style, it might look like this: (with various format/argument mismatches fixed, formatting changes, etc) ok. and can't we delete some of the messages like DSP download is complete. and then printing the statename. there are lots of messages like that which is just printing the function name and the statename .. Dunno. Maybe somebody cares about the state of the dsp download, but I doubt it as I suspect the hardware isn't used much. and, even if i modify it a little and send it, most of the work has been done by you. so how do i send the patch in your name or add your name in the patch ? I have not done anything so my name should not be there. shrug you started it. I spend a couple minutes running a couple scripts. I don't have the hardware, I've never looked at that code before, do what you think appropriate. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] ALSA: korg1212: cleanup of printk
On Tue, 2014-11-25 at 11:51 +0530, Sudip Mukherjee wrote:
> problems with this way:
> 1) macro name is little misleading - macro says printk, but we are using
> dev_dbg
> 2) if some one later wants to add something to this file, and doesnot want to
> use the variable name korg1212 in his function.
>
> any suggestions ?
Some code is so old and unlikely to be ever used
again, it may be better to move it to an "ancient
and crufty" folder and forget about it.
This may be one of those.
If the debugging was written in a more current style,
it might look like this: (with various format/argument
mismatches fixed, formatting changes, etc)
I don't have this card (does anyone?), so it's compiled
but untested.
---
sound/pci/korg1212/korg1212.c | 363 +-
1 file changed, 184 insertions(+), 179 deletions(-)
diff --git a/sound/pci/korg1212/korg1212.c b/sound/pci/korg1212/korg1212.c
index 59d21c9..9dd7f96 100644
--- a/sound/pci/korg1212/korg1212.c
+++ b/sound/pci/korg1212/korg1212.c
@@ -38,20 +38,9 @@
#include
-//
-// Debug Stuff
-//
-#define K1212_DEBUG_LEVEL 0
-#if K1212_DEBUG_LEVEL > 0
-#define K1212_DEBUG_PRINTK(fmt,args...)printk(KERN_DEBUG fmt,##args)
-#else
-#define K1212_DEBUG_PRINTK(fmt,...)
-#endif
-#if K1212_DEBUG_LEVEL > 1
-#define K1212_DEBUG_PRINTK_VERBOSE(fmt,args...)printk(KERN_DEBUG
fmt,##args)
-#else
-#define K1212_DEBUG_PRINTK_VERBOSE(fmt,...)
-#endif
+/* #define K1212_DEBUG_MEMORY_RANGES */
+#define k1212_dbg(k1212, fmt, ...) \
+ dev_dbg((k1212)->card->dev, fmt, ##__VA_ARGS__)
//
// Record/Play Buffer Allocation Method. If K1212_LARGEALLOC is defined all
@@ -530,12 +519,12 @@ static int snd_korg1212_Send1212Command(struct
snd_korg1212 *korg1212,
int rc = K1212_CMDRET_Success;
if (!korg1212->outDoorbellPtr) {
- K1212_DEBUG_PRINTK_VERBOSE("K1212_DEBUG: CardUninitialized\n");
+ k1212_dbg(korg1212, "CardUninitialized\n");
return K1212_CMDRET_CardUninitialized;
}
- K1212_DEBUG_PRINTK("K1212_DEBUG: Card <- 0x%08x 0x%08x [%s]\n",
- doorbellVal, mailBox0Val,
stateName[korg1212->cardState]);
+ k1212_dbg(korg1212, "Card <- 0x%08x 0x%08x [%s]\n",
+ doorbellVal, mailBox0Val, stateName[korg1212->cardState]);
for (retryCount = 0; retryCount < MAX_COMMAND_RETRIES; retryCount++) {
writel(mailBox3Val, korg1212->mailbox3Ptr);
writel(mailBox2Val, korg1212->mailbox2Ptr);
@@ -562,7 +551,7 @@ static int snd_korg1212_Send1212Command(struct snd_korg1212
*korg1212,
mailBox3Lo = readl(korg1212->mailbox3Ptr);
if (mailBox3Lo & COMMAND_ACK_MASK) {
if ((mailBox3Lo & DOORBELL_VAL_MASK) == (doorbellVal &
DOORBELL_VAL_MASK)) {
- K1212_DEBUG_PRINTK_VERBOSE("K1212_DEBUG: Card
<- Success\n");
+ k1212_dbg(korg1212, "Card <- Success\n");
rc = K1212_CMDRET_Success;
break;
}
@@ -571,7 +560,7 @@ static int snd_korg1212_Send1212Command(struct snd_korg1212
*korg1212,
korg1212->cmdRetryCount += retryCount;
if (retryCount >= MAX_COMMAND_RETRIES) {
- K1212_DEBUG_PRINTK_VERBOSE("K1212_DEBUG: Card <-
NoAckFromCard\n");
+ k1212_dbg(korg1212, "Card <- NoAckFromCard\n");
rc = K1212_CMDRET_NoAckFromCard;
}
@@ -612,8 +601,8 @@ static void snd_korg1212_timer_func(unsigned long data)
korg1212->stop_pending_cnt = 0;
korg1212->dsp_stop_is_processed = 1;
wake_up(>wait);
- K1212_DEBUG_PRINTK_VERBOSE("K1212_DEBUG: Stop ack'ed [%s]\n",
- stateName[korg1212->cardState]);
+ k1212_dbg(korg1212, "Stop ack'ed [%s]\n",
+ stateName[korg1212->cardState]);
} else {
if (--korg1212->stop_pending_cnt > 0) {
/* reprogram timer */
@@ -624,8 +613,8 @@ static void snd_korg1212_timer_func(unsigned long data)
korg1212->sharedBufferPtr->cardCommand = 0;
korg1212->dsp_stop_is_processed = 1;
wake_up(>wait);
- K1212_DEBUG_PRINTK("K1212_DEBUG: Stop timeout [%s]\n",
- stateName[korg1212->cardState]);
+ k1212_dbg(korg1212, "Stop timeout [%s]\n",
+ stateName[korg1212->cardState]);
}
}
Re: [PATCH v2] ALSA: korg1212: cleanup of printk
On Tue, 2014-11-25 at 11:51 +0530, Sudip Mukherjee wrote:
problems with this way:
1) macro name is little misleading - macro says printk, but we are using
dev_dbg
2) if some one later wants to add something to this file, and doesnot want to
use the variable name korg1212 in his function.
any suggestions ?
Some code is so old and unlikely to be ever used
again, it may be better to move it to an ancient
and crufty folder and forget about it.
This may be one of those.
If the debugging was written in a more current style,
it might look like this: (with various format/argument
mismatches fixed, formatting changes, etc)
I don't have this card (does anyone?), so it's compiled
but untested.
---
sound/pci/korg1212/korg1212.c | 363 +-
1 file changed, 184 insertions(+), 179 deletions(-)
diff --git a/sound/pci/korg1212/korg1212.c b/sound/pci/korg1212/korg1212.c
index 59d21c9..9dd7f96 100644
--- a/sound/pci/korg1212/korg1212.c
+++ b/sound/pci/korg1212/korg1212.c
@@ -38,20 +38,9 @@
#include asm/io.h
-//
-// Debug Stuff
-//
-#define K1212_DEBUG_LEVEL 0
-#if K1212_DEBUG_LEVEL 0
-#define K1212_DEBUG_PRINTK(fmt,args...)printk(KERN_DEBUG fmt,##args)
-#else
-#define K1212_DEBUG_PRINTK(fmt,...)
-#endif
-#if K1212_DEBUG_LEVEL 1
-#define K1212_DEBUG_PRINTK_VERBOSE(fmt,args...)printk(KERN_DEBUG
fmt,##args)
-#else
-#define K1212_DEBUG_PRINTK_VERBOSE(fmt,...)
-#endif
+/* #define K1212_DEBUG_MEMORY_RANGES */
+#define k1212_dbg(k1212, fmt, ...) \
+ dev_dbg((k1212)-card-dev, fmt, ##__VA_ARGS__)
//
// Record/Play Buffer Allocation Method. If K1212_LARGEALLOC is defined all
@@ -530,12 +519,12 @@ static int snd_korg1212_Send1212Command(struct
snd_korg1212 *korg1212,
int rc = K1212_CMDRET_Success;
if (!korg1212-outDoorbellPtr) {
- K1212_DEBUG_PRINTK_VERBOSE(K1212_DEBUG: CardUninitialized\n);
+ k1212_dbg(korg1212, CardUninitialized\n);
return K1212_CMDRET_CardUninitialized;
}
- K1212_DEBUG_PRINTK(K1212_DEBUG: Card - 0x%08x 0x%08x [%s]\n,
- doorbellVal, mailBox0Val,
stateName[korg1212-cardState]);
+ k1212_dbg(korg1212, Card - 0x%08x 0x%08x [%s]\n,
+ doorbellVal, mailBox0Val, stateName[korg1212-cardState]);
for (retryCount = 0; retryCount MAX_COMMAND_RETRIES; retryCount++) {
writel(mailBox3Val, korg1212-mailbox3Ptr);
writel(mailBox2Val, korg1212-mailbox2Ptr);
@@ -562,7 +551,7 @@ static int snd_korg1212_Send1212Command(struct snd_korg1212
*korg1212,
mailBox3Lo = readl(korg1212-mailbox3Ptr);
if (mailBox3Lo COMMAND_ACK_MASK) {
if ((mailBox3Lo DOORBELL_VAL_MASK) == (doorbellVal
DOORBELL_VAL_MASK)) {
- K1212_DEBUG_PRINTK_VERBOSE(K1212_DEBUG: Card
- Success\n);
+ k1212_dbg(korg1212, Card - Success\n);
rc = K1212_CMDRET_Success;
break;
}
@@ -571,7 +560,7 @@ static int snd_korg1212_Send1212Command(struct snd_korg1212
*korg1212,
korg1212-cmdRetryCount += retryCount;
if (retryCount = MAX_COMMAND_RETRIES) {
- K1212_DEBUG_PRINTK_VERBOSE(K1212_DEBUG: Card -
NoAckFromCard\n);
+ k1212_dbg(korg1212, Card - NoAckFromCard\n);
rc = K1212_CMDRET_NoAckFromCard;
}
@@ -612,8 +601,8 @@ static void snd_korg1212_timer_func(unsigned long data)
korg1212-stop_pending_cnt = 0;
korg1212-dsp_stop_is_processed = 1;
wake_up(korg1212-wait);
- K1212_DEBUG_PRINTK_VERBOSE(K1212_DEBUG: Stop ack'ed [%s]\n,
- stateName[korg1212-cardState]);
+ k1212_dbg(korg1212, Stop ack'ed [%s]\n,
+ stateName[korg1212-cardState]);
} else {
if (--korg1212-stop_pending_cnt 0) {
/* reprogram timer */
@@ -624,8 +613,8 @@ static void snd_korg1212_timer_func(unsigned long data)
korg1212-sharedBufferPtr-cardCommand = 0;
korg1212-dsp_stop_is_processed = 1;
wake_up(korg1212-wait);
- K1212_DEBUG_PRINTK(K1212_DEBUG: Stop timeout [%s]\n,
- stateName[korg1212-cardState]);
+ k1212_dbg(korg1212, Stop timeout [%s]\n,
+ stateName[korg1212-cardState]);
}
}
spin_unlock_irqrestore(korg1212-lock, flags);
@@
Re: [PATCH v2] ALSA: korg1212: cleanup of printk
On Mon, Nov 24, 2014 at 09:25:10AM -0800, Joe Perches wrote: > On Mon, 2014-11-24 at 18:08 +0100, Takashi Iwai wrote: > > At Sun, 23 Nov 2014 13:40:51 +0530, Sudip Mukherjee wrote: > [] > > > replaced all references of the debug messages via printk > > > with dev_* macro (mostly dev_dbg). > > > one reference was changed to pr_err as there the card might have been > > > uninitialized. > > > > > > this patch will generate warning from checkpatch about broken quoted > > > strings. but that was not fixed intentionally to improve the > > > readability. > > I think it'd be easier to read and grep coalesced. or maybe better will be individual dev_dbg calls > > [] > > > > in your review of v1, you said about some lines which are not ending > > > with \n. but i was not able to find them. did i miss them somewhere? > [] > > The problem is the one with multiple "\n", for example: > > > > dev_dbg(korg1212->card->dev, "dspMemPhy = %08x U[%08x], " > > "PlayDataPhy = %08x L[%08x]\n" > > "korg1212: RecDataPhy = %08x L[%08x], " > > "VolumeTablePhy = %08x L[%08x]\n" > > "korg1212: RoutingTablePhy = %08x L[%08x], " > > "AdatTimeCodePhy = %08x L[%08x]\n", > > I think these should be individual dev_dbg calls > > dev_dbg(korg1212->card->dev, "dspMemPhy = %08x U[%08x]\n", val, val2) > dev_dbg(korg1212->card->dev, "PhyDataPhy = %08x L[%08x]\n", val, val2); > dev_dbg(korg1212->card->dev, "RecDataPhy = %08x L[%08x]\n", val, val2); > dev_dbg(korg1212->card->dev, "VolumeTablePhy = %08x L[%08x]\n", val, > val2); > > etc.. > > Another possibility is to use another macro like: > > #define k1212_dbg(k1212, fmt, ...)\ > dev_dbg((k)->card->dev, fmt, ##__VA_ARGS__) > > and change all these to > > k1212_dbg(korg1212, "dspMemPhy = %08x U[%08x]\n", val, val2) > k1212_dbg(korg1212, "PhyDataPhy = %08x L[%08x]\n", val, val2); > k1212_dbg(korg1212, "RecDataPhy = %08x L[%08x]\n", val, val2); > k1212_dbg(korg1212, "VolumeTablePhy = %08x L[%08x]\n", val, val2); > > etc. > > > My biggest concern right now is, however, about the unnecessary code > > increase by this patch. Currently, most of debug prints were simply > > not built, because of: > > > > > // > > > > > > -// Debug Stuff > > > -// > > > > > > -#define K1212_DEBUG_LEVEL0 > > > -#if K1212_DEBUG_LEVEL > 0 > > > -#define K1212_DEBUG_PRINTK(fmt,args...) printk(KERN_DEBUG fmt,##args) > > > -#else > > > -#define K1212_DEBUG_PRINTK(fmt,...) > > > -#endif > > > -#if K1212_DEBUG_LEVEL > 1 > > > -#define K1212_DEBUG_PRINTK_VERBOSE(fmt,args...) printk(KERN_DEBUG > > > fmt,##args) > > > -#else > > > -#define K1212_DEBUG_PRINTK_VERBOSE(fmt,...) > > > -#endif > > > > With your patch, now all these codes are compiled. > > Not really. > > dev_dbg is a no-op unless DEBUG is #defined > or CONFIG_DYNAMIC_DEBUG is set. > > > I have no clear answer what would be the best in such a case. I'd say > > it really depends. If they are just silly messages that can be > > covered in a better way (like ftrace), just get rid of them. If they > > are intended for some good register dumps, then dev_dbg() might make > > sense. > > very true. there are many dev_dbg which can be removed, they are just printing status message along with the statename of the card, and some dev_dbg is printing the arguments that the function has received. in the exising file we already have the macro: #define K1212_DEBUG_PRINTK(fmt,args...)printk(KERN_DEBUG fmt,##args) we can just modify the macro to: #define K1212_DEBUG_PRINTK(fmt,args...)dev_dbg(korg1212->card->dev, fmt,##args) then we will have very little thing to change in the code. and concerns of Takashi about size will also be taken care of, and at the same time all printk will be cleared. problems with this way: 1) macro name is little misleading - macro says printk, but we are using dev_dbg 2) if some one later wants to add something to this file, and doesnot want to use the variable name korg1212 in his function. any suggestions ? thanks sudip > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] ALSA: korg1212: cleanup of printk
On Mon, 2014-11-24 at 18:08 +0100, Takashi Iwai wrote: > At Sun, 23 Nov 2014 13:40:51 +0530, Sudip Mukherjee wrote: [] > > replaced all references of the debug messages via printk > > with dev_* macro (mostly dev_dbg). > > one reference was changed to pr_err as there the card might have been > > uninitialized. > > > > this patch will generate warning from checkpatch about broken quoted > > strings. but that was not fixed intentionally to improve the > > readability. I think it'd be easier to read and grep coalesced. [] > > in your review of v1, you said about some lines which are not ending > > with \n. but i was not able to find them. did i miss them somewhere? [] > The problem is the one with multiple "\n", for example: > > dev_dbg(korg1212->card->dev, "dspMemPhy = %08x U[%08x], " > "PlayDataPhy = %08x L[%08x]\n" > "korg1212: RecDataPhy = %08x L[%08x], " > "VolumeTablePhy = %08x L[%08x]\n" > "korg1212: RoutingTablePhy = %08x L[%08x], " > "AdatTimeCodePhy = %08x L[%08x]\n", I think these should be individual dev_dbg calls dev_dbg(korg1212->card->dev, "dspMemPhy = %08x U[%08x]\n", val, val2) dev_dbg(korg1212->card->dev, "PhyDataPhy = %08x L[%08x]\n", val, val2); dev_dbg(korg1212->card->dev, "RecDataPhy = %08x L[%08x]\n", val, val2); dev_dbg(korg1212->card->dev, "VolumeTablePhy = %08x L[%08x]\n", val, val2); etc.. Another possibility is to use another macro like: #define k1212_dbg(k1212, fmt, ...) \ dev_dbg((k)->card->dev, fmt, ##__VA_ARGS__) and change all these to k1212_dbg(korg1212, "dspMemPhy = %08x U[%08x]\n", val, val2) k1212_dbg(korg1212, "PhyDataPhy = %08x L[%08x]\n", val, val2); k1212_dbg(korg1212, "RecDataPhy = %08x L[%08x]\n", val, val2); k1212_dbg(korg1212, "VolumeTablePhy = %08x L[%08x]\n", val, val2); etc. > My biggest concern right now is, however, about the unnecessary code > increase by this patch. Currently, most of debug prints were simply > not built, because of: > > > // > > > > -// Debug Stuff > > -// > > > > -#define K1212_DEBUG_LEVEL 0 > > -#if K1212_DEBUG_LEVEL > 0 > > -#define K1212_DEBUG_PRINTK(fmt,args...)printk(KERN_DEBUG fmt,##args) > > -#else > > -#define K1212_DEBUG_PRINTK(fmt,...) > > -#endif > > -#if K1212_DEBUG_LEVEL > 1 > > -#define K1212_DEBUG_PRINTK_VERBOSE(fmt,args...)printk(KERN_DEBUG > > fmt,##args) > > -#else > > -#define K1212_DEBUG_PRINTK_VERBOSE(fmt,...) > > -#endif > > With your patch, now all these codes are compiled. Not really. dev_dbg is a no-op unless DEBUG is #defined or CONFIG_DYNAMIC_DEBUG is set. > I have no clear answer what would be the best in such a case. I'd say > it really depends. If they are just silly messages that can be > covered in a better way (like ftrace), just get rid of them. If they > are intended for some good register dumps, then dev_dbg() might make > sense. very true. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] ALSA: korg1212: cleanup of printk
At Sun, 23 Nov 2014 13:40:51 +0530, Sudip Mukherjee wrote: > > From: Sudip Mukherjee > > replaced all references of the debug messages via printk > with dev_* macro (mostly dev_dbg). > one reference was changed to pr_err as there the card might have been > uninitialized. > > this patch will generate warning from checkpatch about broken quoted > strings. but that was not fixed intentionally to improve the > readability. > > Signed-off-by: Sudip Mukherjee > --- > > change in v2: the build warnings of v1 are fixed. > > hi Takashi, > in your review of v1, you said about some lines which are not ending > with \n. but i was not able to find them. did i miss them somewhere? The problem is the one with multiple "\n", for example: dev_dbg(korg1212->card->dev, "dspMemPhy = %08x U[%08x], " "PlayDataPhy = %08x L[%08x]\n" "korg1212: RecDataPhy = %08x L[%08x], " "VolumeTablePhy = %08x L[%08x]\n" "korg1212: RoutingTablePhy = %08x L[%08x], " "AdatTimeCodePhy = %08x L[%08x]\n", My biggest concern right now is, however, about the unnecessary code increase by this patch. Currently, most of debug prints were simply not built, because of: > // > > -// Debug Stuff > -// > > -#define K1212_DEBUG_LEVEL0 > -#if K1212_DEBUG_LEVEL > 0 > -#define K1212_DEBUG_PRINTK(fmt,args...) printk(KERN_DEBUG fmt,##args) > -#else > -#define K1212_DEBUG_PRINTK(fmt,...) > -#endif > -#if K1212_DEBUG_LEVEL > 1 > -#define K1212_DEBUG_PRINTK_VERBOSE(fmt,args...) printk(KERN_DEBUG > fmt,##args) > -#else > -#define K1212_DEBUG_PRINTK_VERBOSE(fmt,...) > -#endif With your patch, now all these codes are compiled. I have no clear answer what would be the best in such a case. I'd say it really depends. If they are just silly messages that can be covered in a better way (like ftrace), just get rid of them. If they are intended for some good register dumps, then dev_dbg() might make sense. Takashi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] ALSA: korg1212: cleanup of printk
At Sun, 23 Nov 2014 13:40:51 +0530, Sudip Mukherjee wrote: From: Sudip Mukherjee su...@vectorindia.org replaced all references of the debug messages via printk with dev_* macro (mostly dev_dbg). one reference was changed to pr_err as there the card might have been uninitialized. this patch will generate warning from checkpatch about broken quoted strings. but that was not fixed intentionally to improve the readability. Signed-off-by: Sudip Mukherjee su...@vectorindia.org --- change in v2: the build warnings of v1 are fixed. hi Takashi, in your review of v1, you said about some lines which are not ending with \n. but i was not able to find them. did i miss them somewhere? The problem is the one with multiple \n, for example: dev_dbg(korg1212-card-dev, dspMemPhy = %08x U[%08x], PlayDataPhy = %08x L[%08x]\n korg1212: RecDataPhy = %08x L[%08x], VolumeTablePhy = %08x L[%08x]\n korg1212: RoutingTablePhy = %08x L[%08x], AdatTimeCodePhy = %08x L[%08x]\n, My biggest concern right now is, however, about the unnecessary code increase by this patch. Currently, most of debug prints were simply not built, because of: // -// Debug Stuff -// -#define K1212_DEBUG_LEVEL0 -#if K1212_DEBUG_LEVEL 0 -#define K1212_DEBUG_PRINTK(fmt,args...) printk(KERN_DEBUG fmt,##args) -#else -#define K1212_DEBUG_PRINTK(fmt,...) -#endif -#if K1212_DEBUG_LEVEL 1 -#define K1212_DEBUG_PRINTK_VERBOSE(fmt,args...) printk(KERN_DEBUG fmt,##args) -#else -#define K1212_DEBUG_PRINTK_VERBOSE(fmt,...) -#endif With your patch, now all these codes are compiled. I have no clear answer what would be the best in such a case. I'd say it really depends. If they are just silly messages that can be covered in a better way (like ftrace), just get rid of them. If they are intended for some good register dumps, then dev_dbg() might make sense. Takashi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] ALSA: korg1212: cleanup of printk
On Mon, 2014-11-24 at 18:08 +0100, Takashi Iwai wrote: At Sun, 23 Nov 2014 13:40:51 +0530, Sudip Mukherjee wrote: [] replaced all references of the debug messages via printk with dev_* macro (mostly dev_dbg). one reference was changed to pr_err as there the card might have been uninitialized. this patch will generate warning from checkpatch about broken quoted strings. but that was not fixed intentionally to improve the readability. I think it'd be easier to read and grep coalesced. [] in your review of v1, you said about some lines which are not ending with \n. but i was not able to find them. did i miss them somewhere? [] The problem is the one with multiple \n, for example: dev_dbg(korg1212-card-dev, dspMemPhy = %08x U[%08x], PlayDataPhy = %08x L[%08x]\n korg1212: RecDataPhy = %08x L[%08x], VolumeTablePhy = %08x L[%08x]\n korg1212: RoutingTablePhy = %08x L[%08x], AdatTimeCodePhy = %08x L[%08x]\n, I think these should be individual dev_dbg calls dev_dbg(korg1212-card-dev, dspMemPhy = %08x U[%08x]\n, val, val2) dev_dbg(korg1212-card-dev, PhyDataPhy = %08x L[%08x]\n, val, val2); dev_dbg(korg1212-card-dev, RecDataPhy = %08x L[%08x]\n, val, val2); dev_dbg(korg1212-card-dev, VolumeTablePhy = %08x L[%08x]\n, val, val2); etc.. Another possibility is to use another macro like: #define k1212_dbg(k1212, fmt, ...) \ dev_dbg((k)-card-dev, fmt, ##__VA_ARGS__) and change all these to k1212_dbg(korg1212, dspMemPhy = %08x U[%08x]\n, val, val2) k1212_dbg(korg1212, PhyDataPhy = %08x L[%08x]\n, val, val2); k1212_dbg(korg1212, RecDataPhy = %08x L[%08x]\n, val, val2); k1212_dbg(korg1212, VolumeTablePhy = %08x L[%08x]\n, val, val2); etc. My biggest concern right now is, however, about the unnecessary code increase by this patch. Currently, most of debug prints were simply not built, because of: // -// Debug Stuff -// -#define K1212_DEBUG_LEVEL 0 -#if K1212_DEBUG_LEVEL 0 -#define K1212_DEBUG_PRINTK(fmt,args...)printk(KERN_DEBUG fmt,##args) -#else -#define K1212_DEBUG_PRINTK(fmt,...) -#endif -#if K1212_DEBUG_LEVEL 1 -#define K1212_DEBUG_PRINTK_VERBOSE(fmt,args...)printk(KERN_DEBUG fmt,##args) -#else -#define K1212_DEBUG_PRINTK_VERBOSE(fmt,...) -#endif With your patch, now all these codes are compiled. Not really. dev_dbg is a no-op unless DEBUG is #defined or CONFIG_DYNAMIC_DEBUG is set. I have no clear answer what would be the best in such a case. I'd say it really depends. If they are just silly messages that can be covered in a better way (like ftrace), just get rid of them. If they are intended for some good register dumps, then dev_dbg() might make sense. very true. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] ALSA: korg1212: cleanup of printk
On Mon, Nov 24, 2014 at 09:25:10AM -0800, Joe Perches wrote: On Mon, 2014-11-24 at 18:08 +0100, Takashi Iwai wrote: At Sun, 23 Nov 2014 13:40:51 +0530, Sudip Mukherjee wrote: [] replaced all references of the debug messages via printk with dev_* macro (mostly dev_dbg). one reference was changed to pr_err as there the card might have been uninitialized. this patch will generate warning from checkpatch about broken quoted strings. but that was not fixed intentionally to improve the readability. I think it'd be easier to read and grep coalesced. or maybe better will be individual dev_dbg calls [] in your review of v1, you said about some lines which are not ending with \n. but i was not able to find them. did i miss them somewhere? [] The problem is the one with multiple \n, for example: dev_dbg(korg1212-card-dev, dspMemPhy = %08x U[%08x], PlayDataPhy = %08x L[%08x]\n korg1212: RecDataPhy = %08x L[%08x], VolumeTablePhy = %08x L[%08x]\n korg1212: RoutingTablePhy = %08x L[%08x], AdatTimeCodePhy = %08x L[%08x]\n, I think these should be individual dev_dbg calls dev_dbg(korg1212-card-dev, dspMemPhy = %08x U[%08x]\n, val, val2) dev_dbg(korg1212-card-dev, PhyDataPhy = %08x L[%08x]\n, val, val2); dev_dbg(korg1212-card-dev, RecDataPhy = %08x L[%08x]\n, val, val2); dev_dbg(korg1212-card-dev, VolumeTablePhy = %08x L[%08x]\n, val, val2); etc.. Another possibility is to use another macro like: #define k1212_dbg(k1212, fmt, ...)\ dev_dbg((k)-card-dev, fmt, ##__VA_ARGS__) and change all these to k1212_dbg(korg1212, dspMemPhy = %08x U[%08x]\n, val, val2) k1212_dbg(korg1212, PhyDataPhy = %08x L[%08x]\n, val, val2); k1212_dbg(korg1212, RecDataPhy = %08x L[%08x]\n, val, val2); k1212_dbg(korg1212, VolumeTablePhy = %08x L[%08x]\n, val, val2); etc. My biggest concern right now is, however, about the unnecessary code increase by this patch. Currently, most of debug prints were simply not built, because of: // -// Debug Stuff -// -#define K1212_DEBUG_LEVEL0 -#if K1212_DEBUG_LEVEL 0 -#define K1212_DEBUG_PRINTK(fmt,args...) printk(KERN_DEBUG fmt,##args) -#else -#define K1212_DEBUG_PRINTK(fmt,...) -#endif -#if K1212_DEBUG_LEVEL 1 -#define K1212_DEBUG_PRINTK_VERBOSE(fmt,args...) printk(KERN_DEBUG fmt,##args) -#else -#define K1212_DEBUG_PRINTK_VERBOSE(fmt,...) -#endif With your patch, now all these codes are compiled. Not really. dev_dbg is a no-op unless DEBUG is #defined or CONFIG_DYNAMIC_DEBUG is set. I have no clear answer what would be the best in such a case. I'd say it really depends. If they are just silly messages that can be covered in a better way (like ftrace), just get rid of them. If they are intended for some good register dumps, then dev_dbg() might make sense. very true. there are many dev_dbg which can be removed, they are just printing status message along with the statename of the card, and some dev_dbg is printing the arguments that the function has received. in the exising file we already have the macro: #define K1212_DEBUG_PRINTK(fmt,args...)printk(KERN_DEBUG fmt,##args) we can just modify the macro to: #define K1212_DEBUG_PRINTK(fmt,args...)dev_dbg(korg1212-card-dev, fmt,##args) then we will have very little thing to change in the code. and concerns of Takashi about size will also be taken care of, and at the same time all printk will be cleared. problems with this way: 1) macro name is little misleading - macro says printk, but we are using dev_dbg 2) if some one later wants to add something to this file, and doesnot want to use the variable name korg1212 in his function. any suggestions ? thanks sudip -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
