Re: [PATCH NAND 0/5] Replace printk statements with pr_*macros

Re: [PATCH NAND 0/5] Replace printk statements with pr_*macros

[Ezequiel Garcia]

Hi Shreeya,

Thanks for the contribution.

On 16 February 2018 at 13:50, Shreeya Patel
<shreeya.patel23498@gmail.com> wrote:
> This patchset removes all the log levels i.e. KERN_WARN,
> KERN_NOTICE, KERN_ERR, KERN_INFO, KERN_DEBUG used in the printk
> statements and replaces the printk statements with appropriate
> pr_*macros.
> According to the kernel coding style, pr_*macro is the preferred
> way to print the message.
>

So, two things to begin with.

First of all, despite this contribution being part of outreachy,
I believe you can include mailing lists in your case.

In other words, don't use the "nol" option in get_maintainer
script and Cc the MTD mailing list: linux-mtd at lists.infradead.org.

>
> Shreeya Patel (5):
>   mtd/nand: Replace printk with pr_warn
>   mtd/nand: Replace printk with pr_notice
>   mtd/nand: Replace printk with pr_err
>   mtd/nand: Replace printk with pr_info
>   mtd/nand: Replace printk with pr_debug
>

This is a non-usual split. We usually split patches per-driver,
instead of grouping all the pr_{} of a given type together.

Can you re-do the series? Grouping the changes per-driver,
i.e. all the pr_{} changes in cafe_nand.c together, and so on.

>  drivers/mtd/nand/ams-delta.c     |  4 +--
>  drivers/mtd/nand/cafe_nand.c     |  4 +--
>  drivers/mtd/nand/cs553x_nand.c   |  8 ++---
>  drivers/mtd/nand/diskonchip.c    | 76 +++++++++++++++++++++-------------------
>  drivers/mtd/nand/fsl_elbc_nand.c |  4 +--
>  drivers/mtd/nand/fsl_ifc_nand.c  |  2 +-
>  drivers/mtd/nand/mxc_nand.c      |  2 +-
>  drivers/mtd/nand/nand_bch.c      | 12 +++----
>  drivers/mtd/nand/nandsim.c       | 10 +++---
>  drivers/mtd/nand/r852.c          |  2 +-
>  drivers/mtd/nand/r852.h          |  6 ++--
>  drivers/mtd/nand/sh_flctl.c      |  2 +-
>  drivers/mtd/nand/sm_common.c     |  5 ++-
>  13 files changed, 70 insertions(+), 67 deletions(-)
>
> --
> 2.7.4
>



-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [Outreachy kernel] Re: [PATCH NAND 0/5] Replace printk statements with pr_*macros

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 2717 bytes --]



On Fri, 16 Feb 2018, Ezequiel Garcia wrote:

> Hi Shreeya,
>
> Thanks for the contribution.
>
> On 16 February 2018 at 13:50, Shreeya Patel
> <shreeya.patel23498@gmail.com> wrote:
> > This patchset removes all the log levels i.e. KERN_WARN,
> > KERN_NOTICE, KERN_ERR, KERN_INFO, KERN_DEBUG used in the printk
> > statements and replaces the printk statements with appropriate
> > pr_*macros.
> > According to the kernel coding style, pr_*macro is the preferred
> > way to print the message.
> >
>
> So, two things to begin with.
>
> First of all, despite this contribution being part of outreachy,
> I believe you can include mailing lists in your case.
>
> In other words, don't use the "nol" option in get_maintainer
> script and Cc the MTD mailing list: linux-mtd at lists.infradead.org.

Shouldn't the dev_* functions also be usable?

julia

