Messages in this thread | | | Date | Mon, 10 Feb 2014 20:52:55 +0530 | Subject | Re: [PATCH v2] mfd: MAX6650/6651 support | From | Sachin Kamat <> |
| |
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
| |