[lm-sensors] [RFT][PATCH 1/2] hwmon: (adm9240) Avoid forward declaration

[lm-sensors] [RFT][PATCH 1/2] hwmon: (adm9240) Avoid forward declaration

[Axel Lin]

Reorder functions to avoid forward declaration.

Signed-off-by: Axel Lin <axel.lin@ingics.com>
---
 drivers/hwmon/adm9240.c | 315 +++++++++++++++++++++++-------------------------
 1 file changed, 153 insertions(+), 162 deletions(-)

diff --git a/drivers/hwmon/adm9240.c b/drivers/hwmon/adm9240.c
index 086d02a..8235ae4 100644
--- a/drivers/hwmon/adm9240.c
+++ b/drivers/hwmon/adm9240.c
@@ -130,35 +130,6 @@ static inline unsigned int AOUT_FROM_REG(u8 reg)
 	return SCALE(reg, 1250, 255);
 }
 
-static int adm9240_probe(struct i2c_client *client,
-			 const struct i2c_device_id *id);
-static int adm9240_detect(struct i2c_client *client,
-			  struct i2c_board_info *info);
-static void adm9240_init_client(struct i2c_client *client);
-static int adm9240_remove(struct i2c_client *client);
-static struct adm9240_data *adm9240_update_device(struct device *dev);
-
-/* driver data */
-static const struct i2c_device_id adm9240_id[] = {
-	{ "adm9240", adm9240 },
-	{ "ds1780", ds1780 },
-	{ "lm81", lm81 },
-	{ }
-};
-MODULE_DEVICE_TABLE(i2c, adm9240_id);
-
-static struct i2c_driver adm9240_driver = {
-	.class		= I2C_CLASS_HWMON,
-	.driver = {
-		.name	= "adm9240",
-	},
-	.probe		= adm9240_probe,
-	.remove		= adm9240_remove,
-	.id_table	= adm9240_id,
-	.detect		= adm9240_detect,
-	.address_list	= normal_i2c,
-};
-
 /* per client data */
 struct adm9240_data {
 	struct device *hwmon_dev;
@@ -181,6 +152,110 @@ struct adm9240_data {
 	u8 vrm;			/* --	vrm set on startup, no accessor */
 };
 
+/* write new fan div, callers must hold data->update_lock */
+static void adm9240_write_fan_div(struct i2c_client *client, int nr,
+		u8 fan_div)
+{
+	u8 reg, old, shift = (nr + 2) * 2;
+
+	reg = i2c_smbus_read_byte_data(client, ADM9240_REG_VID_FAN_DIV);
+	old = (reg >> shift) & 3;
+	reg &= ~(3 << shift);
+	reg |= (fan_div << shift);
+	i2c_smbus_write_byte_data(client, ADM9240_REG_VID_FAN_DIV, reg);
+	dev_dbg(&client->dev,
+		"fan%d clock divider changed from %u to %u\n",
+		nr + 1, 1 << old, 1 << fan_div);
+}
+
+static struct adm9240_data *adm9240_update_device(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct adm9240_data *data = i2c_get_clientdata(client);
+	int i;
+
+	mutex_lock(&data->update_lock);
+
+	/* minimum measurement cycle: 1.75 seconds */
+	if (time_after(jiffies, data->last_updated_measure + (HZ * 7 / 4))
+			|| !data->valid) {
+
+		for (i = 0; i < 6; i++) { /* read voltages */
+			data->in[i] = i2c_smbus_read_byte_data(client,
+					ADM9240_REG_IN(i));
+		}
+		data->alarms = i2c_smbus_read_byte_data(client,
+					ADM9240_REG_INT(0)) |
+					i2c_smbus_read_byte_data(client,
+					ADM9240_REG_INT(1)) << 8;
+
+		/*
+		 * read temperature: assume temperature changes less than
+		 * 0.5'C per two measurement cycles thus ignore possible
+		 * but unlikely aliasing error on lsb reading. --Grant
+		 */
+		data->temp = ((i2c_smbus_read_byte_data(client,
+					ADM9240_REG_TEMP) << 8) |
+					i2c_smbus_read_byte_data(client,
+					ADM9240_REG_TEMP_CONF)) / 128;
+
+		for (i = 0; i < 2; i++) { /* read fans */
+			data->fan[i] = i2c_smbus_read_byte_data(client,
+					ADM9240_REG_FAN(i));
+
+			/* adjust fan clock divider on overflow */
+			if (data->valid && data->fan[i] = 255 &&
+					data->fan_div[i] < 3) {
+
+				adm9240_write_fan_div(client, i,
+						++data->fan_div[i]);
+
+				/* adjust fan_min if active, but not to 0 */
+				if (data->fan_min[i] < 255 &&
+						data->fan_min[i] >= 2)
+					data->fan_min[i] /= 2;
+			}
+		}
+		data->last_updated_measure = jiffies;
+	}
+
+	/* minimum config reading cycle: 300 seconds */
+	if (time_after(jiffies, data->last_updated_config + (HZ * 300))
+			|| !data->valid) {
+
+		for (i = 0; i < 6; i++) {
+			data->in_min[i] = i2c_smbus_read_byte_data(client,
+					ADM9240_REG_IN_MIN(i));
+			data->in_max[i] = i2c_smbus_read_byte_data(client,
+					ADM9240_REG_IN_MAX(i));
+		}
+		for (i = 0; i < 2; i++) {
+			data->fan_min[i] = i2c_smbus_read_byte_data(client,
+					ADM9240_REG_FAN_MIN(i));
+		}
+		data->temp_max[0] = i2c_smbus_read_byte_data(client,
+				ADM9240_REG_TEMP_MAX(0));
+		data->temp_max[1] = i2c_smbus_read_byte_data(client,
+				ADM9240_REG_TEMP_MAX(1));
+
+		/* read fan divs and 5-bit VID */
+		i = i2c_smbus_read_byte_data(client, ADM9240_REG_VID_FAN_DIV);
+		data->fan_div[0] = (i >> 4) & 3;
+		data->fan_div[1] = (i >> 6) & 3;
+		data->vid = i & 0x0f;
+		data->vid |= (i2c_smbus_read_byte_data(client,
+					ADM9240_REG_VID4) & 1) << 4;
+		/* read analog out */
+		data->aout = i2c_smbus_read_byte_data(client,
+				ADM9240_REG_ANALOG_OUT);
+
+		data->last_updated_config = jiffies;
+		data->valid = 1;
+	}
+	mutex_unlock(&data->update_lock);
+	return data;
+}
+
 /*** sysfs accessors ***/
 
 /* temperature */
@@ -340,22 +415,6 @@ static ssize_t show_fan_div(struct device *dev,
 	return sprintf(buf, "%d\n", 1 << data->fan_div[attr->index]);
 }
 
-/* write new fan div, callers must hold data->update_lock */
-static void adm9240_write_fan_div(struct i2c_client *client, int nr,
-		u8 fan_div)
-{
-	u8 reg, old, shift = (nr + 2) * 2;
-
-	reg = i2c_smbus_read_byte_data(client, ADM9240_REG_VID_FAN_DIV);
-	old = (reg >> shift) & 3;
-	reg &= ~(3 << shift);
-	reg |= (fan_div << shift);
-	i2c_smbus_write_byte_data(client, ADM9240_REG_VID_FAN_DIV, reg);
-	dev_dbg(&client->dev,
-		"fan%d clock divider changed from %u to %u\n",
-		nr + 1, 1 << old, 1 << fan_div);
-}
-
 /*
  * set fan speed low limit:
  *
@@ -620,49 +679,6 @@ static int adm9240_detect(struct i2c_client *new_client,
 	return 0;
 }
 
-static int adm9240_probe(struct i2c_client *new_client,
-			 const struct i2c_device_id *id)
-{
-	struct adm9240_data *data;
-	int err;
-
-	data = devm_kzalloc(&new_client->dev, sizeof(*data), GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-
-	i2c_set_clientdata(new_client, data);
-	mutex_init(&data->update_lock);
-
-	adm9240_init_client(new_client);
-
-	/* populate sysfs filesystem */
-	err = sysfs_create_group(&new_client->dev.kobj, &adm9240_group);
-	if (err)
-		return err;
-
-	data->hwmon_dev = hwmon_device_register(&new_client->dev);
-	if (IS_ERR(data->hwmon_dev)) {
-		err = PTR_ERR(data->hwmon_dev);
-		goto exit_remove;
-	}
-
-	return 0;
-
-exit_remove:
-	sysfs_remove_group(&new_client->dev.kobj, &adm9240_group);
-	return err;
-}
-
-static int adm9240_remove(struct i2c_client *client)
-{
-	struct adm9240_data *data = i2c_get_clientdata(client);
-
-	hwmon_device_unregister(data->hwmon_dev);
-	sysfs_remove_group(&client->dev.kobj, &adm9240_group);
-
-	return 0;
-}
-
 static void adm9240_init_client(struct i2c_client *client)
 {
 	struct adm9240_data *data = i2c_get_clientdata(client);
@@ -705,93 +721,68 @@ static void adm9240_init_client(struct i2c_client *client)
 	}
 }
 
-static struct adm9240_data *adm9240_update_device(struct device *dev)
+static int adm9240_probe(struct i2c_client *new_client,
+			 const struct i2c_device_id *id)
 {
-	struct i2c_client *client = to_i2c_client(dev);
-	struct adm9240_data *data = i2c_get_clientdata(client);
-	int i;
+	struct adm9240_data *data;
+	int err;
 
-	mutex_lock(&data->update_lock);
+	data = devm_kzalloc(&new_client->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
 
-	/* minimum measurement cycle: 1.75 seconds */
-	if (time_after(jiffies, data->last_updated_measure + (HZ * 7 / 4))
-			|| !data->valid) {
+	i2c_set_clientdata(new_client, data);
+	mutex_init(&data->update_lock);
 
-		for (i = 0; i < 6; i++) { /* read voltages */
-			data->in[i] = i2c_smbus_read_byte_data(client,
-					ADM9240_REG_IN(i));
-		}
-		data->alarms = i2c_smbus_read_byte_data(client,
-					ADM9240_REG_INT(0)) |
-					i2c_smbus_read_byte_data(client,
-					ADM9240_REG_INT(1)) << 8;
+	adm9240_init_client(new_client);
 
-		/*
-		 * read temperature: assume temperature changes less than
-		 * 0.5'C per two measurement cycles thus ignore possible
-		 * but unlikely aliasing error on lsb reading. --Grant
-		 */
-		data->temp = ((i2c_smbus_read_byte_data(client,
-					ADM9240_REG_TEMP) << 8) |
-					i2c_smbus_read_byte_data(client,
-					ADM9240_REG_TEMP_CONF)) / 128;
+	/* populate sysfs filesystem */
+	err = sysfs_create_group(&new_client->dev.kobj, &adm9240_group);
+	if (err)
+		return err;
 
-		for (i = 0; i < 2; i++) { /* read fans */
-			data->fan[i] = i2c_smbus_read_byte_data(client,
-					ADM9240_REG_FAN(i));
+	data->hwmon_dev = hwmon_device_register(&new_client->dev);
+	if (IS_ERR(data->hwmon_dev)) {
+		err = PTR_ERR(data->hwmon_dev);
+		goto exit_remove;
+	}
 
-			/* adjust fan clock divider on overflow */
-			if (data->valid && data->fan[i] = 255 &&
-					data->fan_div[i] < 3) {
+	return 0;
 
-				adm9240_write_fan_div(client, i,
-						++data->fan_div[i]);
+exit_remove:
+	sysfs_remove_group(&new_client->dev.kobj, &adm9240_group);
+	return err;
+}
 
-				/* adjust fan_min if active, but not to 0 */
-				if (data->fan_min[i] < 255 &&
-						data->fan_min[i] >= 2)
-					data->fan_min[i] /= 2;
-			}
-		}
-		data->last_updated_measure = jiffies;
-	}
+static int adm9240_remove(struct i2c_client *client)
+{
+	struct adm9240_data *data = i2c_get_clientdata(client);
 
-	/* minimum config reading cycle: 300 seconds */
-	if (time_after(jiffies, data->last_updated_config + (HZ * 300))
-			|| !data->valid) {
+	hwmon_device_unregister(data->hwmon_dev);
+	sysfs_remove_group(&client->dev.kobj, &adm9240_group);
 
-		for (i = 0; i < 6; i++) {
-			data->in_min[i] = i2c_smbus_read_byte_data(client,
-					ADM9240_REG_IN_MIN(i));
-			data->in_max[i] = i2c_smbus_read_byte_data(client,
-					ADM9240_REG_IN_MAX(i));
-		}
-		for (i = 0; i < 2; i++) {
-			data->fan_min[i] = i2c_smbus_read_byte_data(client,
-					ADM9240_REG_FAN_MIN(i));
-		}
-		data->temp_max[0] = i2c_smbus_read_byte_data(client,
-				ADM9240_REG_TEMP_MAX(0));
-		data->temp_max[1] = i2c_smbus_read_byte_data(client,
-				ADM9240_REG_TEMP_MAX(1));
+	return 0;
+}
 
-		/* read fan divs and 5-bit VID */
-		i = i2c_smbus_read_byte_data(client, ADM9240_REG_VID_FAN_DIV);
-		data->fan_div[0] = (i >> 4) & 3;
-		data->fan_div[1] = (i >> 6) & 3;
-		data->vid = i & 0x0f;
-		data->vid |= (i2c_smbus_read_byte_data(client,
-					ADM9240_REG_VID4) & 1) << 4;
-		/* read analog out */
-		data->aout = i2c_smbus_read_byte_data(client,
-				ADM9240_REG_ANALOG_OUT);
+static const struct i2c_device_id adm9240_id[] = {
+	{ "adm9240", adm9240 },
+	{ "ds1780", ds1780 },
+	{ "lm81", lm81 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, adm9240_id);
 
-		data->last_updated_config = jiffies;
-		data->valid = 1;
-	}
-	mutex_unlock(&data->update_lock);
-	return data;
-}
+static struct i2c_driver adm9240_driver = {
+	.class		= I2C_CLASS_HWMON,
+	.driver = {
+		.name	= "adm9240",
+	},
+	.probe		= adm9240_probe,
+	.remove		= adm9240_remove,
+	.id_table	= adm9240_id,
+	.detect		= adm9240_detect,
+	.address_list	= normal_i2c,
+};
 
 module_i2c_driver(adm9240_driver);
 
-- 
1.9.1




_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFT][PATCH 1/2] hwmon: (adm9240) Avoid forward declaration

[Grant Coady]

Hi Axel, others,

Please note I no longer have the hardware to test this driver.

Thanks,
Grant.

On Thu, 03 Jul 2014 21:59:49 +0800, you wrote:

