Re: [PATCH V3 2/4] mfd: pv88080: MFD core support

[Lee Jones <lee.jones@linaro.org>]

On Fri, 25 Nov 2016, Eric Hyeung Dong Jeong wrote:
> On Monday, November 21, 2016 10:09 PM, Lee Jones Wrote:
> > On Fri, 18 Nov 2016, Eric Jeong wrote:
> > 
> > >
> > > From: Eric Jeong <eric.jeong.opensource@diasemi.com>
> > >
> > > 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 <eric.jeong.opensource@diasemi.com>
> > >
> > > ---
> > > 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.

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