Re: [PATCH 2/3] mfd: Add hi6421 PMIC core driver

2014-09-01 Thread Guodong Xu 


On 08/27/2014 10:11 PM, Lee Jones 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 
>> ---
> 
> The patch is missing version information normally indicated to in the
> £SUBJECT line with a full description added here.  This looks like a
> first submission, but we both know that it's not. 
> 

Ok. I will add it back, continuing the versions. Actually I purposely
restarted version number in this submission because the Hi6421 regulator
driver part has been accepted in v5, so this patchset is not inheriting
'exactly' to the previous ones. But seems I am thinking in a wrong way.

I will take this as my v7. And next patchset I send will be v8.

Thanks.

>>  drivers/mfd/Kconfig |  13 +
>>  drivers/mfd/Makefile|   1 +
>>  drivers/mfd/hi6421-pmic-core.c  | 112 
>> 
>>  include/linux/mfd/hi6421-pmic.h |  42 +++
>>  4 files changed, 168 insertions(+)
>>  create mode 100644 drivers/mfd/hi6421-pmic-core.c
>>  create mode 100644 include/linux/mfd/hi6421-pmic.h
> 
> [...]
> 
>> diff --git a/drivers/mfd/hi6421-pmic-core.c b/drivers/mfd/hi6421-pmic-core.c
>> new file mode 100644
>> index 000..c6cba8f
>> --- /dev/null
>> +++ b/drivers/mfd/hi6421-pmic-core.c
>> @@ -0,0 +1,112 @@
> 
> [...]
> 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +static struct of_device_id of_hi6421_pmic_match_tbl[] = {
>> +{ .compatible = "hisilicon,hi6421-pmic", },
>> +{ },
>> +};
> 
> Unless you're going to use this to of_match() on in future, this
> should be down by the definition of 'struct platform_driver'.
> 

Ok, Will do.

>> +static int hi6421_pmic_probe(struct platform_device *pdev)
>> +{
> 
> [...]
> 
>> +/* set over-current protection debounce 8ms */
>> +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));
> 
> These might read a little better stacked and aligned.
>

Will do.

>
>> +pmic->dev = &pdev->dev;
>
> Can't you retrieve this via dev->parent?
>

It is not used. I will remove dev from 'struct hi6421_pmic'.

Thanks.

Best regards,
Guodong Xu

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3] mfd: Add hi6421 PMIC core driver

2014-08-27 Thread Lee Jones 
> 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 
> ---

The patch is missing version information normally indicated to in the
£SUBJECT line with a full description added here.  This looks like a
first submission, but we both know that it's not. 

>  drivers/mfd/Kconfig |  13 +
>  drivers/mfd/Makefile|   1 +
>  drivers/mfd/hi6421-pmic-core.c  | 112 
> 
>  include/linux/mfd/hi6421-pmic.h |  42 +++
>  4 files changed, 168 insertions(+)
>  create mode 100644 drivers/mfd/hi6421-pmic-core.c
>  create mode 100644 include/linux/mfd/hi6421-pmic.h

[...]

> diff --git a/drivers/mfd/hi6421-pmic-core.c b/drivers/mfd/hi6421-pmic-core.c
> new file mode 100644
> index 000..c6cba8f
> --- /dev/null
> +++ b/drivers/mfd/hi6421-pmic-core.c
> @@ -0,0 +1,112 @@

[...]

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +static struct of_device_id of_hi6421_pmic_match_tbl[] = {
> + { .compatible = "hisilicon,hi6421-pmic", },
> + { },
> +};

Unless you're going to use this to of_match() on in future, this
should be down by the definition of 'struct platform_driver'.

> +static int hi6421_pmic_probe(struct platform_device *pdev)
> +{

[...]

> + /* set over-current protection debounce 8ms */
> + 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));

These might read a little better stacked and aligned.

> + pmic->dev = &pdev->dev;

Can't you retrieve this via dev->parent?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/3] mfd: Add hi6421 PMIC core driver

2014-08-25 Thread Guodong Xu 
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 
---
 drivers/mfd/Kconfig |  13 +
 drivers/mfd/Makefile|   1 +
 drivers/mfd/hi6421-pmic-core.c  | 112 
 include/linux/mfd/hi6421-pmic.h |  42 +++
 4 files changed, 168 insertions(+)
 create mode 100644 drivers/mfd/hi6421-pmic-core.c
 create mode 100644 include/linux/mfd/hi6421-pmic.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index de5abf2..2de4919 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
+ Add support for HiSilicon Hi6421 PMIC. Hi6421 includes multi-
+ functions, such as regulators, RTC, codec, 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.
+ 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 000..c6cba8f
--- /dev/null
+++ b/drivers/mfd/hi6421-pmic-core.c
@@ -0,0 +1,112 @@
+/*
+ * 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
+ *
+ * Author: Guodong Xu 
+ *
+ * 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.
+ *
+ * This program is distributed in the hope 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see .
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+static struct of_device_id of_hi6421_pmic_match_tbl[] = {
+   { .compatible = "hisilicon,hi6421-pmic", },
+   { },
+};
+
+static const struct mfd_cell hi6421_devs[] = {
+   { .name = "hi6421-regulator", },
+};
+
+static struct regmap_config hi6421_regmap_config = {
+   .reg_bits = 32,
+   .reg_stride = 4,
+   .val_bits = 8,
+   .max_register = HI6421_REG_TO_BUS_ADDR(HI6421_REG_MAX),
+};
+
+static int hi6421_pmic_probe(struct platform_device *pdev)
+{
+   struct hi6421_pmic *pmic;
+   struct resource *res;
+   void __iomem *base;
+   int ret;
+
+   pmic = devm_kzalloc(&pdev->dev, sizeof(*pmic), GFP_KERNEL);
+   if (!pmic)
+   return -ENOMEM;
+
+   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+   base = devm_ioremap_resource(&pdev->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));
+   return PTR_ERR(pmic->regmap);
+   }
+
+   /* set over-current protection debounce 8ms */
+   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));
+
+   pmic->dev = &pdev->dev;
+   plat