>Reorder functions to avoid forward declaration.
>
>Signed-off-by: Axel Lin <axel.lin@ingics.com>
>---
> drivers/hwmon/adm9240.c | 315 +++++++++++++++++++++++-------------------------
> 1 file changed, 153 insertions(+), 162 deletions(-)
>
>diff --git a/drivers/hwmon/adm9240.c b/drivers/hwmon/adm9240.c
>index 086d02a..8235ae4 100644
>--- a/drivers/hwmon/adm9240.c
>+++ b/drivers/hwmon/adm9240.c
>@@ -130,35 +130,6 @@ static inline unsigned int AOUT_FROM_REG(u8 reg)
> 	return SCALE(reg, 1250, 255);
> }
> 
>-static int adm9240_probe(struct i2c_client *client,
>-			 const struct i2c_device_id *id);
>-static int adm9240_detect(struct i2c_client *client,
>-			  struct i2c_board_info *info);
>-static void adm9240_init_client(struct i2c_client *client);
>-static int adm9240_remove(struct i2c_client *client);
>-static struct adm9240_data *adm9240_update_device(struct device *dev);
>-
>-/* driver data */
>-static const struct i2c_device_id adm9240_id[] = {
>-	{ "adm9240", adm9240 },
>-	{ "ds1780", ds1780 },
>-	{ "lm81", lm81 },
>-	{ }
>-};
>-MODULE_DEVICE_TABLE(i2c, adm9240_id);
>-
>-static struct i2c_driver adm9240_driver = {
>-	.class		= I2C_CLASS_HWMON,
>-	.driver = {
>-		.name	= "adm9240",
>-	},
>-	.probe		= adm9240_probe,
>-	.remove		= adm9240_remove,
>-	.id_table	= adm9240_id,
>-	.detect		= adm9240_detect,
>-	.address_list	= normal_i2c,
>-};
>-
> /* per client data */
> struct adm9240_data {
> 	struct device *hwmon_dev;
>@@ -181,6 +152,110 @@ struct adm9240_data {
> 	u8 vrm;			/* --	vrm set on startup, no accessor */
> };
> 
>+/* write new fan div, callers must hold data->update_lock */
>+static void adm9240_write_fan_div(struct i2c_client *client, int nr,
>+		u8 fan_div)
>+{
>+	u8 reg, old, shift = (nr + 2) * 2;
>+
>+	reg = i2c_smbus_read_byte_data(client, ADM9240_REG_VID_FAN_DIV);
>+	old = (reg >> shift) & 3;
>+	reg &= ~(3 << shift);
>+	reg |= (fan_div << shift);
>+	i2c_smbus_write_byte_data(client, ADM9240_REG_VID_FAN_DIV, reg);
>+	dev_dbg(&client->dev,
>+		"fan%d clock divider changed from %u to %u\n",
>+		nr + 1, 1 << old, 1 << fan_div);
>+}
>+
>+static struct adm9240_data *adm9240_update_device(struct device *dev)
>+{
>+	struct i2c_client *client = to_i2c_client(dev);
>+	struct adm9240_data *data = i2c_get_clientdata(client);
>+	int i;
>+
>+	mutex_lock(&data->update_lock);
>+
>+	/* minimum measurement cycle: 1.75 seconds */
>+	if (time_after(jiffies, data->last_updated_measure + (HZ * 7 / 4))
>+			|| !data->valid) {
>+
>+		for (i = 0; i < 6; i++) { /* read voltages */
>+			data->in[i] = i2c_smbus_read_byte_data(client,
>+					ADM9240_REG_IN(i));
>+		}
>+		data->alarms = i2c_smbus_read_byte_data(client,
>+					ADM9240_REG_INT(0)) |
>+					i2c_smbus_read_byte_data(client,
>+					ADM9240_REG_INT(1)) << 8;
>+
>+		/*
>+		 * read temperature: assume temperature changes less than
>+		 * 0.5'C per two measurement cycles thus ignore possible
>+		 * but unlikely aliasing error on lsb reading. --Grant
>+		 */
>+		data->temp = ((i2c_smbus_read_byte_data(client,
>+					ADM9240_REG_TEMP) << 8) |
>+					i2c_smbus_read_byte_data(client,
>+					ADM9240_REG_TEMP_CONF)) / 128;
>+
>+		for (i = 0; i < 2; i++) { /* read fans */
>+			data->fan[i] = i2c_smbus_read_byte_data(client,
>+					ADM9240_REG_FAN(i));
>+
>+			/* adjust fan clock divider on overflow */
>+			if (data->valid && data->fan[i] = 255 &&
>+					data->fan_div[i] < 3) {
>+
>+				adm9240_write_fan_div(client, i,
>+						++data->fan_div[i]);
>+
>+				/* adjust fan_min if active, but not to 0 */
>+				if (data->fan_min[i] < 255 &&
>+						data->fan_min[i] >= 2)
>+					data->fan_min[i] /= 2;
>+			}
>+		}
>+		data->last_updated_measure = jiffies;
>+	}
>+
>+	/* minimum config reading cycle: 300 seconds */
>+	if (time_after(jiffies, data->last_updated_config + (HZ * 300))
>+			|| !data->valid) {
>+
>+		for (i = 0; i < 6; i++) {
>+			data->in_min[i] = i2c_smbus_read_byte_data(client,
>+					ADM9240_REG_IN_MIN(i));
>+			data->in_max[i] = i2c_smbus_read_byte_data(client,
>+					ADM9240_REG_IN_MAX(i));
>+		}
>+		for (i = 0; i < 2; i++) {
>+			data->fan_min[i] = i2c_smbus_read_byte_data(client,
>+					ADM9240_REG_FAN_MIN(i));
>+		}
>+		data->temp_max[0] = i2c_smbus_read_byte_data(client,
>+				ADM9240_REG_TEMP_MAX(0));
>+		data->temp_max[1] = i2c_smbus_read_byte_data(client,
>+				ADM9240_REG_TEMP_MAX(1));
>+
>+		/* read fan divs and 5-bit VID */
>+		i = i2c_smbus_read_byte_data(client, ADM9240_REG_VID_FAN_DIV);
>+		data->fan_div[0] = (i >> 4) & 3;
>+		data->fan_div[1] = (i >> 6) & 3;
>+		data->vid = i & 0x0f;
>+		data->vid |= (i2c_smbus_read_byte_data(client,
>+					ADM9240_REG_VID4) & 1) << 4;
>+		/* read analog out */
>+		data->aout = i2c_smbus_read_byte_data(client,
>+				ADM9240_REG_ANALOG_OUT);
>+
>+		data->last_updated_config = jiffies;
>+		data->valid = 1;
>+	}
>+	mutex_unlock(&data->update_lock);
>+	return data;
>+}
>+
> /*** sysfs accessors ***/
> 
> /* temperature */
>@@ -340,22 +415,6 @@ static ssize_t show_fan_div(struct device *dev,
> 	return sprintf(buf, "%d\n", 1 << data->fan_div[attr->index]);
> }
> 
>-/* write new fan div, callers must hold data->update_lock */
>-static void adm9240_write_fan_div(struct i2c_client *client, int nr,
>-		u8 fan_div)
>-{
>-	u8 reg, old, shift = (nr + 2) * 2;
>-
>-	reg = i2c_smbus_read_byte_data(client, ADM9240_REG_VID_FAN_DIV);
>-	old = (reg >> shift) & 3;
>-	reg &= ~(3 << shift);
>-	reg |= (fan_div << shift);
>-	i2c_smbus_write_byte_data(client, ADM9240_REG_VID_FAN_DIV, reg);
>-	dev_dbg(&client->dev,
>-		"fan%d clock divider changed from %u to %u\n",
>-		nr + 1, 1 << old, 1 << fan_div);
>-}
>-
> /*
>  * set fan speed low limit:
>  *
>@@ -620,49 +679,6 @@ static int adm9240_detect(struct i2c_client *new_client,
> 	return 0;
> }
> 
>-static int adm9240_probe(struct i2c_client *new_client,
>-			 const struct i2c_device_id *id)
>-{
>-	struct adm9240_data *data;
>-	int err;
>-
>-	data = devm_kzalloc(&new_client->dev, sizeof(*data), GFP_KERNEL);
>-	if (!data)
>-		return -ENOMEM;
>-
>-	i2c_set_clientdata(new_client, data);
>-	mutex_init(&data->update_lock);
>-
>-	adm9240_init_client(new_client);
>-
>-	/* populate sysfs filesystem */
>-	err = sysfs_create_group(&new_client->dev.kobj, &adm9240_group);
>-	if (err)
>-		return err;
>-
>-	data->hwmon_dev = hwmon_device_register(&new_client->dev);
>-	if (IS_ERR(data->hwmon_dev)) {
>-		err = PTR_ERR(data->hwmon_dev);
>-		goto exit_remove;
>-	}
>-
>-	return 0;
>-
>-exit_remove:
>-	sysfs_remove_group(&new_client->dev.kobj, &adm9240_group);
>-	return err;
>-}
>-
>-static int adm9240_remove(struct i2c_client *client)
>-{
>-	struct adm9240_data *data = i2c_get_clientdata(client);
>-
>-	hwmon_device_unregister(data->hwmon_dev);
>-	sysfs_remove_group(&client->dev.kobj, &adm9240_group);
>-
>-	return 0;
>-}
>-
> static void adm9240_init_client(struct i2c_client *client)
> {
> 	struct adm9240_data *data = i2c_get_clientdata(client);
>@@ -705,93 +721,68 @@ static void adm9240_init_client(struct i2c_client *client)
> 	}
> }
> 
>-static struct adm9240_data *adm9240_update_device(struct device *dev)
>+static int adm9240_probe(struct i2c_client *new_client,
>+			 const struct i2c_device_id *id)
> {
>-	struct i2c_client *client = to_i2c_client(dev);
>-	struct adm9240_data *data = i2c_get_clientdata(client);
>-	int i;
>+	struct adm9240_data *data;
>+	int err;
> 
>-	mutex_lock(&data->update_lock);
>+	data = devm_kzalloc(&new_client->dev, sizeof(*data), GFP_KERNEL);
>+	if (!data)
>+		return -ENOMEM;
> 
>-	/* minimum measurement cycle: 1.75 seconds */
>-	if (time_after(jiffies, data->last_updated_measure + (HZ * 7 / 4))
>-			|| !data->valid) {
>+	i2c_set_clientdata(new_client, data);
>+	mutex_init(&data->update_lock);
> 
>-		for (i = 0; i < 6; i++) { /* read voltages */
>-			data->in[i] = i2c_smbus_read_byte_data(client,
>-					ADM9240_REG_IN(i));
>-		}
>-		data->alarms = i2c_smbus_read_byte_data(client,
>-					ADM9240_REG_INT(0)) |
>-					i2c_smbus_read_byte_data(client,
>-					ADM9240_REG_INT(1)) << 8;
>+	adm9240_init_client(new_client);
> 
>-		/*
>-		 * read temperature: assume temperature changes less than
>-		 * 0.5'C per two measurement cycles thus ignore possible
>-		 * but unlikely aliasing error on lsb reading. --Grant
>-		 */
>-		data->temp = ((i2c_smbus_read_byte_data(client,
>-					ADM9240_REG_TEMP) << 8) |
>-					i2c_smbus_read_byte_data(client,
>-					ADM9240_REG_TEMP_CONF)) / 128;
>+	/* populate sysfs filesystem */
>+	err = sysfs_create_group(&new_client->dev.kobj, &adm9240_group);
>+	if (err)
>+		return err;
> 
>-		for (i = 0; i < 2; i++) { /* read fans */
>-			data->fan[i] = i2c_smbus_read_byte_data(client,
>-					ADM9240_REG_FAN(i));
>+	data->hwmon_dev = hwmon_device_register(&new_client->dev);
>+	if (IS_ERR(data->hwmon_dev)) {
>+		err = PTR_ERR(data->hwmon_dev);
>+		goto exit_remove;
>+	}
> 
>-			/* adjust fan clock divider on overflow */
>-			if (data->valid && data->fan[i] = 255 &&
>-					data->fan_div[i] < 3) {
>+	return 0;
> 
>-				adm9240_write_fan_div(client, i,
>-						++data->fan_div[i]);
>+exit_remove:
>+	sysfs_remove_group(&new_client->dev.kobj, &adm9240_group);
>+	return err;
>+}
> 
>-				/* adjust fan_min if active, but not to 0 */
>-				if (data->fan_min[i] < 255 &&
>-						data->fan_min[i] >= 2)
>-					data->fan_min[i] /= 2;
>-			}
>-		}
>-		data->last_updated_measure = jiffies;
>-	}
>+static int adm9240_remove(struct i2c_client *client)
>+{
>+	struct adm9240_data *data = i2c_get_clientdata(client);
> 
>-	/* minimum config reading cycle: 300 seconds */
>-	if (time_after(jiffies, data->last_updated_config + (HZ * 300))
>-			|| !data->valid) {
>+	hwmon_device_unregister(data->hwmon_dev);
>+	sysfs_remove_group(&client->dev.kobj, &adm9240_group);
> 
>-		for (i = 0; i < 6; i++) {
>-			data->in_min[i] = i2c_smbus_read_byte_data(client,
>-					ADM9240_REG_IN_MIN(i));
>-			data->in_max[i] = i2c_smbus_read_byte_data(client,
>-					ADM9240_REG_IN_MAX(i));
>-		}
>-		for (i = 0; i < 2; i++) {
>-			data->fan_min[i] = i2c_smbus_read_byte_data(client,
>-					ADM9240_REG_FAN_MIN(i));
>-		}
>-		data->temp_max[0] = i2c_smbus_read_byte_data(client,
>-				ADM9240_REG_TEMP_MAX(0));
>-		data->temp_max[1] = i2c_smbus_read_byte_data(client,
>-				ADM9240_REG_TEMP_MAX(1));
>+	return 0;
>+}
> 
>-		/* read fan divs and 5-bit VID */
>-		i = i2c_smbus_read_byte_data(client, ADM9240_REG_VID_FAN_DIV);
>-		data->fan_div[0] = (i >> 4) & 3;
>-		data->fan_div[1] = (i >> 6) & 3;
>-		data->vid = i & 0x0f;
>-		data->vid |= (i2c_smbus_read_byte_data(client,
>-					ADM9240_REG_VID4) & 1) << 4;
>-		/* read analog out */
>-		data->aout = i2c_smbus_read_byte_data(client,
>-				ADM9240_REG_ANALOG_OUT);
>+static const struct i2c_device_id adm9240_id[] = {
>+	{ "adm9240", adm9240 },
>+	{ "ds1780", ds1780 },
>+	{ "lm81", lm81 },
>+	{ }
>+};
>+MODULE_DEVICE_TABLE(i2c, adm9240_id);
> 
>-		data->last_updated_config = jiffies;
>-		data->valid = 1;
>-	}
>-	mutex_unlock(&data->update_lock);
>-	return data;
>-}
>+static struct i2c_driver adm9240_driver = {
>+	.class		= I2C_CLASS_HWMON,
>+	.driver = {
>+		.name	= "adm9240",
>+	},
>+	.probe		= adm9240_probe,
>+	.remove		= adm9240_remove,
>+	.id_table	= adm9240_id,
>+	.detect		= adm9240_detect,
>+	.address_list	= normal_i2c,
>+};
> 
> module_i2c_driver(adm9240_driver);
> 

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFT][PATCH 1/2] hwmon: (adm9240) Avoid forward declaration

