[v6,4/6] mfd: Add hi6421 PMIC core driver

This adds driver to support HiSilicon Hi6421 PMIC. Hi6421 includes multi-
functions, such as regulators, codec, ADCs, Coulomb counter, etc.
This driver includes core APIs _only_.

Drivers for individul components, like voltage regulators, are
implemented in corresponding driver directories and files.

Registers in Hi6421 are memory mapped, so using regmap-mmio API.

Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
---
 Documentation/devicetree/bindings/mfd/hi6421.txt |  37 +++++++
 drivers/mfd/Kconfig                              |  13 +++
 drivers/mfd/Makefile                             |   1 +
 drivers/mfd/hi6421-pmic-core.c                   | 117 +++++++++++++++++++++++
 include/linux/mfd/hi6421-pmic.h                  |  39 ++++++++
 5 files changed, 207 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/hi6421.txt
 create mode 100644 drivers/mfd/hi6421-pmic-core.c
 create mode 100644 include/linux/mfd/hi6421-pmic.h

On Mon, Aug 18, 2014 at 09:09:14PM +0800, Guodong Xu wrote:
> This adds driver to support HiSilicon Hi6421 PMIC. Hi6421 includes multi-
> functions, such as regulators, codec, ADCs, Coulomb counter, etc.
> This driver includes core APIs _only_.

Please don't resend already applied patches, if there are any updates
that need applying please send incremental patches.

On Mon, 18 Aug 2014, Guodong Xu wrote:
> This adds driver to support HiSilicon Hi6421 PMIC. Hi6421 includes multi-
> functions, such as regulators, codec, ADCs, Coulomb counter, etc.
> This driver includes core APIs _only_.
> 
> Drivers for individul components, like voltage regulators, are
> implemented in corresponding driver directories and files.
> 
> Registers in Hi6421 are memory mapped, so using regmap-mmio API.
> 
> Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
> ---
>  Documentation/devicetree/bindings/mfd/hi6421.txt |  37 +++++++

DT documentation should be in a patch of its own.

See: Documentation/devicetree/bindings/submitting-patches.txt

>  drivers/mfd/Kconfig                              |  13 +++
>  drivers/mfd/Makefile                             |   1 +
>  drivers/mfd/hi6421-pmic-core.c                   | 117 +++++++++++++++++++++++
>  include/linux/mfd/hi6421-pmic.h                  |  39 ++++++++
>  5 files changed, 207 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/hi6421.txt
>  create mode 100644 drivers/mfd/hi6421-pmic-core.c
>  create mode 100644 include/linux/mfd/hi6421-pmic.h
> 
> diff --git a/Documentation/devicetree/bindings/mfd/hi6421.txt b/Documentation/devicetree/bindings/mfd/hi6421.txt
> new file mode 100644
> index 0000000..1512123
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/hi6421.txt
> @@ -0,0 +1,37 @@
> +* HI6421 Multi-Functional Device (MFD), by HiSilicon Ltd.
> +
> +Required parent device properties:
> +- reg		: register range space of hi6421;

You need to describe the compatible string here.

> +Supported Hi6421 sub-devices include:
> +
> +Device                     IRQ Names              Supply Names   Description
> +------                     ---------              ------------   -----------
> +regulators               :                      :              : Regulators

If a cell has been intentionally left blank, put 'None' to be clear.

When will the other devices be added?

> +Required child device properties:
> +None.
> +
> +Example:
> +	pmic: pmic@c00000 {

24bit addressing?

Is this node referenced by another node via phandle?  If not, you can
drop the "pmic" label.  If it is referenced by another node, but there
is more than one PMIC, label "pmic" won't do.

> +		compatible = "hisilicon,hi6421-pmic";
> +		reg = <0xc00000 0x0180>; /* 0x60 << 2 */
> +
> +		regulators {
> +			// supply for MLC NAND/ eMMC
> +			hi6421_vout0_reg: hi6421_vout0 {
> +				regulator-name = "VOUT0";

I may be mistaken, but "vout0" doesn't come across as a very good
regulator name to me.

> +				regulator-min-microvolt = <2850000>;
> +				regulator-max-microvolt = <2850000>;
> +			};
> +
> +			// supply for 26M Oscillator
> +			hi6421_vout1_reg: hi6421_vout1 {
> +				regulator-name = "VOUT1";
> +				regulator-min-microvolt = <1700000>;
> +				regulator-max-microvolt = <2000000>;
> +				regulator-boot-on;
> +				regulator-always-on;

Again an assumption here, but doesn't 'egulator-always-on' insinuate
'regulator-boot-on', or does it simply mean "once it's on, it must
stay on"?

> +			};
> +		};
> +       	};

