drivers/hwmon/emc1403.c: add support for emc1412

PATCH In review Josef Gajdusek 3 participants 8 messages May 11, 2014 – May 12, 2014 Hardware monitoring

lore

Share & tools

lore

Josef Gajdusek

author

2014-05-11 13:00 UTC #0 lore raw

PATCH 1/1 drivers/hwmon/emc1403.c: add support for emc1412

Adds support for emc1412.

---

drivers/hwmon/emc1403.c +84 ~~-12~~

index 90ec117..47a98a7 100644
@@ -40,7 +40,7 @@
 
 struct thermal_data {
 	struct i2c_client *client;
-	const struct attribute_group *groups[3];
+	const struct attribute_group *groups[2];
 	struct mutex mutex;
 	/*
 	 * Cache the hyst value so we don't keep re-reading it. In theory

@@ -252,6 +252,28 @@ static SENSOR_DEVICE_ATTR(temp4_crit_hyst, S_IRUGO | S_IWUSR,
 static SENSOR_DEVICE_ATTR_2(power_state, S_IRUGO | S_IWUSR,
 	show_bit, store_bit, 0x03, 0x40);
 
+
+static struct attribute *emc1412_attrs[] = {
+	&sensor_dev_attr_temp1_min.dev_attr.attr,
+	&sensor_dev_attr_temp1_max.dev_attr.attr,
+	&sensor_dev_attr_temp1_crit.dev_attr.attr,
+	&sensor_dev_attr_temp1_input.dev_attr.attr,
+	&sensor_dev_attr_temp1_crit_hyst.dev_attr.attr,
+
+	&sensor_dev_attr_temp2_min.dev_attr.attr,
+	&sensor_dev_attr_temp2_max.dev_attr.attr,
+	&sensor_dev_attr_temp2_crit.dev_attr.attr,
+	&sensor_dev_attr_temp2_input.dev_attr.attr,
+	&sensor_dev_attr_temp2_crit_hyst.dev_attr.attr,
+
+	&sensor_dev_attr_power_state.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group emc1412_group = {
+		.attrs = emc1412_attrs,
+};
+
 static struct attribute *emc1403_attrs[] = {
 	&sensor_dev_attr_temp1_min.dev_attr.attr,
 	&sensor_dev_attr_temp1_max.dev_attr.attr,

@@ -261,6 +283,7 @@ static struct attribute *emc1403_attrs[] = {
 	&sensor_dev_attr_temp1_max_alarm.dev_attr.attr,
 	&sensor_dev_attr_temp1_crit_alarm.dev_attr.attr,
 	&sensor_dev_attr_temp1_crit_hyst.dev_attr.attr,
+
 	&sensor_dev_attr_temp2_min.dev_attr.attr,
 	&sensor_dev_attr_temp2_max.dev_attr.attr,
 	&sensor_dev_attr_temp2_crit.dev_attr.attr,

@@ -269,6 +292,7 @@ static struct attribute *emc1403_attrs[] = {
 	&sensor_dev_attr_temp2_max_alarm.dev_attr.attr,
 	&sensor_dev_attr_temp2_crit_alarm.dev_attr.attr,
 	&sensor_dev_attr_temp2_crit_hyst.dev_attr.attr,
+
 	&sensor_dev_attr_temp3_min.dev_attr.attr,
 	&sensor_dev_attr_temp3_max.dev_attr.attr,
 	&sensor_dev_attr_temp3_crit.dev_attr.attr,

@@ -277,6 +301,7 @@ static struct attribute *emc1403_attrs[] = {
 	&sensor_dev_attr_temp3_max_alarm.dev_attr.attr,
 	&sensor_dev_attr_temp3_crit_alarm.dev_attr.attr,
 	&sensor_dev_attr_temp3_crit_hyst.dev_attr.attr,
+
 	&sensor_dev_attr_power_state.dev_attr.attr,
 	NULL
 };

@@ -286,6 +311,33 @@ static const struct attribute_group emc1403_group = {
 };
 
 static struct attribute *emc1404_attrs[] = {
+	&sensor_dev_attr_temp1_min.dev_attr.attr,
+	&sensor_dev_attr_temp1_max.dev_attr.attr,
+	&sensor_dev_attr_temp1_crit.dev_attr.attr,
+	&sensor_dev_attr_temp1_input.dev_attr.attr,
+	&sensor_dev_attr_temp1_min_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp1_max_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp1_crit_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp1_crit_hyst.dev_attr.attr,
+
+	&sensor_dev_attr_temp2_min.dev_attr.attr,
+	&sensor_dev_attr_temp2_max.dev_attr.attr,
+	&sensor_dev_attr_temp2_crit.dev_attr.attr,
+	&sensor_dev_attr_temp2_input.dev_attr.attr,
+	&sensor_dev_attr_temp2_min_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp2_max_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp2_crit_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp2_crit_hyst.dev_attr.attr,
+
+	&sensor_dev_attr_temp3_min.dev_attr.attr,
+	&sensor_dev_attr_temp3_max.dev_attr.attr,
+	&sensor_dev_attr_temp3_crit.dev_attr.attr,
+	&sensor_dev_attr_temp3_input.dev_attr.attr,
+	&sensor_dev_attr_temp3_min_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp3_max_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp3_crit_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp3_crit_hyst.dev_attr.attr,
+
 	&sensor_dev_attr_temp4_min.dev_attr.attr,
 	&sensor_dev_attr_temp4_max.dev_attr.attr,
 	&sensor_dev_attr_temp4_crit.dev_attr.attr,

@@ -294,6 +346,8 @@ static struct attribute *emc1404_attrs[] = {
 	&sensor_dev_attr_temp4_max_alarm.dev_attr.attr,
 	&sensor_dev_attr_temp4_crit_alarm.dev_attr.attr,
 	&sensor_dev_attr_temp4_crit_hyst.dev_attr.attr,
+
+	&sensor_dev_attr_power_state.dev_attr.attr,
 	NULL
 };

@@ -301,11 +355,13 @@ static const struct attribute_group emc1404_group = {
 	.attrs = emc1404_attrs,
 };
 
+
+
 static int emc1403_detect(struct i2c_client *client,
 			struct i2c_board_info *info)
 {
 	int id;
-	/* Check if thermal chip is SMSC and EMC1403 or EMC1423 */
+	/* Check whether the chip is SMSC and get its type */
 
 	id = i2c_smbus_read_byte_data(client, THERMAL_SMSC_ID_REG);
 	if (id != 0x5d)

@@ -313,6 +369,9 @@ static int emc1403_detect(struct i2c_client *client,
 
 	id = i2c_smbus_read_byte_data(client, THERMAL_PID_REG);
 	switch (id) {
+	case 0x20:
+		strlcpy(info->type, "emc1412", I2C_NAME_SIZE);
+		break;
 	case 0x21:
 		strlcpy(info->type, "emc1403", I2C_NAME_SIZE);
 		break;

@@ -330,9 +389,9 @@ static int emc1403_detect(struct i2c_client *client,
 	}
 
 	id = i2c_smbus_read_byte_data(client, THERMAL_REVISION_REG);
-	if (id != 0x01)
+	if (id != 0x01 && id != 0x04) {
 		return -ENODEV;
-
+	}
 	return 0;
 }

@@ -351,9 +410,17 @@ static int emc1403_probe(struct i2c_client *client,
 	mutex_init(&data->mutex);
 	data->hyst_valid = jiffies - 1;		/* Expired */
 
-	data->groups[0] = &emc1403_group;
-	if (id->driver_data)
-		data->groups[1] = &emc1404_group;
+	switch (id->driver_data) {
+	case 0:
+		data->groups[0] = &emc1412_group;
+		break;
+	case 1:
+		data->groups[0] = &emc1403_group;
+		break;
+	case 2:
+		data->groups[0] = &emc1404_group;
+		break;
+	}
 
 	hwmon_dev = hwmon_device_register_with_groups(&client->dev,
 						      client->name, data,

@@ -366,14 +433,19 @@ static int emc1403_probe(struct i2c_client *client,
 }
 
 static const unsigned short emc1403_address_list[] = {
-	0x18, 0x29, 0x4c, 0x4d, I2C_CLIENT_END
+	/* emc1403/emc1404/emc1423/emc1424 */
+	0x4c, 0x4d, 0x18, 0x29,
+	/* emc1412 */
+	0x5c, 0x4c, 0x6c, 0x1c, 0x3c, I2C_CLIENT_END
 };
 
+/* Last number in name indicates the amount of channels */
 static const struct i2c_device_id emc1403_idtable[] = {
-	{ "emc1403", 0 },
-	{ "emc1404", 1 },
-	{ "emc1423", 0 },
-	{ "emc1424", 1 },
+	{ "emc1412", 0 },
+	{ "emc1403", 1 },
+	{ "emc1423", 1 },
+	{ "emc1404", 2 },
+	{ "emc1424", 2 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, emc1403_idtable);

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Guenter Roeck

maintainer

2014-05-11 22:40 UTC #1 lore raw

Quoted 2 lines · Josef Gajdusek

Adds support for emc1412.

It is customary to explain the difference to already supported chips and what changes were made to support the new chip(s).

drivers/hwmon/emc1403.c line 43

Quoted 11 lines

---

drivers/hwmon/emc1403.c +1 -1

index 90ec117..47a98a7 100644
@@ -40,7 +40,7 @@
 
  struct thermal_data {
  	struct i2c_client *client;
-	const struct attribute_group *groups[3];
+	const struct attribute_group *groups[2];

The purpose of having more than one group is to simplify support for optional features while at the same time avoiding duplication. This is why there used to be two groups (plus an unused entry for list termination).

Separating and duplicating all attributes into one group per supported chip type defeats that purpose. Please don't do that, and revert the related changes.

This should not be reduced to two entries, but increased to four or even five; see below.

Quoted 15 lines

drivers/hwmon/emc1403.c +8 -0

  	struct mutex mutex;
  	/*
  	 * Cache the hyst value so we don't keep re-reading it. In theory

@@ -252,6 +252,28 @@ static SENSOR_DEVICE_ATTR(temp4_crit_hyst, S_IRUGO | S_IWUSR,
  static SENSOR_DEVICE_ATTR_2(power_state, S_IRUGO | S_IWUSR,
  	show_bit, store_bit, 0x03, 0x40);
 
+
+static struct attribute *emc1412_attrs[] = {
+	&sensor_dev_attr_temp1_min.dev_attr.attr,
+	&sensor_dev_attr_temp1_max.dev_attr.attr,
+	&sensor_dev_attr_temp1_crit.dev_attr.attr,
+	&sensor_dev_attr_temp1_input.dev_attr.attr,
+	&sensor_dev_attr_temp1_crit_hyst.dev_attr.attr,
+

drivers/hwmon/emc1403.c static SENSOR_DEVICE_ATTR(temp4_crit_hyst, S_IRUGO | S_IWUSR, · line 272

Quoted 10 lines

drivers/hwmon/emc1403.c +10 -0

+	&sensor_dev_attr_temp2_min.dev_attr.attr,
+	&sensor_dev_attr_temp2_max.dev_attr.attr,
+	&sensor_dev_attr_temp2_crit.dev_attr.attr,
+	&sensor_dev_attr_temp2_input.dev_attr.attr,
+	&sensor_dev_attr_temp2_crit_hyst.dev_attr.attr,
+
+	&sensor_dev_attr_power_state.dev_attr.attr,
+	NULL
+};
+

This should become the core attribute group, ie the list of attributes supported by all chips.

The EMC1412 does support alarms, so I would suggest to add another optional group for EMC1412 alarms (needed because the alarm register and bits are not compatible to the registers/bits used by other chips supported by this driver).

Quoted 24 lines

drivers/hwmon/emc1403.c +6 -0

+static const struct attribute_group emc1412_group = {
+		.attrs = emc1412_attrs,
+};
+
  static struct attribute *emc1403_attrs[] = {
  	&sensor_dev_attr_temp1_min.dev_attr.attr,
  	&sensor_dev_attr_temp1_max.dev_attr.attr,

@@ -261,6 +283,7 @@ static struct attribute *emc1403_attrs[] = {
  	&sensor_dev_attr_temp1_max_alarm.dev_attr.attr,
  	&sensor_dev_attr_temp1_crit_alarm.dev_attr.attr,
  	&sensor_dev_attr_temp1_crit_hyst.dev_attr.attr,
+
  	&sensor_dev_attr_temp2_min.dev_attr.attr,
  	&sensor_dev_attr_temp2_max.dev_attr.attr,
  	&sensor_dev_attr_temp2_crit.dev_attr.attr,

@@ -269,6 +292,7 @@ static struct attribute *emc1403_attrs[] = {
  	&sensor_dev_attr_temp2_max_alarm.dev_attr.attr,
  	&sensor_dev_attr_temp2_crit_alarm.dev_attr.attr,
  	&sensor_dev_attr_temp2_crit_hyst.dev_attr.attr,
+
  	&sensor_dev_attr_temp3_min.dev_attr.attr,
  	&sensor_dev_attr_temp3_max.dev_attr.attr,
  	&sensor_dev_attr_temp3_crit.dev_attr.attr,

drivers/hwmon/emc1403.c static struct attribute *emc1403_attrs[] = { · line 307

Quoted 7 lines

drivers/hwmon/emc1403.c +1 -0

@@ -277,6 +301,7 @@ static struct attribute *emc1403_attrs[] = {
  	&sensor_dev_attr_temp3_max_alarm.dev_attr.attr,
  	&sensor_dev_attr_temp3_crit_alarm.dev_attr.attr,
  	&sensor_dev_attr_temp3_crit_hyst.dev_attr.attr,
+
  	&sensor_dev_attr_power_state.dev_attr.attr,
  	NULL
  };

Modify this group to list the attributes only supported by EMC1403 and compatible chips (on top of the core or emc1412 group).

Quoted 21 lines

drivers/hwmon/emc1403.c +17 -0

@@ -286,6 +311,33 @@ static const struct attribute_group emc1403_group = {
  };
 
  static struct attribute *emc1404_attrs[] = {
+	&sensor_dev_attr_temp1_min.dev_attr.attr,
+	&sensor_dev_attr_temp1_max.dev_attr.attr,
+	&sensor_dev_attr_temp1_crit.dev_attr.attr,
+	&sensor_dev_attr_temp1_input.dev_attr.attr,
+	&sensor_dev_attr_temp1_min_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp1_max_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp1_crit_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp1_crit_hyst.dev_attr.attr,
+
+	&sensor_dev_attr_temp2_min.dev_attr.attr,
+	&sensor_dev_attr_temp2_max.dev_attr.attr,
+	&sensor_dev_attr_temp2_crit.dev_attr.attr,
+	&sensor_dev_attr_temp2_input.dev_attr.attr,
+	&sensor_dev_attr_temp2_min_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp2_max_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp2_crit_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp2_crit_hyst.dev_attr.attr,

drivers/hwmon/emc1403.c static const struct attribute_group emc1403_group = { · line 340

Quoted 10 lines

drivers/hwmon/emc1403.c +10 -0

+
+	&sensor_dev_attr_temp3_min.dev_attr.attr,
+	&sensor_dev_attr_temp3_max.dev_attr.attr,
+	&sensor_dev_attr_temp3_crit.dev_attr.attr,
+	&sensor_dev_attr_temp3_input.dev_attr.attr,
+	&sensor_dev_attr_temp3_min_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp3_max_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp3_crit_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp3_crit_hyst.dev_attr.attr,
+

This is exactly what you don't want to do; it is unnecessary duplication. There was a reason for having the second group on top of the first one, as explained above.

Quoted 13 lines

drivers/hwmon/emc1403.c +2 -0

  	&sensor_dev_attr_temp4_min.dev_attr.attr,
  	&sensor_dev_attr_temp4_max.dev_attr.attr,
  	&sensor_dev_attr_temp4_crit.dev_attr.attr,

@@ -294,6 +346,8 @@ static struct attribute *emc1404_attrs[] = {
  	&sensor_dev_attr_temp4_max_alarm.dev_attr.attr,
  	&sensor_dev_attr_temp4_crit_alarm.dev_attr.attr,
  	&sensor_dev_attr_temp4_crit_hyst.dev_attr.attr,
+
+	&sensor_dev_attr_power_state.dev_attr.attr,
  	NULL
  };

drivers/hwmon/emc1403.c static const struct attribute_group emc1404_group = { · line 359

Quoted 5 lines

drivers/hwmon/emc1403.c +2 -0

@@ -301,11 +355,13 @@ static const struct attribute_group emc1404_group = {
  	.attrs = emc1404_attrs,
  };
 
+
+

Unnecessary empty lines and unnecessary whitespace change.

drivers/hwmon/emc1403.c static const struct attribute_group emc1404_group = { · line 364

Quoted 6 lines

drivers/hwmon/emc1403.c +1 -1

  static int emc1403_detect(struct i2c_client *client,
  			struct i2c_board_info *info)
  {
  	int id;
-	/* Check if thermal chip is SMSC and EMC1403 or EMC1423 */
+	/* Check whether the chip is SMSC and get its type */

This change makes the comment pretty much worthless. It used to describe what the function is doing, which is no longer the case after your change.

Quoted 14 lines

drivers/hwmon/emc1403.c +3 -0

  	id = i2c_smbus_read_byte_data(client, THERMAL_SMSC_ID_REG);
  	if (id != 0x5d)

@@ -313,6 +369,9 @@ static int emc1403_detect(struct i2c_client *client,
 
  	id = i2c_smbus_read_byte_data(client, THERMAL_PID_REG);
  	switch (id) {
+	case 0x20:
+		strlcpy(info->type, "emc1412", I2C_NAME_SIZE);
+		break;
  	case 0x21:
  		strlcpy(info->type, "emc1403", I2C_NAME_SIZE);
  		break;

drivers/hwmon/emc1403.c static int emc1403_detect(struct i2c_client *client, · line 393

Quoted 6 lines

drivers/hwmon/emc1403.c +1 -1

@@ -330,9 +389,9 @@ static int emc1403_detect(struct i2c_client *client,
  	}
 
  	id = i2c_smbus_read_byte_data(client, THERMAL_REVISION_REG);
-	if (id != 0x01)
+	if (id != 0x01 && id != 0x04) {
  		return -ENODEV;

This should be a separate patch, as it applies to emc1403/emc1404 as well, so we can backport it into -stable.

Quoted 14 lines

drivers/hwmon/emc1403.c +3 -4

-
+	}
  	return 0;
  }

@@ -351,9 +410,17 @@ static int emc1403_probe(struct i2c_client *client,
  	mutex_init(&data->mutex);
  	data->hyst_valid = jiffies - 1;		/* Expired */
 
-	data->groups[0] = &emc1403_group;
-	if (id->driver_data)
-		data->groups[1] = &emc1404_group;
+	switch (id->driver_data) {
+	case 0:

drivers/hwmon/emc1403.c static int emc1403_probe(struct i2c_client *client, · line 424

Quoted 10 lines

drivers/hwmon/emc1403.c +9 -0

+		data->groups[0] = &emc1412_group;
+		break;
+	case 1:
+		data->groups[0] = &emc1403_group;
+		break;
+	case 2:
+		data->groups[0] = &emc1404_group;
+		break;
+	}

Not acceptable. Again, there is a reason for having multiple attribute groups.

drivers/hwmon/emc1403.c static int emc1403_probe(struct i2c_client *client, · line 439

Quoted 11 lines

drivers/hwmon/emc1403.c +4 -1

  	hwmon_dev = hwmon_device_register_with_groups(&client->dev,
  						      client->name, data,

@@ -366,14 +433,19 @@ static int emc1403_probe(struct i2c_client *client,
  }
 
  static const unsigned short emc1403_address_list[] = {
-	0x18, 0x29, 0x4c, 0x4d, I2C_CLIENT_END
+	/* emc1403/emc1404/emc1423/emc1424 */
+	0x4c, 0x4d, 0x18, 0x29,
+	/* emc1412 */
+	0x5c, 0x4c, 0x6c, 0x1c, 0x3c, I2C_CLIENT_END

No duplication of addresses, and addresses are by convention in order. Jean, any addresses which should not be scanned ?

drivers/hwmon/emc1403.c static int emc1403_probe(struct i2c_client *client, · line 448

Quoted 13 lines

drivers/hwmon/emc1403.c +6 -4

  };
 
+/* Last number in name indicates the amount of channels */
  static const struct i2c_device_id emc1403_idtable[] = {
-	{ "emc1403", 0 },
-	{ "emc1404", 1 },
-	{ "emc1423", 0 },
-	{ "emc1424", 1 },
+	{ "emc1412", 0 },
+	{ "emc1403", 1 },
+	{ "emc1423", 1 },
+	{ "emc1404", 2 },
+	{ "emc1424", 2 },

It may be time to introduce an enum for supported chip variants. Also, the list is by custom in alphabetical order.

Guenter

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Guenter Roeck

maintainer

2014-05-12 02:20 UTC #2 lore raw

On 05/11/2014 03:40 PM, Guenter Roeck wrote:
[ ... ]

Quoted 9 lines

drivers/hwmon/emc1403.c static int emc1403_detect(struct i2c_client *client, · line 393
Quoted 5 lines
      id = i2c_smbus_read_byte_data(client, THERMAL_REVISION_REG);
drivers/hwmon/emc1403.c +1 -1
-    if (id != 0x01)
+    if (id != 0x01 && id != 0x04) {
          return -ENODEV;
This should be a separate patch, as it applies to emc1403/emc1404 as well, so we can backport it into -stable.

Also, the chip datasheet suggests that chip revision 3 exists as well. Given that, I would suggest to replace the revision number check with something like

	if (id < 0x01 || id > 0x04)
		return -ENODEV;

Guenter

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Jean Delvare

2014-05-12 06:10 UTC #3 lore raw

In reply to Guenter Roeck

Hi Guenter, Josef,

Quoted 4 lines · Guenter Roeck

Quoted 3 lines · Josef Gajdusek
(quoted diff) +0 -0
@@ -366,14 +433,19 @@ static int emc1403_probe(struct i2c_client *client,
  }
 

Quoted 9 lines

drivers/hwmon/emc1403.c static int emc1403_probe(struct i2c_client *client, · line 439
Quoted 6 lines
drivers/hwmon/emc1403.c +4 -1
  static const unsigned short emc1403_address_list[] = {
-	0x18, 0x29, 0x4c, 0x4d, I2C_CLIENT_END
+	/* emc1403/emc1404/emc1423/emc1424 */
+	0x4c, 0x4d, 0x18, 0x29,
+	/* emc1412 */
+	0x5c, 0x4c, 0x6c, 0x1c, 0x3c, I2C_CLIENT_END
No duplication of addresses, and addresses are by convention in order. Jean, any addresses which should not be scanned ?

0x3c and 0x6c should indeed not be scanned, sensors-detect does not scan them as they aren't typically used by hwmon devices. 0x5c is questionable (currently scanned, but used only by a limited number of chips, we may drop it at some point.) 0x1c and 0x4c are OK to scan.

Signature · 7 lines

Jean Delvare
SUSE L3 Support

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Guenter Roeck

maintainer

2014-05-12 15:59 UTC #4 lore raw

Quoted 17 lines · Jean Delvare

Hi Guenter, Josef,
Quoted 4 lines · Guenter Roeck
Quoted 3 lines · Josef Gajdusek
(quoted diff) +0 -0
@@ -366,14 +433,19 @@ static int emc1403_probe(struct i2c_client *client,
  }
 
Quoted 9 lines
drivers/hwmon/emc1403.c static int emc1403_probe(struct i2c_client *client, · line 439
Quoted 6 lines
drivers/hwmon/emc1403.c +4 -1
  static const unsigned short emc1403_address_list[] = {
-	0x18, 0x29, 0x4c, 0x4d, I2C_CLIENT_END
+	/* emc1403/emc1404/emc1423/emc1424 */
+	0x4c, 0x4d, 0x18, 0x29,
+	/* emc1412 */
+	0x5c, 0x4c, 0x6c, 0x1c, 0x3c, I2C_CLIENT_END
No duplication of addresses, and addresses are by convention in order. Jean, any addresses which should not be scanned ?

Quoted 5 lines

0x3c and 0x6c should indeed not be scanned, sensors-detect does not scan them as they aren't typically used by hwmon devices. 0x5c is questionable (currently scanned, but used only by a limited number of chips, we may drop it at some point.) 0x1c and 0x4c are OK to scan.

Hi Jean,

I only see the adt7462 driver scanning for 0x5c. Guess I'll accept the address for now; I don't see a good reason not to.

Couple of other questions:
- would it make sense to relax store_hyst to not return ERANGE but use clamp_val instead ?
- Currently hyst can be stored for all crit attributes even though there is only one hyst register. Should we change this to only support writing it for temp1_crit_hyst ?
- I might convert the driver to use regmap if I find the time. Do you have any concerns with that ?

Thanks,
Guenter

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Jean Delvare

2014-05-12 16:48 UTC #5 lore raw

Hi Guenter,

Quoted 2 lines · Guenter Roeck

I only see the adt7462 driver scanning for 0x5c. Guess I'll accept the address for now; I don't see a good reason not to.

sensors-detect also scans it for the SMSC EMC1072, EMC1073 and EMC1074 which we don't support yet. I have no objection to scanning it.

Quoted 3 lines

Couple of other questions:
- would it make sense to relax store_hyst to not return ERANGE but use clamp_val instead ?

Yes, I had exactly the same thought when reading the code this morning.

Quoted 3 lines

- Currently hyst can be stored for all crit attributes even though there is only one hyst register. Should we change this to only support writing it for temp1_crit_hyst ?

Yes, that would align this driver with what other drivers do.

Quoted 2 lines

- I might convert the driver to use regmap if I find the time. Do you have any concerns with that ?

I know nothing about regmap, so no objection, I simply don't care ;-)

Signature · 7 lines

Jean Delvare
SUSE L3 Support

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Guenter Roeck

maintainer

2014-05-12 18:09 UTC #6 lore raw

Quoted 19 lines · Jean Delvare

Hi Guenter,
Quoted 2 lines · Guenter Roeck
I only see the adt7462 driver scanning for 0x5c. Guess I'll accept the address for now; I don't see a good reason not to.
sensors-detect also scans it for the SMSC EMC1072, EMC1073 and EMC1074 which we don't support yet. I have no objection to scanning it.
Quoted 3 lines
Couple of other questions:
- would it make sense to relax store_hyst to not return ERANGE but use clamp_val instead ?
Yes, I had exactly the same thought when reading the code this morning.
Quoted 3 lines
- Currently hyst can be stored for all crit attributes even though there is only one hyst register. Should we change this to only support writing it for temp1_crit_hyst ?

Quoted 7 lines

Yes, that would align this driver with what other drivers do.
Quoted 2 lines
- I might convert the driver to use regmap if I find the time. Do you have any concerns with that ?
I know nothing about regmap, so no objection, I simply don't care ;-)

Pretty much provides caching.

I'll submit patches for all of it, plus some more (add support for fault attributes and for the alarm attributes on the emc14x2 chips, and add driver documentation).

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Guenter Roeck

maintainer

2014-05-11 22:47 UTC #7 lore raw

In reply to Josef Gajdusek

Quoted 2 lines · Josef Gajdusek

Adds support for emc1412.

[ ... ]

drivers/hwmon/emc1403.c static int emc1403_detect(struct i2c_client *client, · line 394

Quoted 9 lines

drivers/hwmon/emc1403.c +2 -2

@@ -330,9 +389,9 @@ static int emc1403_detect(struct i2c_client *client,
  	}
 
  	id = i2c_smbus_read_byte_data(client, THERMAL_REVISION_REG);
-	if (id != 0x01)
+	if (id != 0x01 && id != 0x04) {
  		return -ENODEV;
-
+	}

Forgot: Please see Documents/CodingStyle, Chapter 3:

"Do not unnecessarily use braces where a single statement will do."

As it turns out, this change generates a checkpatch warning.

Also, you forgot to sign this patch.

Guenter

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors