From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754135AbaKXRIG (ORCPT ); Mon, 24 Nov 2014 12:08:06 -0500 Received: from cantor2.suse.de ([195.135.220.15]:50490 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751572AbaKXRIE (ORCPT ); Mon, 24 Nov 2014 12:08:04 -0500 Date: Mon, 24 Nov 2014 18:08:02 +0100 Message-ID: From: Takashi Iwai To: Sudip Mukherjee Cc: Jaroslav Kysela , alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] ALSA: korg1212: cleanup of printk In-Reply-To: <1416730251-5194-1-git-send-email-sudipm.mukherjee@gmail.com> References: <1416730251-5194-1-git-send-email-sudipm.mukherjee@gmail.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/24.4 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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_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. 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