[PATCH 0/3] Add GPIO support for the MAX6650/6651 ICs

[PATCH 0/3] Add GPIO support for the MAX6650/6651 ICs

[Laszlo Papp]

Important text here later. It is now just an attempt to get some feedback about
the design of the new features. This set has only been compilation tested, and
has many errors with checkpatch.pl, et al. Really, the point is only to get
feedback about the direction, as there has been a lot of paid work put into
this to work better with upstream after the previously rejected change simple
hwmon change.

Laszlo Papp (3):
  mfd: MAX6650/6651 support
  hwmon: (max6650) Convert to be a platform driver
  gpio: MAX6650/6651 support

 drivers/gpio/Kconfig                |  14 ++
 drivers/gpio/Makefile               |   2 +
 drivers/gpio/gpio-max6651.c         |  72 +++++++++
 drivers/gpio/gpio-max665x.c         | 301 ++++++++++++++++++++++++++++++++++++
 drivers/hwmon/Kconfig               |   2 +-
 drivers/hwmon/max6650.c             | 150 +++++++++---------
 drivers/mfd/Kconfig                 |  11 ++
 drivers/mfd/Makefile                |   1 +
 drivers/mfd/max6651.c               | 132 ++++++++++++++++
 include/linux/mfd/max6651-private.h |  63 ++++++++
 10 files changed, 675 insertions(+), 73 deletions(-)
 create mode 100644 drivers/gpio/gpio-max6651.c
 create mode 100644 drivers/gpio/gpio-max665x.c
 create mode 100644 drivers/mfd/max6651.c
 create mode 100644 include/linux/mfd/max6651-private.h

-- 
1.8.5.1

[PATCH 1/3] mfd: MAX6650/6651 support

[Laszlo Papp]

MAX6650/MAX6651 chip is a multi-function device with I2C busses. The
chip includes fan-speed regulators and monitors, GPIO, and alarm.

This patch is an initial release of a MAX6650/6651 MFD driver that
supports to enable the chip with its primary I2C bus that will connect
the hwmon, and then the gpio devices for now.

Signed-off-by: Laszlo Papp <lpapp@kde.org>
---
 drivers/mfd/Kconfig                 |  11 +++
 drivers/mfd/Makefile                |   1 +
 drivers/mfd/max6651.c               | 132 ++++++++++++++++++++++++++++++++++++
 include/linux/mfd/max6651-private.h |  53 +++++++++++++++
 4 files changed, 197 insertions(+)
 create mode 100644 drivers/mfd/max6651.c
 create mode 100644 include/linux/mfd/max6651-private.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index dd67158..706c4e5 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -321,6 +321,17 @@ config MFD_88PM860X
 	  select individual components like voltage regulators, RTC and
 	  battery-charger under the corresponding menus.
 
+config MFD_MAX6651
+	bool "Maxim Semiconductor MAX6651 Support"
+	depends on I2C=y
+	select MFD_CORE
+	select IRQ_DOMAIN
+	help
+	  Say yes here to support for Maxim Semiconductor MAX6651. This is
+	  a fan speed regulator and monitor IC. This driver provies common support
+	  for accessing the device, additional drivers must be enabled in order to
+	  use the functionality of the device.
+
 config MFD_MAX77686
 	bool "Maxim Semiconductor MAX77686 PMIC Support"
 	depends on I2C=y
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 8a28dc9..254a8a7 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -110,6 +110,7 @@ obj-$(CONFIG_MFD_DA9055)	+= da9055.o
 da9063-objs			:= da9063-core.o da9063-irq.o da9063-i2c.o
 obj-$(CONFIG_MFD_DA9063)	+= da9063.o
 
+obj-$(CONFIG_MFD_MAX6651)	+= max6651.o
 obj-$(CONFIG_MFD_MAX77686)	+= max77686.o max77686-irq.o
 obj-$(CONFIG_MFD_MAX77693)	+= max77693.o max77693-irq.o
 obj-$(CONFIG_MFD_MAX8907)	+= max8907.o
diff --git a/drivers/mfd/max6651.c b/drivers/mfd/max6651.c
new file mode 100644
index 0000000..c6b1716
--- /dev/null
+++ b/drivers/mfd/max6651.c
@@ -0,0 +1,132 @@
+/*
+ * Device access for MAX6651
+ *
+ * Copyright(c) 2013 Polatis Ltd.
+ *
+ * Author: Laszlo Papp <laszlo.papp@polatis.com>
+ *
+ *  This program is free software; you can redistribute  it and/or modify it
+ *  under  the terms of  the GNU General  Public License as published by the
+ *  Free Software Foundation;  either version 2 of the  License, or (at your
+ *  option) any later version.
+ */
+
+#include <linux/device.h>
+#include <linux/delay.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/core.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/i2c.h>
+
+#include <linux/mfd/max6651-private.h>
+
+static struct mfd_cell max6651_devs[] = {
+    { .name = "max6651-gpio", },
+    { .name = "max6650", },
+};
+
+int max6651_read_reg(struct i2c_client *i2c, u8 reg, u8 *dest)
+{
+    struct max6651_dev *max6651 = i2c_get_clientdata(i2c);
+    int ret;
+
+    mutex_lock(&max6651->iolock);
+    ret = i2c_smbus_read_byte_data(i2c, reg);
+    mutex_unlock(&max6651->iolock);
+    if (ret < 0)
+        return ret;
+
+    ret &= 0xff;
+    *dest = ret;
+    return 0;
+}
+EXPORT_SYMBOL_GPL(max6651_read_reg);
+
+int max6651_write_reg(struct i2c_client *i2c, u8 reg, u8 value)
+{
+    struct max6651_dev *max6651 = i2c_get_clientdata(i2c);
+    int ret;
+
+    mutex_lock(&max6651->iolock);
+    ret = i2c_smbus_write_byte_data(i2c, reg, value);
+    mutex_unlock(&max6651->iolock);
+    return ret;
+}
+EXPORT_SYMBOL_GPL(max6651_write_reg);
+
+static int max6651_i2c_probe(struct i2c_client *i2c,
+			    const struct i2c_device_id *id)
+{
+	struct max6651_dev *max6651;
+	int ret = 0;
+
+	max6651 = kzalloc(sizeof(struct max6651_dev), GFP_KERNEL);
+	if (max6651 == NULL)
+		return -ENOMEM;
+
+	i2c_set_clientdata(i2c, max6651);
+	max6651->dev = &i2c->dev;
+
+	mutex_init(&max6651->iolock);
+
+	ret = mfd_add_devices(max6651->dev, -1, max6651_devs,
+			ARRAY_SIZE(max6651_devs),
+			NULL, 0, NULL);
+
+	if (ret < 0) {
+        dev_err(max6651->dev, "cannot add mfd cells\n");
+		goto err_mfd;
+    }
+
+	return ret;
+
+err_mfd:
+	mfd_remove_devices(max6651->dev);
+	kfree(max6651);
+	return ret;
+}
+
+static int max6651_i2c_remove(struct i2c_client *i2c)
+{
+    struct max6651_dev *max6651 = i2c_get_clientdata(i2c);
+
+    mfd_remove_devices(max6651->dev);
+
+    return 0;
+}
+
+static const struct i2c_device_id max6651_i2c_id[] = {
+    { "max6650", TYPE_MAX6650 },
+    { "max6651", TYPE_MAX6651 },
+    { }
+};
+MODULE_DEVICE_TABLE(i2c, max6651_i2c_id);
+
+static struct i2c_driver max6651_i2c_driver = {
+    .driver = {
+           .name = "max6651",
+           .owner = THIS_MODULE,
+    },
+    .probe = max6651_i2c_probe,
+    .remove = max6651_i2c_remove,
+    .id_table = max6651_i2c_id,
+};
+
+static int __init max6651_i2c_init(void)
+{
+    return i2c_add_driver(&max6651_i2c_driver);
+}
+/* init early so consumer devices can complete system boot */
+subsys_initcall(max6651_i2c_init);
+
+static void __exit max6651_i2c_exit(void)
+{
+    i2c_del_driver(&max6651_i2c_driver);
+}
+module_exit(max6651_i2c_exit);
+
+MODULE_AUTHOR("Laszlo Papp <laszlo.papp@polatis.com>");
+MODULE_DESCRIPTION("MAX6651 MFD");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/max6651-private.h b/include/linux/mfd/max6651-private.h
new file mode 100644
index 0000000..ae90261
--- /dev/null
+++ b/include/linux/mfd/max6651-private.h
@@ -0,0 +1,53 @@
+/*
+ * max6650.h - Driver for the Maxim 6650/6651
+ *
+ *  Copyright (C) 2013 Laszlo Papp <laszlo.papp@polatis.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ *
+ * This driver is based on max8998.h
+ *
+ * MAX8997 has PMIC, MUIC, HAPTIC, RTC, FLASH, and Fuel Gauge devices.
+ * Except Fuel Gauge, every device shares the same I2C bus and included in
+ * this mfd driver. Although the fuel gauge is included in the chip, it is
+ * excluded from the driver because a) it has a different I2C bus from
+ * others and b) it can be enabled simply by using MAX17042 driver.
+ */
+
+#ifndef __LINUX_MFD_MAX6651_PRIVATE_H
+#define __LINUX_MFD_MAX6651_PRIVATE_H
+
+#include <linux/i2c.h>
+#include <linux/export.h>
+#include <linux/irqdomain.h>
+
+struct max6651_dev {
+    struct device *dev;
+    struct mutex iolock;
+
+    struct i2c_client *i2c;
+
+	int type;
+};
+
+enum max6651_types {
+	TYPE_MAX6650,
+	TYPE_MAX6651,
+};
+
+extern int max6651_read_reg(struct i2c_client *i2c, u8 reg, u8 *dest);
+extern int max6651_write_reg(struct i2c_client *i2c, u8 reg, u8 value);
+
+#endif
-- 
1.8.5.1

[PATCH 2/3] 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>
---
 drivers/hwmon/Kconfig   |   2 +-
 drivers/hwmon/max6650.c | 150 +++++++++++++++++++++++++-----------------------
 drivers/mfd/max6651.c   |   2 +-
 3 files changed, 80 insertions(+), 74 deletions(-)

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 52d548f..ee4fb07 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -925,7 +925,7 @@ config SENSORS_MAX6642
 
 config SENSORS_MAX6650
 	tristate "Maxim MAX6650 sensor chip"
-	depends on I2C
+	depends on MFD_MAX6651
 	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..ec7dac2 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/max6651-private.h>
 
 /*
  * Insmod parameters
@@ -105,24 +108,24 @@ 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);
 
-static struct i2c_driver max6650_driver = {
+MODULE_DEVICE_TABLE(platform, max6650_id);
+
+static struct platform_driver max6650_driver = {
 	.driver = {
 		.name	= "max6650",
 	},
@@ -136,7 +139,9 @@ static struct i2c_driver max6650_driver = {
  */
 
 struct max6650_data {
+	struct max6651 *max6651;
 	struct device *hwmon_dev;
+	struct max6651_dev *iodev;
 	struct mutex update_lock;
 	int nr_fans;
 	char valid; /* zero until following fields are valid */
@@ -235,8 +240,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 +268,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);
+	max6651_write_reg(data->iodev->i2c, MAX6650_REG_SPEED, data->speed);
 
 	mutex_unlock(&data->update_lock);
 
@@ -304,8 +308,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 +325,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);
+	max6651_write_reg(data->iodev->i2c, MAX6650_REG_DAC, data->dac);
 
 	mutex_unlock(&data->update_lock);
 
@@ -350,8 +353,8 @@ 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 platform_device *pdev = to_platform_device(dev);
+	struct max6650_data *data = platform_get_drvdata(pdev);
 	int max6650_modes[3] = {0, 3, 2};
 	unsigned long mode;
 	int err;
@@ -365,11 +368,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);
+	max6651_read_reg(data->iodev->i2c, 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);
+	max6651_write_reg(data->iodev->i2c, MAX6650_REG_CONFIG, data->config);
 
 	mutex_unlock(&data->update_lock);
 
@@ -400,8 +403,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 +430,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);
+	max6651_write_reg(data->iodev->i2c, MAX6650_REG_COUNT, data->count);
 	mutex_unlock(&data->update_lock);
 
 	return count;
@@ -446,15 +448,14 @@ static ssize_t get_alarm(struct device *dev, struct device_attribute *devattr,
 {
 	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);
 	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);
+		max6651_read_reg(data->iodev->i2c, MAX6650_REG_ALARM,
+				 &data->alarm);
 		mutex_unlock(&data->update_lock);
 	}
 
@@ -484,10 +485,13 @@ 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);
+	u8 alarm_en;
+
 	struct device_attribute *devattr;
 
+	max6651_read_reg(data->iodev->i2c, MAX6650_REG_ALARM_EN, &alarm_en);
+
 	/*
 	 * Hide the alarms that have not been enabled by the firmware
 	 */
@@ -539,74 +543,72 @@ 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 max6650_data *data;
 	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);