[Guenter Roeck]

On 07/03/2014 10:39 PM, Grant Coady wrote:
> Hi Axel, others,
>
> Please note I no longer have the hardware to test this driver.
>

Thanks to Jean we have a register dump and thus can do some module testing.

Not as good as real hardware, but I found a number of day-1 bugs in various
drivers this way, so it is not that bad.

Guenter

> Thanks,
> Grant.
>
> On Thu, 03 Jul 2014 21:59:49 +0800, you wrote:
>
>> Reorder functions to avoid forward declaration.
>>
>> Signed-off-by: Axel Lin <axel.lin@ingics.com>
>> ---
>> drivers/hwmon/adm9240.c | 315 +++++++++++++++++++++++-------------------------
>> 1 file changed, 153 insertions(+), 162 deletions(-)
>>
>> diff --git a/drivers/hwmon/adm9240.c b/drivers/hwmon/adm9240.c
>> index 086d02a..8235ae4 100644
>> --- a/drivers/hwmon/adm9240.c
>> +++ b/drivers/hwmon/adm9240.c
>> @@ -130,35 +130,6 @@ static inline unsigned int AOUT_FROM_REG(u8 reg)
>> 	return SCALE(reg, 1250, 255);
>> }
>>
>> -static int adm9240_probe(struct i2c_client *client,
>> -			 const struct i2c_device_id *id);
>> -static int adm9240_detect(struct i2c_client *client,
>> -			  struct i2c_board_info *info);
>> -static void adm9240_init_client(struct i2c_client *client);
>> -static int adm9240_remove(struct i2c_client *client);
>> -static struct adm9240_data *adm9240_update_device(struct device *dev);
>> -
>> -/* driver data */
>> -static const struct i2c_device_id adm9240_id[] = {
>> -	{ "adm9240", adm9240 },
>> -	{ "ds1780", ds1780 },
>> -	{ "lm81", lm81 },
>> -	{ }
>> -};
>> -MODULE_DEVICE_TABLE(i2c, adm9240_id);
>> -
>> -static struct i2c_driver adm9240_driver = {
>> -	.class		= I2C_CLASS_HWMON,
>> -	.driver = {
>> -		.name	= "adm9240",
>> -	},
>> -	.probe		= adm9240_probe,
>> -	.remove		= adm9240_remove,
>> -	.id_table	= adm9240_id,
>> -	.detect		= adm9240_detect,
>> -	.address_list	= normal_i2c,
>> -};
>> -
>> /* per client data */
>> struct adm9240_data {
>> 	struct device *hwmon_dev;
>> @@ -181,6 +152,110 @@ struct adm9240_data {
>> 	u8 vrm;			/* --	vrm set on startup, no accessor */
>> };
>>
>> +/* write new fan div, callers must hold data->update_lock */
>> +static void adm9240_write_fan_div(struct i2c_client *client, int nr,
>> +		u8 fan_div)
>> +{
>> +	u8 reg, old, shift = (nr + 2) * 2;
>> +
>> +	reg = i2c_smbus_read_byte_data(client, ADM9240_REG_VID_FAN_DIV);
>> +	old = (reg >> shift) & 3;
>> +	reg &= ~(3 << shift);
>> +	reg |= (fan_div << shift);
>> +	i2c_smbus_write_byte_data(client, ADM9240_REG_VID_FAN_DIV, reg);
>> +	dev_dbg(&client->dev,
>> +		"fan%d clock divider changed from %u to %u\n",
>> +		nr + 1, 1 << old, 1 << fan_div);
>> +}
>> +
>> +static struct adm9240_data *adm9240_update_device(struct device *dev)
>> +{
>> +	struct i2c_client *client = to_i2c_client(dev);
>> +	struct adm9240_data *data = i2c_get_clientdata(client);
>> +	int i;
>> +
>> +	mutex_lock(&data->update_lock);
>> +
>> +	/* minimum measurement cycle: 1.75 seconds */
>> +	if (time_after(jiffies, data->last_updated_measure + (HZ * 7 / 4))
>> +			|| !data->valid) {
>> +
>> +		for (i = 0; i < 6; i++) { /* read voltages */
>> +			data->in[i] = i2c_smbus_read_byte_data(client,
>> +					ADM9240_REG_IN(i));
>> +		}
>> +		data->alarms = i2c_smbus_read_byte_data(client,
>> +					ADM9240_REG_INT(0)) |
>> +					i2c_smbus_read_byte_data(client,
>> +					ADM9240_REG_INT(1)) << 8;
>> +
>> +		/*
>> +		 * read temperature: assume temperature changes less than
>> +		 * 0.5'C per two measurement cycles thus ignore possible
>> +		 * but unlikely aliasing error on lsb reading. --Grant
>> +		 */
>> +		data->temp = ((i2c_smbus_read_byte_data(client,
>> +					ADM9240_REG_TEMP) << 8) |
>> +					i2c_smbus_read_byte_data(client,
>> +					ADM9240_REG_TEMP_CONF)) / 128;
>> +
>> +		for (i = 0; i < 2; i++) { /* read fans */
>> +			data->fan[i] = i2c_smbus_read_byte_data(client,
>> +					ADM9240_REG_FAN(i));
>> +
>> +			/* adjust fan clock divider on overflow */
>> +			if (data->valid && data->fan[i] = 255 &&
>> +					data->fan_div[i] < 3) {
>> +
>> +				adm9240_write_fan_div(client, i,
>> +						++data->fan_div[i]);
>> +
>> +				/* adjust fan_min if active, but not to 0 */
>> +				if (data->fan_min[i] < 255 &&
>> +						data->fan_min[i] >= 2)
>> +					data->fan_min[i] /= 2;
>> +			}
>> +		}
>> +		data->last_updated_measure = jiffies;
>> +	}
>> +
>> +	/* minimum config reading cycle: 300 seconds */
>> +	if (time_after(jiffies, data->last_updated_config + (HZ * 300))
>> +			|| !data->valid) {
>> +
>> +		for (i = 0; i < 6; i++) {
>> +			data->in_min[i] = i2c_smbus_read_byte_data(client,
>> +					ADM9240_REG_IN_MIN(i));
>> +			data->in_max[i] = i2c_smbus_read_byte_data(client,
>> +					ADM9240_REG_IN_MAX(i));
>> +		}
>> +		for (i = 0; i < 2; i++) {
>> +			data->fan_min[i] = i2c_smbus_read_byte_data(client,
>> +					ADM9240_REG_FAN_MIN(i));
>> +		}
>> +		data->temp_max[0] = i2c_smbus_read_byte_data(client,
>> +				ADM9240_REG_TEMP_MAX(0));
>> +		data->temp_max[1] = i2c_smbus_read_byte_data(client,
>> +				ADM9240_REG_TEMP_MAX(1));
>> +
>> +		/* read fan divs and 5-bit VID */
>> +		i = i2c_smbus_read_byte_data(client, ADM9240_REG_VID_FAN_DIV);
>> +		data->fan_div[0] = (i >> 4) & 3;
>> +		data->fan_div[1] = (i >> 6) & 3;
>> +		data->vid = i & 0x0f;
>> +		data->vid |= (i2c_smbus_read_byte_data(client,
>> +					ADM9240_REG_VID4) & 1) << 4;
>> +		/* read analog out */
>> +		data->aout = i2c_smbus_read_byte_data(client,
>> +				ADM9240_REG_ANALOG_OUT);
>> +
>> +		data->last_updated_config = jiffies;
>> +		data->valid = 1;
>> +	}
>> +	mutex_unlock(&data->update_lock);
>> +	return data;
>> +}
>> +
>> /*** sysfs accessors ***/
>>
>> /* temperature */
>> @@ -340,22 +415,6 @@ static ssize_t show_fan_div(struct device *dev,
>> 	return sprintf(buf, "%d\n", 1 << data->fan_div[attr->index]);
>> }
>>
>> -/* write new fan div, callers must hold data->update_lock */
>> -static void adm9240_write_fan_div(struct i2c_client *client, int nr,
>> -		u8 fan_div)
>> -{
>> -	u8 reg, old, shift = (nr + 2) * 2;
>> -
>> -	reg = i2c_smbus_read_byte_data(client, ADM9240_REG_VID_FAN_DIV);
>> -	old = (reg >> shift) & 3;
>> -	reg &= ~(3 << shift);
>> -	reg |= (fan_div << shift);
>> -	i2c_smbus_write_byte_data(client, ADM9240_REG_VID_FAN_DIV, reg);
>> -	dev_dbg(&client->dev,
>> -		"fan%d clock divider changed from %u to %u\n",
>> -		nr + 1, 1 << old, 1 << fan_div);
>> -}
>> -
>> /*
>>   * set fan speed low limit:
>>   *
>> @@ -620,49 +679,6 @@ static int adm9240_detect(struct i2c_client *new_client,
>> 	return 0;
>> }
>>
>> -static int adm9240_probe(struct i2c_client *new_client,
>> -			 const struct i2c_device_id *id)
>> -{
>> -	struct adm9240_data *data;
>> -	int err;
>> -
>> -	data = devm_kzalloc(&new_client->dev, sizeof(*data), GFP_KERNEL);
>> -	if (!data)
>> -		return -ENOMEM;
>> -
>> -	i2c_set_clientdata(new_client, data);
>> -	mutex_init(&data->update_lock);
>> -
>> -	adm9240_init_client(new_client);
>> -
>> -	/* populate sysfs filesystem */
>> -	err = sysfs_create_group(&new_client->dev.kobj, &adm9240_group);
>> -	if (err)
>> -		return err;
>> -
>> -	data->hwmon_dev = hwmon_device_register(&new_client->dev);
>> -	if (IS_ERR(data->hwmon_dev)) {
>> -		err = PTR_ERR(data->hwmon_dev);
>> -		goto exit_remove;
>> -	}
>> -
>> -	return 0;
>> -
>> -exit_remove:
>> -	sysfs_remove_group(&new_client->dev.kobj, &adm9240_group);
>> -	return err;
>> -}
>> -
>> -static int adm9240_remove(struct i2c_client *client)
>> -{
>> -	struct adm9240_data *data = i2c_get_clientdata(client);
>> -
>> -	hwmon_device_unregister(data->hwmon_dev);
>> -	sysfs_remove_group(&client->dev.kobj, &adm9240_group);
>> -
>> -	return 0;
>> -}
>> -
>> static void adm9240_init_client(struct i2c_client *client)
>> {
>> 	struct adm9240_data *data = i2c_get_clientdata(client);
>> @@ -705,93 +721,68 @@ static void adm9240_init_client(struct i2c_client *client)
>> 	}
>> }
>>
>> -static struct adm9240_data *adm9240_update_device(struct device *dev)
>> +static int adm9240_probe(struct i2c_client *new_client,
>> +			 const struct i2c_device_id *id)
>> {
>> -	struct i2c_client *client = to_i2c_client(dev);
>> -	struct adm9240_data *data = i2c_get_clientdata(client);
>> -	int i;
>> +	struct adm9240_data *data;
>> +	int err;
>>
>> -	mutex_lock(&data->update_lock);
>> +	data = devm_kzalloc(&new_client->dev, sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>>
>> -	/* minimum measurement cycle: 1.75 seconds */
>> -	if (time_after(jiffies, data->last_updated_measure + (HZ * 7 / 4))
>> -			|| !data->valid) {
>> +	i2c_set_clientdata(new_client, data);
>> +	mutex_init(&data->update_lock);
>>
>> -		for (i = 0; i < 6; i++) { /* read voltages */
>> -			data->in[i] = i2c_smbus_read_byte_data(client,
>> -					ADM9240_REG_IN(i));
>> -		}
>> -		data->alarms = i2c_smbus_read_byte_data(client,
>> -					ADM9240_REG_INT(0)) |
>> -					i2c_smbus_read_byte_data(client,
>> -					ADM9240_REG_INT(1)) << 8;
>> +	adm9240_init_client(new_client);
>>
>> -		/*
>> -		 * read temperature: assume temperature changes less than
>> -		 * 0.5'C per two measurement cycles thus ignore possible
>> -		 * but unlikely aliasing error on lsb reading. --Grant
>> -		 */
>> -		data->temp = ((i2c_smbus_read_byte_data(client,
>> -					ADM9240_REG_TEMP) << 8) |
>> -					i2c_smbus_read_byte_data(client,
>> -					ADM9240_REG_TEMP_CONF)) / 128;
>> +	/* populate sysfs filesystem */
>> +	err = sysfs_create_group(&new_client->dev.kobj, &adm9240_group);
>> +	if (err)
>> +		return err;
>>
>> -		for (i = 0; i < 2; i++) { /* read fans */
>> -			data->fan[i] = i2c_smbus_read_byte_data(client,
>> -					ADM9240_REG_FAN(i));
>> +	data->hwmon_dev = hwmon_device_register(&new_client->dev);
>> +	if (IS_ERR(data->hwmon_dev)) {
>> +		err = PTR_ERR(data->hwmon_dev);
>> +		goto exit_remove;
>> +	}
>>
>> -			/* adjust fan clock divider on overflow */
>> -			if (data->valid && data->fan[i] = 255 &&
>> -					data->fan_div[i] < 3) {
>> +	return 0;
>>
>> -				adm9240_write_fan_div(client, i,
>> -						++data->fan_div[i]);
>> +exit_remove:
>> +	sysfs_remove_group(&new_client->dev.kobj, &adm9240_group);
>> +	return err;
>> +}
>>
>> -				/* adjust fan_min if active, but not to 0 */
>> -				if (data->fan_min[i] < 255 &&
>> -						data->fan_min[i] >= 2)
>> -					data->fan_min[i] /= 2;
>> -			}
>> -		}
>> -		data->last_updated_measure = jiffies;
>> -	}
>> +static int adm9240_remove(struct i2c_client *client)
>> +{
>> +	struct adm9240_data *data = i2c_get_clientdata(client);
>>
>> -	/* minimum config reading cycle: 300 seconds */
>> -	if (time_after(jiffies, data->last_updated_config + (HZ * 300))
>> -			|| !data->valid) {
>> +	hwmon_device_unregister(data->hwmon_dev);
>> +	sysfs_remove_group(&client->dev.kobj, &adm9240_group);
>>
>> -		for (i = 0; i < 6; i++) {
>> -			data->in_min[i] = i2c_smbus_read_byte_data(client,
>> -					ADM9240_REG_IN_MIN(i));
>> -			data->in_max[i] = i2c_smbus_read_byte_data(client,
>> -					ADM9240_REG_IN_MAX(i));
>> -		}
>> -		for (i = 0; i < 2; i++) {
>> -			data->fan_min[i] = i2c_smbus_read_byte_data(client,
>> -					ADM9240_REG_FAN_MIN(i));
>> -		}
>> -		data->temp_max[0] = i2c_smbus_read_byte_data(client,
>> -				ADM9240_REG_TEMP_MAX(0));
>> -		data->temp_max[1] = i2c_smbus_read_byte_data(client,
>> -				ADM9240_REG_TEMP_MAX(1));
>> +	return 0;
>> +}
>>
>> -		/* read fan divs and 5-bit VID */
>> -		i = i2c_smbus_read_byte_data(client, ADM9240_REG_VID_FAN_DIV);
>> -		data->fan_div[0] = (i >> 4) & 3;
>> -		data->fan_div[1] = (i >> 6) & 3;
>> -		data->vid = i & 0x0f;
>> -		data->vid |= (i2c_smbus_read_byte_data(client,
>> -					ADM9240_REG_VID4) & 1) << 4;
>> -		/* read analog out */
>> -		data->aout = i2c_smbus_read_byte_data(client,
>> -				ADM9240_REG_ANALOG_OUT);
>> +static const struct i2c_device_id adm9240_id[] = {
>> +	{ "adm9240", adm9240 },
>> +	{ "ds1780", ds1780 },
>> +	{ "lm81", lm81 },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, adm9240_id);
>>
>> -		data->last_updated_config = jiffies;
>> -		data->valid = 1;
>> -	}
>> -	mutex_unlock(&data->update_lock);
>> -	return data;
>> -}
>> +static struct i2c_driver adm9240_driver = {
>> +	.class		= I2C_CLASS_HWMON,
>> +	.driver = {
>> +		.name	= "adm9240",
>> +	},
>> +	.probe		= adm9240_probe,
>> +	.remove		= adm9240_remove,
>> +	.id_table	= adm9240_id,
>> +	.detect		= adm9240_detect,
>> +	.address_list	= normal_i2c,
>> +};
>>
>> module_i2c_driver(adm9240_driver);
>>
>
>


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFT][PATCH 1/2] hwmon: (adm9240) Avoid forward declaration

