[PATCH 0/2] mmc: sdhci-pxav3: Fix tabbing issue

[PATCH 0/2] mmc: sdhci-pxav3: Fix tabbing issue

[Vaibhav Hiremath]

Trivial patch-series, which fixes the tabbing issue in the driver
and second patch prints the return value from sdhci_add_host() function


Vaibhav Hiremath (2):
  mmc: sdhci-pxav3: Fix tabbing issue
  mmc: sdhci-pxav3: Print ret value on error from sdhci_add_host() fn

 drivers/mmc/host/sdhci-pxav3.c | 65 ++++++++++++++++++++++--------------------
 1 file changed, 34 insertions(+), 31 deletions(-)

-- 
1.9.1

[PATCH 1/2] mmc: sdhci-pxav3: Fix tabbing issue

[Vaibhav Hiremath]

There were some coding style issues where spaces have been used instead
of tabs, for example, in macro definitions, alignment of function
declarations/definitions, etc...

This patch fixes all such occurrences in the code.
And also use BIT for bit definitions.

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 drivers/mmc/host/sdhci-pxav3.c | 63 ++++++++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 30 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
index 946d37f..d02bc37 100644
--- a/drivers/mmc/host/sdhci-pxav3.c
+++ b/drivers/mmc/host/sdhci-pxav3.c
@@ -39,24 +39,29 @@
 #include "sdhci.h"
 #include "sdhci-pltfm.h"
 
-#define PXAV3_RPM_DELAY_MS     50
+#define PXAV3_RPM_DELAY_MS		50
 
-#define SD_CLOCK_BURST_SIZE_SETUP		0x10A
-#define SDCLK_SEL	0x100
-#define SDCLK_DELAY_SHIFT	9
-#define SDCLK_DELAY_MASK	0x1f
+#define SD_CLOCK_BURST_SIZE_SETUP	0x10A
+#define SDCLK_SEL			0x100
+#define  SDCLK_DELAY_SHIFT		9
+#define  SDCLK_DELAY_MASK		0x1f
 
-#define SD_CFG_FIFO_PARAM       0x100
-#define SDCFG_GEN_PAD_CLK_ON	(1<<6)
-#define SDCFG_GEN_PAD_CLK_CNT_MASK	0xFF
-#define SDCFG_GEN_PAD_CLK_CNT_SHIFT	24
+#define SD_CFG_FIFO_PARAM		0x100
+#define  SDCFG_GEN_PAD_CLK_ON		BIT(6)
+#define  SDCFG_GEN_PAD_CLK_CNT_MASK	0xFF
+#define  SDCFG_GEN_PAD_CLK_CNT_SHIFT	24
 
-#define SD_SPI_MODE          0x108
-#define SD_CE_ATA_1          0x10C
+#define SD_SPI_MODE			0x108
+#define SD_CE_ATA_1			0x10C
 
-#define SD_CE_ATA_2          0x10E
-#define SDCE_MISC_INT		(1<<2)
-#define SDCE_MISC_INT_EN	(1<<1)
+#define SD_CE_ATA_2			0x10E
+#define  SDCE_MISC_INT			BIT(2)
+#define  SDCE_MISC_INT_EN		BIT(1)
+
+/* IO Power control */
+#define IO_PWR_AKEY_ASFAR		0xbaba
+#define IO_PWR_AKEY_ASSAR		0xeb10
+#define IO_PWR_MMC1_PAD_1V8		BIT(2)
 
 struct sdhci_pxa {
 	struct clk *clk_core;
@@ -128,7 +133,7 @@ static int mv_conf_mbus_windows(struct platform_device *pdev,
 }
 
 static int armada_38x_quirks(struct platform_device *pdev,
-			     struct sdhci_host *host)
+			struct sdhci_host *host)
 {
 	struct device_node *np = pdev->dev.of_node;
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
@@ -136,8 +141,7 @@ static int armada_38x_quirks(struct platform_device *pdev,
 	struct resource *res;
 
 	host->quirks |= SDHCI_QUIRK_MISSING_CAPS;
-	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
-					   "conf-sdio3");
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "conf-sdio3");
 	if (res) {
 		pxa->sdio3_conf_reg = devm_ioremap_resource(&pdev->dev, res);
 		if (IS_ERR(pxa->sdio3_conf_reg))
@@ -284,10 +288,10 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
 	 * FE-2946959
 	 */
 	if (pxa->sdio3_conf_reg) {
-		u8 reg_val  = readb(pxa->sdio3_conf_reg);
+		u8 reg_val = readb(pxa->sdio3_conf_reg);
 
 		if (uhs == MMC_TIMING_UHS_SDR50 ||
-		    uhs == MMC_TIMING_UHS_DDR50) {
+				uhs == MMC_TIMING_UHS_DDR50) {
 			reg_val &= ~SDIO3_CONF_CLK_INV;
 			reg_val |= SDIO3_CONF_SD_FB_CLK;
 		} else {
@@ -304,20 +308,20 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
 }
 
 static const struct sdhci_ops pxav3_sdhci_ops = {
-	.set_clock = sdhci_set_clock,
-	.platform_send_init_74_clocks = pxav3_gen_init_74_clocks,
-	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
-	.set_bus_width = sdhci_set_bus_width,
-	.reset = pxav3_reset,
-	.set_uhs_signaling = pxav3_set_uhs_signaling,
+	.set_clock			= sdhci_set_clock,
+	.platform_send_init_74_clocks	= pxav3_gen_init_74_clocks,
+	.get_max_clock			= sdhci_pltfm_clk_get_max_clock,
+	.set_bus_width			= sdhci_set_bus_width,
+	.reset				= pxav3_reset,
+	.set_uhs_signaling		= pxav3_set_uhs_signaling,
 };
 
 static struct sdhci_pltfm_data sdhci_pxav3_pdata = {
-	.quirks = SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK
+	.quirks	= SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK
 		| SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC
 		| SDHCI_QUIRK_32BIT_ADMA_SIZE
 		| SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
-	.ops = &pxav3_sdhci_ops,
+	.ops	= &pxav3_sdhci_ops,
 };
 
 #ifdef CONFIG_OF
@@ -343,7 +347,7 @@ static struct sdhci_pxa_platdata *pxav3_get_mmc_pdata(struct device *dev)
 		return NULL;
 
 	if (!of_property_read_u32(np, "mrvl,clk-delay-cycles",
-				  &clk_delay_cycles))
+				&clk_delay_cycles))
 		pdata->clk_delay_cycles = clk_delay_cycles;
 
 	return pdata;
@@ -433,8 +437,7 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
 			host->mmc->pm_caps |= pdata->pm_caps;
 
 		if (gpio_is_valid(pdata->ext_cd_gpio)) {
-			ret = mmc_gpio_request_cd(host->mmc, pdata->ext_cd_gpio,
-						  0);
+			ret = mmc_gpio_request_cd(host->mmc, pdata->ext_cd_gpio, 0);
 			if (ret) {
 				dev_err(mmc_dev(host->mmc),
 					"failed to allocate card detect gpio\n");
-- 
1.9.1

[PATCH 2/2] mmc: sdhci-pxav3: Print ret value on error from sdhci_add_host() fn

[Vaibhav Hiremath]

Return value would give clear information about the actual root-cause
of the failure.

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 drivers/mmc/host/sdhci-pxav3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
index d02bc37..5d26fe0 100644
--- a/drivers/mmc/host/sdhci-pxav3.c
+++ b/drivers/mmc/host/sdhci-pxav3.c
@@ -455,7 +455,7 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
 
 	ret = sdhci_add_host(host);
 	if (ret) {
-		dev_err(&pdev->dev, "failed to add host\n");
+		dev_err(&pdev->dev, "failed to add host ret - %d\n", ret);
 		goto err_add_host;
 	}
 
-- 
1.9.1

[PATCH 1/2] mmc: sdhci-pxav3: Fix tabbing issue

[Joe Perches]

On Wed, 2015-09-02 at 00:54 +0530, Vaibhav Hiremath wrote:
> There were some coding style issues where spaces have been used instead
> of tabs, for example, in macro definitions, alignment of function
> declarations/definitions, etc...
> 
> This patch fixes all such occurrences in the code.
> And also use BIT for bit definitions.

Please send 2 patches instead.

> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
[]
> @@ -128,7 +133,7 @@ static int mv_conf_mbus_windows(struct platform_device *pdev,
>  }
>  
>  static int armada_38x_quirks(struct platform_device *pdev,
> -			     struct sdhci_host *host)
> +			struct sdhci_host *host)

This is not an improvement.

The old code is aligned to the open parenthesis
and is fine.

> @@ -284,10 +288,10 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
>  	 * FE-2946959
>  	 */
>  	if (pxa->sdio3_conf_reg) {
> -		u8 reg_val  = readb(pxa->sdio3_conf_reg);
> +		u8 reg_val = readb(pxa->sdio3_conf_reg);
>  
>  		if (uhs == MMC_TIMING_UHS_SDR50 ||
> -		    uhs == MMC_TIMING_UHS_DDR50) {
> +				uhs == MMC_TIMING_UHS_DDR50) {

Also not an improvement.

> @@ -304,20 +308,20 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
>  }
>  
>  static const struct sdhci_ops pxav3_sdhci_ops = {
> -	.set_clock = sdhci_set_clock,
> -	.platform_send_init_74_clocks = pxav3_gen_init_74_clocks,
> -	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
> -	.set_bus_width = sdhci_set_bus_width,
> -	.reset = pxav3_reset,
> -	.set_uhs_signaling = pxav3_set_uhs_signaling,
> +	.set_clock			= sdhci_set_clock,
> +	.platform_send_init_74_clocks	= pxav3_gen_init_74_clocks,
> +	.get_max_clock			= sdhci_pltfm_clk_get_max_clock,
> +	.set_bus_width			= sdhci_set_bus_width,
> +	.reset				= pxav3_reset,
> +	.set_uhs_signaling		= pxav3_set_uhs_signaling,

<shrug>  If you want.
>  };
>  
>  static struct sdhci_pltfm_data sdhci_pxav3_pdata = {
> -	.quirks = SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK
> +	.quirks	= SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK

Also not an improvement.  Why change the space to a tab here?

> @@ -343,7 +347,7 @@ static struct sdhci_pxa_platdata *pxav3_get_mmc_pdata(struct device *dev)
>  		return NULL;
>  
>  	if (!of_property_read_u32(np, "mrvl,clk-delay-cycles",
> -				  &clk_delay_cycles))
> +				&clk_delay_cycles))

Not an improvement, etc...

[PATCH 2/2] mmc: sdhci-pxav3: Print ret value on error from sdhci_add_host() fn

[Joe Perches]

On Wed, 2015-09-02 at 00:54 +0530, Vaibhav Hiremath wrote:
> Return value would give clear information about the actual root-cause
> of the failure.

I'm not sure why that is as nearly every error path in
sdhci_add_host emits a message.

> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
[]
> @@ -455,7 +455,7 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
>  
>  	ret = sdhci_add_host(host);
>  	if (ret) {
> -		dev_err(&pdev->dev, "failed to add host\n");
> +		dev_err(&pdev->dev, "failed to add host ret - %d\n", ret);
>  		goto err_add_host;
>  	}
>  

If this is really desirable, there are many other callers of
sdhci_add_host with error messages just like this one.

[PATCH 1/2] mmc: sdhci-pxav3: Fix tabbing issue

[Vaibhav Hiremath]

On Wednesday 02 September 2015 02:01 AM, Joe Perches wrote:
> On Wed, 2015-09-02 at 00:54 +0530, Vaibhav Hiremath wrote:
>> There were some coding style issues where spaces have been used instead
>> of tabs, for example, in macro definitions, alignment of function
>> declarations/definitions, etc...
>>
>> This patch fixes all such occurrences in the code.
>> And also use BIT for bit definitions.
>
> Please send 2 patches instead.
>

Really?

I had two separate patches earlier, but after looking at the changes
squahsed them. Anyway, I do not have any strong opinion on this, so
will separate it in next version.


>> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
> []
>> @@ -128,7 +133,7 @@ static int mv_conf_mbus_windows(struct platform_device *pdev,
>>   }
>>
>>   static int armada_38x_quirks(struct platform_device *pdev,
>> -			     struct sdhci_host *host)
>> +			struct sdhci_host *host)
>
> This is not an improvement.
>
> The old code is aligned to the open parenthesis
> and is fine.
>

Ok,
Same here, I always use tabs. No strong opinion, so will remove it.

>> @@ -284,10 +288,10 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
>>   	 * FE-2946959
>>   	 */
>>   	if (pxa->sdio3_conf_reg) {
>> -		u8 reg_val  = readb(pxa->sdio3_conf_reg);
>> +		u8 reg_val = readb(pxa->sdio3_conf_reg);
>>
>>   		if (uhs == MMC_TIMING_UHS_SDR50 ||
>> -		    uhs == MMC_TIMING_UHS_DDR50) {
>> +				uhs == MMC_TIMING_UHS_DDR50) {
>
> Also not an improvement.
>
>> @@ -304,20 +308,20 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
>>   }
>>
>>   static const struct sdhci_ops pxav3_sdhci_ops = {
>> -	.set_clock = sdhci_set_clock,
>> -	.platform_send_init_74_clocks = pxav3_gen_init_74_clocks,
>> -	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
>> -	.set_bus_width = sdhci_set_bus_width,
>> -	.reset = pxav3_reset,
>> -	.set_uhs_signaling = pxav3_set_uhs_signaling,
>> +	.set_clock			= sdhci_set_clock,
>> +	.platform_send_init_74_clocks	= pxav3_gen_init_74_clocks,
>> +	.get_max_clock			= sdhci_pltfm_clk_get_max_clock,
>> +	.set_bus_width			= sdhci_set_bus_width,
>> +	.reset				= pxav3_reset,
>> +	.set_uhs_signaling		= pxav3_set_uhs_signaling,
>
> <shrug>  If you want.

Please clarify.

Thanks,
Vaibhav

[PATCH 2/2] mmc: sdhci-pxav3: Print ret value on error from sdhci_add_host() fn

[Vaibhav Hiremath]

On Wednesday 02 September 2015 02:07 AM, Joe Perches wrote:
> On Wed, 2015-09-02 at 00:54 +0530, Vaibhav Hiremath wrote:
>> Return value would give clear information about the actual root-cause
>> of the failure.
>
> I'm not sure why that is as nearly every error path in
> sdhci_add_host emits a message.
>

Not for everything.
No error message for -EPROBE_DEFER.

>> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
> []
>> @@ -455,7 +455,7 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
>>
>>   	ret = sdhci_add_host(host);
>>   	if (ret) {
>> -		dev_err(&pdev->dev, "failed to add host\n");
>> +		dev_err(&pdev->dev, "failed to add host ret - %d\n", ret);
>>   		goto err_add_host;
>>   	}
>>
>
> If this is really desirable, there are many other callers of
> sdhci_add_host with error messages just like this one.
>

Yes, true.
Ho about, adding pr_err into sdhci_add_host for -EPROBE_DEFER.

Thanks,
Vaibhav

[PATCH 1/2] mmc: sdhci-pxav3: Fix tabbing issue

[Jisheng Zhang]

On Wed, 2 Sep 2015 00:54:13 +0530
Vaibhav Hiremath <vaibhav.hiremath@linaro.org> wrote:

> There were some coding style issues where spaces have been used instead
> of tabs, for example, in macro definitions, alignment of function
> declarations/definitions, etc...
> 
> This patch fixes all such occurrences in the code.
> And also use BIT for bit definitions.
> 
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> ---
>  drivers/mmc/host/sdhci-pxav3.c | 63 ++++++++++++++++++++++--------------------
>  1 file changed, 33 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
> index 946d37f..d02bc37 100644
> --- a/drivers/mmc/host/sdhci-pxav3.c
> +++ b/drivers/mmc/host/sdhci-pxav3.c
> @@ -39,24 +39,29 @@
>  #include "sdhci.h"
>  #include "sdhci-pltfm.h"
>  
> -#define PXAV3_RPM_DELAY_MS     50
> +#define PXAV3_RPM_DELAY_MS		50
>  
> -#define SD_CLOCK_BURST_SIZE_SETUP		0x10A
> -#define SDCLK_SEL	0x100
> -#define SDCLK_DELAY_SHIFT	9
> -#define SDCLK_DELAY_MASK	0x1f
> +#define SD_CLOCK_BURST_SIZE_SETUP	0x10A
> +#define SDCLK_SEL			0x100
> +#define  SDCLK_DELAY_SHIFT		9
> +#define  SDCLK_DELAY_MASK		0x1f
>  
> -#define SD_CFG_FIFO_PARAM       0x100
> -#define SDCFG_GEN_PAD_CLK_ON	(1<<6)
> -#define SDCFG_GEN_PAD_CLK_CNT_MASK	0xFF
> -#define SDCFG_GEN_PAD_CLK_CNT_SHIFT	24
> +#define SD_CFG_FIFO_PARAM		0x100
> +#define  SDCFG_GEN_PAD_CLK_ON		BIT(6)
> +#define  SDCFG_GEN_PAD_CLK_CNT_MASK	0xFF
> +#define  SDCFG_GEN_PAD_CLK_CNT_SHIFT	24
>  
> -#define SD_SPI_MODE          0x108
> -#define SD_CE_ATA_1          0x10C
> +#define SD_SPI_MODE			0x108
> +#define SD_CE_ATA_1			0x10C
>  
> -#define SD_CE_ATA_2          0x10E
> -#define SDCE_MISC_INT		(1<<2)
> -#define SDCE_MISC_INT_EN	(1<<1)
> +#define SD_CE_ATA_2			0x10E
> +#define  SDCE_MISC_INT			BIT(2)
> +#define  SDCE_MISC_INT_EN		BIT(1)

BIT or (1 << y) are both fine, is there any reason to change to BIT?
If so, there will be lots of source code need such changes.

> +
> +/* IO Power control */
> +#define IO_PWR_AKEY_ASFAR		0xbaba
> +#define IO_PWR_AKEY_ASSAR		0xeb10
> +#define IO_PWR_MMC1_PAD_1V8		BIT(2)
>  
>  struct sdhci_pxa {
>  	struct clk *clk_core;
> @@ -128,7 +133,7 @@ static int mv_conf_mbus_windows(struct platform_device *pdev,
>  }
>  
>  static int armada_38x_quirks(struct platform_device *pdev,
> -			     struct sdhci_host *host)
> +			struct sdhci_host *host)
>  {
>  	struct device_node *np = pdev->dev.of_node;
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> @@ -136,8 +141,7 @@ static int armada_38x_quirks(struct platform_device *pdev,
>  	struct resource *res;
>  
>  	host->quirks |= SDHCI_QUIRK_MISSING_CAPS;
> -	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> -					   "conf-sdio3");
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "conf-sdio3");
>  	if (res) {
>  		pxa->sdio3_conf_reg = devm_ioremap_resource(&pdev->dev, res);
>  		if (IS_ERR(pxa->sdio3_conf_reg))
> @@ -284,10 +288,10 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
>  	 * FE-2946959
>  	 */
>  	if (pxa->sdio3_conf_reg) {
> -		u8 reg_val  = readb(pxa->sdio3_conf_reg);
> +		u8 reg_val = readb(pxa->sdio3_conf_reg);
>  
>  		if (uhs == MMC_TIMING_UHS_SDR50 ||
> -		    uhs == MMC_TIMING_UHS_DDR50) {
> +				uhs == MMC_TIMING_UHS_DDR50) {
>  			reg_val &= ~SDIO3_CONF_CLK_INV;
>  			reg_val |= SDIO3_CONF_SD_FB_CLK;
>  		} else {
> @@ -304,20 +308,20 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
>  }
>  
>  static const struct sdhci_ops pxav3_sdhci_ops = {
> -	.set_clock = sdhci_set_clock,
> -	.platform_send_init_74_clocks = pxav3_gen_init_74_clocks,
> -	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
> -	.set_bus_width = sdhci_set_bus_width,
> -	.reset = pxav3_reset,
> -	.set_uhs_signaling = pxav3_set_uhs_signaling,
> +	.set_clock			= sdhci_set_clock,
> +	.platform_send_init_74_clocks	= pxav3_gen_init_74_clocks,
> +	.get_max_clock			= sdhci_pltfm_clk_get_max_clock,
> +	.set_bus_width			= sdhci_set_bus_width,
> +	.reset				= pxav3_reset,
> +	.set_uhs_signaling		= pxav3_set_uhs_signaling,
>  };

IMHO these two styles are both fine. For example, the mmc_sd_ops structure in
drivers/mmc/core/sd.c also follows the sdhci-pxav3's original coding style

>  
>  static struct sdhci_pltfm_data sdhci_pxav3_pdata = {
> -	.quirks = SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK
> +	.quirks	= SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK
>  		| SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC
>  		| SDHCI_QUIRK_32BIT_ADMA_SIZE
>  		| SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
> -	.ops = &pxav3_sdhci_ops,
> +	.ops	= &pxav3_sdhci_ops,
>  };
>  
>  #ifdef CONFIG_OF
> @@ -343,7 +347,7 @@ static struct sdhci_pxa_platdata *pxav3_get_mmc_pdata(struct device *dev)
>  		return NULL;
>  
>  	if (!of_property_read_u32(np, "mrvl,clk-delay-cycles",
> -				  &clk_delay_cycles))
> +				&clk_delay_cycles))
>  		pdata->clk_delay_cycles = clk_delay_cycles;
>  
>  	return pdata;
> @@ -433,8 +437,7 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
>  			host->mmc->pm_caps |= pdata->pm_caps;
>  
>  		if (gpio_is_valid(pdata->ext_cd_gpio)) {
> -			ret = mmc_gpio_request_cd(host->mmc, pdata->ext_cd_gpio,
> -						  0);
> +			ret = mmc_gpio_request_cd(host->mmc, pdata->ext_cd_gpio, 0);
>  			if (ret) {
>  				dev_err(mmc_dev(host->mmc),
>  					"failed to allocate card detect gpio\n");

[PATCH 1/2] mmc: sdhci-pxav3: Fix tabbing issue

[Vaibhav Hiremath]

On Wednesday 02 September 2015 12:24 PM, Jisheng Zhang wrote:
> On Wed, 2 Sep 2015 00:54:13 +0530
> Vaibhav Hiremath <vaibhav.hiremath@linaro.org> wrote:
>
>> There were some coding style issues where spaces have been used instead
>> of tabs, for example, in macro definitions, alignment of function
>> declarations/definitions, etc...
>>
>> This patch fixes all such occurrences in the code.
>> And also use BIT for bit definitions.
>>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>> ---
>>   drivers/mmc/host/sdhci-pxav3.c | 63 ++++++++++++++++++++++--------------------
>>   1 file changed, 33 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
>> index 946d37f..d02bc37 100644
>> --- a/drivers/mmc/host/sdhci-pxav3.c
>> +++ b/drivers/mmc/host/sdhci-pxav3.c
>> @@ -39,24 +39,29 @@
>>   #include "sdhci.h"
>>   #include "sdhci-pltfm.h"
>>
>> -#define PXAV3_RPM_DELAY_MS     50
>> +#define PXAV3_RPM_DELAY_MS		50
>>
>> -#define SD_CLOCK_BURST_SIZE_SETUP		0x10A
>> -#define SDCLK_SEL	0x100
>> -#define SDCLK_DELAY_SHIFT	9
>> -#define SDCLK_DELAY_MASK	0x1f
>> +#define SD_CLOCK_BURST_SIZE_SETUP	0x10A
>> +#define SDCLK_SEL			0x100
>> +#define  SDCLK_DELAY_SHIFT		9
>> +#define  SDCLK_DELAY_MASK		0x1f
>>
>> -#define SD_CFG_FIFO_PARAM       0x100
>> -#define SDCFG_GEN_PAD_CLK_ON	(1<<6)
>> -#define SDCFG_GEN_PAD_CLK_CNT_MASK	0xFF
>> -#define SDCFG_GEN_PAD_CLK_CNT_SHIFT	24
>> +#define SD_CFG_FIFO_PARAM		0x100
>> +#define  SDCFG_GEN_PAD_CLK_ON		BIT(6)
>> +#define  SDCFG_GEN_PAD_CLK_CNT_MASK	0xFF
>> +#define  SDCFG_GEN_PAD_CLK_CNT_SHIFT	24
>>
>> -#define SD_SPI_MODE          0x108
>> -#define SD_CE_ATA_1          0x10C
>> +#define SD_SPI_MODE			0x108
>> +#define SD_CE_ATA_1			0x10C
>>
>> -#define SD_CE_ATA_2          0x10E
>> -#define SDCE_MISC_INT		(1<<2)
>> -#define SDCE_MISC_INT_EN	(1<<1)
>> +#define SD_CE_ATA_2			0x10E
>> +#define  SDCE_MISC_INT			BIT(2)
>> +#define  SDCE_MISC_INT_EN		BIT(1)
>
> BIT or (1 << y) are both fine, is there any reason to change to BIT?
> If so, there will be lots of source code need such changes.
>

I have received comments for my other patches (PMIC/Regulator/mfd),
so applying same here as well before doing any real
functionality/coding changes to the driver.


>> +
>> +/* IO Power control */
>> +#define IO_PWR_AKEY_ASFAR		0xbaba
>> +#define IO_PWR_AKEY_ASSAR		0xeb10
>> +#define IO_PWR_MMC1_PAD_1V8		BIT(2)
>>
>>   struct sdhci_pxa {
>>   	struct clk *clk_core;
>> @@ -128,7 +133,7 @@ static int mv_conf_mbus_windows(struct platform_device *pdev,
>>   }
>>
>>   static int armada_38x_quirks(struct platform_device *pdev,
>> -			     struct sdhci_host *host)
>> +			struct sdhci_host *host)
>>   {
>>   	struct device_node *np = pdev->dev.of_node;
>>   	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> @@ -136,8 +141,7 @@ static int armada_38x_quirks(struct platform_device *pdev,
>>   	struct resource *res;
>>
>>   	host->quirks |= SDHCI_QUIRK_MISSING_CAPS;
>> -	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> -					   "conf-sdio3");
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "conf-sdio3");
>>   	if (res) {
>>   		pxa->sdio3_conf_reg = devm_ioremap_resource(&pdev->dev, res);
>>   		if (IS_ERR(pxa->sdio3_conf_reg))
>> @@ -284,10 +288,10 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
>>   	 * FE-2946959
>>   	 */
>>   	if (pxa->sdio3_conf_reg) {
>> -		u8 reg_val  = readb(pxa->sdio3_conf_reg);
>> +		u8 reg_val = readb(pxa->sdio3_conf_reg);
>>
>>   		if (uhs == MMC_TIMING_UHS_SDR50 ||
>> -		    uhs == MMC_TIMING_UHS_DDR50) {
>> +				uhs == MMC_TIMING_UHS_DDR50) {
>>   			reg_val &= ~SDIO3_CONF_CLK_INV;
>>   			reg_val |= SDIO3_CONF_SD_FB_CLK;
>>   		} else {
>> @@ -304,20 +308,20 @@ static void pxav3_set_uhs_signaling(struct sdhci_host *host, unsigned int uhs)
>>   }
>>
>>   static const struct sdhci_ops pxav3_sdhci_ops = {
>> -	.set_clock = sdhci_set_clock,
>> -	.platform_send_init_74_clocks = pxav3_gen_init_74_clocks,
>> -	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
>> -	.set_bus_width = sdhci_set_bus_width,
>> -	.reset = pxav3_reset,
>> -	.set_uhs_signaling = pxav3_set_uhs_signaling,
>> +	.set_clock			= sdhci_set_clock,
>> +	.platform_send_init_74_clocks	= pxav3_gen_init_74_clocks,
>> +	.get_max_clock			= sdhci_pltfm_clk_get_max_clock,
>> +	.set_bus_width			= sdhci_set_bus_width,
>> +	.reset				= pxav3_reset,
>> +	.set_uhs_signaling		= pxav3_set_uhs_signaling,
>>   };
>
> IMHO these two styles are both fine. For example, the mmc_sd_ops structure in
> drivers/mmc/core/sd.c also follows the sdhci-pxav3's original coding style
>
>>

Ditto here.

I did similar changes to mfd driver, so applied same changes here as
well.

Trying to avoid another spin of actual patches due to such trivial 
comments :)


Thanks,
Vaibhav

[PATCH 1/2] mmc: sdhci-pxav3: Fix tabbing issue

[Vaibhav Hiremath]

On Wednesday 02 September 2015 02:01 AM, Joe Perches wrote:
> On Wed, 2015-09-02 at 00:54 +0530, Vaibhav Hiremath wrote:
>> There were some coding style issues where spaces have been used instead
>> of tabs, for example, in macro definitions, alignment of function
>> declarations/definitions, etc...
>>
>> This patch fixes all such occurrences in the code.
>> And also use BIT for bit definitions.
>
> Please send 2 patches instead.
>
>> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
> []
>> @@ -128,7 +133,7 @@ static int mv_conf_mbus_windows(struct platform_device *pdev,
>>   }
>>
>>   static int armada_38x_quirks(struct platform_device *pdev,
>> -			     struct sdhci_host *host)
>> +			struct sdhci_host *host)
>
> This is not an improvement.
>

Missed to highlight one more point here,

The kernel CodingStyle talks about indentation, pasting it below -

-------
Outside of comments, documentation and except in Kconfig, spaces are
never used for indentation.
-------

But I see spaces are being used at lot of places :)

Till now all the patches or drivers which I submitted, I never used
spaces. :)
Anyway, just wanted to highlight CodingStyle document here.

Thanks,
Vaibhav

[PATCH 2/2] mmc: sdhci-pxav3: Print ret value on error from sdhci_add_host() fn

[Vaibhav Hiremath]

On Wednesday 02 September 2015 02:07 AM, Joe Perches wrote:
> On Wed, 2015-09-02 at 00:54 +0530, Vaibhav Hiremath wrote:
>> Return value would give clear information about the actual root-cause
>> of the failure.
>> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
>> @@ -455,7 +455,7 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
>>
>>   	ret = sdhci_add_host(host);
>>   	if (ret) {
>> -		dev_err(&pdev->dev, "failed to add host\n");
>> +		dev_err(&pdev->dev, "failed to add host ret - %d\n", ret);
>>   		goto err_add_host;
>>   	}
>>
>
> If this is really desirable, there are many other callers of
> sdhci_add_host with error messages just like this one.
>

How about this? If you are ok, I can change it and submit the patch
again.


diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index d2caa60..3a4902c 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -3176,8 +3176,11 @@ int sdhci_add_host(struct sdhci_host *host)
                 mmc->caps |= MMC_CAP_NEEDS_POLL;

         /* If there are external regulators, get them */
-       if (mmc_regulator_get_supply(mmc) == -EPROBE_DEFER)
+       if (mmc_regulator_get_supply(mmc) == -EPROBE_DEFER) {
+               pr_err("%s: regulator supply unavailable, deferring 
probe. \n",
+                               mmc_hostname(mmc));
                 return -EPROBE_DEFER;
+       }

         /* If vqmmc regulator and no 1.8V signalling, then there's no 
UHS */
         if (!IS_ERR(mmc->supply.vqmmc)) {


Thanks,
Vaibhav

[PATCH 2/2] mmc: sdhci-pxav3: Print ret value on error from sdhci_add_host() fn

[Joe Perches]

On Wed, 2015-09-02 at 18:37 +0530, Vaibhav Hiremath wrote:
> On Wednesday 02 September 2015 02:07 AM, Joe Perches wrote:
> > On Wed, 2015-09-02 at 00:54 +0530, Vaibhav Hiremath wrote:
> >> Return value would give clear information about the actual root-cause
> >> of the failure.
> >> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
> >> @@ -455,7 +455,7 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
> >>
> >>   	ret = sdhci_add_host(host);
> >>   	if (ret) {
> >> -		dev_err(&pdev->dev, "failed to add host\n");
> >> +		dev_err(&pdev->dev, "failed to add host ret - %d\n", ret);
> >>   		goto err_add_host;
> >>   	}
> >
> > If this is really desirable, there are many other callers of
> > sdhci_add_host with error messages just like this one.
> >
> How about this? If you are ok, I can change it and submit the patch
> again.
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
[]
> @@ -3176,8 +3176,11 @@ int sdhci_add_host(struct sdhci_host *host)
>                  mmc->caps |= MMC_CAP_NEEDS_POLL;
> 
>          /* If there are external regulators, get them */
> -       if (mmc_regulator_get_supply(mmc) == -EPROBE_DEFER)
> +       if (mmc_regulator_get_supply(mmc) == -EPROBE_DEFER) {
> +               pr_err("%s: regulator supply unavailable, deferring 
> probe. \n",
> +                               mmc_hostname(mmc));
>                  return -EPROBE_DEFER;
> +       }

(your email client has inappropriate line wrapping)

The KERN_<LEVEL> here probably isn't right.

Deferring isn't an error, at best it's a notification
and perhaps should be at pr_notice/KERN_NOTICE

I don't know how often or how many times this deferral
can occur.  Do you?

If it's moderately common, that message should likely
be ratelimited if it exists at all.

[PATCH 2/2] mmc: sdhci-pxav3: Print ret value on error from sdhci_add_host() fn

[Vaibhav Hiremath]

On Wednesday 02 September 2015 08:37 PM, Joe Perches wrote:
> On Wed, 2015-09-02 at 18:37 +0530, Vaibhav Hiremath wrote:
>> On Wednesday 02 September 2015 02:07 AM, Joe Perches wrote:
>>> On Wed, 2015-09-02 at 00:54 +0530, Vaibhav Hiremath wrote:
>>>> Return value would give clear information about the actual root-cause
>>>> of the failure.
>>>> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
>>>> @@ -455,7 +455,7 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
>>>>
>>>>    	ret = sdhci_add_host(host);
>>>>    	if (ret) {
>>>> -		dev_err(&pdev->dev, "failed to add host\n");
>>>> +		dev_err(&pdev->dev, "failed to add host ret - %d\n", ret);
>>>>    		goto err_add_host;
>>>>    	}
>>>
>>> If this is really desirable, there are many other callers of
>>> sdhci_add_host with error messages just like this one.
>>>
>> How about this? If you are ok, I can change it and submit the patch
>> again.
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> []
>> @@ -3176,8 +3176,11 @@ int sdhci_add_host(struct sdhci_host *host)
>>                   mmc->caps |= MMC_CAP_NEEDS_POLL;
>>
>>           /* If there are external regulators, get them */
>> -       if (mmc_regulator_get_supply(mmc) == -EPROBE_DEFER)
>> +       if (mmc_regulator_get_supply(mmc) == -EPROBE_DEFER) {
>> +               pr_err("%s: regulator supply unavailable, deferring
>> probe. \n",
>> +                               mmc_hostname(mmc));
>>                   return -EPROBE_DEFER;
>> +       }
>
> (your email client has inappropriate line wrapping)
>
> The KERN_<LEVEL> here probably isn't right.
>
> Deferring isn't an error, at best it's a notification

I would consider it as an ERROR if it gets deferred
continuously/multiple times due to same reason.

> and perhaps should be at pr_notice/KERN_NOTICE
>

Yeah, KERN_NOTICE looks right here.

> I don't know how often or how many times this deferral
> can occur.  Do you?
>

-EDEFER_PROBE usually means that driver has some dependency,
for which it has to wait.
In my case, during every boot, I pxav3_sdhci_probe gets deferred once
due to regulator unavailability.

Thanks,
Vaibhav

[PATCH 2/2] mmc: sdhci-pxav3: Print ret value on error from sdhci_add_host() fn

[Ulf Hansson]

On 2 September 2015 at 17:16, Vaibhav Hiremath
<vaibhav.hiremath@linaro.org> wrote:
>
>
> On Wednesday 02 September 2015 08:37 PM, Joe Perches wrote:
>>
>> On Wed, 2015-09-02 at 18:37 +0530, Vaibhav Hiremath wrote:
>>>
>>> On Wednesday 02 September 2015 02:07 AM, Joe Perches wrote:
>>>>
>>>> On Wed, 2015-09-02 at 00:54 +0530, Vaibhav Hiremath wrote:
>>>>>
>>>>> Return value would give clear information about the actual root-cause
>>>>> of the failure.
>>>>> diff --git a/drivers/mmc/host/sdhci-pxav3.c
>>>>> b/drivers/mmc/host/sdhci-pxav3.c
>>>>> @@ -455,7 +455,7 @@ static int sdhci_pxav3_probe(struct platform_device
>>>>> *pdev)
>>>>>
>>>>>         ret = sdhci_add_host(host);
>>>>>         if (ret) {
>>>>> -               dev_err(&pdev->dev, "failed to add host\n");
>>>>> +               dev_err(&pdev->dev, "failed to add host ret - %d\n",
>>>>> ret);
>>>>>                 goto err_add_host;
>>>>>         }
>>>>
>>>>
>>>> If this is really desirable, there are many other callers of
>>>> sdhci_add_host with error messages just like this one.
>>>>
>>> How about this? If you are ok, I can change it and submit the patch
>>> again.
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>
>> []
>>>
>>> @@ -3176,8 +3176,11 @@ int sdhci_add_host(struct sdhci_host *host)
>>>                   mmc->caps |= MMC_CAP_NEEDS_POLL;
>>>
>>>           /* If there are external regulators, get them */
>>> -       if (mmc_regulator_get_supply(mmc) == -EPROBE_DEFER)
>>> +       if (mmc_regulator_get_supply(mmc) == -EPROBE_DEFER) {
>>> +               pr_err("%s: regulator supply unavailable, deferring
>>> probe. \n",
>>> +                               mmc_hostname(mmc));
>>>                   return -EPROBE_DEFER;
>>> +       }
>>
>>
>> (your email client has inappropriate line wrapping)
>>
>> The KERN_<LEVEL> here probably isn't right.
>>
>> Deferring isn't an error, at best it's a notification
>
>
> I would consider it as an ERROR if it gets deferred
> continuously/multiple times due to same reason.
>
>> and perhaps should be at pr_notice/KERN_NOTICE
>>
>
> Yeah, KERN_NOTICE looks right here.
>
>> I don't know how often or how many times this deferral
>> can occur.  Do you?
>>
>
> -EDEFER_PROBE usually means that driver has some dependency,
> for which it has to wait.
> In my case, during every boot, I pxav3_sdhci_probe gets deferred once
> due to regulator unavailability.

The driver core already prints a message on the debug level for
-EDEFER_PROBE. Please don't do that in the driver as well.

Kind regards
Uffe