Thread: Re: [zd1211-devs] [PATCH] wireless-2.6 zd1211rw check against regulatory domain rather than hardcod
Status: Beta
Brought to you by:
mayne
From: Daniel D. <ds...@ge...> - 2006-10-23 22:59:04
|
Holden Karau wrote: > From: Holden Karau <ho...@pi...> http://www.holdenkarau.com > > I have made a small patch for the zd1211rw driver which uses the > boundry channels of the regulatory domain, rather than the hard coded > values of 1 & 11. > Signed-off-by: Holden Karau <ho...@pi...> > http://www.holdenkarau.com Thanks for the patch! Please always look up the MAINTAINERS entry for the code you are modifying and CC the developers on patches. Comments below, all minor points. > I'm not entirely sure how useful this patch is, but it seems like a > good idea. If its totally misguided, let me know :-) In case the patch > gets mangled I've put it up at > http://www.holdenkarau.com/~holden/projects/zd1211rw/zd1211rw-use-geo-for-channels.patch Your mailer ate tabs and wrapped long lines. You're going to need to fix that. > --- a/drivers/net/wireless/zd1211rw/zd_chip.c 2006-10-23 > 10:07:39.000000000 -0400 > +++ b/drivers/net/wireless/zd1211rw/zd_chip.c 2006-10-23 > 10:41:51.000000000 -0400 > @@ -38,6 +38,8 @@ void zd_chip_init(struct zd_chip *chip, > mutex_init(&chip->mutex); > zd_usb_init(&chip->usb, netdev, intf); > zd_rf_init(&chip->rf); > + /* The chip needs to know which geo it is in */ > + chip->geo = > ieee80211_get_geo(zd_mac_to_ieee80211(zd_netdev_mac(netdev))); There is no need to store a geo reference here. You can use zd_chip_to_mac() to go from chip to mac, then mac-to-ieee80211 is easy. > } > > void zd_chip_clear(struct zd_chip *chip) > @@ -606,14 +608,17 @@ static int patch_6m_band_edge(struct zd_ > { CR128, 0x14 }, { CR129, 0x12 }, { CR130, 0x10 }, > { CR47, 0x1e }, > }; > + struct ieee80211_geo *geo = chip->geo; > > if (!chip->patch_6m_band_edge || !chip->rf.patch_6m_band_edge) > return 0; > > - /* FIXME: Channel 11 is not the edge for all regulatory domains. */ > - if (channel == 1 || channel == 11) > + /* Checks the channel boundry of the region */ > + dev_dbg_f("checking boundry == %d || %d\n" , 1 , geo->bg_channels); > + if (channel == 1 || channel == geo->bg_channels) Typo, you mean boundary. Also, I think the debug message can go once you're confident it's working correctly. > ioreqs[0].value = 0x12; > > + This added line could go as well. > dev_dbg_f(zd_chip_dev(chip), "patching for channel %d\n", channel); > return zd_iowrite16a_locked(chip, ioreqs, ARRAY_SIZE(ioreqs)); > } I think that after the above changes, your modifications to zd_chip.h can be removed. > --- a/drivers/net/wireless/zd1211rw/zd_chip.h 2006-10-23 > 10:07:39.000000000 -0400 > +++ b/drivers/net/wireless/zd1211rw/zd_chip.h 2006-10-23 > 10:39:08.000000000 -0400 > @@ -21,6 +21,8 @@ > #include "zd_types.h" > #include "zd_rf.h" > #include "zd_usb.h" > +#include "zd_ieee80211.h" > +#include <linux/wireless.h> > > /* Header for the Media Access Controller (MAC) and the Baseband Processor > * (BBP). It appears that the ZD1211 wraps the old ZD1205 with USB glue and > @@ -669,6 +671,7 @@ struct zd_chip { > /* SetPointOFDM in the vendor driver */ > u8 ofdm_cal_values[3][E2P_CHANNEL_COUNT]; > u16 link_led; > + struct ieee80211_geo* geo; > unsigned int pa_type:4, > patch_cck_gain:1, patch_cr157:1, patch_6m_band_edge:1, > new_phy_layout:1, > - > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to maj...@vg... > More majordomo info at http://vger.kernel.org/majordomo-info.html > |
From: Daniel D. <ds...@ge...> - 2006-10-24 15:10:23
|
Holden Karau wrote: > I've changed the patch based on your suggestions :-) Thanks, looks fine. Let's just wait for an OK from Ulrich, then you can send it to John, without broken tabs/lines, with signoff and description. Daniel |
From: Uli K. <ku...@de...> - 2006-10-29 15:42:10
|
Daniel Drake wrote: > Holden Karau wrote: >> I've changed the patch based on your suggestions :-) > > Thanks, looks fine. Let's just wait for an OK from Ulrich, then you can > send it to John, without broken tabs/lines, with signoff and description. > > Daniel I'm not so sure about this. This patching might be US-specific and we cannot simply apply the setting for top channel of another domain instead of channel 11. One option would be to set the value only under the US regulatory domain. Kind regards, Uli -- Uli Kunitz (ku...@de...) |
From: Holden K. <ho...@pi...> - 2006-10-30 19:23:58
|
---------- Forwarded message ---------- From: Holden Karau <ho...@pi...> Date: Oct 30, 2006 12:49 PM Subject: Re: [PATCH] wireless-2.6 zd1211rw check against regulatory domain rather than hardcoded value of 11 To: ku...@de... Cc: Daniel Drake <ds...@ge...>, zd1...@li..., lin...@tu..., netdev <ne...@vg...>, lin...@vg..., ho...@xa..., Johannes Berg <joh...@si...> Hi Ulrich, I'm fairly certain the patch is safe, even for non-US regulatory domains. Looking at the zd1211rw code the highest channel in the geo object would appear to be set in zd_geo_init as determined by zd_channel_range which just does a simple look up in channel_ranges[], so if this code behaves correctly [and there is no indication it does not], then setting the upper channel to be that in the geo object would appear to be safe for any of the supported regulatory domains. Cheers, Holden :-) On 10/30/06, Johannes Berg <joh...@si...> wrote: > > > I'm not so sure about this. This patching might be US-specific and we > > cannot simply apply the setting for top channel of another domain > > instead of channel 11. One option would be to set the value only under > > the US regulatory domain. > > ?? > What the patch does is replace the top channel which is hardcoded to 11 > by the top channel given by the current regulatory domain. How can that > be wrong? Except that you may want to init the regulatory domain from > the EEPROM but I'm not sure how the ieee80211 code works wrt. that. > > johannes > -- Cell: 613-276-1645 -- Cell: 613-276-1645 |
From: Uli K. <ku...@de...> - 2006-10-30 22:59:47
|
Johannes Berg wrote: >> I'm not so sure about this. This patching might be US-specific and we >> cannot simply apply the setting for top channel of another domain >> instead of channel 11. One option would be to set the value only under >> the US regulatory domain. > > ?? > What the patch does is replace the top channel which is hardcoded to 11 > by the top channel given by the current regulatory domain. How can that > be wrong? Except that you may want to init the regulatory domain from > the EEPROM but I'm not sure how the ieee80211 code works wrt. that. > > johannes The problem is not so much that I don't trust the geo code, but whether setting the register to that band-edge value for a higher channel is the right thing to do. It looks like that this is a hack for FFC compliance. Therefore I suggest to patch CR128 only for the US regulatory domain. Here is the code from the GPL vendor driver (zdhw.c): if (pObj->HWFeature & BIT_21) //6321 for FCC regulation, enabled HWFeature 6M band edge bit (for AL2230, AL2230S) { if (ChannelNo == 1 || ChannelNo == 11) //MARK_003, band edge, these may depend on PCB layout { pObj->SetReg(reg, ZD_CR128, 0x12); pObj->SetReg(reg, ZD_CR129, 0x12); pObj->SetReg(reg, ZD_CR130, 0x10); pObj->SetReg(reg, ZD_CR47, 0x1E); } else //(ChannelNo 2 ~ 10, 12 ~ 14) { pObj->SetReg(reg, ZD_CR128, 0x14); pObj->SetReg(reg, ZD_CR129, 0x12); pObj->SetReg(reg, ZD_CR130, 0x10); pObj->SetReg(reg, ZD_CR47, 0x1E); } } The patch from Holden would set ZD_CR128 to 0x12 for the highest channel, which would not reflect the logic of the vendor driver. Kind regards, Uli -- Uli Kunitz (ku...@de...) |
From: Holden K. <ho...@pi...> - 2006-10-31 14:26:41
|
The patch was based off the FIXME comment in the code [which suggested replacing 11 with the boundary for the regulatory domain]. I'll take a look at the vendors driver and see about modifying the patch to fit more with its logic or just limmiting it to the FCC regulatory domain. On 10/30/06, Uli Kunitz <ku...@de...> wrote: > Johannes Berg wrote: > >> I'm not so sure about this. This patching might be US-specific and we > >> cannot simply apply the setting for top channel of another domain > >> instead of channel 11. One option would be to set the value only under > >> the US regulatory domain. > > > > ?? > > What the patch does is replace the top channel which is hardcoded to 11 > > by the top channel given by the current regulatory domain. How can that > > be wrong? Except that you may want to init the regulatory domain from > > the EEPROM but I'm not sure how the ieee80211 code works wrt. that. > > > > johannes > > The problem is not so much that I don't trust the geo code, but whether > setting the register to that band-edge value for a higher channel is > the right thing to do. It looks like that this is a hack for FFC > compliance. Therefore I suggest to patch CR128 only > for the US regulatory domain. > > Here is the code from the GPL vendor driver (zdhw.c): > > if (pObj->HWFeature & BIT_21) //6321 for FCC regulation, enabled HWFeature 6M band edge bit (for AL2230, AL2230S) > { > if (ChannelNo == 1 || ChannelNo == 11) //MARK_003, band edge, these may depend on PCB layout > { > pObj->SetReg(reg, ZD_CR128, 0x12); > pObj->SetReg(reg, ZD_CR129, 0x12); > pObj->SetReg(reg, ZD_CR130, 0x10); > pObj->SetReg(reg, ZD_CR47, 0x1E); > } > else //(ChannelNo 2 ~ 10, 12 ~ 14) > { > pObj->SetReg(reg, ZD_CR128, 0x14); > pObj->SetReg(reg, ZD_CR129, 0x12); > pObj->SetReg(reg, ZD_CR130, 0x10); > pObj->SetReg(reg, ZD_CR47, 0x1E); > } > } > > The patch from Holden would set ZD_CR128 to 0x12 for the highest channel, > which would not reflect the logic of the vendor driver. > > Kind regards, > > Uli > > -- > Uli Kunitz (ku...@de...) > - > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to maj...@vg... > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- Cell: 613-276-1645 |
From: Daniel D. <ds...@ge...> - 2006-11-01 00:36:13
|
Michael Buesch wrote: > I think the real question is: What does this "band edge" bit actually do? Not entirely sure, and I don't think we've even seen a device where this code path runs (it only runs if a certain bit is set in the EEPROM). However, considering that this looks like it plays with some kind of radio stuff, and it's simple to implement, it makes sense to at least meet the behavior of the vendor driver (as opposed to violating some weird regulation somewhere). > I don't know what channel 1 and 11 have in common. They are the edges of the channel range in most places. > Why don't we set the > bit for channel 14? Isn't that an "edge", too? The vendor driver is full of stuff like this, many corners have been cut. Chances are that they just wanted to hit the edges in the common domain while not breaking things for channel 12~14 users, and didn't go the full way of implementing it accurately. I will email the developers for clarification. Daniel |