On Wed, 2012-11-28 at 11:42 +0200, Arik Nemtsov wrote:
> From: Igal Chernobelsky <igalc@xxxxxx>
>
> FW memory block size and FW log end marker parameters
> are added to wl structure and are initialized per
> chip architecture. convert_hwaddr hw operation is added
> to convert chip dependent FW internal address.
> Copy from FW log is also simplified to copy the entire
> memory block as FW logger utility is repsponsible
> for parsing of FW log content.
>
> Signed-off-by: Igal Chernobelsky <igalc@xxxxxx>
> Signed-off-by: Arik Nemtsov <arik@xxxxxxxxxx>
> ---
This commit log explains what has been done, which can quite easily be
see in the patch itself. That's okay, but what I'm missing here is the
explanation *why* this needs to be done.
> diff --git a/drivers/net/wireless/ti/wl12xx/main.c b/drivers/net/wireless/ti/wl12xx/main.c
> index 951b88c..5e3c808 100644
> --- a/drivers/net/wireless/ti/wl12xx/main.c
> +++ b/drivers/net/wireless/ti/wl12xx/main.c
> @@ -706,6 +706,9 @@ static int wl12xx_identify_chip(struct wl1271 *wl)
> goto out;
> }
>
> + wl->fw_mem_block_size = 256;
> + wl->fwlog_end = 0x2000000;
This value used to be in a macro before (WLCORE_FW_LOG_END), why kill
it? It should just be renamed to WL12XX_FW_LOG_END and a new one be
created for WL18XX.
> @@ -1632,6 +1635,11 @@ static int wl12xx_set_peer_cap(struct wl1271 *wl,
> hlid);
> }
>
> +static u32 wl12xx_convert_hwaddr(struct wl1271 *wl, u32 hwaddr)
> +{
> + return hwaddr << 5;
> +}
There was a good comment about this calculation before, it should be
moved here instead of deleted.
> @@ -1669,6 +1677,7 @@ static struct wlcore_ops wl12xx_ops = {
> .channel_switch = wl12xx_cmd_channel_switch,
> .pre_pkt_send = NULL,
> .set_peer_cap = wl12xx_set_peer_cap,
> + .convert_hwaddr = wl12xx_convert_hwaddr,
No need to break the alignment here.
> static struct ieee80211_sta_ht_cap wl12xx_ht_cap = {
> diff --git a/drivers/net/wireless/ti/wl18xx/main.c b/drivers/net/wireless/ti/wl18xx/main.c
> index df8de71..d806241 100644
> --- a/drivers/net/wireless/ti/wl18xx/main.c
> +++ b/drivers/net/wireless/ti/wl18xx/main.c
> @@ -667,6 +667,9 @@ static int wl18xx_identify_chip(struct wl1271 *wl)
> goto out;
> }
>
> + wl->fw_mem_block_size = 272;
> + wl->fwlog_end = 0x40000000;
Again, the magic number here can be avoided.
> @@ -1423,6 +1426,11 @@ static int wl18xx_set_peer_cap(struct wl1271 *wl,
> rate_set, hlid);
> }
>
> +static u32 wl18xx_convert_hwaddr(struct wl1271 *wl, u32 hwaddr)
> +{
> + return hwaddr & ~0x80000000;
> +}
A small explanation here would be nice.
[...]
> /* Traverse the memory blocks linked list */
> do {
> - memset(block, 0, WL12XX_HW_BLOCK_SIZE);
> - ret = wlcore_read_hwaddr(wl, addr, block, WL12XX_HW_BLOCK_SIZE,
> - false);
> + part.mem.start = wlcore_hw_convert_hwaddr(wl, addr);
> + part.mem.size = PAGE_SIZE;
> +
> + ret = wlcore_set_partition(wl, &part);
> + if (ret < 0) {
> + wl1271_error("%s: set_partition start=0x%X size=%d",
> + __func__, part.mem.start, part.mem.size);
This could probably be a bit more descriptive, like saying that the
fwlog seems to be corrupt because the address in the linked list can't
be used for setting the partition.
> diff --git a/drivers/net/wireless/ti/wlcore/io.h b/drivers/net/wireless/ti/wlcore/io.h
> index f48530f..5897747 100644
> --- a/drivers/net/wireless/ti/wlcore/io.h
> +++ b/drivers/net/wireless/ti/wlcore/io.h
> @@ -165,8 +165,8 @@ static inline int __must_check wlcore_read_hwaddr(struct wl1271 *wl, int hwaddr,
> int physical;
> int addr;
>
> - /* Addresses are stored internally as addresses to 32 bytes blocks */
> - addr = hwaddr << 5;
> + /* Convert from FW internal address which is chip arch dependent */
> + addr = wl->ops->convert_hwaddr(wl, hwaddr);
This could be more descriptive. As I understand, this is converting an
internal representation of pointers in the hardware to an address that
can be used to set the partitions.
> diff --git a/drivers/net/wireless/ti/wlcore/wlcore_i.h b/drivers/net/wireless/ti/wlcore/wlcore_i.h
> index a664662..a8647bd 100644
> --- a/drivers/net/wireless/ti/wlcore/wlcore_i.h
> +++ b/drivers/net/wireless/ti/wlcore/wlcore_i.h
> @@ -523,6 +523,4 @@ void wl1271_rx_filter_flatten_fields(struct wl12xx_rx_filter *filter,
> #define HW_HT_RATES_OFFSET 16
> #define HW_MIMO_RATES_OFFSET 24
>
> -#define WL12XX_HW_BLOCK_SIZE 256
I think this should also be created separately for wl12xx and wl18xx
instead of deleting the macro and hardcoding the values.
--
Luca.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