[Jean Delvare]

On Fri, 04 Jul 2014 00:36:10 -0700, Guenter Roeck wrote:
> On 07/03/2014 10:39 PM, Grant Coady wrote:
> > Hi Axel, others,
> >
> > Please note I no longer have the hardware to test this driver.
> 
> Thanks to Jean we have a register dump and thus can do some module testing.

Thanks to Grant actually, as I'm pretty sure I got that dump from him
in the first place ;-)

> Not as good as real hardware, but I found a number of day-1 bugs in various
> drivers this way, so it is not that bad.

Note that support for banked registers could be added if you think this
would help.

-- 
Jean Delvare
SUSE L3 Support

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFT][PATCH 1/2] hwmon: (adm9240) Avoid forward declaration

[Guenter Roeck]

On 07/03/2014 06:59 AM, Axel Lin wrote:
> Reorder functions to avoid forward declaration.
>
> Signed-off-by: Axel Lin <axel.lin@ingics.com>

Applied to -next.

Thanks,
Guenter



_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFT][PATCH 1/2] hwmon: (adm9240) Avoid forward declaration

[Guenter Roeck]

On 07/04/2014 05:23 AM, Jean Delvare wrote:
> On Fri, 04 Jul 2014 00:36:10 -0700, Guenter Roeck wrote:
>> On 07/03/2014 10:39 PM, Grant Coady wrote:
>>> Hi Axel, others,
>>>
>>> Please note I no longer have the hardware to test this driver.
>>
>> Thanks to Jean we have a register dump and thus can do some module testing.
>
> Thanks to Grant actually, as I'm pretty sure I got that dump from him
> in the first place ;-)
>
>> Not as good as real hardware, but I found a number of day-1 bugs in various
>> drivers this way, so it is not that bad.
>
> Note that support for banked registers could be added if you think this
> would help.
>

Hi Jean,

That would be quite useful. How would you implement it, though ? You would have
to have a means to tell the driver "this is a bank register". Another module
parameter, maybe ?

Thanks,
Guenter


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFT][PATCH 1/2] hwmon: (adm9240) Avoid forward declaration

[Jean Delvare]

Hi Guenter,

On Fri, 04 Jul 2014 10:10:14 -0700, Guenter Roeck wrote:
> On 07/04/2014 05:23 AM, Jean Delvare wrote:
> > Note that support for banked registers could be added if you think this
> > would help.
> 
> That would be quite useful. How would you implement it, though ? You would have
> to have a means to tell the driver "this is a bank register". Another module
> parameter, maybe ?

Not just "this is a bank register". You also most specify which bits
set the bank number (which in turn define the maximum number of banks),
and the range of banked registers.

I did not think about it in depth yet, but there are two options that
come to my mind: module parameters indeed, or sysfs attributes. We
could create a sysfs directory for every client address and have
writable attributes bank_reg, bank_mask, banked_start_reg and
banked_end_reg in that directory. Not sure exactly where actually,
maybe we would need to introduce a virtual i2c-stub device, or use
debugfs.

Module parameters would work too, that's probably easier to implement
that way, but that's also less flexible, as you have to setup
everything upon module loading. That might be just fine though, as
being able to change the bank setup at run-time has little practical
value IMHO.

-- 
Jean Delvare
SUSE L3 Support

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFT][PATCH 1/2] hwmon: (adm9240) Avoid forward declaration

[Guenter Roeck]

On 07/04/2014 01:20 PM, Jean Delvare wrote:
> Hi Guenter,
>
> On Fri, 04 Jul 2014 10:10:14 -0700, Guenter Roeck wrote:
>> On 07/04/2014 05:23 AM, Jean Delvare wrote:
>>> Note that support for banked registers could be added if you think this
>>> would help.
>>
>> That would be quite useful. How would you implement it, though ? You would have
>> to have a means to tell the driver "this is a bank register". Another module
>> parameter, maybe ?
>
> Not just "this is a bank register". You also most specify which bits
> set the bank number (which in turn define the maximum number of banks),
> and the range of banked registers.
>
> I did not think about it in depth yet, but there are two options that
> come to my mind: module parameters indeed, or sysfs attributes. We
> could create a sysfs directory for every client address and have
> writable attributes bank_reg, bank_mask, banked_start_reg and
> banked_end_reg in that directory. Not sure exactly where actually,
> maybe we would need to introduce a virtual i2c-stub device, or use
> debugfs.
>
> Module parameters would work too, that's probably easier to implement
> that way, but that's also less flexible, as you have to setup
> everything upon module loading. That might be just fine though, as
> being able to change the bank setup at run-time has little practical
> value IMHO.
>

Hi Jean,

At least with my module test scripts, I re-load  the i2c-stub driver
before testing a new chip, to make sure that its state is 'clean'.
So module parameters work just fine for me.

Could we use a single module parameter ?

bank_reg=<reg> <mask>

would probably do for a start, and mask could even be optional. Not sure
if banked_start_reg and banked_end_reg are necessary. If so the above
could be extended to

bank_reg=<reg> <mask> <start> <end>

That would not work for PMBus chips - those are so complex that
it is unlikely we'll ever be able to simulate one without much
more complexity - it would require the ability to return a failure
when trying to access unsupported registers/commands, and that
on each of the pages.

Guenter


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFT][PATCH 1/2] hwmon: (adm9240) Avoid forward declaration

