[PATCH v4 0/2] regulator: act8945a: add regulator driver for the sub-device of ACT8945A MFD

[PATCH v4 0/2] regulator: act8945a: add regulator driver for the sub-device of ACT8945A MFD

[Wenyou Yang]

The ACT8945A is a Multi Function Device with the following subdevices:
 - Regulator
 - Charger

This patch set is to add regulator driver for ACT8945A.
It is based on the patch set:
	http://lists.infradead.org/pipermail/linux-arm-kernel/2016-January/402199.html

Changes in v4:
 - change the name "act8945a-pmic" to "act8945a-regulator"
   to align mfd subdevice's name.

Changes in v3:
 - use the core regulators_node and of_match implementation,
   remove the redundant code.
 - use the core dev_get_regmap(dev->parent) code, remove
   the redundant code.
 - change the regulator name to easily implement macro.
 - use a space after #define, not tabs.
 - add OF match table.

Changes in v2:
 - add more help information in Kconfig.
 - update the file header with short version license and author line.
 - remove unused structure, struct act8945a_pmic.
 - substitute of_property_read_bool() for of_get_property().
 - due to removing the member of stuct act8945a_dev, dev.
 - remove the unnecessary print out.
 - use module_platform_driver(), instead of subsys_initcall().
 - substitute MODULE_LICENSE("GPL") for MODULE_LICENSE("GPL v2").
 - add Acked-by Rob Herring.

Wenyou Yang (2):
  regulator: act8945a: add regulator driver for ACT8945A
  regulator: add documentation for ACT8945A's regulator DT bindings

 .../bindings/regulator/act8945a-regulator.txt      |   78 +++++++++
 drivers/regulator/Kconfig                          |    9 +
 drivers/regulator/Makefile                         |    1 +
 drivers/regulator/act8945a-regulator.c             |  172 ++++++++++++++++++++
 4 files changed, 260 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/act8945a-regulator.txt
 create mode 100644 drivers/regulator/act8945a-regulator.c

-- 
1.7.9.5

[PATCH v4 1/2] regulator: act8945a: add regulator driver for ACT8945A

[Wenyou Yang]

This patch adds new regulator driver to support ACT8945A MFD
chip's regulators.

The ACT8945A has three step-down DC/DC converters and four
low-dropout regulators.

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
---

Changes in v4:
 - change the name "act8945a-pmic" to "act8945a-regulator"
   to align mfd subdevice's name.

Changes in v3:
 - use the core regulators_node and of_match implementation,
   remove the redundant code.
 - use the core dev_get_regmap(dev->parent) code, remove
   the redundant code.
 - change the regulator name to easily implement macro.
 - use a space after #define, not tabs.
 - add OF match table.

Changes in v2:
 - add more help information in Kconfig.
 - update the file header with short version license and author line.
 - remove unused structure, struct act8945a_pmic.
 - substitute of_property_read_bool() for of_get_property().
 - due to removing the member of stuct act8945a_dev, dev.
 - remove the unnecessary print out.
 - use module_platform_driver(), instead of subsys_initcall().
 - substitute MODULE_LICENSE("GPL") for MODULE_LICENSE("GPL v2").

 drivers/regulator/Kconfig              |    9 ++
 drivers/regulator/Makefile             |    1 +
 drivers/regulator/act8945a-regulator.c |  172 ++++++++++++++++++++++++++++++++
 3 files changed, 182 insertions(+)
 create mode 100644 drivers/regulator/act8945a-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 8155e80..74a6354 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -78,6 +78,15 @@ config REGULATOR_ACT8865
 	  This driver controls a active-semi act8865 voltage output
 	  regulator via I2C bus.
 
+config REGULATOR_ACT8945A
+	tristate "Active-semi ACT8945A voltage regulator"
+	depends on MFD_ACT8945A
+	help
+	  This driver controls a active-semi ACT8945A voltage regulator
+	  via I2C bus. The ACT8945A features three step-down DC/DC converters
+	  and four low-dropout linear regulators, along with a ActivePath
+	  battery charger.
+
 config REGULATOR_AD5398
 	tristate "Analog Devices AD5398/AD5821 regulators"
 	depends on I2C
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 980b194..348cfd7 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_REGULATOR_AAT2870) += aat2870-regulator.o
 obj-$(CONFIG_REGULATOR_AB3100) += ab3100.o
 obj-$(CONFIG_REGULATOR_AB8500)	+= ab8500-ext.o ab8500.o
 obj-$(CONFIG_REGULATOR_ACT8865) += act8865-regulator.o
+obj-$(CONFIG_REGULATOR_ACT8945A) += act8945a-regulator.o
 obj-$(CONFIG_REGULATOR_AD5398) += ad5398.o
 obj-$(CONFIG_REGULATOR_ANATOP) += anatop-regulator.o
 obj-$(CONFIG_REGULATOR_ARIZONA) += arizona-micsupp.o arizona-ldo1.o