>
> >
> > Shreeya Patel (5):
> >   mtd/nand: Replace printk with pr_warn
> >   mtd/nand: Replace printk with pr_notice
> >   mtd/nand: Replace printk with pr_err
> >   mtd/nand: Replace printk with pr_info
> >   mtd/nand: Replace printk with pr_debug
> >
>
> This is a non-usual split. We usually split patches per-driver,
> instead of grouping all the pr_{} of a given type together.
>
> Can you re-do the series? Grouping the changes per-driver,
> i.e. all the pr_{} changes in cafe_nand.c together, and so on.
>
> >  drivers/mtd/nand/ams-delta.c     |  4 +--
> >  drivers/mtd/nand/cafe_nand.c     |  4 +--
> >  drivers/mtd/nand/cs553x_nand.c   |  8 ++---
> >  drivers/mtd/nand/diskonchip.c    | 76 +++++++++++++++++++++-------------------
> >  drivers/mtd/nand/fsl_elbc_nand.c |  4 +--
> >  drivers/mtd/nand/fsl_ifc_nand.c  |  2 +-
> >  drivers/mtd/nand/mxc_nand.c      |  2 +-
> >  drivers/mtd/nand/nand_bch.c      | 12 +++----
> >  drivers/mtd/nand/nandsim.c       | 10 +++---
> >  drivers/mtd/nand/r852.c          |  2 +-
> >  drivers/mtd/nand/r852.h          |  6 ++--
> >  drivers/mtd/nand/sh_flctl.c      |  2 +-
> >  drivers/mtd/nand/sm_common.c     |  5 ++-
> >  13 files changed, 70 insertions(+), 67 deletions(-)
> >
> > --
> > 2.7.4
> >
>
>
>
> --
> Ezequiel García, VanguardiaSur
> www.vanguardiasur.com.ar
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/CAAEAJfAg_hei2PF37_oX2tf%2BMv43ye-nEOt%2BKRVFerp1Jfzn2Q%40mail.gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] Re: [PATCH NAND 0/5] Replace printk statements with pr_*macros

[Ezequiel Garcia]

On 16 February 2018 at 14:23, Julia Lawall <julia.lawall@lip6.fr> wrote:
>
>
> On Fri, 16 Feb 2018, Ezequiel Garcia wrote:
>
>> Hi Shreeya,
>>
>> Thanks for the contribution.
>>
>> On 16 February 2018 at 13:50, Shreeya Patel
>> <shreeya.patel23498@gmail.com> wrote:
>> > This patchset removes all the log levels i.e. KERN_WARN,
>> > KERN_NOTICE, KERN_ERR, KERN_INFO, KERN_DEBUG used in the printk
>> > statements and replaces the printk statements with appropriate
>> > pr_*macros.
>> > According to the kernel coding style, pr_*macro is the preferred
>> > way to print the message.
>> >
>>
>> So, two things to begin with.
>>
>> First of all, despite this contribution being part of outreachy,
>> I believe you can include mailing lists in your case.
>>
>> In other words, don't use the "nol" option in get_maintainer
>> script and Cc the MTD mailing list: linux-mtd at lists.infradead.org.
>
> Shouldn't the dev_* functions also be usable?
>

Provided that:
1. it's applicable, i.e. if in the context of a device.
2. Richard is not too opposed to the idea.

Then, yes.
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [Outreachy kernel] Re: [PATCH NAND 0/5] Replace printk statements with pr_*macros

[Boris Brezillon]

On Fri, 16 Feb 2018 14:26:56 -0300
Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:

> On 16 February 2018 at 14:23, Julia Lawall <julia.lawall@lip6.fr> wrote:
> >
> >
> > On Fri, 16 Feb 2018, Ezequiel Garcia wrote:
> >  
> >> Hi Shreeya,
> >>
> >> Thanks for the contribution.
> >>
> >> On 16 February 2018 at 13:50, Shreeya Patel
> >> <shreeya.patel23498@gmail.com> wrote:  
> >> > This patchset removes all the log levels i.e. KERN_WARN,
> >> > KERN_NOTICE, KERN_ERR, KERN_INFO, KERN_DEBUG used in the printk
> >> > statements and replaces the printk statements with appropriate
> >> > pr_*macros.
> >> > According to the kernel coding style, pr_*macro is the preferred
> >> > way to print the message.
> >> >  
> >>
> >> So, two things to begin with.
> >>
> >> First of all, despite this contribution being part of outreachy,
> >> I believe you can include mailing lists in your case.
> >>
> >> In other words, don't use the "nol" option in get_maintainer
> >> script and Cc the MTD mailing list: linux-mtd at lists.infradead.org.  
> >
> > Shouldn't the dev_* functions also be usable?
> >  
> 
> Provided that:
> 1. it's applicable, i.e. if in the context of a device.