[Jean Delvare]

Hi Guenter,

On Fri, 04 Jul 2014 18:01:25 -0700, Guenter Roeck wrote:
> On 07/04/2014 01:20 PM, Jean Delvare wrote:
> > Not just "this is a bank register". You also most specify which bits
> > set the bank number (which in turn define the maximum number of banks),
> > and the range of banked registers.
> >
> > I did not think about it in depth yet, but there are two options that
> > come to my mind: module parameters indeed, or sysfs attributes. We
> > could create a sysfs directory for every client address and have
> > writable attributes bank_reg, bank_mask, banked_start_reg and
> > banked_end_reg in that directory. Not sure exactly where actually,
> > maybe we would need to introduce a virtual i2c-stub device, or use
> > debugfs.
> >
> > Module parameters would work too, that's probably easier to implement
> > that way, but that's also less flexible, as you have to setup
> > everything upon module loading. That might be just fine though, as
> > being able to change the bank setup at run-time has little practical
> > value IMHO.
> 
> At least with my module test scripts, I re-load the i2c-stub driver
> before testing a new chip, to make sure that its state is 'clean'.
> So module parameters work just fine for me.

I started working on the idea using module parameters. A lot more work
is needed though, and I have a lot of other things to do today.

> Could we use a single module parameter ?
> 
> bank_reg=<reg> <mask>

What would be the benefit? Using parameters with single integer values
is simple. Using space-separated lists would require us to parse the
parameters ourselves.

> would probably do for a start, and mask could even be optional.

I don't think we can make the mask optional. For one thing, that would
force us to allocate memory for all 256 possible banks, which would be
a waste of memory. For another, it would make a single bank appear as
separate ones, so changes to one of the bank "copies" would not reflect
in the other "copies". Winbond for example uses the MSB of the register
bank to select the low or high byte of the vendor ID.

> Not sure
> if banked_start_reg and banked_end_reg are necessary. If so the above
> could be extended to
> 
> bank_reg=<reg> <mask> <start> <end>

I think we have to, for mostly the same reason we need to limit the
number of banks: non-banked registers are shared between banks. If we
don't specify the range of banked registers, changes done to shared
registers won't reflect between banks, and that will confuse the
drivers.

One thing I'm not yet sure of is if we want to handle banks on more
than one chip at a time. I started coding that way but it might be
somewhat overkill actually. I guess you won't need that for your
testing.

> That would not work for PMBus chips - those are so complex that
> it is unlikely we'll ever be able to simulate one without much
> more complexity - it would require the ability to return a failure
> when trying to access unsupported registers/commands, and that
> on each of the pages.

PMBus chips are a breed on their own. I don't think the generic
i2c-stub driver will ever be enough for these. You'd need a dedicated
emulator for them.

-- 
Jean Delvare
SUSE L3 Support

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFT][PATCH 1/2] hwmon: (adm9240) Avoid forward declaration

[Guenter Roeck]

On 07/05/2014 01:53 AM, Jean Delvare wrote:
> Hi Guenter,
>
> On Fri, 04 Jul 2014 18:01:25 -0700, Guenter Roeck wrote:
>> On 07/04/2014 01:20 PM, Jean Delvare wrote:
>>> Not just "this is a bank register". You also most specify which bits
>>> set the bank number (which in turn define the maximum number of banks),
>>> and the range of banked registers.
>>>
>>> I did not think about it in depth yet, but there are two options that
>>> come to my mind: module parameters indeed, or sysfs attributes. We
>>> could create a sysfs directory for every client address and have
>>> writable attributes bank_reg, bank_mask, banked_start_reg and
>>> banked_end_reg in that directory. Not sure exactly where actually,
>>> maybe we would need to introduce a virtual i2c-stub device, or use
>>> debugfs.
>>>
>>> Module parameters would work too, that's probably easier to implement
>>> that way, but that's also less flexible, as you have to setup
>>> everything upon module loading. That might be just fine though, as
>>> being able to change the bank setup at run-time has little practical
>>> value IMHO.
>>
>> At least with my module test scripts, I re-load the i2c-stub driver
>> before testing a new chip, to make sure that its state is 'clean'.
>> So module parameters work just fine for me.
>
> I started working on the idea using module parameters. A lot more work
> is needed though, and I have a lot of other things to do today.
>
>> Could we use a single module parameter ?
>>
>> bank_reg=<reg> <mask>
>
> What would be the benefit? Using parameters with single integer values
> is simple. Using space-separated lists would require us to parse the
> parameters ourselves.
>
Sorry, I meant using a parameter array.

>> would probably do for a start, and mask could even be optional.
>
> I don't think we can make the mask optional. For one thing, that would
> force us to allocate memory for all 256 possible banks, which would be
> a waste of memory. For another, it would make a single bank appear as
> separate ones, so changes to one of the bank "copies" would not reflect
> in the other "copies". Winbond for example uses the MSB of the register
> bank to select the low or high byte of the vendor ID.
>
>> Not sure
>> if banked_start_reg and banked_end_reg are necessary. If so the above
>> could be extended to
>>
>> bank_reg=<reg> <mask> <start> <end>
>
> I think we have to, for mostly the same reason we need to limit the
> number of banks: non-banked registers are shared between banks. If we
> don't specify the range of banked registers, changes done to shared
> registers won't reflect between banks, and that will confuse the
> drivers.
>
Ok, makes sense.

> One thing I'm not yet sure of is if we want to handle banks on more
> than one chip at a time. I started coding that way but it might be
> somewhat overkill actually. I guess you won't need that for your
> testing.
>
No, I would consider that overkill.

>> That would not work for PMBus chips - those are so complex that
>> it is unlikely we'll ever be able to simulate one without much
>> more complexity - it would require the ability to return a failure
>> when trying to access unsupported registers/commands, and that
>> on each of the pages.
>
> PMBus chips are a breed on their own. I don't think the generic
> i2c-stub driver will ever be enough for these. You'd need a dedicated
> emulator for them.
>
Agreed.

Something else though that would help: SMBus block commands.
Any idea why this is not currently supported ? I see that
I2C_SMBUS_I2C_BLOCK_DATA is supported, so I2C_SMBUS_BLOCK_DATA
should not be hard. Am I missing something ?

Thanks,
Guenter


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFT][PATCH 1/2] hwmon: (adm9240) Avoid forward declaration

[Guenter Roeck]

On 07/05/2014 06:39 AM, Guenter Roeck wrote:
>
> Something else though that would help: SMBus block commands.
> Any idea why this is not currently supported ? I see that
> I2C_SMBUS_I2C_BLOCK_DATA is supported, so I2C_SMBUS_BLOCK_DATA
> should not be hard. Am I missing something ?
>
Hi Jean,

I took a stab at that. Guess the main difference to
I2C_SMBUS_I2C_BLOCK_DATA is that it needs a separate buffer
for the SMBus data block; the 'word' buffer can not be used.
Turns out that was quite straightforward to implement.

Guenter


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFT][PATCH 1/2] hwmon: (adm9240) Avoid forward declaration

[Jean Delvare]

Hi Guenter,

On Sat, 05 Jul 2014 07:43:45 -0700, Guenter Roeck wrote:
> On 07/05/2014 06:39 AM, Guenter Roeck wrote:
> > Something else though that would help: SMBus block commands.
> > Any idea why this is not currently supported ? I see that
> > I2C_SMBUS_I2C_BLOCK_DATA is supported, so I2C_SMBUS_BLOCK_DATA
> > should not be hard. Am I missing something ?
> 
> I took a stab at that. Guess the main difference to
> I2C_SMBUS_I2C_BLOCK_DATA is that it needs a separate buffer
> for the SMBus data block; the 'word' buffer can not be used.
> Turns out that was quite straightforward to implement.

The main problem I see is that for I2C_SMBUS_BLOCK_DATA reads, the chip
first returns the number of data bytes (as opposed to
I2C_SMBUS_I2C_BLOCK_DATA where the controller decides how many bytes it
wants to read.) There is no way the i2c-stub driver can guess that byte
count, as it depends on the chip it is supposed to emulate (and might
even change dynamically, at least in theory.)

We could have limited support for that, but that would require extra
module parameters to specify the block size for every register offset
on which SMBus block reads can be attempted. This also assumes that these
block sizes are static. And as you found out, that may also require
allocating extra memory for every such register offset.

But another difficulty is also that when SMBus block reads enter the
game, the usual read/write symmetry tends to disappear. Often the
registers you read with SMBus block read commands are also readable and
writable at individual register addresses. i2c-stub has no way to know
that. Drivers would typically use SMBus block reads for performance
reason, but byte writes for convenience. So drivers operating on top of
i2c-stub would get confused in no time.

All these issues have so far convinced me that adding support for SMBus
block read/writes to i2c-stub wasn't worth it. That being said, if you
have a specific chip in mind that could be supported easily, I have no
objection.

