All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] mfd: MAX6650/6651 support
@ 2014-02-08 11:33 Laszlo Papp
  2014-02-09 19:50 ` Laszlo Papp
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Laszlo Papp @ 2014-02-08 11:33 UTC (permalink / raw)
  To: lee.jones; +Cc: linus.walleij, linux-kernel, Laszlo Papp

MAX6650/MAX6651 chip is a multi-function device with I2C busses. The
chip includes fan-speed regulators and monitors, GPIO, and alarm.

This patch is an initial release of a MAX6650/6651 MFD driver that
supports to enable the chip with its primary I2C bus that will connect
the hwmon, and then the gpio devices for now.

Signed-off-by: Laszlo Papp <lpapp@kde.org>
---
 drivers/mfd/Kconfig                 | 11 +++++
 drivers/mfd/Makefile                |  1 +
 drivers/mfd/max665x.c               | 88 +++++++++++++++++++++++++++++++++++++
 include/linux/mfd/max665x-private.h | 42 ++++++++++++++++++
 4 files changed, 142 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..e25be62 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"
+	select MFD_CORE
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  Say yes here to support for Maxim Semiconductor MAX6650/MAX6651. This is
+	  a fan speed regulator and monitor IC. This driver provies 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..395373fe
--- /dev/null
+++ b/drivers/mfd/max665x.c
@@ -0,0 +1,88 @@
+/*
+ * 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 = 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 const struct i2c_device_id max665x_id[] = {
+	{ "max6650", TYPE_MAX6650 },
+	{ "max6651", TYPE_MAX6651 },
+	{},
+};
+MODULE_DEVICE_TABLE(i2c, max665x_id);
+
+static struct i2c_driver max665x_driver = {
+	.driver = {
+		.name = "max665x",
+		.owner = THIS_MODULE,
+	},
+	.probe = max665x_probe,
+	.remove = max665x_remove,
+	.id_table = max665x_id,
+};
+
+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..92da00c
--- /dev/null
+++ b/include/linux/mfd/max665x-private.h
@@ -0,0 +1,42 @@
+/*
+ * max665x-private.h - Driver for the Maxim 6650/6651
+ *
+ * Copyright 2013 Polatis Ltd.
+ *
+ * Author: Milo Kim <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/export.h>
+#include <linux/regmap.h>
+
+struct max665x_dev {
+	struct device *dev;
+	struct mutex iolock;
+
+	struct i2c_client *i2c;
+	struct regmap     *map;
+
+	int type;
+};
+
+enum max665x_types {
+	TYPE_MAX6650,
+	TYPE_MAX6651,
+};
+
+#endif
-- 
1.8.5.4


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

* Re: [PATCH v2] mfd: MAX6650/6651 support
  2014-02-08 11:33 [PATCH v2] mfd: MAX6650/6651 support Laszlo Papp
@ 2014-02-09 19:50 ` Laszlo Papp
  2014-02-10  7:37 ` Krzysztof Kozlowski
  2014-02-10 12:02 ` Sachin Kamat
  2 siblings, 0 replies; 16+ messages in thread
From: Laszlo Papp @ 2014-02-09 19:50 UTC (permalink / raw)
  To: Lee Jones; +Cc: Linus Walleij, LKML, Laszlo Papp

> + * Author: Milo Kim <laszlo.papp@polatis.com>

s/Milo Kim/Laszlo Papp/

I just copied and pasted some existing copyrights, and I, apparently,
have not changed it properly to my name; apologies for that. I will
fix this in the next batch after getting some review from others.

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

* Re: [PATCH v2] mfd: MAX6650/6651 support
  2014-02-08 11:33 [PATCH v2] mfd: MAX6650/6651 support Laszlo Papp
  2014-02-09 19:50 ` Laszlo Papp
@ 2014-02-10  7:37 ` Krzysztof Kozlowski
  2014-02-10  7:46   ` Laszlo Papp
  2014-02-10 12:02 ` Sachin Kamat
  2 siblings, 1 reply; 16+ messages in thread
From: Krzysztof Kozlowski @ 2014-02-10  7:37 UTC (permalink / raw)
  To: Laszlo Papp; +Cc: lee.jones, linus.walleij, linux-kernel

Hi,

On Sat, 2014-02-08 at 11:33 +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               | 88 +++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/max665x-private.h | 42 ++++++++++++++++++
>  4 files changed, 142 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..e25be62 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"
> +	select MFD_CORE
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	  Say yes here to support for Maxim Semiconductor MAX6650/MAX6651. This is
> +	  a fan speed regulator and monitor IC. This driver provies common support

s/provies/provides/

(...)


> +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 = regmap_init_i2c(i2c, &max665x_regmap_config);

Use devm_regmap_init_i2c() (or add missing regmap_exit()).


Best regards,
Krzysztof



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

* Re: [PATCH v2] mfd: MAX6650/6651 support
  2014-02-10  7:37 ` Krzysztof Kozlowski
@ 2014-02-10  7:46   ` Laszlo Papp
  0 siblings, 0 replies; 16+ messages in thread
From: Laszlo Papp @ 2014-02-10  7:46 UTC (permalink / raw)
  To: Krzysztof Kozlowski; +Cc: Lee Jones, Linus Walleij, LKML

>> +     help
>> +       Say yes here to support for Maxim Semiconductor MAX6650/MAX6651. This is
>> +       a fan speed regulator and monitor IC. This driver provies common support
>
> s/provies/provides/

Good catch!

(Note to myself: I should have run my vim spellchecker... )

>> +     max665x->map = regmap_init_i2c(i2c, &max665x_regmap_config);
>
> Use devm_regmap_init_i2c() (or add missing regmap_exit()).

Ah, okay, I was not aware of that, thanks.

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

* Re: [PATCH v2] mfd: MAX6650/6651 support
  2014-02-08 11:33 [PATCH v2] mfd: MAX6650/6651 support Laszlo Papp
  2014-02-09 19:50 ` Laszlo Papp
  2014-02-10  7:37 ` Krzysztof Kozlowski
@ 2014-02-10 12:02 ` Sachin Kamat
  2014-02-10 12:21   ` Lee Jones
       [not found]   ` <CAOMwXhNmjehcd+aiWGuKs7CijheXXngsXTHMfG9SDLpHO=KaPw@mail.gmail.com>
  2 siblings, 2 replies; 16+ messages in thread
From: Sachin Kamat @ 2014-02-10 12:02 UTC (permalink / raw)
  To: Laszlo Papp; +Cc: Lee Jones, Linus Walleij, LKML

Hi Laszlo,

If you are considering re-spinning this patch based on Lee's comments, please
also consider my comments inline.

On 8 February 2014 17:03, 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>
> ---
>  drivers/mfd/Kconfig                 | 11 +++++
>  drivers/mfd/Makefile                |  1 +
>  drivers/mfd/max665x.c               | 88 +++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/max665x-private.h | 42 ++++++++++++++++++
>  4 files changed, 142 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..e25be62 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"
> +       select MFD_CORE
> +       depends on I2C
> +       select REGMAP_I2C

Move the "depends on" before the select MFD_CORE.

> +       help
> +         Say yes here to support for Maxim Semiconductor MAX6650/MAX6651.

 s/here to support/here to add support

>This is
> +         a fan speed regulator and monitor IC. This driver provies 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..395373fe
> --- /dev/null
> +++ b/drivers/mfd/max665x.c
> @@ -0,0 +1,88 @@
> +/*
> + * 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>

Please arrange these alphabetically.

[snip]
> +
> +struct max665x_dev {
> +       struct device *dev;
> +       struct mutex iolock;
> +
> +       struct i2c_client *i2c;
> +       struct regmap     *map;
> +
> +       int type;

Unnecessary extra lines above could be removed.


-- 
With warm regards,
Sachin

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

* Re: [PATCH v2] mfd: MAX6650/6651 support
  2014-02-10 12:02 ` Sachin Kamat
@ 2014-02-10 12:21   ` Lee Jones
  2014-02-10 15:20     ` Sachin Kamat
       [not found]   ` <CAOMwXhNmjehcd+aiWGuKs7CijheXXngsXTHMfG9SDLpHO=KaPw@mail.gmail.com>
  1 sibling, 1 reply; 16+ messages in thread
From: Lee Jones @ 2014-02-10 12:21 UTC (permalink / raw)
  To: Sachin Kamat; +Cc: Laszlo Papp, Linus Walleij, LKML

> > +#include <linux/device.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/module.h>
> > +#include <linux/i2c.h>
> 
> Please arrange these alphabetically.

Why?

-- 
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] 16+ messages in thread

* Re: [PATCH v2] mfd: MAX6650/6651 support
  2014-02-10 12:21   ` Lee Jones
