Re: [PATCH WIP] parport: add device model

[Sudip Mukherjee <sudipm.mukherjee@gmail.com>]

On Fri, Apr 10, 2015 at 05:49:55PM +0300, Dan Carpenter wrote:
> On Fri, Apr 10, 2015 at 08:00:38PM +0530, Sudip Mukherjee wrote:
<snip>
> > +
> >  	parport_default_sysctl_table.sysctl_header =
> >  		register_sysctl_table(parport_default_sysctl_table.dev_dir);
> 
> Should we return an error if this fails?
not sure. but even if it fails it will not affect the normal functioning
of the parallel port. but I will add that in the next WIP patch.
> 
> > -	return 0;
> > +	ret = parport_bus_init();
> > +	if (ret)
> > +		unregister_sysctl_table(parport_default_sysctl_table.
> > +					sysctl_header);
> 
> 
> 	ret = parport_bus_init();
> 	if (ret) {
> 		unregister_sysctl_table(
> 				parport_default_sysctl_table.sysctl_header);
> 		return ret;
> 	}
> 
> 	return 0;
do we need two returns here? parport_bus_init() will return 0 if it succeeds,
so return ret will return either 0 or the error code whatever the case maybe.	
> 
> 
> > +	return ret;
> >  }
> >  
> >  static void __exit parport_default_proc_unregister(void)
> > @@ -570,6 +576,7 @@ static void __exit parport_default_proc_unregister(void)
> >  					sysctl_header);
> >  		parport_default_sysctl_table.sysctl_header = NULL;
> >  	}
> > +	parport_bus_exit();
> 
> Do we need this function?  Can't we call bus_unregister() directly?
no, we dont need. on similar reasoning we also donot need parport_bus_init().
I will remove both. :)
> 
<snip>
> 
> > +struct bus_type parport_bus_type = {
> > +	.name           = "parport",
> > +	.match		= parport_match,
> 
> There is no need for a match function.  If it's NULL that's the same a
> "return 1" fuction.  This is called from driver_match_device().
ok.
> 
<snip>
> > +	ret = driver_register(&drv->driver);
> > +	if (ret < 0) {
> 
> 	if (ret) {
> 
> > +		mutex_lock(&registration_lock);
> > +		list_del_init(&drv->list);
> > +		list_for_each_entry(port, &portlist, list)
> > +			drv->detach(port);
> 
> Does this free port?  Should this be list_for_each_entry_safe?
I am not sure what you meant by "free port". attach will claim the port,
and the port will be marked. detach will just remove that connection and
the driver will release the port.
> 
> > +		mutex_unlock(&registration_lock);
> 
> 		return ret;
> > +	}
> > +
> > +	return ret;
> 
> 	return 0;
do we need two returns? as ret will be either 0 or error code.
> 
> >  }
> >  
<snip>	
> 
> Please use "if (ret) " everywhere unless it returns positive on success.
sure.
> 
> I know that I have done a rubbish review.  I'm going to have to review
> this properly later.
main thing i wanted to know is if my approach is correct. since nothing
on that so I hope I am on the correct track. Thanks.
I will send in the next version in a day or two.

regards
sudip
> 
> regards,
> dan carpenter