Re: [PATCH] i2c: i2c-pca-platform: Change i2c adapter name

[Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>]

Hi Nobuhiro,

On Mon, 18 Oct 2010 12:24:30 +0900, Nobuhiro Iwamatsu wrote:
> In kernel which enableded this driver output The following warning,
> because a slash (/) is in the i2c adapter name by the current driver.
> This set another adapter name (i2c-pca-platform at 0x) what removed slash.
> 
> [    2.872000] PCA9564/PCA9665 at 0x06000000: Registering gpio failed!
> [    2.876000] name 'PCA9564/PCA9665 at 0x06000000'
> [    2.880000] ------------[ cut here ]------------
> [    2.880000] WARNING: at fs/proc/generic.c:323
> [    2.880000] Modules linked in:
> [    2.880000]
> [    2.880000] Pid : 1, Comm:           swapper
> [    2.880000] CPU : 0                  Not tainted  (2.6.36-rc1-00238-g67e5b3a #261)
> [    2.880000]
> [    2.880000] PC is at __xlate_proc_name+0x88/0xa4
> [    2.880000] PR is at __xlate_proc_name+0x88/0xa4
> [    2.880000] PC  : 800b3bc4 SP  : 9fc41d34 SR  : 40008001 TEA : c0018000
> [    2.880000] R0  : 00000037 R1  : 9fc40000 R2  : 9fc40000 R3  : 8049db48
> [    2.880000] R4  : 00000001 R5  : 000000f0 R6  : ffffffff R7  : 8019e450
> [    2.880000] R8  : 00000000 R9  : 9fc41d9c R10 : 00000007 R11 : 9fc41d9c
> [    2.880000] R12 : 9fc41d80 R13 : 80167fce R14 : 9fc41d34
> [    2.880000] MACH: 00000461 MACL: 00000000 GBR : 00000000 PR  : 800b3bc4
> [    2.880000]
> [    2.880000] Call trace:
> [    2.880000]  [<800b4056>] __proc_create+0x46/0x110
> [    2.880000]  [<80162b00>] strlen+0x0/0x58
> [    2.880000]  [<800b4824>] proc_mkdir_mode+0x24/0x60
> [    2.880000]  [<800b486e>] proc_mkdir+0xe/0x1c
> [    2.880000]  [<8004740a>] register_handler_proc+0x9e/0xd4
> [    2.880000]  [<80045ab2>] __setup_irq+0x226/0x2c0
> [    2.880000]  [<80075d50>] kmem_cache_alloc+0x94/0xf0
> [    2.880000]  [<80045bcc>] request_threaded_irq+0x80/0xdc
> [    2.880000]  [<802041c0>] i2c_pca_pf_handler+0x0/0x3c
> [    2.880000]  [<802ecb62>] i2c_pca_pf_probe+0x162/0x294
> [    2.880000]  [<801a3b38>] platform_drv_probe+0x14/0x1c
> (...)

The i2c adapter names are strings which appear as the _contents_ of
sysfs attributes. They don't appear in procfs. What happens here is
that the adapter name is used also as the IRQ name:

	if (irq) {
		ret = request_irq(irq, i2c_pca_pf_handler,
			IRQF_TRIGGER_FALLING, i2c->adap.name, i2c);
		if (ret)
			goto e_reqirq;
	}

This is the problem. i2c->adap.name shouldn't be used here, instead the
driver name "i2c-pca-platform" should be used.

> (...)
> Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
> CC: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-pca-platform.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-pca-platform.c b/drivers/i2c/busses/i2c-pca-platform.c
> index ef5c784..4cc3cdf 100644
> --- a/drivers/i2c/busses/i2c-pca-platform.c
> +++ b/drivers/i2c/busses/i2c-pca-platform.c
> @@ -27,6 +27,8 @@
>  
>  #include <asm/irq.h>
>  
> +#define DRIVER_NAME "i2c-pca-platform"
> +
>  struct i2c_pca_pf_data {
>  	void __iomem			*reg_base;
>  	int				irq;	/* if 0, use polling */
> @@ -171,7 +173,7 @@ static int __devinit i2c_pca_pf_probe(struct platform_device *pdev)
>  	i2c->adap.nr = pdev->id >= 0 ? pdev->id : 0;
>  	i2c->adap.owner = THIS_MODULE;
>  	snprintf(i2c->adap.name, sizeof(i2c->adap.name),
> -		 "PCA9564/PCA9665 at 0x%08lx",
> +		 DRIVER_NAME " at 0x%08lx",
>  		 (unsigned long) res->start);

I would like to avoid changing the adapter name when we don't have to.
People may be using this string as an identifier in i2c tools. Please
leave the adapter name unchanged and only change the IRQ name.

>  	i2c->adap.algo_data = &i2c->algo_data;
>  	i2c->adap.dev.parent = &pdev->dev;
> @@ -250,7 +252,7 @@ e_remap:
>  e_alloc:
>  	release_mem_region(res->start, resource_size(res));
>  e_print:
> -	printk(KERN_ERR "Registering PCA9564/PCA9665 FAILED! (%d)\n", ret);
> +	printk(KERN_ERR "Registering " DRIVER_NAME " FAILED! (%d)\n", ret);

This is semantically incorrect. We are registering a device here, not a
driver. The proposed change makes this error message confusing. Please
revert.

>  	return ret;
>  }
>  
> @@ -278,7 +280,7 @@ static struct platform_driver i2c_pca_pf_driver = {
>  	.probe = i2c_pca_pf_probe,
>  	.remove = __devexit_p(i2c_pca_pf_remove),
>  	.driver = {
> -		.name = "i2c-pca-platform",
> +		.name = DRIVER_NAME,
>  		.owner = THIS_MODULE,
>  	},
>  };


-- 
Jean Delvare