Re: [rt2x00-users] [PATCH 2/4] rt2800: radio 3xxx: program RF_R1 during channel switch

[Stanislaw Gruszka <sgruszka@redhat.com>]

On Mon, Jan 30, 2012 at 11:06:49PM +0100, Gertjan van Wingerde wrote:
> On 01/30/12 16:17, Stanislaw Gruszka wrote:
> > +	rt2800_rfcsr_read(rt2x00dev, 1, &rfcsr);
> > +	rt2x00_set_field8(&rfcsr, RFCSR1_RX0_PD, 0);
> > +	rt2x00_set_field8(&rfcsr, RFCSR1_TX0_PD, 0);
> > +	if (rt2x00_rt(rt2x00dev, RT3390)) {
> > +		rt2x00_set_field8(&rfcsr, RFCSR1_RX1_PD,
> > +				  rt2x00dev->default_ant.rx_chain_num == 1);
> > +		rt2x00_set_field8(&rfcsr, RFCSR1_TX1_PD,
> > +				  rt2x00dev->default_ant.tx_chain_num == 1);
> > +	} else {
> > +		rt2x00_set_field8(&rfcsr, RFCSR1_RX1_PD, 0);
> > +		rt2x00_set_field8(&rfcsr, RFCSR1_TX1_PD, 0);
> > +		rt2x00_set_field8(&rfcsr, RFCSR1_RX2_PD, 0);
> > +		rt2x00_set_field8(&rfcsr, RFCSR1_TX2_PD, 0);
> > +
> > +		switch (rt2x00dev->default_ant.tx_chain_num) {
> > +		case 1:
> > +			rt2x00_set_field8(&rfcsr, RFCSR1_TX1_PD, 1);
> > +			/* fall through */
> > +		case 2:
> > +			rt2x00_set_field8(&rfcsr, RFCSR1_TX2_PD, 1);
> > +			break;
> > +		}
> > +
> > +		switch (rt2x00dev->default_ant.rx_chain_num) {
> > +		case 1:
> > +			rt2x00_set_field8(&rfcsr, RFCSR1_RX1_PD, 1);
> > +			/* fall through */
> > +		case 2:
> > +			rt2x00_set_field8(&rfcsr, RFCSR1_RX2_PD, 1);
> > +			break;
> > +		}
> > +	}
> > +	rt2800_rfcsr_write(rt2x00dev, 1, rfcsr);
> > +
> >  	rt2800_rfcsr_read(rt2x00dev, 23, &rfcsr);
> >  	rt2x00_set_field8(&rfcsr, RFCSR23_FREQ_OFFSET, rt2x00dev->freq_offset);
> >  	rt2800_rfcsr_write(rt2x00dev, 23, rfcsr);
> 
> To be honest, I think that this can be simplied to a single case for
> both RT30xx and RT33xx. Just take the RT30xx branch of the added
> if-statement and it should just work fine on both chipset families.
> 
> Yes, I am aware the Ralink driver has slightly different code here, but
> that just seems to be because they work with knowledge of the
> limitations of RT33xx, which ensures that tx_chain_num and rx_chain_num
> can never be 2 on that chipset, but still handling it doesn't harm. It
> would merely result in better readable code.

Not only the code is different, but RF_R1 register value we program
is different for 30xx and 33xx when chain_num == 1 (changed by
RFCSR1_{RX2,TX2)_PD bit).

I'm not against merging these two cases and program different values
into register than Ralink driver do, but maybe in the next linux
release (counting from release of that change), so any breakage
eventually caused by that merge could be easily detected.

Stanislaw