Your tabbing is out here.

> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index de5abf2..347cbf6 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -210,6 +210,19 @@ config MFD_MC13XXX_I2C
>  	help
>  	  Select this if your MC13xxx is connected via an I2C bus.
>  
> +config MFD_HI6421_PMIC
> +	tristate "HiSilicon Hi6421 PMU/Codec IC"
> +	depends on OF
> +	select MFD_CORE
> +	select REGMAP_MMIO
> +	help
> +	  This driver supports HiSilicon Hi6421 PMIC. Hi6421 includes multi-

It doesn't support it, it adds support for it - subtle difference.

> +	  functions, such as regulators, codec, ADCs, Coulomb counter, etc.
> +	  This driver includes core APIs _only_. You have to select
> +	  individul components like voltage regulators under corresponding
> +	  menus in order to enable them.
> +	  Memory-mapped I/O is the way to communicate with Hi6421.

"We communicate with the Hi6421 via memory-mapped I/O." 

>  config HTC_EGPIO
>  	bool "HTC EGPIO support"
>  	depends on GPIOLIB && ARM
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index f001487..dc59efd 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -169,6 +169,7 @@ obj-$(CONFIG_MFD_AS3711)	+= as3711.o
>  obj-$(CONFIG_MFD_AS3722)	+= as3722.o
>  obj-$(CONFIG_MFD_STW481X)	+= stw481x.o
>  obj-$(CONFIG_MFD_IPAQ_MICRO)	+= ipaq-micro.o
> +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
> diff --git a/drivers/mfd/hi6421-pmic-core.c b/drivers/mfd/hi6421-pmic-core.c
> new file mode 100644
> index 0000000..2be4d3f
> --- /dev/null
> +++ b/drivers/mfd/hi6421-pmic-core.c
> @@ -0,0 +1,117 @@
> +/*
> + * Device driver for Hi6421 IC
> + *
> + * Copyright (c) <2011-2014> HiSilicon Technologies Co., Ltd.
> + *              http://www.hisilicon.com
> + * Copyright (c) <2013-2014> Linaro Ltd.
> + *              http://www.linaro.org

I'm not sure Linaro own the copyright to this driver.  It should still
belong to HiSilicon.

> + * Author: Guodong Xu <guodong.xu@linaro.org>
> + *
> + * 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 should also contain a link to the full licence.

See: COPYING

> + */
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/mfd/hi6421-pmic.h>
> +
> +static struct of_device_id of_hi6421_pmic_match_tbl[] = {
> +	{
> +		.compatible = "hisilicon,hi6421-pmic",
> +	},

This can be just one line.

> +	{ /* end */ }

No real need for this comment.

> +};
> +
> +static const struct mfd_cell hi6421_devs[] = {
> +	{
> +		.name = "hi6421-regulator",
> +	},

Again, one line.

> +};
> +
> +static struct regmap_config hi6421_regmap_config = {
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 8,
> +	.max_register = HI6421_REG_TO_BUS_ADDR(0xFF),

0xFF should really be defined.

> +};
> +
> +static int hi6421_pmic_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;

You only use this a couple of times.  You use '&pdev->dev' more
regularly. May as well remove it and just use '&pdev->dev' all
the time.

> +	struct hi6421_pmic *pmic = NULL;

No need to initialise this.

> +	struct resource *res;
> +	void __iomem *base;
> +	int ret;
> +
> +	pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL);
> +	if (!pmic) {
> +		dev_err(dev, "cannot allocate hi6421_pmic device info\n");

No need to print out messages on OOM, the OS will do that for you.

> +		return -ENOMEM;
> +	}
> +
> +	/* get resources */

This comment doesn't add to the code.  The name of the function call
is clear enough.

> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (unlikely(!res)) {

Only 21 out of 1718 uses of platform_get_resource() use unlikely() to
check the return value.  I think it's okay to drop it.

> +		dev_err(&pdev->dev, "Invalid mem resource.\n");

"No memory resource specified"?

> +		return -ENODEV;
> +	}

Actually, scrap all that.  You can remove error checking altogether
for platform_get_resource(), as devm_ioremap_resource() does it for
you.  Code should read:

  res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
  base = devm_ioremap_resource(dev, res);
  if (IS_ERR(base))
	  return PTR_ERR(base);

> +	base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	pmic->regmap = devm_regmap_init_mmio_clk(&pdev->dev, NULL, base,
> +						 &hi6421_regmap_config);
> +	if (IS_ERR(pmic->regmap)) {
> +		dev_err(&pdev->dev, "regmap init failed: %ld\n",
> +						PTR_ERR(pmic->regmap));

Can you break the line at the first ',/' and line up against '('?

> +		return PTR_ERR(pmic->regmap);
> +	}
> +
> +	pmic->dev = dev;
> +	platform_set_drvdata(pdev, pmic);

It's not _that_ important, but I like to see this at the end after you
know everything else has succeeded.

> +	/* set over-current protection debounce 8ms*/

Should be a ' ' between '8ms' and '*/'.

> +	regmap_update_bits(pmic->regmap, HI6421_OCP_DEB_CTRL_REG,
> +		(HI6421_OCP_DEB_SEL_MASK | HI6421_OCP_EN_DEBOUNCE_MASK |
> +		 HI6421_OCP_AUTO_STOP_MASK),
> +		(HI6421_OCP_DEB_SEL_8MS | HI6421_OCP_EN_DEBOUNCE_ENABLE));
> +
> +	ret = mfd_add_devices(dev, 0, hi6421_devs,
> +			ARRAY_SIZE(hi6421_devs), NULL, 0, NULL);

I'd like to see an error message if mfd_add_devices() fails.

> +	return ret;
> +}
> +
> +static int hi6421_pmic_remove(struct platform_device *pdev)
> +{
> +	mfd_remove_devices(&pdev->dev);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver hi6421_pmic_driver = {
> +	.driver = {
> +		.name	= "hi6421_pmic",
> +		.owner  = THIS_MODULE,

This is taken care of for you, you can remove it.

> +		.of_match_table = of_hi6421_pmic_match_tbl,
> +	},
> +	.probe	= hi6421_pmic_probe,
> +	.remove	= hi6421_pmic_remove,
> +};
> +module_platform_driver(hi6421_pmic_driver);
> +
> +MODULE_AUTHOR("Guodong Xu <guodong.xu@linaro.org>");
> +MODULE_DESCRIPTION("Hi6421 PMIC driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/hi6421-pmic.h b/include/linux/mfd/hi6421-pmic.h
> new file mode 100644
> index 0000000..36bcb34
> --- /dev/null
> +++ b/include/linux/mfd/hi6421-pmic.h
> @@ -0,0 +1,39 @@
> +/*
> + * Header file for device driver Hi6421 PMIC
> + *
> + * Copyright (c) <2011-2014> HiSilicon Technologies Co., Ltd.
> + *              http://www.hisilicon.com
> + * Copyright (c) <2013-2014> Linaro Ltd.
> + *              http://www.linaro.org
> + *
> + * Author: Guodong Xu <guodong.xu@linaro.org>
> + *
> + * 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.
> + */
> +
> +#ifndef	__HI6421_PMIC_H
> +#define	__HI6421_PMIC_H
> +
> +/* Hi6421 registers are mapped to memory bus in 4 bytes stride */
> +#define HI6421_REG_TO_BUS_ADDR(x)	(x << 2)
> +
> +/* Hi6421 OCP (over current protection) and DEB (debounce) control register */
> +#define	HI6421_OCP_DEB_CTRL_REG		HI6421_REG_TO_BUS_ADDR(0x51)
> +#define	HI6421_OCP_DEB_SEL_MASK		(0x0C)
> +#define HI6421_OCP_DEB_SEL_8MS		(0x00)
> +#define HI6421_OCP_DEB_SEL_16MS		(0x04)
> +#define HI6421_OCP_DEB_SEL_32MS		(0x08)
> +#define HI6421_OCP_DEB_SEL_64MS		(0x0C)
> +#define HI6421_OCP_EN_DEBOUNCE_MASK	(0x02)
> +#define HI6421_OCP_EN_DEBOUNCE_ENABLE	(0x02)
> +#define HI6421_OCP_AUTO_STOP_MASK	(0x01)
> +#define HI6421_OCP_AUTO_STOP_ENABLE	(0x01)

Remove the ()'s.

> +struct hi6421_pmic {
> +	struct device		*dev;
> +	struct regmap		*regmap;
> +};
> +
> +#endif		/* __HI6421_PMIC_H */

Hi, Lee

Thanks. I added for most of your comments. Some items, I have a
different understanding. I added in below.

On 08/20/2014 04:09 PM, Lee Jones wrote:
> On Mon, 18 Aug 2014, Guodong Xu wrote:
>> This adds driver to support HiSilicon Hi6421 PMIC. Hi6421 includes multi-
>> functions, such as regulators, codec, ADCs, Coulomb counter, etc.
>> This driver includes core APIs _only_.
>>
>> Drivers for individul components, like voltage regulators, are
>> implemented in corresponding driver directories and files.
>>
>> Registers in Hi6421 are memory mapped, so using regmap-mmio API.
>>
>> Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
>> ---
>>  Documentation/devicetree/bindings/mfd/hi6421.txt |  37 +++++++
> 
> DT documentation should be in a patch of its own.
> 
> See: Documentation/devicetree/bindings/submitting-patches.txt
> 

Thanks. Will do.

>>  drivers/mfd/Kconfig                              |  13 +++
>>  drivers/mfd/Makefile                             |   1 +
>>  drivers/mfd/hi6421-pmic-core.c                   | 117 +++++++++++++++++++++++
>>  include/linux/mfd/hi6421-pmic.h                  |  39 ++++++++
>>  5 files changed, 207 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mfd/hi6421.txt
>>  create mode 100644 drivers/mfd/hi6421-pmic-core.c
>>  create mode 100644 include/linux/mfd/hi6421-pmic.h
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/hi6421.txt b/Documentation/devicetree/bindings/mfd/hi6421.txt
>> new file mode 100644
>> index 0000000..1512123
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/hi6421.txt
>> @@ -0,0 +1,37 @@
>> +* HI6421 Multi-Functional Device (MFD), by HiSilicon Ltd.
>> +
>> +Required parent device properties:
>> +- reg		: register range space of hi6421;
> 
> You need to describe the compatible string here.
> 

Yes.

>> +Supported Hi6421 sub-devices include:
>> +
>> +Device                     IRQ Names              Supply Names   Description
>> +------                     ---------              ------------   -----------
>> +regulators               :                      :              : Regulators
> 
> If a cell has been intentionally left blank, put 'None' to be clear.
> 
> When will the other devices be added?
> 

RTC and irq will follow up next Month.

>> +Required child device properties:
>> +None.
>> +
>> +Example:
>> +	pmic: pmic@c00000 {
> 
> 24bit addressing?
> 

No. that's due to a range property in parent node. I will change it to
32-bit address.

> Is this node referenced by another node via phandle?  If not, you can
> drop the "pmic" label.  If it is referenced by another node, but there
> is more than one PMIC, label "pmic" won't do.
> 

ok. Will change node name to
 +               hi6421 {
...

>> +		compatible = "hisilicon,hi6421-pmic";
>> +		reg = <0xc00000 0x0180>; /* 0x60 << 2 */
>> +
>> +		regulators {
>> +			// supply for MLC NAND/ eMMC
>> +			hi6421_vout0_reg: hi6421_vout0 {
>> +				regulator-name = "VOUT0";
> 
> I may be mistaken, but "vout0" doesn't come across as a very good
> regulator name to me.
>

Right, but 'VOUT0' is the name used in hi4511 boards' schematic. So I
used this name to facilitate dts and board design matching.


>> +				regulator-min-microvolt = <2850000>;
>> +				regulator-max-microvolt = <2850000>;
>> +			};
>> +
>> +			// supply for 26M Oscillator
>> +			hi6421_vout1_reg: hi6421_vout1 {
>> +				regulator-name = "VOUT1";
>> +				regulator-min-microvolt = <1700000>;
>> +				regulator-max-microvolt = <2000000>;
>> +				regulator-boot-on;
>> +				regulator-always-on;
> 
> Again an assumption here, but doesn't 'egulator-always-on' insinuate
> 'regulator-boot-on', or does it simply mean "once it's on, it must
> stay on"?
>

From Documentation/devicetree/bindings/regulator/regulator.txt,
- regulator-boot-on: bootloader/firmware enabled regulator
- regulator-always-on: boolean, regulator should never be disabled

'regulator-boot-on' is describing a board status, so I added it into dts
file. Although in kernel code there is no much operation depending on it.

>> +			};
>> +		};
>> +       	};
> 
> Your tabbing is out here.
> 

Sorry. will fix.

>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index de5abf2..347cbf6 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -210,6 +210,19 @@ config MFD_MC13XXX_I2C
>>  	help
>>  	  Select this if your MC13xxx is connected via an I2C bus.
>>  
>> +config MFD_HI6421_PMIC
>> +	tristate "HiSilicon Hi6421 PMU/Codec IC"
>> +	depends on OF
>> +	select MFD_CORE
>> +	select REGMAP_MMIO
>> +	help
>> +	  This driver supports HiSilicon Hi6421 PMIC. Hi6421 includes multi-
> 
> It doesn't support it, it adds support for it - subtle difference.
> 

Thanks, will fix.

>> +	  functions, such as regulators, codec, ADCs, Coulomb counter, etc.
>> +	  This driver includes core APIs _only_. You have to select
>> +	  individul components like voltage regulators under corresponding
>> +	  menus in order to enable them.
>> +	  Memory-mapped I/O is the way to communicate with Hi6421.
> 
> "We communicate with the Hi6421 via memory-mapped I/O." 
> 
>>  config HTC_EGPIO
>>  	bool "HTC EGPIO support"
>>  	depends on GPIOLIB && ARM
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index f001487..dc59efd 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -169,6 +169,7 @@ obj-$(CONFIG_MFD_AS3711)	+= as3711.o
>>  obj-$(CONFIG_MFD_AS3722)	+= as3722.o
>>  obj-$(CONFIG_MFD_STW481X)	+= stw481x.o
>>  obj-$(CONFIG_MFD_IPAQ_MICRO)	+= ipaq-micro.o
>> +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
>> diff --git a/drivers/mfd/hi6421-pmic-core.c b/drivers/mfd/hi6421-pmic-core.c
>> new file mode 100644
>> index 0000000..2be4d3f
>> --- /dev/null
>> +++ b/drivers/mfd/hi6421-pmic-core.c
>> @@ -0,0 +1,117 @@
>> +/*
>> + * Device driver for Hi6421 IC
>> + *
>> + * Copyright (c) <2011-2014> HiSilicon Technologies Co., Ltd.
>> + *              http://www.hisilicon.com
>> + * Copyright (c) <2013-2014> Linaro Ltd.
>> + *              http://www.linaro.org
> 
> I'm not sure Linaro own the copyright to this driver.  It should still
> belong to HiSilicon.
> 

That was discussed with HiSilicon and agreement was we need both. The
driver code is a complete re-write by me. HiSilicon original driver is
reference for functionality understanding purpose.

>> + * Author: Guodong Xu <guodong.xu@linaro.org>
>> + *
>> + * 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 should also contain a link to the full licence.
> 
> See: COPYING
> 

Thanks. I checked COPYING, but there is no 'link' to full license. I
copied a link from other c source: http://www.gnu.org/licenses/
is that OK?

>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +#include <linux/mfd/hi6421-pmic.h>
>> +
>> +static struct of_device_id of_hi6421_pmic_match_tbl[] = {
>> +	{
>> +		.compatible = "hisilicon,hi6421-pmic",
>> +	},
> 
> This can be just one line.
> 
>> +	{ /* end */ }
> 
> No real need for this comment.
> 

Fixed.

>> +};
>> +
>> +static const struct mfd_cell hi6421_devs[] = {
>> +	{
>> +		.name = "hi6421-regulator",
>> +	},
> 
> Again, one line.
> 
>> +};
>> +
>> +static struct regmap_config hi6421_regmap_config = {
>> +	.reg_bits = 32,
>> +	.reg_stride = 4,
>> +	.val_bits = 8,
>> +	.max_register = HI6421_REG_TO_BUS_ADDR(0xFF),
> 
> 0xFF should really be defined.
> 
>> +};
>> +
>> +static int hi6421_pmic_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
> 
> You only use this a couple of times.  You use '&pdev->dev' more
> regularly. May as well remove it and just use '&pdev->dev' all
> the time.
> 

Right. Will fix.

>> +	struct hi6421_pmic *pmic = NULL;
> 
> No need to initialise this.
> 
>> +	struct resource *res;
>> +	void __iomem *base;
>> +	int ret;
>> +
>> +	pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL);
>> +	if (!pmic) {
>> +		dev_err(dev, "cannot allocate hi6421_pmic device info\n");
> 
> No need to print out messages on OOM, the OS will do that for you.
> 
>> +		return -ENOMEM;
>> +	}
>> +
>> +	/* get resources */
> 
> This comment doesn't add to the code.  The name of the function call
> is clear enough.
> 
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (unlikely(!res)) {
> 
> Only 21 out of 1718 uses of platform_get_resource() use unlikely() to
> check the return value.  I think it's okay to drop it.
> 
>> +		dev_err(&pdev->dev, "Invalid mem resource.\n");
> 
> "No memory resource specified"?
> 
>> +		return -ENODEV;
>> +	}
> 
> Actually, scrap all that.  You can remove error checking altogether
> for platform_get_resource(), as devm_ioremap_resource() does it for
> you.  Code should read:
> 

Thanks. Will do.

>   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>   base = devm_ioremap_resource(dev, res);
>   if (IS_ERR(base))
> 	  return PTR_ERR(base);
> 
>> +	base = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(base))
>> +		return PTR_ERR(base);
>> +
>> +	pmic->regmap = devm_regmap_init_mmio_clk(&pdev->dev, NULL, base,
>> +						 &hi6421_regmap_config);
>> +	if (IS_ERR(pmic->regmap)) {
>> +		dev_err(&pdev->dev, "regmap init failed: %ld\n",
>> +						PTR_ERR(pmic->regmap));
> 
> Can you break the line at the first ',/' and line up against '('?
> 
>> +		return PTR_ERR(pmic->regmap);
>> +	}
>> +
>> +	pmic->dev = dev;
>> +	platform_set_drvdata(pdev, pmic);
> 
> It's not _that_ important, but I like to see this at the end after you
> know everything else has succeeded.
>

When I move this after mfd_add_devices(), it fails to boot. In mfd
devices's probe, pmic->regmap is used.


>> +	/* set over-current protection debounce 8ms*/
> 
> Should be a ' ' between '8ms' and '*/'.
> 
>> +	regmap_update_bits(pmic->regmap, HI6421_OCP_DEB_CTRL_REG,
>> +		(HI6421_OCP_DEB_SEL_MASK | HI6421_OCP_EN_DEBOUNCE_MASK |
>> +		 HI6421_OCP_AUTO_STOP_MASK),
>> +		(HI6421_OCP_DEB_SEL_8MS | HI6421_OCP_EN_DEBOUNCE_ENABLE));
>> +
>> +	ret = mfd_add_devices(dev, 0, hi6421_devs,
>> +			ARRAY_SIZE(hi6421_devs), NULL, 0, NULL);
> 
> I'd like to see an error message if mfd_add_devices() fails.
> 

