Re: [PATCH v2] Staging: comedi: fix printk issue in adv_pci_dio.c

[Jesper Juhl]

On Thu, 28 Jul 2011, ravi shankar wrote:

> Hi,
> 
>       Jesper as you told it would be better to make one
> printk(KERN_ERR...) for all levels instead of two. What is your
> opinion?
> 

That's not what I said. Read my mail again.

I said that in my opinion the failure message(s) and the success message 
should probably be two different printk()'s.

But regardless of whether you keep it as-is or not, the other comments 
still need to be addressed.


> Thanks & Regards
>  Ravishankar
> 
> On Wed, Jul 27, 2011 at 6:05 PM, Jesper Juhl <jj@xxxxxxxxxxxxx> wrote:
> > On Wed, 27 Jul 2011, Ravishankar wrote:
> >
> >> From: Ravishankar <ravi.shankar@xxxxxxxxxxxxxxx>
> >>
> >> This is a patch to the adv_pci_dio.c file that fixes up a printk warning found by the checkpatch.pl tool
> >>
> >> Signed-off-by: Ravishankar <ravishankarkm32@xxxxxxxxx>
> >> ---
> >>  drivers/staging/comedi/drivers/adv_pci_dio.c |   18 +++++++++---------
> >>  1 files changed, 9 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/staging/comedi/drivers/adv_pci_dio.c b/drivers/staging/comedi/drivers/adv_pci_dio.c
> >> index d23799b..1d2261f 100644
> >> --- a/drivers/staging/comedi/drivers/adv_pci_dio.c
> >> +++ b/drivers/staging/comedi/drivers/adv_pci_dio.c
> >> @@ -1106,11 +1106,11 @@ static int pci_dio_attach(struct comedi_device *dev,
> >>       unsigned long iobase;
> >>       struct pci_dev *pcidev = NULL;
> >>
> >> -     printk("comedi%d: adv_pci_dio: ", dev->minor);
> >> +     printk(KERN_INFO "comedi%d: adv_pci_dio: ", dev->minor);
> >>
> >
> > This printk() is used both for printing out information in the case of
> > success, in which case the KERN_INFO level is fine. But it is also used to
> > print out error messages in case of failure, in which case KERN_WARNING
> > would probably be better. So I'm wondering if it wouldn't be better to
> > restructure the code so that the printing of error messages and success
> > info becomes two seperate printk()'s each with the apropriate level.
> >
> >
> >>       ret = alloc_private(dev, sizeof(struct pci_dio_private));
> >>       if (ret < 0) {
> >> -             printk(", Error: Cann't allocate private memory!\n");
> >> +             printk(KERN_CONT ", Error: Cann't allocate private memory!\n");
> >
> > Might as well fix the spelling error ( s/Cann't/Can't/ ) while you are
> > changing the line anyway.
> >
> >
> >>               return -ENOMEM;
> >>       }
> >>
> >> @@ -1140,19 +1140,19 @@ static int pci_dio_attach(struct comedi_device *dev,
> >>       }
> >>
> >>       if (!dev->board_ptr) {
> >> -             printk(", Error: Requested type of the card was not found!\n");
> >> +             printk(KERN_WARNING ", Error: Requested type of the card was not "
> >> +             "found!\n");
> >
> > As Joe Perches already pointed out to you, this is a continuation of the
> > first printk() and should be using KERN_CONT.
> >
> >
> >>               return -EIO;
> >>       }
> >>
> >>       if (comedi_pci_enable(pcidev, driver_pci_dio.driver_name)) {
> >>               printk
> >> -                 (", Error: Can't enable PCI device and request regions!\n");
> >> +             (KERN_WARNING ", Error: Can't enable PCI device and request "
> >> +             "regions!\n");
> >
> > This one as well. And it has got to be possible to find a less hideous way
> > to break that line..
> >
> > what about:
> >
> >  printk(KERN_CONT
> >         ", Error: Can't enable PCI device and request regions!\n");
> >
> > ? If it even needs to be broken at all (the 80 column rule is not fixed in
> > stone).
> >
> >
> >>               return -EIO;
> >>       }
> >>       iobase = pci_resource_start(pcidev, this_board->main_pci_region);
> >> -     printk(", b:s:f=%d:%d:%d, io=0x%4lx",
> >> -            pcidev->bus->number, PCI_SLOT(pcidev->devfn),
> >> -            PCI_FUNC(pcidev->devfn), iobase);
> >> +     printk(KERN_CONT ", b:s:f=%d:%d:%d, io=0x%4lx",
> >> +            pcidev->bus->number, PCI_SLOT(pcidev->devfn),
> >> +            PCI_FUNC(pcidev->devfn), iobase);
> >>
> >>       dev->iobase = iobase;
> >>       dev->board_name = this_board->name;
> >> @@ -1178,11 +1178,11 @@ static int pci_dio_attach(struct comedi_device *dev,
> >>
> >>       ret = alloc_subdevices(dev, n_subdevices);
> >>       if (ret < 0) {
> >> -             printk(", Error: Cann't allocate subdevice memory!\n");
> >> +             printk(KERN_CONT ", Error: Cann't allocate subdevice memory!\n");
> >>               return ret;
> >>       }
> >>
> >> -     printk(".\n");
> >> +     printk(KERN_CONT ".\n");
> >>
> >>       subdev = 0;
> >>
> >
> > Please take the feedback people give you serious. Joe kindly pointed out
> > your mistakes the last time you posted this and yet your next patch has
> > the exact same mistakes.
> >
> > Read through your patches before you send them and double check each and
> > every change - then check it again.
> >


-- 
Jesper Juhl <jj@xxxxxxxxxxxxx>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.