From: Holden Karau <[email protected]> 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 <[email protected]> http://www.holdenkarau.com
---
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
And now for the patch:
--- 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)));
}
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)
ioreqs[0].value = 0x12;
+
dev_dbg_f(zd_chip_dev(chip), "patching for channel %d\n", channel);
return zd_iowrite16a_locked(chip, ioreqs, ARRAY_SIZE(ioreqs));
}
--- 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,
Holden Karau wrote:
> From: Holden Karau <[email protected]> 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 <[email protected]>
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
I've changed the patch based on your suggestions :-)
Hopefully my mailer doesn't eat the tabs this time, if it does I've
put the patch up at
http://www.holdenkarau.com/~holden/projects/zd1211rw/zd1211rw-use-geo-for-channels-r2.patch
On 10/23/06, Daniel Drake <[email protected]> wrote:
> Holden Karau wrote:
> > From: Holden Karau <[email protected]> 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 <[email protected]>
> > 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.
>
I looked at the mainteners file, but the zd1211rw website (
http://zd1211.ath.cx/wiki/DriverRewrite ) says not to e-mail you guys
directly, rather use the mailing list.
> 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.
>
whoops. Sorry about 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.
>
That is much cleaner. Thanks :-)
> > }
> >
> > 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.
Yup. Much cleaner :-)
>
> > --- 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 [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
>
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
The new patch :
--- a/wireless-2.6/drivers/net/wireless/zd1211rw/zd_chip.c 2006-10-23
10:07:39.000000000 -0400
+++ b/wireless-2.6/drivers/net/wireless/zd1211rw/zd_chip.c 2006-10-24
09:39:10.000000000 -0400
@@ -606,12 +606,13 @@ static int patch_6m_band_edge(struct zd_
{ CR128, 0x14 }, { CR129, 0x12 }, { CR130, 0x10 },
{ CR47, 0x1e },
};
+ struct ieee80211_geo *geo =
ieee80211_get_geo(zd_mac_to_ieee80211(zd_chip_to_mac(chip)));
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 boundary of the region */
+ if (channel == 1 || channel == geo->bg_channels)
ioreqs[0].value = 0x12;
dev_dbg_f(zd_chip_dev(chip), "patching for channel %d\n", channel);
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
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 ([email protected])
> 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
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 <[email protected]> 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
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 ([email protected])
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 <[email protected]> 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 ([email protected])
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Cell: 613-276-1645
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
On Monday 30 October 2006 23:59, Uli Kunitz 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.
I think the real question is: What does this "band edge" bit actually do?
Did you notice any difference when setting it? Does TX power in/decrease?
Did you see differences in the physical range (max distance from the AP
for which you're still able to connect).
I don't know what channel 1 and 11 have in common. Why don't we set the
bit for channel 14? Isn't that an "edge", too?
--
Greetings Michael.