Message-id: <55BEE8FF.3030207@samsung.com>
Date: Mon, 03 Aug 2015 13:07:27 +0900
From: Jaehoon Chung <jh80.chung@samsung.com>
User-Agent: Mozilla/5.0 (X11; Linux i686; rv:31.0) Gecko/20100101
 Thunderbird/31.7.0
MIME-version: 1.0
To: Shawn Lin <shawn.lin@rock-chips.com>, Ulf Hansson <ulf.hansson@linaro.org>
Cc: Seungwon Jeon <tgih.jun@samsung.com>, linux-mmc@vger.kernel.org,
        linux-kernel@vger.kernel.org, dianders@chromium.org
Subject: Re: [PATCH] mmc: dw_mmc: Fix coding style issues
References: <1438052781-25261-1-git-send-email-shawn.lin@rock-chips.com>
In-reply-to: <1438052781-25261-1-git-send-email-shawn.lin@rock-chips.com>
Content-type: text/plain; charset=windows-1252
Content-transfer-encoding: 7bit
DLP-Filter: Pass
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 15622
Lines: 434

Hi, Shawn.

On 07/28/2015 12:06 PM, Shawn Lin wrote:
> This patch fixes the following issues reported by checkpatch.pl:
> - use -EINVAL instead of -ENOSYS, to fix warning message:
>   "ENOSYS means 'invalid syscall nr' and nothing else"
> - split lines whose length is greater than 80 characters
> - avoid quoted string split across lines
> - use min_t instead of min, to fix warning message:
>   "min() should probably be min_t(int, cnt, host->part_buf_count)"