Yep, be careful with that. The MTD/NAND subsystem initializes
mtd->dev.name quite late, so it's not safe to use &mtd->dev with
dev_<loglevel>(). Note that you can use the NAND controller pdev->dev if
available.

> 2. Richard is not too opposed to the idea.
> 
> Then, yes.



-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [Outreachy kernel] Re: [PATCH NAND 0/5] Replace printk statements with pr_*macros

[Shreeya Patel]

On Fri, 2018-02-16 at 18:48 +0100, Boris Brezillon wrote:
> On Fri, 16 Feb 2018 14:26:56 -0300
> Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
> 
> > 
> > On 16 February 2018 at 14:23, Julia Lawall <julia.lawall@lip6.fr>
> > wrote:
> > > 
> > > 
> > > 
> > > On Fri, 16 Feb 2018, Ezequiel Garcia wrote:
> > >  
> > > > 
> > > > Hi Shreeya,
> > > > 
> > > > Thanks for the contribution.
> > > > 
> > > > On 16 February 2018 at 13:50, Shreeya Patel
> > > > <shreeya.patel23498@gmail.com> wrote:  
> > > > > 
> > > > > This patchset removes all the log levels i.e. KERN_WARN,
> > > > > KERN_NOTICE, KERN_ERR, KERN_INFO, KERN_DEBUG used in the
> > > > > printk
> > > > > statements and replaces the printk statements with
> > > > > appropriate
> > > > > pr_*macros.
> > > > > According to the kernel coding style, pr_*macro is the
> > > > > preferred
> > > > > way to print the message.
> > > > >  
> > > > So, two things to begin with.
> > > > 
> > > > First of all, despite this contribution being part of
> > > > outreachy,
> > > > I believe you can include mailing lists in your case.
> > > > 
> > > > In other words, don't use the "nol" option in get_maintainer
> > > > script and Cc the MTD mailing list: linux-mtd at
> > > > lists.infradead.org.  
> > > Shouldn't the dev_* functions also be usable?
> > >  
> > Provided that:
> > 1. it's applicable, i.e. if in the context of a device.
> Yep, be careful with that. The MTD/NAND subsystem initializes
> mtd->dev.name quite late, so it's not safe to use &mtd->dev with
> dev_<loglevel>(). Note that you can use the NAND controller pdev->dev 
> if
> available.

This information is quite useful, thanks.
I'll see that what will be the best thing to use here.

> 
> > 
> > 2. Richard is not too opposed to the idea.
> > 
> > Then, yes.
> 
> 

Re: [Outreachy kernel] Re: [PATCH NAND 0/5] Replace printk statements with pr_*macros

[Boris Brezillon]

Hi Shreeya,

Please try to keep everyone in the loop when you reply to an email. All
discussions should happen publicly to keep everyone aware of the
progress and let other developers/maintainers take part to the
discussion if they have something to add.


On Sun, 18 Feb 2018 23:56:05 +0530
Shreeya Patel <shreeya.patel23498@gmail.com> wrote:

