Return-Path: Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 9.0 \(3094\)) Subject: Re: [PATCH 4/4] Bluetooth: btmrvl: change debug prints to btmrvl_dbg From: Marcel Holtmann In-Reply-To: <8fc952a00b5c4a5a8a892517a2408d17@SC-EXCH04.marvell.com> Date: Thu, 15 Oct 2015 16:51:23 +0200 Cc: linux-bluetooth , Cathy Luo , Zhaoyang Liu Message-Id: <11913A3A-9988-4614-ABBE-68CF0E119186@holtmann.org> References: <1444836883-14701-1-git-send-email-akarwar@marvell.com> <1444836883-14701-4-git-send-email-akarwar@marvell.com> <8fc952a00b5c4a5a8a892517a2408d17@SC-EXCH04.marvell.com> To: Amitkumar Karwar Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Amitkumar, >> From: Marcel Holtmann [mailto:marcel@holtmann.org] >> Sent: Thursday, October 15, 2015 4:39 AM >> To: Amitkumar Karwar >> Cc: linux-bluetooth; Cathy Luo; Zhaoyang Liu >> Subject: Re: [PATCH 4/4] Bluetooth: btmrvl: change debug prints to >> btmrvl_dbg >> >> Hi Amitkumar, >> >>> This patch changes all debug print from BT_INFO/BT_DBG/BT_ERR to >>> marvell bluetooth driver specific debug functions. >> >> so BT_INFO and BT_ERR are not debug prints. They are genuine valid >> information that should be printed when something goes wrong or is >> informational. > > We will correct the words. > >> >>> >>> Signed-off-by: Zhaoyang Liu >>> Signed-off-by: Cathy Luo >>> Signed-off-by: Amitkumar Karwar >>> --- >>> drivers/bluetooth/btmrvl_debugfs.c | 3 +- >>> drivers/bluetooth/btmrvl_main.c | 145 ++++++++++------- >>> drivers/bluetooth/btmrvl_sdio.c | 311 ++++++++++++++++++++++------- >> -------- >>> 3 files changed, 275 insertions(+), 184 deletions(-) >>> >>> diff --git a/drivers/bluetooth/btmrvl_debugfs.c >>> b/drivers/bluetooth/btmrvl_debugfs.c >>> index af52b03..7bbe3dc 100644 >>> --- a/drivers/bluetooth/btmrvl_debugfs.c >>> +++ b/drivers/bluetooth/btmrvl_debugfs.c >>> @@ -257,7 +257,8 @@ void btmrvl_debugfs_init(struct hci_dev *hdev) >>> priv->debugfs_data = dbg; >>> >>> if (!dbg) { >>> - BT_ERR("Can not allocate memory for btmrvl_debugfs_data."); >>> + btmrvl_dbg(priv->adapter, ERROR, >>> + "Can not allocate memory for >> btmrvl_debugfs_data."); >> >> Errors are not debug messages. The syntax for btmrvl_dbg is not correct. >> We can not have such a code merged upstream. >> >> I am also feeling to see all this value with debug_mask and so on. The >> Linux kernel supports dynamic debug which is way more powerful than >> trying to invent your own mask based logging. And BT_DBG and other debug >> print functions are hooked into it. So why not use it. >> >> If you prefer having some btmrvl_dev_info wrappers, then that seems >> reasonable, but they either map to BT_INFO or pr_dev_info or similar. >> >> I would really prefer if this patch series gets re-thought and done the >> Linux kernel way. > > CONFIG_DYNAMIC_DEBUG compiler flag is not by default enabled on some of the platforms. > This was the reason we decided to implement this mask based logging. We do have such implementation for our WLAN driver. Also, I can see similar thing in other driver in wireless subsystem. and that is a good enough reason to duplicate functionality? My viewpoint is that it is not and that even wireless drivers should be cleaned up. Regards Marcel