Re: [PATCH] Marvell CF8381

[Marek Vasut <marek.vasut@gmail.com>]

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@gmail.com>

cat ../0002-Add-support-for-CF8381-WiFi-card.patch | head -n 6
~~~~~~~
Signed-off-by: Marek Vasut <marek.vasut@gmail.com>

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!