Thanks for fixing coding style.
But i will apply this patch(https://patchwork.kernel.org/patch/6672581/).
So if you can fix with this patch, then it's helpful to me.
If you can't, i will modify your patch before applying.
How about?

> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
> 
>  drivers/mmc/host/dw_mmc.c | 85 ++++++++++++++++++++++++++++++-----------------
>  1 file changed, 54 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 40e9d8e..6aede38 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -235,8 +235,8 @@ static u32 dw_mci_prepare_command(struct mmc_host *mmc, struct mmc_command *cmd)
>  	struct dw_mci *host = slot->host;
>  	const struct dw_mci_drv_data *drv_data = slot->host->drv_data;
>  	u32 cmdr;
> -	cmd->error = -EINPROGRESS;
>  
> +	cmd->error = -EINPROGRESS;
>  	cmdr = cmd->opcode;
>  
>  	if (cmd->opcode == MMC_STOP_TRANSMISSION ||
> @@ -371,6 +371,7 @@ static void dw_mci_start_command(struct dw_mci *host,
>  		 cmd->arg, cmd_flags);
>  
>  	mci_writel(host, CMDARG, cmd->arg);
> +	/* Make sure CMDARG is configured before CMD */
>  	wmb();
>  	dw_mci_wait_while_busy(host, cmd_flags);
>  
> @@ -380,6 +381,7 @@ static void dw_mci_start_command(struct dw_mci *host,
>  static inline void send_stop_abort(struct dw_mci *host, struct mmc_data *data)
>  {
>  	struct mmc_command *stop = data->stop ? data->stop : &host->stop_abort;
> +
>  	dw_mci_start_command(host, stop, host->stop_cmdr);
>  }
>  
> @@ -463,6 +465,7 @@ static void dw_mci_translate_sglist(struct dw_mci *host, struct mmc_data *data,
>  				    unsigned int sg_len)
>  {
>  	int i;
> +
>  	if (host->dma_64bit_address == 1) {
>  		struct idmac_desc_64addr *desc = host->sg_cpu;
>  
> @@ -524,7 +527,7 @@ static void dw_mci_translate_sglist(struct dw_mci *host, struct mmc_data *data,
>  		desc->des0 |= cpu_to_le32(IDMAC_DES0_LD);
>  	}
>  
> -	wmb();
> +	wmb(); /* drain writebuffer */
>  }
>  
>  static void dw_mci_idmac_start_dma(struct dw_mci *host, unsigned int sg_len)
> @@ -542,7 +545,7 @@ static void dw_mci_idmac_start_dma(struct dw_mci *host, unsigned int sg_len)
>  	temp |= SDMMC_CTRL_USE_IDMAC;
>  	mci_writel(host, CTRL, temp);
>  
> -	wmb();
> +	wmb(); /* drain writebuffer */
>  
>  	/* Enable the IDMAC */
>  	temp = mci_readl(host, BMOD);
> @@ -589,7 +592,9 @@ static int dw_mci_idmac_init(struct dw_mci *host)
>  		host->ring_size = PAGE_SIZE / sizeof(struct idmac_desc);
>  
>  		/* Forward link the descriptor list */
> -		for (i = 0, p = host->sg_cpu; i < host->ring_size - 1; i++, p++) {
> +		for (i = 0, p = host->sg_cpu;
> +		     i < host->ring_size - 1;
> +		     i++, p++) {
>  			p->des3 = cpu_to_le32(host->sg_dma +
>  					(sizeof(struct idmac_desc) * (i + 1)));
>  			p->des1 = 0;
> @@ -718,7 +723,7 @@ static void dw_mci_adjust_fifoth(struct dw_mci *host, struct mmc_data *data)
>  	u32 fifo_width = 1 << host->data_shift;
>  	u32 blksz_depth = blksz / fifo_width, fifoth_val;
>  	u32 msize = 0, rx_wmark = 1, tx_wmark, tx_wmark_invers;
> -	int idx = (sizeof(mszs) / sizeof(mszs[0])) - 1;
> +	int idx = ARRAY_SIZE(mszs) - 1;
>  
>  	tx_wmark = (host->fifo_depth) / 2;
>  	tx_wmark_invers = host->fifo_depth - tx_wmark;
> @@ -843,6 +848,7 @@ static int dw_mci_submit_data_dma(struct dw_mci *host, struct mmc_data *data)
>  static void dw_mci_submit_data(struct dw_mci *host, struct mmc_data *data)
>  {
>  	unsigned long irqflags;
> +	int flags = SG_MITER_ATOMIC;
>  	u32 temp;
>  
>  	data->error = -EINPROGRESS;
> @@ -859,7 +865,6 @@ static void dw_mci_submit_data(struct dw_mci *host, struct mmc_data *data)
>  	}
>  
>  	if (dw_mci_submit_data_dma(host, data)) {
> -		int flags = SG_MITER_ATOMIC;
>  		if (host->data->flags & MMC_DATA_READ)
>  			flags |= SG_MITER_TO_SG;
>  		else
> @@ -906,6 +911,7 @@ static void mci_send_cmd(struct dw_mci_slot *slot, u32 cmd, u32 arg)
>  	unsigned int cmd_status = 0;
>  
>  	mci_writel(host, CMDARG, arg);
> +	/* Make sure CMDARG is configured before CMD */
>  	wmb();
>  	dw_mci_wait_while_busy(host, cmd);
>  	mci_writel(host, CMD, SDMMC_CMD_START | cmd);
> @@ -1019,6 +1025,7 @@ static void __dw_mci_start_request(struct dw_mci *host,
>  
>  	if (data) {
>  		dw_mci_submit_data(host, data);
> +		/* drain writebuffer */
>  		wmb();

I think good that you have consistency for above comment.
wmb(); /* drain writebuffer */ 
or 
/* drain writebuffer */
wmb();

What do you want?

Best Regards,
Jaehoon Chung

>  	}
>  
> @@ -1384,14 +1391,15 @@ static int dw_mci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>  	struct dw_mci_slot *slot = mmc_priv(mmc);
>  	struct dw_mci *host = slot->host;
>  	const struct dw_mci_drv_data *drv_data = host->drv_data;
> -	int err = -ENOSYS;
> +	int err = -EINVAL;
>  
>  	if (drv_data && drv_data->execute_tuning)
>  		err = drv_data->execute_tuning(slot);
>  	return err;
>  }
>  
> -static int dw_mci_prepare_hs400_tuning(struct mmc_host *mmc, struct mmc_ios *ios)
> +static int dw_mci_prepare_hs400_tuning(struct mmc_host *mmc,
> +				       struct mmc_ios *ios)
>  {
>  	struct dw_mci_slot *slot = mmc_priv(mmc);
>  	struct dw_mci *host = slot->host;
> @@ -1743,7 +1751,7 @@ static int dw_mci_push_part_bytes(struct dw_mci *host, void *buf, int cnt)
>  /* pull first bytes from part_buf, only use during pull */
>  static int dw_mci_pull_part_bytes(struct dw_mci *host, void *buf, int cnt)
>  {
> -	cnt = min(cnt, (int)host->part_buf_count);
> +	cnt = min_t(int, cnt, host->part_buf_count);
>  	if (cnt) {
>  		memcpy(buf, (void *)&host->part_buf + host->part_buf_start,
>  		       cnt);
> @@ -1769,6 +1777,7 @@ static void dw_mci_push_data16(struct dw_mci *host, void *buf, int cnt)
>  	/* try and push anything in the part_buf */
>  	if (unlikely(host->part_buf_count)) {
>  		int len = dw_mci_push_part_bytes(host, buf, cnt);
> +
>  		buf += len;
>  		cnt -= len;
>  		if (host->part_buf_count == 2) {
> @@ -1795,6 +1804,7 @@ static void dw_mci_push_data16(struct dw_mci *host, void *buf, int cnt)
>  #endif
>  	{
>  		u16 *pdata = buf;
> +
>  		for (; cnt >= 2; cnt -= 2)
>  			mci_fifo_writew(host->fifo_reg, *pdata++);
>  		buf = pdata;
> @@ -1819,6 +1829,7 @@ static void dw_mci_pull_data16(struct dw_mci *host, void *buf, int cnt)
>  			int len = min(cnt & -2, (int)sizeof(aligned_buf));
>  			int items = len >> 1;
>  			int i;
> +
>  			for (i = 0; i < items; ++i)
>  				aligned_buf[i] = mci_fifo_readw(host->fifo_reg);
>  			/* memcpy from aligned buffer into output buffer */
> @@ -1830,6 +1841,7 @@ static void dw_mci_pull_data16(struct dw_mci *host, void *buf, int cnt)
>  #endif
>  	{
>  		u16 *pdata = buf;
> +
>  		for (; cnt >= 2; cnt -= 2)
>  			*pdata++ = mci_fifo_readw(host->fifo_reg);
>  		buf = pdata;
> @@ -1848,6 +1860,7 @@ static void dw_mci_push_data32(struct dw_mci *host, void *buf, int cnt)
>  	/* try and push anything in the part_buf */
>  	if (unlikely(host->part_buf_count)) {
>  		int len = dw_mci_push_part_bytes(host, buf, cnt);
> +
>  		buf += len;
>  		cnt -= len;
>  		if (host->part_buf_count == 4) {
> @@ -1874,6 +1887,7 @@ static void dw_mci_push_data32(struct dw_mci *host, void *buf, int cnt)
>  #endif
>  	{
>  		u32 *pdata = buf;
> +
>  		for (; cnt >= 4; cnt -= 4)
>  			mci_fifo_writel(host->fifo_reg, *pdata++);
>  		buf = pdata;
> @@ -1898,6 +1912,7 @@ static void dw_mci_pull_data32(struct dw_mci *host, void *buf, int cnt)
>  			int len = min(cnt & -4, (int)sizeof(aligned_buf));
>  			int items = len >> 2;
>  			int i;
> +
>  			for (i = 0; i < items; ++i)
>  				aligned_buf[i] = mci_fifo_readl(host->fifo_reg);
>  			/* memcpy from aligned buffer into output buffer */
> @@ -1909,6 +1924,7 @@ static void dw_mci_pull_data32(struct dw_mci *host, void *buf, int cnt)
>  #endif
>  	{
>  		u32 *pdata = buf;
> +
>  		for (; cnt >= 4; cnt -= 4)
>  			*pdata++ = mci_fifo_readl(host->fifo_reg);
>  		buf = pdata;
> @@ -1927,6 +1943,7 @@ static void dw_mci_push_data64(struct dw_mci *host, void *buf, int cnt)
>  	/* try and push anything in the part_buf */
>  	if (unlikely(host->part_buf_count)) {
>  		int len = dw_mci_push_part_bytes(host, buf, cnt);
> +
>  		buf += len;
>  		cnt -= len;
>  
> @@ -1954,6 +1971,7 @@ static void dw_mci_push_data64(struct dw_mci *host, void *buf, int cnt)
>  #endif
>  	{
>  		u64 *pdata = buf;
> +
>  		for (; cnt >= 8; cnt -= 8)
>  			mci_fifo_writeq(host->fifo_reg, *pdata++);
>  		buf = pdata;
> @@ -1978,6 +1996,7 @@ static void dw_mci_pull_data64(struct dw_mci *host, void *buf, int cnt)
>  			int len = min(cnt & -8, (int)sizeof(aligned_buf));
>  			int items = len >> 3;
>  			int i;
> +
>  			for (i = 0; i < items; ++i)
>  				aligned_buf[i] = mci_fifo_readq(host->fifo_reg);
>  
> @@ -1990,6 +2009,7 @@ static void dw_mci_pull_data64(struct dw_mci *host, void *buf, int cnt)
>  #endif
>  	{
>  		u64 *pdata = buf;
> +
>  		for (; cnt >= 8; cnt -= 8)
>  			*pdata++ = mci_fifo_readq(host->fifo_reg);
>  		buf = pdata;
> @@ -2065,7 +2085,7 @@ static void dw_mci_read_data_pio(struct dw_mci *host, bool dto)
>  done:
>  	sg_miter_stop(sg_miter);
>  	host->sg = NULL;
> -	smp_wmb();
> +	smp_wmb(); /* drain writebuffer */
>  	set_bit(EVENT_XFER_COMPLETE, &host->pending_events);
>  }
>  
> @@ -2119,7 +2139,7 @@ static void dw_mci_write_data_pio(struct dw_mci *host)
>  done:
>  	sg_miter_stop(sg_miter);
>  	host->sg = NULL;
> -	smp_wmb();
> +	smp_wmb(); /* drain writebuffer */
>  	set_bit(EVENT_XFER_COMPLETE, &host->pending_events);
>  }
>  
> @@ -2128,6 +2148,7 @@ static void dw_mci_cmd_interrupt(struct dw_mci *host, u32 status)
>  	if (!host->cmd_status)
>  		host->cmd_status = status;
>  
> +	/* drain writebuffer */
>  	smp_wmb();
>  
>  	set_bit(EVENT_CMD_COMPLETE, &host->pending_events);
> @@ -2192,7 +2213,7 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
>  		if (pending & DW_MCI_CMD_ERROR_FLAGS) {
>  			mci_writel(host, RINTSTS, DW_MCI_CMD_ERROR_FLAGS);
>  			host->cmd_status = pending;
> -			smp_wmb();
> +			smp_wmb(); /* drain writebuffer */
>  			set_bit(EVENT_CMD_COMPLETE, &host->pending_events);
>  		}
>  
> @@ -2200,7 +2221,7 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
>  			/* if there is an error report DATA_ERROR */
>  			mci_writel(host, RINTSTS, DW_MCI_DATA_ERROR_FLAGS);
>  			host->data_status = pending;
> -			smp_wmb();
> +			smp_wmb(); /* drain writebuffer */
>  			set_bit(EVENT_DATA_ERROR, &host->pending_events);
>  			tasklet_schedule(&host->tasklet);
>  		}
> @@ -2209,7 +2230,7 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
>  			mci_writel(host, RINTSTS, SDMMC_INT_DATA_OVER);
>  			if (!host->data_status)
>  				host->data_status = pending;
> -			smp_wmb();
> +			smp_wmb(); /* drain writebuffer */
>  			if (host->dir_status == DW_MCI_RECV_STATUS) {
>  				if (host->sg != NULL)
>  					dw_mci_read_data_pio(host, true);
> @@ -2473,8 +2494,8 @@ static void dw_mci_init_dma(struct dw_mci *host)
>  	if (host->dma_ops->init && host->dma_ops->start &&
>  	    host->dma_ops->stop && host->dma_ops->cleanup) {
>  		if (host->dma_ops->init(host)) {
> -			dev_err(host->dev, "%s: Unable to initialize "
> -				"DMA Controller.\n", __func__);
> +			dev_err(host->dev, "%s: Unable to initialize DMA Controller.\n",
> +				__func__);
>  			goto no_dma;
>  		}
>  	} else {
> @@ -2488,7 +2509,6 @@ static void dw_mci_init_dma(struct dw_mci *host)
>  no_dma:
>  	dev_info(host->dev, "Using PIO mode.\n");
>  	host->use_dma = 0;
> -	return;
>  }
>  
>  static bool dw_mci_ctrl_reset(struct dw_mci *host, u32 reset)
> @@ -2542,6 +2562,7 @@ static bool dw_mci_reset(struct dw_mci *host)
>  		if (host->use_dma) {
>  			unsigned long timeout = jiffies + msecs_to_jiffies(500);
>  			u32 status;
> +
>  			do {
>  				status = mci_readl(host, STATUS);
>  				if (!(status & SDMMC_STATUS_DMA_REQ))
> @@ -2551,8 +2572,8 @@ static bool dw_mci_reset(struct dw_mci *host)
>  
>  			if (status & SDMMC_STATUS_DMA_REQ) {
>  				dev_err(host->dev,
> -					"%s: Timeout waiting for dma_req to "
> -					"clear during reset\n", __func__);
> +					"%s: Timeout waiting for dma_req to clear during reset\n",
> +					__func__);
>  				goto ciu_out;
>  			}
>  
> @@ -2563,8 +2584,8 @@ static bool dw_mci_reset(struct dw_mci *host)
>  	} else {
>  		/* if the controller reset bit did clear, then set clock regs */
>  		if (!(mci_readl(host, CTRL) & SDMMC_CTRL_RESET)) {
> -			dev_err(host->dev, "%s: fifo/dma reset bits didn't "
> -				"clear but ciu was reset, doing clock update\n",
> +			dev_err(host->dev,
> +				"%s: fifo/dma reset bits didn't clear but ciu was reset, doing clock update\n",
>  				__func__);
>  			goto ciu_out;
>  		}
> @@ -2625,8 +2646,8 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
>  	/* find out number of slots supported */
>  	if (of_property_read_u32(dev->of_node, "num-slots",
>  				&pdata->num_slots)) {
> -		dev_info(dev, "num-slots property not found, "
> -				"assuming 1 slot is available\n");
> +		dev_info(dev,
> +			 "num-slots property not found, assuming 1 slot is available\n");
>  		pdata->num_slots = 1;
>  	}
>  
> @@ -2636,8 +2657,8 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
>  			pdata->quirks |= of_quirks[idx].id;
>  
>  	if (of_property_read_u32(np, "fifo-depth", &pdata->fifo_depth))
> -		dev_info(dev, "fifo-depth property not found, using "
> -				"value of FIFOTH register as default\n");
> +		dev_info(dev,
> +			 "fifo-depth property not found, using value of FIFOTH register as default\n");
>  
>  	of_property_read_u32(np, "card-detect-delay", &pdata->detect_delay_ms);
>  
> @@ -2874,11 +2895,11 @@ int dw_mci_probe(struct dw_mci *host)
>  	mci_writel(host, INTMASK, SDMMC_INT_CMD_DONE | SDMMC_INT_DATA_OVER |
>  		   SDMMC_INT_TXDR | SDMMC_INT_RXDR |
>  		   DW_MCI_ERROR_FLAGS);
> -	mci_writel(host, CTRL, SDMMC_CTRL_INT_ENABLE); /* Enable mci interrupt */
> +	/* Enable mci interrupt */
> +	mci_writel(host, CTRL, SDMMC_CTRL_INT_ENABLE);
>  
> -	dev_info(host->dev, "DW MMC controller at irq %d, "
> -		 "%d bit host data width, "
> -		 "%u deep fifo\n",
> +	dev_info(host->dev,
> +		 "DW MMC controller at irq %d,%d bit host data width,%u deep fifo\n",
>  		 host->irq, width, fifo_size);
>  
>  	/* We need at least one slot to succeed */
> @@ -2893,8 +2914,9 @@ int dw_mci_probe(struct dw_mci *host)
>  	if (init_slots) {
>  		dev_info(host->dev, "%d slots initialized\n", init_slots);
>  	} else {
> -		dev_dbg(host->dev, "attempted to initialize %d slots, "
> -					"but failed on all\n", host->num_slots);
> +		dev_dbg(host->dev,
> +			"attempted to initialize %d slots, but failed on all\n",
> +			host->num_slots);
>  		goto err_dmaunmap;
>  	}
>  
> @@ -2992,6 +3014,7 @@ int dw_mci_resume(struct dw_mci *host)
>  
>  	for (i = 0; i < host->num_slots; i++) {
>  		struct dw_mci_slot *slot = host->slot[i];
> +
>  		if (!slot)
>  			continue;
>  		if (slot->mmc->pm_flags & MMC_PM_KEEP_POWER) {
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/