Re: [PATCH v4 WIP 2/4] i2c-parport: modify driver to use new parport device model

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

On Sun, May 03, 2015 at 03:33:40PM +0200, Jean Delvare wrote:
> Hi Sudip,
Thanks Jean for your time.	
> 
> On Tue, 28 Apr 2015 17:00:21 +0530, Sudip Mukherjee wrote:
<snip>
> > +	if (!is_parport(&port->bus_dev))
> > +		return;
> 
> Can this actually happen?
i got this idea from i2c_verify_client(), as I was getting problem with
different devices registered with the parallel port. But, anyways, i
guess I will not need it in the next iteration of the WIP and can be
handled in the core level.
> 
<snip>
> > +	adapter->pdev = parport_register_dev_model(port, name,
> > +						   &i2c_parport_cb);
> > +	kfree(name);
> 
> If you can free "name" at this point, this suggests that
> parport_register_dev_model made a copy somehow. In that case, please
> don't use dynamic memory allocation in the first place. Either use a
> static buffer and sprintf to it, or (probably better) pass the instance
> number to parport_register_dev_model() as a separate parameter.
well, first thought - there will be some drivers who will not have multiple
instances. but second thought - if we have separately device name and
instance number, we can just combine them when registering with the device
model, but it will become easier in the probe for the name comparison which
you have pointed out later in your reply.
I will try it out in the next iteration.
> 
> >  	if (!adapter->pdev) {
> >  		printk(KERN_ERR "i2c-parport: Unable to register with parport\n");
> >  		goto err_free;
> > @@ -237,39 +266,26 @@ static void i2c_parport_attach(struct parport *port)
> >  	parport_unregister_device(adapter->pdev);
> >   err_free:
> >  	kfree(adapter);
> > +	return;
> 
> This return statement serves no purpose.
sorry, it is a leftover from an idea I was trying,
> 
> >  }
> >  
> > -static void i2c_parport_detach(struct parport *port)
> > +static int i2c_parport_probe(struct device *dev)
> >  {
> > -	struct i2c_par *adapter, *_n;
> > +	char *name = dev_name(dev);
> 
> This adds the following warning at build time:
> 
>   CC [M]  drivers/i2c/busses/i2c-parport.o
> drivers/i2c/busses/i2c-parport.c: In function ‘i2c_parport_probe’:
> drivers/i2c/busses/i2c-parport.c:274:15: warning: initialization discards ‘const’ qualifier from pointer target type [enabled by default]
>   char *name = dev_name(dev);
> 
> Very easy to fix, just declare "name" as const char *.
I didnot get this warning, maybe I need to upgrade my gcc or will
W=1 show it?
> 
> >  
> > -	/* Walk the list */
<snip>
> >  
> > -	return parport_register_driver(&i2c_parport_driver);
> > +	return parport_register_drv(&i2c_parport_driver);
> 
> Can't you call parport_register_driver() for both models and decide
> what to do based on which members of struct parport_driver are set?
> This would be less confusing IMHO.
I guess it can be done. let me try it out.
> 
> >  }
> >  
<snip>
> > +	}
> > +	mutex_unlock(&adapter_list_lock);
> 
> Moving all this code here seems inappropriate. What if a parallel port
> is removed from the system while the i2c-parport driver is loaded? I
> think this can happen with laptop docking stations or parallel ports on
> PCI cards for example. Your new model should be able to deal with that,
> so each driver still needs a detach or remove function which the core
> can call on parallel port removal.
ok, will be done.
To be frank, I am actually confused with this part, not only for parport
subsystem but for all the other codes I have seen. We have a remove
function for all subsystems, lets assume PCI, so a pci driver is having
a remove callback. So if the particular pci device is removed then the
remove is called which does all the clearing part. But the driver still
remains registered, then what happens to the driver?
> 
my next WIP will have some changes in the core level also, so I shouldnot
add your Tested-by: to it. And I will again request you to check that.
And since Alan has not yet tested it on his backpack cd driver, so
please do not test this series. I will send in the next version in a day
or two. Please test that.

regards
sudip

> 
> -- 
> Jean Delvare
> SUSE L3 Support