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/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
