Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: kvalo=codeaurora.org@mg.codeaurora.org
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 6665FC433F1
From:   Kalle Valo <kvalo@codeaurora.org>
To:     <Ajay.Kathat@microchip.com>
Cc:     <linux-wireless@vger.kernel.org>, <Claudiu.Beznea@microchip.com>
Subject: Re: [PATCH 08/12] wilc1000: invoke chip reset register while FW download
References: <20210423182925.5865-1-ajay.kathat@microchip.com>
        <20210423182925.5865-9-ajay.kathat@microchip.com>
Date:   Thu, 24 Jun 2021 12:32:34 +0300
In-Reply-To: <20210423182925.5865-9-ajay.kathat@microchip.com> (Ajay Kathat's
        message of "Fri, 23 Apr 2021 18:29:47 +0000")
Message-ID: <87a6nfip59.fsf@tynnyri.adurom.net>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

<Ajay.Kathat@microchip.com> writes:

> From: Ajay Singh <ajay.kathat@microchip.com>
>
> Added chip-reset command before firmware download to initialize the
> chip. Also configure chip in wake state, ready to accept the FW
> binary.

"Added" -> "Add"

This repeats many times, so I don't comment on that anymore.

> @@ -1088,10 +1089,22 @@ int wilc_wlan_firmware_download(struct wilc *wilc, const u8 *buffer,
>  		return -EIO;
>  
>  	offset = 0;
> +	pr_info("%s: Downloading firmware size = %d\n", __func__, buffer_size);
> +
> +	acquire_bus(wilc, WILC_BUS_ACQUIRE_AND_WAKEUP);
> +
> +	wilc->hif_func->hif_read_reg(wilc, WILC_GLB_RESET_0, &reg);
> +	reg &= ~BIT(10);
> +	ret = wilc->hif_func->hif_write_reg(wilc, WILC_GLB_RESET_0, reg);
> +	wilc->hif_func->hif_read_reg(wilc, WILC_GLB_RESET_0, &reg);
> +	if (reg & BIT(10))
> +		pr_err("%s: Failed to reset\n", __func__);
> +
> +	release_bus(wilc, WILC_BUS_RELEASE_ONLY);
>  	do {
>  		addr = get_unaligned_le32(&buffer[offset]);
>  		size = get_unaligned_le32(&buffer[offset + 4]);
> -		acquire_bus(wilc, WILC_BUS_ACQUIRE_ONLY);
> +		acquire_bus(wilc, WILC_BUS_ACQUIRE_AND_WAKEUP);
>  		offset += 8;
>  		while (((int)size) && (offset < buffer_size)) {
>  			if (size <= blksz)
> @@ -1109,10 +1122,13 @@ int wilc_wlan_firmware_download(struct wilc *wilc, const u8 *buffer,
>  			offset += size2;
>  			size -= size2;
>  		}
> -		release_bus(wilc, WILC_BUS_RELEASE_ONLY);
> +		release_bus(wilc, WILC_BUS_RELEASE_ALLOW_SLEEP);
>  
> -		if (ret)
> +		if (ret) {
> +			pr_err("%s Bus error\n", __func__);
>  			goto fail;
> +		}
> +		pr_info("%s Offset = %d\n", __func__, offset);

Please use pr_info() sparingly, it's only for important messages like
hardware info, firmware version/features and so on. I think pr_info()
calls in this function should be debug messages instead.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches