Re: [PATCH v2 3/8] regulator: MT6397: Add support for MT6397 regulator

Re: [PATCH v2 3/8] regulator: MT6397: Add support for MT6397 regulator

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1749 bytes --]

On Fri, Nov 28, 2014 at 11:54:29AM +0800, Flora Fu wrote:

> @@ -96,5 +97,4 @@ obj-$(CONFIG_REGULATOR_WM8350) += wm8350-regulator.o
>  obj-$(CONFIG_REGULATOR_WM8400) += wm8400-regulator.o
>  obj-$(CONFIG_REGULATOR_WM8994) += wm8994-regulator.o
>  
> -
>  ccflags-$(CONFIG_REGULATOR_DEBUG) += -DDEBUG

Random whitespace change here...

> +	/* For HW design, buck voltage control register has two parts.
> +	 * part 1: vsel_reg for register mode
> +	 * part 2: voselon_reg or hw control mode
> +	 * Both parts should be updated and sync when user set voltage.
> +	 */

Why does nothing else in the driver know about this "hw control mode" -
what does it actually mean, shouldn't it affect some of the other
operations?

> +	ret = regmap_update_bits(rdev->regmap, info->desc.vsel_reg,
> +		info->desc.vsel_mask, sel);
> +	if (ret != 0) {
> +		dev_err(&rdev->dev, "Failed to update vsel: %d\n", ret);
> +		return ret;
> +	}
> +	ret = regmap_update_bits(rdev->regmap, info->voselon_reg,

Missing blank line between these blocks.

> +static int mt6397_buck_get_voltage_sel(struct regulator_dev *rdev)
> +{

> +static int mt6397_regulator_is_enabled(struct regulator_dev *rdev)
> +{

To repeat my comments on the last version: please use the generic regmap
operations rather than copying them.

> +	np = of_node_get(pdev->dev.parent->of_node);
> +	if (!np)
> +		return -EINVAL;
> +
> +	regulators = of_get_child_by_name(np, "regulators");

To further repeat my previous review comments:

| Define regulators_node and of_match in the regulator desc and you can
| remove both this table and all your DT matching code in the driver, the
| core will handle it for you.

Please don't ignore review comments.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v2 8/8] ARM: dts: mt8135: Add support for MT6397 regulator

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1813 bytes --]

On Fri, Nov 28, 2014 at 11:54:34AM +0800, Flora Fu wrote:

> Add device tree for MT6397 regulators in mt8135.dtsi.
> 
> Signed-off-by: Flora Fu <flora.fu@mediatek.com>
> ---
>  arch/arm/boot/dts/mt8135.dtsi | 192 ++++++++++++++++++++++++++++++++++++++++++

This appears to be the DT fragment for a SoC but you are defining the
system integration for the PMIC.  That's bad, the PMIC is a separate
device so should be hooked up by the board using it.  If there's common
elements from a reference design they should be in their own .dtsi.

> +					mt6397_vsramca15_reg: buck_vsramca15 {
> +						regulator-name = "vsramca15";
> +						regulator-min-microvolt = < 700000>;
> +						regulator-max-microvolt = <1493750>;
> +						regulator-ramp-delay = <12500>;
> +						regulator-enable-ramp-delay = <115>;
> +						regulator-always-on;
> +						regulator-boot-on;

Why do these regulators have both a board specific ramp delay specified
and -always-on?  If they are always on presumably they never ramp at
runtime; if this is a generic parameter for the device it should be in
the driver.

Similarly why is boot_on being specified - we can read the startup state
for all these regulators?

> +					};
> +
> +					mt6397_vsramca7_reg: buck_vsramca7 {
> +						regulator-name = "vsramca7";
> +						regulator-min-microvolt = < 700000>;
> +						regulator-max-microvolt = <1493750>;

All these regulators seem to have exactly the same range specified which
looks awfully like it might be the maximum variability the regulator has
rather than the board specific range that's supported; the fact that
they appear to have no consumers that might vary the voltage is another
warning sign.  This is probably wrong, the constraints should be
whatever is verified good for the board.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v2 6/8] dt-bindings: Add document for MT6397 regulator

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 506 bytes --]

On Fri, Nov 28, 2014 at 11:54:32AM +0800, Flora Fu wrote:

> +- compatible: "mediatek,mt6397"
> +- regulators: This is the list of child nodes that specify the regulator
> +  initialization data for defined regulators. The definition for each of these
> +  nodes is defined using the standard binding for regulators found at
> +  Documentation/devicetree/bindings/regulator/regulator.txt.

You need to specify which regulator names are valid and how they
correspond to the hardware as part of the binding.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v2 1/8] soc: mediatek: Add PMIC wrapper for MT8135 and MT6397 SoC

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 325 bytes --]

On Fri, Nov 28, 2014 at 11:54:27AM +0800, Flora Fu wrote:

> +config MT8135_PMIC_WRAP
> +	tristate "MediaTek MT8135 PMIC Wrapper Support"
> +	depends on ARCH_MEDIATEK
> +	help

> +#include <linux/regmap.h>

This uses regmap but doesn't select anything from the regmap framework
so won't build if nothing else selects regmap.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v2 8/8] ARM: dts: mt8135: Add support for MT6397 regulator

[Sascha Hauer]

On Mon, Dec 01, 2014 at 11:19:25AM +0800, Flora Fu wrote:
> Hi, Mark, 
> 
> On Fri, 2014-11-28 at 15:30 +0000, Mark Brown wrote: 
> > On Fri, Nov 28, 2014 at 11:54:34AM +0800, Flora Fu wrote:
> > 
> > > Add device tree for MT6397 regulators in mt8135.dtsi.
> > > 
> > > Signed-off-by: Flora Fu <flora.fu@mediatek.com>
> > > ---
> > >  arch/arm/boot/dts/mt8135.dtsi | 192 ++++++++++++++++++++++++++++++++++++++++++
> > 
> > This appears to be the DT fragment for a SoC but you are defining the
> > system integration for the PMIC.  That's bad, the PMIC is a separate
> > device so should be hooked up by the board using it.  If there's common
> > elements from a reference design they should be in their own .dtsi.
> > 
> 
> Do you mean that we should add a mt6397.dtsi and include it from
> mt8135.dtsi? For board specific, update them in mt8135.dtsi?

You can't include it from mt8135.dtsi since here you don't know if a
mt6397.dtsi is connected. You have to include both the mt8135.dtsi and
the mt6397.dtsi from the board file.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 0/8] Add Support for MediaTek PMIC MT6397 MFD Core and Regulator

[Sascha Hauer]

Hi Flora,

Your Cc list is huge. You should limit it to the lists and to the
individuals you know are interested in this series, like maintainers,
people who work on Mediatek SoCs, people who commented on previous
series.

Sascha

