clean up and warnings patch for 2.6.12-rc4-mm1 i2c-chip

clean up and warnings patch for 2.6.12-rc4-mm1 i2c-chip

[Alexey Fisher]

Hi here is some clean ups for Kconfig in i2c/chips subdirectory and error handling fix up for chip max1619.
Best  regards.
Alexey Fisher


diff -uprN linux/drivers/i2c/chips/Kconfig linux-2.6-dev/drivers/i2c/chips/Kconfig
--- linux/drivers/i2c/chips/Kconfig	2005-05-15 22:49:31.000000000 +0200
+++ linux-2.6-dev/drivers/i2c/chips/Kconfig	2005-05-15 22:58:28.000000000 +0200
@@ -29,6 +29,7 @@ config SENSORS_ADM1025
 	help
 	  If you say yes here you get support for Analog Devices ADM1025
 	  and Philips NE1619 sensor chips.
+	  
 	  This driver can also be built as a module.  If so, the module
 	  will be called adm1025.
 
@@ -38,6 +39,7 @@ config SENSORS_ADM1026
 	select I2C_SENSOR
 	help
 	  If you say yes here you get support for Analog Devices ADM1026
+	  
 	  This driver can also be built as a module.  If so, the module
 	  will be called adm1026.
 
@@ -48,6 +50,7 @@ config SENSORS_ADM1031
 	help
 	  If you say yes here you get support for Analog Devices ADM1031 
 	  and ADM1030 sensor chips.
+	  
 	  This driver can also be built as a module.  If so, the module
 	  will be called adm1031.
 
@@ -198,8 +201,7 @@ config SENSORS_LM78
 	select I2C_SENSOR
 	help
 	  If you say yes here you get support for National Semiconductor LM78,
-	  LM78-J and LM79.  This can also be built as a module which can be
-	  inserted and removed while the kernel is running.
+	  LM78-J and LM79.
 
 	  This driver can also be built as a module.  If so, the module
 	  will be called lm78.
@@ -232,7 +234,7 @@ config SENSORS_LM85
 	select I2C_SENSOR
 	help
 	  If you say yes here you get support for National Semiconductor LM85
-	  sensor chips and clones: ADT7463 and ADM1027.
+	  sensor chips and clones: ADT7463, EMC6D100, EMC6D102 and ADM1027.
 
 	  This driver can also be built as a module.  If so, the module
 	  will be called lm85.
