All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v6] mfd: MAX6650/6651 support
@ 2014-02-12  4:02 Laszlo Papp
  2014-02-12  4:42 ` Sachin Kamat
  2014-02-12 17:50 ` Mark Brown
  0 siblings, 2 replies; 15+ messages in thread
From: Laszlo Papp @ 2014-02-12  4:02 UTC (permalink / raw)
  To: lee.jones
  Cc: linus.walleij, linux-kernel, k.kozlowski, sachin.kamat, jdelvare,
	linux, lpapp

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               | 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 <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 */
+	{},
+};
+
+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 <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] 15+ messages in thread

* Re: [PATCH v6] mfd: MAX6650/6651 support
  2014-02-12  4:02 [PATCH v6] mfd: MAX6650/6651 support Laszlo Papp
@ 2014-02-12  4:42 ` Sachin Kamat
  2014-02-12  7:06   ` Laszlo Papp
  2014-02-12 17:50 ` Mark Brown
  1 sibling, 1 reply; 15+ messages in thread
From: Sachin Kamat @ 2014-02-12  4:42 UTC (permalink / raw)
  To: Laszlo Papp
  Cc: Lee Jones, Linus Walleij, LKML, Krzysztof Kozlowski, jdelvare,
	Guenter Roeck

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 <lpapp@kde.org> wrote:
> MAX6650/MAX6651 chip is a multi-function device with I2C busses. The
> chip includes fan-speed regulators and monitors, GPIO, and alarm.
>
> This patch is an initial release of a MAX6650/6651 MFD driver that
> supports to enable the chip with its primary I2C bus that will connect
> the hwmon, and then the gpio devices for now.
>
> Signed-off-by: Laszlo Papp <lpapp@kde.org>
> ---
[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 <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 */
> +       {},
> +};
> +
> +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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6] mfd: MAX6650/6651 support
  2014-02-12  4:42 ` Sachin Kamat
@ 2014-02-12  7:06   ` Laszlo Papp
  2014-02-12  8:32     ` Lee Jones
  0 siblings, 1 reply; 15+ messages in thread
From: Laszlo Papp @ 2014-02-12  7:06 UTC (permalink / raw)
  To: Sachin Kamat
  Cc: Lee Jones, Linus Walleij, LKML, Krzysztof Kozlowski,
	Jean Delvare, Guenter Roeck

On Wed, Feb 12, 2014 at 4:42 AM, Sachin Kamat <sachin.kamat@linaro.org> 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 <lpapp@kde.org> wrote:
>> MAX6650/MAX6651 chip is a multi-function device with I2C busses. The
>> chip includes fan-speed regulators and monitors, GPIO, and alarm.
>>
>> This patch is an initial release of a MAX6650/6651 MFD driver that
>> supports to enable the chip with its primary I2C bus that will connect
>> the hwmon, and then the gpio devices for now.
>>
>> Signed-off-by: Laszlo Papp <lpapp@kde.org>
>> ---
> [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 <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 */
>> +       {},
>> +};
>> +
>> +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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6] mfd: MAX6650/6651 support
  2014-02-12  7:06   ` Laszlo Papp
@ 2014-02-12  8:32     ` Lee Jones
  2014-02-12  9:34       ` Laszlo Papp
  0 siblings, 1 reply; 15+ messages in thread
From: Lee Jones @ 2014-02-12  8:32 UTC (permalink / raw)
  To: Laszlo Papp
  Cc: Sachin Kamat, Linus Walleij, LKML, Krzysztof Kozlowski,
	Jean Delvare, Guenter Roeck

> >> +       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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6] mfd: MAX6650/6651 support
  2014-02-12  8:32     ` Lee Jones
@ 2014-02-12  9:34       ` Laszlo Papp
  0 siblings, 0 replies; 15+ messages in thread
From: Laszlo Papp @ 2014-02-12  9:34 UTC (permalink / raw)
  To: Lee Jones
  Cc: Sachin Kamat, Linus Walleij, LKML, Krzysztof Kozlowski,
	Jean Delvare, Guenter Roeck

On Wed, Feb 12, 2014 at 8:32 AM, Lee Jones <lee.jones@linaro.org> 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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6] mfd: MAX6650/6651 support
  2014-02-12  4:02 [PATCH v6] mfd: MAX6650/6651 support Laszlo Papp
  2014-02-12  4:42 ` Sachin Kamat