On Fri, Nov 28, 2014 at 11:54:26AM +0800, Flora Fu wrote:
> 
> The patch sets add support for MediaTek PMIC MT6397 MFD core and its regulator driver.
> This is hardware layout for access PMIC MT6397 from AP SoC MT8135.
> Between PMIC MT6397 and MT8135, the physical signal channel is SPI bus.
> A specific hardware called PMIC Wrapper or PWRAP to handle access protocols in both PMIC and AP side.
> 
> +-----------------+           +---------------+
> |                 |           |               |
> | Mediatek AP SoC |           |               |
> | (ex. MT8135)    |           |    MT6397     |
> |                 |           |               |
> |      +--------+ | (SPI bus) | +--------+    |
> |      |        | |-----------| |        |    |
> |      |  PMIC  | |-----------| |  PMIC  |    |
> |      | Wrapper| |-----------| | Wrapper|    |
> |      |        | |-----------| |        |    |
> |      +--------+ |           | +--------+    |
> |                 |           |               |
> +-----------------+           +---------------+
> 
> Changes since v1
> ================
> (1) Patch 1/8: Add MT8135 PMIC wrapper driver for SoC's proprietary hardware.
> (2) Patch 2/8: Update patch of MT6397 MFD driver to contain only MFD related codes and fix defeat of coding styles.
> (3) Patch 3/8: Update MT6397 regulator driver
> 	- use helpers and standard ways for specifying data in regulator description.
> 	- add more comments to explaining why the driver implement its own regulator_ops for ".is_enabled", ".set_voltage_sel" and ".get_voltage_sel".
> 	- update driver implement for coding styles.
> (4) Patch 4/8: Add document for MT8135 PMIC wrapper.
> (5) Patch 5/8: Update document for MT6397 MFD.
> (6) Patch 6/8: Update document for MT6397 regulators.
> (7) Patch 7/8: Add device tree for MT6397 MFD in mt8135.dtsi.
> (8) Patch 8/8: Update device tree for MT6397 regulators in mt8135.dtsi.
> 
> Initial version (v1):
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/302984.html
> 
> This driver is based on 3.18-rc1.
> 
> Flora Fu (8):
>   soc: mediatek: Add PMIC wrapper for MT8135 and MT6397 SoC
>   mfd: MT6397: Add support for PMIC MT6397 MFD
>   regulator: MT6397: Add support for MT6397 regulator
>   dt-bindings:: Add document for MT8135 PMIC Wrapper
>   dt-bindings: Add document for MT6397 MFD
>   dt-bindings: Add document for MT6397 regulator
>   ARM: dts: mt8135: Add support for PMIC MT6397 MFD
>   ARM: dts: mt8135: Add support for MT6397 regulator
> 
>  Documentation/devicetree/bindings/mfd/mt6397.txt   |  73 ++
>  .../bindings/regulator/mt6397-regulator.txt        |  32 +
>  .../soc/mediatek/mediatek,mt8135-pwrap.txt         |  79 ++
>  arch/arm/boot/dts/mt8135.dtsi                      | 211 ++++++
>  drivers/mfd/Kconfig                                |  10 +
>  drivers/mfd/Makefile                               |   1 +
>  drivers/mfd/mt6397-core.c                          |  84 ++
>  drivers/regulator/Kconfig                          |   9 +
>  drivers/regulator/Makefile                         |   2 +-
>  drivers/regulator/mt6397-regulator.c               | 369 +++++++++
>  drivers/soc/Kconfig                                |   1 +
>  drivers/soc/Makefile                               |   1 +
>  drivers/soc/mediatek/Kconfig                       |  11 +
>  drivers/soc/mediatek/Makefile                      |   1 +
>  drivers/soc/mediatek/mt8135-pmic-wrap.c            | 844 +++++++++++++++++++++
>  drivers/soc/mediatek/mt8135-pmic-wrap.h            | 138 ++++
>  include/linux/mfd/mt6397/core.h                    |  23 +
>  include/linux/mfd/mt6397/registers.h               | 362 +++++++++
>  include/linux/regulator/mt6397-regulator.h         |  49 ++
>  include/linux/soc/mediatek/mtk-pmic-wrap.h         |  25 +
>  20 files changed, 2324 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/mfd/mt6397.txt
>  create mode 100644 Documentation/devicetree/bindings/regulator/mt6397-regulator.txt
>  create mode 100644 Documentation/devicetree/bindings/soc/mediatek/mediatek,mt8135-pwrap.txt
>  create mode 100644 drivers/mfd/mt6397-core.c
>  create mode 100644 drivers/regulator/mt6397-regulator.c
>  create mode 100644 drivers/soc/mediatek/Kconfig
>  create mode 100644 drivers/soc/mediatek/Makefile
>  create mode 100644 drivers/soc/mediatek/mt8135-pmic-wrap.c
>  create mode 100644 drivers/soc/mediatek/mt8135-pmic-wrap.h
>  create mode 100644 include/linux/mfd/mt6397/core.h
>  create mode 100644 include/linux/mfd/mt6397/registers.h
>  create mode 100644 include/linux/regulator/mt6397-regulator.h
>  create mode 100644 include/linux/soc/mediatek/mtk-pmic-wrap.h
> 
> --
> 1.8.1.1.dirty
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 8/8] ARM: dts: mt8135: Add support for MT6397 regulator

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 748 bytes --]

On Mon, Dec 01, 2014 at 11:19:25AM +0800, Flora Fu wrote:
> On Fri, 2014-11-28 at 15:30 +0000, Mark Brown wrote: 

> > This appears to be the DT fragment for a SoC but you are defining the
> > system integration for the PMIC.  That's bad, the PMIC is a separate
> > device so should be hooked up by the board using it.  If there's common
> > elements from a reference design they should be in their own .dtsi.

> Do you mean that we should add a mt6397.dtsi and include it from
> mt8135.dtsi? For board specific, update them in mt8135.dtsi? 

More likely you should just add any integration needed in the board
file.  There should be no need to specify anything generically for the
regulators on a PMIC except registering the device.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v2 2/8] mfd: MT6397: Add support for PMIC MT6397 MFD

[Lee Jones]

On Fri, 28 Nov 2014, Flora Fu wrote:

