Hi Woody
CCing users@xxxxxxxxxxxxxxxxxxxxxxx, please add this list to CC
when you will be posting next patch evalutation.
On Mon, May 28, 2012 at 03:43:32PM +0800, Woody Hung wrote:
> + rt2800_register_read(rt2x00dev, OSC_CTRL, ®);
> + rt2x00_set_field32(®, OSC_ROSC_EN, 1);
> + rt2800_register_write(rt2x00dev, OSC_CTRL, reg);
> + rt2x00_set_field32(®, OSC_ROSC_EN, 1);
> + rt2x00_set_field32(®, OSC_CAL_REQ, 1);
> + rt2x00_set_field32(®, OSC_REF_CYCLE, 0x27);
> + rt2800_register_write(rt2x00dev, OSC_CTRL, reg);
You write OSC_CTRL register twice, was that intended? If so please
comment why?
> + rt2800_register_read(rt2x00dev, COEX_CFG0, ®);
> + rt2x00_set_field32(®, COEX_CFG_ANT, 0);
> + rt2x00_set_field32(®, COEX_CFG_ANT, 0x5e);
> + rt2800_register_write(rt2x00dev, COEX_CFG0, reg);
One COEX_CFG_ANT set is unneeded, perhaps you wanted to set some other
field.
> + rt2800_register_write(rt2x00dev, COEX_CFG2, 0x0017937F);
What this magic number mean?
> + if (rt2x00_rt(rt2x00dev, RT3290))
> + rt2800_register_write(rt2x00dev, TX_SW_CFG0,
> + 0x00000404);
> + else
> + rt2800_register_write(rt2x00dev, TX_SW_CFG0,
> + 0x00000400);
Would be nice if last argument will be aligned to the "(" bracket.
> + rt2800_bbp_read(rt2x00dev, 47, &value);
> + rt2x00_set_field8(&value, RFCSR2_RESCAL_EN, 1);
> + rt2800_bbp_write(rt2x00dev, 47, value);
> +
> + rt2800_bbp_read(rt2x00dev, 3, &value);
> + rt2x00_set_field8(&value, RFCSR7_BITS67, 1);
> + rt2800_bbp_write(rt2x00dev, 3, value);
You use RFCSR2_ and RFCSR7_ defines for BBP registers 47 and 3. Assuming
values are correct, they need proper BBP_ defines (with descriptive
name, not like BBP3_BITS67)
> + if (rt2x00_rt(rt2x00dev, RT3290)) {
> + rt2800_rfcsr_read(rt2x00dev, 29, &rfcsr);
> + rt2x00_set_field8(&rfcsr, RFCSR27_ADC, 0xc0);
> + rt2800_rfcsr_write(rt2x00dev, 29, rfcsr);
Same here: RFCSR29 reg use RFCSR27 value.
> +static int rt2800_enable_wlan_rt3290(struct rt2x00_dev *rt2x00dev)
> +{
> + u32 reg;
> + int i, count;
> +
> + rt2800_register_read(rt2x00dev, WLAN_FUN_CTRL, ®);
> + rt2x00_set_field32(®, WLAN_GPIO_OUT_OE_BIT_ALL, 0xff);
> + rt2x00_set_field32(®, FRC_WL_ANT_SET, 1);
> + if ((rt2x00_get_field32(reg, WLAN_EN) == 1))
> + return 0;
This check should be before two rt2x00_set_field32(), otherwise
looks like rt2800_register_write() is missing.
> + count = 0;
> + do {
> + reg = 0;
This must be not needed, otherwise we check register values, which we do
not read from hardware.
> + /*
> + * Check PLL_LD & XTAL_RDY.
> + */
> + for (i = 0; i < REGISTER_BUSY_COUNT; i++) {
> + rt2800_register_read(rt2x00dev, CMB_CTRL, ®);
> + if ((rt2x00_get_field32(reg, PLL_LD) == 1) &&
> + (rt2x00_get_field32(reg, XTAL_RDY) == 1))
Alignment. And also brackets are unneeded, i.e. use (x == 1 && y == 1)
instead of ((x == 1) && (y == 1)).
> + break;
> + udelay(REGISTER_BUSY_DELAY);
> + }
> +
> + if (i >= REGISTER_BUSY_COUNT) {
> +
> + if (count >= 10)
> + break;
Shouldn't we return error if device fail to initialize PLL & XTAL ? Upper
functions probably should then print error and stop initialization i.e.
make ->pci_probe function fail and disallow to use this this device by
driver.
> + rt2800_register_write(rt2x00dev, 0x58, 0x018);
> + udelay(REGISTER_BUSY_DELAY);
> + rt2800_register_write(rt2x00dev, 0x58, 0x418);
> + udelay(REGISTER_BUSY_DELAY);
> + rt2800_register_write(rt2x00dev, 0x58, 0x618);
> + udelay(REGISTER_BUSY_DELAY);
> + count++;
> + } else {
> + rt2800_register_read(rt2x00dev,
> + WPDMA_GLO_CFG, ®);
Not needed line break.
> + rt2800_register_write(rt2x00dev,
> + INT_SOURCE_CSR, 0x7fffffff);
Not needed line break.
Thanks
Stanislaw
--
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