diff -uprN linux/drivers/i2c/chips/max1619.c linux-2.6-dev/drivers/i2c/chips/max1619.c
--- linux/drivers/i2c/chips/max1619.c	2005-05-15 22:49:31.000000000 +0200
+++ linux-2.6-dev/drivers/i2c/chips/max1619.c	2005-05-15 21:05:56.000000000 +0200
@@ -195,6 +195,7 @@ static int max1619_detect(struct i2c_ada
 	u8 reg_config=0, reg_convrate=0, reg_status=0;
 	u8 man_id, chip_id;
 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
+		err = -ENODEV;
 		goto exit;
 
 	if (!(data = kmalloc(sizeof(struct max1619_data), GFP_KERNEL))) {
@@ -234,6 +235,7 @@ static int max1619_detect(struct i2c_ada
 			dev_dbg(&adapter->dev,
 				"MAX1619 detection failed at 0x%02x.\n",
 				address);
+			return -ENODEV;
 			goto exit_free;
 		}
 	}
@@ -254,6 +256,7 @@ static int max1619_detect(struct i2c_ada
 			dev_info(&adapter->dev,
 			    "Unsupported chip (man_id=0x%02X, "
 			    "chip_id=0x%02X).\n", man_id, chip_id);
+			return -ENODEV;
 			goto exit_free;
 		}
 	
diff -uprN linux/drivers/pci/pci.ids linux-2.6-dev/drivers/pci/pci.ids
--- linux/drivers/pci/pci.ids	2005-05-07 07:20:31.000000000 +0200
+++ linux-2.6-dev/drivers/pci/pci.ids	2005-05-15 23:16:21.000000000 +0200
@@ -5322,6 +5322,7 @@
 	0459  LT WinModem
 	045a  LT WinModem
 	045c  LT WinModem
+	045d  LT WinModem
 	0461  V90 WildWire Modem
 	0462  V90 WildWire Modem
 	0480  Venus Modem (V90, 56KFlex)

Re: clean up and warnings patch for 2.6.12-rc4-mm1 i2c-chip

[Daniel Paschka]

[-- Attachment #1: Type: text/plain, Size: 1475 bytes --]

Alexey Fisher wrote:

> diff -uprN linux/drivers/i2c/chips/max1619.c linux-2.6-dev/drivers/i2c/chips/max1619.c
> --- linux/drivers/i2c/chips/max1619.c	2005-05-15 22:49:31.000000000 +0200
> +++ linux-2.6-dev/drivers/i2c/chips/max1619.c	2005-05-15 21:05:56.000000000 +0200
> @@ -195,6 +195,7 @@ static int max1619_detect(struct i2c_ada
>  	u8 reg_config=0, reg_convrate=0, reg_status=0;
>  	u8 man_id, chip_id;
>  	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> +		err = -ENODEV;
>  		goto exit;


Sure about that, I think there are some brackets missing.

  
>  	if (!(data = kmalloc(sizeof(struct max1619_data), GFP_KERNEL))) {
> @@ -234,6 +235,7 @@ static int max1619_detect(struct i2c_ada
>  			dev_dbg(&adapter->dev,
>  				"MAX1619 detection failed at 0x%02x.\n",
>  				address);
> +			return -ENODEV;
>  			goto exit_free;
>  		}
>  	}


I would expect gcc to throw a warning about unreachable code here
because goto exit_free ist behind a return statement.
Don't you mean
err = -ENODEV;
goto exit_free;

That way the data structure data will be freed, too.

> @@ -254,6 +256,7 @@ static int max1619_detect(struct i2c_ada
>  			dev_info(&adapter->dev,
>  			    "Unsupported chip (man_id=0x%02X, "
>  			    "chip_id=0x%02X).\n", man_id, chip_id);
> +			return -ENODEV;
>  			goto exit_free;
>  		}

Same here.

I am not using this driver and am not fully up to date with the kernel
sources so I am sorry if I am talking nonesense here.

Bye Daniel

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: clean up and warnings patch for 2.6.12-rc4-mm1 i2c-chip

[Jean Delvare]

Hi Alexey,

> Hi here is some clean ups for Kconfig in i2c/chips subdirectory and
> error handling fix up for chip max1619. Best  regards.

Thanks for the patch, see my comments inline.

> diff -uprN linux/drivers/i2c/chips/Kconfig linux-2.6-dev/drivers/i2c/chips/Kconfig
> --- linux/drivers/i2c/chips/Kconfig	2005-05-15 22:49:31.000000000 +0200
> +++ linux-2.6-dev/drivers/i2c/chips/Kconfig	2005-05-15 22:58:28.000000000 +0200
> @@ -29,6 +29,7 @@ config SENSORS_ADM1025
>  	help
>  	  If you say yes here you get support for Analog Devices ADM1025
>  	  and Philips NE1619 sensor chips.
> +	  
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called adm1025.
>  
> @@ -38,6 +39,7 @@ config SENSORS_ADM1026
>  	select I2C_SENSOR
>  	help
>  	  If you say yes here you get support for Analog Devices ADM1026
> +	  
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called adm1026.
>  
> @@ -48,6 +50,7 @@ config SENSORS_ADM1031
>  	help
>  	  If you say yes here you get support for Analog Devices ADM1031
>  	   and ADM1030 sensor chips.
> +	  
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called adm1031.
>  
> @@ -198,8 +201,7 @@ config SENSORS_LM78
>  	select I2C_SENSOR
>  	help
>  	  If you say yes here you get support for National Semiconductor
>  	  LM78,
> -	  LM78-J and LM79.  This can also be built as a module which can be
> -	  inserted and removed while the kernel is running.
> +	  LM78-J and LM79.
>  
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called lm78.
> @@ -232,7 +234,7 @@ config SENSORS_LM85
>  	select I2C_SENSOR
>  	help
>  	  If you say yes here you get support for National Semiconductor
>  	  LM85
> -	  sensor chips and clones: ADT7463 and ADM1027.
> +	  sensor chips and clones: ADT7463, EMC6D100, EMC6D102 and ADM1027.
>  
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called lm85.

Yes, these fixes to Kconfig are all correct.

> diff -uprN linux/drivers/i2c/chips/max1619.c linux-2.6-dev/drivers/i2c/chips/max1619.c
> --- linux/drivers/i2c/chips/max1619.c	2005-05-15 22:49:31.000000000 +0200
> +++ linux-2.6-dev/drivers/i2c/chips/max1619.c	2005-05-15 21:05:56.000000000 +0200
> @@ -195,6 +195,7 @@ static int max1619_detect(struct i2c_ada
>  	u8 reg_config=0, reg_convrate=0, reg_status=0;
>  	u8 man_id, chip_id;
>  	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> +		err = -ENODEV;
>  		goto exit;
>  
>  	if (!(data = kmalloc(sizeof(struct max1619_data), GFP_KERNEL)))
>  	{
> @@ -234,6 +235,7 @@ static int max1619_detect(struct i2c_ada
>  			dev_dbg(&adapter->dev,
>  				"MAX1619 detection failed at 0x%02x.\n",
>  				address);
> +			return -ENODEV;
>  			goto exit_free;
>  		}
>  	}
> @@ -254,6 +256,7 @@ static int max1619_detect(struct i2c_ada
>  			dev_info(&adapter->dev,
>  			    "Unsupported chip (man_id=0x%02X, "
>  			    "chip_id=0x%02X).\n", man_id, chip_id);
> +			return -ENODEV;
>  			goto exit_free;
>  		}
>  	

No, these changes are not correct. Due to the fact i2c_detect() is
designed, i2c detection functions should never return -ENODEV. Doing so
would result in i2c_detect() to skip the probing of remaining addresses
for a given chip driver. We might change i2c_detect() to make it behave
differently at some point, but for now your previous code is correct and
should not be changed.

(Also note that the code of your changes itself is not correct at all,
as was pointed out elsewhere in this thread - but it doesn't really
matter now.)

> diff -uprN linux/drivers/pci/pci.ids linux-2.6-dev/drivers/pci/pci.ids
> --- linux/drivers/pci/pci.ids	2005-05-07 07:20:31.000000000 +0200
> +++ linux-2.6-dev/drivers/pci/pci.ids	2005-05-15 23:16:21.000000000 +0200
> @@ -5322,6 +5322,7 @@
>  	0459  LT WinModem
>  	045a  LT WinModem
>  	045c  LT WinModem
> +	045d  LT WinModem
>  	0461  V90 WildWire Modem
>  	0462  V90 WildWire Modem
>  	0480  Venus Modem (V90, 56KFlex)

This is a completely different change. Please submit unrelated changes
as different patches, because they will be applied by different persons
so it's way more convenient that way.

If you resubmit a patch with only the drivers/i2c/chips/Kconfig changes,
I'll get it applied. Don't forget a desription of the patch and the
Signed-Off-By line with your name and mail address at the top of the
post.

Thanks,
-- 
Jean Delvare

Re: clean up and warnings patch for 2.6.12-rc4-mm1 i2c-chip

[Alexey Fisher]

Hi Jean
with module w83627hf  i found useful due  detection return -ENODEV
because I can see in commandline if it's some thing wrong. If it's not correct, there is a bug 
in the w83627hf and some other drivers.



int w83627hf_detect(struct i2c_adapter *adapter, int address,
                   int kind)
{
        int val;
        struct i2c_client *new_client;
        struct w83627hf_data *data;
        int err = 0;
        const char *client_name = "";

        if (!i2c_is_isa_adapter(adapter)) {
                err = -ENODEV;
                goto ERROR0;
        }

Re: clean up and warnings patch for 2.6.12-rc4-mm1 i2c-chip

[Jean Delvare]

Hi Alexey,

> with module w83627hf  i found useful due  detection return -ENODEV
> because I can see in commandline if it's some thing wrong. If it's not
> correct, there is a bug in the w83627hf and some other drivers.
>
> int w83627hf_detect(struct i2c_adapter *adapter, int address,
>                    int kind)
> {
>         int val;
>         struct i2c_client *new_client;
>         struct w83627hf_data *data;
>         int err = 0;
>         const char *client_name = "";
>
>         if (!i2c_is_isa_adapter(adapter)) {
>                 err = -ENODEV;
>                 goto ERROR0;
>         }

This is actually not correct if you consider the i2c_detect() design, but
happens to cause no problem in this specific case - and, ironically,
might even speed up the detection loop.

The way i2c_detect() works for now, it will stop probing a given adapter
as soon as one address probed on that bus returned an error. As it
happens that i2c_is_isa_adapter(adapter) is either true or false for all
addresses of a given adapter, skipping to the next adapter directly when
the bus type (isa or not) doesn't match actually makes sense.

At any rate, I have plans to rework the way ISA hardware monitoring chips
are handled, so this code is likely to be gone in a near future anyway
(providing I can actually find the time to work on this... sigh). Things
should be much clearer after that.

Thanks,
--
Jean Delvare