[PATCH] libertas: provide reset_card() callback on OLPC

[Andres Salomon]

Awesome, I'll be happy to see this patch upstream.  Comments below.


On Tue, 20 May 2008 16:48:52 +0100
David Woodhouse <dwmw2 at infradead.org> wrote:

> Signed-off-by: David Woodhouse <dwmw2 at infradead.org>
> 
> diff --git a/drivers/net/wireless/libertas/if_usb.c
> b/drivers/net/wireless/libertas/if_usb.c index 8032df7..8a7eb63 100644
> --- a/drivers/net/wireless/libertas/if_usb.c
> +++ b/drivers/net/wireless/libertas/if_usb.c
> @@ -7,6 +7,10 @@
>  #include <linux/netdevice.h>
>  #include <linux/usb.h>
>  
> +#ifdef CONFIG_OLPC
> +#include <asm/olpc.h>
> +#endif
> +
>  #define DRV_NAME "usb8xxx"
>  
>  #include "host.h"
> @@ -146,6 +150,14 @@ static void if_usb_fw_timeo(unsigned long priv)
>  	wake_up(&cardp->fw_wq);
>  }
>  
> +#ifdef CONFIG_OLPC
> +static int if_usb_reset_olpc_card(struct lbs_private *priv)
> +{
> +	printk(KERN_CRIT "Resetting OLPC wireless via EC...\n");
> +	olpc_ec_cmd(0x25, NULL, 0, NULL, 0);
> +}
> +#endif


Can you please add '#define EC_WIRELESS_RESET 0x25' (or whatever you want
to call it) to include/asm-x86/olpc.h, and use EC_WIRELESS_RESET here
rather than 0x25?  All EC commands should end up there.


> +
>  /**
>   *  @brief sets the configuration values
>   *  @param ifnum	interface number
> @@ -231,6 +243,11 @@ static int if_usb_probe(struct usb_interface
> *intf, cardp->priv->fw_ready = 1;
>  
>  	priv->hw_host_to_card = if_usb_host_to_card;
> +#ifdef CONFIG_OLPC
> +	if (machine_is_olpc())
> +		priv->reset_card = if_usb_reset_olpc_card;
> +#endif
> +

So, um, what happens if I plug a libertas dongle into an XO?  Is
machine_is_olpc() really enough to ensure that we want to reset this
specific libertas device?



>  	cardp->boot2_version = udev->descriptor.bcdDevice;
>  
>  	if_usb_submit_rx_urb(cardp);
> @@ -364,6 +381,11 @@ static int if_usb_reset_device(struct
> if_usb_card *cardp) ret = usb_reset_device(cardp->udev);
>  	msleep(100);
>  
> +#ifdef CONFIG_OLPC
> +	if (ret && machine_is_olpc())
> +		if_usb_reset_olpc_card(NULL);
> +#endif
> +

Any particular reason why we need the ifdef?  I'd prefer the following, if
it's a safe assumption to make:

if (ret && priv->reset_card)
	priv->reset_card(NULL);




>  	lbs_deb_leave_args(LBS_DEB_USB, "ret %d", ret);
>  
>  	return ret;