Re: [PATCH 3/3] regulator: lp873x: Add support for lp873x PMIC regulators

[Keerthy]

On Friday 06 May 2016 05:32 PM, Mark Brown wrote:

On Fri, May 06, 2016 at 10:13:24AM +0530, Keerthy wrote:


I am using of_platform_populate function in the mfd driver to create
platform devices for the child nodes, in my case regulators.



of_platform_populate in turn calls on to of_platform_bus_create which
mandates compatible properties. It quietly skips device creation if there
are no compatible properties.


You shouldn't be using that, you should just have a table of subdevices
in the MFD.


Okay. I will send v2 with compatibles removed.

Thanks,
Keerthy

Re: [PATCH 3/3] regulator: lp873x: Add support for lp873x PMIC regulators

[Keerthy]

On Friday 06 May 2016 05:32 PM, Mark Brown wrote:

On Fri, May 06, 2016 at 10:13:24AM +0530, Keerthy wrote:


I am using of_platform_populate function in the mfd driver to create
platform devices for the child nodes, in my case regulators.



of_platform_populate in turn calls on to of_platform_bus_create which
mandates compatible properties. It quietly skips device creation if there
are no compatible properties.


You shouldn't be using that, you should just have a table of subdevices
in the MFD.


Okay. I will send v2 with compatibles removed.

Thanks,
Keerthy

Re: [PATCH 3/3] regulator: lp873x: Add support for lp873x PMIC regulators

[Mark Brown]

On Fri, May 06, 2016 at 10:13:24AM +0530, Keerthy wrote:

> I am using of_platform_populate function in the mfd driver to create
> platform devices for the child nodes, in my case regulators.

> of_platform_populate in turn calls on to of_platform_bus_create which
> mandates compatible properties. It quietly skips device creation if there
> are no compatible properties.

You shouldn't be using that, you should just have a table of subdevices
in the MFD.


signature.asc
Description: PGP signature

Re: [PATCH 3/3] regulator: lp873x: Add support for lp873x PMIC regulators

[Mark Brown]

On Fri, May 06, 2016 at 10:13:24AM +0530, Keerthy wrote:

> I am using of_platform_populate function in the mfd driver to create
> platform devices for the child nodes, in my case regulators.

> of_platform_populate in turn calls on to of_platform_bus_create which
> mandates compatible properties. It quietly skips device creation if there
> are no compatible properties.

You shouldn't be using that, you should just have a table of subdevices
in the MFD.


signature.asc
Description: PGP signature

Re: [PATCH 3/3] regulator: lp873x: Add support for lp873x PMIC regulators

[Keerthy]

Hi Mark,


On Thursday 05 May 2016 09:08 PM, Mark Brown wrote:

On Thu, May 05, 2016 at 10:40:40AM +0530, Keerthy wrote:


+static const struct of_device_id of_lp873x_match_tbl[] = {
+   { .compatible = "ti,lp8733-regulators",},
+   { .compatible = "ti,lp8732-regulators",},
+   { .compatible = "ti,lp873x-regulators",},
+   {},
+};


There should be no need for compatible strings here, we already know
what device this is from the parent.  The way we split drivers up for
Linux is something that's internal to Linux and shouldn't be in the
device tree.  If we do have explicit compatible strings then they should
(as always) be specific to a device, no wildcards.


Thanks for the review.

I am using of_platform_populate function in the mfd driver to create 
platform devices for the child nodes, in my case regulators.


of_platform_populate in turn calls on to of_platform_bus_create which 
mandates compatible properties. It quietly skips device creation if 
there are no compatible properties.


When i enabled a debug print, i see this:
skipping /ocp/i2c@4807/lp8732@61/regulators, no compatible prop
Hence i kept the compatible properties.

The driver supports two variants of LP873x family:

1) LP8732
2) LP8733

I will knock off the ti,lp873x-regulators compatible which is more 
generic/wildcard entry.


Let me know if this approach is fine.



Otherwise this looks sensible.

Re: [PATCH 3/3] regulator: lp873x: Add support for lp873x PMIC regulators

[Keerthy]

Hi Mark,


On Thursday 05 May 2016 09:08 PM, Mark Brown wrote:

On Thu, May 05, 2016 at 10:40:40AM +0530, Keerthy wrote:


+static const struct of_device_id of_lp873x_match_tbl[] = {
+   { .compatible = "ti,lp8733-regulators",},
+   { .compatible = "ti,lp8732-regulators",},
+   { .compatible = "ti,lp873x-regulators",},
+   {},
+};


There should be no need for compatible strings here, we already know
what device this is from the parent.  The way we split drivers up for
Linux is something that's internal to Linux and shouldn't be in the
device tree.  If we do have explicit compatible strings then they should
(as always) be specific to a device, no wildcards.


Thanks for the review.

I am using of_platform_populate function in the mfd driver to create 
platform devices for the child nodes, in my case regulators.


of_platform_populate in turn calls on to of_platform_bus_create which 
mandates compatible properties. It quietly skips device creation if 
there are no compatible properties.


When i enabled a debug print, i see this:
skipping /ocp/i2c@4807/lp8732@61/regulators, no compatible prop
Hence i kept the compatible properties.

The driver supports two variants of LP873x family:

1) LP8732
2) LP8733

I will knock off the ti,lp873x-regulators compatible which is more 
generic/wildcard entry.


Let me know if this approach is fine.



Otherwise this looks sensible.

Re: [PATCH 3/3] regulator: lp873x: Add support for lp873x PMIC regulators

[Mark Brown]

On Thu, May 05, 2016 at 10:40:40AM +0530, Keerthy wrote:

> +static const struct of_device_id of_lp873x_match_tbl[] = {
> + { .compatible = "ti,lp8733-regulators",},
> + { .compatible = "ti,lp8732-regulators",},
> + { .compatible = "ti,lp873x-regulators",},
> + {},
> +};

There should be no need for compatible strings here, we already know
what device this is from the parent.  The way we split drivers up for
Linux is something that's internal to Linux and shouldn't be in the
device tree.  If we do have explicit compatible strings then they should
(as always) be specific to a device, no wildcards.

Otherwise this looks sensible.


signature.asc
Description: PGP signature

Re: [PATCH 3/3] regulator: lp873x: Add support for lp873x PMIC regulators

[Mark Brown]

On Thu, May 05, 2016 at 10:40:40AM +0530, Keerthy wrote:

> +static const struct of_device_id of_lp873x_match_tbl[] = {
> + { .compatible = "ti,lp8733-regulators",},
> + { .compatible = "ti,lp8732-regulators",},
> + { .compatible = "ti,lp873x-regulators",},
> + {},
> +};

There should be no need for compatible strings here, we already know
what device this is from the parent.  The way we split drivers up for
Linux is something that's internal to Linux and shouldn't be in the
device tree.  If we do have explicit compatible strings then they should
(as always) be specific to a device, no wildcards.

Otherwise this looks sensible.


signature.asc
Description: PGP signature

[PATCH 3/3] regulator: lp873x: Add support for lp873x PMIC regulators

[Keerthy]

The regulators set consists of 2 BUCKs and 2 LDOs. The output
voltages are configurable and are meant to supply power to the
main processor and other components. The ramp delay is configurable
for both BUCKs.

Signed-off-by: Keerthy 
---
 drivers/regulator/Kconfig|   9 ++
 drivers/regulator/Makefile   |   1 +
 drivers/regulator/lp873x-regulator.c | 242 +++
 3 files changed, 252 insertions(+)
 create mode 100644 drivers/regulator/lp873x-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index c77dc08..4d2d737 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -321,6 +321,15 @@ config REGULATOR_LP872X
help
  This driver supports LP8720/LP8725 PMIC
 
+config REGULATOR_LP873X
+   tristate "TI LP873X Power regulators"
+   depends on MFD_LP873X && OF
+   help
+ This driver supports LP873X voltage regulator chips. LP873X
+ provides two step-down converters and two general-purpose LDO
+ voltage regulators. It supports software based voltage control
+ for different voltage domains
+
 config REGULATOR_LP8755
tristate "TI LP8755 High Performance PMU driver"
depends on I2C
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 61bfbb9..7182b5f 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -42,6 +42,7 @@ obj-$(CONFIG_REGULATOR_LM363X) += lm363x-regulator.o
 obj-$(CONFIG_REGULATOR_LP3971) += lp3971.o
 obj-$(CONFIG_REGULATOR_LP3972) += lp3972.o
 obj-$(CONFIG_REGULATOR_LP872X) += lp872x.o
+obj-$(CONFIG_REGULATOR_LP873X) += lp873x-regulator.o
 obj-$(CONFIG_REGULATOR_LP8788) += lp8788-buck.o
 obj-$(CONFIG_REGULATOR_LP8788) += lp8788-ldo.o
 obj-$(CONFIG_REGULATOR_LP8755) += lp8755.o
diff --git a/drivers/regulator/lp873x-regulator.c 
b/drivers/regulator/lp873x-regulator.c
new file mode 100644
index 000..607eb13
--- /dev/null
+++ b/drivers/regulator/lp873x-regulator.c
@@ -0,0 +1,242 @@
+/*
+ * Regulator driver for LP873X PMIC
+ *
+ * Copyright (C) 2016 Texas Instruments Incorporated - http://www.ti.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 "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether expressed or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License version 2 for more details.
+ */
+
+#include 
+#include 
+#include 
+
+#include 
+
+#define LP873X_REGULATOR(_name, _id, _of, _ops, _n, _vr, _vm, _er, _em, \
+_delay, _lr, _nlr, _cr)\
+   [_id] = {   \
+   .desc = {   \
+   .name   = _name,\
+   .id = _id,  \
+   .of_match   = of_match_ptr(_of),\
+   .regulators_node= of_match_ptr("regulators"),\
+   .ops= &_ops,\
+   .n_voltages = _n,   \
+   .type   = REGULATOR_VOLTAGE,\
+   .owner  = THIS_MODULE,  \
+   .vsel_reg   = _vr,  \
+   .vsel_mask  = _vm,  \
+   .enable_reg = _er,  \
+   .enable_mask= _em,  \
+   .ramp_delay = _delay,   \
+   .linear_ranges  = _lr,  \
+   .n_linear_ranges= _nlr, \
+   },  \
+   .ctrl2_reg = _cr,   \
+   }
+
+struct lp873x_regulator {
+   struct regulator_desc desc;
+   unsigned int ctrl2_reg;
+};
+
+static const struct lp873x_regulator regulators[];
+
+static const struct regulator_linear_range buck0_buck1_ranges[] = {
+   REGULATOR_LINEAR_RANGE(0, 0x0, 0x13, 0),
+   REGULATOR_LINEAR_RANGE(70, 0x14, 0x17, 1),
+   REGULATOR_LINEAR_RANGE(735000, 0x18, 0x9d, 5000),
+   REGULATOR_LINEAR_RANGE(142, 0x9e, 0xff, 2),
+};
+
+static const struct regulator_linear_range ldo0_ldo1_ranges[] = {
+   REGULATOR_LINEAR_RANGE(80, 0x0, 0x19, 10),
+};
+
+static unsigned int lp873x_buck_ramp_delay[] = {
+   3, 15000, 1, 7500, 3800, 1900, 940, 470
+};
+
+/* LP873X BUCK current limit */

[PATCH 3/3] regulator: lp873x: Add support for lp873x PMIC regulators

[Keerthy]

The regulators set consists of 2 BUCKs and 2 LDOs. The output
voltages are configurable and are meant to supply power to the
main processor and other components. The ramp delay is configurable
for both BUCKs.

