| Message ID | 1303397412-24813-2-git-send-email-grinberg@compulab.co.il (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 21, 2011 at 09:50, Igor Grinberg <grinberg@compulab.co.il> wrote: > replace "printk(KERN_ERR" by "pr_err(" > and fix needlessly multi-lined #ifdef > > Signed-off-by: Igor Grinberg <grinberg@compulab.co.il> > --- > arch/arm/mach-omap2/gpmc-smsc911x.c | 14 +++++++------- > arch/arm/plat-omap/include/plat/gpmc-smsc911x.h | 3 +-- > 2 files changed, 8 insertions(+), 9 deletions(-) > > diff --git a/arch/arm/mach-omap2/gpmc-smsc911x.c b/arch/arm/mach-omap2/gpmc-smsc911x.c > index d30293a..b45efff 100644 > --- a/arch/arm/mach-omap2/gpmc-smsc911x.c > +++ b/arch/arm/mach-omap2/gpmc-smsc911x.c minor suggestion: wont using pr_fmt help to reduce the need to add %s, __func__ for pr_err through out the file? Regards, Nishanth Menon > @@ -55,7 +55,7 @@ void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *board_data) > gpmc_cfg = board_data; > > if (gpmc_cs_request(gpmc_cfg->cs, SZ_16M, &cs_mem_base) < 0) { > - printk(KERN_ERR "Failed to request GPMC mem for smsc911x\n"); > + pr_err("%s: Failed to request GPMC mem region\n", __func__); > return; > } > > @@ -63,8 +63,8 @@ void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *board_data) > gpmc_smsc911x_resources[0].end = cs_mem_base + 0xff; > > if (gpio_request(gpmc_cfg->gpio_irq, "smsc911x irq") < 0) { > - printk(KERN_ERR "Failed to request GPIO%d for smsc911x IRQ\n", > - gpmc_cfg->gpio_irq); > + pr_err("%s: Failed to request IRQ GPIO%d\n", __func__, > + gpmc_cfg->gpio_irq); > goto free1; > } > > @@ -74,8 +74,8 @@ void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *board_data) > if (gpio_is_valid(gpmc_cfg->gpio_reset)) { > ret = gpio_request(gpmc_cfg->gpio_reset, "smsc911x reset"); > if (ret) { > - printk(KERN_ERR "Failed to request GPIO%d for smsc911x reset\n", > - gpmc_cfg->gpio_reset); > + pr_err("%s: Failed to request reset GPIO%d\n", __func__, > + gpmc_cfg->gpio_reset); > goto free2; > } > > @@ -92,7 +92,7 @@ void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *board_data) > gpmc_smsc911x_resources, ARRAY_SIZE(gpmc_smsc911x_resources), > &gpmc_smsc911x_config, sizeof(gpmc_smsc911x_config)); > if (!pdev) { > - printk(KERN_ERR "Unable to register smsc911x device\n"); > + pr_err("%s: Unable to register platform device\n", __func__); > gpio_free(gpmc_cfg->gpio_reset); > goto free2; > } > @@ -104,5 +104,5 @@ free2: > free1: > gpmc_cs_free(gpmc_cfg->cs); > > - printk(KERN_ERR "Could not initialize smsc911x\n"); > + pr_err("Could not initialize smsc911x\n"); > } > diff --git a/arch/arm/plat-omap/include/plat/gpmc-smsc911x.h b/arch/arm/plat-omap/include/plat/gpmc-smsc911x.h > index d3f1579..ea6c9c8 100644 > --- a/arch/arm/plat-omap/include/plat/gpmc-smsc911x.h > +++ b/arch/arm/plat-omap/include/plat/gpmc-smsc911x.h > @@ -21,8 +21,7 @@ struct omap_smsc911x_platform_data { > u32 flags; > }; > > -#if defined(CONFIG_SMSC911X) || \ > - defined(CONFIG_SMSC911X_MODULE) > +#if defined(CONFIG_SMSC911X) || defined(CONFIG_SMSC911X_MODULE) > > extern void gpmc_smsc911x_init(struct omap_smsc911x_platform_data *d); > > -- > 1.7.3.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Nishanth, On 04/26/11 16:45, Menon, Nishanth wrote: > On Thu, Apr 21, 2011 at 09:50, Igor Grinberg <grinberg@compulab.co.il> wrote: >> replace "printk(KERN_ERR" by "pr_err(" >> and fix needlessly multi-lined #ifdef >> >> Signed-off-by: Igor Grinberg <grinberg@compulab.co.il> >> --- >> arch/arm/mach-omap2/gpmc-smsc911x.c | 14 +++++++------- >> arch/arm/plat-omap/include/plat/gpmc-smsc911x.h | 3 +-- >> 2 files changed, 8 insertions(+), 9 deletions(-) >> >> diff --git a/arch/arm/mach-omap2/gpmc-smsc911x.c b/arch/arm/mach-omap2/gpmc-smsc911x.c >> index d30293a..b45efff 100644 >> --- a/arch/arm/mach-omap2/gpmc-smsc911x.c >> +++ b/arch/arm/mach-omap2/gpmc-smsc911x.c > minor suggestion: wont using pr_fmt help to reduce the need to add %s, > __func__ for pr_err through out the file? I don't understand of what "need" are you talking about. I don't know of any need to add "%s, __func__" with pr_fmt... I've added "%s, __func__" as a meter of choice. IMO, it makes it easier to parse the dmesg output. If anyone objects it, I can remove them, but I think both pr_fmt and __func__ are nice here and way better then "printk(KERN_*" with embedded "smsc911x". >> @@ -55,7 +55,7 @@ void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *board_data) >> gpmc_cfg = board_data; >> >> if (gpmc_cs_request(gpmc_cfg->cs, SZ_16M, &cs_mem_base) < 0) { >> - printk(KERN_ERR "Failed to request GPMC mem for smsc911x\n"); >> + pr_err("%s: Failed to request GPMC mem region\n", __func__); >> return; >> } >> >> @@ -63,8 +63,8 @@ void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *board_data) >> gpmc_smsc911x_resources[0].end = cs_mem_base + 0xff; >> >> if (gpio_request(gpmc_cfg->gpio_irq, "smsc911x irq") < 0) { >> - printk(KERN_ERR "Failed to request GPIO%d for smsc911x IRQ\n", >> - gpmc_cfg->gpio_irq); >> + pr_err("%s: Failed to request IRQ GPIO%d\n", __func__, >> + gpmc_cfg->gpio_irq); >> goto free1; >> } >> >> @@ -74,8 +74,8 @@ void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *board_data) >> if (gpio_is_valid(gpmc_cfg->gpio_reset)) { >> ret = gpio_request(gpmc_cfg->gpio_reset, "smsc911x reset"); >> if (ret) { >> - printk(KERN_ERR "Failed to request GPIO%d for smsc911x reset\n", >> - gpmc_cfg->gpio_reset); >> + pr_err("%s: Failed to request reset GPIO%d\n", __func__, >> + gpmc_cfg->gpio_reset); >> goto free2; >> } >> >> @@ -92,7 +92,7 @@ void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *board_data) >> gpmc_smsc911x_resources, ARRAY_SIZE(gpmc_smsc911x_resources), >> &gpmc_smsc911x_config, sizeof(gpmc_smsc911x_config)); >> if (!pdev) { >> - printk(KERN_ERR "Unable to register smsc911x device\n"); >> + pr_err("%s: Unable to register platform device\n", __func__); >> gpio_free(gpmc_cfg->gpio_reset); >> goto free2; >> } >> @@ -104,5 +104,5 @@ free2: >> free1: >> gpmc_cs_free(gpmc_cfg->cs); >> >> - printk(KERN_ERR "Could not initialize smsc911x\n"); >> + pr_err("Could not initialize smsc911x\n"); >> } >> diff --git a/arch/arm/plat-omap/include/plat/gpmc-smsc911x.h b/arch/arm/plat-omap/include/plat/gpmc-smsc911x.h >> index d3f1579..ea6c9c8 100644 >> --- a/arch/arm/plat-omap/include/plat/gpmc-smsc911x.h >> +++ b/arch/arm/plat-omap/include/plat/gpmc-smsc911x.h >> @@ -21,8 +21,7 @@ struct omap_smsc911x_platform_data { >> u32 flags; >> }; >> >> -#if defined(CONFIG_SMSC911X) || \ >> - defined(CONFIG_SMSC911X_MODULE) >> +#if defined(CONFIG_SMSC911X) || defined(CONFIG_SMSC911X_MODULE) >> >> extern void gpmc_smsc911x_init(struct omap_smsc911x_platform_data *d); >> >> -- >> 1.7.3.4 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-omap" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >>
On Tue, Apr 26, 2011 at 09:40, Igor Grinberg <grinberg@compulab.co.il> wrote: > Hi Nishanth, > > On 04/26/11 16:45, Menon, Nishanth wrote: > >> On Thu, Apr 21, 2011 at 09:50, Igor Grinberg <grinberg@compulab.co.il> wrote: >>> replace "printk(KERN_ERR" by "pr_err(" >>> and fix needlessly multi-lined #ifdef >>> >>> Signed-off-by: Igor Grinberg <grinberg@compulab.co.il> >>> --- >>> arch/arm/mach-omap2/gpmc-smsc911x.c | 14 +++++++------- >>> arch/arm/plat-omap/include/plat/gpmc-smsc911x.h | 3 +-- >>> 2 files changed, 8 insertions(+), 9 deletions(-) >>> >>> diff --git a/arch/arm/mach-omap2/gpmc-smsc911x.c b/arch/arm/mach-omap2/gpmc-smsc911x.c >>> index d30293a..b45efff 100644 >>> --- a/arch/arm/mach-omap2/gpmc-smsc911x.c >>> +++ b/arch/arm/mach-omap2/gpmc-smsc911x.c >> minor suggestion: wont using pr_fmt help to reduce the need to add %s, >> __func__ for pr_err through out the file? > > I don't understand of what "need" are you talking about. > I don't know of any need to add "%s, __func__" with pr_fmt... > I've added "%s, __func__" as a meter of choice. > IMO, it makes it easier to parse the dmesg output. > If anyone objects it, I can remove them, > but I think both pr_fmt and __func__ are nice here and > way better then "printk(KERN_*" with embedded "smsc911x". Allow me to rephrase. I like the change of printk->pr_err - thanks for doing it. just suggesting a minor improvement if you add the following line before the #includes #define pr_fmt(fmt) "%s: " fmt, __func__ then - printk(KERN_ERR "Failed to request GPIO%d for smsc911x IRQ\n", - gpmc_cfg->gpio_irq); + pr_err("%s: Failed to request IRQ GPIO%d\n", __func__, + gpmc_cfg->gpio_irq); becomes, - printk(KERN_ERR "Failed to request GPIO%d for smsc911x IRQ\n", - gpmc_cfg->gpio_irq); + pr_err("Failed to request IRQ GPIO%d\n", gpmc_cfg->gpio_irq); Both give you exactly the output you would like to see (which I personally prefer as well), but the second could be lesser lines of code change ;) Regards, Nishanth Menon > >>> @@ -55,7 +55,7 @@ void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *board_data) >>> gpmc_cfg = board_data; >>> >>> if (gpmc_cs_request(gpmc_cfg->cs, SZ_16M, &cs_mem_base) < 0) { >>> - printk(KERN_ERR "Failed to request GPMC mem for smsc911x\n"); >>> + pr_err("%s: Failed to request GPMC mem region\n", __func__); >>> return; >>> } >>> >>> @@ -63,8 +63,8 @@ void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *board_data) >>> gpmc_smsc911x_resources[0].end = cs_mem_base + 0xff; >>> >>> if (gpio_request(gpmc_cfg->gpio_irq, "smsc911x irq") < 0) { >>> - printk(KERN_ERR "Failed to request GPIO%d for smsc911x IRQ\n", >>> - gpmc_cfg->gpio_irq); >>> + pr_err("%s: Failed to request IRQ GPIO%d\n", __func__, >>> + gpmc_cfg->gpio_irq); >>> goto free1; >>> } >>> >>> @@ -74,8 +74,8 @@ void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *board_data) >>> if (gpio_is_valid(gpmc_cfg->gpio_reset)) { >>> ret = gpio_request(gpmc_cfg->gpio_reset, "smsc911x reset"); >>> if (ret) { >>> - printk(KERN_ERR "Failed to request GPIO%d for smsc911x reset\n", >>> - gpmc_cfg->gpio_reset); >>> + pr_err("%s: Failed to request reset GPIO%d\n", __func__, >>> + gpmc_cfg->gpio_reset); >>> goto free2; >>> } >>> >>> @@ -92,7 +92,7 @@ void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *board_data) >>> gpmc_smsc911x_resources, ARRAY_SIZE(gpmc_smsc911x_resources), >>> &gpmc_smsc911x_config, sizeof(gpmc_smsc911x_config)); >>> if (!pdev) { >>> - printk(KERN_ERR "Unable to register smsc911x device\n"); >>> + pr_err("%s: Unable to register platform device\n", __func__); >>> gpio_free(gpmc_cfg->gpio_reset); >>> goto free2; >>> } >>> @@ -104,5 +104,5 @@ free2: >>> free1: >>> gpmc_cs_free(gpmc_cfg->cs); >>> >>> - printk(KERN_ERR "Could not initialize smsc911x\n"); >>> + pr_err("Could not initialize smsc911x\n"); >>> } >>> diff --git a/arch/arm/plat-omap/include/plat/gpmc-smsc911x.h b/arch/arm/plat-omap/include/plat/gpmc-smsc911x.h >>> index d3f1579..ea6c9c8 100644 >>> --- a/arch/arm/plat-omap/include/plat/gpmc-smsc911x.h >>> +++ b/arch/arm/plat-omap/include/plat/gpmc-smsc911x.h >>> @@ -21,8 +21,7 @@ struct omap_smsc911x_platform_data { >>> u32 flags; >>> }; >>> >>> -#if defined(CONFIG_SMSC911X) || \ >>> - defined(CONFIG_SMSC911X_MODULE) >>> +#if defined(CONFIG_SMSC911X) || defined(CONFIG_SMSC911X_MODULE) >>> >>> extern void gpmc_smsc911x_init(struct omap_smsc911x_platform_data *d); >>> >>> -- >>> 1.7.3.4 >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> > > -- > Regards, > Igor. > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/26/11 17:49, Menon, Nishanth wrote: > On Tue, Apr 26, 2011 at 09:40, Igor Grinberg <grinberg@compulab.co.il> wrote: >> Hi Nishanth, >> >> On 04/26/11 16:45, Menon, Nishanth wrote: >> >>> On Thu, Apr 21, 2011 at 09:50, Igor Grinberg <grinberg@compulab.co.il> wrote: >>>> replace "printk(KERN_ERR" by "pr_err(" >>>> and fix needlessly multi-lined #ifdef >>>> >>>> Signed-off-by: Igor Grinberg <grinberg@compulab.co.il> >>>> --- >>>> arch/arm/mach-omap2/gpmc-smsc911x.c | 14 +++++++------- >>>> arch/arm/plat-omap/include/plat/gpmc-smsc911x.h | 3 +-- >>>> 2 files changed, 8 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/arch/arm/mach-omap2/gpmc-smsc911x.c b/arch/arm/mach-omap2/gpmc-smsc911x.c >>>> index d30293a..b45efff 100644 >>>> --- a/arch/arm/mach-omap2/gpmc-smsc911x.c >>>> +++ b/arch/arm/mach-omap2/gpmc-smsc911x.c >>> minor suggestion: wont using pr_fmt help to reduce the need to add %s, >>> __func__ for pr_err through out the file? >> I don't understand of what "need" are you talking about. >> I don't know of any need to add "%s, __func__" with pr_fmt... >> I've added "%s, __func__" as a meter of choice. >> IMO, it makes it easier to parse the dmesg output. >> If anyone objects it, I can remove them, >> but I think both pr_fmt and __func__ are nice here and >> way better then "printk(KERN_*" with embedded "smsc911x". > Allow me to rephrase. > I like the change of printk->pr_err - thanks for doing it. just > suggesting a minor improvement > > if you add the following line before the #includes > #define pr_fmt(fmt) "%s: " fmt, __func__ > then > - printk(KERN_ERR "Failed to request GPIO%d for smsc911x IRQ\n", > - gpmc_cfg->gpio_irq); > + pr_err("%s: Failed to request IRQ GPIO%d\n", __func__, > + gpmc_cfg->gpio_irq); > becomes, > - printk(KERN_ERR "Failed to request GPIO%d for smsc911x IRQ\n", > - gpmc_cfg->gpio_irq); > + pr_err("Failed to request IRQ GPIO%d\n", gpmc_cfg->gpio_irq); > > Both give you exactly the output you would like to see (which I > personally prefer as well), but the second could be lesser lines of > code change ;) Ahh, now I understand, that can be useful indeed. I'll send a v2... Thanks
diff --git a/arch/arm/mach-omap2/gpmc-smsc911x.c b/arch/arm/mach-omap2/gpmc-smsc911x.c index d30293a..b45efff 100644 --- a/arch/arm/mach-omap2/gpmc-smsc911x.c +++ b/arch/arm/mach-omap2/gpmc-smsc911x.c @@ -55,7 +55,7 @@ void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *board_data) gpmc_cfg = board_data; if (gpmc_cs_request(gpmc_cfg->cs, SZ_16M, &cs_mem_base) < 0) { - printk(KERN_ERR "Failed to request GPMC mem for smsc911x\n"); + pr_err("%s: Failed to request GPMC mem region\n", __func__); return; } @@ -63,8 +63,8 @@ void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *board_data) gpmc_smsc911x_resources[0].end = cs_mem_base + 0xff; if (gpio_request(gpmc_cfg->gpio_irq, "smsc911x irq") < 0) { - printk(KERN_ERR "Failed to request GPIO%d for smsc911x IRQ\n", - gpmc_cfg->gpio_irq); + pr_err("%s: Failed to request IRQ GPIO%d\n", __func__, + gpmc_cfg->gpio_irq); goto free1; } @@ -74,8 +74,8 @@ void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *board_data) if (gpio_is_valid(gpmc_cfg->gpio_reset)) { ret = gpio_request(gpmc_cfg->gpio_reset, "smsc911x reset"); if (ret) { - printk(KERN_ERR "Failed to request GPIO%d for smsc911x reset\n", - gpmc_cfg->gpio_reset); + pr_err("%s: Failed to request reset GPIO%d\n", __func__, + gpmc_cfg->gpio_reset); goto free2; } @@ -92,7 +92,7 @@ void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *board_data) gpmc_smsc911x_resources, ARRAY_SIZE(gpmc_smsc911x_resources), &gpmc_smsc911x_config, sizeof(gpmc_smsc911x_config)); if (!pdev) { - printk(KERN_ERR "Unable to register smsc911x device\n"); + pr_err("%s: Unable to register platform device\n", __func__); gpio_free(gpmc_cfg->gpio_reset); goto free2; } @@ -104,5 +104,5 @@ free2: free1: gpmc_cs_free(gpmc_cfg->cs); - printk(KERN_ERR "Could not initialize smsc911x\n"); + pr_err("Could not initialize smsc911x\n"); } diff --git a/arch/arm/plat-omap/include/plat/gpmc-smsc911x.h b/arch/arm/plat-omap/include/plat/gpmc-smsc911x.h index d3f1579..ea6c9c8 100644 --- a/arch/arm/plat-omap/include/plat/gpmc-smsc911x.h +++ b/arch/arm/plat-omap/include/plat/gpmc-smsc911x.h @@ -21,8 +21,7 @@ struct omap_smsc911x_platform_data { u32 flags; }; -#if defined(CONFIG_SMSC911X) || \ - defined(CONFIG_SMSC911X_MODULE) +#if defined(CONFIG_SMSC911X) || defined(CONFIG_SMSC911X_MODULE) extern void gpmc_smsc911x_init(struct omap_smsc911x_platform_data *d);
replace "printk(KERN_ERR" by "pr_err(" and fix needlessly multi-lined #ifdef Signed-off-by: Igor Grinberg <grinberg@compulab.co.il> --- arch/arm/mach-omap2/gpmc-smsc911x.c | 14 +++++++------- arch/arm/plat-omap/include/plat/gpmc-smsc911x.h | 3 +-- 2 files changed, 8 insertions(+), 9 deletions(-)