* [RFC PATCH 0/2] Redesign the MAX6650-6651 driver to be more flexible @ 2014-02-14 9:15 ` Laszlo Papp 0 siblings, 0 replies; 14+ messages in thread From: Laszlo Papp @ 2014-02-14 9:15 UTC (permalink / raw) To: jdelvare, linux; +Cc: lm-sensors, lee.jones, lpapp, linux-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [lm-sensors] [RFC PATCH 0/2] Redesign the MAX6650-6651 driver to be more flexible @ 2014-02-14 9:15 ` Laszlo Papp 0 siblings, 0 replies; 14+ messages in thread From: Laszlo Papp @ 2014-02-14 9:15 UTC (permalink / raw) To: jdelvare, linux; +Cc: lm-sensors, lee.jones, lpapp, linux-kernel 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 _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] mfd: MAX6650/6651 support 2014-02-14 9:15 ` [lm-sensors] " Laszlo Papp @ 2014-02-14 9:15 ` Laszlo Papp -1 siblings, 0 replies; 14+ messages in thread From: Laszlo Papp @ 2014-02-14 9:15 UTC (permalink / raw) To: jdelvare, linux; +Cc: lm-sensors, lee.jones, lpapp, linux-kernel 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [lm-sensors] [PATCH 1/2] mfd: MAX6650/6651 support @ 2014-02-14 9:15 ` Laszlo Papp 0 siblings, 0 replies; 14+ messages in thread From: Laszlo Papp @ 2014-02-14 9:15 UTC (permalink / raw) To: jdelvare, linux; +Cc: lm-sensors, lee.jones, lpapp, linux-kernel 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 _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] mfd: MAX6650/6651 support 2014-02-14 9:15 ` [lm-sensors] " Laszlo Papp @ 2014-02-14 11:43 ` Lee Jones -1 siblings, 0 replies; 14+ messages in thread From: Lee Jones @ 2014-02-14 11:43 UTC (permalink / raw) To: Laszlo Papp; +Cc: jdelvare, linux, lm-sensors, linux-kernel >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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] mfd: MAX6650/6651 support @ 2014-02-14 11:43 ` Lee Jones 0 siblings, 0 replies; 14+ messages in thread From: Lee Jones @ 2014-02-14 11:43 UTC (permalink / raw) To: Laszlo Papp; +Cc: jdelvare, linux, lm-sensors, linux-kernel RnJvbSBbUEFUQ0ggMC8yXToKICBXaGF0J3MgdGhlIHJlc2FvbiBmb3Igbm90IHRlc3RpbmcgdGhp cyBvbiBoL3cgeWV0PwoKPiBNQVg2NjUwL01BWDY2NTEgY2hpcCBpcyBhIG11bHRpLWZ1bmN0aW9u IGRldmljZSB3aXRoIEkyQyBidXNzZXMuIFRoZQo+IGNoaXAgaW5jbHVkZXMgZmFuLXNwZWVkIHJl Z3VsYXRvcnMgYW5kIG1vbml0b3JzLCBHUElPLCBhbmQgYWxhcm0uCj4gCj4gVGhpcyBwYXRjaCBp cyBhbiBpbml0aWFsIHJlbGVhc2Ugb2YgYSBNQVg2NjUwLzY2NTEgTUZEIGRyaXZlciB0aGF0Cj4g c3VwcG9ydHMgdG8gZW5hYmxlIHRoZSBjaGlwIHdpdGggaXRzIHByaW1hcnkgSTJDIGJ1cyB0aGF0 IHdpbGwgY29ubmVjdAo+IHRoZSBod21vbiwgYW5kIHRoZW4gdGhlIGdwaW8gZGV2aWNlcyBmb3Ig bm93Lgo+IAo+IFNpZ25lZC1vZmYtYnk6IExhc3psbyBQYXBwIDxscGFwcEBrZGUub3JnPgo+IC0t LQo+ICBkcml2ZXJzL21mZC9LY29uZmlnICAgICAgICAgICAgICAgICB8IDExICsrKysrCj4gIGRy aXZlcnMvbWZkL01ha2VmaWxlICAgICAgICAgICAgICAgIHwgIDEgKwo+ICBkcml2ZXJzL21mZC9t YXg2NjV4LmMgICAgICAgICAgICAgICB8IDk0ICsrKysrKysrKysrKysrKysrKysrKysrKysrKysr KysrKysrKysKPiAgaW5jbHVkZS9saW51eC9tZmQvbWF4NjY1eC1wcml2YXRlLmggfCAzNCArKysr KysrKysrKysrKwo+ICA0IGZpbGVzIGNoYW5nZWQsIDE0MCBpbnNlcnRpb25zKCspCj4gIGNyZWF0 ZSBtb2RlIDEwMDY0NCBkcml2ZXJzL21mZC9tYXg2NjV4LmMKPiAgY3JlYXRlIG1vZGUgMTAwNjQ0 IGluY2x1ZGUvbGludXgvbWZkL21heDY2NXgtcHJpdmF0ZS5oCgo8c25pcD4KCj4gKysrIGIvZHJp dmVycy9tZmQvbWF4NjY1eC5jCj4gQEAgLTAsMCArMSw5NCBAQAo+ICsvKgo+ICsgKiBEZXZpY2Ug YWNjZXNzIGZvciBNQVg2NjUwLU1BWDY2NTEKCk5pdDogUHJlZmVyICJmb3IgTWF4aW0gTUFYNjY1 MCBhbmQgTUFYNjY1MSIgKyA8ZGV2aWNlIHR5cGU+Cgo8c25pcD4KCj4gKyNpbmNsdWRlIDxsaW51 eC9kZXZpY2UuaD4KPiArI2luY2x1ZGUgPGxpbnV4L21mZC9jb3JlLmg+Cj4gKyNpbmNsdWRlIDxs aW51eC9tb2R1bGUuaD4KCkkgdGhvdWdodCB0aGlzIGRyaXZlciB3YXMgYm9vbCBpLmUuIFkgb3Ig Tiwgbm90IHRyaXN0YXRlPwoKPHNuaXA+Cgo+ICtzdGF0aWMgc3RydWN0IG1mZF9jZWxsIG1heDY2 NXhfZGV2c1tdID0gewo+ICsJeyAubmFtZSA9ICJtYXg2NjUxLWdwaW8iLCB9LAo+ICsJeyAubmFt ZSA9ICJtYXg2NjUwIiwgfSwgLyogaHdtb24gZHJpdmVyICovCgpXZSByZWFsbHkgbmVlZCB0byBk byBzb21ldGhpbmcgYWJvdXQgdGhpcy4KCkd1ZW50ZXIsCiAgb25jZSB3ZSd2ZSBjb252ZXJ0ZWQg dGhlIGh3bW9uIHBhcnQgdG8gYSBwbGF0Zm9ybSBkZXZpY2UsIGNhbiB3ZQogIHRoZW4gc2FmZWx5 IHJlbmFtZSBpdCB3aXRoICItaHdtb24iIGFwcGVuZGVkPwoKPiArCXt9LAo+ICt9Owo+ICsKPiAr c3RhdGljIGNvbnN0IHN0cnVjdCByZWdtYXBfY29uZmlnIG1heDY2NXhfcmVnbWFwX2NvbmZpZyA9 IHsKPiArCS5yZWdfYml0cyA9IDgsCj4gKwkudmFsX2JpdHMgPSA4LAoKVGhpcyBsb29rcyBtb3Jl IHNhbmUsIGJ1dCBpdCByZWFsbHkgc2hvdWxkIGJlIHRlc3RlZC4KCj4gK3N0YXRpYyBpbnQgbWF4 NjY1eF9wcm9iZShzdHJ1Y3QgaTJjX2NsaWVudCAqaTJjLAo+ICsJCQkgICAgY29uc3Qgc3RydWN0 IGkyY19kZXZpY2VfaWQgKmlkKQo+ICt7Cj4gKwlzdHJ1Y3QgbWF4NjY1eF9kZXYgKm1heDY2NXg7 Cj4gKwlpbnQgcmV0Owo+ICsKPiArCW1heDY2NXggPSBkZXZtX2t6YWxsb2MoJmkyYy0+ZGV2LCBz aXplb2YoKm1heDY2NXgpLCBHRlBfS0VSTkVMKTsKPiArCWlmICghbWF4NjY1eCkKPiArCQlyZXR1 cm4gLUVOT01FTTsKPiArCj4gKwlpMmNfc2V0X2NsaWVudGRhdGEoaTJjLCBtYXg2NjV4KTsKPiAr CW1heDY2NXgtPmRldiA9ICZpMmMtPmRldjsKPiArCW1heDY2NXgtPmkyYyA9IGkyYzsKPiArCW1h eDY2NXgtPm1hcCA9IGRldm1fcmVnbWFwX2luaXRfaTJjKGkyYywgJm1heDY2NXhfcmVnbWFwX2Nv bmZpZyk7Cj4gKwlpZiAoSVNfRVJSKG1heDY2NXgtPm1hcCkpIHsKPiArCQlyZXQgPSBQVFJfRVJS KG1heDY2NXgtPm1hcCk7Cj4gKwkJZGV2X2VycihtYXg2NjV4LT5kZXYsICJGYWlsZWQgdG8gYWxs b2NhdGUgcmVnaXN0ZXIgbWFwOiAlZFxuIiwKPiArCQkJcmV0KTsKPiArCQlyZXR1cm4gcmV0Owo+ ICsJfQo+ICsKPiArCW11dGV4X2luaXQoJm1heDY2NXgtPmlvbG9jayk7CgoqY291Z2gqCgo+ICsJ cmV0ID0gbWZkX2FkZF9kZXZpY2VzKG1heDY2NXgtPmRldiwgLTEsIG1heDY2NXhfZGV2cywKPiAr CQkJQVJSQVlfU0laRShtYXg2NjV4X2RldnMpLAo+ICsJCQlOVUxMLCAwLCBOVUxMKTsKPiArCj4g KwlpZiAocmV0IDwgMCkKCkp1c3QgJ2lmIChyZXQpJwoKPHNuaXA+Cgo+ICtzdGF0aWMgc3RydWN0 IG9mX2RldmljZV9pZCBtYXg2NjV4X2R0X21hdGNoW10gPSB7Cj4gKwl7IC5jb21wYXRpYmxlID0g Im1heGltLG1heDY2NXgiIH0sCgogIAl7IC5jb21wYXRpYmxlID0gIm1heGltLG1heDY2NTAiIH0s CiAgCXsgLmNvbXBhdGlibGUgPSAibWF4aW0sbWF4NjY1MSIgfSwKCj4gKwl7fSwKPiArfTsKPiAr TU9EVUxFX0RFVklDRV9UQUJMRShvZiwgbWF4NjY1eF9kdF9tYXRjaCk7CgpNb2R1bGU/Cgo+ICtz dGF0aWMgc3RydWN0IGkyY19kcml2ZXIgbWF4NjY1eF9kcml2ZXIgPSB7Cj4gKwkuZHJpdmVyID0g ewo+ICsJCS5uYW1lID0gIm1heDY2NXgiLAo+ICsJCS5vd25lciA9IFRISVNfTU9EVUxFLAo+ICsJ CS5vZl9tYXRjaF90YWJsZSA9IG1heDY2NXhfZHRfbWF0Y2gsCgpvZl9tYXRjaF9wdHIoKQoKPiAr CX0sCj4gKwkucHJvYmUgPSBtYXg2NjV4X3Byb2JlLAo+ICsJLnJlbW92ZSA9IG1heDY2NXhfcmVt b3ZlLAo+ICt9Owo+ICsKPiArbW9kdWxlX2kyY19kcml2ZXIobWF4NjY1eF9kcml2ZXIpOwoKTW9k dWxlPwoKPiArTU9EVUxFX0FVVEhPUigiTGFzemxvIFBhcHAgPGxhc3psby5wYXBwQHBvbGF0aXMu Y29tPiIpOwo+ICtNT0RVTEVfREVTQ1JJUFRJT04oIk1BWDY2NTAtTUFYNjY1MSBNRkQiKTsKCnMv LS8gYW5kIC8KCj4gK01PRFVMRV9MSUNFTlNFKCJHUEwiKTsKCnYxIG9yIHYyPwoKPHNuaXA+Cgo+ ICsjaWZuZGVmIF9fTElOVVhfTUZEX01BWDY2NVhfUFJJVkFURV9ICj4gKyNkZWZpbmUgX19MSU5V WF9NRkRfTUFYNjY1WF9QUklWQVRFX0gKPiArCj4gKyNpbmNsdWRlIDxsaW51eC9pMmMuaD4KPiAr I2luY2x1ZGUgPGxpbnV4L3JlZ21hcC5oPgo+ICsKPiArc3RydWN0IG1heDY2NXhfZGV2IHsKPiAr CXN0cnVjdCBkZXZpY2UgKmRldjsKPiArCXN0cnVjdCBtdXRleCBpb2xvY2s7CgoqQWggaGVtKgoK PiArCXN0cnVjdCBpMmNfY2xpZW50ICppMmM7Cj4gKwlzdHJ1Y3QgcmVnbWFwICAgICAqbWFwOwo+ ICsJaW50IHR5cGU7CgpXaGF0J3MgdGhpcyB1c2VkIGZvcj8KCj4gK307Cgo+ICsjZW5kaWYKCi0t IApMZWUgSm9uZXMKTGluYXJvIFNUTWljcm9lbGVjdHJvbmljcyBMYW5kaW5nIFRlYW0gTGVhZApM aW5hcm8ub3JnIOKUgiBPcGVuIHNvdXJjZSBzb2Z0d2FyZSBmb3IgQVJNIFNvQ3MKRm9sbG93IExp bmFybzogRmFjZWJvb2sgfCBUd2l0dGVyIHwgQmxvZwoKX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX18KbG0tc2Vuc29ycyBtYWlsaW5nIGxpc3QKbG0tc2Vuc29y c0BsbS1zZW5zb3JzLm9yZwpodHRwOi8vbGlzdHMubG0tc2Vuc29ycy5vcmcvbWFpbG1hbi9saXN0 aW5mby9sbS1zZW5zb3Jz ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] mfd: MAX6650/6651 support 2014-02-14 9:15 ` [lm-sensors] " Laszlo Papp @ 2014-02-14 17:24 ` Guenter Roeck -1 siblings, 0 replies; 14+ messages in thread From: Guenter Roeck @ 2014-02-14 17:24 UTC (permalink / raw) To: Laszlo Papp; +Cc: jdelvare, lm-sensors, lee.jones, linux-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] mfd: MAX6650/6651 support @ 2014-02-14 17:24 ` Guenter Roeck 0 siblings, 0 replies; 14+ messages in thread From: Guenter Roeck @ 2014-02-14 17:24 UTC (permalink / raw) To: Laszlo Papp; +Cc: jdelvare, lm-sensors, lee.jones, linux-kernel 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 _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] hwmon: (max6650) Convert to be a platform driver 2014-02-14 9:15 ` [lm-sensors] " Laszlo Papp @ 2014-02-14 9:15 ` Laszlo Papp -1 siblings, 0 replies; 14+ messages in thread From: Laszlo Papp @ 2014-02-14 9:15 UTC (permalink / raw) To: jdelvare, linux; +Cc: lm-sensors, lee.jones, lpapp, linux-kernel 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [lm-sensors] [PATCH 2/2] hwmon: (max6650) Convert to be a platform driver @ 2014-02-14 9:15 ` Laszlo Papp 0 siblings, 0 replies; 14+ messages in thread From: Laszlo Papp @ 2014-02-14 9:15 UTC (permalink / raw) To: jdelvare, linux; +Cc: lm-sensors, lee.jones, lpapp, linux-kernel 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 _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] hwmon: (max6650) Convert to be a platform driver 2014-02-14 9:15 ` [lm-sensors] " Laszlo Papp @ 2014-02-14 12:00 ` Lee Jones -1 siblings, 0 replies; 14+ messages in thread From: Lee Jones @ 2014-02-14 12:00 UTC (permalink / raw) To: Laszlo Papp; +Cc: jdelvare, linux, lm-sensors, linux-kernel > 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [lm-sensors] [PATCH 2/2] hwmon: (max6650) Convert to be a platform driver @ 2014-02-14 12:00 ` Lee Jones 0 siblings, 0 replies; 14+ messages in thread From: Lee Jones @ 2014-02-14 12:00 UTC (permalink / raw) To: Laszlo Papp; +Cc: jdelvare, linux, lm-sensors, linux-kernel PiBUaGUgTUZEIGRyaXZlciBoYXMgbm93IGJlZW4gYWRkZWQsIHNvIHRoaXMgZHJpdmVyIGlzIG5v dyBiZWluZyBhZG9wdGVkIHRvIGJlIGEKPiBzdWJkZXZpY2UgZHJpdmVyIG9uIHRvcCBvZiBpdC4g VGhpcyBtZWFucywgdGhlIGkyYyBkcml2ZXIgdXNhZ2UgaXMgYmVpbmcKPiBjb252ZXJ0ZWQgdG8g cGxhdGZvcm0gZHJpdmVyIHVzYWdlIGFsbCBhcm91bmQuCj4gCj4gU2lnbmVkLW9mZi1ieTogTGFz emxvIFBhcHAgPGxwYXBwQGtkZS5vcmc+Cj4gLS0tCj4gIGRyaXZlcnMvaHdtb24vS2NvbmZpZyAg IHwgICAyICstCj4gIGRyaXZlcnMvaHdtb24vbWF4NjY1MC5jIHwgMTI1ICsrKysrKysrKysrKysr KysrKysrKysrKy0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQo+ICAyIGZpbGVzIGNoYW5nZWQsIDYz IGluc2VydGlvbnMoKyksIDY0IGRlbGV0aW9ucygtKQo+IAo8c25pcD4KCj4gQEAgLTM5LDYgKzM5 LDkgQEAKPiAgI2luY2x1ZGUgPGxpbnV4L2h3bW9uLmg+Cj4gICNpbmNsdWRlIDxsaW51eC9od21v bi1zeXNmcy5oPgo+ICAjaW5jbHVkZSA8bGludXgvZXJyLmg+Cj4gKyNpbmNsdWRlIDxsaW51eC9w bGF0Zm9ybV9kZXZpY2UuaD4KPiArCgpOb3QgcmVhbGx5IGFueSBuZWVkIGZvciB0aGlzLgoKPiAr I2luY2x1ZGUgPGxpbnV4L21mZC9tYXg2NjV4LXByaXZhdGUuaD4KCjxzbmlwPgoKPiAgc3RydWN0 IG1heDY2NTBfZGF0YSB7Cj4gIAlzdHJ1Y3QgaTJjX2NsaWVudCAqY2xpZW50Owo+ICAJY29uc3Qg c3RydWN0IGF0dHJpYnV0ZV9ncm91cCAqZ3JvdXBzWzNdOwo+ICsJc3RydWN0IG1heDY2NXhfZGV2 ICppb2RldjsKCkkgZmluZCB0aGUgbmFtZSBpb2RldiBhIGJpdCBjb25mdXNpbmcsIHdoeSBub3Qg bWF4NjY1eF9kZXY/Cgo8c25pcD4KCj4gLQkJZGF0YS0+c3BlZWQgPSBpMmNfc21idXNfcmVhZF9i eXRlX2RhdGEoY2xpZW50LAo+IC0JCQkJCQkgICAgICAgTUFYNjY1MF9SRUdfU1BFRUQpOwo+IC0J CWRhdGEtPmNvbmZpZyA9IGkyY19zbWJ1c19yZWFkX2J5dGVfZGF0YShjbGllbnQsCj4gLQkJCQkJ CQlNQVg2NjUwX1JFR19DT05GSUcpOwo+ICsJCXJlZ21hcF9yZWFkKG1hcCwgTUFYNjY1MF9SRUdf U1BFRUQsICZkYXRhLT5zcGVlZCk7Cj4gKwkJcmVnbWFwX3JlYWQobWFwLCBNQVg2NjUwX1JFR19D T05GSUcsICZkYXRhLT5jb25maWcpOwoKSSdkIGxpa2UgTWFyayB0byBsb29rIG92ZXIgeW91ciBS ZWdtYXAgaW1wbGVtZW50YXRpb24gaWYgcG9zc2libGUuCgo+ICAJaWYgKGNvbmZpZyA8IDApIHsK PiAtCQlkZXZfZXJyKGRldiwgIkVycm9yIHJlYWRpbmcgY29uZmlnLCBhYm9ydGluZy5cbiIpOwo+ ICsJCWRldl9lcnIoJnBkZXYtPmRldiwgIkVycm9yIHJlYWRpbmcgY29uZmlnLCBhYm9ydGluZy5c biIpOwoKUmF0aGVyIHRoYW4gbWFrZSBhbGwgdGhlc2UgY2hhbmdlcywganVzdCBkbzoKCiAgc3Ry dWN0IGRldmljZSAqZGV2ID0gJnBkZXYtPmRldjsKCi4uLiBhdCB0aGUgdG9wLgoKPHNuaXA+Cgo+ ICtzdGF0aWMgaW50IG1heDY2NTBfcHJvYmUoc3RydWN0IHBsYXRmb3JtX2RldmljZSAqcGRldikK PiAgewo+IC0Jc3RydWN0IGRldmljZSAqZGV2ID0gJmNsaWVudC0+ZGV2Owo+IC0Jc3RydWN0IG1h eDY2NTBfZGF0YSAqZGF0YTsKPiArCXN0cnVjdCBtYXg2NjUwX2RhdGEgKmRhdGEgPSBwbGF0Zm9y bV9nZXRfZHJ2ZGF0YShwZGV2KTsKCkRvbid0IGRvIHRoaXMgaGVyZSwgaXQncyBtZXNzeS4gRGVj bGFyZSBpdCBoZXJlIGFuZCBpbml0aWFsaXNlIGl0IGxhdGVyLgoKPiArCWNvbnN0IHN0cnVjdCBw bGF0Zm9ybV9kZXZpY2VfaWQgKmlkID0gcGxhdGZvcm1fZ2V0X2RldmljZV9pZChwZGV2KTsKClNh bWUgaGVyZS4KCj4gIAlzdHJ1Y3QgZGV2aWNlICpod21vbl9kZXY7Cj4gIAlpbnQgZXJyOwo+ICAK PiAtCWRhdGEgPSBkZXZtX2t6YWxsb2MoZGV2LCBzaXplb2Yoc3RydWN0IG1heDY2NTBfZGF0YSks IEdGUF9LRVJORUwpOwo+ICsJZGF0YSA9IGRldm1fa3phbGxvYygmcGRldi0+ZGV2LCBzaXplb2Yo c3RydWN0IG1heDY2NTBfZGF0YSksCj4gKwkJCQkJCQlHRlBfS0VSTkVMKTsKCkVoPyBkaWRuJ3Qg eW91IGFscmVhZHkgaW5pdGlhbGlzZSAnZGF0YSc/Cgo8c25pcD4KCj4gLQlod21vbl9kZXYgPSBk ZXZtX2h3bW9uX2RldmljZV9yZWdpc3Rlcl93aXRoX2dyb3VwcyhkZXYsCj4gLQkJCQkJCQkgICBj bGllbnQtPm5hbWUsIGRhdGEsCj4gLQkJCQkJCQkgICBkYXRhLT5ncm91cHMpOwo+ICsJaHdtb25f ZGV2ID0gZGV2bV9od21vbl9kZXZpY2VfcmVnaXN0ZXJfd2l0aF9ncm91cHMoJnBkZXYtPmRldiwK PiArCQkJCQkJCSAgIGRhdGEtPmNsaWVudC0+bmFtZSwKPiArCQkJCQkJCSAgIGRhdGEsIGRhdGEt Pmdyb3Vwcyk7CgpMZWF2ZSBpdCBhcyAnZGV2Jy4KCj4gLXN0YXRpYyBjb25zdCBzdHJ1Y3QgaTJj X2RldmljZV9pZCBtYXg2NjUwX2lkW10gPSB7Cj4gK3N0YXRpYyBjb25zdCBzdHJ1Y3QgcGxhdGZv cm1fZGV2aWNlX2lkIG1heDY2NTBfaWRbXSA9IHsKPiAgCXsgIm1heDY2NTAiLCAxIH0sCj4gIAl7 ICJtYXg2NjUxIiwgNCB9LAoKSSdtIHN0aWxsIHByZXR0eSBrZWVuIHRvIGhhdmUgdGhlc2UgbWFn aWMgbnVtYmVycyAjZGVmaW5lZC4KCk5vdCB5ZXQgdGhvdWdoLCBsZXQncyBnZXQgdGhpcyBzb3J0 ZWQgZmlyc3QuCgo+ICtzdGF0aWMgc3RydWN0IHBsYXRmb3JtX2RyaXZlciBtYXg2NjUwX2RyaXZl ciA9IHsKPiAgCS5kcml2ZXIgPSB7Cj4gIAkJLm5hbWUJPSAibWF4NjY1MCIsCgpHdWVudGVyLCBK ZWFuLAogIENhbiB0aGlzIGJlIGNoYW5nZWQgbm93IGl0J3MgYSBwbGF0Zm9ybSBkZXZpY2U/CgpM YXN6aW8sCiAgTmV4dCB0aGluZyB0byBkbyBpcyB0byBnZXQgdGhpcyB3b3JraW5nIGFuZCBydW50 aW1lIHRlc3QgaXQuIE5vIG1vcmUKICBtaWQtdGVybSByZXZpZXdzIHVudGlsIGl0J3MgZnVsbHkg ZnVuY3Rpb25hbC4KCi0tIApMZWUgSm9uZXMKTGluYXJvIFNUTWljcm9lbGVjdHJvbmljcyBMYW5k aW5nIFRlYW0gTGVhZApMaW5hcm8ub3JnIOKUgiBPcGVuIHNvdXJjZSBzb2Z0d2FyZSBmb3IgQVJN IFNvQ3MKRm9sbG93IExpbmFybzogRmFjZWJvb2sgfCBUd2l0dGVyIHwgQmxvZwoKX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KbG0tc2Vuc29ycyBtYWlsaW5n IGxpc3QKbG0tc2Vuc29yc0BsbS1zZW5zb3JzLm9yZwpodHRwOi8vbGlzdHMubG0tc2Vuc29ycy5v cmcvbWFpbG1hbi9saXN0aW5mby9sbS1zZW5zb3Jz ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] hwmon: (max6650) Convert to be a platform driver 2014-02-14 9:15 ` [lm-sensors] " Laszlo Papp @ 2014-02-14 17:45 ` Guenter Roeck -1 siblings, 0 replies; 14+ messages in thread From: Guenter Roeck @ 2014-02-14 17:45 UTC (permalink / raw) To: Laszlo Papp; +Cc: jdelvare, lm-sensors, lee.jones, linux-kernel 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 > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [lm-sensors] [PATCH 2/2] hwmon: (max6650) Convert to be a platform driver @ 2014-02-14 17:45 ` Guenter Roeck 0 siblings, 0 replies; 14+ messages in thread From: Guenter Roeck @ 2014-02-14 17:45 UTC (permalink / raw) To: Laszlo Papp; +Cc: jdelvare, lm-sensors, lee.jones, linux-kernel 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 > > _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2014-02-14 17:45 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-02-14 9:15 [RFC PATCH 0/2] Redesign the MAX6650-6651 driver to be more flexible Laszlo Papp 2014-02-14 9:15 ` [lm-sensors] " Laszlo Papp 2014-02-14 9:15 ` [PATCH 1/2] mfd: MAX6650/6651 support Laszlo Papp 2014-02-14 9:15 ` [lm-sensors] " Laszlo Papp 2014-02-14 11:43 ` Lee Jones 2014-02-14 11:43 ` [lm-sensors] " Lee Jones 2014-02-14 17:24 ` Guenter Roeck 2014-02-14 17:24 ` [lm-sensors] " Guenter Roeck 2014-02-14 9:15 ` [PATCH 2/2] hwmon: (max6650) Convert to be a platform driver Laszlo Papp 2014-02-14 9:15 ` [lm-sensors] " Laszlo Papp 2014-02-14 12:00 ` Lee Jones 2014-02-14 12:00 ` [lm-sensors] " Lee Jones 2014-02-14 17:45 ` Guenter Roeck 2014-02-14 17:45 ` [lm-sensors] " Guenter Roeck
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.