Signed-off-by: Keerthy 
---
 drivers/regulator/Kconfig|   9 ++
 drivers/regulator/Makefile   |   1 +
 drivers/regulator/lp873x-regulator.c | 242 +++
 3 files changed, 252 insertions(+)
 create mode 100644 drivers/regulator/lp873x-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index c77dc08..4d2d737 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -321,6 +321,15 @@ config REGULATOR_LP872X
help
  This driver supports LP8720/LP8725 PMIC
 
+config REGULATOR_LP873X
+   tristate "TI LP873X Power regulators"
+   depends on MFD_LP873X && OF
+   help
+ This driver supports LP873X voltage regulator chips. LP873X
+ provides two step-down converters and two general-purpose LDO
+ voltage regulators. It supports software based voltage control
+ for different voltage domains
+
 config REGULATOR_LP8755
tristate "TI LP8755 High Performance PMU driver"
depends on I2C
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 61bfbb9..7182b5f 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -42,6 +42,7 @@ obj-$(CONFIG_REGULATOR_LM363X) += lm363x-regulator.o
 obj-$(CONFIG_REGULATOR_LP3971) += lp3971.o
 obj-$(CONFIG_REGULATOR_LP3972) += lp3972.o
 obj-$(CONFIG_REGULATOR_LP872X) += lp872x.o
+obj-$(CONFIG_REGULATOR_LP873X) += lp873x-regulator.o
 obj-$(CONFIG_REGULATOR_LP8788) += lp8788-buck.o
 obj-$(CONFIG_REGULATOR_LP8788) += lp8788-ldo.o
 obj-$(CONFIG_REGULATOR_LP8755) += lp8755.o
diff --git a/drivers/regulator/lp873x-regulator.c 
b/drivers/regulator/lp873x-regulator.c
new file mode 100644
index 000..607eb13
--- /dev/null
+++ b/drivers/regulator/lp873x-regulator.c
@@ -0,0 +1,242 @@
+/*
+ * Regulator driver for LP873X PMIC
+ *
+ * Copyright (C) 2016 Texas Instruments Incorporated - http://www.ti.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 "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether expressed or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License version 2 for more details.
+ */
+
+#include 
+#include 
+#include 
+
+#include 
+
+#define LP873X_REGULATOR(_name, _id, _of, _ops, _n, _vr, _vm, _er, _em, \
+_delay, _lr, _nlr, _cr)\
+   [_id] = {   \
+   .desc = {   \
+   .name   = _name,\
+   .id = _id,  \
+   .of_match   = of_match_ptr(_of),\
+   .regulators_node= of_match_ptr("regulators"),\
+   .ops= &_ops,\
+   .n_voltages = _n,   \
+   .type   = REGULATOR_VOLTAGE,\
+   .owner  = THIS_MODULE,  \
+   .vsel_reg   = _vr,  \
+   .vsel_mask  = _vm,  \
+   .enable_reg = _er,  \
+   .enable_mask= _em,  \
+   .ramp_delay = _delay,   \
+   .linear_ranges  = _lr,  \
+   .n_linear_ranges= _nlr, \
+   },  \
+   .ctrl2_reg = _cr,   \
+   }
+
+struct lp873x_regulator {
+   struct regulator_desc desc;
+   unsigned int ctrl2_reg;
+};
+
+static const struct lp873x_regulator regulators[];
+
+static const struct regulator_linear_range buck0_buck1_ranges[] = {
+   REGULATOR_LINEAR_RANGE(0, 0x0, 0x13, 0),
+   REGULATOR_LINEAR_RANGE(70, 0x14, 0x17, 1),
+   REGULATOR_LINEAR_RANGE(735000, 0x18, 0x9d, 5000),
+   REGULATOR_LINEAR_RANGE(142, 0x9e, 0xff, 2),
+};
+
+static const struct regulator_linear_range ldo0_ldo1_ranges[] = {
+   REGULATOR_LINEAR_RANGE(80, 0x0, 0x19, 10),
+};
+
+static unsigned int lp873x_buck_ramp_delay[] = {
+   3, 15000, 1, 7500, 3800, 1900, 940, 470
+};
+
+/* LP873X BUCK current limit */
+static const