Re: [PATCH] i2c: don't autograb i2c-pca-isa

Re: [PATCH] i2c: don't autograb i2c-pca-isa

[Jean Delvare]

Hi Rene,

Please Cc the i2c list for i2c patches (added.)

On Fri, 08 Aug 2008 19:19:17 +0200, Rene Herman wrote:
> Grabbing ISA bus resources without anything or anyone telling us we
> should can break boot on randconfig/allyesconfig builds by keeping
> resources that are in fact owned by different hardware busy and does
> as reported by Ingo Molnar.

I don't think it makes much sense to boot randomconfig kernels. The
i2c-pca-isa driver is for rare hardware, most people will not use it
and certainly won't build it into the kernel. So I don't think there
really is a problem in practice here. That being said...

> Generally it's also dangerous to just poke at random I/O ports and
> especially those in the range where other old easily confused ISA
> hardware might live.
> 
> For this specialized I2C bus driver, insist that the user specifies
> the resources before grabbing them.

I agree that such legacy drivers should not assume default I/O port and
IRQ, that's dangerous.

> 
> The^WA user of this driver is a one time
> 
> echo "options i2c-pca-isa base=0x330 irq=10" >> /etc/modprobe.conf
> 
> away from the old behaviour.

I am curious how many users of this driver are left. Maybe it's time to
get rid of it.

> Signed-off-by: Rene Herman <rene.herman-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-pca-isa.c |   20 +++++++++++++++++---
>  1 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-pca-isa.c b/drivers/i2c/busses/i2c-pca-isa.c
> index a119784..2579169 100644
> --- a/drivers/i2c/busses/i2c-pca-isa.c
> +++ b/drivers/i2c/busses/i2c-pca-isa.c
> @@ -36,8 +36,8 @@
>  #define DRIVER "i2c-pca-isa"
>  #define IO_SIZE 4
>  
> -static unsigned long base   = 0x330;
> -static int irq 	  = 10;
> +static unsigned long base;
> +static int irq = -1;
>  
>  /* Data sheet recommends 59kHz for 100kHz operation due to variation
>   * in the actual clock rate */
> @@ -107,6 +107,19 @@ static struct i2c_adapter pca_isa_ops = {
>  	.timeout	= 100,
>  };
>  
> +static int __devinit pca_isa_match(struct device *dev, unsigned int id)
> +{
> +	int match = base != 0;
> +
> +	if (match) {
> +		if (irq == -1)
> +			dev_warn(dev, "using poling mode (specify irq)\n");

Spelling: polling.

> +	} else
> +		dev_err(dev, "please specify base\n");
> +
> +	return match;
> +}
> +
>  static int __devinit pca_isa_probe(struct device *dev, unsigned int id)
>  {
>  	init_waitqueue_head(&pca_wait);
> @@ -153,7 +166,7 @@ static int __devexit pca_isa_remove(struct device *dev, unsigned int id)
>  {
>  	i2c_del_adapter(&pca_isa_ops);
>  
> -	if (irq > 0) {
> +	if (irq > -1) {
>  		disable_irq(irq);
>  		free_irq(irq, &pca_isa_ops);
>  	}
> @@ -163,6 +176,7 @@ static int __devexit pca_isa_remove(struct device *dev, unsigned int id)
>  }
>  
>  static struct isa_driver pca_isa_driver = {
> +	.match		= pca_isa_match,
>  	.probe		= pca_isa_probe,
>  	.remove		= __devexit_p(pca_isa_remove),
>  	.driver = {

Other than that, this looks OK. I'll queue this for 2.6.28.

Thanks,
-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] i2c: don't autograb i2c-pca-isa

[Rene Herman]

On 09-08-08 15:28, Jean Delvare wrote:

> On Fri, 08 Aug 2008 19:19:17 +0200, Rene Herman wrote:

>> Grabbing ISA bus resources without anything or anyone telling us we
>> should can break boot on randconfig/allyesconfig builds by keeping
>> resources that are in fact owned by different hardware busy and does
>> as reported by Ingo Molnar.
> 
> I don't think it makes much sense to boot randomconfig kernels. The
> i2c-pca-isa driver is for rare hardware, most people will not use it
> and certainly won't build it into the kernel. So I don't think there
> really is a problem in practice here. That being said...

Ingo actually hit this -- i2c-pca-isa "broke" his e1000 by grabbing 
IRQ10. He does randonfig builds for testing purposes (which a little 
while ago caught a dumb yet very old bug in an ALSA ISA driver which 
impressed the usefulness of such on me...)

> I am curious how many users of this driver are left. Maybe it's time to
> get rid of it.

I rather expect the number is 0 but unless it's a maintenance burden I'd 
say to keep it around if only as a "see, this is how simple it gets" 
sort of thing.

(not that the other drivers in there are wildly complex it seems upon 
looking, so, well, what do I know)

>> +static int __devinit pca_isa_match(struct device *dev, unsigned int id)
>> +{
>> +	int match = base != 0;
>> +
>> +	if (match) {
>> +		if (irq == -1)
>> +			dev_warn(dev, "using poling mode (specify irq)\n");
> 
> Spelling: polling.

Argh.

> Other than that, this looks OK. I'll queue this for 2.6.28.

Thanks.

Rene.

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c