Re: [PATCH] ath10k: improve vdev map handling.

[Kalle Valo <kvalo@qca.qualcomm.com>]

Ben Greear <greearb@candelatech.com> writes:

> On 05/16/2014 06:37 AM, Kalle Valo wrote:
>
>>> -	ar->free_vdev_map &= ~BIT(arvif->vdev_id);
>>> +	ar->free_vdev_map &= ~(1 << arvif->vdev_id);
>> 
>> Why remove the BIT()? Not that it matters much, I just think it's easier
>> to read when BIT() macro is used. Would be good to convert all cases to
>> use BIT anyway, but that's for a separate patch.
>
> BIT doesn't work on 64-bit numbers (ie, if vdev_id > 31)

Oh, I didn't know that. Too bad, but then removing it makes sense.

> and it takes a long time to figure out exactly what it does (try
> grepping for BIT). Open-coding means much easier to fully understand
> the code.

All Linux engineers should know what BIT() does. If not, they should
learn that ;)

>>> -	ar->free_vdev_map |= 1 << (arvif->vdev_id);
>>> +	ar->free_vdev_map |= (1 << arvif->vdev_id);
>> 
>> Do we need the parenthesis?
>
> No, though I like them visually.  It's at least more useful than
> the previous placement.
>
> I can respin the patch w/out them and with the == 0 and such.

Thanks.

-- 
Kalle Valo