> 
> Add core files for MT6397 MFD driver.
> 
> Signed-off-by: Flora Fu <flora.fu@mediatek.com>
> ---
>  drivers/mfd/Kconfig                  |  10 +
>  drivers/mfd/Makefile                 |   1 +
>  drivers/mfd/mt6397-core.c            |  84 ++++++++
>  include/linux/mfd/mt6397/core.h      |  23 +++
>  include/linux/mfd/mt6397/registers.h | 362 +++++++++++++++++++++++++++++++++++
>  5 files changed, 480 insertions(+)
>  create mode 100644 drivers/mfd/mt6397-core.c
>  create mode 100644 include/linux/mfd/mt6397/core.h
>  create mode 100644 include/linux/mfd/mt6397/registers.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 1456ea7..f0b3efc 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1318,6 +1318,16 @@ config MFD_STW481X
>  	  in various ST Microelectronics and ST-Ericsson embedded
>  	  Nomadik series.
>  
> +config MFD_MT6397
> +	tristate "MediaTek MT6397 PMIC Support"
> +	select MFD_CORE
> +	select IRQ_DOMAIN
> +	help
> +	  Say yes here to add support for MediaTek MT6397 PMIC. This is
> +	  a Power Management IC. This driver provides common support for
> +	  accessing the device; additional drivers must be enabled in order
> +	  to use the functionality of the device.
> +
>  menu "Multimedia Capabilities Port drivers"
>  	depends on ARCH_SA1100
>  
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 8bd54b1..7168193 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -177,3 +177,4 @@ obj-$(CONFIG_MFD_HI6421_PMIC)	+= hi6421-pmic-core.o
>  
>  intel-soc-pmic-objs		:= intel_soc_pmic_core.o intel_soc_pmic_crc.o
>  obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
> +obj-$(CONFIG_MFD_MT6397)	+= mt6397-core.o
> diff --git a/drivers/mfd/mt6397-core.c b/drivers/mfd/mt6397-core.c
> new file mode 100644
> index 0000000..d1a6913
> --- /dev/null
> +++ b/drivers/mfd/mt6397-core.c
> @@ -0,0 +1,84 @@
> +/*
> + * Copyright (c) 2014 MediaTek Inc.
> + * Author: Flora Fu <flora.fu@mediatek.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * 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.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/mt6397/core.h>
> +#include <linux/soc/mediatek/mtk-pmic-wrap.h>
> +
> +static const struct mfd_cell mt6397_devs[] = {
> +	{ .name = "mt6397-rtc" },
> +	{ .name = "mt6397-regulator" },
> +	{
> +		.name = "mt6397-codec",
> +		.of_compatible = "mediatek,mt6397-codec"
> +	},
> +};
> +
> +static int mt6397_probe(struct platform_device *pdev)
> +{
> +	u32 ret;
> +	struct mt6397_chip *mt6397;
> +	struct pmic_wrapper *wrp;
> +
> +	/* mt6397 MFD is child device of soc pmic wrapper. */
> +	wrp = dev_get_drvdata(pdev->dev.parent);

You might want to check this prior to dereferencing it.

> +	mt6397 = devm_kzalloc(&pdev->dev,
> +			sizeof(*mt6397), GFP_KERNEL);

Why is this wrapped?

> +	if (!mt6397)
> +		return -ENOMEM;
> +
> +	mt6397->dev = &pdev->dev;

Is this used else where?  If not, I think you can remove it.

> +	mt6397->regmap = wrp->regmap;
> +	platform_set_drvdata(pdev, mt6397);

Then you can platform_set_drvdata(pdev, regmap);

Although I don't see this being used either.  Is it used in the child
devices?

> +	ret = mfd_add_devices(mt6397->dev, -1, &mt6397_devs[0],
> +			ARRAY_SIZE(mt6397_devs), NULL, 0, NULL);
> +	if (ret)
> +		dev_err(mt6397->dev, "failed to add child devices: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static int mt6397_remove(struct platform_device *pdev)
> +{
> +	struct mt6397_chip *mt6397 = platform_get_drvdata(pdev);

No need for this.

> +	mfd_remove_devices(mt6397->dev);

Just use &pdev->dev, as you did when you registered.

> +	return 0;
> +}

[...]

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

Re: [PATCH v2 3/8] regulator: MT6397: Add support for MT6397 regulator

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 4338 bytes --]

On Mon, Dec 01, 2014 at 10:51:44AM +0800, Flora Fu wrote:
> On Fri, 2014-11-28 at 15:22 +0000, Mark Brown wrote:

> > Why does nothing else in the driver know about this "hw control mode" -
> > what does it actually mean, shouldn't it affect some of the other
> > operations?

> In the regulator, we have two parts of registers to control the output
> in voltage selection. The mode setting is done in boot loader stage
> before kernel.

> The hw control mode is used for external signal to control voltage
> selection. When the hw control mode is chosen, "voselon" register is the
> action register to do voltage selection if consumer make voltage
> selection. Without hw control mode, vsel_reg is the action register.
> To fit all mode selection, we update and sync two parts of registers in
> regulator framework.

There must be more going on in hardware control mode than that, for
example this "external signal to control voltage selection" must be able
to pick between at least different voltages otherwise it wouldn't seem
to make sense.  I'm concerned that this implementation is making some
assumption about the way in which the hardware control is being used in
a particular system and will be broken in other systems, for example if
something does start actively trying to vary the voltage.

