[RFC PATCH 0/2] Redesign the MAX6650-6651 driver to be more flexible

[RFC PATCH 0/2] Redesign the MAX6650-6651 driver to be more flexible

[Laszlo Papp]

Yet to be run-time tested, but early reviews are welcome to catch any obvious
mistakes that are potentially hard and time-consuming to debug.

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

 drivers/hwmon/Kconfig               |   2 +-
 drivers/hwmon/max6650.c             | 125 ++++++++++++++++++------------------
 drivers/mfd/Kconfig                 |  11 ++++
 drivers/mfd/Makefile                |   1 +
 drivers/mfd/max665x.c               |  94 +++++++++++++++++++++++++++
 include/linux/mfd/max665x-private.h |  34 ++++++++++
 6 files changed, 203 insertions(+), 64 deletions(-)
 create mode 100644 drivers/mfd/max665x.c
 create mode 100644 include/linux/mfd/max665x-private.h

-- 
1.8.5.4

[PATCH 1/2] 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/max665x.c               | 94 +++++++++++++++++++++++++++++++++++++
 include/linux/mfd/max665x-private.h | 34 ++++++++++++++
 4 files changed, 140 insertions(+)
 create mode 100644 drivers/mfd/max665x.c
 create mode 100644 include/linux/mfd/max665x-private.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 49bb445..a6c3152 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -368,6 +368,17 @@ config MFD_MAX8907
 	  accessing the device; additional drivers must be enabled in order
 	  to use the functionality of the device.
 
+config MFD_MAX665X
+	bool "Maxim Semiconductor MAX6650/MAX6651 Support"
+	depends on I2C
+	select MFD_CORE
+	select REGMAP_I2C
+	help
+	  Say yes here to add support for Maxim Semiconductor MAX6650/MAX6651. This
+	  is a fan speed regulator and monitor IC. This driver provides common
+	  support for accessing the device, additional drivers must be enabled in
+	  order to use the functionality of the device.
+
 config MFD_MAX8925
 	bool "Maxim Semiconductor MAX8925 PMIC Support"
 	depends on I2C=y
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 5aea5ef..63668c5 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -111,6 +111,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_MAX665X)	+= max665x.o
 obj-$(CONFIG_MFD_MAX14577)	+= max14577.o
 obj-$(CONFIG_MFD_MAX77686)	+= max77686.o max77686-irq.o
 obj-$(CONFIG_MFD_MAX77693)	+= max77693.o max77693-irq.o
diff --git a/drivers/mfd/max665x.c b/drivers/mfd/max665x.c
new file mode 100644
index 0000000..80672d7
--- /dev/null
+++ b/drivers/mfd/max665x.c
@@ -0,0 +1,94 @@
+/*
+ * Device access for MAX6650-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/mfd/core.h>
+#include <linux/module.h>
+#include <linux/i2c.h>
+
+#include <linux/mfd/max665x-private.h>
+
+static struct mfd_cell max665x_devs[] = {
+	{ .name = "max6651-gpio", },
+	{ .name = "max6650", }, /* hwmon driver */
+	{},
+};
+
+static const struct regmap_config max665x_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+static int max665x_probe(struct i2c_client *i2c,
+			    const struct i2c_device_id *id)
+{
+	struct max665x_dev *max665x;
+	int ret;
+
+	max665x = devm_kzalloc(&i2c->dev, sizeof(*max665x), GFP_KERNEL);
+	if (!max665x)
+		return -ENOMEM;
+
+	i2c_set_clientdata(i2c, max665x);
+	max665x->dev = &i2c->dev;
+	max665x->i2c = i2c;
+	max665x->map = devm_regmap_init_i2c(i2c, &max665x_regmap_config);
+	if (IS_ERR(max665x->map)) {
+		ret = PTR_ERR(max665x->map);
+		dev_err(max665x->dev, "Failed to allocate register map: %d\n",
+			ret);
+		return ret;
+	}
+
+	mutex_init(&max665x->iolock);
+
+	ret = mfd_add_devices(max665x->dev, -1, max665x_devs,
+			ARRAY_SIZE(max665x_devs),
+			NULL, 0, NULL);
+
+	if (ret < 0)
+		dev_err(max665x->dev, "failed to register child devices\n");
+
+	return ret;
+}
+
+static int max665x_remove(struct i2c_client *i2c)
+{
+	struct max665x_dev *max665x = i2c_get_clientdata(i2c);
+
+	mfd_remove_devices(max665x->dev);
+
+	return 0;
+}
+
+static struct of_device_id max665x_dt_match[] = {
+	{ .compatible = "maxim,max665x" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, max665x_dt_match);
+
+static struct i2c_driver max665x_driver = {
+	.driver = {
+		.name = "max665x",
+		.owner = THIS_MODULE,
+		.of_match_table = max665x_dt_match,
+	},
+	.probe = max665x_probe,
+	.remove = max665x_remove,
+};
+
+module_i2c_driver(max665x_driver);
+
+MODULE_AUTHOR("Laszlo Papp <laszlo.papp@polatis.com>");
+MODULE_DESCRIPTION("MAX6650-MAX6651 MFD");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/max665x-private.h b/include/linux/mfd/max665x-private.h
new file mode 100644
index 0000000..293db4b
--- /dev/null
+++ b/include/linux/mfd/max665x-private.h
@@ -0,0 +1,34 @@
+/*
+ * max665x-private.h - Driver for the Maxim 6650/6651
+ *
+ * Copyright 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.
+ *
+ * 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.
+ *
+ */
+
+#ifndef __LINUX_MFD_MAX665X_PRIVATE_H
+#define __LINUX_MFD_MAX665X_PRIVATE_H
+
+#include <linux/i2c.h>
+#include <linux/regmap.h>
+
+struct max665x_dev {
+	struct device *dev;
+	struct mutex iolock;
+	struct i2c_client *i2c;
+	struct regmap     *map;
+	int type;
+};
+
+#endif
-- 
1.8.5.4

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

[Lee Jones]

>From [PATCH 0/2]:
  What's the resaon for not testing this on h/w yet?

> 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/max665x.c               | 94 +++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/max665x-private.h | 34 ++++++++++++++
>  4 files changed, 140 insertions(+)
>  create mode 100644 drivers/mfd/max665x.c
>  create mode 100644 include/linux/mfd/max665x-private.h

<snip>

> +++ b/drivers/mfd/max665x.c
> @@ -0,0 +1,94 @@
> +/*
> + * Device access for MAX6650-MAX6651

Nit: Prefer "for Maxim MAX6650 and MAX6651" + <device type>

<snip>

> +#include <linux/device.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>

I thought this driver was bool i.e. Y or N, not tristate?

<snip>

> +static struct mfd_cell max665x_devs[] = {
> +	{ .name = "max6651-gpio", },
> +	{ .name = "max6650", }, /* hwmon driver */

We really need to do something about this.

Guenter,
  once we've converted the hwmon part to a platform device, can we
  then safely rename it with "-hwmon" appended?

> +	{},
> +};
> +
> +static const struct regmap_config max665x_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,

This looks more sane, but it really should be tested.

> +static int max665x_probe(struct i2c_client *i2c,
> +			    const struct i2c_device_id *id)
> +{
> +	struct max665x_dev *max665x;
> +	int ret;
> +
> +	max665x = devm_kzalloc(&i2c->dev, sizeof(*max665x), GFP_KERNEL);
> +	if (!max665x)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(i2c, max665x);
> +	max665x->dev = &i2c->dev;
> +	max665x->i2c = i2c;
> +	max665x->map = devm_regmap_init_i2c(i2c, &max665x_regmap_config);
> +	if (IS_ERR(max665x->map)) {
> +		ret = PTR_ERR(max665x->map);
> +		dev_err(max665x->dev, "Failed to allocate register map: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	mutex_init(&max665x->iolock);

*cough*

> +	ret = mfd_add_devices(max665x->dev, -1, max665x_devs,
> +			ARRAY_SIZE(max665x_devs),
> +			NULL, 0, NULL);
> +
> +	if (ret < 0)

Just 'if (ret)'

<snip>

> +static struct of_device_id max665x_dt_match[] = {
> +	{ .compatible = "maxim,max665x" },

  	{ .compatible = "maxim,max6650" },
  	{ .compatible = "maxim,max6651" },

> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, max665x_dt_match);

Module?

> +static struct i2c_driver max665x_driver = {
> +	.driver = {
> +		.name = "max665x",
> +		.owner = THIS_MODULE,
> +		.of_match_table = max665x_dt_match,

of_match_ptr()

> +	},
> +	.probe = max665x_probe,
> +	.remove = max665x_remove,
> +};
> +
> +module_i2c_driver(max665x_driver);

Module?

> +MODULE_AUTHOR("Laszlo Papp <laszlo.papp@polatis.com>");
> +MODULE_DESCRIPTION("MAX6650-MAX6651 MFD");

s/-/ and /

> +MODULE_LICENSE("GPL");

v1 or v2?

<snip>

> +#ifndef __LINUX_MFD_MAX665X_PRIVATE_H
> +#define __LINUX_MFD_MAX665X_PRIVATE_H
> +
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +
> +struct max665x_dev {
> +	struct device *dev;
> +	struct mutex iolock;

*Ah hem*

> +	struct i2c_client *i2c;
> +	struct regmap     *map;
> +	int type;

What's this used for?

> +};

> +#endif

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

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

[Guenter Roeck]

On Fri, Feb 14, 2014 at 09:15:41AM +0000, Laszlo Papp 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/max665x.c               | 94 +++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/max665x-private.h | 34 ++++++++++++++
>  4 files changed, 140 insertions(+)
>  create mode 100644 drivers/mfd/max665x.c
>  create mode 100644 include/linux/mfd/max665x-private.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 49bb445..a6c3152 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -368,6 +368,17 @@ config MFD_MAX8907
>  	  accessing the device; additional drivers must be enabled in order
>  	  to use the functionality of the device.
>  
> +config MFD_MAX665X
> +	bool "Maxim Semiconductor MAX6650/MAX6651 Support"
> +	depends on I2C
> +	select MFD_CORE
> +	select REGMAP_I2C
> +	help
> +	  Say yes here to add support for Maxim Semiconductor MAX6650/MAX6651. This
> +	  is a fan speed regulator and monitor IC. This driver provides common
> +	  support for accessing the device, additional drivers must be enabled in
> +	  order to use the functionality of the device.
> +
>  config MFD_MAX8925
>  	bool "Maxim Semiconductor MAX8925 PMIC Support"
>  	depends on I2C=y
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 5aea5ef..63668c5 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -111,6 +111,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_MAX665X)	+= max665x.o
>  obj-$(CONFIG_MFD_MAX14577)	+= max14577.o
>  obj-$(CONFIG_MFD_MAX77686)	+= max77686.o max77686-irq.o
>  obj-$(CONFIG_MFD_MAX77693)	+= max77693.o max77693-irq.o
> diff --git a/drivers/mfd/max665x.c b/drivers/mfd/max665x.c
> new file mode 100644
> index 0000000..80672d7
> --- /dev/null
> +++ b/drivers/mfd/max665x.c
> @@ -0,0 +1,94 @@
> +/*
> + * Device access for MAX6650-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/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +
> +#include <linux/mfd/max665x-private.h>
> +
> +static struct mfd_cell max665x_devs[] = {
> +	{ .name = "max6651-gpio", },
> +	{ .name = "max6650", }, /* hwmon driver */
> +	{},
> +};
> +
> +static const struct regmap_config max665x_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +};
> +
> +static int max665x_probe(struct i2c_client *i2c,
> +			    const struct i2c_device_id *id)
> +{
> +	struct max665x_dev *max665x;
> +	int ret;
> +
> +	max665x = devm_kzalloc(&i2c->dev, sizeof(*max665x), GFP_KERNEL);
> +	if (!max665x)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(i2c, max665x);
> +	max665x->dev = &i2c->dev;
> +	max665x->i2c = i2c;
> +	max665x->map = devm_regmap_init_i2c(i2c, &max665x_regmap_config);
> +	if (IS_ERR(max665x->map)) {
> +		ret = PTR_ERR(max665x->map);
> +		dev_err(max665x->dev, "Failed to allocate register map: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	mutex_init(&max665x->iolock);
> +
> +	ret = mfd_add_devices(max665x->dev, -1, max665x_devs,
> +			ARRAY_SIZE(max665x_devs),
> +			NULL, 0, NULL);
> +
> +	if (ret < 0)
> +		dev_err(max665x->dev, "failed to register child devices\n");
> +
> +	return ret;
> +}
> +
> +static int max665x_remove(struct i2c_client *i2c)
> +{
> +	struct max665x_dev *max665x = i2c_get_clientdata(i2c);
> +
> +	mfd_remove_devices(max665x->dev);
> +
> +	return 0;
> +}
> +
> +static struct of_device_id max665x_dt_match[] = {
> +	{ .compatible = "maxim,max665x" },

There was a separate patch to provide actual device types, which I would
suggest to merge with this one.

> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, max665x_dt_match);
> +
> +static struct i2c_driver max665x_driver = {
> +	.driver = {
> +		.name = "max665x",
> +		.owner = THIS_MODULE,
> +		.of_match_table = max665x_dt_match,
> +	},

The 'legacy' means to instantiate i2c devices will need to be supported
for non-DT systems.

> +	.probe = max665x_probe,
> +	.remove = max665x_remove,
> +};
> +
> +module_i2c_driver(max665x_driver);
> +
> +MODULE_AUTHOR("Laszlo Papp <laszlo.papp@polatis.com>");
> +MODULE_DESCRIPTION("MAX6650-MAX6651 MFD");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/max665x-private.h b/include/linux/mfd/max665x-private.h
> new file mode 100644
> index 0000000..293db4b
> --- /dev/null
> +++ b/include/linux/mfd/max665x-private.h
> @@ -0,0 +1,34 @@
> +/*
> + * max665x-private.h - Driver for the Maxim 6650/6651
> + *
> + * Copyright 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.
> + *
> + * 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.
> + *
> + */
> +
> +#ifndef __LINUX_MFD_MAX665X_PRIVATE_H
> +#define __LINUX_MFD_MAX665X_PRIVATE_H
> +
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +
> +struct max665x_dev {
> +	struct device *dev;
> +	struct mutex iolock;
> +	struct i2c_client *i2c;
> +	struct regmap     *map;
> +	int type;

Unless I am missing something, only map is really used in practice.

Clients should not need anything else except possibly type, which is currently
not initialized or used. dev == &i2c->dev and thus redundant even if one is used.

You'll have to decide for a means to pass the device type (max6650 or max6651)
to the client, either through type, the client driver name, or some other means.

Guenter

[PATCH 2/2] 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 | 125 ++++++++++++++++++++++++------------------------
 2 files changed, 63 insertions(+), 64 deletions(-)

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 7ed3aa6..fcfe484 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -962,7 +962,7 @@ config SENSORS_MAX6642
 
 config SENSORS_MAX6650
 	tristate "Maxim MAX6650 sensor chip"
-	depends on I2C
+	depends on MFD_MAX665X
 	help
 	  If you say yes here you get support for the MAX6650 / MAX6651
 	  sensor chips.
diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
index 162a520..4d79413 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -39,6 +39,9 @@
 #include <linux/hwmon.h>
 #include <linux/hwmon-sysfs.h>
 #include <linux/err.h>
+#include <linux/platform_device.h>
+
+#include <linux/mfd/max665x-private.h>
 
 /*
  * Insmod parameters
@@ -112,18 +115,19 @@ module_param(clock, int, S_IRUGO);
 struct max6650_data {
 	struct i2c_client *client;
 	const struct attribute_group *groups[3];
+	struct max665x_dev *iodev;
 	struct mutex update_lock;
 	int nr_fans;
 	char valid; /* zero until following fields are valid */
 	unsigned long last_updated; /* in jiffies */
 
 	/* register values */
-	u8 speed;
-	u8 config;
-	u8 tach[4];
-	u8 count;
-	u8 dac;
-	u8 alarm;
+	int speed;
+	int config;
+	int tach[4];
+	int count;
+	int dac;
+	int alarm;
 };
 
 static const u8 tach_reg[] = {
@@ -136,31 +140,29 @@ static const u8 tach_reg[] = {
 static struct max6650_data *max6650_update_device(struct device *dev)
 {
 	struct max6650_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
+	struct regmap *map = data->iodev->map;
 	int i;
+	int alarm;
 
 	mutex_lock(&data->update_lock);
 
 	if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
-		data->speed = i2c_smbus_read_byte_data(client,
-						       MAX6650_REG_SPEED);
-		data->config = i2c_smbus_read_byte_data(client,
-							MAX6650_REG_CONFIG);
+		regmap_read(map, MAX6650_REG_SPEED, &data->speed);
+		regmap_read(map, MAX6650_REG_CONFIG, &data->config);
 		for (i = 0; i < data->nr_fans; i++) {
-			data->tach[i] = i2c_smbus_read_byte_data(client,
-								 tach_reg[i]);
+			data->tach[i] = regmap_read(map, tach_reg[i],
+							&data->tach[i]);
 		}
-		data->count = i2c_smbus_read_byte_data(client,
-							MAX6650_REG_COUNT);
-		data->dac = i2c_smbus_read_byte_data(client, MAX6650_REG_DAC);
+		regmap_read(map, MAX6650_REG_COUNT, &data->count);
+		regmap_read(map, MAX6650_REG_DAC, &data->dac);
 
 		/*
 		 * Alarms are cleared on read in case the condition that
 		 * caused the alarm is removed. Keep the value latched here
 		 * for providing the register through different alarm files.
 		 */
-		data->alarm |= i2c_smbus_read_byte_data(client,
-							MAX6650_REG_ALARM);
+		regmap_read(map, MAX6650_REG_ALARM, &alarm);
+		data->alarm |= alarm;
 
 		data->last_updated = jiffies;
 		data->valid = 1;
@@ -256,7 +258,6 @@ static ssize_t set_target(struct device *dev, struct device_attribute *devattr,
 			 const char *buf, size_t count)
 {
 	struct max6650_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
 	int kscale, ktach;
 	unsigned long rpm;
 	int err;
@@ -284,7 +285,7 @@ static ssize_t set_target(struct device *dev, struct device_attribute *devattr,
 		ktach = 255;
 	data->speed = ktach;
 
-	i2c_smbus_write_byte_data(client, MAX6650_REG_SPEED, data->speed);
+	regmap_write(data->iodev->map, MAX6650_REG_SPEED, data->speed);
 
 	mutex_unlock(&data->update_lock);
 
@@ -325,7 +326,6 @@ static ssize_t set_pwm(struct device *dev, struct device_attribute *devattr,
 			const char *buf, size_t count)
 {
 	struct max6650_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
 	unsigned long pwm;
 	int err;
 
@@ -342,7 +342,7 @@ static ssize_t set_pwm(struct device *dev, struct device_attribute *devattr,
 	else
 		data->dac = 76 - (76 * pwm)/255;
 
-	i2c_smbus_write_byte_data(client, MAX6650_REG_DAC, data->dac);
+	regmap_write(data->iodev->map, MAX6650_REG_DAC, data->dac);
 
 	mutex_unlock(&data->update_lock);
 
@@ -371,7 +371,7 @@ static ssize_t set_enable(struct device *dev, struct device_attribute *devattr,
 			  const char *buf, size_t count)
 {
 	struct max6650_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
+	struct regmap *map = data->iodev->map;
 	int max6650_modes[3] = {0, 3, 2};
 	unsigned long mode;
 	int err;
@@ -385,11 +385,11 @@ static ssize_t set_enable(struct device *dev, struct device_attribute *devattr,
 
 	mutex_lock(&data->update_lock);
 
-	data->config = i2c_smbus_read_byte_data(client, MAX6650_REG_CONFIG);
+	regmap_read(map, MAX6650_REG_CONFIG, &data->config);
 	data->config = (data->config & ~MAX6650_CFG_MODE_MASK)
 		       | (max6650_modes[mode] << 4);
 
-	i2c_smbus_write_byte_data(client, MAX6650_REG_CONFIG, data->config);
+	regmap_write(map, MAX6650_REG_CONFIG, data->config);
 
 	mutex_unlock(&data->update_lock);
 
@@ -421,7 +421,6 @@ static ssize_t set_div(struct device *dev, struct device_attribute *devattr,
 		       const char *buf, size_t count)
 {
 	struct max6650_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
 	unsigned long div;
 	int err;
 
@@ -448,7 +447,7 @@ static ssize_t set_div(struct device *dev, struct device_attribute *devattr,
 		return -EINVAL;
 	}
 
-	i2c_smbus_write_byte_data(client, MAX6650_REG_COUNT, data->count);
+	regmap_write(data->iodev->map, MAX6650_REG_COUNT, data->count);
 	mutex_unlock(&data->update_lock);
 
 	return count;
@@ -465,16 +464,15 @@ static ssize_t get_alarm(struct device *dev, struct device_attribute *devattr,
 			 char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct max6650_data *data = max6650_update_device(dev);
-	struct i2c_client *client = data->client;
+	struct max6650_data *data = dev_get_drvdata(dev);
 	int alarm = 0;
 
 	if (data->alarm & attr->index) {
 		mutex_lock(&data->update_lock);
-		alarm = 1;
 		data->alarm &= ~attr->index;
-		data->alarm |= i2c_smbus_read_byte_data(client,
-							MAX6650_REG_ALARM);
+		regmap_read(data->iodev->map, MAX6650_REG_ALARM, &alarm);
+		data->alarm |= alarm;
+		alarm = 1;
 		mutex_unlock(&data->update_lock);
 	}
 
@@ -505,10 +503,11 @@ static umode_t max6650_attrs_visible(struct kobject *kobj, struct attribute *a,
 {
 	struct device *dev = container_of(kobj, struct device, kobj);
 	struct max6650_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
-	u8 alarm_en = i2c_smbus_read_byte_data(client, MAX6650_REG_ALARM_EN);
+	int alarm_en;
 	struct device_attribute *devattr;
 
+	regmap_read(data->iodev->map, MAX6650_REG_ALARM_EN, &alarm_en);
+
 	/*
 	 * Hide the alarms that have not been enabled by the firmware
 	 */
@@ -560,17 +559,17 @@ static const struct attribute_group max6651_group = {
  * Real code
  */
 
-static int max6650_init_client(struct max6650_data *data,
-			       struct i2c_client *client)
+static int max6650_init_client(struct platform_device *pdev)
 {
-	struct device *dev = &client->dev;
+	struct max6650_data *data = platform_get_drvdata(pdev);
+	struct regmap *map = data->iodev->map;
 	int config;
 	int err = -EIO;
 
-	config = i2c_smbus_read_byte_data(client, MAX6650_REG_CONFIG);
+	regmap_read(map, MAX6650_REG_CONFIG, &config);
 
 	if (config < 0) {
-		dev_err(dev, "Error reading config, aborting.\n");
+		dev_err(&pdev->dev, "Error reading config, aborting.\n");
 		return err;
 	}
 
@@ -584,11 +583,11 @@ static int max6650_init_client(struct max6650_data *data,
 		config |= MAX6650_CFG_V12;
 		break;
 	default:
-		dev_err(dev, "illegal value for fan_voltage (%d)\n",
+		dev_err(&pdev->dev, "illegal value for fan_voltage (%d)\n",
 			fan_voltage);
 	}
 
-	dev_info(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) {
@@ -614,10 +613,11 @@ static int max6650_init_client(struct max6650_data *data,
 			 | MAX6650_CFG_PRESCALER_16;
 		break;
 	default:
-		dev_err(dev, "illegal value for prescaler (%d)\n", prescaler);
+		dev_err(&pdev->dev, "illegal value for prescaler (%d)\n",
+			prescaler);
 	}
 
-	dev_info(dev, "Prescaler is set to %d.\n",
+	dev_info(&pdev->dev, "Prescaler is set to %d.\n",
 		 1 << (config & MAX6650_CFG_PRESCALER_MASK));
 
 	/*
@@ -627,46 +627,45 @@ static int max6650_init_client(struct max6650_data *data,
 	 */
 
 	if ((config & MAX6650_CFG_MODE_MASK) == MAX6650_CFG_MODE_OFF) {
-		dev_dbg(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(dev, "DAC write error, aborting.\n");
+		if (regmap_write(data->iodev->map, MAX6650_REG_DAC, 255) < 0) {
+			dev_err(&pdev->dev, "DAC write error, aborting.\n");
 			return err;
 		}
 	}
 
-	if (i2c_smbus_write_byte_data(client, MAX6650_REG_CONFIG, config)) {
-		dev_err(dev, "Config write error, aborting.\n");
+	if (regmap_write(map, MAX6650_REG_CONFIG, config) < 0) {
+		dev_err(&pdev->dev, "Config write error, aborting.\n");
 		return err;
 	}
 
 	data->config = config;
-	data->count = i2c_smbus_read_byte_data(client, MAX6650_REG_COUNT);
+	regmap_read(map, MAX6650_REG_COUNT, &data->count);
 
 	return 0;
 }
 
-static int max6650_probe(struct i2c_client *client,
-			 const struct i2c_device_id *id)
+static int max6650_probe(struct platform_device *pdev)
 {
-	struct device *dev = &client->dev;
-	struct max6650_data *data;
+	struct max6650_data *data = platform_get_drvdata(pdev);
+	const struct platform_device_id *id = platform_get_device_id(pdev);
 	struct device *hwmon_dev;
 	int err;
 
-	data = devm_kzalloc(dev, sizeof(struct max6650_data), GFP_KERNEL);
+	data = devm_kzalloc(&pdev->dev, sizeof(struct max6650_data),
+							GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
-	data->client = client;
 	mutex_init(&data->update_lock);
 	data->nr_fans = id->driver_data;
 
 	/*
 	 * Initialize the max6650 chip
 	 */
-	err = max6650_init_client(data, client);
+	err = max6650_init_client(pdev);
 	if (err)
 		return err;
 
@@ -675,20 +674,20 @@ static int max6650_probe(struct i2c_client *client,
 	if (data->nr_fans == 4)
 		data->groups[1] = &max6651_group;
 
-	hwmon_dev = devm_hwmon_device_register_with_groups(dev,
-							   client->name, data,
-							   data->groups);
+	hwmon_dev = devm_hwmon_device_register_with_groups(&pdev->dev,
+							   data->client->name,
+							   data, data->groups);
 	return PTR_ERR_OR_ZERO(hwmon_dev);
 }
 
-static const struct i2c_device_id max6650_id[] = {
+static const struct platform_device_id max6650_id[] = {
 	{ "max6650", 1 },
 	{ "max6651", 4 },
 	{ }
 };
-MODULE_DEVICE_TABLE(i2c, max6650_id);
+MODULE_DEVICE_TABLE(platform, max6650_id);
 
-static struct i2c_driver max6650_driver = {
+static struct platform_driver max6650_driver = {
 	.driver = {
 		.name	= "max6650",
 	},
@@ -696,7 +695,7 @@ static struct i2c_driver max6650_driver = {
 	.id_table	= max6650_id,
 };
 
-module_i2c_driver(max6650_driver);
+module_platform_driver(max6650_driver);
 
 MODULE_AUTHOR("Hans J. Koch");
 MODULE_DESCRIPTION("MAX6650 sensor driver");
-- 
1.8.5.4

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

[Lee Jones]

> The MFD driver has now been added, so this driver is now being adopted to be a
> subdevice driver on top of it. This means, the i2c driver usage is being
> converted to platform driver usage all around.
> 
> Signed-off-by: Laszlo Papp <lpapp@kde.org>
> ---
>  drivers/hwmon/Kconfig   |   2 +-
>  drivers/hwmon/max6650.c | 125 ++++++++++++++++++++++++------------------------
>  2 files changed, 63 insertions(+), 64 deletions(-)
> 
<snip>

> @@ -39,6 +39,9 @@
>  #include <linux/hwmon.h>
>  #include <linux/hwmon-sysfs.h>
>  #include <linux/err.h>
> +#include <linux/platform_device.h>
> +

Not really any need for this.

> +#include <linux/mfd/max665x-private.h>

<snip>

>  struct max6650_data {
>  	struct i2c_client *client;
>  	const struct attribute_group *groups[3];
> +	struct max665x_dev *iodev;

I find the name iodev a bit confusing, why not max665x_dev?

<snip>

> -		data->speed = i2c_smbus_read_byte_data(client,
> -						       MAX6650_REG_SPEED);
> -		data->config = i2c_smbus_read_byte_data(client,
> -							MAX6650_REG_CONFIG);
> +		regmap_read(map, MAX6650_REG_SPEED, &data->speed);
> +		regmap_read(map, MAX6650_REG_CONFIG, &data->config);

I'd like Mark to look over your Regmap implementation if possible.

>  	if (config < 0) {
> -		dev_err(dev, "Error reading config, aborting.\n");
> +		dev_err(&pdev->dev, "Error reading config, aborting.\n");

Rather than make all these changes, just do:

  struct device *dev = &pdev->dev;

... at the top.

<snip>

> +static int max6650_probe(struct platform_device *pdev)
>  {
> -	struct device *dev = &client->dev;
> -	struct max6650_data *data;
> +	struct max6650_data *data = platform_get_drvdata(pdev);

Don't do this here, it's messy. Declare it here and initialise it later.

> +	const struct platform_device_id *id = platform_get_device_id(pdev);

Same here.

>  	struct device *hwmon_dev;
>  	int err;
>  
> -	data = devm_kzalloc(dev, sizeof(struct max6650_data), GFP_KERNEL);
> +	data = devm_kzalloc(&pdev->dev, sizeof(struct max6650_data),
> +							GFP_KERNEL);

Eh? didn't you already initialise 'data'?

<snip>

> -	hwmon_dev = devm_hwmon_device_register_with_groups(dev,
> -							   client->name, data,
> -							   data->groups);
> +	hwmon_dev = devm_hwmon_device_register_with_groups(&pdev->dev,
> +							   data->client->name,
> +							   data, data->groups);

Leave it as 'dev'.

> -static const struct i2c_device_id max6650_id[] = {
> +static const struct platform_device_id max6650_id[] = {
>  	{ "max6650", 1 },
>  	{ "max6651", 4 },

I'm still pretty keen to have these magic numbers #defined.

Not yet though, let's get this sorted first.

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

Guenter, Jean,
  Can this be changed now it's a platform device?

Laszio,
  Next thing to do is to get this working and runtime test it. No more
  mid-term reviews until it's fully functional.

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

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

[Guenter Roeck]

On Fri, Feb 14, 2014 at 09:15:42AM +0000, Laszlo Papp 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 | 125 ++++++++++++++++++++++++------------------------
>  2 files changed, 63 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 7ed3aa6..fcfe484 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -962,7 +962,7 @@ config SENSORS_MAX6642
>  
>  config SENSORS_MAX6650
>  	tristate "Maxim MAX6650 sensor chip"
> -	depends on I2C
> +	depends on MFD_MAX665X
>  	help
>  	  If you say yes here you get support for the MAX6650 / MAX6651
>  	  sensor chips.
> diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
> index 162a520..4d79413 100644
> --- a/drivers/hwmon/max6650.c
> +++ b/drivers/hwmon/max6650.c
> @@ -39,6 +39,9 @@
>  #include <linux/hwmon.h>
>  #include <linux/hwmon-sysfs.h>
>  #include <linux/err.h>
> +#include <linux/platform_device.h>
> +
> +#include <linux/mfd/max665x-private.h>

'private' is a somewhat unusual file name in a global include file.
Why not just max665x.h ?

>  
>  /*
>   * Insmod parameters
> @@ -112,18 +115,19 @@ module_param(clock, int, S_IRUGO);
>  struct max6650_data {
>  	struct i2c_client *client;

client is no longer needed, and all other references to i2c
(such as include files) should be removed as well.

>  	const struct attribute_group *groups[3];
> +	struct max665x_dev *iodev;

The only use of this variable is to dereference map. 'struct regmap *map'
might thus make more sense here.

Also, iodev is not initialized. The first access to iodev->map will result
in a null pointer access.

>  	struct mutex update_lock;
>  	int nr_fans;
>  	char valid; /* zero until following fields are valid */
>  	unsigned long last_updated; /* in jiffies */
>  
>  	/* register values */
> -	u8 speed;
> -	u8 config;
> -	u8 tach[4];
> -	u8 count;
> -	u8 dac;
> -	u8 alarm;
> +	int speed;
> +	int config;
> +	int tach[4];
> +	int count;
> +	int dac;
> +	int alarm;
>  };
>  
>  static const u8 tach_reg[] = {
> @@ -136,31 +140,29 @@ static const u8 tach_reg[] = {
>  static struct max6650_data *max6650_update_device(struct device *dev)
>  {
>  	struct max6650_data *data = dev_get_drvdata(dev);
> -	struct i2c_client *client = data->client;
> +	struct regmap *map = data->iodev->map;
>  	int i;
> +	int alarm;
>  
>  	mutex_lock(&data->update_lock);
>  
>  	if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
> -		data->speed = i2c_smbus_read_byte_data(client,
> -						       MAX6650_REG_SPEED);
> -		data->config = i2c_smbus_read_byte_data(client,
> -							MAX6650_REG_CONFIG);
> +		regmap_read(map, MAX6650_REG_SPEED, &data->speed);
> +		regmap_read(map, MAX6650_REG_CONFIG, &data->config);
>  		for (i = 0; i < data->nr_fans; i++) {
> -			data->tach[i] = i2c_smbus_read_byte_data(client,
> -								 tach_reg[i]);
> +			data->tach[i] = regmap_read(map, tach_reg[i],
> +							&data->tach[i]);
>  		}
> -		data->count = i2c_smbus_read_byte_data(client,
> -							MAX6650_REG_COUNT);
> -		data->dac = i2c_smbus_read_byte_data(client, MAX6650_REG_DAC);
> +		regmap_read(map, MAX6650_REG_COUNT, &data->count);
> +		regmap_read(map, MAX6650_REG_DAC, &data->dac);
>  
>  		/*
>  		 * Alarms are cleared on read in case the condition that
>  		 * caused the alarm is removed. Keep the value latched here
>  		 * for providing the register through different alarm files.
>  		 */
> -		data->alarm |= i2c_smbus_read_byte_data(client,
> -							MAX6650_REG_ALARM);
> +		regmap_read(map, MAX6650_REG_ALARM, &alarm);
> +		data->alarm |= alarm;
>  
>  		data->last_updated = jiffies;
>  		data->valid = 1;
> @@ -256,7 +258,6 @@ static ssize_t set_target(struct device *dev, struct device_attribute *devattr,
>  			 const char *buf, size_t count)
>  {
>  	struct max6650_data *data = dev_get_drvdata(dev);
> -	struct i2c_client *client = data->client;
>  	int kscale, ktach;
>  	unsigned long rpm;
>  	int err;
> @@ -284,7 +285,7 @@ static ssize_t set_target(struct device *dev, struct device_attribute *devattr,
>  		ktach = 255;
>  	data->speed = ktach;
>  
> -	i2c_smbus_write_byte_data(client, MAX6650_REG_SPEED, data->speed);
> +	regmap_write(data->iodev->map, MAX6650_REG_SPEED, data->speed);
>  
>  	mutex_unlock(&data->update_lock);
>  
> @@ -325,7 +326,6 @@ static ssize_t set_pwm(struct device *dev, struct device_attribute *devattr,
>  			const char *buf, size_t count)
>  {
>  	struct max6650_data *data = dev_get_drvdata(dev);
> -	struct i2c_client *client = data->client;
>  	unsigned long pwm;
>  	int err;
>  
> @@ -342,7 +342,7 @@ static ssize_t set_pwm(struct device *dev, struct device_attribute *devattr,
>  	else
>  		data->dac = 76 - (76 * pwm)/255;
>  
> -	i2c_smbus_write_byte_data(client, MAX6650_REG_DAC, data->dac);
> +	regmap_write(data->iodev->map, MAX6650_REG_DAC, data->dac);
>  
>  	mutex_unlock(&data->update_lock);
>  
> @@ -371,7 +371,7 @@ static ssize_t set_enable(struct device *dev, struct device_attribute *devattr,
>  			  const char *buf, size_t count)
>  {
>  	struct max6650_data *data = dev_get_drvdata(dev);
> -	struct i2c_client *client = data->client;
> +	struct regmap *map = data->iodev->map;
>  	int max6650_modes[3] = {0, 3, 2};
>  	unsigned long mode;
>  	int err;
> @@ -385,11 +385,11 @@ static ssize_t set_enable(struct device *dev, struct device_attribute *devattr,
>  
>  	mutex_lock(&data->update_lock);
>  
> -	data->config = i2c_smbus_read_byte_data(client, MAX6650_REG_CONFIG);
> +	regmap_read(map, MAX6650_REG_CONFIG, &data->config);
>  	data->config = (data->config & ~MAX6650_CFG_MODE_MASK)
>  		       | (max6650_modes[mode] << 4);
>  
> -	i2c_smbus_write_byte_data(client, MAX6650_REG_CONFIG, data->config);
> +	regmap_write(map, MAX6650_REG_CONFIG, data->config);
>  
>  	mutex_unlock(&data->update_lock);
>  
> @@ -421,7 +421,6 @@ static ssize_t set_div(struct device *dev, struct device_attribute *devattr,
>  		       const char *buf, size_t count)
>  {
>  	struct max6650_data *data = dev_get_drvdata(dev);
> -	struct i2c_client *client = data->client;
>  	unsigned long div;
>  	int err;
>  
> @@ -448,7 +447,7 @@ static ssize_t set_div(struct device *dev, struct device_attribute *devattr,
>  		return -EINVAL;
>  	}
>  
> -	i2c_smbus_write_byte_data(client, MAX6650_REG_COUNT, data->count);
> +	regmap_write(data->iodev->map, MAX6650_REG_COUNT, data->count);
>  	mutex_unlock(&data->update_lock);
>  
>  	return count;
> @@ -465,16 +464,15 @@ static ssize_t get_alarm(struct device *dev, struct device_attribute *devattr,
>  			 char *buf)
>  {
>  	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct max6650_data *data = max6650_update_device(dev);
> -	struct i2c_client *client = data->client;
> +	struct max6650_data *data = dev_get_drvdata(dev);
>  	int alarm = 0;
>  
>  	if (data->alarm & attr->index) {
>  		mutex_lock(&data->update_lock);
> -		alarm = 1;
>  		data->alarm &= ~attr->index;
> -		data->alarm |= i2c_smbus_read_byte_data(client,
> -							MAX6650_REG_ALARM);
> +		regmap_read(data->iodev->map, MAX6650_REG_ALARM, &alarm);
> +		data->alarm |= alarm;
> +		alarm = 1;
>  		mutex_unlock(&data->update_lock);
>  	}
>  
> @@ -505,10 +503,11 @@ static umode_t max6650_attrs_visible(struct kobject *kobj, struct attribute *a,
>  {
>  	struct device *dev = container_of(kobj, struct device, kobj);
>  	struct max6650_data *data = dev_get_drvdata(dev);
> -	struct i2c_client *client = data->client;
> -	u8 alarm_en = i2c_smbus_read_byte_data(client, MAX6650_REG_ALARM_EN);
> +	int alarm_en;
>  	struct device_attribute *devattr;
>  
> +	regmap_read(data->iodev->map, MAX6650_REG_ALARM_EN, &alarm_en);
> +

There is now a difference in handline alarm_en and in general
return values from i2c_smbus_read_byte_data vs. regmap_read.
alarm_en is no longer initialized if regmap_read() returns an error.
It will therefore be necessary to check the return values or pre-initialize
the variable. The same applies to all other places where the reeturn value of
regmap_read is not checked.

>  	/*
>  	 * Hide the alarms that have not been enabled by the firmware
>  	 */
> @@ -560,17 +559,17 @@ static const struct attribute_group max6651_group = {
>   * Real code
>   */
>  
> -static int max6650_init_client(struct max6650_data *data,
> -			       struct i2c_client *client)
> +static int max6650_init_client(struct platform_device *pdev)
>  {
> -	struct device *dev = &client->dev;

I had a reason for introducing this variable here and in the probe function.
Please don't remove it.

> +	struct max6650_data *data = platform_get_drvdata(pdev);
> +	struct regmap *map = data->iodev->map;
>  	int config;
>  	int err = -EIO;
>  
> -	config = i2c_smbus_read_byte_data(client, MAX6650_REG_CONFIG);
> +	regmap_read(map, MAX6650_REG_CONFIG, &config);
>  
>  	if (config < 0) {
> -		dev_err(dev, "Error reading config, aborting.\n");
> +		dev_err(&pdev->dev, "Error reading config, aborting.\n");
>  		return err;
>  	}
>  
> @@ -584,11 +583,11 @@ static int max6650_init_client(struct max6650_data *data,
>  		config |= MAX6650_CFG_V12;
>  		break;
>  	default:
> -		dev_err(dev, "illegal value for fan_voltage (%d)\n",
> +		dev_err(&pdev->dev, "illegal value for fan_voltage (%d)\n",
>  			fan_voltage);
>  	}
>  
> -	dev_info(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) {
> @@ -614,10 +613,11 @@ static int max6650_init_client(struct max6650_data *data,
>  			 | MAX6650_CFG_PRESCALER_16;
>  		break;
>  	default:
> -		dev_err(dev, "illegal value for prescaler (%d)\n", prescaler);
> +		dev_err(&pdev->dev, "illegal value for prescaler (%d)\n",
> +			prescaler);
>  	}
>  
> -	dev_info(dev, "Prescaler is set to %d.\n",
> +	dev_info(&pdev->dev, "Prescaler is set to %d.\n",
>  		 1 << (config & MAX6650_CFG_PRESCALER_MASK));
>  
>  	/*
> @@ -627,46 +627,45 @@ static int max6650_init_client(struct max6650_data *data,
>  	 */
>  
>  	if ((config & MAX6650_CFG_MODE_MASK) == MAX6650_CFG_MODE_OFF) {
> -		dev_dbg(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(dev, "DAC write error, aborting.\n");
> +		if (regmap_write(data->iodev->map, MAX6650_REG_DAC, 255) < 0) {
> +			dev_err(&pdev->dev, "DAC write error, aborting.\n");
>  			return err;
>  		}
>  	}
>  
> -	if (i2c_smbus_write_byte_data(client, MAX6650_REG_CONFIG, config)) {
> -		dev_err(dev, "Config write error, aborting.\n");
> +	if (regmap_write(map, MAX6650_REG_CONFIG, config) < 0) {
> +		dev_err(&pdev->dev, "Config write error, aborting.\n");
>  		return err;
>  	}
>  
>  	data->config = config;
> -	data->count = i2c_smbus_read_byte_data(client, MAX6650_REG_COUNT);
> +	regmap_read(map, MAX6650_REG_COUNT, &data->count);
>  
>  	return 0;
>  }
>  
> -static int max6650_probe(struct i2c_client *client,
> -			 const struct i2c_device_id *id)
> +static int max6650_probe(struct platform_device *pdev)
>  {
> -	struct device *dev = &client->dev;
> -	struct max6650_data *data;
> +	struct max6650_data *data = platform_get_drvdata(pdev);

driver data points to struct max665x_dev *.

This should probably be something like
	struct max665x_dev *iodev = platform_get_drvdata(pdev);
as a separate variable. It also needs to be validated to ensure
that it is not NULL.

> +	const struct platform_device_id *id = platform_get_device_id(pdev);
>  	struct device *hwmon_dev;
>  	int err;
>  
> -	data = devm_kzalloc(dev, sizeof(struct max6650_data), GFP_KERNEL);
> +	data = devm_kzalloc(&pdev->dev, sizeof(struct max6650_data),
> +							GFP_KERNEL);
>  	if (!data)
>  		return -ENOMEM;
>  
> -	data->client = client;

	data->iodev = iodev;
or better
	data->map = iodev->map;

is missing.

>  	mutex_init(&data->update_lock);
>  	data->nr_fans = id->driver_data;
>  
>  	/*
>  	 * Initialize the max6650 chip
>  	 */
> -	err = max6650_init_client(data, client);
> +	err = max6650_init_client(pdev);
>  	if (err)
>  		return err;
>  
> @@ -675,20 +674,20 @@ static int max6650_probe(struct i2c_client *client,
>  	if (data->nr_fans == 4)
>  		data->groups[1] = &max6651_group;
>  
> -	hwmon_dev = devm_hwmon_device_register_with_groups(dev,
> -							   client->name, data,
> -							   data->groups);
> +	hwmon_dev = devm_hwmon_device_register_with_groups(&pdev->dev,
> +							   data->client->name,

This will crash (client is NULL).

You'll have to come up with a scheme to pass the device type from the mfd driver
to this one, and use "max6650" and "max6651" as names here.

> +							   data, data->groups);
>  	return PTR_ERR_OR_ZERO(hwmon_dev);
>  }
>  
> -static const struct i2c_device_id max6650_id[] = {
> +static const struct platform_device_id max6650_id[] = {
>  	{ "max6650", 1 },
>  	{ "max6651", 4 },
>  	{ }
>  };
> -MODULE_DEVICE_TABLE(i2c, max6650_id);
> +MODULE_DEVICE_TABLE(platform, max6650_id);
>  
> -static struct i2c_driver max6650_driver = {
> +static struct platform_driver max6650_driver = {
>  	.driver = {
>  		.name	= "max6650",
>  	},
> @@ -696,7 +695,7 @@ static struct i2c_driver max6650_driver = {
>  	.id_table	= max6650_id,
>  };
>  
> -module_i2c_driver(max6650_driver);
> +module_platform_driver(max6650_driver);
>  
>  MODULE_AUTHOR("Hans J. Koch");
>  MODULE_DESCRIPTION("MAX6650 sensor driver");
> -- 
> 1.8.5.4
> 
>