Re: [PATCH 2/3] staging: panel: return register value

[Willy Tarreau]

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/