@ 2014-02-12 17:50 ` Mark Brown
  2014-02-13  8:23   ` Lee Jones
  2014-02-14  9:02   ` Laszlo Papp
  1 sibling, 2 replies; 15+ messages in thread
From: Mark Brown @ 2014-02-12 17:50 UTC (permalink / raw)
  To: Laszlo Papp
  Cc: lee.jones, linus.walleij, linux-kernel, k.kozlowski,
	sachin.kamat, jdelvare, linux

[-- Attachment #1: Type: text/plain, Size: 337 bytes --]

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?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6] mfd: MAX6650/6651 support
  2014-02-12 17:50 ` Mark Brown
@ 2014-02-13  8:23   ` Lee Jones
  2014-02-13  9:14     ` Laszlo Papp
  2014-02-14  9:02   ` Laszlo Papp
  1 sibling, 1 reply; 15+ messages in thread
From: Lee Jones @ 2014-02-13  8:23 UTC (permalink / raw)
  To: Mark Brown
  Cc: Laszlo Papp, linus.walleij, linux-kernel, k.kozlowski,
	sachin.kamat, jdelvare, linux

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6] mfd: MAX6650/6651 support
  2014-02-13  8:23   ` Lee Jones
@ 2014-02-13  9:14     ` Laszlo Papp
  2014-02-13 10:26       ` Mark Brown
  2014-02-13 10:34       ` Laszlo Papp
  0 siblings, 2 replies; 15+ messages in thread
From: Laszlo Papp @ 2014-02-13  9:14 UTC (permalink / raw)
  To: Lee Jones
  Cc: Mark Brown, Linus Walleij, LKML, Krzysztof Kozlowski,
	Sachin Kamat, Jean Delvare, Guenter Roeck

On Thu, Feb 13, 2014 at 8:23 AM, Lee Jones <lee.jones@linaro.org> 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...

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6] mfd: MAX6650/6651 support
  2014-02-13  9:14     ` Laszlo Papp
@ 2014-02-13 10:26       ` Mark Brown
  2014-02-13 10:34       ` Laszlo Papp
  1 sibling, 0 replies; 15+ messages in thread
From: Mark Brown @ 2014-02-13 10:26 UTC (permalink / raw)
  To: Laszlo Papp
  Cc: Lee Jones, Linus Walleij, LKML, Krzysztof Kozlowski,
	Sachin Kamat, Jean Delvare, Guenter Roeck

[-- Attachment #1: Type: text/plain, Size: 855 bytes --]

On Thu, Feb 13, 2014 at 09:14:11AM +0000, Laszlo Papp wrote:
> On Thu, Feb 13, 2014 at 8:23 AM, Lee Jones <lee.jones@linaro.org> 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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6] mfd: MAX6650/6651 support
  2014-02-13  9:14     ` Laszlo Papp
  2014-02-13 10:26       ` Mark Brown
@ 2014-02-13 10:34       ` Laszlo Papp
  1 sibling, 0 replies; 15+ messages in thread
From: Laszlo Papp @ 2014-02-13 10:34 UTC (permalink / raw)
  To: Lee Jones
  Cc: Mark Brown, Linus Walleij, LKML, Krzysztof Kozlowski,
	Sachin Kamat, Jean Delvare, Guenter Roeck

On Thu, Feb 13, 2014 at 9:14 AM, Laszlo Papp <lpapp@kde.org> wrote:
> On Thu, Feb 13, 2014 at 8:23 AM, Lee Jones <lee.jones@linaro.org> 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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6] mfd: MAX6650/6651 support
  2014-02-12 17:50 ` Mark Brown
  2014-02-13  8:23   ` Lee Jones
@ 2014-02-14  9:02   ` Laszlo Papp
  2014-02-14 10:19     ` Lee Jones
  2014-02-14 20:57     ` Mark Brown
  1 sibling, 2 replies; 15+ messages in thread
From: Laszlo Papp @ 2014-02-14  9:02 UTC (permalink / raw)
  To: Mark Brown
  Cc: Lee Jones, Linus Walleij, LKML, Krzysztof Kozlowski,
	Sachin Kamat, Jean Delvare, Guenter Roeck

On Wed, Feb 12, 2014 at 5:50 PM, Mark Brown <broonie@kernel.org> 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?

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6] mfd: MAX6650/6651 support
  2014-02-14  9:02   ` Laszlo Papp
@ 2014-02-14 10:19     ` Lee Jones
  2014-02-14 10:59       ` Laszlo Papp
  2014-02-14 20:57     ` Mark Brown
  1 sibling, 1 reply; 15+ messages in thread
From: Lee Jones @ 2014-02-14 10:19 UTC (permalink / raw)
  To: Laszlo Papp
  Cc: Mark Brown, Linus Walleij, LKML, Krzysztof Kozlowski,
	Sachin Kamat, Jean Delvare, Guenter Roeck

> >> +     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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6] mfd: MAX6650/6651 support
  2014-02-14 10:19     ` Lee Jones
@ 2014-02-14 10:59       ` Laszlo Papp
  0 siblings, 0 replies; 15+ messages in thread
From: Laszlo Papp @ 2014-02-14 10:59 UTC (permalink / raw)
  To: Lee Jones
  Cc: Mark Brown, Linus Walleij, LKML, Krzysztof Kozlowski,
	Sachin Kamat, Jean Delvare, Guenter Roeck

On Fri, Feb 14, 2014 at 10:19 AM, Lee Jones <lee.jones@linaro.org> 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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6] mfd: MAX6650/6651 support
  2014-02-14  9:02   ` Laszlo Papp
  2014-02-14 10:19     ` Lee Jones
@ 2014-02-14 20:57     ` Mark Brown
  2014-02-17  8:55       ` Laszlo Papp
  1 sibling, 1 reply; 15+ messages in thread
From: Mark Brown @ 2014-02-14 20:57 UTC (permalink / raw)
  To: Laszlo Papp
  Cc: Lee Jones, Linus Walleij, LKML, Krzysztof Kozlowski,
	Sachin Kamat, Jean Delvare, Guenter Roeck

[-- Attachment #1: Type: text/plain, Size: 805 bytes --]

On Fri, Feb 14, 2014 at 09:02:20AM +0000, Laszlo Papp wrote:
> On Wed, Feb 12, 2014 at 5:50 PM, Mark Brown <broonie@kernel.org> 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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6] mfd: MAX6650/6651 support
  2014-02-14 20:57     ` Mark Brown
@ 2014-02-17  8:55       ` Laszlo Papp
  0 siblings, 0 replies; 15+ messages in thread
From: Laszlo Papp @ 2014-02-17  8:55 UTC (permalink / raw)
  To: Mark Brown
  Cc: Lee Jones, Linus Walleij, LKML, Krzysztof Kozlowski,
	Sachin Kamat, Jean Delvare, Guenter Roeck

On Fri, Feb 14, 2014 at 8:57 PM, Mark Brown <broonie@kernel.org> 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 <broonie@kernel.org> 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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2014-02-17  8:55 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-12  4:02 [PATCH v6] mfd: MAX6650/6651 support Laszlo Papp
2014-02-12  4:42 ` Sachin Kamat
2014-02-12  7:06   ` Laszlo Papp
2014-02-12  8:32     ` Lee Jones
2014-02-12  9:34       ` Laszlo Papp
2014-02-12 17:50 ` Mark Brown
2014-02-13  8:23   ` Lee Jones
2014-02-13  9:14     ` Laszlo Papp
2014-02-13 10:26       ` Mark Brown
2014-02-13 10:34       ` Laszlo Papp
2014-02-14  9:02   ` Laszlo Papp
2014-02-14 10:19     ` Lee Jones
2014-02-14 10:59       ` Laszlo Papp
2014-02-14 20:57     ` Mark Brown
2014-02-17  8:55       ` Laszlo Papp

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.