Will do.

>> +	return ret;
>> +}
>> +
>> +static int hi6421_pmic_remove(struct platform_device *pdev)
>> +{
>> +	mfd_remove_devices(&pdev->dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver hi6421_pmic_driver = {
>> +	.driver = {
>> +		.name	= "hi6421_pmic",
>> +		.owner  = THIS_MODULE,
> 
> This is taken care of for you, you can remove it.
> 

Will do.

>> +		.of_match_table = of_hi6421_pmic_match_tbl,
>> +	},
>> +	.probe	= hi6421_pmic_probe,
>> +	.remove	= hi6421_pmic_remove,
>> +};
>> +module_platform_driver(hi6421_pmic_driver);
>> +
>> +MODULE_AUTHOR("Guodong Xu <guodong.xu@linaro.org>");
>> +MODULE_DESCRIPTION("Hi6421 PMIC driver");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/mfd/hi6421-pmic.h b/include/linux/mfd/hi6421-pmic.h
>> new file mode 100644
>> index 0000000..36bcb34
>> --- /dev/null
>> +++ b/include/linux/mfd/hi6421-pmic.h
>> @@ -0,0 +1,39 @@
>> +/*
>> + * Header file for device driver Hi6421 PMIC
>> + *
>> + * Copyright (c) <2011-2014> HiSilicon Technologies Co., Ltd.
>> + *              http://www.hisilicon.com
>> + * Copyright (c) <2013-2014> Linaro Ltd.
>> + *              http://www.linaro.org
>> + *
>> + * Author: Guodong Xu <guodong.xu@linaro.org>
>> + *
>> + * 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.
>> + */
>> +
>> +#ifndef	__HI6421_PMIC_H
>> +#define	__HI6421_PMIC_H
>> +
>> +/* Hi6421 registers are mapped to memory bus in 4 bytes stride */
>> +#define HI6421_REG_TO_BUS_ADDR(x)	(x << 2)
>> +
>> +/* Hi6421 OCP (over current protection) and DEB (debounce) control register */
>> +#define	HI6421_OCP_DEB_CTRL_REG		HI6421_REG_TO_BUS_ADDR(0x51)
>> +#define	HI6421_OCP_DEB_SEL_MASK		(0x0C)
>> +#define HI6421_OCP_DEB_SEL_8MS		(0x00)
>> +#define HI6421_OCP_DEB_SEL_16MS		(0x04)
>> +#define HI6421_OCP_DEB_SEL_32MS		(0x08)
>> +#define HI6421_OCP_DEB_SEL_64MS		(0x0C)
>> +#define HI6421_OCP_EN_DEBOUNCE_MASK	(0x02)
>> +#define HI6421_OCP_EN_DEBOUNCE_ENABLE	(0x02)
>> +#define HI6421_OCP_AUTO_STOP_MASK	(0x01)
>> +#define HI6421_OCP_AUTO_STOP_ENABLE	(0x01)
> 
> Remove the ()'s.
> 

Will do.

Thanks.
Guodong

>> +struct hi6421_pmic {
>> +	struct device		*dev;
>> +	struct regmap		*regmap;
>> +};
>> +
>> +#endif		/* __HI6421_PMIC_H */
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, 25 Aug 2014, Guodong Xu wrote:
> On 08/20/2014 04:09 PM, Lee Jones wrote:
> > On Mon, 18 Aug 2014, Guodong Xu wrote:
> >> This adds driver to support HiSilicon Hi6421 PMIC. Hi6421 includes multi-
> >> functions, such as regulators, codec, ADCs, Coulomb counter, etc.
> >> This driver includes core APIs _only_.
> >>
> >> Drivers for individul components, like voltage regulators, are
> >> implemented in corresponding driver directories and files.
> >>
> >> Registers in Hi6421 are memory mapped, so using regmap-mmio API.
> >>
> >> Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
> >> ---
> >>  Documentation/devicetree/bindings/mfd/hi6421.txt |  37 +++++++

[...]

> >> + * Author: Guodong Xu <guodong.xu@linaro.org>
> >> + *
> >> + * 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 should also contain a link to the full licence.
> > 
> > See: COPYING
> > 
> 
> Thanks. I checked COPYING, but there is no 'link' to full license. I

I was making reference to the fact that COPYING tells you to provide a
link to the full notice:

"To do so, attach the following notices to the program.  It is safest
to attach them to the start of each source file to most effectively
convey the exclusion of warranty; and each file should have at least
the "copyright" line and a pointer to where the full notice is found."

> copied a link from other c source: http://www.gnu.org/licenses/
> is that OK?

Yes, that's fine.

> >> +	platform_set_drvdata(pdev, pmic);
> > 
> > It's not _that_ important, but I like to see this at the end after you
> > know everything else has succeeded.
> 
> When I move this after mfd_add_devices(), it fails to boot. In mfd
> devices's probe, pmic->regmap is used.

You can move it to just before mfd_add_devices().