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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150520095724.286c21ef@endymion.delvare>
Date:	Wed, 20 May 2015 09:57:24 +0200
From:	Jean Delvare <jdelvare@...e.de>
To:	Sudip Mukherjee <sudipm.mukherjee@...il.com>
Cc:	Greg KH <gregkh@...uxfoundation.org>,
	Dan Carpenter <dan.carpenter@...cle.com>,
	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>,
	linux-kernel@...r.kernel.org
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@...torindia.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@...e.de>

-- 
Jean Delvare
SUSE L3 Support
--
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/