@ 2014-02-10 15:20     ` Sachin Kamat
  2014-02-10 15:56       ` Lee Jones
  2014-02-10 18:36       ` Laszlo Papp
  0 siblings, 2 replies; 16+ messages in thread
From: Sachin Kamat @ 2014-02-10 15:20 UTC (permalink / raw)
  To: Lee Jones; +Cc: Laszlo Papp, Linus Walleij, LKML

On 10 February 2014 17:51, Lee Jones <lee.jones@linaro.org> wrote:
>> > +#include <linux/device.h>
>> > +#include <linux/mfd/core.h>
>> > +#include <linux/module.h>
>> > +#include <linux/i2c.h>
>>
>> Please arrange these alphabetically.
>
> Why?

1. It makes it easier to avoid adding duplicate includes.
2. Code looks more ordered/organized.
3. Prevents further clean up patches arranging them so :)

-- 
With warm regards,
Sachin

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

* Re: [PATCH v2] mfd: MAX6650/6651 support
       [not found]   ` <CAOMwXhNmjehcd+aiWGuKs7CijheXXngsXTHMfG9SDLpHO=KaPw@mail.gmail.com>
@ 2014-02-10 15:22     ` Sachin Kamat
  2014-02-11 10:10       ` Laszlo Papp
  0 siblings, 1 reply; 16+ messages in thread
From: Sachin Kamat @ 2014-02-10 15:22 UTC (permalink / raw)
  To: Laszlo Papp, LKML, Lee Jones

On 10 February 2014 18:48, Laszlo Papp <lpapp@kde.org> wrote:
> On Mon, Feb 10, 2014 at 12:02 PM, Sachin Kamat <sachin.kamat@linaro.org> wrote:
>> Hi Laszlo,
>>
>> If you are considering re-spinning this patch based on Lee's comments, please
>> also consider my comments inline.
>
> Hi, sure, thanks.
>
>> On 8 February 2014 17:03, 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>
>>> ---
>>>  drivers/mfd/Kconfig                 | 11 +++++
>>>  drivers/mfd/Makefile                |  1 +
>>>  drivers/mfd/max665x.c               | 88 +++++++++++++++++++++++++++++++++++++
>>>  include/linux/mfd/max665x-private.h | 42 ++++++++++++++++++
>>>  4 files changed, 142 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..e25be62 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"
>>> +       select MFD_CORE
>>> +       depends on I2C
>>> +       select REGMAP_I2C
>>
>> Move the "depends on" before the select MFD_CORE.
>
> OK.
>
>>> +       help
>>> +         Say yes here to support for Maxim Semiconductor MAX6650/MAX6651.
>>
>>  s/here to support/here to add support
>
> OK.
>
>>>This is
>>> +         a fan speed regulator and monitor IC. This driver provies 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..395373fe
>>> --- /dev/null
>>> +++ b/drivers/mfd/max665x.c
>>> @@ -0,0 +1,88 @@
>>> +/*
>>> + * 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>
>>
>> Please arrange these alphabetically.
>
> OK, it does not matter for me. Please agree upon something with Lee. :-)
>
>>
>> [snip]
>>> +
>>> +struct max665x_dev {
>>> +       struct device *dev;
>>> +       struct mutex iolock;
>>> +
>>> +       struct i2c_client *i2c;
>>> +       struct regmap     *map;
>>> +
>>> +       int type;
>>
>> Unnecessary extra lines above could be removed.
>
> I prefer it that way, but I will remove the two extra lines as you wish.

As I already said, these are just nits and only since you were
re-spinning the patch,
I suggested them. Also, since this is a new file being added, it
avoids further patches
doing these same things.

-- 
With warm regards,
Sachin

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

* Re: [PATCH v2] mfd: MAX6650/6651 support
  2014-02-10 15:20     ` Sachin Kamat
@ 2014-02-10 15:56       ` Lee Jones
  2014-02-10 18:36       ` Laszlo Papp
  1 sibling, 0 replies; 16+ messages in thread
From: Lee Jones @ 2014-02-10 15:56 UTC (permalink / raw)
  To: Sachin Kamat; +Cc: Laszlo Papp, Linus Walleij, LKML

> >> > +#include <linux/device.h>
> >> > +#include <linux/mfd/core.h>
> >> > +#include <linux/module.h>
> >> > +#include <linux/i2c.h>
> >>
> >> Please arrange these alphabetically.
> >
> > Why?
> 
> 1. It makes it easier to avoid adding duplicate includes.
> 2. Code looks more ordered/organized.
> 3. Prevents further clean up patches arranging them so :)

I wouldn't accept any patches doing such worthless things. ;)

-- 
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] 16+ messages in thread

* Re: [PATCH v2] mfd: MAX6650/6651 support
  2014-02-10 15:20     ` Sachin Kamat
  2014-02-10 15:56       ` Lee Jones
@ 2014-02-10 18:36       ` Laszlo Papp
  1 sibling, 0 replies; 16+ messages in thread
From: Laszlo Papp @ 2014-02-10 18:36 UTC (permalink / raw)
  To: Sachin Kamat; +Cc: Lee Jones, Linus Walleij, LKML

On Mon, Feb 10, 2014 at 3:20 PM, Sachin Kamat <sachin.kamat@linaro.org> wrote:
> On 10 February 2014 17:51, Lee Jones <lee.jones@linaro.org> wrote:
>>> > +#include <linux/device.h>
>>> > +#include <linux/mfd/core.h>
>>> > +#include <linux/module.h>
>>> > +#include <linux/i2c.h>
>>>
>>> Please arrange these alphabetically.
>>
>> Why?
>
> 1. It makes it easier to avoid adding duplicate includes.
> 2. Code looks more ordered/organized.
> 3. Prevents further clean up patches arranging them so :)

1) I am sorry, but I need to disagree with this one, personally. Check
duplicates could be done by a util at any given moment if it becomes a
pressing issue.

2) Not for me. I prefer hierarchical dependency based inclusion
between headers if there is such a thing, or just orthogonal if not.

3) It does not apply to my taste due to 1-2).

I would also like to add further detriments:

4) file rename could rearrange the list with your suggestion.

5) It would be inconsistent with a large code base out there.

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

* Re: [PATCH v2] mfd: MAX6650/6651 support
  2014-02-10 15:22     ` Sachin Kamat
@ 2014-02-11 10:10       ` Laszlo Papp
  2014-02-11 11:14         ` Sachin Kamat
  0 siblings, 1 reply; 16+ messages in thread
From: Laszlo Papp @ 2014-02-11 10:10 UTC (permalink / raw)
  To: Sachin Kamat; +Cc: LKML, Lee Jones

>>> [snip]
>>>> +
>>>> +struct max665x_dev {
>>>> +       struct device *dev;
>>>> +       struct mutex iolock;
>>>> +
>>>> +       struct i2c_client *i2c;
>>>> +       struct regmap     *map;
>>>> +
>>>> +       int type;
>>>
>>> Unnecessary extra lines above could be removed.
>>
>> I prefer it that way, but I will remove the two extra lines as you wish.
>
> As I already said, these are just nits and only since you were
> re-spinning the patch,
> I suggested them. Also, since this is a new file being added, it
> avoids further patches
> doing these same things.

Well, it is worse in my opinion because it makes the code more bloated
without separation between units, but I made the requested change
anyway because the feature matters lotta more to me than this styling.
Hope it is OK now?

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

* Re: [PATCH v2] mfd: MAX6650/6651 support
  2014-02-11 10:10       ` Laszlo Papp
@ 2014-02-11 11:14         ` Sachin Kamat
  2014-02-11 11:39           ` Laszlo Papp
  0 siblings, 1 reply; 16+ messages in thread
From: Sachin Kamat @ 2014-02-11 11:14 UTC (permalink / raw)
  To: Laszlo Papp; +Cc: LKML, Lee Jones

On 11 February 2014 15:40, Laszlo Papp <lpapp@kde.org> wrote:
>>>> [snip]
>>>>> +
>>>>> +struct max665x_dev {
>>>>> +       struct device *dev;
>>>>> +       struct mutex iolock;
>>>>> +
>>>>> +       struct i2c_client *i2c;
>>>>> +       struct regmap     *map;
>>>>> +
>>>>> +       int type;
>>>>
>>>> Unnecessary extra lines above could be removed.
>>>
>>> I prefer it that way, but I will remove the two extra lines as you wish.
>>
>> As I already said, these are just nits and only since you were
>> re-spinning the patch,
>> I suggested them. Also, since this is a new file being added, it
>> avoids further patches
>> doing these same things.
>
> Well, it is worse in my opinion because it makes the code more bloated
> without separation between units, but I made the requested change
> anyway because the feature matters lotta more to me than this styling.
> Hope it is OK now?

The only thing that you missed among what you agreed to change is the
arrangement of
"depends on" and "select" in Kconfig. Rest look OK. Thanks for taking care.

-- 
With warm regards,
Sachin

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

* Re: [PATCH v2] mfd: MAX6650/6651 support
  2014-02-11 11:14         ` Sachin Kamat
@ 2014-02-11 11:39           ` Laszlo Papp
  2014-02-11 11:42             ` Sachin Kamat
  0 siblings, 1 reply; 16+ messages in thread
From: Laszlo Papp @ 2014-02-11 11:39 UTC (permalink / raw)
  To: Sachin Kamat; +Cc: LKML, Lee Jones

On Tue, Feb 11, 2014 at 11:14 AM, Sachin Kamat <sachin.kamat@linaro.org> wrote:
> On 11 February 2014 15:40, Laszlo Papp <lpapp@kde.org> wrote:
>>>>> [snip]
>>>>>> +
>>>>>> +struct max665x_dev {
>>>>>> +       struct device *dev;
>>>>>> +       struct mutex iolock;
>>>>>> +
>>>>>> +       struct i2c_client *i2c;
>>>>>> +       struct regmap     *map;
>>>>>> +
>>>>>> +       int type;
>>>>>
>>>>> Unnecessary extra lines above could be removed.
>>>>
>>>> I prefer it that way, but I will remove the two extra lines as you wish.
>>>
>>> As I already said, these are just nits and only since you were
>>> re-spinning the patch,
>>> I suggested them. Also, since this is a new file being added, it
>>> avoids further patches
>>> doing these same things.
>>
>> Well, it is worse in my opinion because it makes the code more bloated
>> without separation between units, but I made the requested change
>> anyway because the feature matters lotta more to me than this styling.
>> Hope it is OK now?
>
> The only thing that you missed among what you agreed to change is the
> arrangement of
> "depends on" and "select" in Kconfig. Rest look OK. Thanks for taking care.

Ah, true. I will do that tomorrow if it is a blocker for the integration.

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

* Re: [PATCH v2] mfd: MAX6650/6651 support
  2014-02-11 11:39           ` Laszlo Papp
@ 2014-02-11 11:42             ` Sachin Kamat
  2014-02-11 12:23               ` Laszlo Papp
  0 siblings, 1 reply; 16+ messages in thread
From: Sachin Kamat @ 2014-02-11 11:42 UTC (permalink / raw)
  To: Laszlo Papp; +Cc: LKML, Lee Jones

On 11 February 2014 17:09, Laszlo Papp <lpapp@kde.org> wrote:
> On Tue, Feb 11, 2014 at 11:14 AM, Sachin Kamat <sachin.kamat@linaro.org> wrote:
>> On 11 February 2014 15:40, Laszlo Papp <lpapp@kde.org> wrote:
>>>>>> [snip]
>>>>>>> +
>>>>>>> +struct max665x_dev {
>>>>>>> +       struct device *dev;
>>>>>>> +       struct mutex iolock;
>>>>>>> +
>>>>>>> +       struct i2c_client *i2c;
>>>>>>> +       struct regmap     *map;
>>>>>>> +
>>>>>>> +       int type;
>>>>>>
>>>>>> Unnecessary extra lines above could be removed.
>>>>>
>>>>> I prefer it that way, but I will remove the two extra lines as you wish.
>>>>
>>>> As I already said, these are just nits and only since you were
>>>> re-spinning the patch,
>>>> I suggested them. Also, since this is a new file being added, it
>>>> avoids further patches
>>>> doing these same things.
>>>
>>> Well, it is worse in my opinion because it makes the code more bloated
>>> without separation between units, but I made the requested change
>>> anyway because the feature matters lotta more to me than this styling.
>>> Hope it is OK now?
>>
>> The only thing that you missed among what you agreed to change is the
>> arrangement of
>> "depends on" and "select" in Kconfig. Rest look OK. Thanks for taking care.
>
> Ah, true. I will do that tomorrow if it is a blocker for the integration.

I don't think it is a blocker in any way and you may choose to ignore
it as well.

-- 
With warm regards,
Sachin

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

* Re: [PATCH v2] mfd: MAX6650/6651 support
  2014-02-11 11:42             ` Sachin Kamat
@ 2014-02-11 12:23               ` Laszlo Papp
  2014-02-11 12:28                 ` Lee Jones
  0 siblings, 1 reply; 16+ messages in thread
From: Laszlo Papp @ 2014-02-11 12:23 UTC (permalink / raw)
  To: Sachin Kamat; +Cc: LKML, Lee Jones

On Tue, Feb 11, 2014 at 11:42 AM, Sachin Kamat <sachin.kamat@linaro.org> wrote:
> On 11 February 2014 17:09, Laszlo Papp <lpapp@kde.org> wrote:
>> On Tue, Feb 11, 2014 at 11:14 AM, Sachin Kamat <sachin.kamat@linaro.org> wrote:
>>> On 11 February 2014 15:40, Laszlo Papp <lpapp@kde.org> wrote:
>>>>>>> [snip]
>>>>>>>> +
>>>>>>>> +struct max665x_dev {
>>>>>>>> +       struct device *dev;
>>>>>>>> +       struct mutex iolock;
>>>>>>>> +
>>>>>>>> +       struct i2c_client *i2c;
>>>>>>>> +       struct regmap     *map;
>>>>>>>> +
>>>>>>>> +       int type;
>>>>>>>
>>>>>>> Unnecessary extra lines above could be removed.
>>>>>>
>>>>>> I prefer it that way, but I will remove the two extra lines as you wish.
>>>>>
>>>>> As I already said, these are just nits and only since you were
>>>>> re-spinning the patch,
>>>>> I suggested them. Also, since this is a new file being added, it
>>>>> avoids further patches
>>>>> doing these same things.
>>>>
>>>> Well, it is worse in my opinion because it makes the code more bloated
>>>> without separation between units, but I made the requested change
>>>> anyway because the feature matters lotta more to me than this styling.
>>>> Hope it is OK now?
>>>
>>> The only thing that you missed among what you agreed to change is the
>>> arrangement of
>>> "depends on" and "select" in Kconfig. Rest look OK. Thanks for taking care.
>>
>> Ah, true. I will do that tomorrow if it is a blocker for the integration.
>
> I don't think it is a blocker in any way and you may choose to ignore
> it as well.

That is alright, Sachin. I will make this change from home. Perhaps,
it is wise anyhow to wait for more feedback, like e.g. from Lee, etc.

Thanks again.

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

* Re: [PATCH v2] mfd: MAX6650/6651 support
  2014-02-11 12:23               ` Laszlo Papp
@ 2014-02-11 12:28                 ` Lee Jones
  0 siblings, 0 replies; 16+ messages in thread
From: Lee Jones @ 2014-02-11 12:28 UTC (permalink / raw)
  To: Laszlo Papp; +Cc: Sachin Kamat, LKML

> > I don't think it is a blocker in any way and you may choose to ignore
> > it as well.
> 
> That is alright, Sachin. I will make this change from home. Perhaps,
> it is wise anyhow to wait for more feedback, like e.g. from Lee, etc.

It's fine, just fixup and resend.

-- 
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] 16+ messages in thread

end of thread, other threads:[~2014-02-11 12:29 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-08 11:33 [PATCH v2] mfd: MAX6650/6651 support Laszlo Papp
2014-02-09 19:50 ` Laszlo Papp
2014-02-10  7:37 ` Krzysztof Kozlowski
2014-02-10  7:46   ` Laszlo Papp
2014-02-10 12:02 ` Sachin Kamat
2014-02-10 12:21   ` Lee Jones
2014-02-10 15:20     ` Sachin Kamat
2014-02-10 15:56       ` Lee Jones
2014-02-10 18:36       ` Laszlo Papp
     [not found]   ` <CAOMwXhNmjehcd+aiWGuKs7CijheXXngsXTHMfG9SDLpHO=KaPw@mail.gmail.com>
2014-02-10 15:22     ` Sachin Kamat
2014-02-11 10:10       ` Laszlo Papp
2014-02-11 11:14         ` Sachin Kamat
2014-02-11 11:39           ` Laszlo Papp
2014-02-11 11:42             ` Sachin Kamat
2014-02-11 12:23               ` Laszlo Papp
2014-02-11 12:28                 ` Lee Jones

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.