diff mbox

[PATCHv2] staging: media: as102: replace custom dprintk() with dev_dbg()

Message ID 1407147434-27732-1-git-send-email-martink@posteo.de (mailing list archive)
State New, archived
Headers show

Commit Message

Martin Kepplinger-Novakovic Aug. 4, 2014, 10:17 a.m. UTC 
remove dprintk() and replace it with dev_dbg() in order to
use the common kernel coding style.

Signed-off-by: Martin Kepplinger <martink@posteo.de>
---
Thanks Dan. And since it continues to succeed if (dev == NULL),
differntiate if (dev) or not.

 drivers/staging/media/as102/as102_drv.c     |   15 +++---
 drivers/staging/media/as102/as102_drv.h     |    7 ---
 drivers/staging/media/as102/as102_fe.c      |   73 ++++++++++++++++++++-------
 drivers/staging/media/as102/as102_usb_drv.c |   41 ++++++++-------
 4 files changed, 86 insertions(+), 50 deletions(-)

Comments

Dan Carpenter Aug. 4, 2014, 10:40 a.m. UTC | #1 
On thinking about it some more the proper way to redo this would have
been to remove the bogus check.  Also a couple trivial things below.

On Mon, Aug 04, 2014 at 12:17:14PM +0200, Martin Kepplinger wrote:
> @@ -82,10 +83,17 @@ static int as102_fe_get_tune_settings(struct dvb_frontend *fe,
>  			struct dvb_frontend_tune_settings *settings) {
>  
>  #if 0
> -	dprintk(debug, "step_size    = %d\n", settings->step_size);
> -	dprintk(debug, "max_drift    = %d\n", settings->max_drift);
> -	dprintk(debug, "min_delay_ms = %d -> %d\n", settings->min_delay_ms,
> -		1000);
> +	struct as102_dev_t *dev;
> +
> +	dev = (struct as102_dev_t *) fe->tuner_priv;
> +	if (dev == NULL)
> +		return -EINVAL;

This doesn't make sense.  Why are we crapping out just because we can't
print some useless debug info?

Debugging code annoys me because of stuff like this.  Debugging code is
buggier than normal code because it never gets tested.  It complicates
the code.  People think they are helping when they add lots of debug
code, but normally they are just making the situation worse.

Also it bloats the kernel a bit.

Just remove this whole block because it is ifdef 0.

> +	dev_dbg(&dev->bus_adap.usb_dev->dev,
> +		"step_size    = %d\n", settings->step_size);
> +	dev_dbg(&dev->bus_adap.usb_dev->dev,
> +		"max_drift    = %d\n", settings->max_drift);
> +	dev_dbg(&dev->bus_adap.usb_dev->dev,
> +		"min_delay_ms = %d -> %d\n", settings->min_delay_ms, 1000);
>  #endif
>  
>  	settings->min_delay_ms = 1000;
> @@ -141,10 +151,10 @@ static int as102_fe_read_status(struct dvb_frontend *fe, fe_status_t *status)
>  		if (as10x_cmd_get_demod_stats(&dev->bus_adap,
>  			(struct as10x_demod_stats *) &dev->demod_stats) < 0) {
>  			memset(&dev->demod_stats, 0, sizeof(dev->demod_stats));
> -			dprintk(debug,
> -				"as10x_cmd_get_demod_stats failed (probably not tuned)\n");
> +			dev_dbg(&dev->bus_adap.usb_dev->dev,
> +			"as10x_cmd_get_demod_stats failed (probably not tuned)\n");

This isn't indented correctly.

>  		} else {
> -			dprintk(debug,
> +			dev_dbg(&dev->bus_adap.usb_dev->dev,
>  				"demod status: fc: 0x%08x, bad fc: 0x%08x, "
>  				"bytes corrected: 0x%08x , MER: 0x%04x\n",
>  				dev->demod_stats.frame_count,
> @@ -447,6 +457,15 @@ static uint8_t as102_fe_get_code_rate(fe_code_rate_t arg)
>  static void as102_fe_copy_tune_parameters(struct as10x_tune_args *tune_args,
>  			  struct dtv_frontend_properties *params)
>  {
> +	struct dvb_frontend *fe;
> +	struct as102_dev_t *dev;
> +
> +	fe = container_of(params, struct dvb_frontend, dtv_property_cache);
> +	dev = (struct as102_dev_t *) fe->tuner_priv;
> +	if (dev == NULL)
> +		pr_err("as102: No device found\n");

This condition is never true.  Don't add tests if you know the answer.

> +	else
> +		dev_err(&dev->bus_adap.usb_dev->dev, "No device found\n");
>  
>  	/* set frequency */
>  	tune_args->freq = params->frequency / 1000;
> @@ -531,10 +550,18 @@ static void as102_fe_copy_tune_parameters(struct as10x_tune_args *tune_args,
>  		break;
>  	}
>  
> -	dprintk(debug, "tuner parameters: freq: %d  bw: 0x%02x  gi: 0x%02x\n",
> +	if (dev) {
> +		dev_dbg(&dev->bus_adap.usb_dev->dev,
> +		"tuner parameters: freq: %d  bw: 0x%02x  gi: 0x%02x\n",
> +			params->frequency,
> +			tune_args->bandwidth,
> +			tune_args->guard_interval);
> +	} else {
> +	pr_debug("as102: tuner parameters: freq: %d  bw: 0x%02x  gi: 0x%02x\n",
>  			params->frequency,
>  			tune_args->bandwidth,
>  			tune_args->guard_interval);
> +	}


This isn't indented correctly.  I wish checkpatch.pl would catch that...
Anyway, the else side can be removed as explained earlier.

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joe Perches Aug. 4, 2014, 11:12 a.m. UTC | #2 
On Mon, 2014-08-04 at 13:40 +0300, Dan Carpenter wrote:
> On Mon, Aug 04, 2014 at 12:17:14PM +0200, Martin Kepplinger wrote:
[]
> > +	if (dev) {
> > +		dev_dbg(&dev->bus_adap.usb_dev->dev,
> > +		"tuner parameters: freq: %d  bw: 0x%02x  gi: 0x%02x\n",
> > +			params->frequency,
> > +			tune_args->bandwidth,
> > +			tune_args->guard_interval);
> > +	} else {
> > +	pr_debug("as102: tuner parameters: freq: %d  bw: 0x%02x  gi: 0x%02x\n",
> >  			params->frequency,
> >  			tune_args->bandwidth,
> >  			tune_args->guard_interval);
> > +	}
[]
> This isn't indented correctly.  I wish checkpatch.pl would catch that...
> Anyway, the else side can be removed as explained earlier.

checkpatch doesn't warn on any of:

$ cat t.c
static int func(void **bar)
{
	bool b;
			/* test */
		int a[2] = {
			3,
				6
	};

		if (b) {
			b = a[0] == a[1];
	}
			return a[b];
	}
$

I think it'd be better if it would one day
but it doesn't seem as simple as it seems
for checkpatch to do it.

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/staging/media/as102/as102_drv.c b/drivers/staging/media/as102/as102_drv.c
index 09d64cd..e0ee618 100644
--- a/drivers/staging/media/as102/as102_drv.c
+++ b/drivers/staging/media/as102/as102_drv.c
@@ -31,10 +31,6 @@ 
 #include "as102_fw.h"
 #include "dvbdev.h"
 
-int as102_debug;
-module_param_named(debug, as102_debug, int, 0644);
-MODULE_PARM_DESC(debug, "Turn on/off debugging (default: off)");
-
 int dual_tuner;
 module_param_named(dual_tuner, dual_tuner, int, 0644);
 MODULE_PARM_DESC(dual_tuner, "Activate Dual-Tuner config (default: off)");
@@ -74,7 +70,8 @@  static void as102_stop_stream(struct as102_dev_t *dev)
 			return;
 
 		if (as10x_cmd_stop_streaming(bus_adap) < 0)
-			dprintk(debug, "as10x_cmd_stop_streaming failed\n");
+			dev_dbg(&dev->bus_adap.usb_dev->dev,
+				"as10x_cmd_stop_streaming failed\n");
 
 		mutex_unlock(&dev->bus_adap.lock);
 	}
@@ -112,14 +109,16 @@  static int as10x_pid_filter(struct as102_dev_t *dev,
 	int ret = -EFAULT;
 
 	if (mutex_lock_interruptible(&dev->bus_adap.lock)) {
-		dprintk(debug, "mutex_lock_interruptible(lock) failed !\n");
+		dev_dbg(&dev->bus_adap.usb_dev->dev,
+			"amutex_lock_interruptible(lock) failed !\n");
 		return -EBUSY;
 	}
 
 	switch (onoff) {
 	case 0:
 		ret = as10x_cmd_del_PID_filter(bus_adap, (uint16_t) pid);
-		dprintk(debug, "DEL_PID_FILTER([%02d] 0x%04x) ret = %d\n",
+		dev_dbg(&dev->bus_adap.usb_dev->dev,
+			"DEL_PID_FILTER([%02d] 0x%04x) ret = %d\n",
 			index, pid, ret);
 		break;
 	case 1:
@@ -131,7 +130,7 @@  static int as10x_pid_filter(struct as102_dev_t *dev,
 		filter.pid = pid;
 
 		ret = as10x_cmd_add_PID_filter(bus_adap, &filter);
-		dprintk(debug,
+		dev_dbg(&dev->bus_adap.usb_dev->dev,
 			"ADD_PID_FILTER([%02d -> %02d], 0x%04x) ret = %d\n",
 			index, filter.idx, filter.pid, ret);
 		break;
diff --git a/drivers/staging/media/as102/as102_drv.h b/drivers/staging/media/as102/as102_drv.h
index a06837d..49d0c42 100644
--- a/drivers/staging/media/as102/as102_drv.h
+++ b/drivers/staging/media/as102/as102_drv.h
@@ -27,17 +27,10 @@ 
 #define DRIVER_FULL_NAME "Abilis Systems as10x usb driver"
 #define DRIVER_NAME "as10x_usb"
 
-extern int as102_debug;
 #define debug	as102_debug
 extern struct usb_driver as102_usb_driver;
 extern int elna_enable;
 
-#define dprintk(debug, args...) \
-	do { if (debug) {	\
-		pr_debug("%s: ", __func__);	\
-		printk(args);	\
-	} } while (0)
-
 #define AS102_DEVICE_MAJOR	192
 
 #define AS102_USB_BUF_SIZE	512
diff --git a/drivers/staging/media/as102/as102_fe.c b/drivers/staging/media/as102/as102_fe.c
index b686b76..4c4c47d 100644
--- a/drivers/staging/media/as102/as102_fe.c
+++ b/drivers/staging/media/as102/as102_fe.c
@@ -46,7 +46,8 @@  static int as102_fe_set_frontend(struct dvb_frontend *fe)
 	/* send abilis command: SET_TUNE */
 	ret =  as10x_cmd_set_tune(&dev->bus_adap, &tune_args);
 	if (ret != 0)
-		dprintk(debug, "as10x_cmd_set_tune failed. (err = %d)\n", ret);
+		dev_dbg(&dev->bus_adap.usb_dev->dev,
+			"as10x_cmd_set_tune failed. (err = %d)\n", ret);
 
 	mutex_unlock(&dev->bus_adap.lock);
 
@@ -82,10 +83,17 @@  static int as102_fe_get_tune_settings(struct dvb_frontend *fe,
 			struct dvb_frontend_tune_settings *settings) {
 
 #if 0
-	dprintk(debug, "step_size    = %d\n", settings->step_size);
-	dprintk(debug, "max_drift    = %d\n", settings->max_drift);
-	dprintk(debug, "min_delay_ms = %d -> %d\n", settings->min_delay_ms,
-		1000);
+	struct as102_dev_t *dev;
+
+	dev = (struct as102_dev_t *) fe->tuner_priv;
+	if (dev == NULL)
+		return -EINVAL;
+	dev_dbg(&dev->bus_adap.usb_dev->dev,
+		"step_size    = %d\n", settings->step_size);
+	dev_dbg(&dev->bus_adap.usb_dev->dev,
+		"max_drift    = %d\n", settings->max_drift);
+	dev_dbg(&dev->bus_adap.usb_dev->dev,
+		"min_delay_ms = %d -> %d\n", settings->min_delay_ms, 1000);
 #endif
 
 	settings->min_delay_ms = 1000;
@@ -110,7 +118,8 @@  static int as102_fe_read_status(struct dvb_frontend *fe, fe_status_t *status)
 	/* send abilis command: GET_TUNE_STATUS */
 	ret = as10x_cmd_get_tune_status(&dev->bus_adap, &tstate);
 	if (ret < 0) {
-		dprintk(debug, "as10x_cmd_get_tune_status failed (err = %d)\n",
+		dev_dbg(&dev->bus_adap.usb_dev->dev,
+			"as10x_cmd_get_tune_status failed (err = %d)\n",
 			ret);
 		goto out;
 	}
@@ -133,7 +142,8 @@  static int as102_fe_read_status(struct dvb_frontend *fe, fe_status_t *status)
 		*status = TUNE_STATUS_NOT_TUNED;
 	}
 
-	dprintk(debug, "tuner status: 0x%02x, strength %d, per: %d, ber: %d\n",
+	dev_dbg(&dev->bus_adap.usb_dev->dev,
+			"tuner status: 0x%02x, strength %d, per: %d, ber: %d\n",
 			tstate.tune_state, tstate.signal_strength,
 			tstate.PER, tstate.BER);
 
@@ -141,10 +151,10 @@  static int as102_fe_read_status(struct dvb_frontend *fe, fe_status_t *status)
 		if (as10x_cmd_get_demod_stats(&dev->bus_adap,
 			(struct as10x_demod_stats *) &dev->demod_stats) < 0) {
 			memset(&dev->demod_stats, 0, sizeof(dev->demod_stats));
-			dprintk(debug,
-				"as10x_cmd_get_demod_stats failed (probably not tuned)\n");
+			dev_dbg(&dev->bus_adap.usb_dev->dev,
+			"as10x_cmd_get_demod_stats failed (probably not tuned)\n");
 		} else {
-			dprintk(debug,
+			dev_dbg(&dev->bus_adap.usb_dev->dev,
 				"demod status: fc: 0x%08x, bad fc: 0x%08x, "
 				"bytes corrected: 0x%08x , MER: 0x%04x\n",
 				dev->demod_stats.frame_count,
@@ -447,6 +457,15 @@  static uint8_t as102_fe_get_code_rate(fe_code_rate_t arg)
 static void as102_fe_copy_tune_parameters(struct as10x_tune_args *tune_args,
 			  struct dtv_frontend_properties *params)
 {
+	struct dvb_frontend *fe;
+	struct as102_dev_t *dev;
+
+	fe = container_of(params, struct dvb_frontend, dtv_property_cache);
+	dev = (struct as102_dev_t *) fe->tuner_priv;
+	if (dev == NULL)
+		pr_err("as102: No device found\n");
+	else
+		dev_err(&dev->bus_adap.usb_dev->dev, "No device found\n");
 
 	/* set frequency */
 	tune_args->freq = params->frequency / 1000;
@@ -531,10 +550,18 @@  static void as102_fe_copy_tune_parameters(struct as10x_tune_args *tune_args,
 		break;
 	}
 
-	dprintk(debug, "tuner parameters: freq: %d  bw: 0x%02x  gi: 0x%02x\n",
+	if (dev) {
+		dev_dbg(&dev->bus_adap.usb_dev->dev,
+		"tuner parameters: freq: %d  bw: 0x%02x  gi: 0x%02x\n",
+			params->frequency,
+			tune_args->bandwidth,
+			tune_args->guard_interval);
+	} else {
+	pr_debug("as102: tuner parameters: freq: %d  bw: 0x%02x  gi: 0x%02x\n",
 			params->frequency,
 			tune_args->bandwidth,
 			tune_args->guard_interval);
+	}
 
 	/*
 	 * Detect a hierarchy selection
@@ -556,14 +583,24 @@  static void as102_fe_copy_tune_parameters(struct as10x_tune_args *tune_args,
 			   as102_fe_get_code_rate(params->code_rate_LP);
 		}
 
-		dprintk(debug,
+		if (dev) {
+			dev_dbg(&dev->bus_adap.usb_dev->dev,
 			"\thierarchy: 0x%02x  selected: %s  code_rate_%s: 0x%02x\n",
-			tune_args->hierarchy,
-			tune_args->hier_select == HIER_HIGH_PRIORITY ?
-			"HP" : "LP",
-			tune_args->hier_select == HIER_HIGH_PRIORITY ?
-			"HP" : "LP",
-			tune_args->code_rate);
+				tune_args->hierarchy,
+				tune_args->hier_select == HIER_HIGH_PRIORITY ?
+				"HP" : "LP",
+				tune_args->hier_select == HIER_HIGH_PRIORITY ?
+				"HP" : "LP",
+				tune_args->code_rate);
+		} else {
+			pr_debug("as102: \thierarchy: 0x%02x  selected: %s  code_rate_%s: 0x%02x\n",
+				tune_args->hierarchy,
+				tune_args->hier_select == HIER_HIGH_PRIORITY ?
+				"HP" : "LP",
+				tune_args->hier_select == HIER_HIGH_PRIORITY ?
+				"HP" : "LP",
+				tune_args->code_rate);
+		}
 	} else {
 		tune_args->code_rate =
 			as102_fe_get_code_rate(params->code_rate_HP);
diff --git a/drivers/staging/media/as102/as102_usb_drv.c b/drivers/staging/media/as102/as102_usb_drv.c
index e6f6278..86f83b9 100644
--- a/drivers/staging/media/as102/as102_usb_drv.c
+++ b/drivers/staging/media/as102/as102_usb_drv.c
@@ -104,21 +104,22 @@  static int as102_usb_xfer_cmd(struct as10x_bus_adapter_t *bus_adap,
 				      send_buf, send_buf_len,
 				      USB_CTRL_SET_TIMEOUT /* 200 */);
 		if (ret < 0) {
-			dprintk(debug, "usb_control_msg(send) failed, err %i\n",
-					ret);
+			dev_dbg(&bus_adap->usb_dev->dev,
+				"usb_control_msg(send) failed, err %i\n", ret);
 			return ret;
 		}
 
 		if (ret != send_buf_len) {
-			dprintk(debug, "only wrote %d of %d bytes\n",
-					ret, send_buf_len);
+			dev_dbg(&bus_adap->usb_dev->dev,
+			"only wrote %d of %d bytes\n", ret, send_buf_len);
 			return -1;
 		}
 	}
 
 	if (recv_buf != NULL) {
 #ifdef TRACE
-		dprintk(debug, "want to read: %d bytes\n", recv_buf_len);
+		dev_dbg(bus_adap->usb_dev->dev,
+			"want to read: %d bytes\n", recv_buf_len);
 #endif
 		ret = usb_control_msg(bus_adap->usb_dev,
 				      usb_rcvctrlpipe(bus_adap->usb_dev, 0),
@@ -130,12 +131,13 @@  static int as102_usb_xfer_cmd(struct as10x_bus_adapter_t *bus_adap,
 				      recv_buf, recv_buf_len,
 				      USB_CTRL_GET_TIMEOUT /* 200 */);
 		if (ret < 0) {
-			dprintk(debug, "usb_control_msg(recv) failed, err %i\n",
-					ret);
+			dev_dbg(&bus_adap->usb_dev->dev,
+				"usb_control_msg(recv) failed, err %i\n", ret);
 			return ret;
 		}
 #ifdef TRACE
-		dprintk(debug, "read %d bytes\n", recv_buf_len);
+		dev_dbg(bus_adap->usb_dev->dev,
+			"read %d bytes\n", recv_buf_len);
 #endif
 	}
 
@@ -153,13 +155,14 @@  static int as102_send_ep1(struct as10x_bus_adapter_t *bus_adap,
 			   usb_sndbulkpipe(bus_adap->usb_dev, 1),
 			   send_buf, send_buf_len, &actual_len, 200);
 	if (ret) {
-		dprintk(debug, "usb_bulk_msg(send) failed, err %i\n", ret);
+		dev_dbg(&bus_adap->usb_dev->dev,
+			"usb_bulk_msg(send) failed, err %i\n", ret);
 		return ret;
 	}
 
 	if (actual_len != send_buf_len) {
-		dprintk(debug, "only wrote %d of %d bytes\n",
-				actual_len, send_buf_len);
+		dev_dbg(&bus_adap->usb_dev->dev, "only wrote %d of %d bytes\n",
+			actual_len, send_buf_len);
 		return -1;
 	}
 	return ret ? ret : actual_len;
@@ -177,13 +180,14 @@  static int as102_read_ep2(struct as10x_bus_adapter_t *bus_adap,
 			   usb_rcvbulkpipe(bus_adap->usb_dev, 2),
 			   recv_buf, recv_buf_len, &actual_len, 200);
 	if (ret) {
-		dprintk(debug, "usb_bulk_msg(recv) failed, err %i\n", ret);
+		dev_dbg(&bus_adap->usb_dev->dev,
+			"usb_bulk_msg(recv) failed, err %i\n", ret);
 		return ret;
 	}
 
 	if (actual_len != recv_buf_len) {
-		dprintk(debug, "only read %d of %d bytes\n",
-				actual_len, recv_buf_len);
+		dev_dbg(&bus_adap->usb_dev->dev, "only read %d of %d bytes\n",
+			actual_len, recv_buf_len);
 		return -1;
 	}
 	return ret ? ret : actual_len;
@@ -211,7 +215,8 @@  static int as102_submit_urb_stream(struct as102_dev_t *dev, struct urb *urb)
 
 	err = usb_submit_urb(urb, GFP_ATOMIC);
 	if (err)
-		dprintk(debug, "%s: usb_submit_urb failed\n", __func__);
+		dev_dbg(&urb->dev->dev,
+			"%s: usb_submit_urb failed\n", __func__);
 
 	return err;
 }
@@ -256,7 +261,8 @@  static int as102_alloc_usb_stream_buffer(struct as102_dev_t *dev)
 				       GFP_KERNEL,
 				       &dev->dma_addr);
 	if (!dev->stream) {
-		dprintk(debug, "%s: usb_buffer_alloc failed\n", __func__);
+		dev_dbg(&dev->bus_adap.usb_dev->dev,
+			"%s: usb_buffer_alloc failed\n", __func__);
 		return -ENOMEM;
 	}
 
@@ -268,7 +274,8 @@  static int as102_alloc_usb_stream_buffer(struct as102_dev_t *dev)
 
 		urb = usb_alloc_urb(0, GFP_ATOMIC);
 		if (urb == NULL) {
-			dprintk(debug, "%s: usb_alloc_urb failed\n", __func__);
+			dev_dbg(&dev->bus_adap.usb_dev->dev,
+				"%s: usb_alloc_urb failed\n", __func__);
 			as102_free_usb_stream_buffer(dev);
 			return -ENOMEM;
 		}