MAX6650/MAX6651 chip is a multi-function device with I2C busses. The
chip includes fan-speed regulators and monitors, GPIO, and alarm.
This patch is an initial release of a MAX6650/6651 MFD driver that
supports to enable the chip with its primary I2C bus that will connect
the hwmon, and then the gpio devices for now.
Signed-off-by: Laszlo Papp <[email protected]>
---
drivers/mfd/Kconfig | 11 +++++
drivers/mfd/Makefile | 1 +
drivers/mfd/max665x.c | 87 +++++++++++++++++++++++++++++++++++++
include/linux/mfd/max665x-private.h | 34 +++++++++++++++
4 files changed, 133 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..bf4f595
--- /dev/null
+++ b/drivers/mfd/max665x.c
@@ -0,0 +1,87 @@
+/*
+ * Device access for MAX6650-MAX6651
+ *
+ * Copyright(c) 2013 Polatis Ltd.
+ *
+ * Author: Laszlo Papp <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ */
+
+#include <linux/device.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/i2c.h>
+
+#include <linux/mfd/max665x-private.h>
+
+static struct mfd_cell max665x_devs[] = {
+ { .name = "max6651-gpio", },
+ { .name = "max6650", }, /* hwmon driver */
+ {},
+};
+
+const struct regmap_config max665x_regmap_config = {
+ .reg_bits = 5,
+};
+
+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);
+
+ mutex_init(&max665x->iolock);
+
+ ret = mfd_add_devices(max665x->dev, -1, max665x_devs,
+ ARRAY_SIZE(max665x_devs),
+ NULL, 0, NULL);
+
+ if (ret < 0)
+ dev_err(max665x->dev, "failed to register child devices\n");
+
+ return ret;
+}
+
+static int max665x_remove(struct i2c_client *i2c)
+{
+ struct max665x_dev *max665x = i2c_get_clientdata(i2c);
+
+ mfd_remove_devices(max665x->dev);
+
+ return 0;
+}
+
+static struct of_device_id max665x_dt_match[] = {
+ { .compatible = "maxim,max665x" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, max665x_dt_match);
+
+static struct i2c_driver max665x_driver = {
+ .driver = {
+ .name = "max665x",
+ .owner = THIS_MODULE,
+ .of_match_table = max665x_dt_match,
+ },
+ .probe = max665x_probe,
+ .remove = max665x_remove,
+};
+
+module_i2c_driver(max665x_driver);
+
+MODULE_AUTHOR("Laszlo Papp <[email protected]>");
+MODULE_DESCRIPTION("MAX6650-MAX6651 MFD");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/max665x-private.h b/include/linux/mfd/max665x-private.h
new file mode 100644
index 0000000..293db4b
--- /dev/null
+++ b/include/linux/mfd/max665x-private.h
@@ -0,0 +1,34 @@
+/*
+ * max665x-private.h - Driver for the Maxim 6650/6651
+ *
+ * Copyright 2013 Polatis Ltd.
+ *
+ * Author: Laszlo Papp <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#ifndef __LINUX_MFD_MAX665X_PRIVATE_H
+#define __LINUX_MFD_MAX665X_PRIVATE_H
+
+#include <linux/i2c.h>
+#include <linux/regmap.h>
+
+struct max665x_dev {
+ struct device *dev;
+ struct mutex iolock;
+ struct i2c_client *i2c;
+ struct regmap *map;
+ int type;
+};
+
+#endif
--
1.8.5.4
Hi Laszlo,
Sorry for missing out on a couple of points during my earlier review.
Please see inline.
On 12 February 2014 09:32, Laszlo Papp <[email protected]> wrote:
> MAX6650/MAX6651 chip is a multi-function device with I2C busses. The
> chip includes fan-speed regulators and monitors, GPIO, and alarm.
>
> This patch is an initial release of a MAX6650/6651 MFD driver that
> supports to enable the chip with its primary I2C bus that will connect
> the hwmon, and then the gpio devices for now.
>
> Signed-off-by: Laszlo Papp <[email protected]>
> ---
[snip]
> --- /dev/null
> +++ b/drivers/mfd/max665x.c
> @@ -0,0 +1,87 @@
> +/*
> + * Device access for MAX6650-MAX6651
> + *
> + * Copyright(c) 2013 Polatis Ltd.
> + *
> + * Author: Laszlo Papp <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +
> +#include <linux/mfd/max665x-private.h>
> +
> +static struct mfd_cell max665x_devs[] = {
> + { .name = "max6651-gpio", },
> + { .name = "max6650", }, /* hwmon driver */
> + {},
> +};
> +
> +const struct regmap_config max665x_regmap_config = {
> + .reg_bits = 5,
> +};
This should be static.
> +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);
Don't you need to check the return value of devm_regmap_init_i2c?
--
With warm regards,
Sachin
On Wed, Feb 12, 2014 at 4:42 AM, Sachin Kamat <[email protected]> wrote:
> Hi Laszlo,
>
> Sorry for missing out on a couple of points during my earlier review.
> Please see inline.
Np.
> On 12 February 2014 09:32, Laszlo Papp <[email protected]> wrote:
>> MAX6650/MAX6651 chip is a multi-function device with I2C busses. The
>> chip includes fan-speed regulators and monitors, GPIO, and alarm.
>>
>> This patch is an initial release of a MAX6650/6651 MFD driver that
>> supports to enable the chip with its primary I2C bus that will connect
>> the hwmon, and then the gpio devices for now.
>>
>> Signed-off-by: Laszlo Papp <[email protected]>
>> ---
> [snip]
>
>> --- /dev/null
>> +++ b/drivers/mfd/max665x.c
>> @@ -0,0 +1,87 @@
>> +/*
>> + * Device access for MAX6650-MAX6651
>> + *
>> + * Copyright(c) 2013 Polatis Ltd.
>> + *
>> + * Author: Laszlo Papp <[email protected]>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License as published by the
>> + * Free Software Foundation; either version 2 of the License, or (at your
>> + * option) any later version.
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/module.h>
>> +#include <linux/i2c.h>
>> +
>> +#include <linux/mfd/max665x-private.h>
>> +
>> +static struct mfd_cell max665x_devs[] = {
>> + { .name = "max6651-gpio", },
>> + { .name = "max6650", }, /* hwmon driver */
>> + {},
>> +};
>> +
>> +const struct regmap_config max665x_regmap_config = {
>> + .reg_bits = 5,
>> +};
>
> This should be static.
Agree.
>> +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);
>
> Don't you need to check the return value of devm_regmap_init_i2c?
I personally think I should. I strived for consistency though with
other similar drivers. After this many reviews about such things, it
seems that consistency matters here less than other projects which I
can cope with, thanks.
> >> + max665x->map = devm_regmap_init_i2c(i2c, &max665x_regmap_config);
> >
> > Don't you need to check the return value of devm_regmap_init_i2c?
>
> I personally think I should. I strived for consistency though with
> other similar drivers. After this many reviews about such things, it
> seems that consistency matters here less than other projects which I
> can cope with, thanks.
The right thing to do is normally preferred.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Wed, Feb 12, 2014 at 8:32 AM, Lee Jones <[email protected]> wrote:
>> >> + max665x->map = devm_regmap_init_i2c(i2c, &max665x_regmap_config);
>> >
>> > Don't you need to check the return value of devm_regmap_init_i2c?
>>
>> I personally think I should. I strived for consistency though with
>> other similar drivers. After this many reviews about such things, it
>> seems that consistency matters here less than other projects which I
>> can cope with, thanks.
>
> The right thing to do is normally preferred.
Yes, sure.
On Wed, Feb 12, 2014 at 04:02:40AM +0000, Laszlo Papp wrote:
> +const struct regmap_config max665x_regmap_config = {
> + .reg_bits = 5,
> +};
This would normally be static too, and I'd *really* expect to see a
val_bits set here. I'm a bit surprised this works without one.
> + mutex_init(&max665x->iolock);
What is this needed for?
Laszlo,
> > +const struct regmap_config max665x_regmap_config = {
> > + .reg_bits = 5,
> > +};
>
> This would normally be static too, and I'd *really* expect to see a
> val_bits set here. I'm a bit surprised this works without one.
Mark (privately) mentioned to me that this patch can't possibly work
given the current Regmap configuration. Patches are accepted into
Mainline based on the premise that they are fully tested and working,
_prior_ to submitting [1].
It's also pretty pointless having an MFD driver without any
children, so unless (at least one of) the child device drivers have
been accepted by pull-time, your work will not be part of the
pull-request headed for Mainline.
Please inform me of your plans as you with to proceed.
[] Documentation/SubmitChecklist
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Thu, Feb 13, 2014 at 8:23 AM, Lee Jones <[email protected]> wrote:
> Laszlo,
>
>> > +const struct regmap_config max665x_regmap_config = {
>> > + .reg_bits = 5,
>> > +};
>>
>> This would normally be static too, and I'd *really* expect to see a
>> val_bits set here. I'm a bit surprised this works without one.
>
> Mark (privately) mentioned to me that this patch can't possibly work
> given the current Regmap configuration.
Strange because I have tested the change, although not for days and
weeks. What exactly cannot possible work?
> Patches are accepted into
> Mainline based on the premise that they are fully tested and working,
> _prior_ to submitting [1].
Yes, I am aware of it, hence the quick testing done.
> It's also pretty pointless having an MFD driver without any
> children,
Agree.
> so unless (at least one of) the child device drivers have
> been accepted by pull-time, your work will not be part of the
> pull-request headed for Mainline.
Sure, I think that is reasonable.
> Please inform me of your plans as you with to proceed.
I already sent the hwmon changes to the maintainers - you included or
linked -, but got no response so far. I am all for making it work
ASAP. I will make a more thorough testing today or tomorrow. After
that, it is up to them...
On Thu, Feb 13, 2014 at 09:14:11AM +0000, Laszlo Papp wrote:
> On Thu, Feb 13, 2014 at 8:23 AM, Lee Jones <[email protected]> wrote:
> > Laszlo,
> >
> >> > +const struct regmap_config max665x_regmap_config = {
> >> > + .reg_bits = 5,
> >> > +};
> >> This would normally be static too, and I'd *really* expect to see a
> >> val_bits set here. I'm a bit surprised this works without one.
> > Mark (privately) mentioned to me that this patch can't possibly work
> > given the current Regmap configuration.
> Strange because I have tested the change, although not for days and
> weeks. What exactly cannot possible work?
The fact that it's using 5 bit registers should cause it to be rejected
when trying to initialise as the regmap code doesn't support 5 bit
registers, and I'd not expect us to have selected sensible value
formatting code either.
On Thu, Feb 13, 2014 at 9:14 AM, Laszlo Papp <[email protected]> wrote:
> On Thu, Feb 13, 2014 at 8:23 AM, Lee Jones <[email protected]> wrote:
>> Laszlo,
>>
>>> > +const struct regmap_config max665x_regmap_config = {
>>> > + .reg_bits = 5,
>>> > +};
>>>
>>> This would normally be static too, and I'd *really* expect to see a
>>> val_bits set here. I'm a bit surprised this works without one.
>>
>> Mark (privately) mentioned to me that this patch can't possibly work
>> given the current Regmap configuration.
>
> Strange because I have tested the change, although not for days and
> weeks. What exactly cannot possible work?
Hmm, yes, as it seems, I have not actually tested the patch set
_after_ all the modifications. I deemed some of them trivial enough,
but apparently I was wrong. In this case, sending trivial updates will
slow down, but I guess that is the way to follow. Well, I apologize.
On Wed, Feb 12, 2014 at 5:50 PM, Mark Brown <[email protected]> wrote:
> On Wed, Feb 12, 2014 at 04:02:40AM +0000, Laszlo Papp wrote:
>
>> +const struct regmap_config max665x_regmap_config = {
>> + .reg_bits = 5,
>> +};
>
> This would normally be static too, and I'd *really* expect to see a
> val_bits set here. I'm a bit surprised this works without one.
Makes sense, thanks. Shall I also set the .max_register or any other
mandatory variables in here?
>> + mutex_init(&max665x->iolock);
>
> What is this needed for?
It was done for consistency with the other mfd drivers (maxim), e.g.
8997 or 8998 as the closest resemblence in this family. Would you
prefer me removing this mutex locker?
> >> + mutex_init(&max665x->iolock);
> >
> > What is this needed for?
>
> It was done for consistency with the other mfd drivers (maxim), e.g.
> 8997 or 8998 as the closest resemblence in this family. Would you
> prefer me removing this mutex locker?
If you're not using mutexes, why would you need to initialise it?
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Fri, Feb 14, 2014 at 10:19 AM, Lee Jones <[email protected]> wrote:
>> >> + mutex_init(&max665x->iolock);
>> >
>> > What is this needed for?
>>
>> It was done for consistency with the other mfd drivers (maxim), e.g.
>> 8997 or 8998 as the closest resemblence in this family. Would you
>> prefer me removing this mutex locker?
>
> If you're not using mutexes, why would you need to initialise it?
Yeah, I had the same thought about an hour ago that this sneaked in
because I was not originally using regmap, but now with the regmap
usage, it is just a left-over. So, it is not even for consistency
anymore. I was wrong claiming that.
On Fri, Feb 14, 2014 at 09:02:20AM +0000, Laszlo Papp wrote:
> On Wed, Feb 12, 2014 at 5:50 PM, Mark Brown <[email protected]> wrote:
> > On Wed, Feb 12, 2014 at 04:02:40AM +0000, Laszlo Papp wrote:
> >> +const struct regmap_config max665x_regmap_config = {
> >> + .reg_bits = 5,
> >> +};
> > This would normally be static too, and I'd *really* expect to see a
> > val_bits set here. I'm a bit surprised this works without one.
> Makes sense, thanks. Shall I also set the .max_register or any other
> mandatory variables in here?
It's better to set max_register since that makes the debug features of
the regmap API more useful but it's not required, only reg_bits and
val_bits are absolutely required. In general the more complete the
description of the register map in the driver the better.
On Fri, Feb 14, 2014 at 8:57 PM, Mark Brown <[email protected]> wrote:
> On Fri, Feb 14, 2014 at 09:02:20AM +0000, Laszlo Papp wrote:
>> On Wed, Feb 12, 2014 at 5:50 PM, Mark Brown <[email protected]> wrote:
>> > On Wed, Feb 12, 2014 at 04:02:40AM +0000, Laszlo Papp wrote:
>
>> >> +const struct regmap_config max665x_regmap_config = {
>> >> + .reg_bits = 5,
>> >> +};
>
>> > This would normally be static too, and I'd *really* expect to see a
>> > val_bits set here. I'm a bit surprised this works without one.
>
>> Makes sense, thanks. Shall I also set the .max_register or any other
>> mandatory variables in here?
>
> It's better to set max_register since that makes the debug features of
> the regmap API more useful but it's not required, only reg_bits and
> val_bits are absolutely required. In general the more complete the
> description of the register map in the driver the better.
OK, thanks.