-- 
Jean Delvare
SUSE L3 Support

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFT][PATCH 1/2] hwmon: (adm9240) Avoid forward declaration

[Guenter Roeck]

Hi Jean,

On 07/05/2014 11:20 AM, Jean Delvare wrote:
> Hi Guenter,
>
> On Sat, 05 Jul 2014 07:43:45 -0700, Guenter Roeck wrote:
>> On 07/05/2014 06:39 AM, Guenter Roeck wrote:
>>> Something else though that would help: SMBus block commands.
>>> Any idea why this is not currently supported ? I see that
>>> I2C_SMBUS_I2C_BLOCK_DATA is supported, so I2C_SMBUS_BLOCK_DATA
>>> should not be hard. Am I missing something ?
>>
>> I took a stab at that. Guess the main difference to
>> I2C_SMBUS_I2C_BLOCK_DATA is that it needs a separate buffer
>> for the SMBus data block; the 'word' buffer can not be used.
>> Turns out that was quite straightforward to implement.
>
> The main problem I see is that for I2C_SMBUS_BLOCK_DATA reads, the chip
> first returns the number of data bytes (as opposed to
> I2C_SMBUS_I2C_BLOCK_DATA where the controller decides how many bytes it
> wants to read.) There is no way the i2c-stub driver can guess that byte
> count, as it depends on the chip it is supposed to emulate (and might
> even change dynamically, at least in theory.)
>
> We could have limited support for that, but that would require extra
> module parameters to specify the block size for every register offset
> on which SMBus block reads can be attempted. This also assumes that these
> block sizes are static. And as you found out, that may also require
> allocating extra memory for every such register offset.
>
I 'solved' the problem so far by only returning smbus block data after
it was written into a specific command. This way it is all dynamic.

> But another difficulty is also that when SMBus block reads enter the
> game, the usual read/write symmetry tends to disappear. Often the
> registers you read with SMBus block read commands are also readable and
> writable at individual register addresses. i2c-stub has no way to know
> that. Drivers would typically use SMBus block reads for performance
> reason, but byte writes for convenience. So drivers operating on top of
> i2c-stub would get confused in no time.
>
> All these issues have so far convinced me that adding support for SMBus
> block read/writes to i2c-stub wasn't worth it. That being said, if you
> have a specific chip in mind that could be supported easily, I have no
> objection.
>
The one I needed it for right now was lineage-pem; it was useful for me
since I don't have easy access to the HW anymore. Of course, problem with
that is what you pointed out ... with my change in the i2c-stub driver,
the lm93 driver now assumes that it can execute block commands. The only
way I see around that would be a module parameter. That would have to be
one which selects if the smbus block command should be treated similar
to the i2c block command or as individual command blocks. I'll play around
with it some more. I'll send a patch if I find a solution which works for
both lm93 and lineage-pem and is not too complicated.

Guenter


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFT][PATCH 1/2] hwmon: (adm9240) Avoid forward declaration

[Jean Delvare]

Hi Guenter,

On Sat, 05 Jul 2014 11:48:55 -0700, Guenter Roeck wrote:
> On 07/05/2014 11:20 AM, Jean Delvare wrote:
> > The main problem I see is that for I2C_SMBUS_BLOCK_DATA reads, the chip
> > first returns the number of data bytes (as opposed to
> > I2C_SMBUS_I2C_BLOCK_DATA where the controller decides how many bytes it
> > wants to read.) There is no way the i2c-stub driver can guess that byte
> > count, as it depends on the chip it is supposed to emulate (and might
> > even change dynamically, at least in theory.)
> >
> > We could have limited support for that, but that would require extra
> > module parameters to specify the block size for every register offset
> > on which SMBus block reads can be attempted. This also assumes that these
> > block sizes are static. And as you found out, that may also require
> > allocating extra memory for every such register offset.
>
> I 'solved' the problem so far by only returning smbus block data after
> it was written into a specific command. This way it is all dynamic.

This is smart :-) But this assumes that block size is the same the same
in both directions for a given register offset. This also assumes that
block sizes do not change over time. But anyway, i2c-stub is bound to
have such limitations, it can only emulate simple devices.

> > But another difficulty is also that when SMBus block reads enter the
> > game, the usual read/write symmetry tends to disappear. Often the
> > registers you read with SMBus block read commands are also readable and
> > writable at individual register addresses. i2c-stub has no way to know
> > that. Drivers would typically use SMBus block reads for performance
> > reason, but byte writes for convenience. So drivers operating on top of
> > i2c-stub would get confused in no time.
> >
> > All these issues have so far convinced me that adding support for SMBus
> > block read/writes to i2c-stub wasn't worth it. That being said, if you
> > have a specific chip in mind that could be supported easily, I have no
> > objection.
>
> The one I needed it for right now was lineage-pem; it was useful for me
> since I don't have easy access to the HW anymore. Of course, problem with
> that is what you pointed out ... with my change in the i2c-stub driver,
> the lm93 driver now assumes that it can execute block commands. The only
> way I see around that would be a module parameter.

Actually there already is one, named "functionality". For now it can
only be used to disable commands, because all supported commands were
enabled by default. It would be trivial to separate the full command
set from the default command set. That way SMBus block commands can be
supported but not enabled by default.

> That would have to be
> one which selects if the smbus block command should be treated similar
> to the i2c block command

Do you know of any device actually behaving that way?

> or as individual command blocks. I'll play around
> with it some more. I'll send a patch if I find a solution which works for
> both lm93 and lineage-pem and is not too complicated.

The change I suggested to the "functionality" parameter should do.

-- 
Jean Delvare
SUSE L3 Support

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFT][PATCH 1/2] hwmon: (adm9240) Avoid forward declaration

[Guenter Roeck]

On 07/06/2014 12:29 AM, Jean Delvare wrote:
> Hi Guenter,
>
> On Sat, 05 Jul 2014 11:48:55 -0700, Guenter Roeck wrote:
>> On 07/05/2014 11:20 AM, Jean Delvare wrote:
>>> The main problem I see is that for I2C_SMBUS_BLOCK_DATA reads, the chip
>>> first returns the number of data bytes (as opposed to
>>> I2C_SMBUS_I2C_BLOCK_DATA where the controller decides how many bytes it
>>> wants to read.) There is no way the i2c-stub driver can guess that byte
>>> count, as it depends on the chip it is supposed to emulate (and might
>>> even change dynamically, at least in theory.)
>>>
>>> We could have limited support for that, but that would require extra
>>> module parameters to specify the block size for every register offset
>>> on which SMBus block reads can be attempted. This also assumes that these
>>> block sizes are static. And as you found out, that may also require
>>> allocating extra memory for every such register offset.
>>
>> I 'solved' the problem so far by only returning smbus block data after
>> it was written into a specific command. This way it is all dynamic.
>
> This is smart :-) But this assumes that block size is the same the same
> in both directions for a given register offset. This also assumes that
> block sizes do not change over time. But anyway, i2c-stub is bound to
> have such limitations, it can only emulate simple devices.
>
>>> But another difficulty is also that when SMBus block reads enter the
>>> game, the usual read/write symmetry tends to disappear. Often the
>>> registers you read with SMBus block read commands are also readable and
>>> writable at individual register addresses. i2c-stub has no way to know
>>> that. Drivers would typically use SMBus block reads for performance
>>> reason, but byte writes for convenience. So drivers operating on top of
>>> i2c-stub would get confused in no time.
>>>
>>> All these issues have so far convinced me that adding support for SMBus
>>> block read/writes to i2c-stub wasn't worth it. That being said, if you
>>> have a specific chip in mind that could be supported easily, I have no
>>> objection.
>>
>> The one I needed it for right now was lineage-pem; it was useful for me
>> since I don't have easy access to the HW anymore. Of course, problem with
>> that is what you pointed out ... with my change in the i2c-stub driver,
>> the lm93 driver now assumes that it can execute block commands. The only
>> way I see around that would be a module parameter.
>
> Actually there already is one, named "functionality". For now it can
> only be used to disable commands, because all supported commands were
> enabled by default. It would be trivial to separate the full command
> set from the default command set. That way SMBus block commands can be
> supported but not enabled by default.
>
>> That would have to be
>> one which selects if the smbus block command should be treated similar
>> to the i2c block command
>
> Do you know of any device actually behaving that way?
>
No, I thought the lm93 would do that, but as you probably know it is much
more complicated than that; I don't think it would be easy to simulate,
so I won't even try.

>> or as individual command blocks. I'll play around
>> with it some more. I'll send a patch if I find a solution which works for
>> both lm93 and lineage-pem and is not too complicated.
>
> The change I suggested to the "functionality" parameter should do.
>

Agreed.

Guenter


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors