Hi Alex,
The code looks quite good to me. Some minor comments though:
On Mon, May 07, 2007 at 11:19:17AM -0500, Alex Villacis Lasso wrote:
> +static int qos_mtt_bits = 0x07 /* > 1ms */ ;
> +module_param(qos_mtt_bits, int, 0);
> +MODULE_PARM_DESC(qos_mtt_bits, "Minimum Turn Time");
Why is that module parameter needed ? Did you have to tune it in order to get
the driver working ? If so, is the default value ane empiric one, or is it
extracted from e.g. what the windows driver send during link negociation ?
> + /* Calculate how much data can be transmitted in this urb */
> + usb_fill_int_urb(kingsun->tx_urb, kingsun->usbdev,
> + usb_sndintpipe(kingsun->usbdev, kingsun->ep_out),
> + kingsun->out_buf, wraplen, kingsun_send_irq,
> + kingsun, 1);
> +
> + if ((ret = usb_submit_urb(kingsun->tx_urb, GFP_ATOMIC))) {
> + IRDA_ERROR("kingsun_hard_xmit: failed tx_urb submit: %d", ret);
If you decided to use the USB debug routines, then this should be an err()
rather than an IRDA_ERROR().
> +/* Receive callback function */
> +static void kingsun_rcv_irq(struct urb *urb)
> +{
> + struct kingsun_cb *kingsun = urb->context;
> + int ret;
> +
> + /* in process of stopping, just drop data */
> + if (!netif_running(kingsun->netdev)) {
> + kingsun->receiving = 0;
> + return;
> + }
> +
> + /* unlink, shutdown, unplug, other nasties */
> + if (urb->status != 0) {
> + err("kingsun_rcv_irq: urb asynchronously failed - %d",
> urb->status);
> + kingsun->receiving = 0;
> + return;
> + }
> +
> + if (urb->actual_length == kingsun->max_rx) {
> + __u8 *bytes = urb->transfer_buffer;
> + int i;
> +
> + /* The very first byte in the buffer indicates the length of
> valid
> + data in the read. This byte must be in the range
> + 1..kingsun->max_rx -1 . Values outside this range indicate an
> + out-of-reach remote side (FIXME: any way to indicate this
> + condition to the IrLAP layer?) */
So, the dongle sends an URB with an out of range length when it times out on a
read ? If that's so, there is no need to notify the IrLAP layer. Be it in
primary or secondary mode, the IrLAP layer will eventually time out, disconnect
from the peer, and go back to NDM.
> +
> + printk(KERN_INFO "KingSun 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);
> +
> + /* That's the Rx capability. */
> + kingsun->qos.baud_rate.bits &= IR_9600;
So, here you never managed to connect ot this dongle at more than 9600 ?
Just checking: have you tried connecting with e.g. an IrDA phone in primary
mode to this dongle ? Was it always falling back to 9600 ?
I'm asking that because if we set those bits to IR_9600 only, then for sure
we will never connect at more than that.
> diff -urN linux-2.6.21-old/drivers/net/irda/Makefile
> linux-2.6.21/drivers/net/irda/Makefile
> --- linux-2.6.21-old/drivers/net/irda/Makefile 2007-05-06
> 16:54:43.000000000 -0500
> +++ linux-2.6.21/drivers/net/irda/Makefile 2007-05-06 17:11:11.000000000
> -0500
> @@ -45,6 +45,7 @@
> obj-$(CONFIG_ACT200L_DONGLE) += act200l-sir.o
> obj-$(CONFIG_MA600_DONGLE) += ma600-sir.o
> obj-$(CONFIG_TOIM3232_DONGLE) += toim3232-sir.o
> +obj-$(CONFIG_KINGSUN_SIR) += kingsun.o
To be consistent here:
+obj-$(CONFIG_KINGSUN_DONGLE) += kingsun-sir.o
even though this driver does not use the irtty-sir driver.
Overall, this looks quite good. I'll be happy to push it upstream once we fix
the small issues I pointed out.
Thanks for your time.
Cheers,
Samuel.
-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
