Re: [PATCH v5 WIP 4/5] i2c-parport: use new parport device model

[Jean Delvare <jdelvare@suse.de>]

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