LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Willy Tarreau <w@1wt.eu>
To: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/3] staging: panel: return register value
Date: Mon, 9 Mar 2015 16:05:00 +0100	[thread overview]
Message-ID: <20150309150500.GB6206@1wt.eu> (raw)
In-Reply-To: <20150309133559.GB13002@sudip-PC>

On Mon, Mar 09, 2015 at 07:05:59PM +0530, Sudip Mukherjee wrote:
> On Mon, Mar 09, 2015 at 07:30:07AM +0100, Willy Tarreau wrote:
> > On Sun, Mar 08, 2015 at 11:07:25PM +0530, Sudip Mukherjee wrote:
> > >  
> > > -	if (parport_register_driver(&panel_driver)) {
> > > -		pr_err("could not register with parport. Aborting.\n");
> > > -		return -EIO;
> > > -	}
> > > -
> > >  	if (pprt)
> > >  		pr_info("driver version " PANEL_VERSION
> > >  			" registered on parport%d (io=0x%lx).\n", parport,
> > > @@ -2375,7 +2370,7 @@ static int __init panel_init_module(void)
> > >  	/* tells various subsystems about the fact that initialization
> > >  	   is finished */
> > >  	init_in_progress = 0;
> > > -	return 0;
> > > +	return parport_register_driver(&panel_driver);
> > 
> > Here I'd rather keep the error message, as it's quite annoying when a
> > driver does not load and you don't know why, especially with something
> > like parport where there are many possible causes. Something like this
> > would be better in my opinion :
> > 
> > -	return 0;
> > +	err = parport_register_driver(&panel_driver);
> > +	if (err)
> > +		pr_err("could not register with parport. Aborting.\n");
> > +	return err;
> > 
> > Well, I just found that currently parport_register_driver() always
> > succeeds, but I still think it's better to verbosely handle errors.
> 
> ok, i will send a v2, but is the error message really required? considering
> the fact that parport_register_driver() always succeeds ..

It's a matter of choice :
   - either we consider that it can fail and we emit an appropriate
     error message ;

   - or we consider it cannot fail and instead of returning its own
     code we always call it without checking it then return zero. That
     way if it were ever to change, it would be easy to spot the change.

I still find that the first choice is appropriate, comes with a very low
cost and is in line with what the parrallel port driver does as well.

Regards,
Willy


  reply	other threads:[~2015-03-09 15:05 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-08 17:37 [PATCH 1/3] staging: panel: register reboot Sudip Mukherjee
2015-03-08 17:37 ` [PATCH 2/3] staging: panel: return register value Sudip Mukherjee
2015-03-09  6:30   ` Willy Tarreau
2015-03-09 13:35     ` Sudip Mukherjee
2015-03-09 15:05       ` Willy Tarreau [this message]
2015-03-08 17:37 ` [PATCH 3/3] staging: panel: remove initialization check Sudip Mukherjee
2015-03-09  6:20   ` Willy Tarreau
2015-03-09  6:19 ` [PATCH 1/3] staging: panel: register reboot Willy Tarreau

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20150309150500.GB6206@1wt.eu \
    --to=w@1wt.eu \
    --cc=devel@driverdev.osuosl.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=sudipm.mukherjee@gmail.com \
    --subject='Re: [PATCH 2/3] staging: panel: return register value' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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).