Re: [irda-users] Kingsun KS-959 USB IrDA dongle - release candidate for submission (try 1)

Hi Alex,

Nice job with this crappy hardware. Some comments on your driver below:

On Fri, Jul 20, 2007 at 04:21:53PM -0500, Alex Villací­s Lasso wrote:
> +/* Callback transmission routine */
> +static void ks959_speed_irq(struct urb *urb)
> +{
> +	/* unlink, shutdown, unplug, other nasties */
> +	if (urb->status != 0) {
> +		err("ks959_speed_irq: urb asynchronously failed - %d", urb->status);

Here, shouldn't we call unlink_urb() if depending on the status value
(in the -EINPROGRESS at least) ?

> +static int ks959_change_speed(struct ks959_cb * kingsun, unsigned speed)
> +{
> +	static unsigned int supported_speeds[] = {2400, 9600, 19200, 38400,
> +		57600, 115200, 576000, 1152000, 4000000, 0};
> +	int err;
> +	unsigned int i;
> +
> +	if (kingsun->speed_setuprequest == NULL || kingsun->speed_urb == NULL)
> +		return -ENOMEM;
> +
> +	/* Check that requested speed is among the supported ones */
> +	for (i = 0; supported_speeds[i] != 0 && supported_speeds[i] != speed; i++);
> +	if (supported_speeds[i] == 0) return -EOPNOTSUPP;
> +
> +	memset(&(kingsun->speedparams), 0, sizeof(struct ks959_speedparams));
> +	kingsun->speedparams.baudrate = cpu_to_le32(speed);
> +	kingsun->speedparams.data_bits = 3;	/* 8 data bits */
> +
> +	kingsun->speed_setuprequest->bRequestType = USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE;
> +	kingsun->speed_setuprequest->bRequest = KINGSUN_REQ_SEND;
> +	kingsun->speed_setuprequest->wValue = cpu_to_le16(0x0200);
> +	kingsun->speed_setuprequest->wIndex = cpu_to_le16(0x0001);
> +	kingsun->speed_setuprequest->wLength = cpu_to_le16(8);
Any reason why we have to initialise the speed_setuprequest everytime ?
Same question goes for tx_setuprequest and rx_setuprequest.


> +static int ks959_probe(struct usb_interface *intf,
> +		      const struct usb_device_id *id)
> +{
> +	struct usb_device *dev = interface_to_usbdev(intf);
> +	struct ks959_cb *kingsun = NULL;
> +	struct net_device *net = NULL;
> +	int ret = -ENOMEM;
> +
> +
> +	/* Allocate network device container. */
> +	net = alloc_irdadev(sizeof(*kingsun));
> +	if(!net)
> +		goto err_out1;
> +
> +	SET_MODULE_OWNER(net);
> +	SET_NETDEV_DEV(net, &intf->dev);
> +	kingsun = netdev_priv(net);
> +	kingsun->netdev = net;
> +	kingsun->usbdev = dev;
> +	kingsun->irlap = NULL;
> +	kingsun->tx_setuprequest = NULL;
> +	kingsun->tx_urb = NULL;
> +	kingsun->tx_buf_clear = NULL;
> +	kingsun->tx_buf_xored = NULL;
> +	kingsun->tx_buf_clear_used = 0;
> +	kingsun->tx_buf_clear_sent = 0;
> +	
> +	kingsun->rx_setuprequest = NULL;
> +	kingsun->rx_urb = NULL;
> +	kingsun->rx_buf = NULL;
> +	kingsun->rx_variable_xormask = 0;
> +	kingsun->rx_unwrap_buff.in_frame = FALSE;
> +	kingsun->rx_unwrap_buff.state = OUTSIDE_FRAME;
> +	kingsun->rx_unwrap_buff.skb = NULL;
> +	kingsun->receiving = 0;
> +	spin_lock_init(&kingsun->lock);
> +
> +	kingsun->speed_setuprequest = NULL;
> +	kingsun->speed_urb = NULL;	
> +	kingsun->speedparams.baudrate = 0;
> +
> +	/* Allocate input buffer */
> +	kingsun->rx_buf = (__u8 *)kmalloc(KINGSUN_RCV_FIFO_SIZE, GFP_KERNEL);
As Domen pointed out earlier, you don't need the cast here and in all
the below kmalloc() calls.


> +	if (!kingsun->rx_buf) 
> +		goto free_mem;
> +
> +	/* Allocate input setup packet */
> +	kingsun->rx_setuprequest = (struct usb_ctrlrequest *)kmalloc(sizeof(struct usb_ctrlrequest), GFP_KERNEL);
> +	if (!kingsun->rx_setuprequest)
> +		goto free_mem;
> +	
> +	/* Allocate output buffer */
> +	kingsun->tx_buf_clear = (__u8 *)kmalloc(KINGSUN_SND_FIFO_SIZE, GFP_KERNEL);
> +	if (!kingsun->tx_buf_clear) 
> +		goto free_mem;
> +	kingsun->tx_buf_xored = (__u8 *)kmalloc(KINGSUN_SND_PACKET_SIZE, GFP_KERNEL);
> +	if (!kingsun->tx_buf_xored) 
> +		goto free_mem;
> +
> +	/* Allocate output setup packet */
> +	kingsun->tx_setuprequest = (struct usb_ctrlrequest *)kmalloc(sizeof(struct usb_ctrlrequest), GFP_KERNEL);
> +	if (!kingsun->tx_setuprequest)
> +		goto free_mem;
> +	
> +	/* Allocate speed setup packet */
> +	kingsun->speed_setuprequest = (struct usb_ctrlrequest *)kmalloc(sizeof(struct usb_ctrlrequest), GFP_KERNEL);
> +	if (!kingsun->speed_setuprequest)
> +		goto free_mem;
> +
> +	printk(KERN_INFO "KingSun KS-959 IRDA/USB found at address %d, "
> +		"Vendor: %x, Product: %x\n",
> +	       dev->devnum, le16_to_cpu(dev->descriptor.idVendor),
> +	       le16_to_cpu(dev->descriptor.idProduct));
> +
> +	/* Initialize QoS for this device */
> +	irda_init_max_qos_capabilies(&kingsun->qos);
> +
> +	/* Baud rates known to be supported. Please uncomment if devices (other 
> +	   than a Sony Ericcson K300 phone) can be shown to support higher speeds
> +	   with this dongle.
> +	 */
> +	kingsun->qos.baud_rate.bits = IR_2400 | IR_9600 | IR_19200  | IR_38400 | IR_57600;
> +	kingsun->qos.min_turn_time.bits   &= KINGSUN_MTT;
> +	irda_qos_bits_to_value(&kingsun->qos);
> +
> +	/* Override the network functions we need to use */
> +	net->hard_start_xmit = ks959_hard_xmit;
> +	net->open            = ks959_net_open;
> +	net->stop            = ks959_net_close;
> +	net->get_stats	     = ks959_net_get_stats;
> +	net->do_ioctl        = ks959_net_ioctl;
> +
> +	ret = register_netdev(net);
> +	if (ret != 0)
> +		goto free_mem;
> +
> +	info("IrDA: Registered KingSun KS-959 device %s", net->name);
> +
> +	usb_set_intfdata(intf, kingsun);
> +
> +	/* Situation at this point:
> +	   - all work buffers allocated
> +	   - urbs not allocated, set to NULL
> +	   - max rx packet known (is KINGSUN_FIFO_SIZE)
> +	   - unwrap state machine (partially) initialized, but skb == NULL
> +	 */
> +
> +	return 0;
> +
> +free_mem:
> +	if (kingsun->speed_setuprequest) kfree(kingsun->speed_setuprequest);
> +	if (kingsun->tx_setuprequest) kfree(kingsun->tx_setuprequest);
> +	if (kingsun->tx_buf_xored) kfree(kingsun->tx_buf_xored);
> +	if (kingsun->tx_buf_clear) kfree(kingsun->tx_buf_clear);
> +	if (kingsun->rx_setuprequest) kfree(kingsun->rx_setuprequest);
> +	if (kingsun->rx_buf) kfree(kingsun->rx_buf);
You can pass a NULL pointer to kfree().

You also have quite many lines on your patch being longer than 80 chars.
Mind fixing that as well ?

Besides those minor comments, the patch looks good to me. Let's try to
push it to netdev as soon as possible.

Cheers,
Samuel.



-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel