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
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 <[email protected]>
---
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 <[email protected]>
+ *
+ * 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 <[email protected]>");
+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 <[email protected]>
+ *
+ * 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
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 <[email protected]>
---
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
>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 <[email protected]>
> ---
> 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 <[email protected]>");
> +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
> 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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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 <[email protected]>
> + *
> + * 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 <[email protected]>");
> +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 <[email protected]>
> + *
> + * 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
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 <[email protected]>
> ---
> 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
>
>