Messages in this thread | | | Date | Wed, 20 May 2015 09:57:24 +0200 | From | Jean Delvare <> | Subject | Re: [PATCH v5 WIP 4/5] i2c-parport: use new parport device model |
| |
Hi Sudip,
On Wed, 6 May 2015 15:46:16 +0530, Sudip Mukherjee wrote: > modify i2c-parport driver to use the new parallel port device model.
Leading capital please.
> > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org> > --- > drivers/i2c/busses/i2c-parport.c | 20 +++++++++++++++++--- > 1 file changed, 17 insertions(+), 3 deletions(-)
I like it very much. The simplicity of this patch is IMHO a good sign that you are going in the right direction.
> > diff --git a/drivers/i2c/busses/i2c-parport.c b/drivers/i2c/busses/i2c-parport.c > index a9b25c3..6db5b45 100644 > --- a/drivers/i2c/busses/i2c-parport.c > +++ b/drivers/i2c/busses/i2c-parport.c > @@ -163,6 +163,11 @@ static void i2c_parport_irq(void *data) > "SMBus alert received but no ARA client!\n"); > } > > +static struct pardev_cb i2c_parport_cb = { > + .flags = PARPORT_FLAG_EXCL, > + .irq_func = i2c_parport_irq, > +};
There's no reason for this variable to be global. It is only needed temporarily at attach time if I understand correctly, so it should be local to function i2c_parport_attach().
> + > static void i2c_parport_attach(struct parport *port) > { > struct i2c_par *adapter; > @@ -184,11 +189,12 @@ static void i2c_parport_attach(struct parport *port) > printk(KERN_ERR "i2c-parport: Failed to kzalloc\n"); > return; > } > + i2c_parport_cb.private = adapter; > > pr_debug("i2c-parport: attaching to %s\n", port->name); > parport_disable_irq(port); > - adapter->pdev = parport_register_device(port, "i2c-parport", > - NULL, NULL, i2c_parport_irq, PARPORT_FLAG_EXCL, adapter); > + adapter->pdev = parport_register_dev_model(port, "i2c-parport", > + &i2c_parport_cb, i); > if (!adapter->pdev) { > printk(KERN_ERR "i2c-parport: Unable to register with parport\n"); > goto err_free; > @@ -281,10 +287,18 @@ static void i2c_parport_detach(struct parport *port) > mutex_unlock(&adapter_list_lock); > } > > +static int i2c_parport_probe(struct pardevice *par_dev) > +{ > + if (strcmp(par_dev->name, "i2c-parport")) > + return -ENODEV; > + return 0; > +}
I'm wondering, is there any reason why this can't be automated by the driver core part of the code? Most drivers will simply compare drv->name with par_dev->name, which could be done in function parport_probe() when no custom probe function is provided.
Now I see that you use the existence of the probe callback to decide that the driver implements the device driver model. I suppose you could use match_port instead, except that for some reason the paride driver doesn't implement one. Maybe it should, or maybe you can check of the presence of either to decide that this is a device model driver.
> + > static struct parport_driver i2c_parport_driver = { > .name = "i2c-parport", > - .attach = i2c_parport_attach, > + .match_port = i2c_parport_attach, > .detach = i2c_parport_detach, > + .probe = i2c_parport_probe, > }; > > /* ----- Module loading, unloading and information ------------------------ */
Tested OK on my ADM1032 evaluation board.
Tested-by: Jean Delvare <jdelvare@suse.de>
-- Jean Delvare SUSE L3 Support
| |