Re: [PATCH 3/4] [media] mceusb: remove redundant function and defines

[Mauro Carvalho Chehab <m.chehab@samsung.com>]

Hi Sean,

Em Mon, 20 Jan 2014 22:10:43 +0000
Sean Young <sean@mess.org> escreveu:

Could you please provide a patch description? Even simple ones should have,
and this one is everything but trivial...

Also, you should likely break it into smaller changesets. For example, the
last hunk adding a usb_kill_urb() looks more like a bugfix than a pure
cleanup change.

Thanks!
Mauro

> Signed-off-by: Sean Young <sean@mess.org>
> ---
>  drivers/media/rc/mceusb.c | 92 +++++++++++++++--------------------------------
>  1 file changed, 28 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/media/rc/mceusb.c b/drivers/media/rc/mceusb.c
> index a25bb15..3a4f95f 100644
> --- a/drivers/media/rc/mceusb.c
> +++ b/drivers/media/rc/mceusb.c
> @@ -166,15 +166,6 @@ static bool debug;
>  			dev_info(dev, fmt, ## __VA_ARGS__);	\
>  	} while (0)
>  
> -/* general constants */
> -#define SEND_FLAG_IN_PROGRESS	1
> -#define SEND_FLAG_COMPLETE	2
> -#define RECV_FLAG_IN_PROGRESS	3
> -#define RECV_FLAG_COMPLETE	4
> -
> -#define MCEUSB_RX		1
> -#define MCEUSB_TX		2
> -
>  #define VENDOR_PHILIPS		0x0471
>  #define VENDOR_SMK		0x0609
>  #define VENDOR_TATUNG		0x1460
> @@ -452,7 +443,6 @@ struct mceusb_dev {
>  	} flags;
>  
>  	/* transmit support */
> -	int send_flags;
>  	u32 carrier;
>  	unsigned char tx_mask;
>  
> @@ -731,45 +721,29 @@ static void mce_async_callback(struct urb *urb)
>  
>  /* request incoming or send outgoing usb packet - used to initialize remote */
>  static void mce_request_packet(struct mceusb_dev *ir, unsigned char *data,
> -			       int size, int urb_type)
> +			       int size)
>  {
>  	int res, pipe;
>  	struct urb *async_urb;
>  	struct device *dev = ir->dev;
>  	unsigned char *async_buf;
>  
> -	if (urb_type == MCEUSB_TX) {
> -		async_urb = usb_alloc_urb(0, GFP_KERNEL);
> -		if (unlikely(!async_urb)) {
> -			dev_err(dev, "Error, couldn't allocate urb!\n");
> -			return;
> -		}
> -
> -		async_buf = kzalloc(size, GFP_KERNEL);
> -		if (!async_buf) {
> -			dev_err(dev, "Error, couldn't allocate buf!\n");
> -			usb_free_urb(async_urb);
> -			return;
> -		}
> +	async_urb = usb_alloc_urb(0, GFP_KERNEL);
> +	if (unlikely(!async_urb))
> +		return;
>  
> -		/* outbound data */
> -		pipe = usb_sndintpipe(ir->usbdev,
> -				      ir->usb_ep_out->bEndpointAddress);
> -		usb_fill_int_urb(async_urb, ir->usbdev, pipe,
> -			async_buf, size, mce_async_callback,
> -			ir, ir->usb_ep_out->bInterval);
> -		memcpy(async_buf, data, size);
> -
> -	} else if (urb_type == MCEUSB_RX) {
> -		/* standard request */
> -		async_urb = ir->urb_in;
> -		ir->send_flags = RECV_FLAG_IN_PROGRESS;
> -
> -	} else {
> -		dev_err(dev, "Error! Unknown urb type %d\n", urb_type);
> +	async_buf = kmalloc(size, GFP_KERNEL);
> +	if (!async_buf) {
> +		usb_free_urb(async_urb);
>  		return;
>  	}
>  
> +	/* outbound data */
> +	pipe = usb_sndintpipe(ir->usbdev, ir->usb_ep_out->bEndpointAddress);
> +	usb_fill_int_urb(async_urb, ir->usbdev, pipe, async_buf, size,
> +			mce_async_callback, ir, ir->usb_ep_out->bInterval);
> +	memcpy(async_buf, data, size);
> +
>  	mce_dbg(dev, "receive request called (size=%#x)\n", size);
>  
>  	async_urb->transfer_buffer_length = size;
> @@ -789,19 +763,14 @@ static void mce_async_out(struct mceusb_dev *ir, unsigned char *data, int size)
>  
>  	if (ir->need_reset) {
>  		ir->need_reset = false;
> -		mce_request_packet(ir, DEVICE_RESUME, rsize, MCEUSB_TX);
> +		mce_request_packet(ir, DEVICE_RESUME, rsize);
>  		msleep(10);
>  	}
>  
> -	mce_request_packet(ir, data, size, MCEUSB_TX);
> +	mce_request_packet(ir, data, size);
>  	msleep(10);
>  }
>  
> -static void mce_flush_rx_buffer(struct mceusb_dev *ir, int size)
> -{
> -	mce_request_packet(ir, NULL, size, MCEUSB_RX);
> -}
> -
>  /* Send data out the IR blaster port(s) */
>  static int mceusb_tx_ir(struct rc_dev *dev, unsigned *txbuf, unsigned count)
>  {
> @@ -1040,7 +1009,6 @@ static void mceusb_process_ir_data(struct mceusb_dev *ir, int buf_len)
>  static void mceusb_dev_recv(struct urb *urb)
>  {
>  	struct mceusb_dev *ir;
> -	int buf_len;
>  
>  	if (!urb)
>  		return;
> @@ -1051,18 +1019,10 @@ static void mceusb_dev_recv(struct urb *urb)
>  		return;
>  	}
>  
> -	buf_len = urb->actual_length;
> -
> -	if (ir->send_flags == RECV_FLAG_IN_PROGRESS) {
> -		ir->send_flags = SEND_FLAG_COMPLETE;
> -		mce_dbg(ir->dev, "setup answer received %d bytes\n",
> -			buf_len);
> -	}
> -
>  	switch (urb->status) {
>  	/* success */
>  	case 0:
> -		mceusb_process_ir_data(ir, buf_len);
> +		mceusb_process_ir_data(ir, urb->actual_length);
>  		break;
>  
>  	case -ECONNRESET:
> @@ -1250,7 +1210,7 @@ static int mceusb_dev_probe(struct usb_interface *intf,
>  	struct usb_endpoint_descriptor *ep_in = NULL;
>  	struct usb_endpoint_descriptor *ep_out = NULL;
>  	struct mceusb_dev *ir = NULL;
> -	int pipe, maxp, i;
> +	int pipe, maxp, i, res;
>  	char buf[63], name[128] = "";
>  	enum mceusb_model_type model = id->driver_info;
>  	bool is_gen3;
> @@ -1346,19 +1306,21 @@ static int mceusb_dev_probe(struct usb_interface *intf,
>  		snprintf(name + strlen(name), sizeof(name) - strlen(name),
>  			 " %s", buf);
>  
> -	ir->rc = mceusb_init_rc_dev(ir);
> -	if (!ir->rc)
> -		goto rc_dev_fail;
> -
>  	/* wire up inbound data handler */
>  	usb_fill_int_urb(ir->urb_in, dev, pipe, ir->buf_in, maxp,
>  				mceusb_dev_recv, ir, ep_in->bInterval);
>  	ir->urb_in->transfer_dma = ir->dma_in;
>  	ir->urb_in->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
>  
> -	/* flush buffers on the device */
> -	mce_dbg(&intf->dev, "Flushing receive buffers\n");
> -	mce_flush_rx_buffer(ir, maxp);
> +	res = usb_submit_urb(ir->urb_in, GFP_KERNEL);
> +	if (res) {
> +		dev_err(&intf->dev, "failed to submit urb: %d\n", res);
> +		goto usb_submit_fail;
> +	}
> +
> +	ir->rc = mceusb_init_rc_dev(ir);
> +	if (!ir->rc)
> +		goto rc_dev_fail;
>  
>  	/* figure out which firmware/emulator version this hardware has */
>  	mceusb_get_emulator_version(ir);
> @@ -1393,6 +1355,8 @@ static int mceusb_dev_probe(struct usb_interface *intf,
>  
>  	/* Error-handling path */
>  rc_dev_fail:
> +	usb_kill_urb(ir->urb_in);
> +usb_submit_fail:
>  	usb_free_urb(ir->urb_in);
>  urb_in_alloc_fail:
>  	usb_free_coherent(dev, maxp, ir->buf_in, ir->dma_in);


-- 

Cheers,
Mauro