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)
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
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
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;
}
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