> > > +static int mt6397_buck_get_voltage_sel(struct regulator_dev *rdev)
> > > +{

> > > +static int mt6397_regulator_is_enabled(struct regulator_dev *rdev)
> > > +{

> > To repeat my comments on the last version: please use the generic regmap
> > operations rather than copying them.

> The generic helper function does not fit usage of the regulator. 
> In general function, it considers that the vsel_reg for selection
> voltage is also the register for querying voltage selection. The enable
> bit for enable function is the bit for querying the status. 

> In the hardware design, the output of voltage selection register is
> different from vsel_reg. Is is located in nivosel. The enable bit is
> locate in the other bit called "qi_mask".  

Since you've currently got a custom set voltage function there's no
reason not to use this as the register that gets passed to the core and
hence to use the generic operation.  However I am a bit concerned about
this - what is this actually reporting?  Is it just something that
switches between the hardware and software control voltage automatically
or is it doing some kind of measurment of the output and reporting that?
The general idea is that get_voltage() should report the configured
voltage, monitoring of performance should be done via hwmon or similar.

Similarly for the enable state; we're looking for the state requested by
software not the current state of the regulator - that should be
reported via get_status() for use in error handling cases.

> > > +	np = of_node_get(pdev->dev.parent->of_node);
> > > +	if (!np)
> > > +		return -EINVAL;

> > > +	regulators = of_get_child_by_name(np, "regulators");

> > To further repeat my

> >  previous review comments:

> > | Define regulators_node and of_match in the regulator desc and you can
> > | remove both this table and all your DT matching code in the driver, the
> > | core will handle it for you.

> > Please don't ignore review comments.

> Sure, I think I completely misunderstood what you meant. Could you give
> more details about the comments?
> In this version, the table for DT matching is removed and merged into
> regulator info in table mt6397_regulators. To register every regulator
> by devm_regulator_register(), the of_node is parsed from
> of_regulator_match() by name. Here is to retrieves the device_node
> "regulators" for of_regulator_match() to get all regulator_init_data and
> corresponding of_node.
> Is any other mechanism I can use to achieve these part without
> of_regulstor_match()?

What I'm understanding from the above is that the code currently only
registers regulators that are listed in DT (I've not read it in detail).
This is not what should be happening, the driver should always register
all the regulators for the device regardless of if they are mentioned in
a DT somewhere.  That way people can read what's currently set in the
hardware which is useful for bringup and diagnostics.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v2 2/8] mfd: MT6397: Add support for PMIC MT6397 MFD

[Flora Fu]

On Mon, 2014-12-01 at 11:47 +0000, Lee Jones wrote:
> On Fri, 28 Nov 2014, Flora Fu wrote:

> 
> > +	if (!mt6397)
> > +		return -ENOMEM;
> > +
> > +	mt6397->dev = &pdev->dev;
> 
> Is this used else where?  If not, I think you can remove it.

No one is used it in this patch sets but it will be used for irq
functions to get irq id or print logs for error case in the future.
Can I keep it in the patch?

> 
> > +	mt6397->regmap = wrp->regmap;
> > +	platform_set_drvdata(pdev, mt6397);
> 
> Then you can platform_set_drvdata(pdev, regmap);
> 
> Although I don't see this being used either.  Is it used in the child
> devices?
> 
Yes, it is used to provide regmap handle for its child device. In the
patch set, regulator uses it.

Thanks,
Flora

Re: [PATCH v2 2/8] mfd: MT6397: Add support for PMIC MT6397 MFD

[Lee Jones]

On Wed, 03 Dec 2014, Flora Fu wrote:

> On Mon, 2014-12-01 at 11:47 +0000, Lee Jones wrote:
> > On Fri, 28 Nov 2014, Flora Fu wrote:
> 
> > 
> > > +	if (!mt6397)
> > > +		return -ENOMEM;
> > > +
> > > +	mt6397->dev = &pdev->dev;
> > 
> > Is this used else where?  If not, I think you can remove it.
> 
> No one is used it in this patch sets but it will be used for irq
> functions to get irq id or print logs for error case in the future.
> Can I keep it in the patch?

It's not uncommon for devices to do this and if you're sure it's going
to be used in the future you may as well keep it in.

> > > +	mt6397->regmap = wrp->regmap;
> > > +	platform_set_drvdata(pdev, mt6397);
> > 
> > Then you can platform_set_drvdata(pdev, regmap);
> > 
> > Although I don't see this being used either.  Is it used in the child
> > devices?
> > 
> Yes, it is used to provide regmap handle for its child device. In the
> patch set, regulator uses it.

Okay.

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