> On Sun, 2018-02-18 at 19:13 +0100, Boris Brezillon wrote:
> > On Sun, 18 Feb 2018 23:31:10 +0530
> > Shreeya Patel <shreeya.patel23498@gmail.com> wrote:
> >   
> > > 
> > > On Fri, 2018-02-16 at 18:48 +0100, Boris Brezillon wrote:
> > > 
> > > Hello Sir,
> > >   
> > > > 
> > > > On Fri, 16 Feb 2018 14:26:56 -0300
> > > > Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
> > > >     
> > > > > 
> > > > > 
> > > > > On 16 February 2018 at 14:23, Julia Lawall <julia.lawall@lip6.f  
> > > > > r>  
> > > > > wrote:    
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On Fri, 16 Feb 2018, Ezequiel Garcia wrote:
> > > > > >      
> > > > > > > 
> > > > > > > 
> > > > > > > Hi Shreeya,
> > > > > > > 
> > > > > > > Thanks for the contribution.
> > > > > > > 
> > > > > > > On 16 February 2018 at 13:50, Shreeya Patel
> > > > > > > <shreeya.patel23498@gmail.com> wrote:      
> > > > > > > > 
> > > > > > > > 
> > > > > > > > This patchset removes all the log levels i.e. KERN_WARN,
> > > > > > > > KERN_NOTICE, KERN_ERR, KERN_INFO, KERN_DEBUG used in the
> > > > > > > > printk
> > > > > > > > statements and replaces the printk statements with
> > > > > > > > appropriate
> > > > > > > > pr_*macros.
> > > > > > > > According to the kernel coding style, pr_*macro is the
> > > > > > > > preferred
> > > > > > > > way to print the message.
> > > > > > > >      
> > > > > > > So, two things to begin with.
> > > > > > > 
> > > > > > > First of all, despite this contribution being part of
> > > > > > > outreachy,
> > > > > > > I believe you can include mailing lists in your case.
> > > > > > > 
> > > > > > > In other words, don't use the "nol" option in
> > > > > > > get_maintainer
> > > > > > > script and Cc the MTD mailing list: linux-mtd at
> > > > > > > lists.infradead.org.      
> > > > > > Shouldn't the dev_* functions also be usable?
> > > > > >      
> > > > > Provided that:
> > > > > 1. it's applicable, i.e. if in the context of a device.    
> > > > Yep, be careful with that. The MTD/NAND subsystem initializes
> > > > mtd->dev.name quite late, so it's not safe to use &mtd->dev with
> > > > dev_<loglevel>(). Note that you can use the NAND controller pdev-  
> > > > >dev   
> > > > if
> > > > available.    
> > > I would like to ask here that what will be the benefit or good
> > > cause if
> > > I use pdev->dev?
> > > How is it different from others?  
> > Well, pdev->dev is guaranteed to be properly initialized when you
> > pass
> > it to dev_<loglevel>(). This is not the case with mtd->dev which is
> > initialized in mtd_device_register(), but the NAND controller driver
> > usually does some operations on the NAND device before reaching this
> > point (reading the ID, reading the bad block markers to determine
> > which
> > blocks are bad, ...).
> >   
> Thanks for making me understand.
> 
> > Anyway, I think it's better if you first do the
> > s/printk(KERN_<LOGLEVEL>(/pr_<loglevel>(/ replacement. Replacing
> > pr_xxx
> > by dev_xxx is something we can do afterwards.  
> 
> I've created one patch where I have replaced printk
> with dev_* and used pdev->dev.

Okay. pdev->dev is not always directly accessible so doing
s/printk(KERN_<LOGLEVEL>/dev_<loglevel>(&pdev->dev, / is likely to
cause build failures. But I guess you compile-tested the changes you're
about to submit.

Regards,

Boris


-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [Outreachy kernel] Re: [PATCH NAND 0/5] Replace printk statements with pr_*macros

[Shreeya Patel]

On Sun, 2018-02-18 at 21:09 +0100, Boris Brezillon wrote:
> Hi Shreeya,
> 
> Please try to keep everyone in the loop when you reply to an email.
> All
> discussions should happen publicly to keep everyone aware of the
> progress and let other developers/maintainers take part to the
> discussion if they have something to add.

I'll keep this in mind.
> 
> 
> On Sun, 18 Feb 2018 23:56:05 +0530
> Shreeya Patel <shreeya.patel23498@gmail.com> wrote:
> 
> > 
> > On Sun, 2018-02-18 at 19:13 +0100, Boris Brezillon wrote:
> > > 
> > > On Sun, 18 Feb 2018 23:31:10 +0530
> > > Shreeya Patel <shreeya.patel23498@gmail.com> wrote:
> > >   
> > > > 
> > > > 
> > > > On Fri, 2018-02-16 at 18:48 +0100, Boris Brezillon wrote:
> > > > 
> > > > Hello Sir,
> > > >   
> > > > > 
> > > > > 
> > > > > On Fri, 16 Feb 2018 14:26:56 -0300
> > > > > Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
> > > > >     
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On 16 February 2018 at 14:23, Julia Lawall <julia.lawall@li
> > > > > > p6.f  
> > > > > > r>  
> > > > > > wrote:    
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > On Fri, 16 Feb 2018, Ezequiel Garcia wrote:
> > > > > > >      
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > Hi Shreeya,
> > > > > > > > 
> > > > > > > > Thanks for the contribution.
> > > > > > > > 
> > > > > > > > On 16 February 2018 at 13:50, Shreeya Patel
> > > > > > > > <shreeya.patel23498@gmail.com> wrote:      
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > This patchset removes all the log levels i.e.
> > > > > > > > > KERN_WARN,
> > > > > > > > > KERN_NOTICE, KERN_ERR, KERN_INFO, KERN_DEBUG used in
> > > > > > > > > the
> > > > > > > > > printk
> > > > > > > > > statements and replaces the printk statements with
> > > > > > > > > appropriate
> > > > > > > > > pr_*macros.
> > > > > > > > > According to the kernel coding style, pr_*macro is
> > > > > > > > > the
> > > > > > > > > preferred
> > > > > > > > > way to print the message.
> > > > > > > > >      
> > > > > > > > So, two things to begin with.
> > > > > > > > 
> > > > > > > > First of all, despite this contribution being part of
> > > > > > > > outreachy,
> > > > > > > > I believe you can include mailing lists in your case.
> > > > > > > > 
> > > > > > > > In other words, don't use the "nol" option in
> > > > > > > > get_maintainer
> > > > > > > > script and Cc the MTD mailing list: linux-mtd at
> > > > > > > > lists.infradead.org.      
> > > > > > > Shouldn't the dev_* functions also be usable?
> > > > > > >      
> > > > > > Provided that:
> > > > > > 1. it's applicable, i.e. if in the context of a device.    
> > > > > Yep, be careful with that. The MTD/NAND subsystem initializes
> > > > > mtd->dev.name quite late, so it's not safe to use &mtd->dev
> > > > > with
> > > > > dev_<loglevel>(). Note that you can use the NAND controller
> > > > > pdev-  
> > > > > > 
> > > > > > dev   
> > > > > if
> > > > > available.    
> > > > I would like to ask here that what will be the benefit or good
> > > > cause if
> > > > I use pdev->dev?
> > > > How is it different from others?  
> > > Well, pdev->dev is guaranteed to be properly initialized when you
> > > pass
> > > it to dev_<loglevel>(). This is not the case with mtd->dev which
> > > is
> > > initialized in mtd_device_register(), but the NAND controller
> > > driver
> > > usually does some operations on the NAND device before reaching
> > > this
> > > point (reading the ID, reading the bad block markers to determine
> > > which
> > > blocks are bad, ...).
> > >   
> > Thanks for making me understand.
> > 
> > > 
> > > Anyway, I think it's better if you first do the
> > > s/printk(KERN_<LOGLEVEL>(/pr_<loglevel>(/ replacement. Replacing
> > > pr_xxx
> > > by dev_xxx is something we can do afterwards.  
> > I've created one patch where I have replaced printk
> > with dev_* and used pdev->dev.
> Okay. pdev->dev is not always directly accessible so doing
> s/printk(KERN_<LOGLEVEL>/dev_<loglevel>(&pdev->dev, / is likely to
> cause build failures. But I guess you compile-tested the changes
> you're
> about to submit.

Yes, it wasn't directly accessible at some places.
Also, I was unable to compile ams-delta.c file.
I am working on it right now as I will have to cross compile for arm.

Thanks
> 
> Regards,
> 
> Boris
> 
> 

Re: [Outreachy kernel] Re: [PATCH NAND 0/5] Replace printk statements with pr_*macros

[Shreeya Patel]

On Sun, 2018-02-18 at 21:09 +0100, Boris Brezillon wrote:
> Hi Shreeya,
> 
> Please try to keep everyone in the loop when you reply to an email.
> All
> discussions should happen publicly to keep everyone aware of the
> progress and let other developers/maintainers take part to the
> discussion if they have something to add.

I'll keep this in mind.

> 
> 
> On Sun, 18 Feb 2018 23:56:05 +0530
> Shreeya Patel <shreeya.patel23498@gmail.com> wrote:
> 
> > 
> > On Sun, 2018-02-18 at 19:13 +0100, Boris Brezillon wrote:
> > > 
> > > On Sun, 18 Feb 2018 23:31:10 +0530
> > > Shreeya Patel <shreeya.patel23498@gmail.com> wrote:
> > >   
> > > > 
> > > > 
> > > > On Fri, 2018-02-16 at 18:48 +0100, Boris Brezillon wrote:
> > > > 
> > > > Hello Sir,
> > > >   
> > > > > 
> > > > > 
> > > > > On Fri, 16 Feb 2018 14:26:56 -0300
> > > > > Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
> > > > >     
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On 16 February 2018 at 14:23, Julia Lawall <julia.lawall@li
> > > > > > p6.f  
> > > > > > r>  
> > > > > > wrote:    
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > On Fri, 16 Feb 2018, Ezequiel Garcia wrote:
> > > > > > >      
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > Hi Shreeya,
> > > > > > > > 
> > > > > > > > Thanks for the contribution.
> > > > > > > > 
> > > > > > > > On 16 February 2018 at 13:50, Shreeya Patel
> > > > > > > > <shreeya.patel23498@gmail.com> wrote:      
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > This patchset removes all the log levels i.e.
> > > > > > > > > KERN_WARN,
> > > > > > > > > KERN_NOTICE, KERN_ERR, KERN_INFO, KERN_DEBUG used in
> > > > > > > > > the
> > > > > > > > > printk
> > > > > > > > > statements and replaces the printk statements with
> > > > > > > > > appropriate
> > > > > > > > > pr_*macros.
> > > > > > > > > According to the kernel coding style, pr_*macro is
> > > > > > > > > the
> > > > > > > > > preferred
> > > > > > > > > way to print the message.
> > > > > > > > >      
> > > > > > > > So, two things to begin with.
> > > > > > > > 
> > > > > > > > First of all, despite this contribution being part of
> > > > > > > > outreachy,
> > > > > > > > I believe you can include mailing lists in your case.
> > > > > > > > 
> > > > > > > > In other words, don't use the "nol" option in
> > > > > > > > get_maintainer
> > > > > > > > script and Cc the MTD mailing list: linux-mtd at
> > > > > > > > lists.infradead.org.      
> > > > > > > Shouldn't the dev_* functions also be usable?
> > > > > > >      
> > > > > > Provided that:
> > > > > > 1. it's applicable, i.e. if in the context of a device.    
> > > > > Yep, be careful with that. The MTD/NAND subsystem initializes
> > > > > mtd->dev.name quite late, so it's not safe to use &mtd->dev
> > > > > with
> > > > > dev_<loglevel>(). Note that you can use the NAND controller
> > > > > pdev-  
> > > > > > 
> > > > > > dev   
> > > > > if
> > > > > available.    
> > > > I would like to ask here that what will be the benefit or good
> > > > cause if
> > > > I use pdev->dev?
> > > > How is it different from others?  
> > > Well, pdev->dev is guaranteed to be properly initialized when you
> > > pass
> > > it to dev_<loglevel>(). This is not the case with mtd->dev which
> > > is
> > > initialized in mtd_device_register(), but the NAND controller
> > > driver
> > > usually does some operations on the NAND device before reaching
> > > this
> > > point (reading the ID, reading the bad block markers to determine
> > > which
> > > blocks are bad, ...).
> > >   
> > Thanks for making me understand.
> > 
> > > 
> > > Anyway, I think it's better if you first do the
> > > s/printk(KERN_<LOGLEVEL>(/pr_<loglevel>(/ replacement. Replacing
> > > pr_xxx
> > > by dev_xxx is something we can do afterwards.  
> > I've created one patch where I have replaced printk
> > with dev_* and used pdev->dev.
> Okay. pdev->dev is not always directly accessible so doing
> s/printk(KERN_<LOGLEVEL>/dev_<loglevel>(&pdev->dev, / is likely to
> cause build failures. But I guess you compile-tested the changes
> you're
> about to submit.

Yes, it wasn't directly accessible at some places.
Also, I was unable to compile ams-delta.c file.
I am working on it right now as I will have to cross compile for arm.

Thanks


> 
> Regards,
> 
> Boris
> 
> 

Re: [PATCH NAND 0/5] Replace printk statements with pr_*macros

[Shreeya Patel]

On Fri, 2018-02-16 at 14:19 -0300, Ezequiel Garcia wrote:
> Hi Shreeya,
> 
> Thanks for the contribution.
> 
> On 16 February 2018 at 13:50, Shreeya Patel
> <shreeya.patel23498@gmail.com> wrote:
> > 
> > This patchset removes all the log levels i.e. KERN_WARN,
> > KERN_NOTICE, KERN_ERR, KERN_INFO, KERN_DEBUG used in the printk
> > statements and replaces the printk statements with appropriate
> > pr_*macros.
> > According to the kernel coding style, pr_*macro is the preferred
> > way to print the message.
> > 
> So, two things to begin with.
> 
> First of all, despite this contribution being part of outreachy,
> I believe you can include mailing lists in your case.
> 
> In other words, don't use the "nol" option in get_maintainer
> script and Cc the MTD mailing list: linux-mtd at lists.infradead.org.

Ok. I'll take care of this.
> 
> > 
> > 
> > Shreeya Patel (5):
> >   mtd/nand: Replace printk with pr_warn
> >   mtd/nand: Replace printk with pr_notice
> >   mtd/nand: Replace printk with pr_err
> >   mtd/nand: Replace printk with pr_info
> >   mtd/nand: Replace printk with pr_debug
> > 
> This is a non-usual split. We usually split patches per-driver,
> instead of grouping all the pr_{} of a given type together.
> 
> Can you re-do the series? Grouping the changes per-driver,
> i.e. all the pr_{} changes in cafe_nand.c together, and so on.

Yes, I'll do it again. :)

Also, I wanted to ask you that there are still printk statements 
present in the driver which are just printing the message i.e. they
don't include any log levels.
So should I leave them as it is?

Thanks for your reviews 
> 
> > 
> >  drivers/mtd/nand/ams-delta.c     |  4 +--
> >  drivers/mtd/nand/cafe_nand.c     |  4 +--
> >  drivers/mtd/nand/cs553x_nand.c   |  8 ++---
> >  drivers/mtd/nand/diskonchip.c    | 76 +++++++++++++++++++++-------
> > ------------
> >  drivers/mtd/nand/fsl_elbc_nand.c |  4 +--
> >  drivers/mtd/nand/fsl_ifc_nand.c  |  2 +-
> >  drivers/mtd/nand/mxc_nand.c      |  2 +-
> >  drivers/mtd/nand/nand_bch.c      | 12 +++----
> >  drivers/mtd/nand/nandsim.c       | 10 +++---
> >  drivers/mtd/nand/r852.c          |  2 +-
> >  drivers/mtd/nand/r852.h          |  6 ++--
> >  drivers/mtd/nand/sh_flctl.c      |  2 +-
> >  drivers/mtd/nand/sm_common.c     |  5 ++-
> >  13 files changed, 70 insertions(+), 67 deletions(-)
> > 
> > --
> > 2.7.4
> > 
> 
> 

Re: [PATCH NAND 0/5] Replace printk statements with pr_*macros

[Boris Brezillon]

On Fri, 16 Feb 2018 14:19:41 -0300
Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:

> Hi Shreeya,
> 
> Thanks for the contribution.
> 
> On 16 February 2018 at 13:50, Shreeya Patel
> <shreeya.patel23498@gmail.com> wrote:
> > This patchset removes all the log levels i.e. KERN_WARN,
> > KERN_NOTICE, KERN_ERR, KERN_INFO, KERN_DEBUG used in the printk
> > statements and replaces the printk statements with appropriate
> > pr_*macros.
> > According to the kernel coding style, pr_*macro is the preferred
> > way to print the message.
> >  
> 
> So, two things to begin with.
> 
> First of all, despite this contribution being part of outreachy,
> I believe you can include mailing lists in your case.
> 
> In other words, don't use the "nol" option in get_maintainer
> script and Cc the MTD mailing list: linux-mtd at lists.infradead.org.
> 
> >
> > Shreeya Patel (5):
> >   mtd/nand: Replace printk with pr_warn
> >   mtd/nand: Replace printk with pr_notice
> >   mtd/nand: Replace printk with pr_err
> >   mtd/nand: Replace printk with pr_info
> >   mtd/nand: Replace printk with pr_debug

Nitpick: prefix should be "mtd: nand: ".

> >  
> 
> This is a non-usual split. We usually split patches per-driver,
> instead of grouping all the pr_{} of a given type together.

I agree.

> 
> Can you re-do the series? Grouping the changes per-driver,
> i.e. all the pr_{} changes in cafe_nand.c together, and so on.

Actually, for such simple/automatic changes you can even group things
in a single patch. I'm fine either way.

> 
> >  drivers/mtd/nand/ams-delta.c     |  4 +--
> >  drivers/mtd/nand/cafe_nand.c     |  4 +--
> >  drivers/mtd/nand/cs553x_nand.c   |  8 ++---
> >  drivers/mtd/nand/diskonchip.c    | 76 +++++++++++++++++++++-------------------
> >  drivers/mtd/nand/fsl_elbc_nand.c |  4 +--
> >  drivers/mtd/nand/fsl_ifc_nand.c  |  2 +-
> >  drivers/mtd/nand/mxc_nand.c      |  2 +-
> >  drivers/mtd/nand/nand_bch.c      | 12 +++----
> >  drivers/mtd/nand/nandsim.c       | 10 +++---
> >  drivers/mtd/nand/r852.c          |  2 +-
> >  drivers/mtd/nand/r852.h          |  6 ++--
> >  drivers/mtd/nand/sh_flctl.c      |  2 +-
> >  drivers/mtd/nand/sm_common.c     |  5 ++-
> >  13 files changed, 70 insertions(+), 67 deletions(-)
> >
> > --
> > 2.7.4
> >  
> 
> 
> 



-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH NAND 0/5] Replace printk statements with pr_*macros

[Ezequiel Garcia]

(Ccing MTD)

On 16 February 2018 at 14:24, Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
> Hey Richard,
>
> On 16 February 2018 at 14:19, Richard Weinberger <richard@nod.at> wrote:
>> Am Freitag, 16. Februar 2018, 17:50:09 CET schrieb Shreeya Patel:
>>> This patchset removes all the log levels i.e. KERN_WARN,
>>> KERN_NOTICE, KERN_ERR, KERN_INFO, KERN_DEBUG used in the printk
>>> statements and replaces the printk statements with appropriate
>>> pr_*macros.
>>> According to the kernel coding style, pr_*macro is the preferred
>>> way to print the message.
>>
>> Beside of that, how does it improve the code?
>> Don't get me wrong, pr_* is the way to go for new code, but I don't think it
>> is worth "fixing" in existing code and make working with git blame more
>> painful.
>>
>
> Shreeya's intention is to work on [1], migrating drivers to exec_op.
> As a first task, before being accepted, she is required to submit
> cleanup patches (or small tasks).
>
> Since I couldn't come up with small tasks for MTD, I suggested
> starting with printk cleaning. So this was my idea.
>
> Printk to pr_{} or dev_{} is not only aesthetical, it's also a consolidation
> change. Centralizing prints allows for other improvements.
>
> See for instance Wolfram's work [2, 3].
>
> In any case, does it hurt git-blame so much? It only affects
> the lines where the print is performed.
>
> BTW, should you have ideas for small cleanup tasks in MTD,
> let us know.
>
> [1] https://www.outreachy.org/communities/cfp/linux-kernel/project/migrate-nand-driver-to-new-exec_op-framework/
> [2] https://elinux.org/Refactor_kernel_strings
> [3] https://elinux.org/Refactor_kernel_strings
>
> Thanks,
> --
> Ezequiel García, VanguardiaSur
> www.vanguardiasur.com.ar



-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar