On Fri, 25 Nov 2016, Eric Hyeung Dong Jeong wrote:

On Monday, November 21, 2016 10:09 PM, Lee Jones Wrote:

quoted

On Fri, 18 Nov 2016, Eric Jeong wrote:

quoted

From: Eric Jeong <redacted>

This patch adds supports for PV88080 MFD core device.

It provides communication through the I2C interface.
It contains the following components:
    - Regulators
    - Configurable GPIOs

Kconfig and Makefile are updated to reflect support for PV88080 PMIC.

Signed-off-by: Eric Jeong <redacted>

---
This patch applies against linux-next and next-20161117

Hi,

This patch adds MFD core driver for PV88080 PMIC.
This is done as part of the existing PV88080 regulator driver by
expending the driver for GPIO function support.

Change since PATCH V2
 - Make one file insted of usging core and i2c file
 - Use devm_ function to be managed resource automatically
 - Separated mfd_cell and regmap_irq_chip declaration for clarification.
 - Updated Kconfig to use OF and assign yes to I2C

Change since PATCH V1
 - Patch separated from PATCH V1

Regards,
Eric Jeong, Dialog Semiconductor Ltd.


 drivers/mfd/Kconfig         |   12 ++
 drivers/mfd/Makefile        |    1 +
 drivers/mfd/pv88080.c       |  331 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/pv88080.h |  222 +++++++++++++++++++++++++++++
 4 files changed, 566 insertions(+)
 create mode 100644 drivers/mfd/pv88080.c  create mode 100644
include/linux/mfd/pv88080.h

It's a good idea to cut out all of the code/comments that is either
not relevant, or you are not providing comment (besides "will do")
on.

quoted

quoted

+struct pv88080 {
+	struct device *dev;
+	struct regmap *regmap;
+	unsigned long type;

Does this really need to be in here?

The *type* member is used for separating silicon type.  
And, regulator and gpio driver also use the member to check the type 
for proper configuration without additional code. 
That is the reason that the member is added in the structure.

I don't see how this is being used, so assuming the other Maintainers
are happy with the implementation it's okay for this to live here.
However, please consider changing to something better than "value" or
"type".  Perhaps "varian"t or "model" or similar would be better?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog