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

[Gertjan van Wingerde <gwingerde@gmail.com>]

On Tue, Jan 31, 2012 at 9:14 AM, Stanislaw Gruszka <sgruszka@redhat.com> wrote:
> 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.

OK. Indeed let's do this in a different commit, at least. I don't know
if it has to be a
different kernel release, as long as we are able to bisect it.

Acked-by: Gertjan van Wingerde <gwingerde@gmail.com>

for this patch as well.

---
Gertjan