Message-ID: <20150519131814.GB7106@sudip-PC>
Date:	Tue, 19 May 2015 18:48:14 +0530
From:	Sudip Mukherjee <sudipm.mukherjee@...il.com>
To:	Dan Carpenter <dan.carpenter@...cle.com>
Cc:	Greg KH <gregkh@...uxfoundation.org>,
	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>,
	Jean Delvare <jdelvare@...e.de>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 WIP 1/5] parport: add device-model to parport subsystem

On Tue, May 19, 2015 at 02:08:13PM +0300, Dan Carpenter wrote:
> On Wed, May 06, 2015 at 03:46:13PM +0530, Sudip Mukherjee wrote:
> > parport subsystem starts using the device-model. drivers using the
> > device-model has to define probe and should register the device with
> > parport with parport_register_dev_model()
> > 
> > Signed-off-by: Sudip Mukherjee <sudip@...torindia.org>
> > ---
<snip>
> > +static int is_parport(struct device *dev)
> > +{
> > +	return dev->type == &parport_device_type;
> > +}
> > +
> > +static int parport_probe(struct device *dev)
> > +{
> > +	struct parport_driver *drv;
> > +
> > +	if (is_parport(dev))
> > +		return -ENODEV;
> 
> Is this test reversed?  You guys have tested probing though so it
> doesn't seem like it can be reversed.
reversed means? this test is to check if the device is a parport or
pardevice. if it is a parport then we should not send that to the driver
probe function.
> 
> > +
> > +	drv = to_parport_driver(dev->driver);
<snip>
> > +
> > +void parport_bus_exit(void)
> > +{
> > +	bus_unregister(&parport_bus_type);
> > +}
> > +
> > +static int driver_check(struct device_driver *_drv, void *_port)
> 
> _drv is a poor name.  _port is a good name because it's actually port,
> but _drv should be dev_drv or something.  The port_check() function has
> a nice comment explaining why it exists can you add one here?
you have suggested _port in an earlier version, so I have taken the idea
about _drv from that :)
will change it and add the comment also. anyhow I will also submit a
patch to add these details in the documentation also.
> 
> > +{
<snip>	
> > +	/*
> > +	 * call the port_check function of the drivers registered in
> 
> s/port_check/driver_check/
my mistake :(
> 
> > +	 * new device model
> > +	 */
> > +
> > -	list_for_each_entry(port, &portlist, list)
> > -		drv->attach(port);
> > -	list_add(&drv->list, &drivers);
> > -	mutex_unlock(&registration_lock);
> > +	if (drv->probe) {
> 
> I feel like this is weird.  The driver should just set ->devmodel
> itself and we test for it here.
well, devmodel and this test all will be removed when we totally remove
the old model. For now we just need a way to check if the driver is using
the old model or the new model. So anything is ok. If you want I will
use the ->devmodel in the driver itself.

> 
> > +		/* using device model */
> > +		int ret;
<snip>
> >  void parport_put_port (struct parport *port)
> >  {
> > -	if (atomic_dec_and_test (&port->ref_count))
> > -		/* Can destroy it now. */
> > -		free_port (port);
> > -
> > -	return;
> > +	put_device(&port->bus_dev);
> 
> I don't understand this.  The comment is out of date now.  Part of the
> issue I'm having is that we need to support both old and new models for
> now and I just get confused.  This seems like new model stuff but then
> what happened to the old model stuff.
> 
> Also I seem to remember that the old model stuff was buggy here?  Is
> that it?  If this is a bug fix then send that separately so we can
> merge it right away.
> 
> Anyway, it's not your fault I'm confused.  But please, if it's a bugfix
> send that by itself and also update the comment.
It is not a bugfix. parport is totally in new model. we can not have few
parport in old model and few in new model. So parport is registered in
the device model, we are only maintaining few of the old linked lists to
make the code compatible. drivers using new model will show up in the sys
tree and who are using the old model will not show.

> 
> >  }
> >  
<snip>	
> > +	par_dev->name = devname;
> > +	par_dev->port = port;
> > +	par_dev->daisy = -1;
> > +	par_dev->preempt = par_dev_cb->preempt;
> > +	par_dev->wakeup = par_dev_cb->wakeup;
> > +	par_dev->private = par_dev_cb->private;
> > +	par_dev->flags = par_dev_cb->flags;
> 
> We have a par_dev->flags, par_dev->port->flags, and
> par_dev->port->physport->flags.  The are slightly different.  Do we need
> all three?
I have not touched these parts as all the drivers are still not converted.
after all drivers are converted then we will have a cleanup of this code,
and then we can see how to reduce or combine these flags.      
> 
> > +	par_dev->irq_func = par_dev_cb->irq_func;
<snip>
> > @@ -926,8 +1214,8 @@ int parport_claim_or_block(struct pardevice *dev)
> >  			       dev->port->physport->cad ?
> >  			       dev->port->physport->cad->name:"nobody");
> >  #endif
> > -	}
> > -	dev->waiting = 0;
> > +	} else if (r == 0)
> > +		dev->waiting = 0;
> 
> What is this change for?
not required. parport_claim is returning only -EAGAIN or 0.
I will remove that.

regards
sudip
> 
> >  	return r;
> >  }
> 
> regards,
> dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists