On Monday 23 of March 2009 13:13:10 Holger Schurig wrote:
> Hi Marek !
>
> Great.
>
>
> Some nitpicks:
>
> 1. you attached the patches, not inline
Yes, I know, I just needed some comments if they are OK
> 2. There's no signed-off
cat ../0001-Correct-return-value-of-firmware-loading-functions-h.patch |
head -n 6
~~~~~~~
Signed-off-by: Marek Vasut <marek.vasut@xxxxxxxxx>
cat ../0002-Add-support-for-CF8381-WiFi-card.patch | head -n 6
~~~~~~~
Signed-off-by: Marek Vasut <marek.vasut@xxxxxxxxx>
So as you see, there is.
> 3. You didn't submit one mail per patch
Yes, I know, see 1.
>
> All of this makes review harder. If you adhere a bit to
> Documentation/SubmittingPatches you'd get the fame all by
> yourself :-)
>
>
> Some more rants:
>
>
> --- a/drivers/net/wireless/libertas/if_cs.c
> +++ b/drivers/net/wireless/libertas/if_cs.c
> @@ -273,7 +273,14 @@ static int if_cs_poll_while_fw_download(struct
> if_cs_card *card, uint addr, u8 r */
> #define IF_CS_PRODUCT_ID 0x0000001C
> #define IF_CS_CF8385_B1_REV 0x12
> +#define IF_CS_CF8381_B3_REV 0x04
>
> Here's a tab, not a space. Maybe you run scripts/checkpatch.pl on it as
> well?
# ./scripts/checkpatch.pl ../0001-Correct-return-value-of-firmware-loading-functions-h.patch
total: 0 errors, 0 warnings, 11 lines checked
../0001-Correct-return-value-of-firmware-loading-functions-h.patch has no
obvious style problems and is ready for submission.
# ./scripts/checkpatch.pl ../0002-Add-support-for-CF8381-WiFi-card.patch
total: 0 errors, 0 warnings, 31 lines checked
../0002-Add-support-for-CF8381-WiFi-card.patch has no obvious style problems
and is ready for submission.
As you can see, I did. Otherwise I wont send them.
>
>
>
>
> - if (if_cs_read8(card, IF_CS_PRODUCT_ID) < IF_CS_CF8385_B1_REV) {
> + if (!(p_dev->manf_id == CF8381_MANFID &&
> + p_dev->card_id == CF8381_CARDID) &&
> + (if_cs_read8(card, IF_CS_PRODUCT_ID) <
> IF_CS_CF8385_B1_REV)) {
>
> I think change then breaks the existing logic for 8385 cards.
> code broke then the 8385 case. Therefore a NAK for now.
This might need review ... hang on, I'll recheck
>
>
>
>
> Kindly repost both patches again, with signed-off and fixes, thanks!
--
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
