diff mbox

ar9170: implement frequency calibration for one-stage/openfw

Message ID 20090928184111.GB4737@tuxdriver.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

John W. Linville Sept. 28, 2009, 6:41 p.m. UTC 
On Sat, Sep 19, 2009 at 02:21:36PM +0200, Joerg Albert wrote:
> On 09/19/2009 02:02 AM, Andrew Morton wrote:
> > On Thu, 3 Sep 2009 20:25:31 +0200
> > Christian Lamparter <chunkeey@googlemail.com> wrote:
> > 
> >> This patch ports some code from the vendor driver, which is
> >> supposed to upload the right calibration values for the
> >> chosen frequency.
> >>
> >> In theory, this should give a better range and throughput
> >> for all users with the open, or one-stage firmware.
> >>
> >> ...
> >>
> >> +		for (i = 0; i < 76; i++) {
> >> +			u32 phy_data;
> >> +			u8 tmp;
> >> +
> >> +			if (i < 25) {
> >> +				tmp = ar9170_interpolate_val(i, &pwrs[0][0],
> >> +							     &vpds[0][0]);
> >> +			} else {
> >> +				tmp = ar9170_interpolate_val(i - 12,
> >> +							     &pwrs[1][0],
> >> +							     &vpds[1][0]);
> >> +			}
> >> +
> >> +			phy_data |= tmp << ((i & 3) << 3);
> > 
> > Clearly buggy and the compiler warns.  The value of phy_data is unknown
> > here.
> > 
> > How did this get all the way into mainline?
> 
> Strangely it compiles without any warning for me with the latest linux-wireless:

That is strange -- no warning here (F-11) either...

Christian, care to propose a patch?  Or maybe just something like this?


John

Comments

Christian Lamparter Sept. 28, 2009, 7:45 p.m. UTC | #1 
2009/9/28 John W. Linville <linville@tuxdriver.com>:
> On Sat, Sep 19, 2009 at 02:21:36PM +0200, Joerg Albert wrote:
>> On 09/19/2009 02:02 AM, Andrew Morton wrote:
>> > On Thu, 3 Sep 2009 20:25:31 +0200
>> > Christian Lamparter <chunkeey@googlemail.com> wrote:
>> >
>> >> This patch ports some code from the vendor driver, which is
>> >> supposed to upload the right calibration values for the
>> >> chosen frequency.
>> >>
>> >> In theory, this should give a better range and throughput
>> >> for all users with the open, or one-stage firmware.
>> >>
>> >> ...
>> >>
>> >> +          for (i = 0; i < 76; i++) {
>> >> +                  u32 phy_data;
>> >> +                  u8 tmp;
>> >> +
>> >> +                  if (i < 25) {
>> >> +                          tmp = ar9170_interpolate_val(i, &pwrs[0][0],
>> >> +                                                       &vpds[0][0]);
>> >> +                  } else {
>> >> +                          tmp = ar9170_interpolate_val(i - 12,
>> >> +                                                       &pwrs[1][0],
>> >> +                                                       &vpds[1][0]);
>> >> +                  }
>> >> +
>> >> +                  phy_data |= tmp << ((i & 3) << 3);
>> >
>> > Clearly buggy and the compiler warns.  The value of phy_data is unknown
>> > here.
>> >
>> > How did this get all the way into mainline?
>>
>> Strangely it compiles without any warning for me with the latest linux-wireless:
>
> That is strange -- no warning here (F-11) either...
>
> Christian, care to propose a patch?  Or maybe just something like this?
>
> diff --git a/drivers/net/wireless/ath/ar9170/phy.c b/drivers/net/wireless/ath/ar9170/phy.c
> index b3e5cf3..49c10cb 100644
> --- a/drivers/net/wireless/ath/ar9170/phy.c
> +++ b/drivers/net/wireless/ath/ar9170/phy.c
> @@ -1220,7 +1220,7 @@ static int ar9170_set_freq_cal_data(struct ar9170 *ar,
>                                                             &vpds[1][0]);
>                        }
>
> -                       phy_data |= tmp << ((i & 3) << 3);
> +                       phy_data = tmp << ((i & 3) << 3);
>                        if ((i & 3) == 3) {
>                                ar9170_regwrite(0x1c6280 + chain * 0x1000 +
>                                                (i & ~3), phy_data);
>
> John
no? Andrew Morton's mail included a tiny patch which initializes the
phy_data to 0. I assumed you would pick it up right away...
Or is there something which would prevent it from including?

Regards,
         Chr
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
John W. Linville Sept. 28, 2009, 7:50 p.m. UTC | #2 
On Mon, Sep 28, 2009 at 09:45:28PM +0200, Christian Lamparter wrote:
> 2009/9/28 John W. Linville <linville@tuxdriver.com>:
> > On Sat, Sep 19, 2009 at 02:21:36PM +0200, Joerg Albert wrote:
> >> On 09/19/2009 02:02 AM, Andrew Morton wrote:

> >> > How did this get all the way into mainline?
> >>
> >> Strangely it compiles without any warning for me with the latest linux-wireless:
> >
> > That is strange -- no warning here (F-11) either...
> >
> > Christian, care to propose a patch?  Or maybe just something like this?
> >
> > diff --git a/drivers/net/wireless/ath/ar9170/phy.c b/drivers/net/wireless/ath/ar9170/phy.c
> > index b3e5cf3..49c10cb 100644
> > --- a/drivers/net/wireless/ath/ar9170/phy.c
> > +++ b/drivers/net/wireless/ath/ar9170/phy.c
> > @@ -1220,7 +1220,7 @@ static int ar9170_set_freq_cal_data(struct ar9170 *ar,
> >                                                             &vpds[1][0]);
> >                        }
> >
> > -                       phy_data |= tmp << ((i & 3) << 3);
> > +                       phy_data = tmp << ((i & 3) << 3);
> >                        if ((i & 3) == 3) {
> >                                ar9170_regwrite(0x1c6280 + chain * 0x1000 +
> >                                                (i & ~3), phy_data);
> >
> > John
> no? Andrew Morton's mail included a tiny patch which initializes the
> phy_data to 0. I assumed you would pick it up right away...
> Or is there something which would prevent it from including?

Don't know what email you got, but there was no patch in my inbox...

So, I presume you have no objection to the patch above?  Initializing
phy_data just to unconditionally assign to it doesn't seem any better.

John
diff mbox

Patch

diff --git a/drivers/net/wireless/ath/ar9170/phy.c b/drivers/net/wireless/ath/ar9170/phy.c
index b3e5cf3..49c10cb 100644
--- a/drivers/net/wireless/ath/ar9170/phy.c
+++ b/drivers/net/wireless/ath/ar9170/phy.c
@@ -1220,7 +1220,7 @@  static int ar9170_set_freq_cal_data(struct ar9170 *ar,
 							     &vpds[1][0]);
 			}
 
-			phy_data |= tmp << ((i & 3) << 3);
+			phy_data = tmp << ((i & 3) << 3);
 			if ((i & 3) == 3) {
 				ar9170_regwrite(0x1c6280 + chain * 0x1000 +
 						(i & ~3), phy_data);