+	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_inpdevent(struct platform_device *pdev)
 {
-	struct max6650_data *data = i2c_get_clientdata(client);
-	int config;
+	struct max6650_data *data = dev_get_drvdata(pdev->dev.parent);
+	u8 config;
 	int err = -EIO;
 
-	config = i2c_smbus_read_byte_data(client, MAX6650_REG_CONFIG);
+	err = max6651_read_reg(data->iodev->i2c, MAX6650_REG_CONFIG, &config);
 
-	if (config < 0) {
-		dev_err(&client->dev, "Error reading config, aborting.\n");
+	if (err < 0) {
+		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,24 @@ 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 (max6651_write_reg(data->iodev->i2c, 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 (max6651_write_reg(data->iodev->i2c,
+		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);
+	max6651_read_reg(data->iodev->i2c, MAX6650_REG_COUNT, &data->count);
 
 	return 0;
 }
@@ -694,31 +698,32 @@ 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 platform_device *pdev = to_platform_device(dev);
+	struct max6650_data *data = platform_get_drvdata(pdev);
+	u8 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);
+		max6651_read_reg(data->iodev->i2c, MAX6650_REG_SPEED,
+				 &data->speed);
+		max6651_read_reg(data->iodev->i2c, 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]);
+			max6651_read_reg(data->iodev->i2c, 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);
+		max6651_read_reg(data->iodev->i2c, MAX6650_REG_COUNT,
+				 &data->count);
+		max6651_read_reg(data->iodev->i2c, 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);
+		max6651_read_reg(data->iodev->i2c, MAX6650_REG_ALARM, &alarm);
+		data->alarm |= alarm;
 
 		data->last_updated = jiffies;
 		data->valid = 1;
@@ -729,8 +734,9 @@ 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");
 MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:max6650-hwmon");
diff --git a/drivers/mfd/max6651.c b/drivers/mfd/max6651.c
index c6b1716..d423b7a 100644
--- a/drivers/mfd/max6651.c
+++ b/drivers/mfd/max6651.c
@@ -24,7 +24,7 @@
 
 static struct mfd_cell max6651_devs[] = {
     { .name = "max6651-gpio", },
-    { .name = "max6650", },
+    { .name = "max6650-hwmon", },
 };
 
 int max6651_read_reg(struct i2c_client *i2c, u8 reg, u8 *dest)
-- 
1.8.5.1

[PATCH 3/3] gpio: MAX6650/6651 support

[Laszlo Papp]

These ICs already have hwmon driver support, but they also have some gpio
functionality which this addition tries to address. Later on, there would be an
MFD driver added as well.

Signed-off-by: Laszlo Papp <lpapp@kde.org>
---
 drivers/gpio/Kconfig                |  14 ++
 drivers/gpio/Makefile               |   2 +
 drivers/gpio/gpio-max6651.c         |  72 +++++++++
 drivers/gpio/gpio-max665x.c         | 301 ++++++++++++++++++++++++++++++++++++
 include/linux/mfd/max6651-private.h |  10 ++
 5 files changed, 399 insertions(+)
 create mode 100644 drivers/gpio/gpio-max6651.c
 create mode 100644 drivers/gpio/gpio-max665x.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 0f04444..50fea6b 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -103,6 +103,9 @@ config GPIO_DA9055
 
 	  If driver is built as a module it will be called gpio-da9055.
 
+config GPIO_MAX665X
+	tristate
+
 config GPIO_MAX730X
 	tristate
 
@@ -381,6 +384,17 @@ config GPIO_ARIZONA
 	help
 	  Support for GPIOs on Wolfson Arizona class devices.
 
+config GPIO_MAX6651
+	tristate "Maxim MAX6651 GPIO pins"
+	depends on MFD_MAX6651
+	select GPIO_MAX665X
+	help
+	  Say yes here to support the GPIO functionality for the MAX6651 Fan Speed
+	  Regulators and Monitors with SMBus/I2C Compatible Interface IC. It has
+	  five GPIO pins that can be set to various functionality, including the
+	  regular input and output operations. It has an internal pull-up resistor
+	  though within the IC.
+
 config GPIO_MAX7300
 	tristate "Maxim MAX7300 GPIO expander"
 	depends on I2C
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 7971e36..a4d34d6 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -37,6 +37,8 @@ obj-$(CONFIG_ARCH_KS8695)	+= gpio-ks8695.o
 obj-$(CONFIG_GPIO_INTEL_MID)	+= gpio-intel-mid.o
 obj-$(CONFIG_ARCH_LPC32XX)	+= gpio-lpc32xx.o
 obj-$(CONFIG_GPIO_LYNXPOINT)	+= gpio-lynxpoint.o
+obj-$(CONFIG_GPIO_MAX665X)	+= gpio-max665x.o
+obj-$(CONFIG_GPIO_MAX6651)	+= gpio-max6651.o
 obj-$(CONFIG_GPIO_MAX730X)	+= gpio-max730x.o
 obj-$(CONFIG_GPIO_MAX7300)	+= gpio-max7300.o
 obj-$(CONFIG_GPIO_MAX7301)	+= gpio-max7301.o
diff --git a/drivers/gpio/gpio-max6651.c b/drivers/gpio/gpio-max6651.c
new file mode 100644
index 0000000..e32c530
--- /dev/null
+++ b/drivers/gpio/gpio-max6651.c
@@ -0,0 +1,72 @@
+/*
+ * Copyright (C) 2013 Laszlo Papp <laszlo.papp@polatis.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Check max665x.c for further details.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+
+#include <linux/mfd/max6651-private.h>
+
+#define PIN_NUMBER 5
+
+static int max6651_probe(struct platform_device *pdev)
+{
+    struct max665x_gpio *gpio;
+    struct da9055_pdata *pdata;
+	int ret;
+
+	gpio = devm_kzalloc(&pdev->dev, sizeof(struct max665x_gpio), GFP_KERNEL);
+	if (!gpio)
+		return -ENOMEM;
+
+    gpio->gp.ngpio = PIN_NUMBER;
+
+	ret = __max665x_probe(gpio);
+	return ret;
+}
+
+static int max6651_remove(struct platform_device *pdev)
+{
+	return __max665x_remove(&pdev->dev);
+}
+
+static const struct i2c_device_id max6651_id[] = {
+	{ "max6651", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(platform, max6651_id);
+
+static struct platform_driver max6651_driver = {
+	.driver = {
+		.name = "gpio-max6651",
+		.owner = THIS_MODULE,
+	},
+	.probe = max6651_probe,
+	.remove = max6651_remove,
+	.id_table = max6651_id,
+};
+
+static int __init max6651_init(void)
+{
+	return platform_driver_register(&max6651_driver);
+}
+subsys_initcall(max6651_init);
+
+static void __exit max6651_exit(void)
+{
+	platform_driver_unregister(&max6651_driver);
+}
+module_exit(max6651_exit);
+
+MODULE_AUTHOR("Laszlo Papp");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("MAX6651 fan controller");
diff --git a/drivers/gpio/gpio-max665x.c b/drivers/gpio/gpio-max665x.c
new file mode 100644
index 0000000..6615d0f
--- /dev/null
+++ b/drivers/gpio/gpio-max665x.c
@@ -0,0 +1,301 @@
+/**
+ * Copyright (C) 2013 Laszlo Papp <laszlo.papp@polatis.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/mutex.h>
+#include <linux/gpio.h>
+#include <linux/slab.h>
+
+#include <linux/mfd/max6651-private.h>
+
+#define MAX665X_REG_GPIO_DEF    0x04
+#define MAX665X_REG_GPIO_STAT   0x14
+
+/*
+ * Gpio Def register bits
+ */
+
+#define PIN0_CONFIG_MASK    0x03
+#define PIN1_CONFIG_MASK    0x0C
+#define PIN2_CONFIG_MASK    0x30
+#define PIN3_CONFIG_MASK    0x40
+#define PIN4_CONFIG_MASK    0x80
+
+#define PIN0_CONFIG_OUT_LOW  0x02
+#define PIN1_CONFIG_OUT_LOW  0x08
+#define PIN2_CONFIG_OUT_LOW  0x20
+
+struct max665x_platform_data {
+    /* number assigned to the first GPIO */
+    unsigned    base;
+    /*  
+     * bitmask controlling the pullup configuration,
+     *
+     * _note_ the 3 highest  bits are unused, because there can be maximum up
+     * to five gpio pins on the MAX6651 chip (three on MAX6650).
+     */
+    u8     input_pullup_active;
+};
+
+static inline struct max665x_gpio *to_max665x_gpio(struct gpio_chip *chip)
+{
+    return container_of(chip, struct max665x_gpio, gp);
+}
+
+static int max665x_direction_input_or_output_high(struct gpio_chip *chip, unsigned offset)
+{
+	struct max665x_gpio *gpio = to_max665x_gpio(chip);
+	u8 config;
+	int ret;
+
+	mutex_lock(&gpio->iodev->iolock);
+
+	max6651_read_reg(gpio->iodev->i2c, MAX665X_REG_GPIO_DEF, &config);
+
+    switch (offset) {
+    case 0:
+        config |= PIN0_CONFIG_MASK;
+        break;
+    case 1:
+        config |= PIN1_CONFIG_MASK;
+        break;
+    case 2:
+        config |= PIN2_CONFIG_MASK;
+        break;
+    case 3:
+        config |= PIN3_CONFIG_MASK;
+        break;
+    case 4:
+        config |= PIN4_CONFIG_MASK;
+        break;
+    default:
+	    mutex_unlock(&gpio->iodev->iolock);
+        return -EINVAL;
+    }   
+
+	ret = max6651_write_reg(gpio->iodev->i2c, MAX665X_REG_GPIO_DEF, config);
+
+	mutex_unlock(&gpio->iodev->iolock);
+
+	return ret;
+}
+
+static int max665x_direction_output_low(struct gpio_chip *chip, unsigned offset)
+{
+	struct max665x_gpio *gpio = to_max665x_gpio(chip);
+	u8 config;
+	int ret;
+
+	mutex_lock(&gpio->iodev->iolock);
+
+	max6651_read_reg(gpio->iodev->i2c, MAX665X_REG_GPIO_DEF, &config);
+
+    switch (offset) {
+    case 0:
+        config &= ~PIN0_CONFIG_MASK;
+        config |= PIN0_CONFIG_OUT_LOW;
+        break;
+    case 1:
+        config &= ~PIN1_CONFIG_MASK;
+        config |= PIN1_CONFIG_OUT_LOW;
+        break;
+    case 2:
+        config &= ~PIN2_CONFIG_MASK;
+        config |= PIN2_CONFIG_OUT_LOW;
+        break;
+    case 3:
+        config &= ~PIN3_CONFIG_MASK;
+        break;
+    case 4:
+        config &= ~PIN4_CONFIG_MASK;
+        break;
+    default:
+	    mutex_unlock(&gpio->iodev->iolock);
+        return -EINVAL;
+    }   
+
+    ret = max6651_write_reg(gpio->iodev->i2c, MAX665X_REG_GPIO_DEF, config);
+
+	mutex_unlock(&gpio->iodev->iolock);
+
+	return ret;
+}
+    
+static int max665x_direction_input(struct gpio_chip *chip, unsigned offset)
+{
+    return max665x_direction_input_or_output_high(chip, offset);
+}
+
+static int max665x_direction_output(struct gpio_chip *chip, unsigned offset,
+				    int value)
+{
+    if (value != 0)
+        return max665x_direction_input_or_output_high(chip, offset);
+
+    return max665x_direction_output_low(chip, offset);
+}
+
+static int max665x_get_level(struct max665x_gpio *gpio, unsigned offset, unsigned gpio_value)
+{
+    int ret;
+
+    if (offset < 3) {
+        switch (gpio_value) {
+        case 0:
+        case 3:
+            if (gpio->input_pullup_active & (1 << offset)) {
+                max6651_read_reg(gpio->iodev->i2c, MAX665X_REG_GPIO_STAT, &ret);
+                ret &= (offset + 1);
+            } else {
+                ret = 1;
+            }
+            break;
+        case 2:
+            ret = 0;
+            break;
+        default:
+            ret = 0;
+            dev_err(gpio->iodev->i2c, "Failed to obtain the gpio %d value\n", offset);
+            break;
+        }
+    } else {
+        if (gpio_value) {
+            if (gpio->input_pullup_active & (1 << offset)) {
+                max6651_read_reg(gpio->iodev->i2c, MAX665X_REG_GPIO_STAT, &ret);
+                ret &= (offset + 1);
+            } else {
+                ret = 1;
+            }
+        } else {
+            ret = 0;
+        }
+    }
+
+    return ret;
+}
+
+static int max665x_get(struct gpio_chip *chip, unsigned offset)
+{
+	struct max665x_gpio *gpio = to_max665x_gpio(chip);
+    int level = -EINVAL;
+	u8 config;
+
+	mutex_lock(&gpio->iodev->iolock);
+
+	max6651_read_reg(gpio->iodev->i2c, MAX665X_REG_GPIO_DEF, &config);
+
+    switch (offset) {
+    case 0:
+        level = max665x_get_level(gpio, offset, config & PIN0_CONFIG_MASK);
+        break;
+    case 1:
+        level = max665x_get_level(gpio, offset, (config & PIN1_CONFIG_MASK) >> 2);
+        break;
+    case 2:
+        level = max665x_get_level(gpio, offset, (config & PIN2_CONFIG_MASK) >> 4);
+        break;
+    case 3:
+        level = max665x_get_level(gpio, offset, (config & PIN3_CONFIG_MASK) >> 5);
+        break;
+    case 4:
+        level = max665x_get_level(gpio, offset, (config & PIN3_CONFIG_MASK) >> 6);
+        break;
+    default:
+	    mutex_unlock(&gpio->iodev->iolock);
+        return -EINVAL;
+    }   
+
+	mutex_unlock(&gpio->iodev->iolock);
+
+	return level;
+}
+
+static void max665x_set(struct gpio_chip *chip, unsigned offset, int value)
+{
+    if (value != 0)
+        max665x_direction_input_or_output_high(chip, offset);
+    else
+        max665x_direction_output_low(chip, offset);
+}
+
+int __max665x_probe(struct max665x_gpio *gpio)
+{
+	struct max665x_platform_data *pdata = dev_get_drvdata(gpio->iodev->dev);
+	int offset, ret;
+
+	mutex_init(&gpio->iodev->iolock);
+	dev_set_drvdata(gpio->iodev->dev, gpio);
+
+    if (pdata) {
+        gpio->input_pullup_active = pdata->input_pullup_active;
+        gpio->gp.base = pdata->base;
+    } else {
+        gpio->gp.base = -1; 
+    }
+	gpio->gp.label = gpio->iodev->dev->driver->name;
+
+	gpio->gp.direction_input = max665x_direction_input;
+	gpio->gp.get = max665x_get;
+	gpio->gp.direction_output = max665x_direction_output;
+	gpio->gp.set = max665x_set;
+
+	gpio->gp.can_sleep = 1;
+	gpio->gp.dev = gpio->iodev->dev;
+	gpio->gp.owner = THIS_MODULE;
+
+    /*  
+     * initialize input pullups according to platform data.
+     */
+
+    for (offset = 0; offset < 5; ++offset) {
+        ret = max665x_direction_input(&gpio->gp, offset);
+
+        if (ret)
+            goto exit_destroy;
+    }
+
+	ret = gpiochip_add(&gpio->gp);
+	if (ret)
+		goto exit_destroy;
+
+	return ret;
+
+exit_destroy:
+	dev_set_drvdata(gpio->iodev->dev, NULL);
+	mutex_destroy(&gpio->iodev->iolock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(__max665x_probe);
+
+int __max665x_remove(struct device *dev)
+{
+	struct max665x_gpio *gpio = dev_get_drvdata(dev);
+	int ret;
+
+	if (gpio == NULL)
+		return -ENODEV;
+
+	dev_set_drvdata(dev, NULL);
+
+	ret = gpiochip_remove(&gpio->gp);
+	if (!ret) {
+		mutex_destroy(&gpio->iodev->iolock);
+		kfree(gpio);
+	} else
+		dev_err(dev, "Failed to remove GPIO controller: %d\n", ret);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(__max665x_remove);
+
+MODULE_AUTHOR("Laszlo Papp");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("MAX665x GPIO-Expanders, generic parts");
diff --git a/include/linux/mfd/max6651-private.h b/include/linux/mfd/max6651-private.h
index ae90261..1fddf59 100644
--- a/include/linux/mfd/max6651-private.h
+++ b/include/linux/mfd/max6651-private.h
@@ -32,6 +32,7 @@
 #include <linux/i2c.h>
 #include <linux/export.h>
 #include <linux/irqdomain.h>
+#include <linux/gpio.h>
 
 struct max6651_dev {
     struct device *dev;
@@ -42,6 +43,15 @@ struct max6651_dev {
 	int type;
 };
 
+struct max665x_gpio {
+    u8     input_pullup_active;
+    struct max6651_dev *iodev;
+    struct gpio_chip gp; 
+};
+
+extern int __max665x_remove(struct device *dev);
+extern int __max665x_probe(struct max665x_gpio *ts);
+
 enum max6651_types {
 	TYPE_MAX6650,
 	TYPE_MAX6651,
-- 
1.8.5.1

Re: [PATCH 0/3] Add GPIO support for the MAX6650/6651 ICs

[Lee Jones]

Don't send private, off-list emails for poking or anything else
related to upstreamming patches.

> Important text here later. It is now just an attempt to get some feedback about
> the design of the new features. This set has only been compilation tested, and
> has many errors with checkpatch.pl, et al. Really, the point is only to get
> feedback about the direction, as there has been a lot of paid work put into
> this to work better with upstream after the previously rejected change simple
> hwmon change.

If you want to obtain time/advice from maintainers, most of which are
already snowed-under, you have to earn it. Making submissions such as
these are not conducive to your cause. Fix the Checkpatch errors and
do your damnedest to ensure the code is the finest standard it can be.

If you fail to adhere to these [1] three documents accidentally, then
you will be guided. If you do so on purpose by sending intentionally
sloppy code, then you are unlikely to gain any attention at all.

[1]
  Documentation/CodingStyle
  Documentation/email-clients.txt
  Documentation/SubmittingPatches

> Laszlo Papp (3):
>   mfd: MAX6650/6651 support
>   hwmon: (max6650) Convert to be a platform driver
>   gpio: MAX6650/6651 support
> 
>  drivers/gpio/Kconfig                |  14 ++
>  drivers/gpio/Makefile               |   2 +
>  drivers/gpio/gpio-max6651.c         |  72 +++++++++
>  drivers/gpio/gpio-max665x.c         | 301 ++++++++++++++++++++++++++++++++++++
>  drivers/hwmon/Kconfig               |   2 +-
>  drivers/hwmon/max6650.c             | 150 +++++++++---------
>  drivers/mfd/Kconfig                 |  11 ++
>  drivers/mfd/Makefile                |   1 +
>  drivers/mfd/max6651.c               | 132 ++++++++++++++++
>  include/linux/mfd/max6651-private.h |  63 ++++++++
>  10 files changed, 675 insertions(+), 73 deletions(-)
>  create mode 100644 drivers/gpio/gpio-max6651.c
>  create mode 100644 drivers/gpio/gpio-max665x.c
>  create mode 100644 drivers/mfd/max6651.c
>  create mode 100644 include/linux/mfd/max6651-private.h
> 

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

Re: [PATCH 1/3] mfd: MAX6650/6651 support

[Linus Walleij]

On Mon, Dec 23, 2013 at 5:08 PM, Laszlo Papp <lpapp@kde.org> wrote:

> MAX6650/MAX6651 chip is a multi-function device with I2C busses. The
> chip includes fan-speed regulators and monitors, GPIO, and alarm.
>
> This patch is an initial release of a MAX6650/6651 MFD driver that
> supports to enable the chip with its primary I2C bus that will connect
> the hwmon, and then the gpio devices for now.
>
> Signed-off-by: Laszlo Papp <lpapp@kde.org>
(...)
> +++ b/include/linux/mfd/max6651-private.h
> +struct max6651_dev {
> +    struct device *dev;
> +    struct mutex iolock;
> +
> +    struct i2c_client *i2c;
> +
> +       int type;
> +};
> +
> +enum max6651_types {
> +       TYPE_MAX6650,
> +       TYPE_MAX6651,
> +};
> +
> +extern int max6651_read_reg(struct i2c_client *i2c, u8 reg, u8 *dest);
> +extern int max6651_write_reg(struct i2c_client *i2c, u8 reg, u8 value);

It looks like you're reinventing regmap.

- In the Kconfig entry for the MFD device:
  select REGMAP_I2C

- Look in drivers/mfd/stw481x.c for an example using
  <linux/regmap.h>

- Look into
  drivers/regulator/stw481x-vmmc.c
  for an example regmap MFD spawned child cell.

Yours,
Linus Walleij

Re: [PATCH 3/3] gpio: MAX6650/6651 support

[Linus Walleij]

On Mon, Dec 23, 2013 at 5:08 PM, Laszlo Papp <lpapp@kde.org> wrote:

> These ICs already have hwmon driver support, but they also have some gpio
> functionality which this addition tries to address. Later on, there would be an
> MFD driver added as well.
>
> Signed-off-by: Laszlo Papp <lpapp@kde.org>

As mentioned please augment the MFD device to use I2C regmap access.

> +++ b/drivers/gpio/gpio-max6651.c
(...)
> +#define PIN_NUMBER 5

As I can see this is really a GPIO+pin control driver it shall be
moved to drivers/pinctrl.

> +static int max6651_probe(struct platform_device *pdev)
> +{
> +    struct max665x_gpio *gpio;
> +    struct da9055_pdata *pdata;
> +       int ret;
> +
> +       gpio = devm_kzalloc(&pdev->dev, sizeof(struct max665x_gpio), GFP_KERNEL);
> +       if (!gpio)
> +               return -ENOMEM;
> +
> +    gpio->gp.ngpio = PIN_NUMBER;
> +
> +       ret = __max665x_probe(gpio);

Do you *really* have to split up this handling into two files with
criss-cross calls like this? Anyway if you absolutely have to do
this don't use "__" prefixes, those are for the preprocessor.
Just max665x_probe() is fine.

> +static struct platform_driver max6651_driver = {
> +       .driver = {
> +               .name = "gpio-max6651",
> +               .owner = THIS_MODULE,
> +       },
> +       .probe = max6651_probe,
> +       .remove = max6651_remove,
> +       .id_table = max6651_id,
> +};
> +
> +static int __init max6651_init(void)
> +{
> +       return platform_driver_register(&max6651_driver);
> +}
> +subsys_initcall(max6651_init);

Why does it have to be subsys_initcall? Please try to avoid
this.

> +static void __exit max6651_exit(void)
> +{
> +       platform_driver_unregister(&max6651_driver);
> +}
> +module_exit(max6651_exit);

Because then this can just be a module_platform_driver() macro.

> +MODULE_AUTHOR("Laszlo Papp");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("MAX6651 fan controller");

And *why* should I have a fan controller in the GPIO subsystem?
I don't quite get this.

> +++ b/drivers/gpio/gpio-max665x.c
(...)
> +#define MAX665X_REG_GPIO_DEF    0x04
> +#define MAX665X_REG_GPIO_STAT   0x14
> +
> +/*
> + * Gpio Def register bits
> + */
> +
> +#define PIN0_CONFIG_MASK    0x03
> +#define PIN1_CONFIG_MASK    0x0C
> +#define PIN2_CONFIG_MASK    0x30
> +#define PIN3_CONFIG_MASK    0x40
> +#define PIN4_CONFIG_MASK    0x80
> +
> +#define PIN0_CONFIG_OUT_LOW  0x02
> +#define PIN1_CONFIG_OUT_LOW  0x08
> +#define PIN2_CONFIG_OUT_LOW  0x20

Since this is really pin configuration the driver should support more
stuff in the long run, and then it should be in drivers/pinctrl.

If it is "just" GPIO, then rename all PIN* prefixes to LINE*

> +struct max665x_platform_data {
> +    /* number assigned to the first GPIO */
> +    unsigned    base;
> +    /*
> +     * bitmask controlling the pullup configuration,
> +     *
> +     * _note_ the 3 highest  bits are unused, because there can be maximum up
> +     * to five gpio pins on the MAX6651 chip (three on MAX6650).
> +     */
> +    u8     input_pullup_active;

So obviously this is not just GPIO but also pin control.

Read Documentation/pinctrl.txt, you will only need to
implement a pin config interface since it seems you have
no muxing in this component.

- In drivers/pinctrl/Kconfig for your driver select
  GENERIC_PINCONF

- Implement a pinctrl and pinconf interface apart from
  the GPIOlib interface you already have.

- Supply pin control tables to set up biasing.

> +static int max665x_direction_input_or_output_high(struct gpio_chip *chip, unsigned offset)

That sounds like an odd combination.

> +static int max665x_get_level(struct max665x_gpio *gpio, unsigned offset, unsigned gpio_value)
> +{
> +    int ret;
> +
> +    if (offset < 3) {
> +        switch (gpio_value) {
> +        case 0:
> +        case 3:
> +            if (gpio->input_pullup_active & (1 << offset)) {

Why does this only work if pullup is active?

Describe with a comment why this is so.

> +                max6651_read_reg(gpio->iodev->i2c, MAX665X_REG_GPIO_STAT, &ret);
> +                ret &= (offset + 1);
> +            } else {
> +                ret = 1;
> +            }
> +            break;
> +        case 2:
> +            ret = 0;
> +            break;

Describe with a comment this special case and why it behaves like that.

> +        default:
> +            ret = 0;
> +            dev_err(gpio->iodev->i2c, "Failed to obtain the gpio %d value\n", offset);
> +            break;
> +        }
> +    } else {
> +        if (gpio_value) {
> +            if (gpio->input_pullup_active & (1 << offset)) {
> +                max6651_read_reg(gpio->iodev->i2c, MAX665X_REG_GPIO_STAT, &ret);

Same thing...

> +static int max665x_get(struct gpio_chip *chip, unsigned offset)
> +{
> +       struct max665x_gpio *gpio = to_max665x_gpio(chip);
> +    int level = -EINVAL;
> +       u8 config;
> +
> +       mutex_lock(&gpio->iodev->iolock);
> +
> +       max6651_read_reg(gpio->iodev->i2c, MAX665X_REG_GPIO_DEF, &config);
> +
> +    switch (offset) {
> +    case 0:
> +        level = max665x_get_level(gpio, offset, config & PIN0_CONFIG_MASK);
> +        break;
> +    case 1:
> +        level = max665x_get_level(gpio, offset, (config & PIN1_CONFIG_MASK) >> 2);
> +        break;
> +    case 2:
> +        level = max665x_get_level(gpio, offset, (config & PIN2_CONFIG_MASK) >> 4);
> +        break;
> +    case 3:
> +        level = max665x_get_level(gpio, offset, (config & PIN3_CONFIG_MASK) >> 5);
> +        break;
> +    case 4:
> +        level = max665x_get_level(gpio, offset, (config & PIN3_CONFIG_MASK) >> 6);
> +        break;

This looks like it could be made a lot simpler using a table.

> +int __max665x_probe(struct max665x_gpio *gpio)
> +{
> +       struct max665x_platform_data *pdata = dev_get_drvdata(gpio->iodev->dev);
> +       int offset, ret;
> +
> +       mutex_init(&gpio->iodev->iolock);
> +       dev_set_drvdata(gpio->iodev->dev, gpio);
> +
> +    if (pdata) {
> +        gpio->input_pullup_active = pdata->input_pullup_active;

No way. No custom interfaces for setting pullups, use generic pin config.

> +        gpio->gp.base = pdata->base;

Why can't you always use dynamic assignments of GPIO numbers?

> +    } else {
> +        gpio->gp.base = -1;

Like this?

> +    }
> +       gpio->gp.label = gpio->iodev->dev->driver->name;

= dev_name(gpio->iodev->dev); I think.

> +    /*
> +     * initialize input pullups according to platform data.
> +     */

No, this shall be done using a pinctrl table.

> +       ret = gpiochip_add(&gpio->gp);
> +       if (ret)
> +               goto exit_destroy;

When implementing the pinctrl interface, you may want to use
gpiochip_add_pin_range() to cross-reference between GPIOs
and pinctrl.

> +EXPORT_SYMBOL_GPL(__max665x_probe);
> +
> +int __max665x_remove(struct device *dev)

Argh these __underscores, get rid of them.

> +++ b/include/linux/mfd/max6651-private.h
(...)
> +struct max665x_gpio {
> +    u8     input_pullup_active;

No way.

> +    struct max6651_dev *iodev;
> +    struct gpio_chip gp;
> +};
> +
> +extern int __max665x_remove(struct device *dev);
> +extern int __max665x_probe(struct max665x_gpio *ts);

Seems overengineered, try to keep all in one file.

Yours,
Linus Walleij

Re: [PATCH 0/3] Add GPIO support for the MAX6650/6651 ICs

[Laszlo Papp]

As the cover page wrote, this was *not* meant to be a good code
because I wanted to get design feedback. I have asked that before,
too, and you claimed to send some code representing the idea. I have
just done that. I find it weird to get design feedback _after_ doing
all the coding.

Anyway, if you are not willing to give a design review without
finalizing the patch, then just scratch this. I will roll back to
using my previously rejected change in our repository.

On Tue, Jan 7, 2014 at 9:09 AM, Lee Jones <lee.jones@linaro.org> wrote:
> Don't send private, off-list emails for poking or anything else
> related to upstreamming patches.
>
>> Important text here later. It is now just an attempt to get some feedback about
>> the design of the new features. This set has only been compilation tested, and
>> has many errors with checkpatch.pl, et al. Really, the point is only to get
>> feedback about the direction, as there has been a lot of paid work put into
>> this to work better with upstream after the previously rejected change simple
>> hwmon change.
>
> If you want to obtain time/advice from maintainers, most of which are
> already snowed-under, you have to earn it. Making submissions such as
> these are not conducive to your cause. Fix the Checkpatch errors and
> do your damnedest to ensure the code is the finest standard it can be.
>
> If you fail to adhere to these [1] three documents accidentally, then
> you will be guided. If you do so on purpose by sending intentionally
> sloppy code, then you are unlikely to gain any attention at all.
>
> [1]
>   Documentation/CodingStyle
>   Documentation/email-clients.txt
>   Documentation/SubmittingPatches
>
>> Laszlo Papp (3):
>>   mfd: MAX6650/6651 support
>>   hwmon: (max6650) Convert to be a platform driver
>>   gpio: MAX6650/6651 support
>>
>>  drivers/gpio/Kconfig                |  14 ++
>>  drivers/gpio/Makefile               |   2 +
>>  drivers/gpio/gpio-max6651.c         |  72 +++++++++
>>  drivers/gpio/gpio-max665x.c         | 301 ++++++++++++++++++++++++++++++++++++
>>  drivers/hwmon/Kconfig               |   2 +-
>>  drivers/hwmon/max6650.c             | 150 +++++++++---------
>>  drivers/mfd/Kconfig                 |  11 ++
>>  drivers/mfd/Makefile                |   1 +
>>  drivers/mfd/max6651.c               | 132 ++++++++++++++++
>>  include/linux/mfd/max6651-private.h |  63 ++++++++
>>  10 files changed, 675 insertions(+), 73 deletions(-)
>>  create mode 100644 drivers/gpio/gpio-max6651.c
>>  create mode 100644 drivers/gpio/gpio-max665x.c
>>  create mode 100644 drivers/mfd/max6651.c
>>  create mode 100644 include/linux/mfd/max6651-private.h
>>
>
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/3] mfd: MAX6650/6651 support

[Laszlo Papp]

On Tue, Jan 7, 2014 at 2:11 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Dec 23, 2013 at 5:08 PM, Laszlo Papp <lpapp@kde.org> wrote:
>
>> MAX6650/MAX6651 chip is a multi-function device with I2C busses. The
>> chip includes fan-speed regulators and monitors, GPIO, and alarm.
>>
>> This patch is an initial release of a MAX6650/6651 MFD driver that
>> supports to enable the chip with its primary I2C bus that will connect
>> the hwmon, and then the gpio devices for now.
>>
>> Signed-off-by: Laszlo Papp <lpapp@kde.org>
> (...)
>> +++ b/include/linux/mfd/max6651-private.h
>> +struct max6651_dev {
>> +    struct device *dev;
>> +    struct mutex iolock;
>> +
>> +    struct i2c_client *i2c;
>> +
>> +       int type;
>> +};
>> +
>> +enum max6651_types {
>> +       TYPE_MAX6650,
>> +       TYPE_MAX6651,
>> +};
>> +
>> +extern int max6651_read_reg(struct i2c_client *i2c, u8 reg, u8 *dest);
>> +extern int max6651_write_reg(struct i2c_client *i2c, u8 reg, u8 value);
>
> It looks like you're reinventing regmap.
>
> - In the Kconfig entry for the MFD device:
>   select REGMAP_I2C
>
> - Look in drivers/mfd/stw481x.c for an example using
>   <linux/regmap.h>
>
> - Look into
>   drivers/regulator/stw481x-vmmc.c
>   for an example regmap MFD spawned child cell.

Oh, thanks, Linus!

I will have a look.

Re: [PATCH 3/3] gpio: MAX6650/6651 support

[Laszlo Papp]

On Tue, Jan 7, 2014 at 2:33 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Dec 23, 2013 at 5:08 PM, Laszlo Papp <lpapp@kde.org> wrote:
>
>> These ICs already have hwmon driver support, but they also have some gpio
>> functionality which this addition tries to address. Later on, there would be an
>> MFD driver added as well.
>>
>> Signed-off-by: Laszlo Papp <lpapp@kde.org>
>
> As mentioned please augment the MFD device to use I2C regmap access.
>
>> +++ b/drivers/gpio/gpio-max6651.c
> (...)
>> +#define PIN_NUMBER 5
>
> As I can see this is really a GPIO+pin control driver it shall be
> moved to drivers/pinctrl.

Hmm, but then I am not sure why the gpio-max*.c are similar in the
drivers/gpio area. Could you please elaborate? They are somewhat
similar to my understanding, but perhaps there is some fundamental
difference I am not aware of?

>> +static int max6651_probe(struct platform_device *pdev)
>> +{
>> +    struct max665x_gpio *gpio;
>> +    struct da9055_pdata *pdata;
>> +       int ret;
>> +
>> +       gpio = devm_kzalloc(&pdev->dev, sizeof(struct max665x_gpio), GFP_KERNEL);
>> +       if (!gpio)
>> +               return -ENOMEM;
>> +
>> +    gpio->gp.ngpio = PIN_NUMBER;
>> +
>> +       ret = __max665x_probe(gpio);
>
> Do you *really* have to split up this handling into two files with
> criss-cross calls like this?

I personally think it is a bit excessive, so I agree with you. I
wished to stay somewhat consistent to the following drivers:

* gpio-max730x.c
* gpio-max7300.c
* gpio-max7301.c

Are you OK with that then if I do not follow the consistency?

> Anyway if you absolutely have to do
> this don't use "__" prefixes, those are for the preprocessor.
> Just max665x_probe() is fine.

This is because of the same reason as above: consistency. I can drop
it if you wish?

>> +static struct platform_driver max6651_driver = {
>> +       .driver = {
>> +               .name = "gpio-max6651",
>> +               .owner = THIS_MODULE,
>> +       },
>> +       .probe = max6651_probe,
>> +       .remove = max6651_remove,
>> +       .id_table = max6651_id,
>> +};
>> +
>> +static int __init max6651_init(void)
>> +{
>> +       return platform_driver_register(&max6651_driver);
>> +}
>> +subsys_initcall(max6651_init);
>
> Why does it have to be subsys_initcall? Please try to avoid
> this.

It is for consistency just as before. :-) Could you please elaborate
why it is better to be avoided, or at least point me to some
documentation?

>> +static void __exit max6651_exit(void)
>> +{
>> +       platform_driver_unregister(&max6651_driver);
>> +}
>> +module_exit(max6651_exit);
>
> Because then this can just be a module_platform_driver() macro.

Hmm.

>> +MODULE_AUTHOR("Laszlo Papp");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("MAX6651 fan controller");
>
> And *why* should I have a fan controller in the GPIO subsystem?
> I don't quite get this.

The MAX6651 chip is multifunctional, but it is ultimate a fan
controller IC as per Kconfig guided text. If you prefer, I can rename
the term here to refer to the GPIO subfunctionality, or you had
something else in mind?

>> +++ b/drivers/gpio/gpio-max665x.c
> (...)
>> +#define MAX665X_REG_GPIO_DEF    0x04
>> +#define MAX665X_REG_GPIO_STAT   0x14
>> +
>> +/*
>> + * Gpio Def register bits
>> + */
>> +
>> +#define PIN0_CONFIG_MASK    0x03
>> +#define PIN1_CONFIG_MASK    0x0C
>> +#define PIN2_CONFIG_MASK    0x30
>> +#define PIN3_CONFIG_MASK    0x40
>> +#define PIN4_CONFIG_MASK    0x80
>> +
>> +#define PIN0_CONFIG_OUT_LOW  0x02
>> +#define PIN1_CONFIG_OUT_LOW  0x08
>> +#define PIN2_CONFIG_OUT_LOW  0x20
>
> Since this is really pin configuration the driver should support more
> stuff in the long run, and then it should be in drivers/pinctrl.

Could you please elaborate what more stuff you have in mind for this?

> If it is "just" GPIO, then rename all PIN* prefixes to LINE*

I am actually not sure about the difference... I will have a look.

>> +struct max665x_platform_data {
>> +    /* number assigned to the first GPIO */
>> +    unsigned    base;
>> +    /*
>> +     * bitmask controlling the pullup configuration,
>> +     *
>> +     * _note_ the 3 highest  bits are unused, because there can be maximum up
>> +     * to five gpio pins on the MAX6651 chip (three on MAX6650).
>> +     */
>> +    u8     input_pullup_active;
>
> So obviously this is not just GPIO but also pin control.
>
> Read Documentation/pinctrl.txt, you will only need to
> implement a pin config interface since it seems you have
> no muxing in this component.

Ah, getting somewhat clearer (maybe) ...

>
> - In drivers/pinctrl/Kconfig for your driver select
>   GENERIC_PINCONF
>
> - Implement a pinctrl and pinconf interface apart from
>   the GPIOlib interface you already have.
>
> - Supply pin control tables to set up biasing.
>
>> +static int max665x_direction_input_or_output_high(struct gpio_chip *chip, unsigned offset)
>
> That sounds like an odd combination.

Perhaps, but that is how the chip behaves after all.

>> +static int max665x_get_level(struct max665x_gpio *gpio, unsigned offset, unsigned gpio_value)
>> +{
>> +    int ret;
>> +
>> +    if (offset < 3) {
>> +        switch (gpio_value) {
>> +        case 0:
>> +        case 3:
>> +            if (gpio->input_pullup_active & (1 << offset)) {
>
> Why does this only work if pullup is active?
>
> Describe with a comment why this is so.

OK.

>> +                max6651_read_reg(gpio->iodev->i2c, MAX665X_REG_GPIO_STAT, &ret);
>> +                ret &= (offset + 1);
>> +            } else {
>> +                ret = 1;
>> +            }
>> +            break;
>> +        case 2:
>> +            ret = 0;
>> +            break;
>
> Describe with a comment this special case and why it behaves like that.

OK.

>
>> +        default:
>> +            ret = 0;
>> +            dev_err(gpio->iodev->i2c, "Failed to obtain the gpio %d value\n", offset);
>> +            break;
>> +        }
>> +    } else {
>> +        if (gpio_value) {
>> +            if (gpio->input_pullup_active & (1 << offset)) {
>> +                max6651_read_reg(gpio->iodev->i2c, MAX665X_REG_GPIO_STAT, &ret);
>
> Same thing...

OK.

>> +static int max665x_get(struct gpio_chip *chip, unsigned offset)
>> +{
>> +       struct max665x_gpio *gpio = to_max665x_gpio(chip);
>> +    int level = -EINVAL;
>> +       u8 config;
>> +
>> +       mutex_lock(&gpio->iodev->iolock);
>> +
>> +       max6651_read_reg(gpio->iodev->i2c, MAX665X_REG_GPIO_DEF, &config);
>> +
>> +    switch (offset) {
>> +    case 0:
>> +        level = max665x_get_level(gpio, offset, config & PIN0_CONFIG_MASK);
>> +        break;
>> +    case 1:
>> +        level = max665x_get_level(gpio, offset, (config & PIN1_CONFIG_MASK) >> 2);
>> +        break;
>> +    case 2:
>> +        level = max665x_get_level(gpio, offset, (config & PIN2_CONFIG_MASK) >> 4);
>> +        break;
>> +    case 3:
>> +        level = max665x_get_level(gpio, offset, (config & PIN3_CONFIG_MASK) >> 5);
>> +        break;
>> +    case 4:
>> +        level = max665x_get_level(gpio, offset, (config & PIN3_CONFIG_MASK) >> 6);
>> +        break;
>
> This looks like it could be made a lot simpler using a table.

How exactly would you like to have it?

>> +int __max665x_probe(struct max665x_gpio *gpio)
>> +{
>> +       struct max665x_platform_data *pdata = dev_get_drvdata(gpio->iodev->dev);
>> +       int offset, ret;
>> +
>> +       mutex_init(&gpio->iodev->iolock);
>> +       dev_set_drvdata(gpio->iodev->dev, gpio);
>> +
>> +    if (pdata) {
>> +        gpio->input_pullup_active = pdata->input_pullup_active;
>
> No way. No custom interfaces for setting pullups, use generic pin config.

What do you mean here? Could you please elaborate a bit more? The pull
up trait depends on the given hardware. It must be coming from
platform data, e.g. we can supply the right variant from our custom
board file.

>> +        gpio->gp.base = pdata->base;
>
> Why can't you always use dynamic assignments of GPIO numbers?

Because this information is board related.

>> +    } else {
>> +        gpio->gp.base = -1;
>
> Like this?

See above.

>> +    }
>> +       gpio->gp.label = gpio->iodev->dev->driver->name;
>
> = dev_name(gpio->iodev->dev); I think.
>
>> +    /*
>> +     * initialize input pullups according to platform data.
>> +     */
>
> No, this shall be done using a pinctrl table.

I will have a look...

>
>> +       ret = gpiochip_add(&gpio->gp);
>> +       if (ret)
>> +               goto exit_destroy;
>
> When implementing the pinctrl interface, you may want to use
> gpiochip_add_pin_range() to cross-reference between GPIOs
> and pinctrl.

Well, Guenter wanted to go through the gpio system... Perhaps, it was
not made clear that pinctrl would be better. I just followed the GPIO
generic guideline. :)

>
>> +EXPORT_SYMBOL_GPL(__max665x_probe);
>> +
>> +int __max665x_remove(struct device *dev)
>
> Argh these __underscores, get rid of them.

OK.

>> +++ b/include/linux/mfd/max6651-private.h
> (...)
>> +struct max665x_gpio {
>> +    u8     input_pullup_active;
>
> No way.

Why so? How would you make it customizable by board files otherwise?

>
>> +    struct max6651_dev *iodev;
>> +    struct gpio_chip gp;
>> +};
>> +
>> +extern int __max665x_remove(struct device *dev);
>> +extern int __max665x_probe(struct max665x_gpio *ts);
>
> Seems overengineered, try to keep all in one file.

OK?

Re: [PATCH 0/3] Add GPIO support for the MAX6650/6651 ICs

[Laszlo Papp]

In other words, I really do not know what format you prefer for design review.

Apparently, not as previously requested, so please tell me what format
one _should_ submit design reviews in.

I personally still think that submitting "pseudo-code" is a good way
of it if it is clearly marked so like in my case, so that it does not
mislead anyone that I accidentally submit a clearly broken change for
an integration candidate.

I hope it is understood that I am asking about design reviews before
the lengthy implementation to potentially avoid a lot of additional
work on both sides with writing and reviewing an implementation with a
"wrong" approach...

Cheers ...

Re: [PATCH 0/3] Add GPIO support for the MAX6650/6651 ICs

[Lee Jones]

On Wed, 08 Jan 2014, Laszlo Papp wrote:

> In other words, I really do not know what format you prefer for design review.
> 
> Apparently, not as previously requested, so please tell me what format
> one _should_ submit design reviews in.
> 
> I personally still think that submitting "pseudo-code" is a good way
> of it if it is clearly marked so like in my case, so that it does not
> mislead anyone that I accidentally submit a clearly broken change for
> an integration candidate.
> 
> I hope it is understood that I am asking about design reviews before
> the lengthy implementation to potentially avoid a lot of additional
> work on both sides with writing and reviewing an implementation with a
> "wrong" approach...

The correct way to do this is to submit RFC patches. If you start your
$SUBJECT line with [PATCH RFC x/x], then we know that they're
development patches and we can treat them as such.

If you're submitting code, in form, which are you in this case, they
should still abide by the submission rules. I you'd like us to review
them, then they need to be in an easy to read format. Submitting
patches with lots of whitespace variation is off-putting and hard to
review. If you want people to put aside their time to help you, then
the least you can do is make it easy for them to read. Conforming to
the 3 documentation files (I know how much you love documentation)
will ensure you're giving yourself a snowball's chance in Hell of
'winning' the response you desire.

All this toing and froing is wasting everyone's time. Just submit
nice patches for us to review and you will receive the help you
crave.

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

Re: [PATCH 0/3] Add GPIO support for the MAX6650/6651 ICs

[Laszlo Papp]

On Wed, Jan 8, 2014 at 5:22 PM, Lee Jones <lee.jones@linaro.org> wrote:
> On Wed, 08 Jan 2014, Laszlo Papp wrote:
>
>> In other words, I really do not know what format you prefer for design review.
>>
>> Apparently, not as previously requested, so please tell me what format
>> one _should_ submit design reviews in.
>>
>> I personally still think that submitting "pseudo-code" is a good way
>> of it if it is clearly marked so like in my case, so that it does not
>> mislead anyone that I accidentally submit a clearly broken change for
>> an integration candidate.
>>
>> I hope it is understood that I am asking about design reviews before
>> the lengthy implementation to potentially avoid a lot of additional
>> work on both sides with writing and reviewing an implementation with a
>> "wrong" approach...
>
> The correct way to do this is to submit RFC patches. If you start your
> $SUBJECT line with [PATCH RFC x/x], then we know that they're
> development patches and we can treat them as such.
>
> If you're submitting code, in form, which are you in this case, they
> should still abide by the submission rules. I you'd like us to review
> them, then they need to be in an easy to read format. Submitting
> patches with lots of whitespace variation is off-putting and hard to
> review. If you want people to put aside their time to help you, then
> the least you can do is make it easy for them to read. Conforming to
> the 3 documentation files (I know how much you love documentation)
> will ensure you're giving yourself a snowball's chance in Hell of
> 'winning' the response you desire.
>
> All this toing and froing is wasting everyone's time. Just submit
> nice patches for us to review and you will receive the help you
> crave.

Please let me know an automatized way for fixing up style issues for
the kernel. Should I use a separate editor profile for Linux kernel
development? What is the best practice out there?

I can say that from practice that Lindent and others things made the
code even messier than it had been. It is not as simple as you may
think for a newcomer than me. I would rather _not_ spend time with
stylistic issues when the main point is clearly not styling, but
design review.

My conclusion from your email is that, I will not submit code then
because it is just a waste of time for design discussion unless there
is a simple and automated way of taking care of the style stuff.

Cheers again ...

Re: [PATCH 1/3] mfd: MAX6650/6651 support

[Lee Jones]

> MAX6650/MAX6651 chip is a multi-function device with I2C busses. The
> chip includes fan-speed regulators and monitors, GPIO, and alarm.
> 
> This patch is an initial release of a MAX6650/6651 MFD driver that
> supports to enable the chip with its primary I2C bus that will connect
> the hwmon, and then the gpio devices for now.
> 
> Signed-off-by: Laszlo Papp <lpapp@kde.org>
> ---
>  drivers/mfd/Kconfig                 |  11 +++
>  drivers/mfd/Makefile                |   1 +
>  drivers/mfd/max6651.c               | 132 ++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/max6651-private.h |  53 +++++++++++++++
>  4 files changed, 197 insertions(+)
>  create mode 100644 drivers/mfd/max6651.c
>  create mode 100644 include/linux/mfd/max6651-private.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index dd67158..706c4e5 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -321,6 +321,17 @@ config MFD_88PM860X
>  	  select individual components like voltage regulators, RTC and
>  	  battery-charger under the corresponding menus.
>  
> +config MFD_MAX6651
> +	bool "Maxim Semiconductor MAX6651 Support"
> +	depends on I2C=y
> +	select MFD_CORE
> +	select IRQ_DOMAIN

Why have you selected IRQ_DOMAIN?

> +#include <linux/device.h>
> +#include <linux/delay.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/core.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/i2c.h>

Are you sure all these are used? I'm pretty sure some of them are
not. Only add headers if you require them. Try not to copy and paste
stuff you don't need.

> +#include <linux/mfd/max6651-private.h>
> +
> +static struct mfd_cell max6651_devs[] = {
> +    { .name = "max6651-gpio", },
> +    { .name = "max6650", },

It would be nice to have a comment here to indicate that this is a
hwmon driver. If you're planning to add support for the MAX6651 to
this existing driver, also consider renaming it to "max665x".

> +};
> +
> +int max6651_read_reg(struct i2c_client *i2c, u8 reg, u8 *dest)
> +{

Probably best to use Regmap instead.

regmap_i2c_read()

> +    struct max6651_dev *max6651 = i2c_get_clientdata(i2c);
> +    int ret;

Always use 8 char tabs for kernel code.

> +    mutex_lock(&max6651->iolock);
> +    ret = i2c_smbus_read_byte_data(i2c, reg);
> +    mutex_unlock(&max6651->iolock);
> +    if (ret < 0)
> +        return ret;
> +
> +    ret &= 0xff;
> +    *dest = ret;

*dest = ret & 0xff is clear enough.

> +    return 0;
> +}
> +EXPORT_SYMBOL_GPL(max6651_read_reg);
> +
> +int max6651_write_reg(struct i2c_client *i2c, u8 reg, u8 value)
> +{
> +    struct max6651_dev *max6651 = i2c_get_clientdata(i2c);
> +    int ret;

Same here.

regmap_i2c_write()

> +    mutex_lock(&max6651->iolock);
> +    ret = i2c_smbus_write_byte_data(i2c, reg, value);
> +    mutex_unlock(&max6651->iolock);
> +    return ret;
> +}
> +EXPORT_SYMBOL_GPL(max6651_write_reg);
> +
> +static int max6651_i2c_probe(struct i2c_client *i2c,
> +			    const struct i2c_device_id *id)
> +{
> +	struct max6651_dev *max6651;
> +	int ret = 0;

Why are you initialising ret?

> +	max6651 = kzalloc(sizeof(struct max6651_dev), GFP_KERNEL);

Use managed resources devm_*.

s/sizeof(struct max6651_dev)/sizeof(*max6651)/

> +	if (max6651 == NULL)

if (!max6651)

> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(i2c, max6651);
> +	max6651->dev = &i2c->dev;
> +
> +	mutex_init(&max6651->iolock);
> +
> +	ret = mfd_add_devices(max6651->dev, -1, max6651_devs,
> +			ARRAY_SIZE(max6651_devs),
> +			NULL, 0, NULL);
> +
> +	if (ret < 0) {
> +        dev_err(max6651->dev, "cannot add mfd cells\n");

Are you trying to add cells or register devices?

> +		goto err_mfd;
> +    }
> +
> +	return ret;
> +
> +err_mfd:
> +	mfd_remove_devices(max6651->dev);

If mfd_add_devices() failed, you don't need to remove them.

> +	kfree(max6651);

If you use managed resources you don't need this.

> +	return ret;
> +}
> +
> +static int max6651_i2c_remove(struct i2c_client *i2c)
> +{
> +    struct max6651_dev *max6651 = i2c_get_clientdata(i2c);
> +
> +    mfd_remove_devices(max6651->dev);

In this case you would normally need to kfree() here, but if you use
managed resources you won't have to.

> +    return 0;
> +}
> +
> +static const struct i2c_device_id max6651_i2c_id[] = {
> +    { "max6650", TYPE_MAX6650 },
> +    { "max6651", TYPE_MAX6651 },

So were're registering the max6650 from here too?

If so, then you need to change the name of the file.

> +    { }

{},

> +};
> +MODULE_DEVICE_TABLE(i2c, max6651_i2c_id);
> +
> +static struct i2c_driver max6651_i2c_driver = {
> +    .driver = {
> +           .name = "max6651",
> +           .owner = THIS_MODULE,
> +    },
> +    .probe = max6651_i2c_probe,
> +    .remove = max6651_i2c_remove,
> +    .id_table = max6651_i2c_id,
> +};

Remove from here ------

> +static int __init max6651_i2c_init(void)
> +{
> +    return i2c_add_driver(&max6651_i2c_driver);
> +}
> +/* init early so consumer devices can complete system boot */

I don't think this is required.

> +subsys_initcall(max6651_i2c_init);
> +
> +static void __exit max6651_i2c_exit(void)
> +{
> +    i2c_del_driver(&max6651_i2c_driver);
> +}
> +module_exit(max6651_i2c_exit);

To here -----

 and replace with one line:

  module_i2c_driver()

<snip>

> +#ifndef __LINUX_MFD_MAX6651_PRIVATE_H
> +#define __LINUX_MFD_MAX6651_PRIVATE_H
> +
> +#include <linux/i2c.h>
> +#include <linux/export.h>

Why is this in here?

> +#include <linux/irqdomain.h>

And this?

> +struct max6651_dev {
> +    struct device *dev;
> +    struct mutex iolock;
> +
> +    struct i2c_client *i2c;

Is this used?

> +	int type;

Or this?

> +};
> +
> +enum max6651_types {
> +	TYPE_MAX6650,
> +	TYPE_MAX6651,
> +};

What are you using these for?

> +extern int max6651_read_reg(struct i2c_client *i2c, u8 reg, u8 *dest);
> +extern int max6651_write_reg(struct i2c_client *i2c, u8 reg, u8 value);

regmap_i2c_read()
regmap_i2c_write()

> +#endif

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

Re: [PATCH 1/3] mfd: MAX6650/6651 support

[Laszlo Papp]

On Wed, Jan 8, 2014 at 10:39 PM, Lee Jones <lee.jones@linaro.org> wrote:
>> MAX6650/MAX6651 chip is a multi-function device with I2C busses. The
>> chip includes fan-speed regulators and monitors, GPIO, and alarm.
>>
>> This patch is an initial release of a MAX6650/6651 MFD driver that
>> supports to enable the chip with its primary I2C bus that will connect
>> the hwmon, and then the gpio devices for now.
>>
>> Signed-off-by: Laszlo Papp <lpapp@kde.org>
>> ---
>>  drivers/mfd/Kconfig                 |  11 +++
>>  drivers/mfd/Makefile                |   1 +
>>  drivers/mfd/max6651.c               | 132 ++++++++++++++++++++++++++++++++++++
>>  include/linux/mfd/max6651-private.h |  53 +++++++++++++++
>>  4 files changed, 197 insertions(+)
>>  create mode 100644 drivers/mfd/max6651.c
>>  create mode 100644 include/linux/mfd/max6651-private.h
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index dd67158..706c4e5 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -321,6 +321,17 @@ config MFD_88PM860X
>>         select individual components like voltage regulators, RTC and
>>         battery-charger under the corresponding menus.
>>
>> +config MFD_MAX6651
>> +     bool "Maxim Semiconductor MAX6651 Support"
>> +     depends on I2C=y
>> +     select MFD_CORE
>> +     select IRQ_DOMAIN
>
> Why have you selected IRQ_DOMAIN?

Initial consistency with other corresponding drivers, but I should
have dropped it once I dropped the irq handling to be as simple as
possible initially.

>> +#include <linux/device.h>
>> +#include <linux/delay.h>
>> +#include <linux/input.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/slab.h>
>> +#include <linux/module.h>
>> +#include <linux/i2c.h>
>
> Are you sure all these are used? I'm pretty sure some of them are
> not. Only add headers if you require them. Try not to copy and paste
> stuff you don't need.

Yes, this was meant to be the "final clean up step". I aimed
functionality and design first.

>> +#include <linux/mfd/max6651-private.h>
>> +
>> +static struct mfd_cell max6651_devs[] = {
>> +    { .name = "max6651-gpio", },
>> +    { .name = "max6650", },
>
> It would be nice to have a comment here to indicate that this is a
> hwmon driver. If you're planning to add support for the MAX6651 to
> this existing driver,

Actually, it is already renamed to max6650-hwmon in the next patch of
this series.

> also consider renaming it to "max665x".

Hmm, I will consider.

>> +};
>> +
>> +int max6651_read_reg(struct i2c_client *i2c, u8 reg, u8 *dest)
>> +{
>
> Probably best to use Regmap instead.
>
> regmap_i2c_read()

Yes, do not worry, this was already done after Linus' initial comment.
Note that, it will be a bit hackish to get it working with 3.2 where
the device regmap convenience wrapper was missing, but I guess
backporting is my problem, not yours...

>> +    struct max6651_dev *max6651 = i2c_get_clientdata(i2c);
>> +    int ret;
>
> Always use 8 char tabs for kernel code.

As discussed, style stuff is not fixed for a design review. I am still
intereted in having an automated fix-up like astyle in other projects?
What is the recommended way? I really would not like to waste too much
time with style clean up.

>> +    mutex_lock(&max6651->iolock);
>> +    ret = i2c_smbus_read_byte_data(i2c, reg);
>> +    mutex_unlock(&max6651->iolock);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    ret &= 0xff;
>> +    *dest = ret;
>
> *dest = ret & 0xff is clear enough.

I think this function would be removed altogether instead.

>> +    return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(max6651_read_reg);
>> +
>> +int max6651_write_reg(struct i2c_client *i2c, u8 reg, u8 value)
>> +{
>> +    struct max6651_dev *max6651 = i2c_get_clientdata(i2c);
>> +    int ret;
>
> Same here.
>
> regmap_i2c_write()

See above.

>> +    mutex_lock(&max6651->iolock);
>> +    ret = i2c_smbus_write_byte_data(i2c, reg, value);
>> +    mutex_unlock(&max6651->iolock);
>> +    return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(max6651_write_reg);
>> +
>> +static int max6651_i2c_probe(struct i2c_client *i2c,
>> +                         const struct i2c_device_id *id)
>> +{
>> +     struct max6651_dev *max6651;
>> +     int ret = 0;
>
> Why are you initialising ret?

Habit for striving for good pratice, I think. It may have also been
consitency. That being said, I already removed it when I took a look
at the other driver based on Linus' suggestion. I was trying to be
consistent with other maxim drivers.

The linux kernel drivers are inconsistent in general at large,
unfortunately. It is hard to pick up the "right one" for consistency.
I will do whatever asked as it really does not make any difference for
me.

>> +     max6651 = kzalloc(sizeof(struct max6651_dev), GFP_KERNEL);
>
> Use managed resources devm_*.

Yes, that was also done after Linus' suggestion.

> s/sizeof(struct max6651_dev)/sizeof(*max6651)/
>
>> +     if (max6651 == NULL)
>
> if (!max6651)

Yes, that was also done after Linus' suggestion, although this is
consistent with the other maxim drivers. I will refactor those
later...

>> +             return -ENOMEM;
>> +
>> +     i2c_set_clientdata(i2c, max6651);
>> +     max6651->dev = &i2c->dev;
>> +
>> +     mutex_init(&max6651->iolock);
>> +
>> +     ret = mfd_add_devices(max6651->dev, -1, max6651_devs,
>> +                     ARRAY_SIZE(max6651_devs),
>> +                     NULL, 0, NULL);
>> +
>> +     if (ret < 0) {
>> +        dev_err(max6651->dev, "cannot add mfd cells\n");
>
> Are you trying to add cells or register devices?

I would not know the difference in this context. Care to elaborate?

>> +             goto err_mfd;
>> +    }
>> +
>> +     return ret;
>> +
>> +err_mfd:
>> +     mfd_remove_devices(max6651->dev);
>
> If mfd_add_devices() failed, you don't need to remove them.

Sure.

>> +     kfree(max6651);
>
> If you use managed resources you don't need this.

I am not sure what exactly you mean by managed resource here. I only
used the malloc above as far as I can tell. Perhaps, the called
function has some magic behind. I would need to double check...

>> +     return ret;
>> +}
>> +
>> +static int max6651_i2c_remove(struct i2c_client *i2c)
>> +{
>> +    struct max6651_dev *max6651 = i2c_get_clientdata(i2c);
>> +
>> +    mfd_remove_devices(max6651->dev);
>
> In this case you would normally need to kfree() here, but if you use
> managed resources you won't have to.

As above...

>> +    return 0;
>> +}
>> +
>> +static const struct i2c_device_id max6651_i2c_id[] = {
>> +    { "max6650", TYPE_MAX6650 },
>> +    { "max6651", TYPE_MAX6651 },
>
> So were're registering the max6650 from here too?

Absolutely, that is the idea.

> If so, then you need to change the name of the file.
>
>> +    { }
>
> {},

Yep, tiring style stuff...

>> +};
>> +MODULE_DEVICE_TABLE(i2c, max6651_i2c_id);
>> +
>> +static struct i2c_driver max6651_i2c_driver = {
>> +    .driver = {
>> +           .name = "max6651",
>> +           .owner = THIS_MODULE,
>> +    },
>> +    .probe = max6651_i2c_probe,
>> +    .remove = max6651_i2c_remove,
>> +    .id_table = max6651_i2c_id,
>> +};
>
> Remove from here ------
>
>> +static int __init max6651_i2c_init(void)
>> +{
>> +    return i2c_add_driver(&max6651_i2c_driver);
>> +}
>> +/* init early so consumer devices can complete system boot */
>
> I don't think this is required.
>
>> +subsys_initcall(max6651_i2c_init);
>> +
>> +static void __exit max6651_i2c_exit(void)
>> +{
>> +    i2c_del_driver(&max6651_i2c_driver);
>> +}
>> +module_exit(max6651_i2c_exit);
>
> To here -----
>
>  and replace with one line:
>
>   module_i2c_driver()

Yes, this was for consistency again, and modified after Linus' hint.
The other maxim drivers need to be patched later....

>> +#ifndef __LINUX_MFD_MAX6651_PRIVATE_H
>> +#define __LINUX_MFD_MAX6651_PRIVATE_H
>> +
>> +#include <linux/i2c.h>
>> +#include <linux/export.h>
>
> Why is this in here?

Because this series was meant for a design review and overall
direction as opposed to a completely fine tuned patch set. Naturally,
I agree with the feedback of removing unnecessary header inclusion.

>> +#include <linux/irqdomain.h>
>
> And this?

Same stuff as above...

>> +struct max6651_dev {
>> +    struct device *dev;
>> +    struct mutex iolock;
>> +
>> +    struct i2c_client *i2c;
>
> Is this used?

Yes, heavily, for reading and writing the registers in the subdevice drivers.

>> +     int type;
>
> Or this?

Absolutely, this identifies the type, which is necessary for
initializing some corresponding data.

>> +};
>> +
>> +enum max6651_types {
>> +     TYPE_MAX6650,
>> +     TYPE_MAX6651,
>> +};
>
> What are you using these for?

See above.

>> +extern int max6651_read_reg(struct i2c_client *i2c, u8 reg, u8 *dest);
>> +extern int max6651_write_reg(struct i2c_client *i2c, u8 reg, u8 value);
>
> regmap_i2c_read()
> regmap_i2c_write()

Sure, that was done after Linus' hint.

Thanks for the review...

Cheers ...

Re: [PATCH 1/3] mfd: MAX6650/6651 support

[Lee Jones]

> >> +config MFD_MAX6651
> >> +     bool "Maxim Semiconductor MAX6651 Support"
> >> +     depends on I2C=y
> >> +     select MFD_CORE
> >> +     select IRQ_DOMAIN
> >
> > Why have you selected IRQ_DOMAIN?
> 
> Initial consistency with other corresponding drivers, but I should
> have dropped it once I dropped the irq handling to be as simple as
> possible initially.

IRQ_DOMAINs are only relevant for IRQ Controllers.

> >> +#include <linux/device.h>
> >> +#include <linux/delay.h>
> >> +#include <linux/input.h>
> >> +#include <linux/interrupt.h>
> >> +#include <linux/mfd/core.h>
> >> +#include <linux/slab.h>
> >> +#include <linux/module.h>
> >> +#include <linux/i2c.h>
> >
> > Are you sure all these are used? I'm pretty sure some of them are
> > not. Only add headers if you require them. Try not to copy and paste
> > stuff you don't need.
> 
> Yes, this was meant to be the "final clean up step". I aimed
> functionality and design first.

In future please only send your best, most cleaned-up
code. Sub-standard codes desearves nothing but a sub-standard review.

> >> +#include <linux/mfd/max6651-private.h>
> >> +
> >> +static struct mfd_cell max6651_devs[] = {
> >> +    { .name = "max6651-gpio", },
> >> +    { .name = "max6650", },
> >
> > It would be nice to have a comment here to indicate that this is a
> > hwmon driver. If you're planning to add support for the MAX6651 to
> > this existing driver,
> 
> Actually, it is already renamed to max6650-hwmon in the next patch of
> this series.

This won't work, as you haven't changed the name in the
platform_driver struct. And rightly so, as it has nothing to do with
converting the driver over to a platform one. Pull the part that
changes the name into another patch.

> >> +    struct max6651_dev *max6651 = i2c_get_clientdata(i2c);
> >> +    int ret;
> >
> > Always use 8 char tabs for kernel code.
> 
> As discussed, style stuff is not fixed for a design review. I am still
> intereted in having an automated fix-up like astyle in other projects?
> What is the recommended way? I really would not like to waste too much
> time with style clean up.

If you send any more lazy patches where it's clear that no attempt has
been made to adhere to the documentation I've provided you with, I
won't review. 'Please', no more half-ar$ed patches RFC or otherwise.

There is no automated way to get styling right, but the first step is
to set your editor's config for 8 char tabbing at a bare minimum.

<snip>

> >> +static int max6651_i2c_probe(struct i2c_client *i2c,
> >> +                         const struct i2c_device_id *id)
> >> +{
> >> +     struct max6651_dev *max6651;
> >> +     int ret = 0;
> >
> > Why are you initialising ret?
> 
> Habit for striving for good pratice, I think. It may have also been
> consitency. That being said, I already removed it when I took a look
> at the other driver based on Linus' suggestion. I was trying to be
> consistent with other maxim drivers.
> 
> The linux kernel drivers are inconsistent in general at large,
> unfortunately. It is hard to pick up the "right one" for consistency.
> I will do whatever asked as it really does not make any difference for
> me.

The kernel should be mostly standard with this kind of stuff. If the
variable 'could possibly' be read before it is written to, then
initialise it, failing that, don't worry.

> >> +     ret = mfd_add_devices(max6651->dev, -1, max6651_devs,
> >> +                     ARRAY_SIZE(max6651_devs),
> >> +                     NULL, 0, NULL);
> >> +
> >> +     if (ret < 0) {
> >> +        dev_err(max6651->dev, "cannot add mfd cells\n");
> >
> > Are you trying to add cells or register devices?
> 
> I would not know the difference in this context. Care to elaborate?

Providing a cell structure is just a tool. A means to an end if you
will. The real goal here is to register child devices.

"failed to register child devices\n"

> >> +     kfree(max6651);
> >
> > If you use managed resources you don't need this.
> 
> I am not sure what exactly you mean by managed resource here. I only
> used the malloc above as far as I can tell. Perhaps, the called
> function has some magic behind. I would need to double check...

Yes, devm_* (managed resources) contains magic so you don't have you
free your own memory. You can remove the goto altogether.

> >> +static int max6651_i2c_remove(struct i2c_client *i2c)
> >> +{
> >> +    struct max6651_dev *max6651 = i2c_get_clientdata(i2c);
> >> +
> >> +    mfd_remove_devices(max6651->dev);
> >
> > In this case you would normally need to kfree() here, but if you use
> > managed resources you won't have to.
> 
> As above...

As above...

> >> +    return 0;
> >> +}
> >> +
> >> +static const struct i2c_device_id max6651_i2c_id[] = {
> >> +    { "max6650", TYPE_MAX6650 },
> >> +    { "max6651", TYPE_MAX6651 },
> >
> > So were're registering the max6650 from here too?
> 
> Absolutely, that is the idea.
> 
> > If so, then you need to change the name of the file.
> >
> >> +    { }
> >
> > {},
> 
> Yep, tiring style stuff...

Styling i.e nice, neat, easily readable/maintainable code should be
your bread and butter. If styling tires you, perhaps a new career
might be in order. ;) 

<snip>

> >> +#include <linux/i2c.h>
> >> +#include <linux/export.h>
> >
> > Why is this in here?
> 
> Because this series was meant for a design review and overall
> direction as opposed to a completely fine tuned patch set. Naturally,
> I agree with the feedback of removing unnecessary header inclusion.

Don't do that.

> >> +struct max6651_dev {
> >> +    struct device *dev;
> >> +    struct mutex iolock;
> >> +
> >> +    struct i2c_client *i2c;
> >
> > Is this used?
> 
> Yes, heavily, for reading and writing the registers in the subdevice drivers.

Can you show me where?

> >> +     int type;
> >
> > Or this?
> 
> Absolutely, this identifies the type, which is necessary for
> initializing some corresponding data.

Can you show me where?

> >> +};
> >> +
> >> +enum max6651_types {
> >> +     TYPE_MAX6650,
> >> +     TYPE_MAX6651,
> >> +};
> >
> > What are you using these for?
> 
> See above.

Can you show me where you are using them?

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

Re: [PATCH 1/3] mfd: MAX6650/6651 support

[Laszlo Papp]

On Thu, Jan 9, 2014 at 9:41 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> >> +config MFD_MAX6651
>> >> +     bool "Maxim Semiconductor MAX6651 Support"
>> >> +     depends on I2C=y
>> >> +     select MFD_CORE
>> >> +     select IRQ_DOMAIN
>> >
>> > Why have you selected IRQ_DOMAIN?
>>
>> Initial consistency with other corresponding drivers, but I should
>> have dropped it once I dropped the irq handling to be as simple as
>> possible initially.
>
> IRQ_DOMAINs are only relevant for IRQ Controllers.

Sure.

>> >> +#include <linux/device.h>
>> >> +#include <linux/delay.h>
>> >> +#include <linux/input.h>
>> >> +#include <linux/interrupt.h>
>> >> +#include <linux/mfd/core.h>
>> >> +#include <linux/slab.h>
>> >> +#include <linux/module.h>
>> >> +#include <linux/i2c.h>
>> >
>> > Are you sure all these are used? I'm pretty sure some of them are
>> > not. Only add headers if you require them. Try not to copy and paste
>> > stuff you don't need.
>>
>> Yes, this was meant to be the "final clean up step". I aimed
>> functionality and design first.
>
> In future please only send your best, most cleaned-up
> code. Sub-standard codes desearves nothing but a sub-standard review.

Do not worry, I appreciate your review a lot. :)

>> >> +#include <linux/mfd/max6651-private.h>
>> >> +
>> >> +static struct mfd_cell max6651_devs[] = {
>> >> +    { .name = "max6651-gpio", },
>> >> +    { .name = "max6650", },
>> >
>> > It would be nice to have a comment here to indicate that this is a
>> > hwmon driver. If you're planning to add support for the MAX6651 to
>> > this existing driver,
>>
>> Actually, it is already renamed to max6650-hwmon in the next patch of
>> this series.
>
> This won't work, as you haven't changed the name in the
> platform_driver struct. And rightly so, as it has nothing to do with
> converting the driver over to a platform one. Pull the part that
> changes the name into another patch.

I already changed that locally last week when debugging the
platform_match bug about it... This two lines can be taken into a
fourth patch, surely, but is it worth it when the change is already
long for the conversion and two lines do not make much difference.
Anyway, I will surely do it since you would seem to be happier.

>> >> +    struct max6651_dev *max6651 = i2c_get_clientdata(i2c);
>> >> +    int ret;
>> >
>> > Always use 8 char tabs for kernel code.
>>
>> As discussed, style stuff is not fixed for a design review. I am still
>> intereted in having an automated fix-up like astyle in other projects?
>> What is the recommended way? I really would not like to waste too much
>> time with style clean up.
>
> If you send any more lazy patches where it's clear that no attempt has
> been made to adhere to the documentation I've provided you with, I
> won't review. 'Please', no more half-ar$ed patches RFC or otherwise.
>
> There is no automated way to get styling right, but the first step is
> to set your editor's config for 8 char tabbing at a bare minimum.

Yes, I understand. The problem is that the Linux kernel is not the
only project I am contributing to. That is why I was asking what
people do in similar scenarios. Do they have different editor profiles
for each project? How is it gently solved? What is the best practice?
I am using vim for what it is worth. Although, I can probably ask this
on the list in a different thread.... Will do it.

>> >> +static int max6651_i2c_probe(struct i2c_client *i2c,
>> >> +                         const struct i2c_device_id *id)
>> >> +{
>> >> +     struct max6651_dev *max6651;
>> >> +     int ret = 0;
>> >
>> > Why are you initialising ret?
>>
>> Habit for striving for good pratice, I think. It may have also been
>> consitency. That being said, I already removed it when I took a look
>> at the other driver based on Linus' suggestion. I was trying to be
>> consistent with other maxim drivers.
>>
>> The linux kernel drivers are inconsistent in general at large,
>> unfortunately. It is hard to pick up the "right one" for consistency.
>> I will do whatever asked as it really does not make any difference for
>> me.
>
> The kernel should be mostly standard with this kind of stuff. If the
> variable 'could possibly' be read before it is written to, then
> initialise it, failing that, don't worry.

Sure. It might be that my experiment was reading so at some point, and
I did not adhere to the change later. I will remove it as you asked
because I agree.

>> >> +     ret = mfd_add_devices(max6651->dev, -1, max6651_devs,
>> >> +                     ARRAY_SIZE(max6651_devs),
>> >> +                     NULL, 0, NULL);
>> >> +
>> >> +     if (ret < 0) {
>> >> +        dev_err(max6651->dev, "cannot add mfd cells\n");
>> >
>> > Are you trying to add cells or register devices?
>>
>> I would not know the difference in this context. Care to elaborate?
>
> Providing a cell structure is just a tool. A means to an end if you
> will. The real goal here is to register child devices.
>
> "failed to register child devices\n"

Right, but in that case, I will go ahead and fix at the place rfom
where I picked this up.

>> >> +     kfree(max6651);
>> >
>> > If you use managed resources you don't need this.
>>
>> I am not sure what exactly you mean by managed resource here. I only
>> used the malloc above as far as I can tell. Perhaps, the called
>> function has some magic behind. I would need to double check...
>
> Yes, devm_* (managed resources) contains magic so you don't have you
> free your own memory. You can remove the goto altogether.

OK, thankie.

>> >> +static int max6651_i2c_remove(struct i2c_client *i2c)
>> >> +{
>> >> +    struct max6651_dev *max6651 = i2c_get_clientdata(i2c);
>> >> +
>> >> +    mfd_remove_devices(max6651->dev);
>> >
>> > In this case you would normally need to kfree() here, but if you use
>> > managed resources you won't have to.
>>
>> As above...
>
> As above...

As above... (thankie). :-)

>> >> +    return 0;
>> >> +}
>> >> +
>> >> +static const struct i2c_device_id max6651_i2c_id[] = {
>> >> +    { "max6650", TYPE_MAX6650 },
>> >> +    { "max6651", TYPE_MAX6651 },
>> >
>> > So were're registering the max6650 from here too?
>>
>> Absolutely, that is the idea.
>>
>> > If so, then you need to change the name of the file.

Yes, we discussed that before and I agree with the "obfuscating" 'x'.

>> >
>> >> +    { }
>> >
>> > {},
>>
>> Yep, tiring style stuff...
>
> Styling i.e nice, neat, easily readable/maintainable code should be
> your bread and butter. If styling tires you, perhaps a new career
> might be in order. ;)

or a new tool to be more professional ...

>> >> +#include <linux/i2c.h>
>> >> +#include <linux/export.h>
>> >
>> > Why is this in here?
>>
>> Because this series was meant for a design review and overall
>> direction as opposed to a completely fine tuned patch set. Naturally,
>> I agree with the feedback of removing unnecessary header inclusion.
>
> Don't do that.

I will guess that you mean "unnecessary header inclusion is OK"...

>> >> +struct max6651_dev {
>> >> +    struct device *dev;
>> >> +    struct mutex iolock;
>> >> +
>> >> +    struct i2c_client *i2c;
>> >
>> > Is this used?
>>
>> Yes, heavily, for reading and writing the registers in the subdevice drivers.
>
> Can you show me where?

Check the gpio driver or even the hwmon in this series. Look at the
places where it is using the read/write_reg functions, or you can just
check their signature in this patch. I am in the process of
refactoring it into regmap as we speak, but it is not painless because
I need to get it working for 3.2, too ...

>> >> +     int type;
>> >
>> > Or this?
>>
>> Absolutely, this identifies the type, which is necessary for
>> initializing some corresponding data.
>
> Can you show me where?

Well, you have different number of GPIO pins for starter ...

>> >> +};
>> >> +
>> >> +enum max6651_types {
>> >> +     TYPE_MAX6650,
>> >> +     TYPE_MAX6651,
>> >> +};
>> >
>> > What are you using these for?
>>
>> See above.
>
> Can you show me where you are using them?

Perhaps, I was not while submitting this change, but the upcoming
changes should.

Re: [PATCH 1/3] mfd: MAX6650/6651 support

[Lee Jones]

> > Styling i.e nice, neat, easily readable/maintainable code should be
> > your bread and butter. If styling tires you, perhaps a new career
> > might be in order. ;)
> 
> or a new tool to be more professional ...

Patches accepted.

> >> >> +#include <linux/i2c.h>
> >> >> +#include <linux/export.h>
> >> >
> >> > Why is this in here?
> >>
> >> Because this series was meant for a design review and overall
> >> direction as opposed to a completely fine tuned patch set. Naturally,
> >> I agree with the feedback of removing unnecessary header inclusion.
> >
> > Don't do that.
> 
> I will guess that you mean "unnecessary header inclusion is OK"...

No, I mean don't send sub-standard patches which crap you don't need
or a copy and pasted mess contained within them.

> >> >> +struct max6651_dev {
> >> >> +    struct device *dev;
> >> >> +    struct mutex iolock;
> >> >> +
> >> >> +    struct i2c_client *i2c;
> >> >
> >> > Is this used?
> >>
> >> Yes, heavily, for reading and writing the registers in the subdevice drivers.
> >
> > Can you show me where?
> 
> Check the gpio driver or even the hwmon in this series. Look at the
> places where it is using the read/write_reg functions, or you can just
> check their signature in this patch. I am in the process of
> refactoring it into regmap as we speak, but it is not painless because
> I need to get it working for 3.2, too ...

Can you show me the line where you initialise it?

> >> >> +     int type;
> >> >
> >> > Or this?
> >>
> >> Absolutely, this identifies the type, which is necessary for
> >> initializing some corresponding data.
> >
> > Can you show me where?
> 
> Well, you have different number of GPIO pins for starter ...

Can you show me the line where this variable is used?

> >> >> +};
> >> >> +
> >> >> +enum max6651_types {
> >> >> +     TYPE_MAX6650,
> >> >> +     TYPE_MAX6651,
> >> >> +};
> >> >
> >> > What are you using these for?
> >>
> >> See above.
> >
> > Can you show me where you are using them?
> 
> Perhaps, I was not while submitting this change, but the upcoming
> changes should.

Again, this adds confusion. Send your best, cleanest, most up-to-date
code, or all you're doing is wasting people's time.
-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/3] mfd: MAX6650/6651 support

[Laszlo Papp]

On Thu, Jan 9, 2014 at 11:06 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> > Styling i.e nice, neat, easily readable/maintainable code should be
>> > your bread and butter. If styling tires you, perhaps a new career
>> > might be in order. ;)
>>
>> or a new tool to be more professional ...
>
> Patches accepted.
>
>> >> >> +#include <linux/i2c.h>
>> >> >> +#include <linux/export.h>
>> >> >
>> >> > Why is this in here?
>> >>
>> >> Because this series was meant for a design review and overall
>> >> direction as opposed to a completely fine tuned patch set. Naturally,
>> >> I agree with the feedback of removing unnecessary header inclusion.
>> >
>> > Don't do that.
>>
>> I will guess that you mean "unnecessary header inclusion is OK"...
>
> No, I mean don't send sub-standard patches which crap you don't need
> or a copy and pasted mess contained within them.
>
>> >> >> +struct max6651_dev {
>> >> >> +    struct device *dev;
>> >> >> +    struct mutex iolock;
>> >> >> +
>> >> >> +    struct i2c_client *i2c;
>> >> >
>> >> > Is this used?
>> >>
>> >> Yes, heavily, for reading and writing the registers in the subdevice drivers.
>> >
>> > Can you show me where?
>>
>> Check the gpio driver or even the hwmon in this series. Look at the
>> places where it is using the read/write_reg functions, or you can just
>> check their signature in this patch. I am in the process of
>> refactoring it into regmap as we speak, but it is not painless because
>> I need to get it working for 3.2, too ...
>
> Can you show me the line where you initialise it?

max6651->dev = &i2c->dev;
max6651->i2c = i2c;

The last line is missing from the patch submitted as I fixed that only
last week while debugging it hard.

>> >> >> +     int type;
>> >> >
>> >> > Or this?
>> >>
>> >> Absolutely, this identifies the type, which is necessary for
>> >> initializing some corresponding data.
>> >
>> > Can you show me where?
>>
>> Well, you have different number of GPIO pins for starter ...
>
> Can you show me the line where this variable is used?

It is not yet, as explained in my previous email.

>> >> >> +};
>> >> >> +
>> >> >> +enum max6651_types {
>> >> >> +     TYPE_MAX6650,
>> >> >> +     TYPE_MAX6651,
>> >> >> +};
>> >> >
>> >> > What are you using these for?
>> >>
>> >> See above.
>> >
>> > Can you show me where you are using them?
>>
>> Perhaps, I was not while submitting this change, but the upcoming
>> changes should.
>
> Again, this adds confusion. Send your best, cleanest, most up-to-date
> code, or all you're doing is wasting people's time.

Well, that is the point why I was clarifying it.

Note that it is not like I can pull off an update within such a short
while after so many comments.... Especially since there are new
concepts revealed like "regmap", etc. I also need to make sure it
works with both latest and 3.2 where the regmap i2c init convenience
resource manager is not present, etc. It is a time-consuming full-time
job. :-)

An update will be coming addressing all the points.

Re: [PATCH 1/3] mfd: MAX6650/6651 support

[Lee Jones]

> >> > Can you show me where you are using them?
> >>
> >> Perhaps, I was not while submitting this change, but the upcoming
> >> changes should.
> >
> > Again, this adds confusion. Send your best, cleanest, most up-to-date
> > code, or all you're doing is wasting people's time.
> 
> Well, that is the point why I was clarifying it.
> 
> Note that it is not like I can pull off an update within such a short
> while after so many comments.... Especially since there are new
> concepts revealed like "regmap", etc. I also need to make sure it
> works with both latest and 3.2 where the regmap i2c init convenience
> resource manager is not present, etc. It is a time-consuming full-time
> job. :-)

No one is asking you to pull off anything. This is your first
submission and I am reviewing as such.

> An update will be coming addressing all the points.

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

Re: [PATCH 1/3] mfd: MAX6650/6651 support

[Laszlo Papp]

>> +int max6651_read_reg(struct i2c_client *i2c, u8 reg, u8 *dest)
>> +{
>
> Probably best to use Regmap instead.
>
> regmap_i2c_read()

>> +int max6651_write_reg(struct i2c_client *i2c, u8 reg, u8 value)
>> +{
>> +    struct max6651_dev *max6651 = i2c_get_clientdata(i2c);
>> +    int ret;
>
> Same here.
>
> regmap_i2c_write()

Hmm, but what Linus linked is using regmap_read/write(...) instead of
regmap_i2c_read/write(...).

I thought I would need to be using the former. Perhaps, I
misunderstand something?

I do not even find the aforementioend functions used by drivers based
on this LXR result:
http://lxr.free-electrons.com/ident?i=regmap_i2c_write

Re: [PATCH 1/3] mfd: MAX6650/6651 support

[Lee Jones]

On Thu, 09 Jan 2014, Laszlo Papp wrote:

> >> +int max6651_read_reg(struct i2c_client *i2c, u8 reg, u8 *dest)
> >> +{
> >
> > Probably best to use Regmap instead.
> >
> > regmap_i2c_read()
> 
> >> +int max6651_write_reg(struct i2c_client *i2c, u8 reg, u8 value)
> >> +{
> >> +    struct max6651_dev *max6651 = i2c_get_clientdata(i2c);
> >> +    int ret;
> >
> > Same here.
> >
> > regmap_i2c_write()
> 
> Hmm, but what Linus linked is using regmap_read/write(...) instead of
> regmap_i2c_read/write(...).
> 
> I thought I would need to be using the former. Perhaps, I
> misunderstand something?
> 
> I do not even find the aforementioend functions used by drivers based
> on this LXR result:
> http://lxr.free-electrons.com/ident?i=regmap_i2c_write

Linus is in a better position to know, use his suggestion.

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

Re: [PATCH 3/3] gpio: MAX6650/6651 support

[Laszlo Papp]

>>> +#define PIN_NUMBER 5
>>
>> As I can see this is really a GPIO+pin control driver it shall be
>> moved to drivers/pinctrl.
>
> Hmm, but then I am not sure why the gpio-max*.c are similar in the
> drivers/gpio area. Could you please elaborate? They are somewhat
> similar to my understanding, but perhaps there is some fundamental
> difference I am not aware of?

I was giving a second thought to this. Would it be acceptable to add
the gpio driver now, and once the need arises, add the pinctrl thin
layer on top of it? My concern is that I would not use anything else
than the gpio functionality of these pins. It would be a needless
additional work (i.e. investment) for my project and employer.

Perhaps, the layer on top of that can be added later without any
drawback if anyone ever finds the need to have more functionality
supported by these pins?

Shall look forward to hearing your opinion.

Re: [PATCH 3/3] gpio: MAX6650/6651 support

[Linus Walleij]

On Wed, Jan 8, 2014 at 3:59 PM, Laszlo Papp <lpapp@kde.org> wrote:
> On Tue, Jan 7, 2014 at 2:33 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

>> As I can see this is really a GPIO+pin control driver it shall be
>> moved to drivers/pinctrl.
>
> Hmm, but then I am not sure why the gpio-max*.c are similar in the
> drivers/gpio area. Could you please elaborate? They are somewhat
> similar to my understanding, but perhaps there is some fundamental
> difference I am not aware of?

The other drivers were merged before the pin control subsystem
existed. If they had been submitted today, the comments would
be the same as for your driver.

>> Do you *really* have to split up this handling into two files with
>> criss-cross calls like this?
>
> I personally think it is a bit excessive, so I agree with you. I
> wished to stay somewhat consistent to the following drivers:
>
> * gpio-max730x.c
> * gpio-max7300.c
> * gpio-max7301.c
>
> Are you OK with that then if I do not follow the consistency?

Yes. It should be moved to pinctrl anyway.

What about rewriting and fixing up all drivers.

>> Anyway if you absolutely have to do
>> this don't use "__" prefixes, those are for the preprocessor.
>> Just max665x_probe() is fine.
>
> This is because of the same reason as above: consistency. I can drop
> it if you wish?

Yes please.

>> Why does it have to be subsys_initcall? Please try to avoid
>> this.
>
> It is for consistency just as before. :-) Could you please elaborate
> why it is better to be avoided, or at least point me to some
> documentation?

We want to move all drivers to module_init() (device_initcall level)
as shoveling initcalls around is creating an uncontrolled and
unmaintained mess.

To fix this, we're using deferred probe.

See commit d1c3414c2a9d10ef7f0f7665f5d2947cd088c093
"drivercore: Add driver probe deferral mechanism"

>> And *why* should I have a fan controller in the GPIO subsystem?
>> I don't quite get this.
>
> The MAX6651 chip is multifunctional, but it is ultimate a fan
> controller IC as per Kconfig guided text. If you prefer, I can rename
> the term here to refer to the GPIO subfunctionality, or you had
> something else in mind?

Aha OK I can live with this, sorry for missing the Kconfig
fragment.

>> Since this is really pin configuration the driver should support more
>> stuff in the long run, and then it should be in drivers/pinctrl.
>
> Could you please elaborate what more stuff you have in mind for this?

Implement pin config portions of pin control. See
Documentation/pinctrl.txt

(You answer this yourself later.)

>>> +    switch (offset) {
>>> +    case 0:
>>> +        level = max665x_get_level(gpio, offset, config & PIN0_CONFIG_MASK);
>>> +        break;
>>> +    case 1:
>>> +        level = max665x_get_level(gpio, offset, (config & PIN1_CONFIG_MASK) >> 2);
>>> +        break;
>>> +    case 2:
>>> +        level = max665x_get_level(gpio, offset, (config & PIN2_CONFIG_MASK) >> 4);
>>> +        break;
>>> +    case 3:
>>> +        level = max665x_get_level(gpio, offset, (config & PIN3_CONFIG_MASK) >> 5);
>>> +        break;
>>> +    case 4:
>>> +        level = max665x_get_level(gpio, offset, (config & PIN3_CONFIG_MASK) >> 6);
>>> +        break;
>>
>> This looks like it could be made a lot simpler using a table.
>
> How exactly would you like to have it?

struct max_config {
    u32 mask;
    u8 shift;
};

struct max_config max_configs = {
    {
        .mask = PIN0_CONFIG_MASK,
        .shift = 0,
    },
    {
        .mask = PIN1_CONFIG_MASK,
        .shift = 2,
    },
....

struct max_config *cfg = max_configs[offset];

level = max665x_get_level(gpio, offset, (config & cfg->mask) >> cfg->shift);

You get the idea.

>> No way. No custom interfaces for setting pullups, use generic pin config.
>
> What do you mean here? Could you please elaborate a bit more? The pull
> up trait depends on the given hardware. It must be coming from
> platform data, e.g. we can supply the right variant from our custom
> board file.

I mean use pin config from pin control for setting this.
Documentation/pinctrl.txt

Regarding your comment on platform data, see the section named
"Board/machine configuration".

>> Why can't you always use dynamic assignments of GPIO numbers?
>
> Because this information is board related.

OK so this board is not using any dynamic number assignment
system such as ACPI or device tree? Which board is it?

>> When implementing the pinctrl interface, you may want to use
>> gpiochip_add_pin_range() to cross-reference between GPIOs
>> and pinctrl.
>
> Well, Guenter wanted to go through the gpio system... Perhaps, it was
> not made clear that pinctrl would be better. I just followed the GPIO
> generic guideline. :)

GPIO is not sufficient since it needs both GPIO and pin
control interfaces. You need both/and not either/or.

Look at other driver in drivers/pinctrl and you will get the hang
of this.

>>> +++ b/include/linux/mfd/max6651-private.h
>> (...)
>>> +struct max665x_gpio {
>>> +    u8     input_pullup_active;
>>
>> No way.
>
> Why so? How would you make it customizable by board files otherwise?

I mean use pin config from pin control for setting this.
Documentation/pinctrl.txt

Regarding your comment on platform data, see the section named
"Board/machine configuration".

Yours,
Linus Walleij

Re: [PATCH 3/3] gpio: MAX6650/6651 support

[Linus Walleij]

On Mon, Jan 13, 2014 at 10:22 AM, Laszlo Papp <lpapp@kde.org> wrote:

> I was giving a second thought to this. Would it be acceptable to add
> the gpio driver now, and once the need arises, add the pinctrl thin
> layer on top of it?

I will not accept the platform data setting the pull-ups.

> My concern is that I would not use anything else
> than the gpio functionality of these pins. It would be a needless
> additional work (i.e. investment) for my project and employer.

But you are still expecting me as a subsystem maintainer to
take responsibility of this driver for as long as I have this role?

Rewriting code is a natural part of the community process,
noone claimed it would be easy ;-)

> Perhaps, the layer on top of that can be added later without any
> drawback if anyone ever finds the need to have more functionality
> supported by these pins?

Your driver already supports setting the pulls using a
*custom* platform data field. This is not OK, and shall be
implemented using the pin control subsystem. I will not
merge drivers using custom platform data fields like this.

The reason that the pin control subsystem even existed was
that at the time my drivers were NACKed because I tried to
shoehorn pin control into the GPIO subsystem, and as a
result now we have an apropriate subsystem for it, so please
use it.

Yours,
Linus Walleij

Re: [PATCH 3/3] gpio: MAX6650/6651 support

[Laszlo Papp]

On Mon, Jan 13, 2014 at 9:43 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Jan 8, 2014 at 3:59 PM, Laszlo Papp <lpapp@kde.org> wrote:
>> On Tue, Jan 7, 2014 at 2:33 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>
>>> As I can see this is really a GPIO+pin control driver it shall be
>>> moved to drivers/pinctrl.
>>
>> Hmm, but then I am not sure why the gpio-max*.c are similar in the
>> drivers/gpio area. Could you please elaborate? They are somewhat
>> similar to my understanding, but perhaps there is some fundamental
>> difference I am not aware of?
>
> The other drivers were merged before the pin control subsystem
> existed. If they had been submitted today, the comments would
> be the same as for your driver.

Fair enough, but I have one question in here: shall I keep the gpio
driver in place and the layer on top of that would be established
under drivers/pinctrl or the whole should be moved to there?

>>> Do you *really* have to split up this handling into two files with
>>> criss-cross calls like this?
>>
>> I personally think it is a bit excessive, so I agree with you. I
>> wished to stay somewhat consistent to the following drivers:
>>
>> * gpio-max730x.c
>> * gpio-max7300.c
>> * gpio-max7301.c
>>
>> Are you OK with that then if I do not follow the consistency?
>
> Yes. It should be moved to pinctrl anyway.

Even the gpio functionality itself? So, to clarify my question above,
I thought it would be something like this:

- gpio
          \
           ---
              \
- alarm ----- pinctrol
              /
         ----
- etc /

So, pinctrl on top of them, and only this high layer would be moved to
pinctrl. Perhaps, I should check existing drivers and the pinctrl
before asking this... Will have a look very soon.

> What about rewriting and fixing up all drivers.
>
>>> Anyway if you absolutely have to do
>>> this don't use "__" prefixes, those are for the preprocessor.
>>> Just max665x_probe() is fine.
>>
>> This is because of the same reason as above: consistency. I can drop
>> it if you wish?
>
> Yes please.

Good, thanks.

>>> Why does it have to be subsys_initcall? Please try to avoid
>>> this.
>>
>> It is for consistency just as before. :-) Could you please elaborate
>> why it is better to be avoided, or at least point me to some
>> documentation?
>
> We want to move all drivers to module_init() (device_initcall level)
> as shoveling initcalls around is creating an uncontrolled and
> unmaintained mess.
>
> To fix this, we're using deferred probe.
>
> See commit d1c3414c2a9d10ef7f0f7665f5d2947cd088c093
> "drivercore: Add driver probe deferral mechanism"

Thanks.

>>> And *why* should I have a fan controller in the GPIO subsystem?
>>> I don't quite get this.
>>
>> The MAX6651 chip is multifunctional, but it is ultimate a fan
>> controller IC as per Kconfig guided text. If you prefer, I can rename
>> the term here to refer to the GPIO subfunctionality, or you had
>> something else in mind?
>
> Aha OK I can live with this, sorry for missing the Kconfig
> fragment.

OK, thanks.

>>>> +    switch (offset) {
>>>> +    case 0:
>>>> +        level = max665x_get_level(gpio, offset, config & PIN0_CONFIG_MASK);
>>>> +        break;
>>>> +    case 1:
>>>> +        level = max665x_get_level(gpio, offset, (config & PIN1_CONFIG_MASK) >> 2);
>>>> +        break;
>>>> +    case 2:
>>>> +        level = max665x_get_level(gpio, offset, (config & PIN2_CONFIG_MASK) >> 4);
>>>> +        break;
>>>> +    case 3:
>>>> +        level = max665x_get_level(gpio, offset, (config & PIN3_CONFIG_MASK) >> 5);
>>>> +        break;
>>>> +    case 4:
>>>> +        level = max665x_get_level(gpio, offset, (config & PIN3_CONFIG_MASK) >> 6);
>>>> +        break;
>>>
>>> This looks like it could be made a lot simpler using a table.
>>
>> How exactly would you like to have it?
>
> struct max_config {
>     u32 mask;
>     u8 shift;
> };
>
> struct max_config max_configs = {
>     {
>         .mask = PIN0_CONFIG_MASK,
>         .shift = 0,
>     },
>     {
>         .mask = PIN1_CONFIG_MASK,
>         .shift = 2,
>     },
> ....
>
> struct max_config *cfg = max_configs[offset];
>
> level = max665x_get_level(gpio, offset, (config & cfg->mask) >> cfg->shift);
>
> You get the idea.

Yes, thanks.

>>> No way. No custom interfaces for setting pullups, use generic pin config.
>>
>> What do you mean here? Could you please elaborate a bit more? The pull
>> up trait depends on the given hardware. It must be coming from
>> platform data, e.g. we can supply the right variant from our custom
>> board file.
>
> I mean use pin config from pin control for setting this.
> Documentation/pinctrl.txt
>
> Regarding your comment on platform data, see the section named
> "Board/machine configuration".

OK, I will have a look; thanks.

>>> Why can't you always use dynamic assignments of GPIO numbers?
>>
>> Because this information is board related.
>
> OK so this board is not using any dynamic number assignment
> system such as ACPI or device tree?

I cannot be sure if I understand the question correctly, but what I
can say now is that we use a custom board file not upstreamed.

> Which board is it?

It is our custom board. We have also been developing hardware, not
just software.

>>> When implementing the pinctrl interface, you may want to use
>>> gpiochip_add_pin_range() to cross-reference between GPIOs
>>> and pinctrl.
>>
>> Well, Guenter wanted to go through the gpio system... Perhaps, it was
>> not made clear that pinctrl would be better. I just followed the GPIO
>> generic guideline. :)
>
> GPIO is not sufficient since it needs both GPIO and pin
> control interfaces. You need both/and not either/or.
> Look at other driver in drivers/pinctrl and you will get the hang
> of this.

OK, I believe the Linux kernel is too big to adequate answers from
anyone for any area, so it is fine. At least, I have been pointed to
here, but then it is good that this gpio functionality was not put
into the hwmon directly as suggested over there if I am not mistaken.

Re: [PATCH 3/3] gpio: MAX6650/6651 support

[Laszlo Papp]

On Mon, Jan 13, 2014 at 9:48 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Jan 13, 2014 at 10:22 AM, Laszlo Papp <lpapp@kde.org> wrote:
>
>> I was giving a second thought to this. Would it be acceptable to add
>> the gpio driver now, and once the need arises, add the pinctrl thin
>> layer on top of it?
>
> I will not accept the platform data setting the pull-ups.

OK.

>> My concern is that I would not use anything else
>> than the gpio functionality of these pins. It would be a needless
>> additional work (i.e. investment) for my project and employer.
>
> But you are still expecting me as a subsystem maintainer to
> take responsibility of this driver for as long as I have this role?

Well, since we need to make sure that our product rocks and rolls, me
and my employer would need be committed for a quite while, but I can
understand where you are coming from.

> Rewriting code is a natural part of the community process,
> noone claimed it would be easy ;-)

Yes, it is difficult, especially for a C++/OOP person like me... I am
trying to do my best.

>> Perhaps, the layer on top of that can be added later without any
>> drawback if anyone ever finds the need to have more functionality
>> supported by these pins?
>
> Your driver already supports setting the pulls using a
> *custom* platform data field. This is not OK, and shall be
> implemented using the pin control subsystem. I will not
> merge drivers using custom platform data fields like this.
>
> The reason that the pin control subsystem even existed was
> that at the time my drivers were NACKed because I tried to
> shoehorn pin control into the GPIO subsystem, and as a
> result now we have an apropriate subsystem for it, so please
> use it.

OK, thanks.

Re: [PATCH 3/3] gpio: MAX6650/6651 support

[Laszlo Papp]

> Even the gpio functionality itself? So, to clarify my question above,
> I thought it would be something like this:
>
> - gpio
>           \
>            ---
>               \
> - alarm ----- pinctrol
>               /
>          ----
> - etc /
>
> So, pinctrl on top of them, and only this high layer would be moved to
> pinctrl. Perhaps, I should check existing drivers and the pinctrl
> before asking this... Will have a look very soon.

I have just had a quick look and I am now worried.... It seems that
the pinctrl system was not as mature in 3.2 that I need to support as
these days... The short term task of mine is to get it working for
that release, and trying to upstream all this is an additional
bonus....

Hence, my question is: would it be acceptable to leave the gpio
functionality clearly separate and keep the pinctrl functionality on
top of it?

Failing that, is it somehow possible to get this functionality nicely
working with both versions? I am trying to do my best, but even I have
the limits. The definite target is 3.2.1 for us as of now. If it
cannot be nicely done without maintaining two relatively different
drivers until our update, I am afraid I would need to skip the
upstream now due to the lack of appropriate position.

I would appreciate any suggestion about it. The pinctrl system was
introduced in 3.1 if I am not mistaken, and 3.2 has lotta less drivers
around, etc... Not quite sure about the core foundation though.

Re: [PATCH 1/3] mfd: MAX6650/6651 support

[Linus Walleij]

On Fri, Jan 10, 2014 at 10:56 AM, Lee Jones <lee.jones@linaro.org> wrote:
> On Thu, 09 Jan 2014, Laszlo Papp wrote:
>
>> >> +int max6651_read_reg(struct i2c_client *i2c, u8 reg, u8 *dest)
>> >> +{
>> >
>> > Probably best to use Regmap instead.
>> >
>> > regmap_i2c_read()
>>
>> >> +int max6651_write_reg(struct i2c_client *i2c, u8 reg, u8 value)
>> >> +{
>> >> +    struct max6651_dev *max6651 = i2c_get_clientdata(i2c);
>> >> +    int ret;
>> >
>> > Same here.
>> >
>> > regmap_i2c_write()
>>
>> Hmm, but what Linus linked is using regmap_read/write(...) instead of
>> regmap_i2c_read/write(...).
>>
>> I thought I would need to be using the former. Perhaps, I
>> misunderstand something?
>>
>> I do not even find the aforementioend functions used by drivers based
>> on this LXR result:
>> http://lxr.free-electrons.com/ident?i=regmap_i2c_write
>
> Linus is in a better position to know, use his suggestion.

Just use regmap_read/write to abstract things and hide the
underlying marshalling in regmap.

regmap_i2c_write is some regmap-internal thing.

Yours,
Linus Walleij

Re: [PATCH 3/3] gpio: MAX6650/6651 support

[Linus Walleij]

On Tue, Jan 14, 2014 at 6:17 PM, Laszlo Papp <lpapp@kde.org> wrote:

[CC:ing Jon Corbet on this as he's better at the consensus process
and may correct me here.]

> I have just had a quick look and I am now worried.... It seems that
> the pinctrl system was not as mature in 3.2 that I need to support as
> these days... The short term task of mine is to get it working for
> that release, and trying to upstream all this is an additional
> bonus....

I am just a community maintainer, I do not worry very much about
your company's infatuation with ancient kernel releases, so please
do not bring such issues to us. We only develop and worry about
the mainline kernel, Torvald's HEAD, with stability fixes going into
the -stable kernels (not new stuff like this).

> Hence, my question is: would it be acceptable to leave the gpio
> functionality clearly separate and keep the pinctrl functionality on
> top of it?

What do you mean?

These are two orthogonal subsystems with some GPIO-to-pin
criss-cross APIs, I'm just asking that you implement both APIs:
GPIO and pin control.

Please look at other combined drivers in drivers/pinctrl
and you will see what I mean.

> Failing that, is it somehow possible to get this functionality nicely
> working with both versions?

The kernel development process is not suited to such things.
This is not how we work, we don't want people to develop
*new features* on old kernels, that is just insane.

> The definite target is 3.2.1 for us as of now.

I'm sorry to tell you that your organization does not understand
how the mainline community develop new features. If you want to
work with the community you develop new features on the HEAD,
last kernel release (v3.12, soon v3.13) or directly on top of the
pin control development tree in this case.

The fact that some person or function in your company think it
is a good idea to base development on kernel v3.2.x does not
concern me, nor anyone else in the community.

I mean, of course it is interesting to know odd stuff about the
remote corners of kernel development, but it doesn't influence our
behavior.

> I would appreciate any suggestion about it. The pinctrl system was
> introduced in 3.1 if I am not mistaken, and 3.2 has lotta less drivers
> around, etc... Not quite sure about the core foundation though.

If a company decides to work on a fork from the community
that is their problem, basically. Then they assume all forward-porting
and maintenance costs, sometimes also a huge cost of
backward-porting entire subsystems and an increasing threshold
to migrate to new kernel releases as time goes by.

I'm not saying this is necessarily a bad decision for the company,
but it means forking and disconnecting from the community,
we will not come around and work like you do.

I have seen this behaviour in many companies, and what they
typically do not realize is that working this way comes with a
cost because they are still dependent on new features only
developed upstream and they often do not realize the issue of
escalting maintenance cost as the fork widens over time.

Then we have the ontology problem:
The suggestion from any system manager (or similar
function/role) in an organization that they want to lock down
development to a certain older kernel version that "they know
is stable" or "have agreed upon" or something like that is a
complete fallacy, they are not the architects and developers of
the Linux kernel and they are not competent to make such
statements, to put one thing very clear.

The only people that can make such statements you will find
in the MAINTAINERS file of the kernel and that's it. Very simple.
Usually they will tell you to do your development on the very
latest kernel.

It might be that someone pays your wages to work and think
counter to this very simple rule, but that is another thing, that
is not the truth or the way the world works, it is just a power
relation in the working place and while I do realize that this is a
fact of life, it doesn't change the way the community works.

Yours,
Linus Walleij

Re: [PATCH 3/3] gpio: MAX6650/6651 support

[Laszlo Papp]

On Wed, Jan 15, 2014 at 10:05 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Tue, Jan 14, 2014 at 6:17 PM, Laszlo Papp <lpapp@kde.org> wrote:
>
> [CC:ing Jon Corbet on this as he's better at the consensus process
> and may correct me here.]
>
>> I have just had a quick look and I am now worried.... It seems that
>> the pinctrl system was not as mature in 3.2 that I need to support as
>> these days... The short term task of mine is to get it working for
>> that release, and trying to upstream all this is an additional
>> bonus....
>
> I am just a community maintainer, I do not worry very much about
> your company's infatuation with ancient kernel releases, so please
> do not bring such issues to us. We only develop and worry about
> the mainline kernel, Torvald's HEAD, with stability fixes going into
> the -stable kernels (not new stuff like this).
>
>> Hence, my question is: would it be acceptable to leave the gpio
>> functionality clearly separate and keep the pinctrl functionality on
>> top of it?
>
> What do you mean?
>
> These are two orthogonal subsystems with some GPIO-to-pin
> criss-cross APIs, I'm just asking that you implement both APIs:
> GPIO and pin control.
>
> Please look at other combined drivers in drivers/pinctrl
> and you will see what I mean.
>
>> Failing that, is it somehow possible to get this functionality nicely
>> working with both versions?
>
> The kernel development process is not suited to such things.
> This is not how we work, we don't want people to develop
> *new features* on old kernels, that is just insane.
>
>> The definite target is 3.2.1 for us as of now.
>
> I'm sorry to tell you that your organization does not understand
> how the mainline community develop new features. If you want to
> work with the community you develop new features on the HEAD,
> last kernel release (v3.12, soon v3.13) or directly on top of the
> pin control development tree in this case.
>
> The fact that some person or function in your company think it
> is a good idea to base development on kernel v3.2.x does not
> concern me, nor anyone else in the community.
>
> I mean, of course it is interesting to know odd stuff about the
> remote corners of kernel development, but it doesn't influence our
> behavior.
>
>> I would appreciate any suggestion about it. The pinctrl system was
>> introduced in 3.1 if I am not mistaken, and 3.2 has lotta less drivers
>> around, etc... Not quite sure about the core foundation though.
>
> If a company decides to work on a fork from the community
> that is their problem, basically. Then they assume all forward-porting
> and maintenance costs, sometimes also a huge cost of
> backward-porting entire subsystems and an increasing threshold
> to migrate to new kernel releases as time goes by.
>
> I'm not saying this is necessarily a bad decision for the company,
> but it means forking and disconnecting from the community,
> we will not come around and work like you do.
>
> I have seen this behaviour in many companies, and what they
> typically do not realize is that working this way comes with a
> cost because they are still dependent on new features only
> developed upstream and they often do not realize the issue of
> escalting maintenance cost as the fork widens over time.
>
> Then we have the ontology problem:
> The suggestion from any system manager (or similar
> function/role) in an organization that they want to lock down
> development to a certain older kernel version that "they know
> is stable" or "have agreed upon" or something like that is a
> complete fallacy, they are not the architects and developers of
> the Linux kernel and they are not competent to make such
> statements, to put one thing very clear.
>
> The only people that can make such statements you will find
> in the MAINTAINERS file of the kernel and that's it. Very simple.
> Usually they will tell you to do your development on the very
> latest kernel.
>
> It might be that someone pays your wages to work and think
> counter to this very simple rule, but that is another thing, that
> is not the truth or the way the world works, it is just a power
> relation in the working place and while I do realize that this is a
> fact of life, it doesn't change the way the community works.
>
> Yours,
> Linus Walleij

Hmm, this email seems about business stuff and how the community
works, but I did not mean to question that...

I believe I am aware some (most?) of this, but thank you for your
explanation either way. It is useful for the readers. Fwiw, I have
been a remote part of the Nokia Maemo/MeeGo Linux kernel fork and
work. I also developed USB drivers 5-6 years ago where
(out-of-my-desire), we could not upstream drivers for our devices.

Either way, I would like to clarify my previous question as I could
not manage to get the point through: Is it possible /technically/ to
have a similar version with both kernel versions? I am only interested
in this on the technical ground to be able to evaluate if it is worth
dealing with upstreaming at this point at all. I was not expecting you
to accept my situation which is again out-of-my-desire why we have to
stick to an older version.

If the answer is that, "the community has no care to give technical
insight for old versions", that is also fine. No harm done there. I
just thought it would not hurt asking because it could be a potential
benefit for the community if I could manage both without maintain two
separate drivers. The world of course continues further on [1] if we
cannot. Please do not get me wrong, this is not unwillingness, but
currently it would require a huge investment compared to just get
things done and draw the income for the wages. It is out-of-my-desire
or my colleagues' of course, but the life is full of compromise as you
also already know.

Thank you for your answer either way. Cheers.

[1] https://plus.google.com/103895730806848715870/posts/1q34T1iYU4j

Re: [PATCH 3/3] gpio: MAX6650/6651 support

[Linus Walleij]

On Wed, Jan 15, 2014 at 11:51 AM, Laszlo Papp <lpapp@kde.org> wrote:

> Hmm, this email seems about business stuff and how the community
> works, but I did not mean to question that...

Well you did ask to merge something I didn't consider a proper
implementation due to some time constraints so that is why I
brought this up.

> Either way, I would like to clarify my previous question as I could
> not manage to get the point through: Is it possible /technically/ to
> have a similar version with both kernel versions?

When it comes to pin control, no. Because it is developing at
a rapid pace. But the same goes soon for GPIO as well, as we're
refactoring the GPIO pace.

So the answer to the technical question is "it depends"
and it depends on the evolutional change rate in the core of
each subsystem. They can be stable for years and then suddenly
there are changes all over the place, so the back-portability is
sometimes there, sometimes not, and the outcome is
unpredictable.

> I am only interested
> in this on the technical ground to be able to evaluate if it is worth
> dealing with upstreaming at this point at all.

OK sorry if I was a bit explosive in this last reply. I defininately
think you should invest in upstreaming, even if the code you
upstream is not usable in your present company-internal codebase
and current deadlines. Because in the long term, if the thing
you're using it with lives long enough, you will get that
code back one day. And then you can move faster at that point.

Basically I think a person working at the inside of a company
should strive to have *something* booting and ticking using just
the mainline kernel, even if not all drivers are in place. If for
nothing else so for keeping track of what is happening upstream.

> If the answer is that, "the community has no care to give technical
> insight for old versions", that is also fine.

Gave some above I think even if it's not what you wanted. Here
is another thing, <linux/version.h> contains
#define KERNEL_VERSION(a,b,c) (((a) << 16) + ((b) << 8) + (c))
Conjuring a macro from a kernelversion matched to
LINUX_VERSION_CODE so that a driver can #if statements
to conditionally compile stuff for different kernels, like

#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,2,0)
  /* foo */
#else
  /* bar */
#endif

I *think* this is not supposed to be used to make the same code
compile on different kernel versions (rather for things like
glibc etc that need to behave differently with different kernel
baselines), but it *can* be used for e.g. creating kernel modules
that compile to different code on different kernels.

> Please do not get me wrong, this is not unwillingness, but
> currently it would require a huge investment compared to just get
> things done and draw the income for the wages. It is out-of-my-desire
> or my colleagues' of course, but the life is full of compromise as you
> also already know.

Yeah, I realize that not everyone can live to their desires,
which we could call an insight of weltschmerz or Dukkha. As I
am existentialist I try very hard, every day, to make the right choice
for each situation. But that is for each and every one of us to do.
Good luck!

Yours,
Linus Walleij

Re: [PATCH 2/3] hwmon: (max6650) Convert to be a platform driver

[Laszlo Papp]

Ping? Fwiw, this change has been submitted a bit less two months ago,
and it has not still received any feedback from the hwmon maintainers.

On Mon, Dec 23, 2013 at 4:08 PM, Laszlo Papp <lpapp@kde.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>
> ---
>  drivers/hwmon/Kconfig   |   2 +-
>  drivers/hwmon/max6650.c | 150 +++++++++++++++++++++++++-----------------------
>  drivers/mfd/max6651.c   |   2 +-
>  3 files changed, 80 insertions(+), 74 deletions(-)
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 52d548f..ee4fb07 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -925,7 +925,7 @@ config SENSORS_MAX6642
>
>  config SENSORS_MAX6650
>         tristate "Maxim MAX6650 sensor chip"
> -       depends on I2C
> +       depends on MFD_MAX6651
>         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..ec7dac2 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/max6651-private.h>
>
>  /*
>   * Insmod parameters
> @@ -105,24 +108,24 @@ 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);
>
> -static struct i2c_driver max6650_driver = {
> +MODULE_DEVICE_TABLE(platform, max6650_id);
> +
> +static struct platform_driver max6650_driver = {
>         .driver = {
>                 .name   = "max6650",
>         },
> @@ -136,7 +139,9 @@ static struct i2c_driver max6650_driver = {
>   */
>
>  struct max6650_data {
> +       struct max6651 *max6651;
>         struct device *hwmon_dev;
> +       struct max6651_dev *iodev;
>         struct mutex update_lock;
>         int nr_fans;
>         char valid; /* zero until following fields are valid */
> @@ -235,8 +240,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 +268,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);
> +       max6651_write_reg(data->iodev->i2c, MAX6650_REG_SPEED, data->speed);
>
>         mutex_unlock(&data->update_lock);
>
> @@ -304,8 +308,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 +325,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);
> +       max6651_write_reg(data->iodev->i2c, MAX6650_REG_DAC, data->dac);
>
>         mutex_unlock(&data->update_lock);
>
> @@ -350,8 +353,8 @@ 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 platform_device *pdev = to_platform_device(dev);
> +       struct max6650_data *data = platform_get_drvdata(pdev);
>         int max6650_modes[3] = {0, 3, 2};
>         unsigned long mode;
>         int err;
> @@ -365,11 +368,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);
> +       max6651_read_reg(data->iodev->i2c, 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);
> +       max6651_write_reg(data->iodev->i2c, MAX6650_REG_CONFIG, data->config);
>
>         mutex_unlock(&data->update_lock);
>
> @@ -400,8 +403,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 +430,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);
> +       max6651_write_reg(data->iodev->i2c, MAX6650_REG_COUNT, data->count);
>         mutex_unlock(&data->update_lock);
>
>         return count;
> @@ -446,15 +448,14 @@ static ssize_t get_alarm(struct device *dev, struct device_attribute *devattr,
>  {
>         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);
>         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);
> +               max6651_read_reg(data->iodev->i2c, MAX6650_REG_ALARM,
> +                                &data->alarm);
>                 mutex_unlock(&data->update_lock);
>         }
>
> @@ -484,10 +485,13 @@ 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);
> +       u8 alarm_en;
> +
>         struct device_attribute *devattr;
>
> +       max6651_read_reg(data->iodev->i2c, MAX6650_REG_ALARM_EN, &alarm_en);
> +
>         /*
>          * Hide the alarms that have not been enabled by the firmware
>          */
> @@ -539,74 +543,72 @@ 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 max6650_data *data;
>         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);
> +       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_inpdevent(struct platform_device *pdev)
>  {
> -       struct max6650_data *data = i2c_get_clientdata(client);
> -       int config;
> +       struct max6650_data *data = dev_get_drvdata(pdev->dev.parent);
> +       u8 config;
>         int err = -EIO;
>
> -       config = i2c_smbus_read_byte_data(client, MAX6650_REG_CONFIG);
> +       err = max6651_read_reg(data->iodev->i2c, MAX6650_REG_CONFIG, &config);
>
> -       if (config < 0) {
> -               dev_err(&client->dev, "Error reading config, aborting.\n");
> +       if (err < 0) {
> +               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,24 @@ 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 (max6651_write_reg(data->iodev->i2c, 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 (max6651_write_reg(data->iodev->i2c,
> +               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);
> +       max6651_read_reg(data->iodev->i2c, MAX6650_REG_COUNT, &data->count);
>
>         return 0;
>  }
> @@ -694,31 +698,32 @@ 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 platform_device *pdev = to_platform_device(dev);
> +       struct max6650_data *data = platform_get_drvdata(pdev);
> +       u8 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);
> +               max6651_read_reg(data->iodev->i2c, MAX6650_REG_SPEED,
> +                                &data->speed);
> +               max6651_read_reg(data->iodev->i2c, 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]);
> +                       max6651_read_reg(data->iodev->i2c, 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);
> +               max6651_read_reg(data->iodev->i2c, MAX6650_REG_COUNT,
> +                                &data->count);
> +               max6651_read_reg(data->iodev->i2c, 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);
> +               max6651_read_reg(data->iodev->i2c, MAX6650_REG_ALARM, &alarm);
> +               data->alarm |= alarm;
>
>                 data->last_updated = jiffies;
>                 data->valid = 1;
> @@ -729,8 +734,9 @@ 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");
>  MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:max6650-hwmon");
> diff --git a/drivers/mfd/max6651.c b/drivers/mfd/max6651.c
> index c6b1716..d423b7a 100644
> --- a/drivers/mfd/max6651.c
> +++ b/drivers/mfd/max6651.c
> @@ -24,7 +24,7 @@
>
>  static struct mfd_cell max6651_devs[] = {
>      { .name = "max6651-gpio", },
> -    { .name = "max6650", },
> +    { .name = "max6650-hwmon", },
>  };
>
>  int max6651_read_reg(struct i2c_client *i2c, u8 reg, u8 *dest)
> --
> 1.8.5.1
>

Re: [PATCH 2/3] hwmon: (max6650) Convert to be a platform driver

[Lee Jones]

> Ping? Fwiw, this change has been submitted a bit less two months ago,
> and it has not still received any feedback from the hwmon maintainers.

Please rebase and submit it as I suggested, then it will receive
comments.

New beginnings remember?

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