Re: [PATCH 3/4] staging: r8712u: Switch driver to use external firmware from linux-firmware

[Stefan Lippers-Hollmann]

Hi

[ Sorry if you already handled these comments in the fourth patch of 
  your series, but as patch 4/4 doesn't appear to have reached lkml 
  yet, I'll respond here. ]

On Friday 14 January 2011, Larry Finger wrote:
> Signed-off-by: Larry Finger <Larry.Finger@xxxxxxxxxxxx>
> ---
>  drivers/staging/rtl8712/TODO       |    2 --
>  drivers/staging/rtl8712/hal_init.c |   22 +++++++++++++++++-----
>  2 files changed, 17 insertions(+), 7 deletions(-)
[...]
> @@ -40,11 +39,24 @@
>  static u32 rtl871x_open_fw(struct _adapter *padapter, void **pphfwfile_hdl,
>  		    const u8 **ppmappedfw)
>  {
> -	u32 len;
> +	int rc;
> +	const char firmware_file[] = "rtl8712u/rtl8712u.bin";

rtl8712u.bin has been merged into linux-firmware.git as 
rtlwifi/rtl8712u.bin, wouldn't it be better to use that location 
instead?

const char firmware_file[] = "rtlwifi/rtl8712u.bin";

> +	const struct firmware **praw = (const struct firmware **)
> +				       (pphfwfile_hdl);
> +	struct dvobj_priv *pdvobjpriv = (struct dvobj_priv *)
> +					(&padapter->dvobjpriv);
> +	struct usb_device *pusbdev = pdvobjpriv->pusbdev;
>  
> -	*ppmappedfw = f_array;
> -	len = sizeof(f_array);
> -	return len;
> +	printk(KERN_INFO "r8712u: Loading firmware from \"%s\"\n",
> +	       firmware_file);
> +	rc = request_firmware(praw, firmware_file, &pusbdev->dev);
> +	if (rc < 0) {
> +		printk(KERN_ERR "r8712u: Unable to load firmware\n");
> +		printk(KERN_ERR "r8712u: Install latest linux-firmware\n");
> +		return 0;
> +	}
> +	*ppmappedfw = (u8 *)((*praw)->data);
> +	return (*praw)->size;
>  }

Additionally I'd suggest to declare the firmware as well, so that 
userspace knows about it, just like selecting FW_LOADER.

MODULE_FIRMWARE("rtlwifi/rtl8712u.bin");

>  
>  static void fill_fwpriv(struct _adapter *padapter, struct fw_priv *pfwpriv)
> 

--- a/drivers/staging/rtl8712/Kconfig
+++ b/drivers/staging/rtl8712/Kconfig
@@ -3,6 +3,7 @@ config R8712U
 	depends on WLAN && USB
 	select WIRELESS_EXT
 	select WEXT_PRIV
+	select FW_LOADER
 	default N
 	---help---
 	This option adds the Realtek RTL8712 USB device such as the D-Link DWA-130.

I'll post an according patch as follow up to this mail, diff'ed against
next-20110114 plus your 3/4 patches that reached lkml:
+ [PATCH 1/4] staging: r8712u: Fix memory leak in firmware loading
+ [PATCH 2/4] staging: r8712u: Fix sparse message
+ [PATCH 3/4] staging: r8712u: Switch driver to use external firmware from linux-firmware

Feel free to merge those changes into your "r8712u: Switch driver to 
use external firmware from linux-firmware", if you like.

Regards
	Stefan Lippers-Hollmann
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/