[U-Boot] [PATCH] mmc: Minor cleanup of sdhci.c

[Pantelis Antoniou]

Hi Darwin (Rambo? - cool name)


On Dec 30, 2013, at 12:25 AM, Darwin Rambo wrote:

>> -----Original Message-----
>> From: Jaehoon Chung [mailto:jh80.chung at samsung.com]
>> Sent: Wednesday, December 25, 2013 11:32 PM
>> To: Darwin Rambo; u-boot at lists.denx.de
>> Cc: Pantelis Antoniou
>> Subject: Re: [U-Boot] [PATCH] mmc: Minor cleanup of sdhci.c
>> 
>> Hi, Darwin.
>> 
>> I didn't think that "__func__" is needs...in my case.
>> There isn't the duplicated message, anywhere.
> 
> Hi Jaehoon,
> 
> I think that the __func__ is needed to show where the prints are coming from when there are prints coming from many other functions outside of the sdhci subsystem as well. It is less confusing and faster to debug if prints in general show where they are coming from. Just because the prints are unique inside sdhci.c is not enough to remove the need for the prefix. This is especially true since the prints in this module are very generic such as "Error detected in status" or "Transfer data timeout". These messages are so generic that they could be from USB, I2C, NET, DMA, etc. Having the function prefix is a nice way to avoid having to search the code base for these generic strings. 
> 
> Best regards,
> Darwin Rambo
> 
>> 
>> Best Regards,
>> Jaehoon Chung
>> 
>> On 12/20/2013 08:13 AM, Darwin Rambo wrote:
>>> Fixup prints to show where the print is done from, and
>>> a few minor formatting/grammar issues.
>>> 
>>> Signed-off-by: Darwin Rambo <drambo at broadcom.com>
>>> ---
>>> drivers/mmc/sdhci.c |   32 +++++++++++++++++++-------------
>>> 1 file changed, 19 insertions(+), 13 deletions(-)
>>> 
>>> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
>>> index 46ae9cb..1e86b92 100644
>>> --- a/drivers/mmc/sdhci.c
>>> +++ b/drivers/mmc/sdhci.c
>>> @@ -24,7 +24,8 @@ static void sdhci_reset(struct sdhci_host *host, u8 mask)
>>> 	sdhci_writeb(host, mask, SDHCI_SOFTWARE_RESET);
>>> 	while (sdhci_readb(host, SDHCI_SOFTWARE_RESET) & mask) {
>>> 		if (timeout == 0) {
>>> -			printf("Reset 0x%x never completed.\n", (int)mask);
>>> +			printf("%s: Reset 0x%x never completed.\n",
>>> +			       __func__, (int)mask);
>>> 			return;
>>> 		}
>>> 		timeout--;
>>> @@ -79,7 +80,8 @@ static int sdhci_transfer_data(struct sdhci_host *host,
>> struct mmc_data *data,
>>> 	do {
>>> 		stat = sdhci_readl(host, SDHCI_INT_STATUS);
>>> 		if (stat & SDHCI_INT_ERROR) {
>>> -			printf("Error detected in status(0x%X)!\n", stat);
>>> +			printf("%s: Error detected in status(0x%X)!\n",
>>> +			       __func__, stat);
>>> 			return -1;
>>> 		}
>>> 		if (stat & rdy) {
>>> @@ -102,7 +104,7 @@ static int sdhci_transfer_data(struct sdhci_host *host,
>> struct mmc_data *data,
>>> 		if (timeout-- > 0)
>>> 			udelay(10);
>>> 		else {
>>> -			printf("Transfer data timeout\n");
>>> +			printf("%s: Transfer data timeout\n", __func__);
>>> 			return -1;
>>> 		}
>>> 	} while (!(stat & SDHCI_INT_DATA_END));
>>> @@ -147,7 +149,7 @@ int sdhci_send_command(struct mmc *mmc, struct
>> mmc_cmd *cmd,
>>> 
>>> 	while (sdhci_readl(host, SDHCI_PRESENT_STATE) & mask) {
>>> 		if (time >= cmd_timeout) {
>>> -			printf("MMC: %d busy ", mmc_dev);
>>> +			printf("%s: MMC: %d busy ", __func__, mmc_dev);
>>> 			if (2 * cmd_timeout <=
>> CONFIG_SDHCI_CMD_MAX_TIMEOUT) {
>>> 				cmd_timeout += cmd_timeout;
>>> 				printf("timeout increasing to: %u ms.\n",
>>> @@ -179,7 +181,7 @@ int sdhci_send_command(struct mmc *mmc, struct
>> mmc_cmd *cmd,
>>> 	if (data)
>>> 		flags |= SDHCI_CMD_DATA;
>>> 
>>> -	/*Set Transfer mode regarding to data flag*/
>>> +	/* Set Transfer mode regarding to data flag */
>>> 	if (data != 0) {
>>> 		sdhci_writeb(host, 0xe, SDHCI_TIMEOUT_CONTROL);
>>> 		mode = SDHCI_TRNS_BLK_CNT_EN;
>>> @@ -230,7 +232,7 @@ int sdhci_send_command(struct mmc *mmc, struct
>> mmc_cmd *cmd,
>>> 		if (host->quirks & SDHCI_QUIRK_BROKEN_R1B)
>>> 			return 0;
>>> 		else {
>>> -			printf("Timeout for status update!\n");
>>> +			printf("%s: Timeout for status update!\n", __func__);
>>> 			return TIMEOUT;
>>> 		}
>>> 	}
>>> @@ -307,7 +309,8 @@ static int sdhci_set_clock(struct mmc *mmc,
>> unsigned int clock)
>>> 	while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL))
>>> 		& SDHCI_CLOCK_INT_STABLE)) {
>>> 		if (timeout == 0) {
>>> -			printf("Internal clock never stabilised.\n");
>>> +			printf("%s: Internal clock never stabilised.\n",
>>> +			       __func__);
>>> 			return -1;
>>> 		}
>>> 		timeout--;
>>> @@ -397,7 +400,8 @@ int sdhci_init(struct mmc *mmc)
>>> 	if ((host->quirks & SDHCI_QUIRK_32BIT_DMA_ADDR) &&
>> !aligned_buffer) {
>>> 		aligned_buffer = memalign(8, 512*1024);
>>> 		if (!aligned_buffer) {
>>> -			printf("Aligned buffer alloc failed!!!");
>>> +			printf("%s: Aligned buffer alloc failed!!!\n",
>>> +			       __func__);
>>> 			return -1;
>>> 		}
>>> 	}
>>> @@ -418,8 +422,8 @@ int sdhci_init(struct mmc *mmc)
>>> 	}
>>> 
>>> 	/* Enable only interrupts served by the SD controller */
>>> -	sdhci_writel(host, SDHCI_INT_DATA_MASK | SDHCI_INT_CMD_MASK
>>> -		     , SDHCI_INT_ENABLE);
>>> +	sdhci_writel(host, SDHCI_INT_DATA_MASK | SDHCI_INT_CMD_MASK,
>>> +		     SDHCI_INT_ENABLE);
>>> 	/* Mask all sdhci interrupt sources */
>>> 	sdhci_writel(host, 0x0, SDHCI_SIGNAL_ENABLE);
>>> 
>>> @@ -433,7 +437,7 @@ int add_sdhci(struct sdhci_host *host, u32 max_clk,
>> u32 min_clk)
>>> 
>>> 	mmc = malloc(sizeof(struct mmc));
>>> 	if (!mmc) {
>>> -		printf("mmc malloc fail!\n");
>>> +		printf("%s: mmc malloc fail!\n", __func__);
>>> 		return -1;
>>> 	}
>>> 
>>> @@ -450,7 +454,8 @@ int add_sdhci(struct sdhci_host *host, u32 max_clk,
>> u32 min_clk)
>>> 	caps = sdhci_readl(host, SDHCI_CAPABILITIES);
>>> #ifdef CONFIG_MMC_SDMA
>>> 	if (!(caps & SDHCI_CAN_DO_SDMA)) {
>>> -		printf("Your controller don't support sdma!!\n");
>>> +		printf("%s: Your controller doesn't support SDMA!!\n",
>>> +		       __func__);
>>> 		return -1;
>>> 	}
>>> #endif
>>> @@ -467,7 +472,8 @@ int add_sdhci(struct sdhci_host *host, u32 max_clk,
>> u32 min_clk)
>>> 		mmc->f_max *= 1000000;
>>> 	}
>>> 	if (mmc->f_max == 0) {
>>> -		printf("Hardware doesn't specify base clock frequency\n");
>>> +		printf("%s: Hardware doesn't specify base clock frequency\n",
>>> +		       __func__);
>>> 		return -1;
>>> 	}
>>> 	if (min_clk)
>>> 
> 

Applied, thanks

Acked-by: Pantelis Antoniou <panto at antoniou-consulting.com>