[RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Laszlo Papp]

The MFD driver has now been added, so this driver is now being adopted to be a
subdevice driver on top of it. This means, the i2c driver usage is being
converted to platform driver usage all around.

Signed-off-by: Laszlo Papp <lpapp@kde.org>
---
This patch has been compile tested only and will be tested with real hardware,
but early reviews to catch any trivial issues would be welcome.
 drivers/hwmon/Kconfig   |   2 +-
 drivers/hwmon/max6650.c | 155 ++++++++++++++++++++++++------------------------
 2 files changed, 79 insertions(+), 78 deletions(-)

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 5ce43d8..48be395 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -926,7 +926,7 @@ config SENSORS_MAX6642
 
 config SENSORS_MAX6650
 	tristate "Maxim MAX6650 sensor chip"
-	depends on I2C
+	depends on MFD_MAX665X
 	help
 	  If you say yes here you get support for the MAX6650 / MAX6651
 	  sensor chips.
diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
index 0cafc39..7dbd60d 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -39,6 +39,9 @@
 #include <linux/hwmon.h>
 #include <linux/hwmon-sysfs.h>
 #include <linux/err.h>
+#include <linux/platform_device.h>
+
+#include <linux/mfd/max665x-private.h>
 
 /*
  * Insmod parameters
@@ -105,24 +108,23 @@ module_param(clock, int, S_IRUGO);
 
 #define DIV_FROM_REG(reg) (1 << (reg & 7))
 
-static int max6650_probe(struct i2c_client *client,
-			 const struct i2c_device_id *id);
-static int max6650_init_client(struct i2c_client *client);
-static int max6650_remove(struct i2c_client *client);
+static int max6650_probe(struct platform_device *pdev);
+static int max6650_init_client(struct platform_device *pdev);
+static int max6650_remove(struct platform_device *pdev);
 static struct max6650_data *max6650_update_device(struct device *dev);
 
 /*
  * Driver data (common to all clients)
  */
 
-static const struct i2c_device_id max6650_id[] = {
+static const struct platform_device_id max6650_id[] = {
 	{ "max6650", 1 },
 	{ "max6651", 4 },
 	{ }
 };
-MODULE_DEVICE_TABLE(i2c, max6650_id);
+MODULE_DEVICE_TABLE(platform, max6650_id);
 
-static struct i2c_driver max6650_driver = {
+static struct platform_driver max6650_driver = {
 	.driver = {
 		.name	= "max6650",
 	},
@@ -137,18 +139,19 @@ static struct i2c_driver max6650_driver = {
 
 struct max6650_data {
 	struct device *hwmon_dev;
+	struct max665x_dev *iodev;
 	struct mutex update_lock;
 	int nr_fans;
 	char valid; /* zero until following fields are valid */
 	unsigned long last_updated; /* in jiffies */
 
 	/* register values */
-	u8 speed;
-	u8 config;
-	u8 tach[4];
-	u8 count;
-	u8 dac;
-	u8 alarm;
+	int speed;
+	int config;
+	int tach[4];
+	int count;
+	int dac;
+	int alarm;
 };
 
 static ssize_t get_fan(struct device *dev, struct device_attribute *devattr,
@@ -235,8 +238,7 @@ static ssize_t get_target(struct device *dev, struct device_attribute *devattr,
 static ssize_t set_target(struct device *dev, struct device_attribute *devattr,
 			 const char *buf, size_t count)
 {
-	struct i2c_client *client = to_i2c_client(dev);
-	struct max6650_data *data = i2c_get_clientdata(client);
+	struct max6650_data *data = dev_get_drvdata(dev);
 	int kscale, ktach;
 	unsigned long rpm;
 	int err;
@@ -264,7 +266,7 @@ static ssize_t set_target(struct device *dev, struct device_attribute *devattr,
 		ktach = 255;
 	data->speed = ktach;
 
-	i2c_smbus_write_byte_data(client, MAX6650_REG_SPEED, data->speed);
+	regmap_write(data->iodev->map, MAX6650_REG_SPEED, data->speed);
 
 	mutex_unlock(&data->update_lock);
 
@@ -304,8 +306,7 @@ static ssize_t get_pwm(struct device *dev, struct device_attribute *devattr,
 static ssize_t set_pwm(struct device *dev, struct device_attribute *devattr,
 			const char *buf, size_t count)
 {
-	struct i2c_client *client = to_i2c_client(dev);
-	struct max6650_data *data = i2c_get_clientdata(client);
+	struct max6650_data *data = dev_get_drvdata(dev);
 	unsigned long pwm;
 	int err;
 
@@ -322,7 +323,7 @@ static ssize_t set_pwm(struct device *dev, struct device_attribute *devattr,
 	else
 		data->dac = 76 - (76 * pwm)/255;
 
-	i2c_smbus_write_byte_data(client, MAX6650_REG_DAC, data->dac);
+	regmap_write(data->iodev->map, MAX6650_REG_DAC, data->dac);
 
 	mutex_unlock(&data->update_lock);
 
@@ -350,8 +351,7 @@ static ssize_t get_enable(struct device *dev, struct device_attribute *devattr,
 static ssize_t set_enable(struct device *dev, struct device_attribute *devattr,
 			  const char *buf, size_t count)
 {
-	struct i2c_client *client = to_i2c_client(dev);
-	struct max6650_data *data = i2c_get_clientdata(client);
+	struct max6650_data *data = dev_get_drvdata(dev);
 	int max6650_modes[3] = {0, 3, 2};
 	unsigned long mode;
 	int err;
@@ -365,11 +365,11 @@ static ssize_t set_enable(struct device *dev, struct device_attribute *devattr,
 
 	mutex_lock(&data->update_lock);
 
-	data->config = i2c_smbus_read_byte_data(client, MAX6650_REG_CONFIG);
+	regmap_read(data->iodev->map, MAX6650_REG_CONFIG, &data->config);
 	data->config = (data->config & ~MAX6650_CFG_MODE_MASK)
 		       | (max6650_modes[mode] << 4);
 
-	i2c_smbus_write_byte_data(client, MAX6650_REG_CONFIG, data->config);
+	regmap_write(data->iodev->map, MAX6650_REG_CONFIG, data->config);
 
 	mutex_unlock(&data->update_lock);
 
@@ -400,8 +400,7 @@ static ssize_t get_div(struct device *dev, struct device_attribute *devattr,
 static ssize_t set_div(struct device *dev, struct device_attribute *devattr,
 		       const char *buf, size_t count)
 {
-	struct i2c_client *client = to_i2c_client(dev);
-	struct max6650_data *data = i2c_get_clientdata(client);
+	struct max6650_data *data = dev_get_drvdata(dev);
 	unsigned long div;
 	int err;
 
@@ -428,7 +427,7 @@ static ssize_t set_div(struct device *dev, struct device_attribute *devattr,
 		return -EINVAL;
 	}
 
-	i2c_smbus_write_byte_data(client, MAX6650_REG_COUNT, data->count);
+	regmap_write(data->iodev->map, MAX6650_REG_COUNT, data->count);
 	mutex_unlock(&data->update_lock);
 
 	return count;
@@ -445,16 +444,15 @@ static ssize_t get_alarm(struct device *dev, struct device_attribute *devattr,
 			 char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct max6650_data *data = max6650_update_device(dev);
-	struct i2c_client *client = to_i2c_client(dev);
+	struct max6650_data *data = dev_get_drvdata(dev);
 	int alarm = 0;
 
 	if (data->alarm & attr->index) {
 		mutex_lock(&data->update_lock);
-		alarm = 1;
 		data->alarm &= ~attr->index;
-		data->alarm |= i2c_smbus_read_byte_data(client,
-							MAX6650_REG_ALARM);
+		regmap_read(data->iodev->map, MAX6650_REG_ALARM, &alarm);
+		data->alarm |= alarm;
+		alarm = 1;
 		mutex_unlock(&data->update_lock);
 	}
 
@@ -484,10 +482,12 @@ static umode_t max6650_attrs_visible(struct kobject *kobj, struct attribute *a,
 				    int n)
 {
 	struct device *dev = container_of(kobj, struct device, kobj);
-	struct i2c_client *client = to_i2c_client(dev);
-	u8 alarm_en = i2c_smbus_read_byte_data(client, MAX6650_REG_ALARM_EN);
+	struct max6650_data *data = dev_get_drvdata(dev);
+	int alarm_en;
 	struct device_attribute *devattr;
 
+	regmap_read(data->iodev->map, MAX6650_REG_ALARM_EN, &alarm_en);
+
 	/*
 	 * Hide the alarms that have not been enabled by the firmware
 	 */
@@ -539,74 +539,76 @@ static const struct attribute_group max6651_attr_grp = {
  * Real code
  */
 
-static int max6650_probe(struct i2c_client *client,
-			 const struct i2c_device_id *id)
+static int max6650_probe(struct platform_device *pdev)
 {
+	struct max665x_dev *max665x = dev_get_drvdata(pdev->dev.parent);
 	struct max6650_data *data;
+	const struct platform_device_id *id = platform_get_device_id(pdev);
 	int err;
 
-	data = devm_kzalloc(&client->dev, sizeof(struct max6650_data),
+	data = devm_kzalloc(&pdev->dev, sizeof(struct max6650_data),
 			    GFP_KERNEL);
 	if (!data) {
-		dev_err(&client->dev, "out of memory.\n");
+		dev_err(&pdev->dev, "out of memory.\n");
 		return -ENOMEM;
 	}
 
-	i2c_set_clientdata(client, data);
+	data->iodev = max665x;
+	platform_set_drvdata(pdev, data);
 	mutex_init(&data->update_lock);
 	data->nr_fans = id->driver_data;
 
 	/*
 	 * Initialize the max6650 chip
 	 */
-	err = max6650_init_client(client);
+	err = max6650_init_client(pdev);
 	if (err)
 		return err;
 
-	err = sysfs_create_group(&client->dev.kobj, &max6650_attr_grp);
+	err = sysfs_create_group(&pdev->dev.kobj, &max6650_attr_grp);
 	if (err)
 		return err;
 	/* 3 additional fan inputs for the MAX6651 */
 	if (data->nr_fans == 4) {
-		err = sysfs_create_group(&client->dev.kobj, &max6651_attr_grp);
+		err = sysfs_create_group(&pdev->dev.kobj, &max6651_attr_grp);
 		if (err)
 			goto err_remove;
 	}
 
-	data->hwmon_dev = hwmon_device_register(&client->dev);
+	data->hwmon_dev = hwmon_device_register(&pdev->dev);
 	if (!IS_ERR(data->hwmon_dev))
 		return 0;
 
 	err = PTR_ERR(data->hwmon_dev);
-	dev_err(&client->dev, "error registering hwmon device.\n");
+	dev_err(&pdev->dev, "error registering hwmon device.\n");
 	if (data->nr_fans == 4)
-		sysfs_remove_group(&client->dev.kobj, &max6651_attr_grp);
+		sysfs_remove_group(&pdev->dev.kobj, &max6651_attr_grp);
 err_remove:
-	sysfs_remove_group(&client->dev.kobj, &max6650_attr_grp);
+	sysfs_remove_group(&pdev->dev.kobj, &max6650_attr_grp);
 	return err;
 }
 
-static int max6650_remove(struct i2c_client *client)
+static int max6650_remove(struct platform_device *pdev)
 {
-	struct max6650_data *data = i2c_get_clientdata(client);
+	struct max6650_data *data = platform_get_drvdata(pdev);
 
 	hwmon_device_unregister(data->hwmon_dev);
 	if (data->nr_fans == 4)
-		sysfs_remove_group(&client->dev.kobj, &max6651_attr_grp);
-	sysfs_remove_group(&client->dev.kobj, &max6650_attr_grp);
+		sysfs_remove_group(&pdev->dev.kobj, &max6651_attr_grp);
+	sysfs_remove_group(&pdev->dev.kobj, &max6650_attr_grp);
 	return 0;
 }
 
-static int max6650_init_client(struct i2c_client *client)
+static int max6650_init_client(struct platform_device *pdev)
 {
-	struct max6650_data *data = i2c_get_clientdata(client);
+	struct max6650_data *data = platform_get_drvdata(pdev);
 	int config;
 	int err = -EIO;
 
-	config = i2c_smbus_read_byte_data(client, MAX6650_REG_CONFIG);
+	regmap_read(data->iodev->map, MAX6650_REG_CONFIG, &config);
 
 	if (config < 0) {
-		dev_err(&client->dev, "Error reading config, aborting.\n");
+		dev_err(&pdev->dev, "Error reading config, aborting.\n");
 		return err;
 	}
 
@@ -620,11 +622,11 @@ static int max6650_init_client(struct i2c_client *client)
 		config |= MAX6650_CFG_V12;
 		break;
 	default:
-		dev_err(&client->dev, "illegal value for fan_voltage (%d)\n",
+		dev_err(&pdev->dev, "illegal value for fan_voltage (%d)\n",
 			fan_voltage);
 	}
 
-	dev_info(&client->dev, "Fan voltage is set to %dV.\n",
+	dev_info(&pdev->dev, "Fan voltage is set to %dV.\n",
 		 (config & MAX6650_CFG_V12) ? 12 : 5);
 
 	switch (prescaler) {
@@ -650,11 +652,11 @@ static int max6650_init_client(struct i2c_client *client)
 			 | MAX6650_CFG_PRESCALER_16;
 		break;
 	default:
-		dev_err(&client->dev, "illegal value for prescaler (%d)\n",
+		dev_err(&pdev->dev, "illegal value for prescaler (%d)\n",
 			prescaler);
 	}
 
-	dev_info(&client->dev, "Prescaler is set to %d.\n",
+	dev_info(&pdev->dev, "Prescaler is set to %d.\n",
 		 1 << (config & MAX6650_CFG_PRESCALER_MASK));
 
 	/*
@@ -664,22 +666,22 @@ static int max6650_init_client(struct i2c_client *client)
 	 */
 
 	if ((config & MAX6650_CFG_MODE_MASK) == MAX6650_CFG_MODE_OFF) {
-		dev_dbg(&client->dev, "Change mode to open loop, full off.\n");
+		dev_dbg(&pdev->dev, "Change mode to open loop, full off.\n");
 		config = (config & ~MAX6650_CFG_MODE_MASK)
 			 | MAX6650_CFG_MODE_OPEN_LOOP;
-		if (i2c_smbus_write_byte_data(client, MAX6650_REG_DAC, 255)) {
-			dev_err(&client->dev, "DAC write error, aborting.\n");
+		if (regmap_write(data->iodev->map, MAX6650_REG_DAC, 255) < 0) {
+			dev_err(&pdev->dev, "DAC write error, aborting.\n");
 			return err;
 		}
 	}
 
-	if (i2c_smbus_write_byte_data(client, MAX6650_REG_CONFIG, config)) {
-		dev_err(&client->dev, "Config write error, aborting.\n");
+	if (regmap_write(data->iodev->map, MAX6650_REG_CONFIG, config) < 0) {
+		dev_err(&pdev->dev, "Config write error, aborting.\n");
 		return err;
 	}
 
 	data->config = config;
-	data->count = i2c_smbus_read_byte_data(client, MAX6650_REG_COUNT);
+	regmap_read(data->iodev->map, MAX6650_REG_COUNT, &data->count);
 
 	return 0;
 }
@@ -694,31 +696,30 @@ static const u8 tach_reg[] = {
 static struct max6650_data *max6650_update_device(struct device *dev)
 {
 	int i;
-	struct i2c_client *client = to_i2c_client(dev);
-	struct max6650_data *data = i2c_get_clientdata(client);
+	struct max6650_data *data = dev_get_drvdata(dev);
+	int alarm;
 
 	mutex_lock(&data->update_lock);
 
 	if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
-		data->speed = i2c_smbus_read_byte_data(client,
-						       MAX6650_REG_SPEED);
-		data->config = i2c_smbus_read_byte_data(client,
-							MAX6650_REG_CONFIG);
+		regmap_read(data->iodev->map, MAX6650_REG_SPEED, &data->speed);
+		regmap_read(data->iodev->map, MAX6650_REG_CONFIG,
+			&data->config);
 		for (i = 0; i < data->nr_fans; i++) {
-			data->tach[i] = i2c_smbus_read_byte_data(client,
-								 tach_reg[i]);
+			regmap_read(data->iodev->map, tach_reg[i],
+				&data->tach[i]);
 		}
-		data->count = i2c_smbus_read_byte_data(client,
-							MAX6650_REG_COUNT);
-		data->dac = i2c_smbus_read_byte_data(client, MAX6650_REG_DAC);
+		regmap_read(data->iodev->map, MAX6650_REG_COUNT,
+			&data->count);
+		regmap_read(data->iodev->map, MAX6650_REG_DAC, &data->dac);
 
 		/*
 		 * Alarms are cleared on read in case the condition that
 		 * caused the alarm is removed. Keep the value latched here
 		 * for providing the register through different alarm files.
 		 */
-		data->alarm |= i2c_smbus_read_byte_data(client,
-							MAX6650_REG_ALARM);
+		regmap_read(data->iodev->map, MAX6650_REG_ALARM, &alarm);
+		data->alarm |= alarm;
 
 		data->last_updated = jiffies;
 		data->valid = 1;
@@ -729,7 +730,7 @@ static struct max6650_data *max6650_update_device(struct device *dev)
 	return data;
 }
 
-module_i2c_driver(max6650_driver);
+module_platform_driver(max6650_driver);
 
 MODULE_AUTHOR("Hans J. Koch");
 MODULE_DESCRIPTION("MAX6650 sensor driver");
-- 
1.8.5.4

[lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Laszlo Papp]

The MFD driver has now been added, so this driver is now being adopted to be a
subdevice driver on top of it. This means, the i2c driver usage is being
converted to platform driver usage all around.

Signed-off-by: Laszlo Papp <lpapp@kde.org>
---
This patch has been compile tested only and will be tested with real hardware,
but early reviews to catch any trivial issues would be welcome.
 drivers/hwmon/Kconfig   |   2 +-
 drivers/hwmon/max6650.c | 155 ++++++++++++++++++++++++------------------------
 2 files changed, 79 insertions(+), 78 deletions(-)

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 5ce43d8..48be395 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -926,7 +926,7 @@ config SENSORS_MAX6642
 
 config SENSORS_MAX6650
 	tristate "Maxim MAX6650 sensor chip"
-	depends on I2C
+	depends on MFD_MAX665X
 	help
 	  If you say yes here you get support for the MAX6650 / MAX6651
 	  sensor chips.
diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
index 0cafc39..7dbd60d 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -39,6 +39,9 @@
 #include <linux/hwmon.h>
 #include <linux/hwmon-sysfs.h>
 #include <linux/err.h>
+#include <linux/platform_device.h>
+
+#include <linux/mfd/max665x-private.h>
 
 /*
  * Insmod parameters
@@ -105,24 +108,23 @@ module_param(clock, int, S_IRUGO);
 
 #define DIV_FROM_REG(reg) (1 << (reg & 7))
 
-static int max6650_probe(struct i2c_client *client,
-			 const struct i2c_device_id *id);
-static int max6650_init_client(struct i2c_client *client);
-static int max6650_remove(struct i2c_client *client);
+static int max6650_probe(struct platform_device *pdev);
+static int max6650_init_client(struct platform_device *pdev);
+static int max6650_remove(struct platform_device *pdev);
 static struct max6650_data *max6650_update_device(struct device *dev);
 
 /*
  * Driver data (common to all clients)
  */
 
-static const struct i2c_device_id max6650_id[] = {
+static const struct platform_device_id max6650_id[] = {
 	{ "max6650", 1 },
 	{ "max6651", 4 },
 	{ }
 };
-MODULE_DEVICE_TABLE(i2c, max6650_id);
+MODULE_DEVICE_TABLE(platform, max6650_id);
 
-static struct i2c_driver max6650_driver = {
+static struct platform_driver max6650_driver = {
 	.driver = {
 		.name	= "max6650",
 	},
@@ -137,18 +139,19 @@ static struct i2c_driver max6650_driver = {
 
 struct max6650_data {
 	struct device *hwmon_dev;
+	struct max665x_dev *iodev;
 	struct mutex update_lock;
 	int nr_fans;
 	char valid; /* zero until following fields are valid */
 	unsigned long last_updated; /* in jiffies */
 
 	/* register values */
-	u8 speed;
-	u8 config;
-	u8 tach[4];
-	u8 count;
-	u8 dac;
-	u8 alarm;
+	int speed;
+	int config;
+	int tach[4];
+	int count;
+	int dac;
+	int alarm;
 };
 
 static ssize_t get_fan(struct device *dev, struct device_attribute *devattr,
@@ -235,8 +238,7 @@ static ssize_t get_target(struct device *dev, struct device_attribute *devattr,
 static ssize_t set_target(struct device *dev, struct device_attribute *devattr,
 			 const char *buf, size_t count)
 {
-	struct i2c_client *client = to_i2c_client(dev);
-	struct max6650_data *data = i2c_get_clientdata(client);
+	struct max6650_data *data = dev_get_drvdata(dev);
 	int kscale, ktach;
 	unsigned long rpm;
 	int err;
@@ -264,7 +266,7 @@ static ssize_t set_target(struct device *dev, struct device_attribute *devattr,
 		ktach = 255;
 	data->speed = ktach;
 
-	i2c_smbus_write_byte_data(client, MAX6650_REG_SPEED, data->speed);
+	regmap_write(data->iodev->map, MAX6650_REG_SPEED, data->speed);
 
 	mutex_unlock(&data->update_lock);
 
@@ -304,8 +306,7 @@ static ssize_t get_pwm(struct device *dev, struct device_attribute *devattr,
 static ssize_t set_pwm(struct device *dev, struct device_attribute *devattr,
 			const char *buf, size_t count)
 {
-	struct i2c_client *client = to_i2c_client(dev);
-	struct max6650_data *data = i2c_get_clientdata(client);
+	struct max6650_data *data = dev_get_drvdata(dev);
 	unsigned long pwm;
 	int err;
 
@@ -322,7 +323,7 @@ static ssize_t set_pwm(struct device *dev, struct device_attribute *devattr,
 	else
 		data->dac = 76 - (76 * pwm)/255;
 
-	i2c_smbus_write_byte_data(client, MAX6650_REG_DAC, data->dac);
+	regmap_write(data->iodev->map, MAX6650_REG_DAC, data->dac);
 
 	mutex_unlock(&data->update_lock);
 
@@ -350,8 +351,7 @@ static ssize_t get_enable(struct device *dev, struct device_attribute *devattr,
 static ssize_t set_enable(struct device *dev, struct device_attribute *devattr,
 			  const char *buf, size_t count)
 {
-	struct i2c_client *client = to_i2c_client(dev);
-	struct max6650_data *data = i2c_get_clientdata(client);
+	struct max6650_data *data = dev_get_drvdata(dev);
 	int max6650_modes[3] = {0, 3, 2};
 	unsigned long mode;
 	int err;
@@ -365,11 +365,11 @@ static ssize_t set_enable(struct device *dev, struct device_attribute *devattr,
 
 	mutex_lock(&data->update_lock);
 
-	data->config = i2c_smbus_read_byte_data(client, MAX6650_REG_CONFIG);
+	regmap_read(data->iodev->map, MAX6650_REG_CONFIG, &data->config);
 	data->config = (data->config & ~MAX6650_CFG_MODE_MASK)
 		       | (max6650_modes[mode] << 4);
 
-	i2c_smbus_write_byte_data(client, MAX6650_REG_CONFIG, data->config);
+	regmap_write(data->iodev->map, MAX6650_REG_CONFIG, data->config);
 
 	mutex_unlock(&data->update_lock);
 
@@ -400,8 +400,7 @@ static ssize_t get_div(struct device *dev, struct device_attribute *devattr,
 static ssize_t set_div(struct device *dev, struct device_attribute *devattr,
 		       const char *buf, size_t count)
 {
-	struct i2c_client *client = to_i2c_client(dev);
-	struct max6650_data *data = i2c_get_clientdata(client);
+	struct max6650_data *data = dev_get_drvdata(dev);
 	unsigned long div;
 	int err;
 
@@ -428,7 +427,7 @@ static ssize_t set_div(struct device *dev, struct device_attribute *devattr,
 		return -EINVAL;
 	}
 
-	i2c_smbus_write_byte_data(client, MAX6650_REG_COUNT, data->count);
+	regmap_write(data->iodev->map, MAX6650_REG_COUNT, data->count);
 	mutex_unlock(&data->update_lock);
 
 	return count;
@@ -445,16 +444,15 @@ static ssize_t get_alarm(struct device *dev, struct device_attribute *devattr,
 			 char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct max6650_data *data = max6650_update_device(dev);
-	struct i2c_client *client = to_i2c_client(dev);
+	struct max6650_data *data = dev_get_drvdata(dev);
 	int alarm = 0;
 
 	if (data->alarm & attr->index) {
 		mutex_lock(&data->update_lock);
-		alarm = 1;
 		data->alarm &= ~attr->index;
-		data->alarm |= i2c_smbus_read_byte_data(client,
-							MAX6650_REG_ALARM);
+		regmap_read(data->iodev->map, MAX6650_REG_ALARM, &alarm);
+		data->alarm |= alarm;
+		alarm = 1;
 		mutex_unlock(&data->update_lock);
 	}
 
@@ -484,10 +482,12 @@ static umode_t max6650_attrs_visible(struct kobject *kobj, struct attribute *a,
 				    int n)
 {
 	struct device *dev = container_of(kobj, struct device, kobj);
-	struct i2c_client *client = to_i2c_client(dev);
-	u8 alarm_en = i2c_smbus_read_byte_data(client, MAX6650_REG_ALARM_EN);
+	struct max6650_data *data = dev_get_drvdata(dev);
+	int alarm_en;
 	struct device_attribute *devattr;
 
+	regmap_read(data->iodev->map, MAX6650_REG_ALARM_EN, &alarm_en);
+
 	/*
 	 * Hide the alarms that have not been enabled by the firmware
 	 */
@@ -539,74 +539,76 @@ static const struct attribute_group max6651_attr_grp = {
  * Real code
  */
 
-static int max6650_probe(struct i2c_client *client,
-			 const struct i2c_device_id *id)
+static int max6650_probe(struct platform_device *pdev)
 {
+	struct max665x_dev *max665x = dev_get_drvdata(pdev->dev.parent);
 	struct max6650_data *data;
+	const struct platform_device_id *id = platform_get_device_id(pdev);
 	int err;
 
-	data = devm_kzalloc(&client->dev, sizeof(struct max6650_data),
+	data = devm_kzalloc(&pdev->dev, sizeof(struct max6650_data),
 			    GFP_KERNEL);
 	if (!data) {
-		dev_err(&client->dev, "out of memory.\n");
+		dev_err(&pdev->dev, "out of memory.\n");
 		return -ENOMEM;
 	}
 
-	i2c_set_clientdata(client, data);
+	data->iodev = max665x;
+	platform_set_drvdata(pdev, data);
 	mutex_init(&data->update_lock);
 	data->nr_fans = id->driver_data;
 
 	/*
 	 * Initialize the max6650 chip
 	 */
-	err = max6650_init_client(client);
+	err = max6650_init_client(pdev);
 	if (err)
 		return err;
 
-	err = sysfs_create_group(&client->dev.kobj, &max6650_attr_grp);
+	err = sysfs_create_group(&pdev->dev.kobj, &max6650_attr_grp);
 	if (err)
 		return err;
 	/* 3 additional fan inputs for the MAX6651 */
 	if (data->nr_fans = 4) {
-		err = sysfs_create_group(&client->dev.kobj, &max6651_attr_grp);
+		err = sysfs_create_group(&pdev->dev.kobj, &max6651_attr_grp);
 		if (err)
 			goto err_remove;
 	}
 
-	data->hwmon_dev = hwmon_device_register(&client->dev);
+	data->hwmon_dev = hwmon_device_register(&pdev->dev);
 	if (!IS_ERR(data->hwmon_dev))
 		return 0;
 
 	err = PTR_ERR(data->hwmon_dev);
-	dev_err(&client->dev, "error registering hwmon device.\n");
+	dev_err(&pdev->dev, "error registering hwmon device.\n");
 	if (data->nr_fans = 4)
-		sysfs_remove_group(&client->dev.kobj, &max6651_attr_grp);
+		sysfs_remove_group(&pdev->dev.kobj, &max6651_attr_grp);
 err_remove:
-	sysfs_remove_group(&client->dev.kobj, &max6650_attr_grp);
+	sysfs_remove_group(&pdev->dev.kobj, &max6650_attr_grp);
 	return err;
 }
 
-static int max6650_remove(struct i2c_client *client)
+static int max6650_remove(struct platform_device *pdev)
 {
-	struct max6650_data *data = i2c_get_clientdata(client);
+	struct max6650_data *data = platform_get_drvdata(pdev);
 
 	hwmon_device_unregister(data->hwmon_dev);
 	if (data->nr_fans = 4)
-		sysfs_remove_group(&client->dev.kobj, &max6651_attr_grp);
-	sysfs_remove_group(&client->dev.kobj, &max6650_attr_grp);
+		sysfs_remove_group(&pdev->dev.kobj, &max6651_attr_grp);
+	sysfs_remove_group(&pdev->dev.kobj, &max6650_attr_grp);
 	return 0;
 }
 
-static int max6650_init_client(struct i2c_client *client)
+static int max6650_init_client(struct platform_device *pdev)
 {
-	struct max6650_data *data = i2c_get_clientdata(client);
+	struct max6650_data *data = platform_get_drvdata(pdev);
 	int config;
 	int err = -EIO;
 
-	config = i2c_smbus_read_byte_data(client, MAX6650_REG_CONFIG);
+	regmap_read(data->iodev->map, MAX6650_REG_CONFIG, &config);
 
 	if (config < 0) {
-		dev_err(&client->dev, "Error reading config, aborting.\n");
+		dev_err(&pdev->dev, "Error reading config, aborting.\n");
 		return err;
 	}
 
@@ -620,11 +622,11 @@ static int max6650_init_client(struct i2c_client *client)
 		config |= MAX6650_CFG_V12;
 		break;
 	default:
-		dev_err(&client->dev, "illegal value for fan_voltage (%d)\n",
+		dev_err(&pdev->dev, "illegal value for fan_voltage (%d)\n",
 			fan_voltage);
 	}
 
-	dev_info(&client->dev, "Fan voltage is set to %dV.\n",
+	dev_info(&pdev->dev, "Fan voltage is set to %dV.\n",
 		 (config & MAX6650_CFG_V12) ? 12 : 5);
 
 	switch (prescaler) {
@@ -650,11 +652,11 @@ static int max6650_init_client(struct i2c_client *client)
 			 | MAX6650_CFG_PRESCALER_16;
 		break;
 	default:
-		dev_err(&client->dev, "illegal value for prescaler (%d)\n",
+		dev_err(&pdev->dev, "illegal value for prescaler (%d)\n",
 			prescaler);
 	}
 
-	dev_info(&client->dev, "Prescaler is set to %d.\n",
+	dev_info(&pdev->dev, "Prescaler is set to %d.\n",
 		 1 << (config & MAX6650_CFG_PRESCALER_MASK));
 
 	/*
@@ -664,22 +666,22 @@ static int max6650_init_client(struct i2c_client *client)
 	 */
 
 	if ((config & MAX6650_CFG_MODE_MASK) = MAX6650_CFG_MODE_OFF) {
-		dev_dbg(&client->dev, "Change mode to open loop, full off.\n");
+		dev_dbg(&pdev->dev, "Change mode to open loop, full off.\n");
 		config = (config & ~MAX6650_CFG_MODE_MASK)
 			 | MAX6650_CFG_MODE_OPEN_LOOP;
-		if (i2c_smbus_write_byte_data(client, MAX6650_REG_DAC, 255)) {
-			dev_err(&client->dev, "DAC write error, aborting.\n");
+		if (regmap_write(data->iodev->map, MAX6650_REG_DAC, 255) < 0) {
+			dev_err(&pdev->dev, "DAC write error, aborting.\n");
 			return err;
 		}
 	}
 
-	if (i2c_smbus_write_byte_data(client, MAX6650_REG_CONFIG, config)) {
-		dev_err(&client->dev, "Config write error, aborting.\n");
+	if (regmap_write(data->iodev->map, MAX6650_REG_CONFIG, config) < 0) {
+		dev_err(&pdev->dev, "Config write error, aborting.\n");
 		return err;
 	}
 
 	data->config = config;
-	data->count = i2c_smbus_read_byte_data(client, MAX6650_REG_COUNT);
+	regmap_read(data->iodev->map, MAX6650_REG_COUNT, &data->count);
 
 	return 0;
 }
@@ -694,31 +696,30 @@ static const u8 tach_reg[] = {
 static struct max6650_data *max6650_update_device(struct device *dev)
 {
 	int i;
-	struct i2c_client *client = to_i2c_client(dev);
-	struct max6650_data *data = i2c_get_clientdata(client);
+	struct max6650_data *data = dev_get_drvdata(dev);
+	int alarm;
 
 	mutex_lock(&data->update_lock);
 
 	if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
-		data->speed = i2c_smbus_read_byte_data(client,
-						       MAX6650_REG_SPEED);
-		data->config = i2c_smbus_read_byte_data(client,
-							MAX6650_REG_CONFIG);
+		regmap_read(data->iodev->map, MAX6650_REG_SPEED, &data->speed);
+		regmap_read(data->iodev->map, MAX6650_REG_CONFIG,
+			&data->config);
 		for (i = 0; i < data->nr_fans; i++) {
-			data->tach[i] = i2c_smbus_read_byte_data(client,
-								 tach_reg[i]);
+			regmap_read(data->iodev->map, tach_reg[i],
+				&data->tach[i]);
 		}
-		data->count = i2c_smbus_read_byte_data(client,
-							MAX6650_REG_COUNT);
-		data->dac = i2c_smbus_read_byte_data(client, MAX6650_REG_DAC);
+		regmap_read(data->iodev->map, MAX6650_REG_COUNT,
+			&data->count);
+		regmap_read(data->iodev->map, MAX6650_REG_DAC, &data->dac);
 
 		/*
 		 * Alarms are cleared on read in case the condition that
 		 * caused the alarm is removed. Keep the value latched here
 		 * for providing the register through different alarm files.
 		 */
-		data->alarm |= i2c_smbus_read_byte_data(client,
-							MAX6650_REG_ALARM);
+		regmap_read(data->iodev->map, MAX6650_REG_ALARM, &alarm);
+		data->alarm |= alarm;
 
 		data->last_updated = jiffies;
 		data->valid = 1;
@@ -729,7 +730,7 @@ static struct max6650_data *max6650_update_device(struct device *dev)
 	return data;
 }
 
-module_i2c_driver(max6650_driver);
+module_platform_driver(max6650_driver);
 
 MODULE_AUTHOR("Hans J. Koch");
 MODULE_DESCRIPTION("MAX6650 sensor driver");
-- 
1.8.5.4


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

Re: [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Lee Jones]

> The MFD driver has now been added, so this driver is now being adopted to be a
> subdevice driver on top of it. This means, the i2c driver usage is being
> converted to platform driver usage all around.
> 
> Signed-off-by: Laszlo Papp <lpapp@kde.org>
> ---
> This patch has been compile tested only and will be tested with real hardware,
> but early reviews to catch any trivial issues would be welcome.
>  drivers/hwmon/Kconfig   |   2 +-
>  drivers/hwmon/max6650.c | 155 ++++++++++++++++++++++++------------------------
>  2 files changed, 79 insertions(+), 78 deletions(-)

<snip>

>  /*
>   * Insmod parameters
> @@ -105,24 +108,23 @@ module_param(clock, int, S_IRUGO);
>  
>  #define DIV_FROM_REG(reg) (1 << (reg & 7))
>  
> -static int max6650_probe(struct i2c_client *client,
> -			 const struct i2c_device_id *id);
> -static int max6650_init_client(struct i2c_client *client);
> -static int max6650_remove(struct i2c_client *client);
> +static int max6650_probe(struct platform_device *pdev);
> +static int max6650_init_client(struct platform_device *pdev);
> +static int max6650_remove(struct platform_device *pdev);
>  static struct max6650_data *max6650_update_device(struct device *dev);

It would be good to remove these forward declarations in the future.

If no one volunteers I'll happily do it.

>  /*
>   * Driver data (common to all clients)
>   */
>  
> -static const struct i2c_device_id max6650_id[] = {
> +static const struct platform_device_id max6650_id[] = {
>  	{ "max6650", 1 },
>  	{ "max6651", 4 },
>  	{ }
>  };
> -MODULE_DEVICE_TABLE(i2c, max6650_id);
> +MODULE_DEVICE_TABLE(platform, max6650_id);

I thought you were going to do the matching in the MFD driver?

If not, what's the point in the exported 'type' attribute?

> -static struct i2c_driver max6650_driver = {
> +static struct platform_driver max6650_driver = {
>  	.driver = {
>  		.name	= "max6650",

This should be changed as it now supports max665{0,1} right?

<snip>

> -	i2c_smbus_write_byte_data(client, MAX6650_REG_SPEED, data->speed);
> +	regmap_write(data->iodev->map, MAX6650_REG_SPEED, data->speed);

Ensure all of the regmap stuff is fully tested.

<snip>

> @@ -484,10 +482,12 @@ static umode_t max6650_attrs_visible(struct kobject *kobj, struct attribute *a,
>  				    int n)
>  {
>  	struct device *dev = container_of(kobj, struct device, kobj);
> -	struct i2c_client *client = to_i2c_client(dev);
> -	u8 alarm_en = i2c_smbus_read_byte_data(client, MAX6650_REG_ALARM_EN);
> +	struct max6650_data *data = dev_get_drvdata(dev);
> +	int alarm_en;
>  	struct device_attribute *devattr;
>  
> +	regmap_read(data->iodev->map, MAX6650_REG_ALARM_EN, &alarm_en);
> +

Where is this then used?

> -static int max6650_probe(struct i2c_client *client,
> -			 const struct i2c_device_id *id)
> +static int max6650_probe(struct platform_device *pdev)
>  {
> +	struct max665x_dev *max665x = dev_get_drvdata(pdev->dev.parent);
>  	struct max6650_data *data;
> +	const struct platform_device_id *id = platform_get_device_id(pdev);

What's the point in 'type' in the global container?

It's looking as though you're not going to need it to be global after
all, right?

<snip>

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Lee Jones]

PiBUaGUgTUZEIGRyaXZlciBoYXMgbm93IGJlZW4gYWRkZWQsIHNvIHRoaXMgZHJpdmVyIGlzIG5v
dyBiZWluZyBhZG9wdGVkIHRvIGJlIGEKPiBzdWJkZXZpY2UgZHJpdmVyIG9uIHRvcCBvZiBpdC4g
VGhpcyBtZWFucywgdGhlIGkyYyBkcml2ZXIgdXNhZ2UgaXMgYmVpbmcKPiBjb252ZXJ0ZWQgdG8g
cGxhdGZvcm0gZHJpdmVyIHVzYWdlIGFsbCBhcm91bmQuCj4gCj4gU2lnbmVkLW9mZi1ieTogTGFz
emxvIFBhcHAgPGxwYXBwQGtkZS5vcmc+Cj4gLS0tCj4gVGhpcyBwYXRjaCBoYXMgYmVlbiBjb21w
aWxlIHRlc3RlZCBvbmx5IGFuZCB3aWxsIGJlIHRlc3RlZCB3aXRoIHJlYWwgaGFyZHdhcmUsCj4g
YnV0IGVhcmx5IHJldmlld3MgdG8gY2F0Y2ggYW55IHRyaXZpYWwgaXNzdWVzIHdvdWxkIGJlIHdl
bGNvbWUuCj4gIGRyaXZlcnMvaHdtb24vS2NvbmZpZyAgIHwgICAyICstCj4gIGRyaXZlcnMvaHdt
b24vbWF4NjY1MC5jIHwgMTU1ICsrKysrKysrKysrKysrKysrKysrKysrKy0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLQo+ICAyIGZpbGVzIGNoYW5nZWQsIDc5IGluc2VydGlvbnMoKyksIDc4IGRlbGV0
aW9ucygtKQoKPHNuaXA+Cgo+ICAvKgo+ICAgKiBJbnNtb2QgcGFyYW1ldGVycwo+IEBAIC0xMDUs
MjQgKzEwOCwyMyBAQCBtb2R1bGVfcGFyYW0oY2xvY2ssIGludCwgU19JUlVHTyk7Cj4gIAo+ICAj
ZGVmaW5lIERJVl9GUk9NX1JFRyhyZWcpICgxIDw8IChyZWcgJiA3KSkKPiAgCj4gLXN0YXRpYyBp
bnQgbWF4NjY1MF9wcm9iZShzdHJ1Y3QgaTJjX2NsaWVudCAqY2xpZW50LAo+IC0JCQkgY29uc3Qg
c3RydWN0IGkyY19kZXZpY2VfaWQgKmlkKTsKPiAtc3RhdGljIGludCBtYXg2NjUwX2luaXRfY2xp
ZW50KHN0cnVjdCBpMmNfY2xpZW50ICpjbGllbnQpOwo+IC1zdGF0aWMgaW50IG1heDY2NTBfcmVt
b3ZlKHN0cnVjdCBpMmNfY2xpZW50ICpjbGllbnQpOwo+ICtzdGF0aWMgaW50IG1heDY2NTBfcHJv
YmUoc3RydWN0IHBsYXRmb3JtX2RldmljZSAqcGRldik7Cj4gK3N0YXRpYyBpbnQgbWF4NjY1MF9p
bml0X2NsaWVudChzdHJ1Y3QgcGxhdGZvcm1fZGV2aWNlICpwZGV2KTsKPiArc3RhdGljIGludCBt
YXg2NjUwX3JlbW92ZShzdHJ1Y3QgcGxhdGZvcm1fZGV2aWNlICpwZGV2KTsKPiAgc3RhdGljIHN0
cnVjdCBtYXg2NjUwX2RhdGEgKm1heDY2NTBfdXBkYXRlX2RldmljZShzdHJ1Y3QgZGV2aWNlICpk
ZXYpOwoKSXQgd291bGQgYmUgZ29vZCB0byByZW1vdmUgdGhlc2UgZm9yd2FyZCBkZWNsYXJhdGlv
bnMgaW4gdGhlIGZ1dHVyZS4KCklmIG5vIG9uZSB2b2x1bnRlZXJzIEknbGwgaGFwcGlseSBkbyBp
dC4KCj4gIC8qCj4gICAqIERyaXZlciBkYXRhIChjb21tb24gdG8gYWxsIGNsaWVudHMpCj4gICAq
Lwo+ICAKPiAtc3RhdGljIGNvbnN0IHN0cnVjdCBpMmNfZGV2aWNlX2lkIG1heDY2NTBfaWRbXSA9
IHsKPiArc3RhdGljIGNvbnN0IHN0cnVjdCBwbGF0Zm9ybV9kZXZpY2VfaWQgbWF4NjY1MF9pZFtd
ID0gewo+ICAJeyAibWF4NjY1MCIsIDEgfSwKPiAgCXsgIm1heDY2NTEiLCA0IH0sCj4gIAl7IH0K
PiAgfTsKPiAtTU9EVUxFX0RFVklDRV9UQUJMRShpMmMsIG1heDY2NTBfaWQpOwo+ICtNT0RVTEVf
REVWSUNFX1RBQkxFKHBsYXRmb3JtLCBtYXg2NjUwX2lkKTsKCkkgdGhvdWdodCB5b3Ugd2VyZSBn
b2luZyB0byBkbyB0aGUgbWF0Y2hpbmcgaW4gdGhlIE1GRCBkcml2ZXI/CgpJZiBub3QsIHdoYXQn
cyB0aGUgcG9pbnQgaW4gdGhlIGV4cG9ydGVkICd0eXBlJyBhdHRyaWJ1dGU/Cgo+IC1zdGF0aWMg
c3RydWN0IGkyY19kcml2ZXIgbWF4NjY1MF9kcml2ZXIgPSB7Cj4gK3N0YXRpYyBzdHJ1Y3QgcGxh
dGZvcm1fZHJpdmVyIG1heDY2NTBfZHJpdmVyID0gewo+ICAJLmRyaXZlciA9IHsKPiAgCQkubmFt
ZQk9ICJtYXg2NjUwIiwKClRoaXMgc2hvdWxkIGJlIGNoYW5nZWQgYXMgaXQgbm93IHN1cHBvcnRz
IG1heDY2NXswLDF9IHJpZ2h0PwoKPHNuaXA+Cgo+IC0JaTJjX3NtYnVzX3dyaXRlX2J5dGVfZGF0
YShjbGllbnQsIE1BWDY2NTBfUkVHX1NQRUVELCBkYXRhLT5zcGVlZCk7Cj4gKwlyZWdtYXBfd3Jp
dGUoZGF0YS0+aW9kZXYtPm1hcCwgTUFYNjY1MF9SRUdfU1BFRUQsIGRhdGEtPnNwZWVkKTsKCkVu
c3VyZSBhbGwgb2YgdGhlIHJlZ21hcCBzdHVmZiBpcyBmdWxseSB0ZXN0ZWQuCgo8c25pcD4KCj4g
QEAgLTQ4NCwxMCArNDgyLDEyIEBAIHN0YXRpYyB1bW9kZV90IG1heDY2NTBfYXR0cnNfdmlzaWJs
ZShzdHJ1Y3Qga29iamVjdCAqa29iaiwgc3RydWN0IGF0dHJpYnV0ZSAqYSwKPiAgCQkJCSAgICBp
bnQgbikKPiAgewo+ICAJc3RydWN0IGRldmljZSAqZGV2ID0gY29udGFpbmVyX29mKGtvYmosIHN0
cnVjdCBkZXZpY2UsIGtvYmopOwo+IC0Jc3RydWN0IGkyY19jbGllbnQgKmNsaWVudCA9IHRvX2ky
Y19jbGllbnQoZGV2KTsKPiAtCXU4IGFsYXJtX2VuID0gaTJjX3NtYnVzX3JlYWRfYnl0ZV9kYXRh
KGNsaWVudCwgTUFYNjY1MF9SRUdfQUxBUk1fRU4pOwo+ICsJc3RydWN0IG1heDY2NTBfZGF0YSAq
ZGF0YSA9IGRldl9nZXRfZHJ2ZGF0YShkZXYpOwo+ICsJaW50IGFsYXJtX2VuOwo+ICAJc3RydWN0
IGRldmljZV9hdHRyaWJ1dGUgKmRldmF0dHI7Cj4gIAo+ICsJcmVnbWFwX3JlYWQoZGF0YS0+aW9k
ZXYtPm1hcCwgTUFYNjY1MF9SRUdfQUxBUk1fRU4sICZhbGFybV9lbik7Cj4gKwoKV2hlcmUgaXMg
dGhpcyB0aGVuIHVzZWQ/Cgo+IC1zdGF0aWMgaW50IG1heDY2NTBfcHJvYmUoc3RydWN0IGkyY19j
bGllbnQgKmNsaWVudCwKPiAtCQkJIGNvbnN0IHN0cnVjdCBpMmNfZGV2aWNlX2lkICppZCkKPiAr
c3RhdGljIGludCBtYXg2NjUwX3Byb2JlKHN0cnVjdCBwbGF0Zm9ybV9kZXZpY2UgKnBkZXYpCj4g
IHsKPiArCXN0cnVjdCBtYXg2NjV4X2RldiAqbWF4NjY1eCA9IGRldl9nZXRfZHJ2ZGF0YShwZGV2
LT5kZXYucGFyZW50KTsKPiAgCXN0cnVjdCBtYXg2NjUwX2RhdGEgKmRhdGE7Cj4gKwljb25zdCBz
dHJ1Y3QgcGxhdGZvcm1fZGV2aWNlX2lkICppZCA9IHBsYXRmb3JtX2dldF9kZXZpY2VfaWQocGRl
dik7CgpXaGF0J3MgdGhlIHBvaW50IGluICd0eXBlJyBpbiB0aGUgZ2xvYmFsIGNvbnRhaW5lcj8K
Ckl0J3MgbG9va2luZyBhcyB0aG91Z2ggeW91J3JlIG5vdCBnb2luZyB0byBuZWVkIGl0IHRvIGJl
IGdsb2JhbCBhZnRlcgphbGwsIHJpZ2h0PwoKPHNuaXA+CgotLSAKTGVlIEpvbmVzCkxpbmFybyBT
VE1pY3JvZWxlY3Ryb25pY3MgTGFuZGluZyBUZWFtIExlYWQKTGluYXJvLm9yZyDilIIgT3BlbiBz
b3VyY2Ugc29mdHdhcmUgZm9yIEFSTSBTb0NzCkZvbGxvdyBMaW5hcm86IEZhY2Vib29rIHwgVHdp
dHRlciB8IEJsb2cKCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f
X19fCmxtLXNlbnNvcnMgbWFpbGluZyBsaXN0CmxtLXNlbnNvcnNAbG0tc2Vuc29ycy5vcmcKaHR0
cDovL2xpc3RzLmxtLXNlbnNvcnMub3JnL21haWxtYW4vbGlzdGluZm8vbG0tc2Vuc29ycw=

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Jean Delvare]

On Thu, 13 Feb 2014 09:58:17 +0000, Lee Jones wrote:
> > The MFD driver has now been added, so this driver is now being adopted to be a
> > subdevice driver on top of it. This means, the i2c driver usage is being
> > converted to platform driver usage all around.
> > 
> > Signed-off-by: Laszlo Papp <lpapp@kde.org>
> > ---
> > This patch has been compile tested only and will be tested with real hardware,
> > but early reviews to catch any trivial issues would be welcome.
> >  drivers/hwmon/Kconfig   |   2 +-
> >  drivers/hwmon/max6650.c | 155 ++++++++++++++++++++++++------------------------
> >  2 files changed, 79 insertions(+), 78 deletions(-)
> 
> <snip>
> 
> >  /*
> >   * Insmod parameters
> > @@ -105,24 +108,23 @@ module_param(clock, int, S_IRUGO);
> >  
> >  #define DIV_FROM_REG(reg) (1 << (reg & 7))
> >  
> > -static int max6650_probe(struct i2c_client *client,
> > -			 const struct i2c_device_id *id);
> > -static int max6650_init_client(struct i2c_client *client);
> > -static int max6650_remove(struct i2c_client *client);
> > +static int max6650_probe(struct platform_device *pdev);
> > +static int max6650_init_client(struct platform_device *pdev);
> > +static int max6650_remove(struct platform_device *pdev);
> >  static struct max6650_data *max6650_update_device(struct device *dev);
> 
> It would be good to remove these forward declarations in the future.
> 
> If no one volunteers I'll happily do it.

Guenter just did:

http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041224.html

Any change to the max6650 driver should go on top of his patch series
to avoid conflicts:

http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html

-- 
Jean Delvare
Suse L3 Support

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Jean Delvare]

On Thu, 13 Feb 2014 09:58:17 +0000, Lee Jones wrote:
> > The MFD driver has now been added, so this driver is now being adopted to be a
> > subdevice driver on top of it. This means, the i2c driver usage is being
> > converted to platform driver usage all around.
> > 
> > Signed-off-by: Laszlo Papp <lpapp@kde.org>
> > ---
> > This patch has been compile tested only and will be tested with real hardware,
> > but early reviews to catch any trivial issues would be welcome.
> >  drivers/hwmon/Kconfig   |   2 +-
> >  drivers/hwmon/max6650.c | 155 ++++++++++++++++++++++++------------------------
> >  2 files changed, 79 insertions(+), 78 deletions(-)
> 
> <snip>
> 
> >  /*
> >   * Insmod parameters
> > @@ -105,24 +108,23 @@ module_param(clock, int, S_IRUGO);
> >  
> >  #define DIV_FROM_REG(reg) (1 << (reg & 7))
> >  
> > -static int max6650_probe(struct i2c_client *client,
> > -			 const struct i2c_device_id *id);
> > -static int max6650_init_client(struct i2c_client *client);
> > -static int max6650_remove(struct i2c_client *client);
> > +static int max6650_probe(struct platform_device *pdev);
> > +static int max6650_init_client(struct platform_device *pdev);
> > +static int max6650_remove(struct platform_device *pdev);
> >  static struct max6650_data *max6650_update_device(struct device *dev);
> 
> It would be good to remove these forward declarations in the future.
> 
> If no one volunteers I'll happily do it.

Guenter just did:

http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041224.html

Any change to the max6650 driver should go on top of his patch series
to avoid conflicts:

http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html

-- 
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] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Laszlo Papp]

On Thu, Feb 13, 2014 at 10:15 AM, Jean Delvare <jdelvare@suse.de> wrote:
> On Thu, 13 Feb 2014 09:58:17 +0000, Lee Jones wrote:
>> > The MFD driver has now been added, so this driver is now being adopted to be a
>> > subdevice driver on top of it. This means, the i2c driver usage is being
>> > converted to platform driver usage all around.
>> >
>> > Signed-off-by: Laszlo Papp <lpapp@kde.org>
>> > ---
>> > This patch has been compile tested only and will be tested with real hardware,
>> > but early reviews to catch any trivial issues would be welcome.
>> >  drivers/hwmon/Kconfig   |   2 +-
>> >  drivers/hwmon/max6650.c | 155 ++++++++++++++++++++++++------------------------
>> >  2 files changed, 79 insertions(+), 78 deletions(-)
>>
>> <snip>
>>
>> >  /*
>> >   * Insmod parameters
>> > @@ -105,24 +108,23 @@ module_param(clock, int, S_IRUGO);
>> >
>> >  #define DIV_FROM_REG(reg) (1 << (reg & 7))
>> >
>> > -static int max6650_probe(struct i2c_client *client,
>> > -                    const struct i2c_device_id *id);
>> > -static int max6650_init_client(struct i2c_client *client);
>> > -static int max6650_remove(struct i2c_client *client);
>> > +static int max6650_probe(struct platform_device *pdev);
>> > +static int max6650_init_client(struct platform_device *pdev);
>> > +static int max6650_remove(struct platform_device *pdev);
>> >  static struct max6650_data *max6650_update_device(struct device *dev);
>>
>> It would be good to remove these forward declarations in the future.
>>
>> If no one volunteers I'll happily do it.
>
> Guenter just did:
>
> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041224.html
>
> Any change to the max6650 driver should go on top of his patch series
> to avoid conflicts:
>
> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html

As far as I can see, that patch set was not even tested, so how can it
go in? I was told that any patch should be _runtime_ tested, too.
Fwiw, I do not have time to test those personally, he would need to
find someone else if that requirement really holds true.

I would not really like to fix bugs appearing in that code to get my
features in.

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Laszlo Papp]

On Thu, Feb 13, 2014 at 10:15 AM, Jean Delvare <jdelvare@suse.de> wrote:
> On Thu, 13 Feb 2014 09:58:17 +0000, Lee Jones wrote:
>> > The MFD driver has now been added, so this driver is now being adopted to be a
>> > subdevice driver on top of it. This means, the i2c driver usage is being
>> > converted to platform driver usage all around.
>> >
>> > Signed-off-by: Laszlo Papp <lpapp@kde.org>
>> > ---
>> > This patch has been compile tested only and will be tested with real hardware,
>> > but early reviews to catch any trivial issues would be welcome.
>> >  drivers/hwmon/Kconfig   |   2 +-
>> >  drivers/hwmon/max6650.c | 155 ++++++++++++++++++++++++------------------------
>> >  2 files changed, 79 insertions(+), 78 deletions(-)
>>
>> <snip>
>>
>> >  /*
>> >   * Insmod parameters
>> > @@ -105,24 +108,23 @@ module_param(clock, int, S_IRUGO);
>> >
>> >  #define DIV_FROM_REG(reg) (1 << (reg & 7))
>> >
>> > -static int max6650_probe(struct i2c_client *client,
>> > -                    const struct i2c_device_id *id);
>> > -static int max6650_init_client(struct i2c_client *client);
>> > -static int max6650_remove(struct i2c_client *client);
>> > +static int max6650_probe(struct platform_device *pdev);
>> > +static int max6650_init_client(struct platform_device *pdev);
>> > +static int max6650_remove(struct platform_device *pdev);
>> >  static struct max6650_data *max6650_update_device(struct device *dev);
>>
>> It would be good to remove these forward declarations in the future.
>>
>> If no one volunteers I'll happily do it.
>
> Guenter just did:
>
> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041224.html
>
> Any change to the max6650 driver should go on top of his patch series
> to avoid conflicts:
>
> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html

As far as I can see, that patch set was not even tested, so how can it
go in? I was told that any patch should be _runtime_ tested, too.
Fwiw, I do not have time to test those personally, he would need to
find someone else if that requirement really holds true.

I would not really like to fix bugs appearing in that code to get my
features in.

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

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Laszlo Papp]

On Thu, Feb 13, 2014 at 10:38 AM, Laszlo Papp <lpapp@kde.org> wrote:
> On Thu, Feb 13, 2014 at 10:15 AM, Jean Delvare <jdelvare@suse.de> wrote:
>> On Thu, 13 Feb 2014 09:58:17 +0000, Lee Jones wrote:
>>> > The MFD driver has now been added, so this driver is now being adopted to be a
>>> > subdevice driver on top of it. This means, the i2c driver usage is being
>>> > converted to platform driver usage all around.
>>> >
>>> > Signed-off-by: Laszlo Papp <lpapp@kde.org>
>>> > ---
>>> > This patch has been compile tested only and will be tested with real hardware,
>>> > but early reviews to catch any trivial issues would be welcome.
>>> >  drivers/hwmon/Kconfig   |   2 +-
>>> >  drivers/hwmon/max6650.c | 155 ++++++++++++++++++++++++------------------------
>>> >  2 files changed, 79 insertions(+), 78 deletions(-)
>>>
>>> <snip>
>>>
>>> >  /*
>>> >   * Insmod parameters
>>> > @@ -105,24 +108,23 @@ module_param(clock, int, S_IRUGO);
>>> >
>>> >  #define DIV_FROM_REG(reg) (1 << (reg & 7))
>>> >
>>> > -static int max6650_probe(struct i2c_client *client,
>>> > -                    const struct i2c_device_id *id);
>>> > -static int max6650_init_client(struct i2c_client *client);
>>> > -static int max6650_remove(struct i2c_client *client);
>>> > +static int max6650_probe(struct platform_device *pdev);
>>> > +static int max6650_init_client(struct platform_device *pdev);
>>> > +static int max6650_remove(struct platform_device *pdev);
>>> >  static struct max6650_data *max6650_update_device(struct device *dev);
>>>
>>> It would be good to remove these forward declarations in the future.
>>>
>>> If no one volunteers I'll happily do it.
>>
>> Guenter just did:
>>
>> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041224.html
>>
>> Any change to the max6650 driver should go on top of his patch series
>> to avoid conflicts:
>>
>> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html
>
> As far as I can see, that patch set was not even tested, so how can it
> go in? I was told that any patch should be _runtime_ tested, too.
> Fwiw, I do not have time to test those personally, he would need to
> find someone else if that requirement really holds true.
>
> I would not really like to fix bugs appearing in that code to get my
> features in.

Also, since my change has been around for 2-3 months now, I would
really prefer not to be forced to rewrite it again from scratch.
Surely, you can wait with those, more or less, cosmetic non-runtime
tested changes?

This would impose me a lot of additional work again, and I personally
do not see the benefit of it. In my book at least, feature is over
internal polishing.

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Laszlo Papp]

On Thu, Feb 13, 2014 at 10:38 AM, Laszlo Papp <lpapp@kde.org> wrote:
> On Thu, Feb 13, 2014 at 10:15 AM, Jean Delvare <jdelvare@suse.de> wrote:
>> On Thu, 13 Feb 2014 09:58:17 +0000, Lee Jones wrote:
>>> > The MFD driver has now been added, so this driver is now being adopted to be a
>>> > subdevice driver on top of it. This means, the i2c driver usage is being
>>> > converted to platform driver usage all around.
>>> >
>>> > Signed-off-by: Laszlo Papp <lpapp@kde.org>
>>> > ---
>>> > This patch has been compile tested only and will be tested with real hardware,
>>> > but early reviews to catch any trivial issues would be welcome.
>>> >  drivers/hwmon/Kconfig   |   2 +-
>>> >  drivers/hwmon/max6650.c | 155 ++++++++++++++++++++++++------------------------
>>> >  2 files changed, 79 insertions(+), 78 deletions(-)
>>>
>>> <snip>
>>>
>>> >  /*
>>> >   * Insmod parameters
>>> > @@ -105,24 +108,23 @@ module_param(clock, int, S_IRUGO);
>>> >
>>> >  #define DIV_FROM_REG(reg) (1 << (reg & 7))
>>> >
>>> > -static int max6650_probe(struct i2c_client *client,
>>> > -                    const struct i2c_device_id *id);
>>> > -static int max6650_init_client(struct i2c_client *client);
>>> > -static int max6650_remove(struct i2c_client *client);
>>> > +static int max6650_probe(struct platform_device *pdev);
>>> > +static int max6650_init_client(struct platform_device *pdev);
>>> > +static int max6650_remove(struct platform_device *pdev);
>>> >  static struct max6650_data *max6650_update_device(struct device *dev);
>>>
>>> It would be good to remove these forward declarations in the future.
>>>
>>> If no one volunteers I'll happily do it.
>>
>> Guenter just did:
>>
>> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041224.html
>>
>> Any change to the max6650 driver should go on top of his patch series
>> to avoid conflicts:
>>
>> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html
>
> As far as I can see, that patch set was not even tested, so how can it
> go in? I was told that any patch should be _runtime_ tested, too.
> Fwiw, I do not have time to test those personally, he would need to
> find someone else if that requirement really holds true.
>
> I would not really like to fix bugs appearing in that code to get my
> features in.

Also, since my change has been around for 2-3 months now, I would
really prefer not to be forced to rewrite it again from scratch.
Surely, you can wait with those, more or less, cosmetic non-runtime
tested changes?

This would impose me a lot of additional work again, and I personally
do not see the benefit of it. In my book at least, feature is over
internal polishing.

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

Re: [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Laszlo Papp]

On Thu, Feb 13, 2014 at 9:58 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> The MFD driver has now been added, so this driver is now being adopted to be a
>> subdevice driver on top of it. This means, the i2c driver usage is being
>> converted to platform driver usage all around.
>>
>> Signed-off-by: Laszlo Papp <lpapp@kde.org>
>> ---
>> This patch has been compile tested only and will be tested with real hardware,
>> but early reviews to catch any trivial issues would be welcome.
>>  drivers/hwmon/Kconfig   |   2 +-
>>  drivers/hwmon/max6650.c | 155 ++++++++++++++++++++++++------------------------
>>  2 files changed, 79 insertions(+), 78 deletions(-)
>
> <snip>
>
>>  /*
>>   * Insmod parameters
>> @@ -105,24 +108,23 @@ module_param(clock, int, S_IRUGO);
>>
>>  #define DIV_FROM_REG(reg) (1 << (reg & 7))
>>
>> -static int max6650_probe(struct i2c_client *client,
>> -                      const struct i2c_device_id *id);
>> -static int max6650_init_client(struct i2c_client *client);
>> -static int max6650_remove(struct i2c_client *client);
>> +static int max6650_probe(struct platform_device *pdev);
>> +static int max6650_init_client(struct platform_device *pdev);
>> +static int max6650_remove(struct platform_device *pdev);
>>  static struct max6650_data *max6650_update_device(struct device *dev);
>
> It would be good to remove these forward declarations in the future.
>
> If no one volunteers I'll happily do it.

I personally do not see any problem with the code either way, hence I
would not personally touch what works. :)

But if it is a strong opinionated style restriction in the codebase,
then yeah, someone could do it, except me.

>>  /*
>>   * Driver data (common to all clients)
>>   */
>>
>> -static const struct i2c_device_id max6650_id[] = {
>> +static const struct platform_device_id max6650_id[] = {
>>       { "max6650", 1 },
>>       { "max6651", 4 },
>>       { }
>>  };
>> -MODULE_DEVICE_TABLE(i2c, max6650_id);
>> +MODULE_DEVICE_TABLE(platform, max6650_id);
>
> I thought you were going to do the matching in the MFD driver?
>
> If not, what's the point in the exported 'type' attribute?

I am yet to understand the concept here. You were objecting to those,
so I removed this in MFD. I could add it to that back in this patch as
proposed.

>> -static struct i2c_driver max6650_driver = {
>> +static struct platform_driver max6650_driver = {
>>       .driver = {
>>               .name   = "max6650",
>
> This should be changed as it now supports max665{0,1} right?

This is a strange historical driver. It has always supported both, yet
it was named max6650 weirdly enough as you note.

> <snip>
>
>> -     i2c_smbus_write_byte_data(client, MAX6650_REG_SPEED, data->speed);
>> +     regmap_write(data->iodev->map, MAX6650_REG_SPEED, data->speed);
>
> Ensure all of the regmap stuff is fully tested.

Yes, sure.

> <snip>
>
>> @@ -484,10 +482,12 @@ static umode_t max6650_attrs_visible(struct kobject *kobj, struct attribute *a,
>>                                   int n)
>>  {
>>       struct device *dev = container_of(kobj, struct device, kobj);
>> -     struct i2c_client *client = to_i2c_client(dev);
>> -     u8 alarm_en = i2c_smbus_read_byte_data(client, MAX6650_REG_ALARM_EN);
>> +     struct max6650_data *data = dev_get_drvdata(dev);
>> +     int alarm_en;
>>       struct device_attribute *devattr;
>>
>> +     regmap_read(data->iodev->map, MAX6650_REG_ALARM_EN, &alarm_en);
>> +
>
> Where is this then used?

Nowhere, so it was a sub-optimal situation in the old code. It is just
a direct platform device port of it whatever it was. Perhaps it is the
right time to clean it a bit, I agree.

>> -static int max6650_probe(struct i2c_client *client,
>> -                      const struct i2c_device_id *id)
>> +static int max6650_probe(struct platform_device *pdev)
>>  {
>> +     struct max665x_dev *max665x = dev_get_drvdata(pdev->dev.parent);
>>       struct max6650_data *data;
>> +     const struct platform_device_id *id = platform_get_device_id(pdev);
>
> What's the point in 'type' in the global container?
>
> It's looking as though you're not going to need it to be global after
> all, right?

I would need it for the bit fiddling, too, I think.

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Laszlo Papp]

On Thu, Feb 13, 2014 at 9:58 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> The MFD driver has now been added, so this driver is now being adopted to be a
>> subdevice driver on top of it. This means, the i2c driver usage is being
>> converted to platform driver usage all around.
>>
>> Signed-off-by: Laszlo Papp <lpapp@kde.org>
>> ---
>> This patch has been compile tested only and will be tested with real hardware,
>> but early reviews to catch any trivial issues would be welcome.
>>  drivers/hwmon/Kconfig   |   2 +-
>>  drivers/hwmon/max6650.c | 155 ++++++++++++++++++++++++------------------------
>>  2 files changed, 79 insertions(+), 78 deletions(-)
>
> <snip>
>
>>  /*
>>   * Insmod parameters
>> @@ -105,24 +108,23 @@ module_param(clock, int, S_IRUGO);
>>
>>  #define DIV_FROM_REG(reg) (1 << (reg & 7))
>>
>> -static int max6650_probe(struct i2c_client *client,
>> -                      const struct i2c_device_id *id);
>> -static int max6650_init_client(struct i2c_client *client);
>> -static int max6650_remove(struct i2c_client *client);
>> +static int max6650_probe(struct platform_device *pdev);
>> +static int max6650_init_client(struct platform_device *pdev);
>> +static int max6650_remove(struct platform_device *pdev);
>>  static struct max6650_data *max6650_update_device(struct device *dev);
>
> It would be good to remove these forward declarations in the future.
>
> If no one volunteers I'll happily do it.

I personally do not see any problem with the code either way, hence I
would not personally touch what works. :)

But if it is a strong opinionated style restriction in the codebase,
then yeah, someone could do it, except me.

>>  /*
>>   * Driver data (common to all clients)
>>   */
>>
>> -static const struct i2c_device_id max6650_id[] = {
>> +static const struct platform_device_id max6650_id[] = {
>>       { "max6650", 1 },
>>       { "max6651", 4 },
>>       { }
>>  };
>> -MODULE_DEVICE_TABLE(i2c, max6650_id);
>> +MODULE_DEVICE_TABLE(platform, max6650_id);
>
> I thought you were going to do the matching in the MFD driver?
>
> If not, what's the point in the exported 'type' attribute?

I am yet to understand the concept here. You were objecting to those,
so I removed this in MFD. I could add it to that back in this patch as
proposed.

>> -static struct i2c_driver max6650_driver = {
>> +static struct platform_driver max6650_driver = {
>>       .driver = {
>>               .name   = "max6650",
>
> This should be changed as it now supports max665{0,1} right?

This is a strange historical driver. It has always supported both, yet
it was named max6650 weirdly enough as you note.

> <snip>
>
>> -     i2c_smbus_write_byte_data(client, MAX6650_REG_SPEED, data->speed);
>> +     regmap_write(data->iodev->map, MAX6650_REG_SPEED, data->speed);
>
> Ensure all of the regmap stuff is fully tested.

Yes, sure.

> <snip>
>
>> @@ -484,10 +482,12 @@ static umode_t max6650_attrs_visible(struct kobject *kobj, struct attribute *a,
>>                                   int n)
>>  {
>>       struct device *dev = container_of(kobj, struct device, kobj);
>> -     struct i2c_client *client = to_i2c_client(dev);
>> -     u8 alarm_en = i2c_smbus_read_byte_data(client, MAX6650_REG_ALARM_EN);
>> +     struct max6650_data *data = dev_get_drvdata(dev);
>> +     int alarm_en;
>>       struct device_attribute *devattr;
>>
>> +     regmap_read(data->iodev->map, MAX6650_REG_ALARM_EN, &alarm_en);
>> +
>
> Where is this then used?

Nowhere, so it was a sub-optimal situation in the old code. It is just
a direct platform device port of it whatever it was. Perhaps it is the
right time to clean it a bit, I agree.

>> -static int max6650_probe(struct i2c_client *client,
>> -                      const struct i2c_device_id *id)
>> +static int max6650_probe(struct platform_device *pdev)
>>  {
>> +     struct max665x_dev *max665x = dev_get_drvdata(pdev->dev.parent);
>>       struct max6650_data *data;
>> +     const struct platform_device_id *id = platform_get_device_id(pdev);
>
> What's the point in 'type' in the global container?
>
> It's looking as though you're not going to need it to be global after
> all, right?

I would need it for the bit fiddling, too, I think.

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

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Jean Delvare]

Hi Laszlo,

Le Thursday 13 February 2014 à 10:46 +0000, Laszlo Papp a écrit :
> On Thu, Feb 13, 2014 at 10:38 AM, Laszlo Papp <lpapp@kde.org> wrote:
> > On Thu, Feb 13, 2014 at 10:15 AM, Jean Delvare <jdelvare@suse.de> wrote:
> >> Any change to the max6650 driver should go on top of his patch series
> >> to avoid conflicts:
> >>
> >> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html
> >
> > As far as I can see, that patch set was not even tested, so how can it
> > go in? I was told that any patch should be _runtime_ tested, too.
> > Fwiw, I do not have time to test those personally, he would need to
> > find someone else if that requirement really holds true.

I find it _very_ funny that you dare to complain here, when you sent a
totally untested patch no later than 2 days ago:

http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041180.html

There's no way that patch can work.

And, actually, Guenter's patches have been reviewed and tested by
myself, to some degree (I don't have access to a physical MAX6650 or
MAX6651 chip so I used an emulation, which I think was good enough given
the nature of the changes.)

> > I would not really like to fix bugs appearing in that code to get my
> > features in.

I have no idea what you mean here.

> Also, since my change has been around for 2-3 months now, I would
> really prefer not to be forced to rewrite it again from scratch.

I'm sure Gunter would have preferred if you could write proper patches
so he wouldn't have to do it himself.

Seriously, nobody here cares about your personal preferences. You said
you want some significant changes done to the max6650 driver, it takes
many steps to get there, either you take them, or you can leave right
now. If you're not going to listen to (and subsequently obey) people who
have been working on this project for years and are well-known and
respected by the vast majority of their peers, then bye bye.

> Surely, you can wait with those, more or less, cosmetic non-runtime
> tested changes?

I see you once again failed to read (or understand) something I repeated
many times already. One of these changes (the one moving the hwmon
attributes from i2c device to hwmon device) is _mandatory_ to get your
own changes accepted. Guenter did you an immense favor by writing these
patches, so if anything you should be very grateful to him.

> This would impose me a lot of additional work again, and I personally
> do not see the benefit of it. In my book at least, feature is over
> internal polishing.

Change books then, yours is just wrong. Bug fixes come first, then
cleanups, then features. Adding features on top of ugly code is a pain
for everyone. Plus cleaning up the code helps you to understand it, so
I'd say this is time well invested. You should try, that would certainly
help you avoid some mistakes you did in the past.

I would like to add a more general comment on the way you behave with
the community and that has been bothering me for days. You apparently
act first and think second. I can no longer count the number of times
you replied to a post only to reply to yourself a few minutes later.
Last occurrence of this is in this thread: first reply from you at
11:38, and then an addition at 11:46, i.e. 8 minutes later. And you do
that all the time.

And it holds for patches too, for example.
http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041179.html
posted at 11:24, then
http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041180.html
v2 of the same posted at 11:28.

So please listen to this piece of advice: take your time. Think more,
and only act after you have thought thoroughly about everything. It will
save you a lot of trouble, and the community a lot of time.

Thanks,
-- 
Jean Delvare
Suse L3 Support

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Jean Delvare]

SGkgTGFzemxvLAoKTGUgVGh1cnNkYXkgMTMgRmVicnVhcnkgMjAxNCDDoCAxMDo0NiArMDAwMCwg
TGFzemxvIFBhcHAgYSDDqWNyaXQgOgo+IE9uIFRodSwgRmViIDEzLCAyMDE0IGF0IDEwOjM4IEFN
LCBMYXN6bG8gUGFwcCA8bHBhcHBAa2RlLm9yZz4gd3JvdGU6Cj4gPiBPbiBUaHUsIEZlYiAxMywg
MjAxNCBhdCAxMDoxNSBBTSwgSmVhbiBEZWx2YXJlIDxqZGVsdmFyZUBzdXNlLmRlPiB3cm90ZToK
PiA+PiBBbnkgY2hhbmdlIHRvIHRoZSBtYXg2NjUwIGRyaXZlciBzaG91bGQgZ28gb24gdG9wIG9m
IGhpcyBwYXRjaCBzZXJpZXMKPiA+PiB0byBhdm9pZCBjb25mbGljdHM6Cj4gPj4KPiA+PiBodHRw
Oi8vbGlzdHMubG0tc2Vuc29ycy5vcmcvcGlwZXJtYWlsL2xtLXNlbnNvcnMvMjAxNC1GZWJydWFy
eS8wNDEyMjMuaHRtbAo+ID4KPiA+IEFzIGZhciBhcyBJIGNhbiBzZWUsIHRoYXQgcGF0Y2ggc2V0
IHdhcyBub3QgZXZlbiB0ZXN0ZWQsIHNvIGhvdyBjYW4gaXQKPiA+IGdvIGluPyBJIHdhcyB0b2xk
IHRoYXQgYW55IHBhdGNoIHNob3VsZCBiZSBfcnVudGltZV8gdGVzdGVkLCB0b28uCj4gPiBGd2l3
LCBJIGRvIG5vdCBoYXZlIHRpbWUgdG8gdGVzdCB0aG9zZSBwZXJzb25hbGx5LCBoZSB3b3VsZCBu
ZWVkIHRvCj4gPiBmaW5kIHNvbWVvbmUgZWxzZSBpZiB0aGF0IHJlcXVpcmVtZW50IHJlYWxseSBo
b2xkcyB0cnVlLgoKSSBmaW5kIGl0IF92ZXJ5XyBmdW5ueSB0aGF0IHlvdSBkYXJlIHRvIGNvbXBs
YWluIGhlcmUsIHdoZW4geW91IHNlbnQgYQp0b3RhbGx5IHVudGVzdGVkIHBhdGNoIG5vIGxhdGVy
IHRoYW4gMiBkYXlzIGFnbzoKCmh0dHA6Ly9saXN0cy5sbS1zZW5zb3JzLm9yZy9waXBlcm1haWwv
bG0tc2Vuc29ycy8yMDE0LUZlYnJ1YXJ5LzA0MTE4MC5odG1sCgpUaGVyZSdzIG5vIHdheSB0aGF0
IHBhdGNoIGNhbiB3b3JrLgoKQW5kLCBhY3R1YWxseSwgR3VlbnRlcidzIHBhdGNoZXMgaGF2ZSBi
ZWVuIHJldmlld2VkIGFuZCB0ZXN0ZWQgYnkKbXlzZWxmLCB0byBzb21lIGRlZ3JlZSAoSSBkb24n
dCBoYXZlIGFjY2VzcyB0byBhIHBoeXNpY2FsIE1BWDY2NTAgb3IKTUFYNjY1MSBjaGlwIHNvIEkg
dXNlZCBhbiBlbXVsYXRpb24sIHdoaWNoIEkgdGhpbmsgd2FzIGdvb2QgZW5vdWdoIGdpdmVuCnRo
ZSBuYXR1cmUgb2YgdGhlIGNoYW5nZXMuKQoKPiA+IEkgd291bGQgbm90IHJlYWxseSBsaWtlIHRv
IGZpeCBidWdzIGFwcGVhcmluZyBpbiB0aGF0IGNvZGUgdG8gZ2V0IG15Cj4gPiBmZWF0dXJlcyBp
bi4KCkkgaGF2ZSBubyBpZGVhIHdoYXQgeW91IG1lYW4gaGVyZS4KCj4gQWxzbywgc2luY2UgbXkg
Y2hhbmdlIGhhcyBiZWVuIGFyb3VuZCBmb3IgMi0zIG1vbnRocyBub3csIEkgd291bGQKPiByZWFs
bHkgcHJlZmVyIG5vdCB0byBiZSBmb3JjZWQgdG8gcmV3cml0ZSBpdCBhZ2FpbiBmcm9tIHNjcmF0
Y2guCgpJJ20gc3VyZSBHdW50ZXIgd291bGQgaGF2ZSBwcmVmZXJyZWQgaWYgeW91IGNvdWxkIHdy
aXRlIHByb3BlciBwYXRjaGVzCnNvIGhlIHdvdWxkbid0IGhhdmUgdG8gZG8gaXQgaGltc2VsZi4K
ClNlcmlvdXNseSwgbm9ib2R5IGhlcmUgY2FyZXMgYWJvdXQgeW91ciBwZXJzb25hbCBwcmVmZXJl
bmNlcy4gWW91IHNhaWQKeW91IHdhbnQgc29tZSBzaWduaWZpY2FudCBjaGFuZ2VzIGRvbmUgdG8g
dGhlIG1heDY2NTAgZHJpdmVyLCBpdCB0YWtlcwptYW55IHN0ZXBzIHRvIGdldCB0aGVyZSwgZWl0
aGVyIHlvdSB0YWtlIHRoZW0sIG9yIHlvdSBjYW4gbGVhdmUgcmlnaHQKbm93LiBJZiB5b3UncmUg
bm90IGdvaW5nIHRvIGxpc3RlbiB0byAoYW5kIHN1YnNlcXVlbnRseSBvYmV5KSBwZW9wbGUgd2hv
CmhhdmUgYmVlbiB3b3JraW5nIG9uIHRoaXMgcHJvamVjdCBmb3IgeWVhcnMgYW5kIGFyZSB3ZWxs
LWtub3duIGFuZApyZXNwZWN0ZWQgYnkgdGhlIHZhc3QgbWFqb3JpdHkgb2YgdGhlaXIgcGVlcnMs
IHRoZW4gYnllIGJ5ZS4KCj4gU3VyZWx5LCB5b3UgY2FuIHdhaXQgd2l0aCB0aG9zZSwgbW9yZSBv
ciBsZXNzLCBjb3NtZXRpYyBub24tcnVudGltZQo+IHRlc3RlZCBjaGFuZ2VzPwoKSSBzZWUgeW91
IG9uY2UgYWdhaW4gZmFpbGVkIHRvIHJlYWQgKG9yIHVuZGVyc3RhbmQpIHNvbWV0aGluZyBJIHJl
cGVhdGVkCm1hbnkgdGltZXMgYWxyZWFkeS4gT25lIG9mIHRoZXNlIGNoYW5nZXMgKHRoZSBvbmUg
bW92aW5nIHRoZSBod21vbgphdHRyaWJ1dGVzIGZyb20gaTJjIGRldmljZSB0byBod21vbiBkZXZp
Y2UpIGlzIF9tYW5kYXRvcnlfIHRvIGdldCB5b3VyCm93biBjaGFuZ2VzIGFjY2VwdGVkLiBHdWVu
dGVyIGRpZCB5b3UgYW4gaW1tZW5zZSBmYXZvciBieSB3cml0aW5nIHRoZXNlCnBhdGNoZXMsIHNv
IGlmIGFueXRoaW5nIHlvdSBzaG91bGQgYmUgdmVyeSBncmF0ZWZ1bCB0byBoaW0uCgo+IFRoaXMg
d291bGQgaW1wb3NlIG1lIGEgbG90IG9mIGFkZGl0aW9uYWwgd29yayBhZ2FpbiwgYW5kIEkgcGVy
c29uYWxseQo+IGRvIG5vdCBzZWUgdGhlIGJlbmVmaXQgb2YgaXQuIEluIG15IGJvb2sgYXQgbGVh
c3QsIGZlYXR1cmUgaXMgb3Zlcgo+IGludGVybmFsIHBvbGlzaGluZy4KCkNoYW5nZSBib29rcyB0
aGVuLCB5b3VycyBpcyBqdXN0IHdyb25nLiBCdWcgZml4ZXMgY29tZSBmaXJzdCwgdGhlbgpjbGVh
bnVwcywgdGhlbiBmZWF0dXJlcy4gQWRkaW5nIGZlYXR1cmVzIG9uIHRvcCBvZiB1Z2x5IGNvZGUg
aXMgYSBwYWluCmZvciBldmVyeW9uZS4gUGx1cyBjbGVhbmluZyB1cCB0aGUgY29kZSBoZWxwcyB5
b3UgdG8gdW5kZXJzdGFuZCBpdCwgc28KSSdkIHNheSB0aGlzIGlzIHRpbWUgd2VsbCBpbnZlc3Rl
ZC4gWW91IHNob3VsZCB0cnksIHRoYXQgd291bGQgY2VydGFpbmx5CmhlbHAgeW91IGF2b2lkIHNv
bWUgbWlzdGFrZXMgeW91IGRpZCBpbiB0aGUgcGFzdC4KCkkgd291bGQgbGlrZSB0byBhZGQgYSBt
b3JlIGdlbmVyYWwgY29tbWVudCBvbiB0aGUgd2F5IHlvdSBiZWhhdmUgd2l0aAp0aGUgY29tbXVu
aXR5IGFuZCB0aGF0IGhhcyBiZWVuIGJvdGhlcmluZyBtZSBmb3IgZGF5cy4gWW91IGFwcGFyZW50
bHkKYWN0IGZpcnN0IGFuZCB0aGluayBzZWNvbmQuIEkgY2FuIG5vIGxvbmdlciBjb3VudCB0aGUg
bnVtYmVyIG9mIHRpbWVzCnlvdSByZXBsaWVkIHRvIGEgcG9zdCBvbmx5IHRvIHJlcGx5IHRvIHlv
dXJzZWxmIGEgZmV3IG1pbnV0ZXMgbGF0ZXIuCkxhc3Qgb2NjdXJyZW5jZSBvZiB0aGlzIGlzIGlu
IHRoaXMgdGhyZWFkOiBmaXJzdCByZXBseSBmcm9tIHlvdSBhdAoxMTozOCwgYW5kIHRoZW4gYW4g
YWRkaXRpb24gYXQgMTE6NDYsIGkuZS4gOCBtaW51dGVzIGxhdGVyLiBBbmQgeW91IGRvCnRoYXQg
YWxsIHRoZSB0aW1lLgoKQW5kIGl0IGhvbGRzIGZvciBwYXRjaGVzIHRvbywgZm9yIGV4YW1wbGUu
Cmh0dHA6Ly9saXN0cy5sbS1zZW5zb3JzLm9yZy9waXBlcm1haWwvbG0tc2Vuc29ycy8yMDE0LUZl
YnJ1YXJ5LzA0MTE3OS5odG1sCnBvc3RlZCBhdCAxMToyNCwgdGhlbgpodHRwOi8vbGlzdHMubG0t
c2Vuc29ycy5vcmcvcGlwZXJtYWlsL2xtLXNlbnNvcnMvMjAxNC1GZWJydWFyeS8wNDExODAuaHRt
bAp2MiBvZiB0aGUgc2FtZSBwb3N0ZWQgYXQgMTE6MjguCgpTbyBwbGVhc2UgbGlzdGVuIHRvIHRo
aXMgcGllY2Ugb2YgYWR2aWNlOiB0YWtlIHlvdXIgdGltZS4gVGhpbmsgbW9yZSwKYW5kIG9ubHkg
YWN0IGFmdGVyIHlvdSBoYXZlIHRob3VnaHQgdGhvcm91Z2hseSBhYm91dCBldmVyeXRoaW5nLiBJ
dCB3aWxsCnNhdmUgeW91IGEgbG90IG9mIHRyb3VibGUsIGFuZCB0aGUgY29tbXVuaXR5IGEgbG90
IG9mIHRpbWUuCgpUaGFua3MsCi0tIApKZWFuIERlbHZhcmUKU3VzZSBMMyBTdXBwb3J0CgoKX19f
X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KbG0tc2Vuc29ycyBt
YWlsaW5nIGxpc3QKbG0tc2Vuc29yc0BsbS1zZW5zb3JzLm9yZwpodHRwOi8vbGlzdHMubG0tc2Vu
c29ycy5vcmcvbWFpbG1hbi9saXN0aW5mby9sbS1zZW5zb3Jz

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Lee Jones]

On Thu, 13 Feb 2014, Jean Delvare wrote:

> On Thu, 13 Feb 2014 09:58:17 +0000, Lee Jones wrote:
> > > The MFD driver has now been added, so this driver is now being adopted to be a
> > > subdevice driver on top of it. This means, the i2c driver usage is being
> > > converted to platform driver usage all around.
> > > 
> > > Signed-off-by: Laszlo Papp <lpapp@kde.org>
> > > ---
> > > This patch has been compile tested only and will be tested with real hardware,
> > > but early reviews to catch any trivial issues would be welcome.
> > >  drivers/hwmon/Kconfig   |   2 +-
> > >  drivers/hwmon/max6650.c | 155 ++++++++++++++++++++++++------------------------
> > >  2 files changed, 79 insertions(+), 78 deletions(-)
> > 
> > <snip>
> > 
> > >  /*
> > >   * Insmod parameters
> > > @@ -105,24 +108,23 @@ module_param(clock, int, S_IRUGO);
> > >  
> > >  #define DIV_FROM_REG(reg) (1 << (reg & 7))
> > >  
> > > -static int max6650_probe(struct i2c_client *client,
> > > -			 const struct i2c_device_id *id);
> > > -static int max6650_init_client(struct i2c_client *client);
> > > -static int max6650_remove(struct i2c_client *client);
> > > +static int max6650_probe(struct platform_device *pdev);
> > > +static int max6650_init_client(struct platform_device *pdev);
> > > +static int max6650_remove(struct platform_device *pdev);
> > >  static struct max6650_data *max6650_update_device(struct device *dev);
> > 
> > It would be good to remove these forward declarations in the future.
> > 
> > If no one volunteers I'll happily do it.
> 
> Guenter just did:
> 
> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041224.html

Nice, FWIW:
  Acked-by: Lee Jones <lee.jones@linaro.org>  

> Any change to the max6650 driver should go on top of his patch series
> to avoid conflicts:
> 
> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html

Do you have a tree Laszlo can rebase on top of?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Lee Jones]

T24gVGh1LCAxMyBGZWIgMjAxNCwgSmVhbiBEZWx2YXJlIHdyb3RlOgoKPiBPbiBUaHUsIDEzIEZl
YiAyMDE0IDA5OjU4OjE3ICswMDAwLCBMZWUgSm9uZXMgd3JvdGU6Cj4gPiA+IFRoZSBNRkQgZHJp
dmVyIGhhcyBub3cgYmVlbiBhZGRlZCwgc28gdGhpcyBkcml2ZXIgaXMgbm93IGJlaW5nIGFkb3B0
ZWQgdG8gYmUgYQo+ID4gPiBzdWJkZXZpY2UgZHJpdmVyIG9uIHRvcCBvZiBpdC4gVGhpcyBtZWFu
cywgdGhlIGkyYyBkcml2ZXIgdXNhZ2UgaXMgYmVpbmcKPiA+ID4gY29udmVydGVkIHRvIHBsYXRm
b3JtIGRyaXZlciB1c2FnZSBhbGwgYXJvdW5kLgo+ID4gPiAKPiA+ID4gU2lnbmVkLW9mZi1ieTog
TGFzemxvIFBhcHAgPGxwYXBwQGtkZS5vcmc+Cj4gPiA+IC0tLQo+ID4gPiBUaGlzIHBhdGNoIGhh
cyBiZWVuIGNvbXBpbGUgdGVzdGVkIG9ubHkgYW5kIHdpbGwgYmUgdGVzdGVkIHdpdGggcmVhbCBo
YXJkd2FyZSwKPiA+ID4gYnV0IGVhcmx5IHJldmlld3MgdG8gY2F0Y2ggYW55IHRyaXZpYWwgaXNz
dWVzIHdvdWxkIGJlIHdlbGNvbWUuCj4gPiA+ICBkcml2ZXJzL2h3bW9uL0tjb25maWcgICB8ICAg
MiArLQo+ID4gPiAgZHJpdmVycy9od21vbi9tYXg2NjUwLmMgfCAxNTUgKysrKysrKysrKysrKysr
KysrKysrKysrLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tCj4gPiA+ICAyIGZpbGVzIGNoYW5nZWQs
IDc5IGluc2VydGlvbnMoKyksIDc4IGRlbGV0aW9ucygtKQo+ID4gCj4gPiA8c25pcD4KPiA+IAo+
ID4gPiAgLyoKPiA+ID4gICAqIEluc21vZCBwYXJhbWV0ZXJzCj4gPiA+IEBAIC0xMDUsMjQgKzEw
OCwyMyBAQCBtb2R1bGVfcGFyYW0oY2xvY2ssIGludCwgU19JUlVHTyk7Cj4gPiA+ICAKPiA+ID4g
ICNkZWZpbmUgRElWX0ZST01fUkVHKHJlZykgKDEgPDwgKHJlZyAmIDcpKQo+ID4gPiAgCj4gPiA+
IC1zdGF0aWMgaW50IG1heDY2NTBfcHJvYmUoc3RydWN0IGkyY19jbGllbnQgKmNsaWVudCwKPiA+
ID4gLQkJCSBjb25zdCBzdHJ1Y3QgaTJjX2RldmljZV9pZCAqaWQpOwo+ID4gPiAtc3RhdGljIGlu
dCBtYXg2NjUwX2luaXRfY2xpZW50KHN0cnVjdCBpMmNfY2xpZW50ICpjbGllbnQpOwo+ID4gPiAt
c3RhdGljIGludCBtYXg2NjUwX3JlbW92ZShzdHJ1Y3QgaTJjX2NsaWVudCAqY2xpZW50KTsKPiA+
ID4gK3N0YXRpYyBpbnQgbWF4NjY1MF9wcm9iZShzdHJ1Y3QgcGxhdGZvcm1fZGV2aWNlICpwZGV2
KTsKPiA+ID4gK3N0YXRpYyBpbnQgbWF4NjY1MF9pbml0X2NsaWVudChzdHJ1Y3QgcGxhdGZvcm1f
ZGV2aWNlICpwZGV2KTsKPiA+ID4gK3N0YXRpYyBpbnQgbWF4NjY1MF9yZW1vdmUoc3RydWN0IHBs
YXRmb3JtX2RldmljZSAqcGRldik7Cj4gPiA+ICBzdGF0aWMgc3RydWN0IG1heDY2NTBfZGF0YSAq
bWF4NjY1MF91cGRhdGVfZGV2aWNlKHN0cnVjdCBkZXZpY2UgKmRldik7Cj4gPiAKPiA+IEl0IHdv
dWxkIGJlIGdvb2QgdG8gcmVtb3ZlIHRoZXNlIGZvcndhcmQgZGVjbGFyYXRpb25zIGluIHRoZSBm
dXR1cmUuCj4gPiAKPiA+IElmIG5vIG9uZSB2b2x1bnRlZXJzIEknbGwgaGFwcGlseSBkbyBpdC4K
PiAKPiBHdWVudGVyIGp1c3QgZGlkOgo+IAo+IGh0dHA6Ly9saXN0cy5sbS1zZW5zb3JzLm9yZy9w
aXBlcm1haWwvbG0tc2Vuc29ycy8yMDE0LUZlYnJ1YXJ5LzA0MTIyNC5odG1sCgpOaWNlLCBGV0lX
OgogIEFja2VkLWJ5OiBMZWUgSm9uZXMgPGxlZS5qb25lc0BsaW5hcm8ub3JnPiAgCgo+IEFueSBj
aGFuZ2UgdG8gdGhlIG1heDY2NTAgZHJpdmVyIHNob3VsZCBnbyBvbiB0b3Agb2YgaGlzIHBhdGNo
IHNlcmllcwo+IHRvIGF2b2lkIGNvbmZsaWN0czoKPiAKPiBodHRwOi8vbGlzdHMubG0tc2Vuc29y
cy5vcmcvcGlwZXJtYWlsL2xtLXNlbnNvcnMvMjAxNC1GZWJydWFyeS8wNDEyMjMuaHRtbAoKRG8g
eW91IGhhdmUgYSB0cmVlIExhc3psbyBjYW4gcmViYXNlIG9uIHRvcCBvZj8KCi0tIApMZWUgSm9u
ZXMKTGluYXJvIFNUTWljcm9lbGVjdHJvbmljcyBMYW5kaW5nIFRlYW0gTGVhZApMaW5hcm8ub3Jn
IOKUgiBPcGVuIHNvdXJjZSBzb2Z0d2FyZSBmb3IgQVJNIFNvQ3MKRm9sbG93IExpbmFybzogRmFj
ZWJvb2sgfCBUd2l0dGVyIHwgQmxvZwoKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f
X19fX19fX19fX19fX18KbG0tc2Vuc29ycyBtYWlsaW5nIGxpc3QKbG0tc2Vuc29yc0BsbS1zZW5z
b3JzLm9yZwpodHRwOi8vbGlzdHMubG0tc2Vuc29ycy5vcmcvbWFpbG1hbi9saXN0aW5mby9sbS1z
ZW5zb3Jz

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Laszlo Papp]

I will try hard to concentrate on the technical and fruitful stuff in
the reply...

On Thu, Feb 13, 2014 at 11:07 AM, Jean Delvare <jdelvare@suse.de> wrote:
> Hi Laszlo,
>
> Le Thursday 13 February 2014 à 10:46 +0000, Laszlo Papp a écrit :
>> On Thu, Feb 13, 2014 at 10:38 AM, Laszlo Papp <lpapp@kde.org> wrote:
>> > On Thu, Feb 13, 2014 at 10:15 AM, Jean Delvare <jdelvare@suse.de> wrote:
>> >> Any change to the max6650 driver should go on top of his patch series
>> >> to avoid conflicts:
>> >>
>> >> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html
>> >
>> > As far as I can see, that patch set was not even tested, so how can it
>> > go in? I was told that any patch should be _runtime_ tested, too.
>> > Fwiw, I do not have time to test those personally, he would need to
>> > find someone else if that requirement really holds true.
>
> I find it _very_ funny that you dare to complain here, when you sent a
> totally untested patch no later than 2 days ago:
>
> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041180.html
>
> There's no way that patch can work.
>
> And, actually, Guenter's patches have been reviewed and tested by
> myself, to some degree (I don't have access to a physical MAX6650 or
> MAX6651 chip so I used an emulation, which I think was good enough given
> the nature of the changes.)

If you read the thread carefully, I did test the change several times,
but not after every minor change. I even apologized for my mistake of
not testing after every minor change. I am not sure what I could do
about it, but let me know if you have an idea.

>> > I would not really like to fix bugs appearing in that code to get my
>> > features in.
>
> I have no idea what you mean here.

The fact that the series is quite intrusive, and most of it seems to
be unnecessary for getting the features done. Hence, it can contain
bugs which I potentially need to debug in _real_ hardware environment
for hours or even days.

>> Also, since my change has been around for 2-3 months now, I would
>> really prefer not to be forced to rewrite it again from scratch.
>
> I'm sure Gunter would have preferred if you could write proper patches
> so he wouldn't have to do it himself.

This is not how an open source community works in my opinion, but see below.

> Seriously, nobody here cares about your personal preferences. You said
> you want some significant changes done to the max6650 driver, it takes
> many steps to get there, either you take them, or you can leave right
> now. If you're not going to listen to (and subsequently obey) people who
> have been working on this project for years and are well-known and
> respected by the vast majority of their peers, then bye bye.
>
>> Surely, you can wait with those, more or less, cosmetic non-runtime
>> tested changes?
>
> I see you once again failed to read (or understand) something I repeated
> many times already. One of these changes (the one moving the hwmon
> attributes from i2c device to hwmon device) is _mandatory_ to get your
> own changes accepted. Guenter did you an immense favor by writing these
> patches, so if anything you should be very grateful to him.

I submitted a change for trying to address the _real_ issue a couple
of days ago _right after_ the suggestion. You can check the times, it
was literally within a couple of hours. Then, someone submits the same
with better content. This is what I meant by not how a community
works. You can leave comments for others, and then they can increase
their expertise.

The patch set is very likely to contain bugs because it is a
significant re-factoring, whereas I focused on as minimal changes as
possible to get the feature reliably in.

Please help me to be involved, and not excluded. It will be hard for
me to feel helpful, and the inherent consequence is that my project
will not work with the kernel project which, I think, we both would
like to avoid.

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Laszlo Papp]

I will try hard to concentrate on the technical and fruitful stuff in
the reply...

On Thu, Feb 13, 2014 at 11:07 AM, Jean Delvare <jdelvare@suse.de> wrote:
> Hi Laszlo,
>
> Le Thursday 13 February 2014 à 10:46 +0000, Laszlo Papp a écrit :
>> On Thu, Feb 13, 2014 at 10:38 AM, Laszlo Papp <lpapp@kde.org> wrote:
>> > On Thu, Feb 13, 2014 at 10:15 AM, Jean Delvare <jdelvare@suse.de> wrote:
>> >> Any change to the max6650 driver should go on top of his patch series
>> >> to avoid conflicts:
>> >>
>> >> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html
>> >
>> > As far as I can see, that patch set was not even tested, so how can it
>> > go in? I was told that any patch should be _runtime_ tested, too.
>> > Fwiw, I do not have time to test those personally, he would need to
>> > find someone else if that requirement really holds true.
>
> I find it _very_ funny that you dare to complain here, when you sent a
> totally untested patch no later than 2 days ago:
>
> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041180.html
>
> There's no way that patch can work.
>
> And, actually, Guenter's patches have been reviewed and tested by
> myself, to some degree (I don't have access to a physical MAX6650 or
> MAX6651 chip so I used an emulation, which I think was good enough given
> the nature of the changes.)

If you read the thread carefully, I did test the change several times,
but not after every minor change. I even apologized for my mistake of
not testing after every minor change. I am not sure what I could do
about it, but let me know if you have an idea.

>> > I would not really like to fix bugs appearing in that code to get my
>> > features in.
>
> I have no idea what you mean here.

The fact that the series is quite intrusive, and most of it seems to
be unnecessary for getting the features done. Hence, it can contain
bugs which I potentially need to debug in _real_ hardware environment
for hours or even days.

>> Also, since my change has been around for 2-3 months now, I would
>> really prefer not to be forced to rewrite it again from scratch.
>
> I'm sure Gunter would have preferred if you could write proper patches
> so he wouldn't have to do it himself.

This is not how an open source community works in my opinion, but see below.

> Seriously, nobody here cares about your personal preferences. You said
> you want some significant changes done to the max6650 driver, it takes
> many steps to get there, either you take them, or you can leave right
> now. If you're not going to listen to (and subsequently obey) people who
> have been working on this project for years and are well-known and
> respected by the vast majority of their peers, then bye bye.
>
>> Surely, you can wait with those, more or less, cosmetic non-runtime
>> tested changes?
>
> I see you once again failed to read (or understand) something I repeated
> many times already. One of these changes (the one moving the hwmon
> attributes from i2c device to hwmon device) is _mandatory_ to get your
> own changes accepted. Guenter did you an immense favor by writing these
> patches, so if anything you should be very grateful to him.

I submitted a change for trying to address the _real_ issue a couple
of days ago _right after_ the suggestion. You can check the times, it
was literally within a couple of hours. Then, someone submits the same
with better content. This is what I meant by not how a community
works. You can leave comments for others, and then they can increase
their expertise.

The patch set is very likely to contain bugs because it is a
significant re-factoring, whereas I focused on as minimal changes as
possible to get the feature reliably in.

Please help me to be involved, and not excluded. It will be hard for
me to feel helpful, and the inherent consequence is that my project
will not work with the kernel project which, I think, we both would
like to avoid.

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

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Lee Jones]

> >>> > -static int max6650_probe(struct i2c_client *client,
> >>> > -                    const struct i2c_device_id *id);
> >>> > -static int max6650_init_client(struct i2c_client *client);
> >>> > -static int max6650_remove(struct i2c_client *client);
> >>> > +static int max6650_probe(struct platform_device *pdev);
> >>> > +static int max6650_init_client(struct platform_device *pdev);
> >>> > +static int max6650_remove(struct platform_device *pdev);
> >>> >  static struct max6650_data *max6650_update_device(struct device *dev);
> >>>
> >>> It would be good to remove these forward declarations in the future.
> >>>
> >>> If no one volunteers I'll happily do it.
> >>
> >> Guenter just did:
> >>
> >> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041224.html
> >>
> >> Any change to the max6650 driver should go on top of his patch series
> >> to avoid conflicts:
> >>
> >> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html
> >
> As far as I can see, that patch set was not even tested, so how can it
> go in? I was told that any patch should be _runtime_ tested, too.
> Fwiw, I do not have time to test those personally, he would need to
> find someone else if that requirement really holds true.
>
> I would not really like to fix bugs appearing in that code to get my
> features in.
> 
> Also, since my change has been around for 2-3 months now, I would
> really prefer not to be forced to rewrite it again from scratch.
> Surely, you can wait with those, more or less, cosmetic non-runtime
> tested changes?
> 
> This would impose me a lot of additional work again, and I personally
> do not see the benefit of it. In my book at least, feature is over
> internal polishing.

Right, I've had enough. I'm removing your patch from the MFD tree.

I've asked too many people to give you a second chance and asked you
privately to behave yourself and treat others with respect. So far I
haven't seen an ounce of self control or depomacy from you.

This is how it's going to work from now on:

 - You submit a patch
 - It gets reviewed                            <----\
 - You fix up the review comments as requested -----/
 - Non-compliance or arguments with the _experts_ results in:
    `$INTEREST > /dev/null || \
      grep "From: Laszio Papp" ~/.mail | xargs rm -rf`

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Lee Jones]

PiA+Pj4gPiAtc3RhdGljIGludCBtYXg2NjUwX3Byb2JlKHN0cnVjdCBpMmNfY2xpZW50ICpjbGll
bnQsCj4gPj4+ID4gLSAgICAgICAgICAgICAgICAgICAgY29uc3Qgc3RydWN0IGkyY19kZXZpY2Vf
aWQgKmlkKTsKPiA+Pj4gPiAtc3RhdGljIGludCBtYXg2NjUwX2luaXRfY2xpZW50KHN0cnVjdCBp
MmNfY2xpZW50ICpjbGllbnQpOwo+ID4+PiA+IC1zdGF0aWMgaW50IG1heDY2NTBfcmVtb3ZlKHN0
cnVjdCBpMmNfY2xpZW50ICpjbGllbnQpOwo+ID4+PiA+ICtzdGF0aWMgaW50IG1heDY2NTBfcHJv
YmUoc3RydWN0IHBsYXRmb3JtX2RldmljZSAqcGRldik7Cj4gPj4+ID4gK3N0YXRpYyBpbnQgbWF4
NjY1MF9pbml0X2NsaWVudChzdHJ1Y3QgcGxhdGZvcm1fZGV2aWNlICpwZGV2KTsKPiA+Pj4gPiAr
c3RhdGljIGludCBtYXg2NjUwX3JlbW92ZShzdHJ1Y3QgcGxhdGZvcm1fZGV2aWNlICpwZGV2KTsK
PiA+Pj4gPiAgc3RhdGljIHN0cnVjdCBtYXg2NjUwX2RhdGEgKm1heDY2NTBfdXBkYXRlX2Rldmlj
ZShzdHJ1Y3QgZGV2aWNlICpkZXYpOwo+ID4+Pgo+ID4+PiBJdCB3b3VsZCBiZSBnb29kIHRvIHJl
bW92ZSB0aGVzZSBmb3J3YXJkIGRlY2xhcmF0aW9ucyBpbiB0aGUgZnV0dXJlLgo+ID4+Pgo+ID4+
PiBJZiBubyBvbmUgdm9sdW50ZWVycyBJJ2xsIGhhcHBpbHkgZG8gaXQuCj4gPj4KPiA+PiBHdWVu
dGVyIGp1c3QgZGlkOgo+ID4+Cj4gPj4gaHR0cDovL2xpc3RzLmxtLXNlbnNvcnMub3JnL3BpcGVy
bWFpbC9sbS1zZW5zb3JzLzIwMTQtRmVicnVhcnkvMDQxMjI0Lmh0bWwKPiA+Pgo+ID4+IEFueSBj
aGFuZ2UgdG8gdGhlIG1heDY2NTAgZHJpdmVyIHNob3VsZCBnbyBvbiB0b3Agb2YgaGlzIHBhdGNo
IHNlcmllcwo+ID4+IHRvIGF2b2lkIGNvbmZsaWN0czoKPiA+Pgo+ID4+IGh0dHA6Ly9saXN0cy5s
bS1zZW5zb3JzLm9yZy9waXBlcm1haWwvbG0tc2Vuc29ycy8yMDE0LUZlYnJ1YXJ5LzA0MTIyMy5o
dG1sCj4gPgo+IEFzIGZhciBhcyBJIGNhbiBzZWUsIHRoYXQgcGF0Y2ggc2V0IHdhcyBub3QgZXZl
biB0ZXN0ZWQsIHNvIGhvdyBjYW4gaXQKPiBnbyBpbj8gSSB3YXMgdG9sZCB0aGF0IGFueSBwYXRj
aCBzaG91bGQgYmUgX3J1bnRpbWVfIHRlc3RlZCwgdG9vLgo+IEZ3aXcsIEkgZG8gbm90IGhhdmUg
dGltZSB0byB0ZXN0IHRob3NlIHBlcnNvbmFsbHksIGhlIHdvdWxkIG5lZWQgdG8KPiBmaW5kIHNv
bWVvbmUgZWxzZSBpZiB0aGF0IHJlcXVpcmVtZW50IHJlYWxseSBob2xkcyB0cnVlLgo+Cj4gSSB3
b3VsZCBub3QgcmVhbGx5IGxpa2UgdG8gZml4IGJ1Z3MgYXBwZWFyaW5nIGluIHRoYXQgY29kZSB0
byBnZXQgbXkKPiBmZWF0dXJlcyBpbi4KPiAKPiBBbHNvLCBzaW5jZSBteSBjaGFuZ2UgaGFzIGJl
ZW4gYXJvdW5kIGZvciAyLTMgbW9udGhzIG5vdywgSSB3b3VsZAo+IHJlYWxseSBwcmVmZXIgbm90
IHRvIGJlIGZvcmNlZCB0byByZXdyaXRlIGl0IGFnYWluIGZyb20gc2NyYXRjaC4KPiBTdXJlbHks
IHlvdSBjYW4gd2FpdCB3aXRoIHRob3NlLCBtb3JlIG9yIGxlc3MsIGNvc21ldGljIG5vbi1ydW50
aW1lCj4gdGVzdGVkIGNoYW5nZXM/Cj4gCj4gVGhpcyB3b3VsZCBpbXBvc2UgbWUgYSBsb3Qgb2Yg
YWRkaXRpb25hbCB3b3JrIGFnYWluLCBhbmQgSSBwZXJzb25hbGx5Cj4gZG8gbm90IHNlZSB0aGUg
YmVuZWZpdCBvZiBpdC4gSW4gbXkgYm9vayBhdCBsZWFzdCwgZmVhdHVyZSBpcyBvdmVyCj4gaW50
ZXJuYWwgcG9saXNoaW5nLgoKUmlnaHQsIEkndmUgaGFkIGVub3VnaC4gSSdtIHJlbW92aW5nIHlv
dXIgcGF0Y2ggZnJvbSB0aGUgTUZEIHRyZWUuCgpJJ3ZlIGFza2VkIHRvbyBtYW55IHBlb3BsZSB0
byBnaXZlIHlvdSBhIHNlY29uZCBjaGFuY2UgYW5kIGFza2VkIHlvdQpwcml2YXRlbHkgdG8gYmVo
YXZlIHlvdXJzZWxmIGFuZCB0cmVhdCBvdGhlcnMgd2l0aCByZXNwZWN0LiBTbyBmYXIgSQpoYXZl
bid0IHNlZW4gYW4gb3VuY2Ugb2Ygc2VsZiBjb250cm9sIG9yIGRlcG9tYWN5IGZyb20geW91LgoK
VGhpcyBpcyBob3cgaXQncyBnb2luZyB0byB3b3JrIGZyb20gbm93IG9uOgoKIC0gWW91IHN1Ym1p
dCBhIHBhdGNoCiAtIEl0IGdldHMgcmV2aWV3ZWQgICAgICAgICAgICAgICAgICAgICAgICAgICAg
PC0tLS1cCiAtIFlvdSBmaXggdXAgdGhlIHJldmlldyBjb21tZW50cyBhcyByZXF1ZXN0ZWQgLS0t
LS0vCiAtIE5vbi1jb21wbGlhbmNlIG9yIGFyZ3VtZW50cyB3aXRoIHRoZSBfZXhwZXJ0c18gcmVz
dWx0cyBpbjoKICAgIGAkSU5URVJFU1QgPiAvZGV2L251bGwgfHwgXAogICAgICBncmVwICJGcm9t
OiBMYXN6aW8gUGFwcCIgfi8ubWFpbCB8IHhhcmdzIHJtIC1yZmAKCi0tIApMZWUgSm9uZXMKTGlu
YXJvIFNUTWljcm9lbGVjdHJvbmljcyBMYW5kaW5nIFRlYW0gTGVhZApMaW5hcm8ub3JnIOKUgiBP
cGVuIHNvdXJjZSBzb2Z0d2FyZSBmb3IgQVJNIFNvQ3MKRm9sbG93IExpbmFybzogRmFjZWJvb2sg
fCBUd2l0dGVyIHwgQmxvZwoKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f
X19fX19fX18KbG0tc2Vuc29ycyBtYWlsaW5nIGxpc3QKbG0tc2Vuc29yc0BsbS1zZW5zb3JzLm9y
ZwpodHRwOi8vbGlzdHMubG0tc2Vuc29ycy5vcmcvbWFpbG1hbi9saXN0aW5mby9sbS1zZW5zb3Jz

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Jean Delvare]

Hi Lee,

On Thu, 13 Feb 2014 11:16:07 +0000, Lee Jones wrote:
> On Thu, 13 Feb 2014, Jean Delvare wrote:
> > Guenter just did:
> > 
> > http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041224.html
> 
> Nice, FWIW:
>   Acked-by: Lee Jones <lee.jones@linaro.org>  
> 
> > Any change to the max6650 driver should go on top of his patch series
> > to avoid conflicts:
> > 
> > http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html
> 
> Do you have a tree Laszlo can rebase on top of?

Now that the patches have my Reviewed-by and your Acked-by, I believe
Guenter will add them shortly to:

http://git.kernel.org/cgit/linux/kernel/git/groeck/linux-staging.git/log/?h=hwmon-staging

-- 
Jean Delvare
Suse L3 Support

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Jean Delvare]

Hi Lee,

On Thu, 13 Feb 2014 11:16:07 +0000, Lee Jones wrote:
> On Thu, 13 Feb 2014, Jean Delvare wrote:
> > Guenter just did:
> > 
> > http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041224.html
> 
> Nice, FWIW:
>   Acked-by: Lee Jones <lee.jones@linaro.org>  
> 
> > Any change to the max6650 driver should go on top of his patch series
> > to avoid conflicts:
> > 
> > http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html
> 
> Do you have a tree Laszlo can rebase on top of?

Now that the patches have my Reviewed-by and your Acked-by, I believe
Guenter will add them shortly to:

http://git.kernel.org/cgit/linux/kernel/git/groeck/linux-staging.git/log/?h=hwmon-staging

-- 
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] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Laszlo Papp]

On Thu, Feb 13, 2014 at 11:33 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> >>> > -static int max6650_probe(struct i2c_client *client,
>> >>> > -                    const struct i2c_device_id *id);
>> >>> > -static int max6650_init_client(struct i2c_client *client);
>> >>> > -static int max6650_remove(struct i2c_client *client);
>> >>> > +static int max6650_probe(struct platform_device *pdev);
>> >>> > +static int max6650_init_client(struct platform_device *pdev);
>> >>> > +static int max6650_remove(struct platform_device *pdev);
>> >>> >  static struct max6650_data *max6650_update_device(struct device *dev);
>> >>>
>> >>> It would be good to remove these forward declarations in the future.
>> >>>
>> >>> If no one volunteers I'll happily do it.
>> >>
>> >> Guenter just did:
>> >>
>> >> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041224.html
>> >>
>> >> Any change to the max6650 driver should go on top of his patch series
>> >> to avoid conflicts:
>> >>
>> >> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html
>> >
>> As far as I can see, that patch set was not even tested, so how can it
>> go in? I was told that any patch should be _runtime_ tested, too.
>> Fwiw, I do not have time to test those personally, he would need to
>> find someone else if that requirement really holds true.
>>
>> I would not really like to fix bugs appearing in that code to get my
>> features in.
>>
>> Also, since my change has been around for 2-3 months now, I would
>> really prefer not to be forced to rewrite it again from scratch.
>> Surely, you can wait with those, more or less, cosmetic non-runtime
>> tested changes?
>>
>> This would impose me a lot of additional work again, and I personally
>> do not see the benefit of it. In my book at least, feature is over
>> internal polishing.
>
> Right, I've had enough. I'm removing your patch from the MFD tree.
>
> I've asked too many people to give you a second chance and asked you
> privately to behave yourself and treat others with respect. So far I
> haven't seen an ounce of self control or depomacy from you.
>
> This is how it's going to work from now on:
>
>  - You submit a patch
>  - It gets reviewed                            <----\
>  - You fix up the review comments as requested -----/
>  - Non-compliance or arguments with the _experts_ results in:
>     `$INTEREST > /dev/null || \
>       grep "From: Laszio Papp" ~/.mail | xargs rm -rf`

http://comments.gmane.org/gmane.linux.kernel/1645251

Step 2 did not happen. I did not get any review for my change. I
literally submitted that within a couple of hours after the request.

Could you please tell me what was wrong with that change, and why I
did not get any respect not to "xargs rm -rf" my work in that area? I
believe I was ignored instead of improving the change, and someone
else tried to address the same thing. There was no argument in that
thread. It was a technical change. I personally do not feel happy
about it.

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Laszlo Papp]

On Thu, Feb 13, 2014 at 11:33 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> >>> > -static int max6650_probe(struct i2c_client *client,
>> >>> > -                    const struct i2c_device_id *id);
>> >>> > -static int max6650_init_client(struct i2c_client *client);
>> >>> > -static int max6650_remove(struct i2c_client *client);
>> >>> > +static int max6650_probe(struct platform_device *pdev);
>> >>> > +static int max6650_init_client(struct platform_device *pdev);
>> >>> > +static int max6650_remove(struct platform_device *pdev);
>> >>> >  static struct max6650_data *max6650_update_device(struct device *dev);
>> >>>
>> >>> It would be good to remove these forward declarations in the future.
>> >>>
>> >>> If no one volunteers I'll happily do it.
>> >>
>> >> Guenter just did:
>> >>
>> >> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041224.html
>> >>
>> >> Any change to the max6650 driver should go on top of his patch series
>> >> to avoid conflicts:
>> >>
>> >> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html
>> >
>> As far as I can see, that patch set was not even tested, so how can it
>> go in? I was told that any patch should be _runtime_ tested, too.
>> Fwiw, I do not have time to test those personally, he would need to
>> find someone else if that requirement really holds true.
>>
>> I would not really like to fix bugs appearing in that code to get my
>> features in.
>>
>> Also, since my change has been around for 2-3 months now, I would
>> really prefer not to be forced to rewrite it again from scratch.
>> Surely, you can wait with those, more or less, cosmetic non-runtime
>> tested changes?
>>
>> This would impose me a lot of additional work again, and I personally
>> do not see the benefit of it. In my book at least, feature is over
>> internal polishing.
>
> Right, I've had enough. I'm removing your patch from the MFD tree.
>
> I've asked too many people to give you a second chance and asked you
> privately to behave yourself and treat others with respect. So far I
> haven't seen an ounce of self control or depomacy from you.
>
> This is how it's going to work from now on:
>
>  - You submit a patch
>  - It gets reviewed                            <----\
>  - You fix up the review comments as requested -----/
>  - Non-compliance or arguments with the _experts_ results in:
>     `$INTEREST > /dev/null || \
>       grep "From: Laszio Papp" ~/.mail | xargs rm -rf`

http://comments.gmane.org/gmane.linux.kernel/1645251

Step 2 did not happen. I did not get any review for my change. I
literally submitted that within a couple of hours after the request.

Could you please tell me what was wrong with that change, and why I
did not get any respect not to "xargs rm -rf" my work in that area? I
believe I was ignored instead of improving the change, and someone
else tried to address the same thing. There was no argument in that
thread. It was a technical change. I personally do not feel happy
about it.

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

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Lee Jones]

> http://comments.gmane.org/gmane.linux.kernel/1645251
> 
> Step 2 did not happen. I did not get any review for my change. I
> literally submitted that within a couple of hours after the request.
> 
> Could you please tell me what was wrong with that change, and why I
> did not get any respect not to "xargs rm -rf" my work in that area? I
> believe I was ignored instead of improving the change, and someone
> else tried to address the same thing. There was no argument in that
> thread. It was a technical change. I personally do not feel happy
> about it.

Let's start again.

Rebase your work on top of the HWMON tree on kernel.org and resubmit
the entire set. If rebasing takes you more than 20 mins, you're
probably doing it wrong. Ensure you submit the entire patchset with a
nice cover letter and all the patches dangling (shallow threaded) from
it. If you don't know how to do that look at `git send-email --help`
and search for "thread", "annotate" and "compose". Keep submitting
them to your own (and only your own) email address until it looks
correct, _then_ submit to the MLs CC'ing all of the maintainers on all
of the patches.

Submission should look like this:

  [PATCH 0/x] Patch-set title
   \->[PATCH 1/x] mfd: Patch title
   |->[PATCH ./x] hwmon: Patch title
   |->[PATCH x/x] gpio/pinctrl: Patch title

We'll give you constructive reviews and you'll ask questions (not
arguments) and resubmit.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Lee Jones]

PiBodHRwOi8vY29tbWVudHMuZ21hbmUub3JnL2dtYW5lLmxpbnV4Lmtlcm5lbC8xNjQ1MjUxCj4g
Cj4gU3RlcCAyIGRpZCBub3QgaGFwcGVuLiBJIGRpZCBub3QgZ2V0IGFueSByZXZpZXcgZm9yIG15
IGNoYW5nZS4gSQo+IGxpdGVyYWxseSBzdWJtaXR0ZWQgdGhhdCB3aXRoaW4gYSBjb3VwbGUgb2Yg
aG91cnMgYWZ0ZXIgdGhlIHJlcXVlc3QuCj4gCj4gQ291bGQgeW91IHBsZWFzZSB0ZWxsIG1lIHdo
YXQgd2FzIHdyb25nIHdpdGggdGhhdCBjaGFuZ2UsIGFuZCB3aHkgSQo+IGRpZCBub3QgZ2V0IGFu
eSByZXNwZWN0IG5vdCB0byAieGFyZ3Mgcm0gLXJmIiBteSB3b3JrIGluIHRoYXQgYXJlYT8gSQo+
IGJlbGlldmUgSSB3YXMgaWdub3JlZCBpbnN0ZWFkIG9mIGltcHJvdmluZyB0aGUgY2hhbmdlLCBh
bmQgc29tZW9uZQo+IGVsc2UgdHJpZWQgdG8gYWRkcmVzcyB0aGUgc2FtZSB0aGluZy4gVGhlcmUg
d2FzIG5vIGFyZ3VtZW50IGluIHRoYXQKPiB0aHJlYWQuIEl0IHdhcyBhIHRlY2huaWNhbCBjaGFu
Z2UuIEkgcGVyc29uYWxseSBkbyBub3QgZmVlbCBoYXBweQo+IGFib3V0IGl0LgoKTGV0J3Mgc3Rh
cnQgYWdhaW4uCgpSZWJhc2UgeW91ciB3b3JrIG9uIHRvcCBvZiB0aGUgSFdNT04gdHJlZSBvbiBr
ZXJuZWwub3JnIGFuZCByZXN1Ym1pdAp0aGUgZW50aXJlIHNldC4gSWYgcmViYXNpbmcgdGFrZXMg
eW91IG1vcmUgdGhhbiAyMCBtaW5zLCB5b3UncmUKcHJvYmFibHkgZG9pbmcgaXQgd3JvbmcuIEVu
c3VyZSB5b3Ugc3VibWl0IHRoZSBlbnRpcmUgcGF0Y2hzZXQgd2l0aCBhCm5pY2UgY292ZXIgbGV0
dGVyIGFuZCBhbGwgdGhlIHBhdGNoZXMgZGFuZ2xpbmcgKHNoYWxsb3cgdGhyZWFkZWQpIGZyb20K
aXQuIElmIHlvdSBkb24ndCBrbm93IGhvdyB0byBkbyB0aGF0IGxvb2sgYXQgYGdpdCBzZW5kLWVt
YWlsIC0taGVscGAKYW5kIHNlYXJjaCBmb3IgInRocmVhZCIsICJhbm5vdGF0ZSIgYW5kICJjb21w
b3NlIi4gS2VlcCBzdWJtaXR0aW5nCnRoZW0gdG8geW91ciBvd24gKGFuZCBvbmx5IHlvdXIgb3du
KSBlbWFpbCBhZGRyZXNzIHVudGlsIGl0IGxvb2tzCmNvcnJlY3QsIF90aGVuXyBzdWJtaXQgdG8g
dGhlIE1McyBDQydpbmcgYWxsIG9mIHRoZSBtYWludGFpbmVycyBvbiBhbGwKb2YgdGhlIHBhdGNo
ZXMuCgpTdWJtaXNzaW9uIHNob3VsZCBsb29rIGxpa2UgdGhpczoKCiAgW1BBVENIIDAveF0gUGF0
Y2gtc2V0IHRpdGxlCiAgIFwtPltQQVRDSCAxL3hdIG1mZDogUGF0Y2ggdGl0bGUKICAgfC0+W1BB
VENIIC4veF0gaHdtb246IFBhdGNoIHRpdGxlCiAgIHwtPltQQVRDSCB4L3hdIGdwaW8vcGluY3Ry
bDogUGF0Y2ggdGl0bGUKCldlJ2xsIGdpdmUgeW91IGNvbnN0cnVjdGl2ZSByZXZpZXdzIGFuZCB5
b3UnbGwgYXNrIHF1ZXN0aW9ucyAobm90CmFyZ3VtZW50cykgYW5kIHJlc3VibWl0LgoKLS0gCkxl
ZSBKb25lcwpMaW5hcm8gU1RNaWNyb2VsZWN0cm9uaWNzIExhbmRpbmcgVGVhbSBMZWFkCkxpbmFy
by5vcmcg4pSCIE9wZW4gc291cmNlIHNvZnR3YXJlIGZvciBBUk0gU29DcwpGb2xsb3cgTGluYXJv
OiBGYWNlYm9vayB8IFR3aXR0ZXIgfCBCbG9nCgpfX19fX19fX19fX19fX19fX19fX19fX19fX19f
X19fX19fX19fX19fX19fX19fXwpsbS1zZW5zb3JzIG1haWxpbmcgbGlzdApsbS1zZW5zb3JzQGxt
LXNlbnNvcnMub3JnCmh0dHA6Ly9saXN0cy5sbS1zZW5zb3JzLm9yZy9tYWlsbWFuL2xpc3RpbmZv
L2xtLXNlbnNvcnM

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Jean Delvare]

Hi Laszlo,	

On Thu, 13 Feb 2014 12:27:28 +0000, Laszlo Papp wrote:
> On Thu, Feb 13, 2014 at 11:33 AM, Lee Jones <lee.jones@linaro.org> wrote:
> > Right, I've had enough. I'm removing your patch from the MFD tree.
> >
> > I've asked too many people to give you a second chance and asked you
> > privately to behave yourself and treat others with respect. So far I
> > haven't seen an ounce of self control or depomacy from you.
> >
> > This is how it's going to work from now on:
> >
> >  - You submit a patch
> >  - It gets reviewed                            <----\
> >  - You fix up the review comments as requested -----/
> >  - Non-compliance or arguments with the _experts_ results in:
> >     `$INTEREST > /dev/null || \
> >       grep "From: Laszio Papp" ~/.mail | xargs rm -rf`
> 
> http://comments.gmane.org/gmane.linux.kernel/1645251
> 
> Step 2 did not happen. I did not get any review for my change. I
> literally submitted that within a couple of hours after the request.

Yes, twice even, and broken each time. And without a changelog on v2
(despite Documentation/SubmittingPatches explaining this is a good
practice - see section 15.)

> Could you please tell me what was wrong with that change, and why I
> did not get any respect not to "xargs rm -rf" my work in that area? I
> believe I was ignored instead of improving the change, and someone
> else tried to address the same thing. There was no argument in that
> thread. It was a technical change. I personally do not feel happy
> about it.

The change itself was so wrong that I don't even know where to start.

But the main problem really was you. You had pissed me (and I suspect,
everybody else) off so much that day that I really didn't want to deal
with your rants or code any longer. As you can imagine, I have more
than enough on my plate, so I just moved on to another task.

Then by the time I may have been willing to give you another chance and
review your code, Guenter wrote a more complete, better patch set. So I
thought I'd just review that one. And it was good, and it took me less
time to review and test it than to (attempt to) teach you how to behave.

Working with Guenter is a pleasure. Working with you is a pain, really.
And guess what, I get to choose who I'm working with.

If people no longer want to work with you, well, blame it on yourself.

-- 
Jean Delvare
Suse L3 Support

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Jean Delvare]

Hi Laszlo,	

On Thu, 13 Feb 2014 12:27:28 +0000, Laszlo Papp wrote:
> On Thu, Feb 13, 2014 at 11:33 AM, Lee Jones <lee.jones@linaro.org> wrote:
> > Right, I've had enough. I'm removing your patch from the MFD tree.
> >
> > I've asked too many people to give you a second chance and asked you
> > privately to behave yourself and treat others with respect. So far I
> > haven't seen an ounce of self control or depomacy from you.
> >
> > This is how it's going to work from now on:
> >
> >  - You submit a patch
> >  - It gets reviewed                            <----\
> >  - You fix up the review comments as requested -----/
> >  - Non-compliance or arguments with the _experts_ results in:
> >     `$INTEREST > /dev/null || \
> >       grep "From: Laszio Papp" ~/.mail | xargs rm -rf`
> 
> http://comments.gmane.org/gmane.linux.kernel/1645251
> 
> Step 2 did not happen. I did not get any review for my change. I
> literally submitted that within a couple of hours after the request.

Yes, twice even, and broken each time. And without a changelog on v2
(despite Documentation/SubmittingPatches explaining this is a good
practice - see section 15.)

> Could you please tell me what was wrong with that change, and why I
> did not get any respect not to "xargs rm -rf" my work in that area? I
> believe I was ignored instead of improving the change, and someone
> else tried to address the same thing. There was no argument in that
> thread. It was a technical change. I personally do not feel happy
> about it.

The change itself was so wrong that I don't even know where to start.

But the main problem really was you. You had pissed me (and I suspect,
everybody else) off so much that day that I really didn't want to deal
with your rants or code any longer. As you can imagine, I have more
than enough on my plate, so I just moved on to another task.

Then by the time I may have been willing to give you another chance and
review your code, Guenter wrote a more complete, better patch set. So I
thought I'd just review that one. And it was good, and it took me less
time to review and test it than to (attempt to) teach you how to behave.

Working with Guenter is a pleasure. Working with you is a pain, really.
And guess what, I get to choose who I'm working with.

If people no longer want to work with you, well, blame it on yourself.

-- 
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] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Laszlo Papp]

On Thu, Feb 13, 2014 at 12:57 PM, Jean Delvare <jdelvare@suse.de> wrote:
> Hi Laszlo,
>
> On Thu, 13 Feb 2014 12:27:28 +0000, Laszlo Papp wrote:
>> On Thu, Feb 13, 2014 at 11:33 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> > Right, I've had enough. I'm removing your patch from the MFD tree.
>> >
>> > I've asked too many people to give you a second chance and asked you
>> > privately to behave yourself and treat others with respect. So far I
>> > haven't seen an ounce of self control or depomacy from you.
>> >
>> > This is how it's going to work from now on:
>> >
>> >  - You submit a patch
>> >  - It gets reviewed                            <----\
>> >  - You fix up the review comments as requested -----/
>> >  - Non-compliance or arguments with the _experts_ results in:
>> >     `$INTEREST > /dev/null || \
>> >       grep "From: Laszio Papp" ~/.mail | xargs rm -rf`
>>
>> http://comments.gmane.org/gmane.linux.kernel/1645251
>>
>> Step 2 did not happen. I did not get any review for my change. I
>> literally submitted that within a couple of hours after the request.
>
> Yes, twice even, and broken each time. And without a changelog on v2
> (despite Documentation/SubmittingPatches explaining this is a good
> practice - see section 15.)
>
>> Could you please tell me what was wrong with that change, and why I
>> did not get any respect not to "xargs rm -rf" my work in that area? I
>> believe I was ignored instead of improving the change, and someone
>> else tried to address the same thing. There was no argument in that
>> thread. It was a technical change. I personally do not feel happy
>> about it.
>
> The change itself was so wrong that I don't even know where to start.
>
> But the main problem really was you. You had pissed me (and I suspect,
> everybody else) off so much that day that I really didn't want to deal
> with your rants or code any longer. As you can imagine, I have more
> than enough on my plate, so I just moved on to another task.
>
> Then by the time I may have been willing to give you another chance and
> review your code, Guenter wrote a more complete, better patch set. So I
> thought I'd just review that one. And it was good, and it took me less
> time to review and test it than to (attempt to) teach you how to behave.
>
> Working with Guenter is a pleasure. Working with you is a pain, really.
> And guess what, I get to choose who I'm working with.
>
> If people no longer want to work with you, well, blame it on yourself.

I think the question was more addressed to Guenter because he did not
try to help me to get involved with that patch. I did not mean to say
that I would have expected reviews within a week, but I would have
hoped for being more inclusive, i.e. trying to tell me what I am
doing, why, even if it is clearly broken. Please forgive me my
technical shortcoming. I am sure this would improve over time, just
like for anyone else here.

Currently, I am not there mentally to submit the patch set requested
by Lee because this situation is making me being afraid of a newbie
and submitting patches with issues. An expert may just come and
rewrite it, and then my effort is going again to "/dev/null". I guess
I will need some time and break to get over this...

That being said, I hope I will come back with peace of mind after the break.

PS.: I have submitted a patch set in December by the way, and got no
review from the hwmon maintainers for the hwmon parts. That patch is
still valid, and review would still be very welcome. The current
submission is pretty much just a direct port of it on top of the
latest mfd and upstream changes.

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Laszlo Papp]

On Thu, Feb 13, 2014 at 12:57 PM, Jean Delvare <jdelvare@suse.de> wrote:
> Hi Laszlo,
>
> On Thu, 13 Feb 2014 12:27:28 +0000, Laszlo Papp wrote:
>> On Thu, Feb 13, 2014 at 11:33 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> > Right, I've had enough. I'm removing your patch from the MFD tree.
>> >
>> > I've asked too many people to give you a second chance and asked you
>> > privately to behave yourself and treat others with respect. So far I
>> > haven't seen an ounce of self control or depomacy from you.
>> >
>> > This is how it's going to work from now on:
>> >
>> >  - You submit a patch
>> >  - It gets reviewed                            <----\
>> >  - You fix up the review comments as requested -----/
>> >  - Non-compliance or arguments with the _experts_ results in:
>> >     `$INTEREST > /dev/null || \
>> >       grep "From: Laszio Papp" ~/.mail | xargs rm -rf`
>>
>> http://comments.gmane.org/gmane.linux.kernel/1645251
>>
>> Step 2 did not happen. I did not get any review for my change. I
>> literally submitted that within a couple of hours after the request.
>
> Yes, twice even, and broken each time. And without a changelog on v2
> (despite Documentation/SubmittingPatches explaining this is a good
> practice - see section 15.)
>
>> Could you please tell me what was wrong with that change, and why I
>> did not get any respect not to "xargs rm -rf" my work in that area? I
>> believe I was ignored instead of improving the change, and someone
>> else tried to address the same thing. There was no argument in that
>> thread. It was a technical change. I personally do not feel happy
>> about it.
>
> The change itself was so wrong that I don't even know where to start.
>
> But the main problem really was you. You had pissed me (and I suspect,
> everybody else) off so much that day that I really didn't want to deal
> with your rants or code any longer. As you can imagine, I have more
> than enough on my plate, so I just moved on to another task.
>
> Then by the time I may have been willing to give you another chance and
> review your code, Guenter wrote a more complete, better patch set. So I
> thought I'd just review that one. And it was good, and it took me less
> time to review and test it than to (attempt to) teach you how to behave.
>
> Working with Guenter is a pleasure. Working with you is a pain, really.
> And guess what, I get to choose who I'm working with.
>
> If people no longer want to work with you, well, blame it on yourself.

I think the question was more addressed to Guenter because he did not
try to help me to get involved with that patch. I did not mean to say
that I would have expected reviews within a week, but I would have
hoped for being more inclusive, i.e. trying to tell me what I am
doing, why, even if it is clearly broken. Please forgive me my
technical shortcoming. I am sure this would improve over time, just
like for anyone else here.

Currently, I am not there mentally to submit the patch set requested
by Lee because this situation is making me being afraid of a newbie
and submitting patches with issues. An expert may just come and
rewrite it, and then my effort is going again to "/dev/null". I guess
I will need some time and break to get over this...

That being said, I hope I will come back with peace of mind after the break.

PS.: I have submitted a patch set in December by the way, and got no
review from the hwmon maintainers for the hwmon parts. That patch is
still valid, and review would still be very welcome. The current
submission is pretty much just a direct port of it on top of the
latest mfd and upstream changes.

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

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Guenter Roeck]

On 02/13/2014 04:27 AM, Laszlo Papp wrote:
> On Thu, Feb 13, 2014 at 11:33 AM, Lee Jones <lee.jones@linaro.org> wrote:
>>>>>>> -static int max6650_probe(struct i2c_client *client,
>>>>>>> -                    const struct i2c_device_id *id);
>>>>>>> -static int max6650_init_client(struct i2c_client *client);
>>>>>>> -static int max6650_remove(struct i2c_client *client);
>>>>>>> +static int max6650_probe(struct platform_device *pdev);
>>>>>>> +static int max6650_init_client(struct platform_device *pdev);
>>>>>>> +static int max6650_remove(struct platform_device *pdev);
>>>>>>>   static struct max6650_data *max6650_update_device(struct device *dev);
>>>>>>
>>>>>> It would be good to remove these forward declarations in the future.
>>>>>>
>>>>>> If no one volunteers I'll happily do it.
>>>>>
>>>>> Guenter just did:
>>>>>
>>>>> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041224.html
>>>>>
>>>>> Any change to the max6650 driver should go on top of his patch series
>>>>> to avoid conflicts:
>>>>>
>>>>> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html
>>>>
>>> As far as I can see, that patch set was not even tested, so how can it
>>> go in? I was told that any patch should be _runtime_ tested, too.
>>> Fwiw, I do not have time to test those personally, he would need to
>>> find someone else if that requirement really holds true.
>>>
>>> I would not really like to fix bugs appearing in that code to get my
>>> features in.
>>>
>>> Also, since my change has been around for 2-3 months now, I would
>>> really prefer not to be forced to rewrite it again from scratch.
>>> Surely, you can wait with those, more or less, cosmetic non-runtime
>>> tested changes?
>>>
>>> This would impose me a lot of additional work again, and I personally
>>> do not see the benefit of it. In my book at least, feature is over
>>> internal polishing.
>>
>> Right, I've had enough. I'm removing your patch from the MFD tree.
>>
>> I've asked too many people to give you a second chance and asked you
>> privately to behave yourself and treat others with respect. So far I
>> haven't seen an ounce of self control or depomacy from you.
>>
>> This is how it's going to work from now on:
>>
>>   - You submit a patch
>>   - It gets reviewed                            <----\
>>   - You fix up the review comments as requested -----/
>>   - Non-compliance or arguments with the _experts_ results in:
>>      `$INTEREST > /dev/null || \
>>        grep "From: Laszio Papp" ~/.mail | xargs rm -rf`
>
> http://comments.gmane.org/gmane.linux.kernel/1645251
>
> Step 2 did not happen. I did not get any review for my change. I
> literally submitted that within a couple of hours after the request.
>

If you had tested your patch on real or simulated hardware,
you might have noticed a crash whenever you accessed any
of the attributes. So you did not test your patch.

Instead of trying to educate you how the conversion to the
new API works, I decided to help you out a bit and do
the conversion myself. I did some cleanup before, since
that made the actual feature patch easier for me to implement,
and I did some more cleanup afterwards just because I like
cleaning up code.

I had hoped that you might find the time to test the result,
but it appears that won't happen. I am gracious to Jean that
he took the time to review the changes and even test the
result in simulation, even though I know he is very busy.
So I consider the changes to be good enough to be made
available in my -staging tree, which I did by now.
I'll move them over to -next once I have the chance to test
on real hardware or after I get a Tested-by: from someone
with real hardware.

Guenter

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Guenter Roeck]

On 02/13/2014 04:27 AM, Laszlo Papp wrote:
> On Thu, Feb 13, 2014 at 11:33 AM, Lee Jones <lee.jones@linaro.org> wrote:
>>>>>>> -static int max6650_probe(struct i2c_client *client,
>>>>>>> -                    const struct i2c_device_id *id);
>>>>>>> -static int max6650_init_client(struct i2c_client *client);
>>>>>>> -static int max6650_remove(struct i2c_client *client);
>>>>>>> +static int max6650_probe(struct platform_device *pdev);
>>>>>>> +static int max6650_init_client(struct platform_device *pdev);
>>>>>>> +static int max6650_remove(struct platform_device *pdev);
>>>>>>>   static struct max6650_data *max6650_update_device(struct device *dev);
>>>>>>
>>>>>> It would be good to remove these forward declarations in the future.
>>>>>>
>>>>>> If no one volunteers I'll happily do it.
>>>>>
>>>>> Guenter just did:
>>>>>
>>>>> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041224.html
>>>>>
>>>>> Any change to the max6650 driver should go on top of his patch series
>>>>> to avoid conflicts:
>>>>>
>>>>> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html
>>>>
>>> As far as I can see, that patch set was not even tested, so how can it
>>> go in? I was told that any patch should be _runtime_ tested, too.
>>> Fwiw, I do not have time to test those personally, he would need to
>>> find someone else if that requirement really holds true.
>>>
>>> I would not really like to fix bugs appearing in that code to get my
>>> features in.
>>>
>>> Also, since my change has been around for 2-3 months now, I would
>>> really prefer not to be forced to rewrite it again from scratch.
>>> Surely, you can wait with those, more or less, cosmetic non-runtime
>>> tested changes?
>>>
>>> This would impose me a lot of additional work again, and I personally
>>> do not see the benefit of it. In my book at least, feature is over
>>> internal polishing.
>>
>> Right, I've had enough. I'm removing your patch from the MFD tree.
>>
>> I've asked too many people to give you a second chance and asked you
>> privately to behave yourself and treat others with respect. So far I
>> haven't seen an ounce of self control or depomacy from you.
>>
>> This is how it's going to work from now on:
>>
>>   - You submit a patch
>>   - It gets reviewed                            <----\
>>   - You fix up the review comments as requested -----/
>>   - Non-compliance or arguments with the _experts_ results in:
>>      `$INTEREST > /dev/null || \
>>        grep "From: Laszio Papp" ~/.mail | xargs rm -rf`
>
> http://comments.gmane.org/gmane.linux.kernel/1645251
>
> Step 2 did not happen. I did not get any review for my change. I
> literally submitted that within a couple of hours after the request.
>

If you had tested your patch on real or simulated hardware,
you might have noticed a crash whenever you accessed any
of the attributes. So you did not test your patch.

Instead of trying to educate you how the conversion to the
new API works, I decided to help you out a bit and do
the conversion myself. I did some cleanup before, since
that made the actual feature patch easier for me to implement,
and I did some more cleanup afterwards just because I like
cleaning up code.

I had hoped that you might find the time to test the result,
but it appears that won't happen. I am gracious to Jean that
he took the time to review the changes and even test the
result in simulation, even though I know he is very busy.
So I consider the changes to be good enough to be made
available in my -staging tree, which I did by now.
I'll move them over to -next once I have the chance to test
on real hardware or after I get a Tested-by: from someone
with real hardware.

Guenter


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

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Guenter Roeck]

On 02/13/2014 03:58 AM, Jean Delvare wrote:
> Hi Lee,
>
> On Thu, 13 Feb 2014 11:16:07 +0000, Lee Jones wrote:
>> On Thu, 13 Feb 2014, Jean Delvare wrote:
>>> Guenter just did:
>>>
>>> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041224.html
>>
>> Nice, FWIW:
>>    Acked-by: Lee Jones <lee.jones@linaro.org>
>>
>>> Any change to the max6650 driver should go on top of his patch series
>>> to avoid conflicts:
>>>
>>> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html
>>
>> Do you have a tree Laszlo can rebase on top of?
>
> Now that the patches have my Reviewed-by and your Acked-by, I believe
> Guenter will add them shortly to:
>
> http://git.kernel.org/cgit/linux/kernel/git/groeck/linux-staging.git/log/?h=hwmon-staging
>
It is there now (in the hwmon-staging branch as Jean suggested).

Jean, Lee, thanks a lot for the reviews and simulation testing.
I'll try to get samples and test on real hardware before moving
the patches into hwmon-next.

Thanks,
Guenter

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Guenter Roeck]

On 02/13/2014 03:58 AM, Jean Delvare wrote:
> Hi Lee,
>
> On Thu, 13 Feb 2014 11:16:07 +0000, Lee Jones wrote:
>> On Thu, 13 Feb 2014, Jean Delvare wrote:
>>> Guenter just did:
>>>
>>> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041224.html
>>
>> Nice, FWIW:
>>    Acked-by: Lee Jones <lee.jones@linaro.org>
>>
>>> Any change to the max6650 driver should go on top of his patch series
>>> to avoid conflicts:
>>>
>>> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html
>>
>> Do you have a tree Laszlo can rebase on top of?
>
> Now that the patches have my Reviewed-by and your Acked-by, I believe
> Guenter will add them shortly to:
>
> http://git.kernel.org/cgit/linux/kernel/git/groeck/linux-staging.git/log/?h=hwmon-staging
>
It is there now (in the hwmon-staging branch as Jean suggested).

Jean, Lee, thanks a lot for the reviews and simulation testing.
I'll try to get samples and test on real hardware before moving
the patches into hwmon-next.

Thanks,
Guenter


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

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Laszlo Papp]

On Thu, Feb 13, 2014 at 4:16 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 02/13/2014 04:27 AM, Laszlo Papp wrote:
>>
>> On Thu, Feb 13, 2014 at 11:33 AM, Lee Jones <lee.jones@linaro.org> wrote:
>>>>>>>>
>>>>>>>> -static int max6650_probe(struct i2c_client *client,
>>>>>>>> -                    const struct i2c_device_id *id);
>>>>>>>> -static int max6650_init_client(struct i2c_client *client);
>>>>>>>> -static int max6650_remove(struct i2c_client *client);
>>>>>>>> +static int max6650_probe(struct platform_device *pdev);
>>>>>>>> +static int max6650_init_client(struct platform_device *pdev);
>>>>>>>> +static int max6650_remove(struct platform_device *pdev);
>>>>>>>>   static struct max6650_data *max6650_update_device(struct device
>>>>>>>> *dev);
>>>>>>>
>>>>>>>
>>>>>>> It would be good to remove these forward declarations in the future.
>>>>>>>
>>>>>>> If no one volunteers I'll happily do it.
>>>>>>
>>>>>>
>>>>>> Guenter just did:
>>>>>>
>>>>>>
>>>>>> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041224.html
>>>>>>
>>>>>> Any change to the max6650 driver should go on top of his patch series
>>>>>> to avoid conflicts:
>>>>>>
>>>>>>
>>>>>> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html
>>>>>
>>>>>
>>>> As far as I can see, that patch set was not even tested, so how can it
>>>> go in? I was told that any patch should be _runtime_ tested, too.
>>>> Fwiw, I do not have time to test those personally, he would need to
>>>> find someone else if that requirement really holds true.
>>>>
>>>> I would not really like to fix bugs appearing in that code to get my
>>>> features in.
>>>>
>>>> Also, since my change has been around for 2-3 months now, I would
>>>> really prefer not to be forced to rewrite it again from scratch.
>>>> Surely, you can wait with those, more or less, cosmetic non-runtime
>>>> tested changes?
>>>>
>>>> This would impose me a lot of additional work again, and I personally
>>>> do not see the benefit of it. In my book at least, feature is over
>>>> internal polishing.
>>>
>>>
>>> Right, I've had enough. I'm removing your patch from the MFD tree.
>>>
>>> I've asked too many people to give you a second chance and asked you
>>> privately to behave yourself and treat others with respect. So far I
>>> haven't seen an ounce of self control or depomacy from you.
>>>
>>> This is how it's going to work from now on:
>>>
>>>   - You submit a patch
>>>   - It gets reviewed                            <----\
>>>   - You fix up the review comments as requested -----/
>>>   - Non-compliance or arguments with the _experts_ results in:
>>>      `$INTEREST > /dev/null || \
>>>        grep "From: Laszio Papp" ~/.mail | xargs rm -rf`
>>
>>
>> http://comments.gmane.org/gmane.linux.kernel/1645251
>>
>> Step 2 did not happen. I did not get any review for my change. I
>> literally submitted that within a couple of hours after the request.
>>
>
> If you had tested your patch on real or simulated hardware,
> you might have noticed a crash whenever you accessed any
> of the attributes. So you did not test your patch.
>
> Instead of trying to educate you how the conversion to the
> new API works, I decided to help you out a bit and do
> the conversion myself.

I am afraid that with this attitude and approach towards potential new
contributors, you will put more and more work on you making it more
difficult to get things done at large. "Educating" new people is a win
in the long run, and this is a general practice out there before
claiming that I am crazy (e.g. someone even told me in the kernel
circles that is "rude").

I am surprised that mentoring new people is considered bad idea in
here. Since there is no guarantee you would not do it next time if I
tried to put effort into contributing, I feel more comfortable to skip
this project for now, and work where I know this cannot happen.
Currently, I do not feel inclusive in this project due to this
approach.

I do not even claim that you would be this to me intentionally, but it
has happened either way.

> I did some cleanup before, since
> that made the actual feature patch easier for me to implement,
> and I did some more cleanup afterwards just because I like
> cleaning up code.

IMO, it is not worth making the git history noisy with needless
re-arranging like forward headers, but that is just my opinion. Please
do not say, it is rude and not respectful. I definitely respect your
opinion, I just do not agree with it, but that is fine, you are a
maintainer so you get to decide, I guess. That being said, perhaps I
missed something why such re-arranging outweighs the git history
disadvantage.

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Laszlo Papp]

On Thu, Feb 13, 2014 at 4:16 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 02/13/2014 04:27 AM, Laszlo Papp wrote:
>>
>> On Thu, Feb 13, 2014 at 11:33 AM, Lee Jones <lee.jones@linaro.org> wrote:
>>>>>>>>
>>>>>>>> -static int max6650_probe(struct i2c_client *client,
>>>>>>>> -                    const struct i2c_device_id *id);
>>>>>>>> -static int max6650_init_client(struct i2c_client *client);
>>>>>>>> -static int max6650_remove(struct i2c_client *client);
>>>>>>>> +static int max6650_probe(struct platform_device *pdev);
>>>>>>>> +static int max6650_init_client(struct platform_device *pdev);
>>>>>>>> +static int max6650_remove(struct platform_device *pdev);
>>>>>>>>   static struct max6650_data *max6650_update_device(struct device
>>>>>>>> *dev);
>>>>>>>
>>>>>>>
>>>>>>> It would be good to remove these forward declarations in the future.
>>>>>>>
>>>>>>> If no one volunteers I'll happily do it.
>>>>>>
>>>>>>
>>>>>> Guenter just did:
>>>>>>
>>>>>>
>>>>>> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041224.html
>>>>>>
>>>>>> Any change to the max6650 driver should go on top of his patch series
>>>>>> to avoid conflicts:
>>>>>>
>>>>>>
>>>>>> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html
>>>>>
>>>>>
>>>> As far as I can see, that patch set was not even tested, so how can it
>>>> go in? I was told that any patch should be _runtime_ tested, too.
>>>> Fwiw, I do not have time to test those personally, he would need to
>>>> find someone else if that requirement really holds true.
>>>>
>>>> I would not really like to fix bugs appearing in that code to get my
>>>> features in.
>>>>
>>>> Also, since my change has been around for 2-3 months now, I would
>>>> really prefer not to be forced to rewrite it again from scratch.
>>>> Surely, you can wait with those, more or less, cosmetic non-runtime
>>>> tested changes?
>>>>
>>>> This would impose me a lot of additional work again, and I personally
>>>> do not see the benefit of it. In my book at least, feature is over
>>>> internal polishing.
>>>
>>>
>>> Right, I've had enough. I'm removing your patch from the MFD tree.
>>>
>>> I've asked too many people to give you a second chance and asked you
>>> privately to behave yourself and treat others with respect. So far I
>>> haven't seen an ounce of self control or depomacy from you.
>>>
>>> This is how it's going to work from now on:
>>>
>>>   - You submit a patch
>>>   - It gets reviewed                            <----\
>>>   - You fix up the review comments as requested -----/
>>>   - Non-compliance or arguments with the _experts_ results in:
>>>      `$INTEREST > /dev/null || \
>>>        grep "From: Laszio Papp" ~/.mail | xargs rm -rf`
>>
>>
>> http://comments.gmane.org/gmane.linux.kernel/1645251
>>
>> Step 2 did not happen. I did not get any review for my change. I
>> literally submitted that within a couple of hours after the request.
>>
>
> If you had tested your patch on real or simulated hardware,
> you might have noticed a crash whenever you accessed any
> of the attributes. So you did not test your patch.
>
> Instead of trying to educate you how the conversion to the
> new API works, I decided to help you out a bit and do
> the conversion myself.

I am afraid that with this attitude and approach towards potential new
contributors, you will put more and more work on you making it more
difficult to get things done at large. "Educating" new people is a win
in the long run, and this is a general practice out there before
claiming that I am crazy (e.g. someone even told me in the kernel
circles that is "rude").

I am surprised that mentoring new people is considered bad idea in
here. Since there is no guarantee you would not do it next time if I
tried to put effort into contributing, I feel more comfortable to skip
this project for now, and work where I know this cannot happen.
Currently, I do not feel inclusive in this project due to this
approach.

I do not even claim that you would be this to me intentionally, but it
has happened either way.

> I did some cleanup before, since
> that made the actual feature patch easier for me to implement,
> and I did some more cleanup afterwards just because I like
> cleaning up code.

IMO, it is not worth making the git history noisy with needless
re-arranging like forward headers, but that is just my opinion. Please
do not say, it is rude and not respectful. I definitely respect your
opinion, I just do not agree with it, but that is fine, you are a
maintainer so you get to decide, I guess. That being said, perhaps I
missed something why such re-arranging outweighs the git history
disadvantage.

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

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Laszlo Papp]

On Thu, Feb 13, 2014 at 12:40 PM, Lee Jones <lee.jones@linaro.org> wrote:
>> http://comments.gmane.org/gmane.linux.kernel/1645251
>>
>> Step 2 did not happen. I did not get any review for my change. I
>> literally submitted that within a couple of hours after the request.
>>
>> Could you please tell me what was wrong with that change, and why I
>> did not get any respect not to "xargs rm -rf" my work in that area? I
>> believe I was ignored instead of improving the change, and someone
>> else tried to address the same thing. There was no argument in that
>> thread. It was a technical change. I personally do not feel happy
>> about it.
>
> Let's start again.
>
> Rebase your work on top of the HWMON tree on kernel.org and resubmit
> the entire set. If rebasing takes you more than 20 mins, you're
> probably doing it wrong.

I tried, but I could not manage it within 20 minutes, so I guess I am
doing something wrong. Can you please provide some pointers how not to
do it wrong? Perhaps, I am not aware of some tricks.

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Laszlo Papp]

On Thu, Feb 13, 2014 at 12:40 PM, Lee Jones <lee.jones@linaro.org> wrote:
>> http://comments.gmane.org/gmane.linux.kernel/1645251
>>
>> Step 2 did not happen. I did not get any review for my change. I
>> literally submitted that within a couple of hours after the request.
>>
>> Could you please tell me what was wrong with that change, and why I
>> did not get any respect not to "xargs rm -rf" my work in that area? I
>> believe I was ignored instead of improving the change, and someone
>> else tried to address the same thing. There was no argument in that
>> thread. It was a technical change. I personally do not feel happy
>> about it.
>
> Let's start again.
>
> Rebase your work on top of the HWMON tree on kernel.org and resubmit
> the entire set. If rebasing takes you more than 20 mins, you're
> probably doing it wrong.

I tried, but I could not manage it within 20 minutes, so I guess I am
doing something wrong. Can you please provide some pointers how not to
do it wrong? Perhaps, I am not aware of some tricks.

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

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Lee Jones]

> >> http://comments.gmane.org/gmane.linux.kernel/1645251
> >>
> >> Step 2 did not happen. I did not get any review for my change. I
> >> literally submitted that within a couple of hours after the request.
> >>
> >> Could you please tell me what was wrong with that change, and why I
> >> did not get any respect not to "xargs rm -rf" my work in that area? I
> >> believe I was ignored instead of improving the change, and someone
> >> else tried to address the same thing. There was no argument in that
> >> thread. It was a technical change. I personally do not feel happy
> >> about it.
> >
> > Let's start again.
> >
> > Rebase your work on top of the HWMON tree on kernel.org and resubmit
> > the entire set. If rebasing takes you more than 20 mins, you're
> > probably doing it wrong.
> 
> I tried, but I could not manage it within 20 minutes, so I guess I am
> doing something wrong. Can you please provide some pointers how not to
> do it wrong? Perhaps, I am not aware of some tricks.

One question, are you still working on this stuff or not? I'm confused
by the disparity in your messages. I'm going to guess that you're in
for now.

Do:
  `git rebase -i <base> --onto <newbase>`
Where:
  <base> is the SHA1 of the first patch below your changes in `git log`
  <new_base> is Guenter's staging tree on kernel.org

Ensure you're rebasing all of your patches (and patches that aren't
yours) when your $EDITOR pops up. If they are wrong, delete all the
lines in the file and the rebase will be aborted. If they're correct
save and close your $EDITOR.

You'll receive conflicts. You can see the state of the conflicts using
`git status` Open the file, find the conflict markers and make a choice
from the HEAD section or the section from your patch. Sometimes
you'll need to manually merge the two, if there are changes from both
refs that you want to keep. Once you're happy `git commit -a` and `git
rebase --continue`. Each conflict should not take you long, but if it
does, keep at it, as it's good practice. After a time of doing it,
you'll be able to fix merge conflicts in no time at all.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Lee Jones]

PiA+PiBodHRwOi8vY29tbWVudHMuZ21hbmUub3JnL2dtYW5lLmxpbnV4Lmtlcm5lbC8xNjQ1MjUx
Cj4gPj4KPiA+PiBTdGVwIDIgZGlkIG5vdCBoYXBwZW4uIEkgZGlkIG5vdCBnZXQgYW55IHJldmll
dyBmb3IgbXkgY2hhbmdlLiBJCj4gPj4gbGl0ZXJhbGx5IHN1Ym1pdHRlZCB0aGF0IHdpdGhpbiBh
IGNvdXBsZSBvZiBob3VycyBhZnRlciB0aGUgcmVxdWVzdC4KPiA+Pgo+ID4+IENvdWxkIHlvdSBw
bGVhc2UgdGVsbCBtZSB3aGF0IHdhcyB3cm9uZyB3aXRoIHRoYXQgY2hhbmdlLCBhbmQgd2h5IEkK
PiA+PiBkaWQgbm90IGdldCBhbnkgcmVzcGVjdCBub3QgdG8gInhhcmdzIHJtIC1yZiIgbXkgd29y
ayBpbiB0aGF0IGFyZWE/IEkKPiA+PiBiZWxpZXZlIEkgd2FzIGlnbm9yZWQgaW5zdGVhZCBvZiBp
bXByb3ZpbmcgdGhlIGNoYW5nZSwgYW5kIHNvbWVvbmUKPiA+PiBlbHNlIHRyaWVkIHRvIGFkZHJl
c3MgdGhlIHNhbWUgdGhpbmcuIFRoZXJlIHdhcyBubyBhcmd1bWVudCBpbiB0aGF0Cj4gPj4gdGhy
ZWFkLiBJdCB3YXMgYSB0ZWNobmljYWwgY2hhbmdlLiBJIHBlcnNvbmFsbHkgZG8gbm90IGZlZWwg
aGFwcHkKPiA+PiBhYm91dCBpdC4KPiA+Cj4gPiBMZXQncyBzdGFydCBhZ2Fpbi4KPiA+Cj4gPiBS
ZWJhc2UgeW91ciB3b3JrIG9uIHRvcCBvZiB0aGUgSFdNT04gdHJlZSBvbiBrZXJuZWwub3JnIGFu
ZCByZXN1Ym1pdAo+ID4gdGhlIGVudGlyZSBzZXQuIElmIHJlYmFzaW5nIHRha2VzIHlvdSBtb3Jl
IHRoYW4gMjAgbWlucywgeW91J3JlCj4gPiBwcm9iYWJseSBkb2luZyBpdCB3cm9uZy4KPiAKPiBJ
IHRyaWVkLCBidXQgSSBjb3VsZCBub3QgbWFuYWdlIGl0IHdpdGhpbiAyMCBtaW51dGVzLCBzbyBJ
IGd1ZXNzIEkgYW0KPiBkb2luZyBzb21ldGhpbmcgd3JvbmcuIENhbiB5b3UgcGxlYXNlIHByb3Zp
ZGUgc29tZSBwb2ludGVycyBob3cgbm90IHRvCj4gZG8gaXQgd3Jvbmc/IFBlcmhhcHMsIEkgYW0g
bm90IGF3YXJlIG9mIHNvbWUgdHJpY2tzLgoKT25lIHF1ZXN0aW9uLCBhcmUgeW91IHN0aWxsIHdv
cmtpbmcgb24gdGhpcyBzdHVmZiBvciBub3Q/IEknbSBjb25mdXNlZApieSB0aGUgZGlzcGFyaXR5
IGluIHlvdXIgbWVzc2FnZXMuIEknbSBnb2luZyB0byBndWVzcyB0aGF0IHlvdSdyZSBpbgpmb3Ig
bm93LgoKRG86CiAgYGdpdCByZWJhc2UgLWkgPGJhc2U+IC0tb250byA8bmV3YmFzZT5gCldoZXJl
OgogIDxiYXNlPiBpcyB0aGUgU0hBMSBvZiB0aGUgZmlyc3QgcGF0Y2ggYmVsb3cgeW91ciBjaGFu
Z2VzIGluIGBnaXQgbG9nYAogIDxuZXdfYmFzZT4gaXMgR3VlbnRlcidzIHN0YWdpbmcgdHJlZSBv
biBrZXJuZWwub3JnCgpFbnN1cmUgeW91J3JlIHJlYmFzaW5nIGFsbCBvZiB5b3VyIHBhdGNoZXMg
KGFuZCBwYXRjaGVzIHRoYXQgYXJlbid0CnlvdXJzKSB3aGVuIHlvdXIgJEVESVRPUiBwb3BzIHVw
LiBJZiB0aGV5IGFyZSB3cm9uZywgZGVsZXRlIGFsbCB0aGUKbGluZXMgaW4gdGhlIGZpbGUgYW5k
IHRoZSByZWJhc2Ugd2lsbCBiZSBhYm9ydGVkLiBJZiB0aGV5J3JlIGNvcnJlY3QKc2F2ZSBhbmQg
Y2xvc2UgeW91ciAkRURJVE9SLgoKWW91J2xsIHJlY2VpdmUgY29uZmxpY3RzLiBZb3UgY2FuIHNl
ZSB0aGUgc3RhdGUgb2YgdGhlIGNvbmZsaWN0cyB1c2luZwpgZ2l0IHN0YXR1c2AgT3BlbiB0aGUg
ZmlsZSwgZmluZCB0aGUgY29uZmxpY3QgbWFya2VycyBhbmQgbWFrZSBhIGNob2ljZQpmcm9tIHRo
ZSBIRUFEIHNlY3Rpb24gb3IgdGhlIHNlY3Rpb24gZnJvbSB5b3VyIHBhdGNoLiBTb21ldGltZXMK
eW91J2xsIG5lZWQgdG8gbWFudWFsbHkgbWVyZ2UgdGhlIHR3bywgaWYgdGhlcmUgYXJlIGNoYW5n
ZXMgZnJvbSBib3RoCnJlZnMgdGhhdCB5b3Ugd2FudCB0byBrZWVwLiBPbmNlIHlvdSdyZSBoYXBw
eSBgZ2l0IGNvbW1pdCAtYWAgYW5kIGBnaXQKcmViYXNlIC0tY29udGludWVgLiBFYWNoIGNvbmZs
aWN0IHNob3VsZCBub3QgdGFrZSB5b3UgbG9uZywgYnV0IGlmIGl0CmRvZXMsIGtlZXAgYXQgaXQs
IGFzIGl0J3MgZ29vZCBwcmFjdGljZS4gQWZ0ZXIgYSB0aW1lIG9mIGRvaW5nIGl0LAp5b3UnbGwg
YmUgYWJsZSB0byBmaXggbWVyZ2UgY29uZmxpY3RzIGluIG5vIHRpbWUgYXQgYWxsLgoKLS0gCkxl
ZSBKb25lcwpMaW5hcm8gU1RNaWNyb2VsZWN0cm9uaWNzIExhbmRpbmcgVGVhbSBMZWFkCkxpbmFy
by5vcmcg4pSCIE9wZW4gc291cmNlIHNvZnR3YXJlIGZvciBBUk0gU29DcwpGb2xsb3cgTGluYXJv
OiBGYWNlYm9vayB8IFR3aXR0ZXIgfCBCbG9nCgpfX19fX19fX19fX19fX19fX19fX19fX19fX19f
X19fX19fX19fX19fX19fX19fXwpsbS1zZW5zb3JzIG1haWxpbmcgbGlzdApsbS1zZW5zb3JzQGxt
LXNlbnNvcnMub3JnCmh0dHA6Ly9saXN0cy5sbS1zZW5zb3JzLm9yZy9tYWlsbWFuL2xpc3RpbmZv
L2xtLXNlbnNvcnM

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Lee Jones]

> >>>>>>>> -static int max6650_probe(struct i2c_client *client,
> >>>>>>>> -                    const struct i2c_device_id *id);
> >>>>>>>> -static int max6650_init_client(struct i2c_client *client);
> >>>>>>>> -static int max6650_remove(struct i2c_client *client);
> >>>>>>>> +static int max6650_probe(struct platform_device *pdev);
> >>>>>>>> +static int max6650_init_client(struct platform_device *pdev);
> >>>>>>>> +static int max6650_remove(struct platform_device *pdev);
> >>>>>>>>   static struct max6650_data *max6650_update_device(struct device
> >>>>>>>> *dev);
> >>>>>>>
> >>>>>>>
> >>>>>>> It would be good to remove these forward declarations in the future.
> >>>>>>>
> >>>>>>> If no one volunteers I'll happily do it.
> >>>>>>
> >>>>>>
> >>>>>> Guenter just did:
> >>>>>>
> >>>>>>
> >>>>>> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041224.html
> >>>>>>
> >>>>>> Any change to the max6650 driver should go on top of his patch series
> >>>>>> to avoid conflicts:
> >>>>>>
> >>>>>>
> >>>>>> http://lists.lm-sensors.org/pipermail/lm-sensors/2014-February/041223.html
> >>>>>
> >>>>>
> >>>> As far as I can see, that patch set was not even tested, so how can it
> >>>> go in? I was told that any patch should be _runtime_ tested, too.
> >>>> Fwiw, I do not have time to test those personally, he would need to
> >>>> find someone else if that requirement really holds true.
> >>>>
> >>>> I would not really like to fix bugs appearing in that code to get my
> >>>> features in.
> >>>>
> >>>> Also, since my change has been around for 2-3 months now, I would
> >>>> really prefer not to be forced to rewrite it again from scratch.
> >>>> Surely, you can wait with those, more or less, cosmetic non-runtime
> >>>> tested changes?
> >>>>
> >>>> This would impose me a lot of additional work again, and I personally
> >>>> do not see the benefit of it. In my book at least, feature is over
> >>>> internal polishing.
> >>>
> >>>
> >>> Right, I've had enough. I'm removing your patch from the MFD tree.
> >>>
> >>> I've asked too many people to give you a second chance and asked you
> >>> privately to behave yourself and treat others with respect. So far I
> >>> haven't seen an ounce of self control or depomacy from you.
> >>>
> >>> This is how it's going to work from now on:
> >>>
> >>>   - You submit a patch
> >>>   - It gets reviewed                            <----\
> >>>   - You fix up the review comments as requested -----/
> >>>   - Non-compliance or arguments with the _experts_ results in:
> >>>      `$INTEREST > /dev/null || \
> >>>        grep "From: Laszio Papp" ~/.mail | xargs rm -rf`
> >>
> >>
> >> http://comments.gmane.org/gmane.linux.kernel/1645251
> >>
> >> Step 2 did not happen. I did not get any review for my change. I
> >> literally submitted that within a couple of hours after the request.
> >>
> >
> > If you had tested your patch on real or simulated hardware,
> > you might have noticed a crash whenever you accessed any
> > of the attributes. So you did not test your patch.
> >
> > Instead of trying to educate you how the conversion to the
> > new API works, I decided to help you out a bit and do
> > the conversion myself.
> 
> I am afraid that with this attitude and approach towards potential new
> contributors, you will put more and more work on you making it more
> difficult to get things done at large. "Educating" new people is a win
> in the long run, and this is a general practice out there before
> claiming that I am crazy (e.g. someone even told me in the kernel
> circles that is "rude").
> 
> I am surprised that mentoring new people is considered bad idea in
> here. Since there is no guarantee you would not do it next time if I
> tried to put effort into contributing, I feel more comfortable to skip
> this project for now, and work where I know this cannot happen.
> Currently, I do not feel inclusive in this project due to this
> approach.
> 
> I do not even claim that you would be this to me intentionally, but it
> has happened either way.

I agree that if Guenter was to go ahead and convert this driver to an
MFD one, then that would be rude conduct. However, the work undertaken
by him was 'add-on' work which you clearly weren't comfortable with. I
think he was genuinely doing you a favour rather than 'stealing your
thunder'. His work has paved the way for yours and should make your
work easier in the long run.

NB: Rebasing on top of different HEADs and other people's work is just
a fact of life in the kernel. If you find it difficult, stick
around. You'll get better at it with practice.

> > I did some cleanup before, since
> > that made the actual feature patch easier for me to implement,
> > and I did some more cleanup afterwards just because I like
> > cleaning up code.
> 
> IMO, it is not worth making the git history noisy with needless
> re-arranging like forward headers, but that is just my opinion. Please
> do not say, it is rude and not respectful. I definitely respect your
> opinion, I just do not agree with it, but that is fine, you are a
> maintainer so you get to decide, I guess. That being said, perhaps I
> missed something why such re-arranging outweighs the git history
> disadvantage.

=:-D

We have nearly half a million commits in the Mainline kernel since
v2.6.12. I don't think Git history is even a consideration here. Also,
we really don't like forward declarations in the kernel. Guenter's
clean-ups where good ones. We can't stop cleaning up and developing in
the fear that someone else is working on that same driver. We have
lots of people making changes to the same files all over the kernel.
Get good at `git rebase`ing and there is no issue, it's just another
day in the life of a Kernel Developer.

If you'd spend more time rebasing and coding as you do writing emails,
you'd have had this done by now. :)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Lee Jones]

PiA+Pj4+Pj4+PiAtc3RhdGljIGludCBtYXg2NjUwX3Byb2JlKHN0cnVjdCBpMmNfY2xpZW50ICpj
bGllbnQsCj4gPj4+Pj4+Pj4gLSAgICAgICAgICAgICAgICAgICAgY29uc3Qgc3RydWN0IGkyY19k
ZXZpY2VfaWQgKmlkKTsKPiA+Pj4+Pj4+PiAtc3RhdGljIGludCBtYXg2NjUwX2luaXRfY2xpZW50
KHN0cnVjdCBpMmNfY2xpZW50ICpjbGllbnQpOwo+ID4+Pj4+Pj4+IC1zdGF0aWMgaW50IG1heDY2
NTBfcmVtb3ZlKHN0cnVjdCBpMmNfY2xpZW50ICpjbGllbnQpOwo+ID4+Pj4+Pj4+ICtzdGF0aWMg
aW50IG1heDY2NTBfcHJvYmUoc3RydWN0IHBsYXRmb3JtX2RldmljZSAqcGRldik7Cj4gPj4+Pj4+
Pj4gK3N0YXRpYyBpbnQgbWF4NjY1MF9pbml0X2NsaWVudChzdHJ1Y3QgcGxhdGZvcm1fZGV2aWNl
ICpwZGV2KTsKPiA+Pj4+Pj4+PiArc3RhdGljIGludCBtYXg2NjUwX3JlbW92ZShzdHJ1Y3QgcGxh
dGZvcm1fZGV2aWNlICpwZGV2KTsKPiA+Pj4+Pj4+PiAgIHN0YXRpYyBzdHJ1Y3QgbWF4NjY1MF9k
YXRhICptYXg2NjUwX3VwZGF0ZV9kZXZpY2Uoc3RydWN0IGRldmljZQo+ID4+Pj4+Pj4+ICpkZXYp
Owo+ID4+Pj4+Pj4KPiA+Pj4+Pj4+Cj4gPj4+Pj4+PiBJdCB3b3VsZCBiZSBnb29kIHRvIHJlbW92
ZSB0aGVzZSBmb3J3YXJkIGRlY2xhcmF0aW9ucyBpbiB0aGUgZnV0dXJlLgo+ID4+Pj4+Pj4KPiA+
Pj4+Pj4+IElmIG5vIG9uZSB2b2x1bnRlZXJzIEknbGwgaGFwcGlseSBkbyBpdC4KPiA+Pj4+Pj4K
PiA+Pj4+Pj4KPiA+Pj4+Pj4gR3VlbnRlciBqdXN0IGRpZDoKPiA+Pj4+Pj4KPiA+Pj4+Pj4KPiA+
Pj4+Pj4gaHR0cDovL2xpc3RzLmxtLXNlbnNvcnMub3JnL3BpcGVybWFpbC9sbS1zZW5zb3JzLzIw
MTQtRmVicnVhcnkvMDQxMjI0Lmh0bWwKPiA+Pj4+Pj4KPiA+Pj4+Pj4gQW55IGNoYW5nZSB0byB0
aGUgbWF4NjY1MCBkcml2ZXIgc2hvdWxkIGdvIG9uIHRvcCBvZiBoaXMgcGF0Y2ggc2VyaWVzCj4g
Pj4+Pj4+IHRvIGF2b2lkIGNvbmZsaWN0czoKPiA+Pj4+Pj4KPiA+Pj4+Pj4KPiA+Pj4+Pj4gaHR0
cDovL2xpc3RzLmxtLXNlbnNvcnMub3JnL3BpcGVybWFpbC9sbS1zZW5zb3JzLzIwMTQtRmVicnVh
cnkvMDQxMjIzLmh0bWwKPiA+Pj4+Pgo+ID4+Pj4+Cj4gPj4+PiBBcyBmYXIgYXMgSSBjYW4gc2Vl
LCB0aGF0IHBhdGNoIHNldCB3YXMgbm90IGV2ZW4gdGVzdGVkLCBzbyBob3cgY2FuIGl0Cj4gPj4+
PiBnbyBpbj8gSSB3YXMgdG9sZCB0aGF0IGFueSBwYXRjaCBzaG91bGQgYmUgX3J1bnRpbWVfIHRl
c3RlZCwgdG9vLgo+ID4+Pj4gRndpdywgSSBkbyBub3QgaGF2ZSB0aW1lIHRvIHRlc3QgdGhvc2Ug
cGVyc29uYWxseSwgaGUgd291bGQgbmVlZCB0bwo+ID4+Pj4gZmluZCBzb21lb25lIGVsc2UgaWYg
dGhhdCByZXF1aXJlbWVudCByZWFsbHkgaG9sZHMgdHJ1ZS4KPiA+Pj4+Cj4gPj4+PiBJIHdvdWxk
IG5vdCByZWFsbHkgbGlrZSB0byBmaXggYnVncyBhcHBlYXJpbmcgaW4gdGhhdCBjb2RlIHRvIGdl
dCBteQo+ID4+Pj4gZmVhdHVyZXMgaW4uCj4gPj4+Pgo+ID4+Pj4gQWxzbywgc2luY2UgbXkgY2hh
bmdlIGhhcyBiZWVuIGFyb3VuZCBmb3IgMi0zIG1vbnRocyBub3csIEkgd291bGQKPiA+Pj4+IHJl
YWxseSBwcmVmZXIgbm90IHRvIGJlIGZvcmNlZCB0byByZXdyaXRlIGl0IGFnYWluIGZyb20gc2Ny
YXRjaC4KPiA+Pj4+IFN1cmVseSwgeW91IGNhbiB3YWl0IHdpdGggdGhvc2UsIG1vcmUgb3IgbGVz
cywgY29zbWV0aWMgbm9uLXJ1bnRpbWUKPiA+Pj4+IHRlc3RlZCBjaGFuZ2VzPwo+ID4+Pj4KPiA+
Pj4+IFRoaXMgd291bGQgaW1wb3NlIG1lIGEgbG90IG9mIGFkZGl0aW9uYWwgd29yayBhZ2Fpbiwg
YW5kIEkgcGVyc29uYWxseQo+ID4+Pj4gZG8gbm90IHNlZSB0aGUgYmVuZWZpdCBvZiBpdC4gSW4g
bXkgYm9vayBhdCBsZWFzdCwgZmVhdHVyZSBpcyBvdmVyCj4gPj4+PiBpbnRlcm5hbCBwb2xpc2hp
bmcuCj4gPj4+Cj4gPj4+Cj4gPj4+IFJpZ2h0LCBJJ3ZlIGhhZCBlbm91Z2guIEknbSByZW1vdmlu
ZyB5b3VyIHBhdGNoIGZyb20gdGhlIE1GRCB0cmVlLgo+ID4+Pgo+ID4+PiBJJ3ZlIGFza2VkIHRv
byBtYW55IHBlb3BsZSB0byBnaXZlIHlvdSBhIHNlY29uZCBjaGFuY2UgYW5kIGFza2VkIHlvdQo+
ID4+PiBwcml2YXRlbHkgdG8gYmVoYXZlIHlvdXJzZWxmIGFuZCB0cmVhdCBvdGhlcnMgd2l0aCBy
ZXNwZWN0LiBTbyBmYXIgSQo+ID4+PiBoYXZlbid0IHNlZW4gYW4gb3VuY2Ugb2Ygc2VsZiBjb250
cm9sIG9yIGRlcG9tYWN5IGZyb20geW91Lgo+ID4+Pgo+ID4+PiBUaGlzIGlzIGhvdyBpdCdzIGdv
aW5nIHRvIHdvcmsgZnJvbSBub3cgb246Cj4gPj4+Cj4gPj4+ICAgLSBZb3Ugc3VibWl0IGEgcGF0
Y2gKPiA+Pj4gICAtIEl0IGdldHMgcmV2aWV3ZWQgICAgICAgICAgICAgICAgICAgICAgICAgICAg
PC0tLS1cCj4gPj4+ICAgLSBZb3UgZml4IHVwIHRoZSByZXZpZXcgY29tbWVudHMgYXMgcmVxdWVz
dGVkIC0tLS0tLwo+ID4+PiAgIC0gTm9uLWNvbXBsaWFuY2Ugb3IgYXJndW1lbnRzIHdpdGggdGhl
IF9leHBlcnRzXyByZXN1bHRzIGluOgo+ID4+PiAgICAgIGAkSU5URVJFU1QgPiAvZGV2L251bGwg
fHwgXAo+ID4+PiAgICAgICAgZ3JlcCAiRnJvbTogTGFzemlvIFBhcHAiIH4vLm1haWwgfCB4YXJn
cyBybSAtcmZgCj4gPj4KPiA+Pgo+ID4+IGh0dHA6Ly9jb21tZW50cy5nbWFuZS5vcmcvZ21hbmUu
bGludXgua2VybmVsLzE2NDUyNTEKPiA+Pgo+ID4+IFN0ZXAgMiBkaWQgbm90IGhhcHBlbi4gSSBk
aWQgbm90IGdldCBhbnkgcmV2aWV3IGZvciBteSBjaGFuZ2UuIEkKPiA+PiBsaXRlcmFsbHkgc3Vi
bWl0dGVkIHRoYXQgd2l0aGluIGEgY291cGxlIG9mIGhvdXJzIGFmdGVyIHRoZSByZXF1ZXN0Lgo+
ID4+Cj4gPgo+ID4gSWYgeW91IGhhZCB0ZXN0ZWQgeW91ciBwYXRjaCBvbiByZWFsIG9yIHNpbXVs
YXRlZCBoYXJkd2FyZSwKPiA+IHlvdSBtaWdodCBoYXZlIG5vdGljZWQgYSBjcmFzaCB3aGVuZXZl
ciB5b3UgYWNjZXNzZWQgYW55Cj4gPiBvZiB0aGUgYXR0cmlidXRlcy4gU28geW91IGRpZCBub3Qg
dGVzdCB5b3VyIHBhdGNoLgo+ID4KPiA+IEluc3RlYWQgb2YgdHJ5aW5nIHRvIGVkdWNhdGUgeW91
IGhvdyB0aGUgY29udmVyc2lvbiB0byB0aGUKPiA+IG5ldyBBUEkgd29ya3MsIEkgZGVjaWRlZCB0
byBoZWxwIHlvdSBvdXQgYSBiaXQgYW5kIGRvCj4gPiB0aGUgY29udmVyc2lvbiBteXNlbGYuCj4g
Cj4gSSBhbSBhZnJhaWQgdGhhdCB3aXRoIHRoaXMgYXR0aXR1ZGUgYW5kIGFwcHJvYWNoIHRvd2Fy
ZHMgcG90ZW50aWFsIG5ldwo+IGNvbnRyaWJ1dG9ycywgeW91IHdpbGwgcHV0IG1vcmUgYW5kIG1v
cmUgd29yayBvbiB5b3UgbWFraW5nIGl0IG1vcmUKPiBkaWZmaWN1bHQgdG8gZ2V0IHRoaW5ncyBk
b25lIGF0IGxhcmdlLiAiRWR1Y2F0aW5nIiBuZXcgcGVvcGxlIGlzIGEgd2luCj4gaW4gdGhlIGxv
bmcgcnVuLCBhbmQgdGhpcyBpcyBhIGdlbmVyYWwgcHJhY3RpY2Ugb3V0IHRoZXJlIGJlZm9yZQo+
IGNsYWltaW5nIHRoYXQgSSBhbSBjcmF6eSAoZS5nLiBzb21lb25lIGV2ZW4gdG9sZCBtZSBpbiB0
aGUga2VybmVsCj4gY2lyY2xlcyB0aGF0IGlzICJydWRlIikuCj4gCj4gSSBhbSBzdXJwcmlzZWQg
dGhhdCBtZW50b3JpbmcgbmV3IHBlb3BsZSBpcyBjb25zaWRlcmVkIGJhZCBpZGVhIGluCj4gaGVy
ZS4gU2luY2UgdGhlcmUgaXMgbm8gZ3VhcmFudGVlIHlvdSB3b3VsZCBub3QgZG8gaXQgbmV4dCB0
aW1lIGlmIEkKPiB0cmllZCB0byBwdXQgZWZmb3J0IGludG8gY29udHJpYnV0aW5nLCBJIGZlZWwg
bW9yZSBjb21mb3J0YWJsZSB0byBza2lwCj4gdGhpcyBwcm9qZWN0IGZvciBub3csIGFuZCB3b3Jr
IHdoZXJlIEkga25vdyB0aGlzIGNhbm5vdCBoYXBwZW4uCj4gQ3VycmVudGx5LCBJIGRvIG5vdCBm
ZWVsIGluY2x1c2l2ZSBpbiB0aGlzIHByb2plY3QgZHVlIHRvIHRoaXMKPiBhcHByb2FjaC4KPiAK
PiBJIGRvIG5vdCBldmVuIGNsYWltIHRoYXQgeW91IHdvdWxkIGJlIHRoaXMgdG8gbWUgaW50ZW50
aW9uYWxseSwgYnV0IGl0Cj4gaGFzIGhhcHBlbmVkIGVpdGhlciB3YXkuCgpJIGFncmVlIHRoYXQg
aWYgR3VlbnRlciB3YXMgdG8gZ28gYWhlYWQgYW5kIGNvbnZlcnQgdGhpcyBkcml2ZXIgdG8gYW4K
TUZEIG9uZSwgdGhlbiB0aGF0IHdvdWxkIGJlIHJ1ZGUgY29uZHVjdC4gSG93ZXZlciwgdGhlIHdv
cmsgdW5kZXJ0YWtlbgpieSBoaW0gd2FzICdhZGQtb24nIHdvcmsgd2hpY2ggeW91IGNsZWFybHkg
d2VyZW4ndCBjb21mb3J0YWJsZSB3aXRoLiBJCnRoaW5rIGhlIHdhcyBnZW51aW5lbHkgZG9pbmcg
eW91IGEgZmF2b3VyIHJhdGhlciB0aGFuICdzdGVhbGluZyB5b3VyCnRodW5kZXInLiBIaXMgd29y
ayBoYXMgcGF2ZWQgdGhlIHdheSBmb3IgeW91cnMgYW5kIHNob3VsZCBtYWtlIHlvdXIKd29yayBl
YXNpZXIgaW4gdGhlIGxvbmcgcnVuLgoKTkI6IFJlYmFzaW5nIG9uIHRvcCBvZiBkaWZmZXJlbnQg
SEVBRHMgYW5kIG90aGVyIHBlb3BsZSdzIHdvcmsgaXMganVzdAphIGZhY3Qgb2YgbGlmZSBpbiB0
aGUga2VybmVsLiBJZiB5b3UgZmluZCBpdCBkaWZmaWN1bHQsIHN0aWNrCmFyb3VuZC4gWW91J2xs
IGdldCBiZXR0ZXIgYXQgaXQgd2l0aCBwcmFjdGljZS4KCj4gPiBJIGRpZCBzb21lIGNsZWFudXAg
YmVmb3JlLCBzaW5jZQo+ID4gdGhhdCBtYWRlIHRoZSBhY3R1YWwgZmVhdHVyZSBwYXRjaCBlYXNp
ZXIgZm9yIG1lIHRvIGltcGxlbWVudCwKPiA+IGFuZCBJIGRpZCBzb21lIG1vcmUgY2xlYW51cCBh
ZnRlcndhcmRzIGp1c3QgYmVjYXVzZSBJIGxpa2UKPiA+IGNsZWFuaW5nIHVwIGNvZGUuCj4gCj4g
SU1PLCBpdCBpcyBub3Qgd29ydGggbWFraW5nIHRoZSBnaXQgaGlzdG9yeSBub2lzeSB3aXRoIG5l
ZWRsZXNzCj4gcmUtYXJyYW5naW5nIGxpa2UgZm9yd2FyZCBoZWFkZXJzLCBidXQgdGhhdCBpcyBq
dXN0IG15IG9waW5pb24uIFBsZWFzZQo+IGRvIG5vdCBzYXksIGl0IGlzIHJ1ZGUgYW5kIG5vdCBy
ZXNwZWN0ZnVsLiBJIGRlZmluaXRlbHkgcmVzcGVjdCB5b3VyCj4gb3BpbmlvbiwgSSBqdXN0IGRv
IG5vdCBhZ3JlZSB3aXRoIGl0LCBidXQgdGhhdCBpcyBmaW5lLCB5b3UgYXJlIGEKPiBtYWludGFp
bmVyIHNvIHlvdSBnZXQgdG8gZGVjaWRlLCBJIGd1ZXNzLiBUaGF0IGJlaW5nIHNhaWQsIHBlcmhh
cHMgSQo+IG1pc3NlZCBzb21ldGhpbmcgd2h5IHN1Y2ggcmUtYXJyYW5naW5nIG91dHdlaWdocyB0
aGUgZ2l0IGhpc3RvcnkKPiBkaXNhZHZhbnRhZ2UuCgo9Oi1ECgpXZSBoYXZlIG5lYXJseSBoYWxm
IGEgbWlsbGlvbiBjb21taXRzIGluIHRoZSBNYWlubGluZSBrZXJuZWwgc2luY2UKdjIuNi4xMi4g
SSBkb24ndCB0aGluayBHaXQgaGlzdG9yeSBpcyBldmVuIGEgY29uc2lkZXJhdGlvbiBoZXJlLiBB
bHNvLAp3ZSByZWFsbHkgZG9uJ3QgbGlrZSBmb3J3YXJkIGRlY2xhcmF0aW9ucyBpbiB0aGUga2Vy
bmVsLiBHdWVudGVyJ3MKY2xlYW4tdXBzIHdoZXJlIGdvb2Qgb25lcy4gV2UgY2FuJ3Qgc3RvcCBj
bGVhbmluZyB1cCBhbmQgZGV2ZWxvcGluZyBpbgp0aGUgZmVhciB0aGF0IHNvbWVvbmUgZWxzZSBp
cyB3b3JraW5nIG9uIHRoYXQgc2FtZSBkcml2ZXIuIFdlIGhhdmUKbG90cyBvZiBwZW9wbGUgbWFr
aW5nIGNoYW5nZXMgdG8gdGhlIHNhbWUgZmlsZXMgYWxsIG92ZXIgdGhlIGtlcm5lbC4KR2V0IGdv
b2QgYXQgYGdpdCByZWJhc2VgaW5nIGFuZCB0aGVyZSBpcyBubyBpc3N1ZSwgaXQncyBqdXN0IGFu
b3RoZXIKZGF5IGluIHRoZSBsaWZlIG9mIGEgS2VybmVsIERldmVsb3Blci4KCklmIHlvdSdkIHNw
ZW5kIG1vcmUgdGltZSByZWJhc2luZyBhbmQgY29kaW5nIGFzIHlvdSBkbyB3cml0aW5nIGVtYWls
cywKeW91J2QgaGF2ZSBoYWQgdGhpcyBkb25lIGJ5IG5vdy4gOikKCi0tIApMZWUgSm9uZXMKTGlu
YXJvIFNUTWljcm9lbGVjdHJvbmljcyBMYW5kaW5nIFRlYW0gTGVhZApMaW5hcm8ub3JnIOKUgiBP
cGVuIHNvdXJjZSBzb2Z0d2FyZSBmb3IgQVJNIFNvQ3MKRm9sbG93IExpbmFybzogRmFjZWJvb2sg
fCBUd2l0dGVyIHwgQmxvZwoKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f
X19fX19fX18KbG0tc2Vuc29ycyBtYWlsaW5nIGxpc3QKbG0tc2Vuc29yc0BsbS1zZW5zb3JzLm9y
ZwpodHRwOi8vbGlzdHMubG0tc2Vuc29ycy5vcmcvbWFpbG1hbi9saXN0aW5mby9sbS1zZW5zb3Jz

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Laszlo Papp]

On Fri, Feb 14, 2014 at 9:02 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> >> http://comments.gmane.org/gmane.linux.kernel/1645251
>> >>
>> >> Step 2 did not happen. I did not get any review for my change. I
>> >> literally submitted that within a couple of hours after the request.
>> >>
>> >> Could you please tell me what was wrong with that change, and why I
>> >> did not get any respect not to "xargs rm -rf" my work in that area? I
>> >> believe I was ignored instead of improving the change, and someone
>> >> else tried to address the same thing. There was no argument in that
>> >> thread. It was a technical change. I personally do not feel happy
>> >> about it.
>> >
>> > Let's start again.
>> >
>> > Rebase your work on top of the HWMON tree on kernel.org and resubmit
>> > the entire set. If rebasing takes you more than 20 mins, you're
>> > probably doing it wrong.
>>
>> I tried, but I could not manage it within 20 minutes, so I guess I am
>> doing something wrong. Can you please provide some pointers how not to
>> do it wrong? Perhaps, I am not aware of some tricks.
>
> One question, are you still working on this stuff or not? I'm confused
> by the disparity in your messages. I'm going to guess that you're in
> for now.
>
> Do:
>   `git rebase -i <base> --onto <newbase>`
> Where:
>   <base> is the SHA1 of the first patch below your changes in `git log`
>   <new_base> is Guenter's staging tree on kernel.org
>
> Ensure you're rebasing all of your patches (and patches that aren't
> yours) when your $EDITOR pops up. If they are wrong, delete all the
> lines in the file and the rebase will be aborted. If they're correct
> save and close your $EDITOR.
>
> You'll receive conflicts. You can see the state of the conflicts using
> `git status` Open the file, find the conflict markers and make a choice
> from the HEAD section or the section from your patch. Sometimes
> you'll need to manually merge the two, if there are changes from both
> refs that you want to keep. Once you're happy `git commit -a` and `git
> rebase --continue`. Each conflict should not take you long, but if it
> does, keep at it, as it's good practice. After a time of doing it,
> you'll be able to fix merge conflicts in no time at all.

Right, that is what I have been following myself for a couple of
years. Why it took me more time because I had to go through his
changes and to understand all in details to make reasonably good
decisions what to keep and what to drop at the conflicts.

Thank you for your answer.

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Laszlo Papp]

On Fri, Feb 14, 2014 at 9:02 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> >> http://comments.gmane.org/gmane.linux.kernel/1645251
>> >>
>> >> Step 2 did not happen. I did not get any review for my change. I
>> >> literally submitted that within a couple of hours after the request.
>> >>
>> >> Could you please tell me what was wrong with that change, and why I
>> >> did not get any respect not to "xargs rm -rf" my work in that area? I
>> >> believe I was ignored instead of improving the change, and someone
>> >> else tried to address the same thing. There was no argument in that
>> >> thread. It was a technical change. I personally do not feel happy
>> >> about it.
>> >
>> > Let's start again.
>> >
>> > Rebase your work on top of the HWMON tree on kernel.org and resubmit
>> > the entire set. If rebasing takes you more than 20 mins, you're
>> > probably doing it wrong.
>>
>> I tried, but I could not manage it within 20 minutes, so I guess I am
>> doing something wrong. Can you please provide some pointers how not to
>> do it wrong? Perhaps, I am not aware of some tricks.
>
> One question, are you still working on this stuff or not? I'm confused
> by the disparity in your messages. I'm going to guess that you're in
> for now.
>
> Do:
>   `git rebase -i <base> --onto <newbase>`
> Where:
>   <base> is the SHA1 of the first patch below your changes in `git log`
>   <new_base> is Guenter's staging tree on kernel.org
>
> Ensure you're rebasing all of your patches (and patches that aren't
> yours) when your $EDITOR pops up. If they are wrong, delete all the
> lines in the file and the rebase will be aborted. If they're correct
> save and close your $EDITOR.
>
> You'll receive conflicts. You can see the state of the conflicts using
> `git status` Open the file, find the conflict markers and make a choice
> from the HEAD section or the section from your patch. Sometimes
> you'll need to manually merge the two, if there are changes from both
> refs that you want to keep. Once you're happy `git commit -a` and `git
> rebase --continue`. Each conflict should not take you long, but if it
> does, keep at it, as it's good practice. After a time of doing it,
> you'll be able to fix merge conflicts in no time at all.

Right, that is what I have been following myself for a couple of
years. Why it took me more time because I had to go through his
changes and to understand all in details to make reasonably good
decisions what to keep and what to drop at the conflicts.

Thank you for your answer.

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

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Lee Jones]

On Fri, 14 Feb 2014, Laszlo Papp wrote:

> On Fri, Feb 14, 2014 at 9:02 AM, Lee Jones <lee.jones@linaro.org> wrote:
> >> >> http://comments.gmane.org/gmane.linux.kernel/1645251
> >> >>
> >> >> Step 2 did not happen. I did not get any review for my change. I
> >> >> literally submitted that within a couple of hours after the request.
> >> >>
> >> >> Could you please tell me what was wrong with that change, and why I
> >> >> did not get any respect not to "xargs rm -rf" my work in that area? I
> >> >> believe I was ignored instead of improving the change, and someone
> >> >> else tried to address the same thing. There was no argument in that
> >> >> thread. It was a technical change. I personally do not feel happy
> >> >> about it.
> >> >
> >> > Let's start again.
> >> >
> >> > Rebase your work on top of the HWMON tree on kernel.org and resubmit
> >> > the entire set. If rebasing takes you more than 20 mins, you're
> >> > probably doing it wrong.
> >>
> >> I tried, but I could not manage it within 20 minutes, so I guess I am
> >> doing something wrong. Can you please provide some pointers how not to
> >> do it wrong? Perhaps, I am not aware of some tricks.
> >
> > One question, are you still working on this stuff or not? I'm confused
> > by the disparity in your messages. I'm going to guess that you're in
> > for now.
> >
> > Do:
> >   `git rebase -i <base> --onto <newbase>`
> > Where:
> >   <base> is the SHA1 of the first patch below your changes in `git log`
> >   <new_base> is Guenter's staging tree on kernel.org
> >
> > Ensure you're rebasing all of your patches (and patches that aren't
> > yours) when your $EDITOR pops up. If they are wrong, delete all the
> > lines in the file and the rebase will be aborted. If they're correct
> > save and close your $EDITOR.
> >
> > You'll receive conflicts. You can see the state of the conflicts using
> > `git status` Open the file, find the conflict markers and make a choice
> > from the HEAD section or the section from your patch. Sometimes
> > you'll need to manually merge the two, if there are changes from both
> > refs that you want to keep. Once you're happy `git commit -a` and `git
> > rebase --continue`. Each conflict should not take you long, but if it
> > does, keep at it, as it's good practice. After a time of doing it,
> > you'll be able to fix merge conflicts in no time at all.
> 
> Right, that is what I have been following myself for a couple of
> years.

> Why it took me more time because I had to go through his
> changes and to understand all in details to make reasonably good
> decisions what to keep and what to drop at the conflicts.

Correct, that's what will improve with time.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [lm-sensors] [RFC PATCH] hwmon: (max6650) Convert to be a platform driver

[Lee Jones]

T24gRnJpLCAxNCBGZWIgMjAxNCwgTGFzemxvIFBhcHAgd3JvdGU6Cgo+IE9uIEZyaSwgRmViIDE0
LCAyMDE0IGF0IDk6MDIgQU0sIExlZSBKb25lcyA8bGVlLmpvbmVzQGxpbmFyby5vcmc+IHdyb3Rl
Ogo+ID4+ID4+IGh0dHA6Ly9jb21tZW50cy5nbWFuZS5vcmcvZ21hbmUubGludXgua2VybmVsLzE2
NDUyNTEKPiA+PiA+Pgo+ID4+ID4+IFN0ZXAgMiBkaWQgbm90IGhhcHBlbi4gSSBkaWQgbm90IGdl
dCBhbnkgcmV2aWV3IGZvciBteSBjaGFuZ2UuIEkKPiA+PiA+PiBsaXRlcmFsbHkgc3VibWl0dGVk
IHRoYXQgd2l0aGluIGEgY291cGxlIG9mIGhvdXJzIGFmdGVyIHRoZSByZXF1ZXN0Lgo+ID4+ID4+
Cj4gPj4gPj4gQ291bGQgeW91IHBsZWFzZSB0ZWxsIG1lIHdoYXQgd2FzIHdyb25nIHdpdGggdGhh
dCBjaGFuZ2UsIGFuZCB3aHkgSQo+ID4+ID4+IGRpZCBub3QgZ2V0IGFueSByZXNwZWN0IG5vdCB0
byAieGFyZ3Mgcm0gLXJmIiBteSB3b3JrIGluIHRoYXQgYXJlYT8gSQo+ID4+ID4+IGJlbGlldmUg
SSB3YXMgaWdub3JlZCBpbnN0ZWFkIG9mIGltcHJvdmluZyB0aGUgY2hhbmdlLCBhbmQgc29tZW9u
ZQo+ID4+ID4+IGVsc2UgdHJpZWQgdG8gYWRkcmVzcyB0aGUgc2FtZSB0aGluZy4gVGhlcmUgd2Fz
IG5vIGFyZ3VtZW50IGluIHRoYXQKPiA+PiA+PiB0aHJlYWQuIEl0IHdhcyBhIHRlY2huaWNhbCBj
aGFuZ2UuIEkgcGVyc29uYWxseSBkbyBub3QgZmVlbCBoYXBweQo+ID4+ID4+IGFib3V0IGl0Lgo+
ID4+ID4KPiA+PiA+IExldCdzIHN0YXJ0IGFnYWluLgo+ID4+ID4KPiA+PiA+IFJlYmFzZSB5b3Vy
IHdvcmsgb24gdG9wIG9mIHRoZSBIV01PTiB0cmVlIG9uIGtlcm5lbC5vcmcgYW5kIHJlc3VibWl0
Cj4gPj4gPiB0aGUgZW50aXJlIHNldC4gSWYgcmViYXNpbmcgdGFrZXMgeW91IG1vcmUgdGhhbiAy
MCBtaW5zLCB5b3UncmUKPiA+PiA+IHByb2JhYmx5IGRvaW5nIGl0IHdyb25nLgo+ID4+Cj4gPj4g
SSB0cmllZCwgYnV0IEkgY291bGQgbm90IG1hbmFnZSBpdCB3aXRoaW4gMjAgbWludXRlcywgc28g
SSBndWVzcyBJIGFtCj4gPj4gZG9pbmcgc29tZXRoaW5nIHdyb25nLiBDYW4geW91IHBsZWFzZSBw
cm92aWRlIHNvbWUgcG9pbnRlcnMgaG93IG5vdCB0bwo+ID4+IGRvIGl0IHdyb25nPyBQZXJoYXBz
LCBJIGFtIG5vdCBhd2FyZSBvZiBzb21lIHRyaWNrcy4KPiA+Cj4gPiBPbmUgcXVlc3Rpb24sIGFy
ZSB5b3Ugc3RpbGwgd29ya2luZyBvbiB0aGlzIHN0dWZmIG9yIG5vdD8gSSdtIGNvbmZ1c2VkCj4g
PiBieSB0aGUgZGlzcGFyaXR5IGluIHlvdXIgbWVzc2FnZXMuIEknbSBnb2luZyB0byBndWVzcyB0
aGF0IHlvdSdyZSBpbgo+ID4gZm9yIG5vdy4KPiA+Cj4gPiBEbzoKPiA+ICAgYGdpdCByZWJhc2Ug
LWkgPGJhc2U+IC0tb250byA8bmV3YmFzZT5gCj4gPiBXaGVyZToKPiA+ICAgPGJhc2U+IGlzIHRo
ZSBTSEExIG9mIHRoZSBmaXJzdCBwYXRjaCBiZWxvdyB5b3VyIGNoYW5nZXMgaW4gYGdpdCBsb2dg
Cj4gPiAgIDxuZXdfYmFzZT4gaXMgR3VlbnRlcidzIHN0YWdpbmcgdHJlZSBvbiBrZXJuZWwub3Jn
Cj4gPgo+ID4gRW5zdXJlIHlvdSdyZSByZWJhc2luZyBhbGwgb2YgeW91ciBwYXRjaGVzIChhbmQg
cGF0Y2hlcyB0aGF0IGFyZW4ndAo+ID4geW91cnMpIHdoZW4geW91ciAkRURJVE9SIHBvcHMgdXAu
IElmIHRoZXkgYXJlIHdyb25nLCBkZWxldGUgYWxsIHRoZQo+ID4gbGluZXMgaW4gdGhlIGZpbGUg
YW5kIHRoZSByZWJhc2Ugd2lsbCBiZSBhYm9ydGVkLiBJZiB0aGV5J3JlIGNvcnJlY3QKPiA+IHNh
dmUgYW5kIGNsb3NlIHlvdXIgJEVESVRPUi4KPiA+Cj4gPiBZb3UnbGwgcmVjZWl2ZSBjb25mbGlj
dHMuIFlvdSBjYW4gc2VlIHRoZSBzdGF0ZSBvZiB0aGUgY29uZmxpY3RzIHVzaW5nCj4gPiBgZ2l0
IHN0YXR1c2AgT3BlbiB0aGUgZmlsZSwgZmluZCB0aGUgY29uZmxpY3QgbWFya2VycyBhbmQgbWFr
ZSBhIGNob2ljZQo+ID4gZnJvbSB0aGUgSEVBRCBzZWN0aW9uIG9yIHRoZSBzZWN0aW9uIGZyb20g
eW91ciBwYXRjaC4gU29tZXRpbWVzCj4gPiB5b3UnbGwgbmVlZCB0byBtYW51YWxseSBtZXJnZSB0
aGUgdHdvLCBpZiB0aGVyZSBhcmUgY2hhbmdlcyBmcm9tIGJvdGgKPiA+IHJlZnMgdGhhdCB5b3Ug
d2FudCB0byBrZWVwLiBPbmNlIHlvdSdyZSBoYXBweSBgZ2l0IGNvbW1pdCAtYWAgYW5kIGBnaXQK
PiA+IHJlYmFzZSAtLWNvbnRpbnVlYC4gRWFjaCBjb25mbGljdCBzaG91bGQgbm90IHRha2UgeW91
IGxvbmcsIGJ1dCBpZiBpdAo+ID4gZG9lcywga2VlcCBhdCBpdCwgYXMgaXQncyBnb29kIHByYWN0
aWNlLiBBZnRlciBhIHRpbWUgb2YgZG9pbmcgaXQsCj4gPiB5b3UnbGwgYmUgYWJsZSB0byBmaXgg
bWVyZ2UgY29uZmxpY3RzIGluIG5vIHRpbWUgYXQgYWxsLgo+IAo+IFJpZ2h0LCB0aGF0IGlzIHdo
YXQgSSBoYXZlIGJlZW4gZm9sbG93aW5nIG15c2VsZiBmb3IgYSBjb3VwbGUgb2YKPiB5ZWFycy4K
Cj4gV2h5IGl0IHRvb2sgbWUgbW9yZSB0aW1lIGJlY2F1c2UgSSBoYWQgdG8gZ28gdGhyb3VnaCBo
aXMKPiBjaGFuZ2VzIGFuZCB0byB1bmRlcnN0YW5kIGFsbCBpbiBkZXRhaWxzIHRvIG1ha2UgcmVh
c29uYWJseSBnb29kCj4gZGVjaXNpb25zIHdoYXQgdG8ga2VlcCBhbmQgd2hhdCB0byBkcm9wIGF0
IHRoZSBjb25mbGljdHMuCgpDb3JyZWN0LCB0aGF0J3Mgd2hhdCB3aWxsIGltcHJvdmUgd2l0aCB0
aW1lLgoKLS0gCkxlZSBKb25lcwpMaW5hcm8gU1RNaWNyb2VsZWN0cm9uaWNzIExhbmRpbmcgVGVh
bSBMZWFkCkxpbmFyby5vcmcg4pSCIE9wZW4gc291cmNlIHNvZnR3YXJlIGZvciBBUk0gU29DcwpG
b2xsb3cgTGluYXJvOiBGYWNlYm9vayB8IFR3aXR0ZXIgfCBCbG9nCgpfX19fX19fX19fX19fX19f
X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpsbS1zZW5zb3JzIG1haWxpbmcgbGlzdAps
bS1zZW5zb3JzQGxtLXNlbnNvcnMub3JnCmh0dHA6Ly9saXN0cy5sbS1zZW5zb3JzLm9yZy9tYWls
bWFuL2xpc3RpbmZvL2xtLXNlbnNvcnM