* [PATCH 0/2] mmc: sdhci-pxav3: Fix tabbing issue @ 2015-09-01 19:24 Vaibhav Hiremath 2015-09-01 19:24 ` [PATCH 1/2] " Vaibhav Hiremath 2015-09-01 19:24 ` [PATCH 2/2] mmc: sdhci-pxav3: Print ret value on error from sdhci_add_host() fn Vaibhav Hiremath 0 siblings, 2 replies; 14+ messages in thread From: Vaibhav Hiremath @ 2015-09-01 19:24 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] mmc: sdhci-pxav3: Fix tabbing issue 2015-09-01 19:24 [PATCH 0/2] mmc: sdhci-pxav3: Fix tabbing issue Vaibhav Hiremath @ 2015-09-01 19:24 ` Vaibhav Hiremath 2015-09-01 20:31 ` Joe Perches 2015-09-02 6:54 ` Jisheng Zhang 2015-09-01 19:24 ` [PATCH 2/2] mmc: sdhci-pxav3: Print ret value on error from sdhci_add_host() fn Vaibhav Hiremath 1 sibling, 2 replies; 14+ messages in thread From: Vaibhav Hiremath @ 2015-09-01 19:24 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 1/2] mmc: sdhci-pxav3: Fix tabbing issue 2015-09-01 19:24 ` [PATCH 1/2] " Vaibhav Hiremath @ 2015-09-01 20:31 ` Joe Perches 2015-09-02 6:29 ` Vaibhav Hiremath 2015-09-02 12:25 ` Vaibhav Hiremath 2015-09-02 6:54 ` Jisheng Zhang 1 sibling, 2 replies; 14+ messages in thread From: Joe Perches @ 2015-09-01 20:31 UTC (permalink / raw) To: linux-arm-kernel 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... ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] mmc: sdhci-pxav3: Fix tabbing issue 2015-09-01 20:31 ` Joe Perches @ 2015-09-02 6:29 ` Vaibhav Hiremath 2015-09-02 12:25 ` Vaibhav Hiremath 1 sibling, 0 replies; 14+ messages in thread From: Vaibhav Hiremath @ 2015-09-02 6:29 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] mmc: sdhci-pxav3: Fix tabbing issue 2015-09-01 20:31 ` Joe Perches 2015-09-02 6:29 ` Vaibhav Hiremath @ 2015-09-02 12:25 ` Vaibhav Hiremath 1 sibling, 0 replies; 14+ messages in thread From: Vaibhav Hiremath @ 2015-09-02 12:25 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] mmc: sdhci-pxav3: Fix tabbing issue 2015-09-01 19:24 ` [PATCH 1/2] " Vaibhav Hiremath 2015-09-01 20:31 ` Joe Perches @ 2015-09-02 6:54 ` Jisheng Zhang 2015-09-02 7:39 ` Vaibhav Hiremath 1 sibling, 1 reply; 14+ messages in thread From: Jisheng Zhang @ 2015-09-02 6:54 UTC (permalink / raw) To: linux-arm-kernel 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"); ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] mmc: sdhci-pxav3: Fix tabbing issue 2015-09-02 6:54 ` Jisheng Zhang @ 2015-09-02 7:39 ` Vaibhav Hiremath 0 siblings, 0 replies; 14+ messages in thread From: Vaibhav Hiremath @ 2015-09-02 7:39 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] mmc: sdhci-pxav3: Print ret value on error from sdhci_add_host() fn 2015-09-01 19:24 [PATCH 0/2] mmc: sdhci-pxav3: Fix tabbing issue Vaibhav Hiremath 2015-09-01 19:24 ` [PATCH 1/2] " Vaibhav Hiremath @ 2015-09-01 19:24 ` Vaibhav Hiremath 2015-09-01 20:37 ` Joe Perches 1 sibling, 1 reply; 14+ messages in thread From: Vaibhav Hiremath @ 2015-09-01 19:24 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/2] mmc: sdhci-pxav3: Print ret value on error from sdhci_add_host() fn 2015-09-01 19:24 ` [PATCH 2/2] mmc: sdhci-pxav3: Print ret value on error from sdhci_add_host() fn Vaibhav Hiremath @ 2015-09-01 20:37 ` Joe Perches 2015-09-02 6:37 ` Vaibhav Hiremath 2015-09-02 13:07 ` Vaibhav Hiremath 0 siblings, 2 replies; 14+ messages in thread From: Joe Perches @ 2015-09-01 20:37 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] mmc: sdhci-pxav3: Print ret value on error from sdhci_add_host() fn 2015-09-01 20:37 ` Joe Perches @ 2015-09-02 6:37 ` Vaibhav Hiremath 2015-09-02 13:07 ` Vaibhav Hiremath 1 sibling, 0 replies; 14+ messages in thread From: Vaibhav Hiremath @ 2015-09-02 6:37 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] mmc: sdhci-pxav3: Print ret value on error from sdhci_add_host() fn 2015-09-01 20:37 ` Joe Perches 2015-09-02 6:37 ` Vaibhav Hiremath @ 2015-09-02 13:07 ` Vaibhav Hiremath 2015-09-02 15:07 ` Joe Perches 1 sibling, 1 reply; 14+ messages in thread From: Vaibhav Hiremath @ 2015-09-02 13:07 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/2] mmc: sdhci-pxav3: Print ret value on error from sdhci_add_host() fn 2015-09-02 13:07 ` Vaibhav Hiremath @ 2015-09-02 15:07 ` Joe Perches 2015-09-02 15:16 ` Vaibhav Hiremath 0 siblings, 1 reply; 14+ messages in thread From: Joe Perches @ 2015-09-02 15:07 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] mmc: sdhci-pxav3: Print ret value on error from sdhci_add_host() fn 2015-09-02 15:07 ` Joe Perches @ 2015-09-02 15:16 ` Vaibhav Hiremath 2015-09-15 11:53 ` Ulf Hansson 0 siblings, 1 reply; 14+ messages in thread From: Vaibhav Hiremath @ 2015-09-02 15:16 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] mmc: sdhci-pxav3: Print ret value on error from sdhci_add_host() fn 2015-09-02 15:16 ` Vaibhav Hiremath @ 2015-09-15 11:53 ` Ulf Hansson 0 siblings, 0 replies; 14+ messages in thread From: Ulf Hansson @ 2015-09-15 11:53 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2015-09-15 11:53 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-09-01 19:24 [PATCH 0/2] mmc: sdhci-pxav3: Fix tabbing issue Vaibhav Hiremath 2015-09-01 19:24 ` [PATCH 1/2] " Vaibhav Hiremath 2015-09-01 20:31 ` Joe Perches 2015-09-02 6:29 ` Vaibhav Hiremath 2015-09-02 12:25 ` Vaibhav Hiremath 2015-09-02 6:54 ` Jisheng Zhang 2015-09-02 7:39 ` Vaibhav Hiremath 2015-09-01 19:24 ` [PATCH 2/2] mmc: sdhci-pxav3: Print ret value on error from sdhci_add_host() fn Vaibhav Hiremath 2015-09-01 20:37 ` Joe Perches 2015-09-02 6:37 ` Vaibhav Hiremath 2015-09-02 13:07 ` Vaibhav Hiremath 2015-09-02 15:07 ` Joe Perches 2015-09-02 15:16 ` Vaibhav Hiremath 2015-09-15 11:53 ` Ulf Hansson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).