diff --git a/drivers/regulator/act8945a-regulator.c b/drivers/regulator/act8945a-regulator.c
new file mode 100644
index 0000000..990f0b7
--- /dev/null
+++ b/drivers/regulator/act8945a-regulator.c
@@ -0,0 +1,172 @@
+/*
+ * Voltage regulation driver for active-semi ACT8945A PMIC
+ *
+ * Copyright (C) 2015 Atmel Corporation
+ *
+ * Author: Wenyou Yang <wenyou.yang@atmel.com>
+ *
+ * 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.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+
+/**
+ * ACT8945A Global Register Map.
+ */
+#define ACT8945A_SYS_MODE	0x00
+#define ACT8945A_SYS_CTRL	0x01
+#define ACT8945A_DCDC1_VSET1	0x20
+#define ACT8945A_DCDC1_VSET2	0x21
+#define ACT8945A_DCDC1_CTRL	0x22
+#define ACT8945A_DCDC2_VSET1	0x30
+#define ACT8945A_DCDC2_VSET2	0x31
+#define ACT8945A_DCDC2_CTRL	0x32
+#define ACT8945A_DCDC3_VSET1	0x40
+#define ACT8945A_DCDC3_VSET2	0x41
+#define ACT8945A_DCDC3_CTRL	0x42
+#define ACT8945A_LDO1_VSET	0x50
+#define ACT8945A_LDO1_CTRL	0x51
+#define ACT8945A_LDO2_VSET	0x54
+#define ACT8945A_LDO2_CTRL	0x55
+#define ACT8945A_LDO3_VSET	0x60
+#define ACT8945A_LDO3_CTRL	0x61
+#define ACT8945A_LDO4_VSET	0x64
+#define ACT8945A_LDO4_CTRL	0x65
+
+/**
+ * Field Definitions.
+ */
+#define ACT8945A_ENA		0x80	/* ON - [7] */
+#define ACT8945A_VSEL_MASK	0x3F	/* VSET - [5:0] */
+
+/**
+ * ACT8945A Voltage Number
+ */
+#define ACT8945A_VOLTAGE_NUM	64
+
+enum {
+	ACT8945A_ID_DCDC1,
+	ACT8945A_ID_DCDC2,
+	ACT8945A_ID_DCDC3,
+	ACT8945A_ID_LDO1,
+	ACT8945A_ID_LDO2,
+	ACT8945A_ID_LDO3,
+	ACT8945A_ID_LDO4,
+	ACT8945A_REG_NUM,
+};
+
+static const struct regulator_linear_range act8945a_voltage_ranges[] = {
+	REGULATOR_LINEAR_RANGE(600000, 0, 23, 25000),
+	REGULATOR_LINEAR_RANGE(1200000, 24, 47, 50000),
+	REGULATOR_LINEAR_RANGE(2400000, 48, 63, 100000),
+};
+
+static struct regulator_ops act8945a_ops = {
+	.list_voltage		= regulator_list_voltage_linear_range,
+	.map_voltage		= regulator_map_voltage_linear_range,
+	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
+	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
+	.enable			= regulator_enable_regmap,
+	.disable		= regulator_disable_regmap,
+	.is_enabled		= regulator_is_enabled_regmap,
+};
+
+#define ACT89xx_REG(_name, _family, _id, _vsel_reg, _supply)		\
+	[_family##_ID_##_id] = {					\
+		.name			= _name,			\
+		.supply_name		= _supply,			\
+		.of_match		= of_match_ptr("REG_"#_id),	\
+		.regulators_node	= of_match_ptr("regulators"),	\
+		.id			= _family##_ID_##_id,		\
+		.type			= REGULATOR_VOLTAGE,		\
+		.ops			= &act8945a_ops,		\
+		.n_voltages		= ACT8945A_VOLTAGE_NUM,		\
+		.linear_ranges		= act8945a_voltage_ranges,	\
+		.n_linear_ranges	= ARRAY_SIZE(act8945a_voltage_ranges), \
+		.vsel_reg		= _family##_##_id##_##_vsel_reg, \
+		.vsel_mask		= ACT8945A_VSEL_MASK,		\
+		.enable_reg		= _family##_##_id##_CTRL,	\
+		.enable_mask		= ACT8945A_ENA,			\
+		.owner			= THIS_MODULE,			\
+	}
+
+static const struct regulator_desc act8945a_regulators[] = {
+	ACT89xx_REG("DCDC_REG1", ACT8945A, DCDC1, VSET1, "vp1"),
+	ACT89xx_REG("DCDC_REG2", ACT8945A, DCDC2, VSET1, "vp2"),
+	ACT89xx_REG("DCDC_REG3", ACT8945A, DCDC3, VSET1, "vp3"),
+	ACT89xx_REG("LDO_REG1", ACT8945A, LDO1, VSET, "inl45"),
+	ACT89xx_REG("LDO_REG2", ACT8945A, LDO2, VSET, "inl45"),
+	ACT89xx_REG("LDO_REG3", ACT8945A, LDO3, VSET, "inl67"),
+	ACT89xx_REG("LDO_REG4", ACT8945A, LDO4, VSET, "inl67"),
+};
+
+static const struct regulator_desc act8945a_alt_regulators[] = {
+	ACT89xx_REG("DCDC_REG1", ACT8945A, DCDC1, VSET2, "vp1"),
+	ACT89xx_REG("DCDC_REG2", ACT8945A, DCDC2, VSET2, "vp2"),
+	ACT89xx_REG("DCDC_REG3", ACT8945A, DCDC3, VSET2, "vp3"),
+	ACT89xx_REG("LDO_REG1", ACT8945A, LDO1, VSET, "inl45"),
+	ACT89xx_REG("LDO_REG2", ACT8945A, LDO2, VSET, "inl45"),
+	ACT89xx_REG("LDO_REG3", ACT8945A, LDO3, VSET, "inl67"),
+	ACT89xx_REG("LDO_REG4", ACT8945A, LDO4, VSET, "inl67"),
+};
+
+static int act8945a_pmic_probe(struct platform_device *pdev)
+{
+	struct regulator_config config = { };
+	const struct regulator_desc *regulators;
+	struct regulator_dev *rdev;
+	int i, num_regulators;
+	bool voltage_select;
+
+	voltage_select = of_property_read_bool(pdev->dev.of_node,
+					       "active-semi,vsel-high");
+
+	if (voltage_select) {
+		regulators = act8945a_alt_regulators;
+		num_regulators = ARRAY_SIZE(act8945a_alt_regulators);
+	} else {
+		regulators = act8945a_regulators;
+		num_regulators = ARRAY_SIZE(act8945a_regulators);
+	}
+
+	for (i = 0; i < num_regulators; i++) {
+		config.dev = &pdev->dev;
+
+		rdev = devm_regulator_register(&pdev->dev, &regulators[i], &config);
+		if (IS_ERR(rdev)) {
+			dev_err(&pdev->dev,
+				"failed to register %s regulator\n",
+				regulators[i].name);
+			return PTR_ERR(rdev);
+		}
+	}
+
+	return 0;
+}
+
+static const struct of_device_id act8945a_pmic_of_match[] = {
+	{ .compatible = "active-semi,act8945a-regulator" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, act8945a_pmic_of_match);
+
+static struct platform_driver act8945a_pmic_driver = {
+	.driver = {
+		.name = "act8945a-regulator",
+		.of_match_table = of_match_ptr(act8945a_pmic_of_match),
+	},
+	.probe = act8945a_pmic_probe,
+};
+module_platform_driver(act8945a_pmic_driver);
+
+MODULE_DESCRIPTION("Active-semi ACT8945A voltage regulator driver");
+MODULE_AUTHOR("Wenyou Yang <wenyou.yang@atmel.com>");
+MODULE_LICENSE("GPL");
-- 
1.7.9.5

[PATCH v4 1/2] regulator: act8945a: add regulator driver for ACT8945A

[Mark Brown]

On Wed, Jan 27, 2016 at 10:57:42AM +0800, Wenyou Yang wrote:

> +static const struct of_device_id act8945a_pmic_of_match[] = {
> +	{ .compatible = "active-semi,act8945a-regulator" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, act8945a_pmic_of_match);

This seems mostly OK but why do we have a compatible string here -
shouldn't the MFD be able to instantiate the regulator function without
needing this?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160129/cd5f0e17/attachment.sig>

[PATCH v4 1/2] regulator: act8945a: add regulator driver for ACT8945A

[Yang, Wenyou]

Hi Mark,

> -----Original Message-----
> From: Mark Brown [mailto:broonie at kernel.org]
> Sent: 2016?1?29? 8:16
> To: Yang, Wenyou <Wenyou.Yang@atmel.com>
> Cc: Liam Girdwood <lgirdwood@gmail.com>; Rob Herring <robh+dt@kernel.org>;
> Pawel Moll <pawel.moll@arm.com>; Ian Campbell
> <ijc+devicetree@hellion.org.uk>; Kumar Gala <galak@codeaurora.org>;
> Krzysztof Kozlowski <k.kozlowski@samsung.com>; Javier Martinez Canillas
> <javier@dowhile0.org>; Lee Jones <lee.jones@linaro.org>; Peter Korsgaard
> <jacmet@gmail.com>; Ferre, Nicolas <Nicolas.FERRE@atmel.com>; linux-arm-
> kernel at lists.infradead.org; linux-kernel at vger.kernel.org;
> devicetree at vger.kernel.org
> Subject: Re: [PATCH v4 1/2] regulator: act8945a: add regulator driver for
> ACT8945A
> 
> On Wed, Jan 27, 2016 at 10:57:42AM +0800, Wenyou Yang wrote:
> 
> > +static const struct of_device_id act8945a_pmic_of_match[] = {
> > +	{ .compatible = "active-semi,act8945a-regulator" },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, act8945a_pmic_of_match);
> 
> This seems mostly OK but why do we have a compatible string here - shouldn't
> the MFD be able to instantiate the regulator function without needing this?

Because I got feedback from Javier for the act8945a-charger patches of this MFD series,
He said missing the OF match table will cause the module autoloading broken. 

http://lists.infradead.org/pipermail/linux-arm-kernel/2016-January/398113.html

What do you think about it?


Best Regards,
Wenyou Yang

[PATCH v4 1/2] regulator: act8945a: add regulator driver for ACT8945A

[Mark Brown]

On Fri, Jan 29, 2016 at 01:20:08AM +0000, Yang, Wenyou wrote:

> > > +static const struct of_device_id act8945a_pmic_of_match[] = {
> > > +	{ .compatible = "active-semi,act8945a-regulator" },
> > > +	{ },
> > > +};
> > > +MODULE_DEVICE_TABLE(of, act8945a_pmic_of_match);

> > This seems mostly OK but why do we have a compatible string here - shouldn't
> > the MFD be able to instantiate the regulator function without needing this?

> Because I got feedback from Javier for the act8945a-charger patches of this MFD series,
> He said missing the OF match table will cause the module autoloading broken. 

> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-January/398113.html

> What do you think about it?

If then device is not being loaded from the DT (and it shouldn't be, the
device looks like it should be instantiated directly by the MFD as it
can't exist separately to that MFD) an OF table will do nothing.  
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160129/04fd078b/attachment.sig>

[PATCH v4 1/2] regulator: act8945a: add regulator driver for ACT8945A

[Javier Martinez Canillas]

Hello Mark,

On Fri, Jan 29, 2016 at 8:35 AM, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Jan 29, 2016 at 01:20:08AM +0000, Yang, Wenyou wrote:
>
>> > > +static const struct of_device_id act8945a_pmic_of_match[] = {
>> > > + { .compatible = "active-semi,act8945a-regulator" },
>> > > + { },
>> > > +};
>> > > +MODULE_DEVICE_TABLE(of, act8945a_pmic_of_match);
>
>> > This seems mostly OK but why do we have a compatible string here - shouldn't
>> > the MFD be able to instantiate the regulator function without needing this?
>
>> Because I got feedback from Javier for the act8945a-charger patches of this MFD series,
>> He said missing the OF match table will cause the module autoloading broken.
>
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-January/398113.html
>
>> What do you think about it?
>
> If then device is not being loaded from the DT (and it shouldn't be, the
> device looks like it should be instantiated directly by the MFD as it
> can't exist separately to that MFD) an OF table will do nothing.

Then he should remove the .of_compatible from the MFD cell definition.

Otherwise the MFD core will assign an of_node to this device in
mfd_add_device(), making the platform core to believe the device was
instantiated by OF. So an OF modalias uevent will be reported to load
the module and the OF table should be used for matching the device
with the driver.

IOW, the MFD and regulator driver have to agree on this. Either
everything should have OF or platform information, both work but
mixing these as it was on a previous version doesn't.

Best regards,
Javier

[PATCH v4 1/2] regulator: act8945a: add regulator driver for ACT8945A

[Mark Brown]

On Fri, Jan 29, 2016 at 08:50:19AM -0300, Javier Martinez Canillas wrote:

> Then he should remove the .of_compatible from the MFD cell definition.

Yes, obviously.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160129/6ef264c7/attachment.sig>

[PATCH v4 1/2] regulator: act8945a: add regulator driver for ACT8945A

[Yang, Wenyou]

Hi Javier & Mark,

I am educated. Thank you all.

> -----Original Message-----
> From: Javier Martinez Canillas [mailto:javier at dowhile0.org]
> Sent: 2016?1?29? 19:50
> To: Mark Brown <broonie@kernel.org>
> Cc: Yang, Wenyou <Wenyou.Yang@atmel.com>; Liam Girdwood
> <lgirdwood@gmail.com>; Rob Herring <robh+dt@kernel.org>; Pawel Moll
> <pawel.moll@arm.com>; Ian Campbell <ijc+devicetree@hellion.org.uk>; Kumar
> Gala <galak@codeaurora.org>; Krzysztof Kozlowski
> <k.kozlowski@samsung.com>; Lee Jones <lee.jones@linaro.org>; Peter
> Korsgaard <jacmet@gmail.com>; Ferre, Nicolas <Nicolas.FERRE@atmel.com>;
> linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org;
> devicetree at vger.kernel.org
> Subject: Re: [PATCH v4 1/2] regulator: act8945a: add regulator driver for
> ACT8945A
> 
> Hello Mark,
> 
> On Fri, Jan 29, 2016 at 8:35 AM, Mark Brown <broonie@kernel.org> wrote:
> > On Fri, Jan 29, 2016 at 01:20:08AM +0000, Yang, Wenyou wrote:
> >
> >> > > +static const struct of_device_id act8945a_pmic_of_match[] = {  {
> >> > > +.compatible = "active-semi,act8945a-regulator" },  { }, };
> >> > > +MODULE_DEVICE_TABLE(of, act8945a_pmic_of_match);
> >
> >> > This seems mostly OK but why do we have a compatible string here -
> >> > shouldn't the MFD be able to instantiate the regulator function without
> needing this?
> >
> >> Because I got feedback from Javier for the act8945a-charger patches
> >> of this MFD series, He said missing the OF match table will cause the module
> autoloading broken.
> >
> >> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-January/39
> >> 8113.html
> >
> >> What do you think about it?
> >
> > If then device is not being loaded from the DT (and it shouldn't be,
> > the device looks like it should be instantiated directly by the MFD as
> > it can't exist separately to that MFD) an OF table will do nothing.
> 
> Then he should remove the .of_compatible from the MFD cell definition.

I tried it,

But if removed this .of_compatible, and reserved the OF table. 
the &pdev->dev->of_node is NULL, the driver fails to get the configuration value from DT, 

It seems the OF table still doesn't work. Where is wrong?

Could you help give some suggestion?

> 
> Otherwise the MFD core will assign an of_node to this device in mfd_add_device(),
> making the platform core to believe the device was instantiated by OF. So an OF
> modalias uevent will be reported to load the module and the OF table should be
> used for matching the device with the driver.
> 
> IOW, the MFD and regulator driver have to agree on this. Either everything should
> have OF or platform information, both work but mixing these as it was on a
> previous version doesn't.
> 
> Best regards,
> Javier


Best Regards,
Wenyou Yang

[PATCH v4 1/2] regulator: act8945a: add regulator driver for ACT8945A

[Javier Martinez Canillas]

Hello Wenyou,

[snip]

>> >
>> > If then device is not being loaded from the DT (and it shouldn't be,
>> > the device looks like it should be instantiated directly by the MFD as
>> > it can't exist separately to that MFD) an OF table will do nothing.
>>
>> Then he should remove the .of_compatible from the MFD cell definition.
>
> I tried it,
>
> But if removed this .of_compatible, and reserved the OF table.

What we said is that you should remove both the mfd cell
.of_compatible field and the OF table. Of course removing one but
leaving the other is not correct.

> the &pdev->dev->of_node is NULL, the driver fails to get the configuration value from DT,
>

I see, you mean that you are not able to lookup the
"active-semi,vsel-high" property.

> It seems the OF table still doesn't work. Where is wrong?
>

I think the problem is with your DT binding. You have a PMIC dev node
with compatible "active-semi,act8945a" that has a child node with
compatible "active-semi,act8945a-regulator" which in turn has the
"regulators" node.

I believe what Mark says is that there shouldn't be a compatible and
node for the regulators IC since is part of the PMIC. IOW, you can't
have it as a standalone node in the DTS.

> Could you help give some suggestion?
>

I would just remove the "active-semi,act8945a-regulator" node and make
"active-semi,vsel-high" a property of the "active-semi,act8945a" node.
That way you can remove the mfd cell .of_compatible and OF table in
the regulator driver and read the "active-semi,vsel-high" using the
platform device's parent of_node.

But it's better if you wait for Mark's opinions before re-spining your patches.

Best regards,
Javier

[PATCH v4 1/2] regulator: act8945a: add regulator driver for ACT8945A

[Yang, Wenyou]

Hi Javier,

> -----Original Message-----
> From: Javier Martinez Canillas [mailto:javier at dowhile0.org]
> Sent: 2016?2?1? 21:15
> To: Yang, Wenyou <Wenyou.Yang@atmel.com>
> Cc: Mark Brown <broonie@kernel.org>; Liam Girdwood <lgirdwood@gmail.com>;
> Rob Herring <robh+dt@kernel.org>; Pawel Moll <pawel.moll@arm.com>; Ian
> Campbell <ijc+devicetree@hellion.org.uk>; Kumar Gala <galak@codeaurora.org>;
> Krzysztof Kozlowski <k.kozlowski@samsung.com>; Lee Jones
> <lee.jones@linaro.org>; Peter Korsgaard <jacmet@gmail.com>; Ferre, Nicolas
> <Nicolas.FERRE@atmel.com>; linux-arm-kernel at lists.infradead.org; linux-
> kernel at vger.kernel.org; devicetree at vger.kernel.org
> Subject: Re: [PATCH v4 1/2] regulator: act8945a: add regulator driver for
> ACT8945A
> 
> Hello Wenyou,
> 
> [snip]
> 
> >> >
> >> > If then device is not being loaded from the DT (and it shouldn't
> >> > be, the device looks like it should be instantiated directly by the
> >> > MFD as it can't exist separately to that MFD) an OF table will do nothing.
> >>
> >> Then he should remove the .of_compatible from the MFD cell definition.
> >
> > I tried it,
> >
> > But if removed this .of_compatible, and reserved the OF table.
> 
> What we said is that you should remove both the mfd cell .of_compatible field and
> the OF table. Of course removing one but leaving the other is not correct.

I am confused :)

> 
> > the &pdev->dev->of_node is NULL, the driver fails to get the
> > configuration value from DT,
> >
> 
> I see, you mean that you are not able to lookup the "active-semi,vsel-high"
> property.

No only this property,

The regulator init_datas are also not gotten from DT due to the &pdev->dev->of_node is NULL.

> 
> > It seems the OF table still doesn't work. Where is wrong?
> >
> 
> I think the problem is with your DT binding. You have a PMIC dev node with
> compatible "active-semi,act8945a" that has a child node with compatible "active-
> semi,act8945a-regulator" which in turn has the "regulators" node.
> 
> I believe what Mark says is that there shouldn't be a compatible and node for the
> regulators IC since is part of the PMIC. IOW, you can't have it as a standalone
> node in the DTS.
> 
> > Could you help give some suggestion?
> >
> 
> I would just remove the "active-semi,act8945a-regulator" node and make "active-
> semi,vsel-high" a property of the "active-semi,act8945a" node.
> That way you can remove the mfd cell .of_compatible and OF table in the
> regulator driver and read the "active-semi,vsel-high" using the platform device's
> parent of_node.
> 
> But it's better if you wait for Mark's opinions before re-spining your patches.

All right.

> 
> Best regards,
> Javier

Thanks 


Best Regards,
Wenyou Yang

[PATCH v4 1/2] regulator: act8945a: add regulator driver for ACT8945A

[Yang, Wenyou]

> -----Original Message-----
> From: Javier Martinez Canillas [mailto:javier at dowhile0.org]
> Sent: 2016?2?1? 21:15
> To: Yang, Wenyou <Wenyou.Yang@atmel.com>
> Cc: Mark Brown <broonie@kernel.org>; Liam Girdwood <lgirdwood@gmail.com>;
> Rob Herring <robh+dt@kernel.org>; Pawel Moll <pawel.moll@arm.com>; Ian
> Campbell <ijc+devicetree@hellion.org.uk>; Kumar Gala <galak@codeaurora.org>;
> Krzysztof Kozlowski <k.kozlowski@samsung.com>; Lee Jones
> <lee.jones@linaro.org>; Peter Korsgaard <jacmet@gmail.com>; Ferre, Nicolas
> <Nicolas.FERRE@atmel.com>; linux-arm-kernel at lists.infradead.org; linux-
> kernel at vger.kernel.org; devicetree at vger.kernel.org
> Subject: Re: [PATCH v4 1/2] regulator: act8945a: add regulator driver for
> ACT8945A
> 
> Hello Wenyou,
> 
> [snip]
> 
> >> >
> >> > If then device is not being loaded from the DT (and it shouldn't
> >> > be, the device looks like it should be instantiated directly by the
> >> > MFD as it can't exist separately to that MFD) an OF table will do nothing.
> >>
> >> Then he should remove the .of_compatible from the MFD cell definition.
> >
> > I tried it,
> >
> > But if removed this .of_compatible, and reserved the OF table.
> 
> What we said is that you should remove both the mfd cell .of_compatible field and
> the OF table. Of course removing one but leaving the other is not correct.
> 
> > the &pdev->dev->of_node is NULL, the driver fails to get the
> > configuration value from DT,
> >
> 
> I see, you mean that you are not able to lookup the "active-semi,vsel-high"
> property.
> 
> > It seems the OF table still doesn't work. Where is wrong?
> >
> 
> I think the problem is with your DT binding. You have a PMIC dev node with
> compatible "active-semi,act8945a" that has a child node with compatible "active-
> semi,act8945a-regulator" which in turn has the "regulators" node.
> 
> I believe what Mark says is that there shouldn't be a compatible and node for the
> regulators IC since is part of the PMIC. IOW, you can't have it as a standalone
> node in the DTS.
> 
> > Could you help give some suggestion?
> >
> 
> I would just remove the "active-semi,act8945a-regulator" node and make "active-
> semi,vsel-high" a property of the "active-semi,act8945a" node.
> That way you can remove the mfd cell .of_compatible and OF table in the
> regulator driver and read the "active-semi,vsel-high" using the platform device's
> parent of_node.
> 
> But it's better if you wait for Mark's opinions before re-spining your patches.

Hi Mark, what is your opinions?

> 
> Best regards,
> Javier


Best Regards,
Wenyou Yang

[PATCH v4 1/2] regulator: act8945a: add regulator driver for ACT8945A

[Mark Brown]

On Wed, Feb 03, 2016 at 02:33:11AM +0000, Yang, Wenyou wrote:

> > I would just remove the "active-semi,act8945a-regulator" node and make "active-
> > semi,vsel-high" a property of the "active-semi,act8945a" node.
> > That way you can remove the mfd cell .of_compatible and OF table in the
> > regulator driver and read the "active-semi,vsel-high" using the platform device's
> > parent of_node.

> > But it's better if you wait for Mark's opinions before re-spining your patches.

> Hi Mark, what is your opinions?

Like Javier says you should make your DT binding look like the DT
bindings for other MFDs and not use of_compatible.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160203/30814480/attachment.sig>

[PATCH v4 1/2] regulator: act8945a: add regulator driver for ACT8945A

[Yang, Wenyou]

> -----Original Message-----
> From: Mark Brown [mailto:broonie at kernel.org]
> Sent: 2016?2?4? 1:48
> To: Yang, Wenyou <Wenyou.Yang@atmel.com>
> Cc: Javier Martinez Canillas <javier@dowhile0.org>; Liam Girdwood
> <lgirdwood@gmail.com>; Rob Herring <robh+dt@kernel.org>; Pawel Moll
> <pawel.moll@arm.com>; Ian Campbell <ijc+devicetree@hellion.org.uk>; Kumar
> Gala <galak@codeaurora.org>; Krzysztof Kozlowski
> <k.kozlowski@samsung.com>; Lee Jones <lee.jones@linaro.org>; Peter
> Korsgaard <jacmet@gmail.com>; Ferre, Nicolas <Nicolas.FERRE@atmel.com>;
> linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org;
> devicetree at vger.kernel.org
> Subject: Re: [PATCH v4 1/2] regulator: act8945a: add regulator driver for
> ACT8945A
> 
> On Wed, Feb 03, 2016 at 02:33:11AM +0000, Yang, Wenyou wrote:
> 
> > > I would just remove the "active-semi,act8945a-regulator" node and
> > > make "active- semi,vsel-high" a property of the "active-semi,act8945a" node.
> > > That way you can remove the mfd cell .of_compatible and OF table in
> > > the regulator driver and read the "active-semi,vsel-high" using the
> > > platform device's parent of_node.
> 
> > > But it's better if you wait for Mark's opinions before re-spining your patches.
> 
> > Hi Mark, what is your opinions?
> 
> Like Javier says you should make your DT binding look like the DT bindings for
> other MFDs and not use of_compatible.

All right.

Thank you, Mark, Javier.


Best Regards,
Wenyou Yang

[PATCH v4 1/2] regulator: act8945a: add regulator driver for ACT8945A

[Peter Korsgaard]

>>>>> "Mark" == Mark Brown <broonie@kernel.org> writes:

 > On Fri, Jan 29, 2016 at 01:20:08AM +0000, Yang, Wenyou wrote:
 >> > > +static const struct of_device_id act8945a_pmic_of_match[] = {
 >> > > +	{ .compatible = "active-semi,act8945a-regulator" },
 >> > > +	{ },
 >> > > +};
 >> > > +MODULE_DEVICE_TABLE(of, act8945a_pmic_of_match);

 >> > This seems mostly OK but why do we have a compatible string here - shouldn't
 >> > the MFD be able to instantiate the regulator function without needing this?

 >> Because I got feedback from Javier for the act8945a-charger patches of this MFD series,
 >> He said missing the OF match table will cause the module autoloading broken. 

 >> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-January/398113.html

 >> What do you think about it?

 > If then device is not being loaded from the DT (and it shouldn't be, the
 > device looks like it should be instantiated directly by the MFD as it
 > can't exist separately to that MFD) an OF table will do nothing.  

To add to the confusion, the regulator part of the chip is actually
identical to act8865, so it could use the existing regulator driver /
compatible, except that it binds to the platform bus instead of i2c.

-- 
Bye, Peter Korsgaard

[PATCH v4 1/2] regulator: act8945a: add regulator driver for ACT8945A

[Yang, Wenyou]

Hi Peter,

> -----Original Message-----
> From: Peter Korsgaard [mailto:jacmet at gmail.com] On Behalf Of Peter Korsgaard
> Sent: 2016?2?3? 1:42
> To: Mark Brown <broonie@kernel.org>
> Cc: Yang, Wenyou <Wenyou.Yang@atmel.com>; Liam Girdwood
> <lgirdwood@gmail.com>; Rob Herring <robh+dt@kernel.org>; Pawel Moll
> <pawel.moll@arm.com>; Ian Campbell <ijc+devicetree@hellion.org.uk>; Kumar
> Gala <galak@codeaurora.org>; Krzysztof Kozlowski
> <k.kozlowski@samsung.com>; Javier Martinez Canillas <javier@dowhile0.org>;
> Lee Jones <lee.jones@linaro.org>; Peter Korsgaard <jacmet@gmail.com>; Ferre,
> Nicolas <Nicolas.FERRE@atmel.com>; linux-arm-kernel at lists.infradead.org;
> linux-kernel at vger.kernel.org; devicetree at vger.kernel.org
> Subject: Re: [PATCH v4 1/2] regulator: act8945a: add regulator driver for
> ACT8945A
> 
> >>>>> "Mark" == Mark Brown <broonie@kernel.org> writes:
> 
>  > On Fri, Jan 29, 2016 at 01:20:08AM +0000, Yang, Wenyou wrote:
>  >> > > +static const struct of_device_id act8945a_pmic_of_match[] = {
>  >> > > +	{ .compatible = "active-semi,act8945a-regulator" },
>  >> > > +	{ },
>  >> > > +};
>  >> > > +MODULE_DEVICE_TABLE(of, act8945a_pmic_of_match);
> 
>  >> > This seems mostly OK but why do we have a compatible string here -
> shouldn't  >> > the MFD be able to instantiate the regulator function without
> needing this?
> 
>  >> Because I got feedback from Javier for the act8945a-charger patches of this
> MFD series,  >> He said missing the OF match table will cause the module
> autoloading broken.
> 
>  >> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-January/398113.html
> 
>  >> What do you think about it?
> 
>  > If then device is not being loaded from the DT (and it shouldn't be, the  > device
> looks like it should be instantiated directly by the MFD as it  > can't exist
> separately to that MFD) an OF table will do nothing.
> 
> To add to the confusion, the regulator part of the chip is actually identical to
> act8865, so it could use the existing regulator driver / compatible, except that it
> binds to the platform bus instead of i2c.

Thank you for your opinion.

But I think It is better to make it a separate driver, the driver is simpler. 

> 
> --
> Bye, Peter Korsgaard


Best Regards,
Wenyou Yang

[PATCH v4 1/2] regulator: act8945a: add regulator driver for ACT8945A

[Krzysztof Kozlowski]

On 03.02.2016 11:29, Yang, Wenyou wrote:
> Hi Peter,
> 
>> -----Original Message-----
>> From: Peter Korsgaard [mailto:jacmet at gmail.com] On Behalf Of Peter Korsgaard
>> Sent: 2016?2?3? 1:42
>> To: Mark Brown <broonie@kernel.org>
>> Cc: Yang, Wenyou <Wenyou.Yang@atmel.com>; Liam Girdwood
>> <lgirdwood@gmail.com>; Rob Herring <robh+dt@kernel.org>; Pawel Moll
>> <pawel.moll@arm.com>; Ian Campbell <ijc+devicetree@hellion.org.uk>; Kumar
>> Gala <galak@codeaurora.org>; Krzysztof Kozlowski
>> <k.kozlowski@samsung.com>; Javier Martinez Canillas <javier@dowhile0.org>;
>> Lee Jones <lee.jones@linaro.org>; Peter Korsgaard <jacmet@gmail.com>; Ferre,
>> Nicolas <Nicolas.FERRE@atmel.com>; linux-arm-kernel at lists.infradead.org;
>> linux-kernel at vger.kernel.org; devicetree at vger.kernel.org
>> Subject: Re: [PATCH v4 1/2] regulator: act8945a: add regulator driver for
>> ACT8945A
>>
>>>>>>> "Mark" == Mark Brown <broonie@kernel.org> writes:
>>
>>  > On Fri, Jan 29, 2016 at 01:20:08AM +0000, Yang, Wenyou wrote:
>>  >> > > +static const struct of_device_id act8945a_pmic_of_match[] = {
>>  >> > > +	{ .compatible = "active-semi,act8945a-regulator" },
>>  >> > > +	{ },
>>  >> > > +};
>>  >> > > +MODULE_DEVICE_TABLE(of, act8945a_pmic_of_match);
>>
>>  >> > This seems mostly OK but why do we have a compatible string here -
>> shouldn't  >> > the MFD be able to instantiate the regulator function without
>> needing this?
>>
>>  >> Because I got feedback from Javier for the act8945a-charger patches of this
>> MFD series,  >> He said missing the OF match table will cause the module
>> autoloading broken.
>>
>>  >> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-January/398113.html
>>
>>  >> What do you think about it?
>>
>>  > If then device is not being loaded from the DT (and it shouldn't be, the  > device
>> looks like it should be instantiated directly by the MFD as it  > can't exist
>> separately to that MFD) an OF table will do nothing.
>>
>> To add to the confusion, the regulator part of the chip is actually identical to
>> act8865, so it could use the existing regulator driver / compatible, except that it
>> binds to the platform bus instead of i2c.
> 
> Thank you for your opinion.
> 
> But I think It is better to make it a separate driver, the driver is simpler. 

Do I understand correctly that you are creating a new device driver for
the same logical device (regulator block)?

By duplicating the code you are not making kernel simpler. Maybe your
new driver will be simple but still this adds a NEW driver instead of
re-using existing code.

Best regards,
Krzysztof

[PATCH v4 2/2] regulator: add documentation for ACT8945A's regulator DT bindings

[Wenyou Yang]

This patch adds documentation for the DT bindings of the regulator
subdevice of ACT8945A MFD.

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
Acked-by: Rob Herring <robh@kernel.org>
---

Changes in v4: None
Changes in v3: None
Changes in v2:
 - add Acked-by Rob Herring.

 .../bindings/regulator/act8945a-regulator.txt      |   78 ++++++++++++++++++++
 1 file changed, 78 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/act8945a-regulator.txt

diff --git a/Documentation/devicetree/bindings/regulator/act8945a-regulator.txt b/Documentation/devicetree/bindings/regulator/act8945a-regulator.txt
new file mode 100644
index 0000000..0e08a3e
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/act8945a-regulator.txt
@@ -0,0 +1,78 @@
+Device-Tree bindings for regulators of Active-semi ACT8945A Multi-Function Device
+
+Required properties:
+ - compatible: "active-semi,act8945a-regulator"
+
+Optional properties:
+- active-semi,vsel-high: Indicates if the VSEL pin is set to logic-high.
+  If this property is missing, assume the VSEL pin is set to logic-low.
+
+Optional input supply properties:
+  - vp1-supply: The input supply for REG_DCDC1
+  - vp2-supply: The input supply for REG_DCDC2
+  - vp3-supply: The input supply for REG_DCDC3
+  - inl45-supply: The input supply for REG_LDO1 and REG_LDO2
+  - inl67-supply: The input supply for REG_LDO3 and REG_LDO4
+
+Any standard regulator properties can be used to configure the single regulator.
+
+The valid names for regulators are:
+	REG_DCDC1, REG_DCDC2, REG_DCDC3, REG_LDO1, REG_LDO2, REG_LDO3, REG_LDO4.
+
+Example:
+
+	pmic {
+		compatible = "active-semi,act8945a-regulator";
+		active-semi,vsel-high;
+
+		regulators {
+			vdd_1v35_reg: REG_DCDC1 {
+				regulator-name = "VDD_1V35";
+				regulator-min-microvolt = <1350000>;
+				regulator-max-microvolt = <1350000>;
+				regulator-always-on;
+			};
+
+			vdd_1v2_reg: REG_DCDC2 {
+				regulator-name = "VDD_1V2";
+				regulator-min-microvolt = <1100000>;
+				regulator-max-microvolt = <1300000>;
+				regulator-always-on;
+			};
+
+			vdd_3v3_reg: REG_DCDC3 {
+				regulator-name = "VDD_3V3";
+				regulator-min-microvolt = <3300000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-always-on;
+			};
+
+			vdd_fuse_reg: REG_LDO1 {
+				regulator-name = "VDD_FUSE";
+				regulator-min-microvolt = <2500000>;
+				regulator-max-microvolt = <2500000>;
+				regulator-always-on;
+			};
+
+			vdd_3v3_lp_reg: REG_LDO2 {
+				regulator-name = "VDD_3V3_LP";
+				regulator-min-microvolt = <3300000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-always-on;
+			};
+
+			vdd_led_reg: REG_LDO3 {
+				regulator-name = "VDD_LED";
+				regulator-min-microvolt = <3300000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-always-on;
+			};
+
+			vdd_sdhc_1v8_reg: REG_LDO4 {
+				regulator-name = "VDD_SDHC_1V8";
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-always-on;
+			};
+		};
+	};
-- 
1.7.9.5