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 | 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 <[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 = 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 <[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..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 <[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/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
> + * Author: Milo Kim <[email protected]>
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.
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 <[email protected]>
> ---
> 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
>> + 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.
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 <[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]>
> ---
> 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 <[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>
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
> > +#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
On 10 February 2014 17:51, Lee Jones <[email protected]> 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
On 10 February 2014 18:48, Laszlo Papp <[email protected]> wrote:
> On Mon, Feb 10, 2014 at 12:02 PM, Sachin Kamat <[email protected]> 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 <[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]>
>>> ---
>>> 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 <[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>
>>
>> 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
> >> > +#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
On Mon, Feb 10, 2014 at 3:20 PM, Sachin Kamat <[email protected]> wrote:
> On 10 February 2014 17:51, Lee Jones <[email protected]> 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.
>>> [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?
On 11 February 2014 15:40, Laszlo Papp <[email protected]> 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
On Tue, Feb 11, 2014 at 11:14 AM, Sachin Kamat <[email protected]> wrote:
> On 11 February 2014 15:40, Laszlo Papp <[email protected]> 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.
On 11 February 2014 17:09, Laszlo Papp <[email protected]> wrote:
> On Tue, Feb 11, 2014 at 11:14 AM, Sachin Kamat <[email protected]> wrote:
>> On 11 February 2014 15:40, Laszlo Papp <[email protected]> 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
On Tue, Feb 11, 2014 at 11:42 AM, Sachin Kamat <[email protected]> wrote:
> On 11 February 2014 17:09, Laszlo Papp <[email protected]> wrote:
>> On Tue, Feb 11, 2014 at 11:14 AM, Sachin Kamat <[email protected]> wrote:
>>> On 11 February 2014 15:40, Laszlo Papp <[email protected]> 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.
> > 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