[PATCH 1/4] dt-bindings: tps65217: Update binding documentation.

[PATCH 1/4] dt-bindings: tps65217: Update binding documentation.

[Enric Balletbo i Serra]

This patch adds a new binding documentation for the TPS65217 MFD and
updates the documentation for all the sub-devices in accordance to get
each individual sub-driver functioning correctly.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
 .../bindings/input/tps65218-pwrbutton.txt          |   2 +-
 .../bindings/leds/backlight/tps65217-backlight.txt |  24 ++---
 Documentation/devicetree/bindings/mfd/tps65217.txt | 100 +++++++++++++++++++++
 .../devicetree/bindings/regulator/tps65217.txt     |   8 +-
 4 files changed, 119 insertions(+), 15 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/tps65217.txt

diff --git a/Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt b/Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt
index 8682ab6..603a3f0 100644
--- a/Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt
+++ b/Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt
@@ -1,7 +1,7 @@
 Texas Instruments TPS65217 and TPS65218 power button
 
 This module is part of the TPS65217/TPS65218. For more details about the whole
-TPS65217 chip see Documentation/devicetree/bindings/regulator/tps65217.txt.
+TPS65217 chip see Documentation/devicetree/bindings/mfd/tps65217.txt.
 
 This driver provides a simple power button event via an Interrupt.
 
diff --git a/Documentation/devicetree/bindings/leds/backlight/tps65217-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/tps65217-backlight.txt
index 5fb9279..a1bc465 100644
--- a/Documentation/devicetree/bindings/leds/backlight/tps65217-backlight.txt
+++ b/Documentation/devicetree/bindings/leds/backlight/tps65217-backlight.txt
@@ -1,11 +1,13 @@
-TPS65217 family of regulators
+Texas Instruments TPS65217 backlight regulator
+
+This module is part of the TPS65217. For more details about the whole
+TPS65217 chip see Documentation/devicetree/bindings/mfd/tps65217.txt.
 
 The TPS65217 chip contains a boost converter and current sinks which can be
 used to drive LEDs for use as backlights.
 
 Required properties:
-- compatible: "ti,tps65217"
-- reg: I2C slave address
+- compatible: "ti,tps65217-bl"
 - backlight: node for specifying WLED1 and WLED2 lines in TPS65217
 - isel: selection bit, valid values: 1 for ISEL1 (low-level) and 2 for ISEL2 (high-level)
 - fdim: PWM dimming frequency, valid values: 100, 200, 500, 1000
@@ -15,13 +17,13 @@ Each regulator is defined using the standard binding for regulators.
 
 Example:
 
-	tps: tps@24 {
-		reg = <0x24>;
-		compatible = "ti,tps65217";
-		backlight {
-			isel = <1>;  /* 1 - ISET1, 2 ISET2 */
-			fdim = <100>; /* TPS65217_BL_FDIM_100HZ */
-			default-brightness = <50>;
-		};
+&tps {
+	backlight {
+		compatible = "ti,tps65217-bl";
+		status = "okay";
+		isel = <1>;  /* 1 - ISET1, 2 ISET2 */
+		fdim = <100>; /* TPS65217_BL_FDIM_100HZ */
+		default-brightness = <50>;
 	};
+};
 
diff --git a/Documentation/devicetree/bindings/mfd/tps65217.txt b/Documentation/devicetree/bindings/mfd/tps65217.txt
new file mode 100644
index 0000000..40c84ba
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/tps65217.txt
@@ -0,0 +1,100 @@
+Texas Instruments TPS65217 Single-Chip PMIC for Battery-Powered Systems
+
+Required properties:
+- compatible: "ti,tps65217"
+- reg: I2C slave address.
+- interrupt-controller: Marks the device node as an interrupt controller.
+- #interrupt-cells: The number of cells to describe an IRQ, this should be 1.
+- backlight: Child node that specify the backlight regulator sub-device. See:
+	Documentation/devicetree/bindings/leds/backlight/tps65217-backlight.txt
+- charger: Child node that specify the charger sub-device. See:
+	Documentation/devicetree/bindings/power/supply/tps65217_charger.txt
+- pwrbutton: Child node that specify the power button sub-device. See:
+	Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt
+- regulators: List of child nodes that specify the regulator initialization
+	data. See:
+	Documentation/devicetree/bindings/regulator/tps65217.txt.
+
+Optional properties:
+- ti,pmic-shutdown-controller: Telling the PMIC to shutdown on PWR_EN toggle.
+
+Example:
+
+	tps: tps@24 {
+		compatible = "ti,tps65217";
+		interrupt-controller;
+		#interrupt-cells = <1>;
+
+		ti,pmic-shutdown-controller;
+
+		backlight {
+			compatible = "ti,tps65217-bl";
+			status = "disabled";
+		};
+
+		charger {
+			compatible = "ti,tps65217-charger";
+			status = "disabled";
+		};
+
+		pwrbutton {
+			compatible = "ti,tps65217-pwrbutton";
+			status = "disabled";
+		};
+
+		regulators {
+			compatible = "ti,tps65217-pmic";
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			dcdc1_reg: dcdc1 {
+				regulator-min-microvolt = <900000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+
+			dcdc2_reg: dcdc2 {
+				regulator-min-microvolt = <900000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+
+			dcdc3_reg: dcc3 {
+				regulator-min-microvolt = <900000>;
+				regulator-max-microvolt = <1500000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+
+			ldo1_reg: ldo1 {
+				regulator-min-microvolt = <1000000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+
+			ldo2_reg: ldo2 {
+				regulator-min-microvolt = <900000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+
+			ldo3_reg: ldo3 {
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+
+			ldo4_reg: ldo4 {
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+		};
+	};
+
diff --git a/Documentation/devicetree/bindings/regulator/tps65217.txt b/Documentation/devicetree/bindings/regulator/tps65217.txt
index 4f05d20..2d9b519 100644
--- a/Documentation/devicetree/bindings/regulator/tps65217.txt
+++ b/Documentation/devicetree/bindings/regulator/tps65217.txt
@@ -1,8 +1,10 @@
-TPS65217 family of regulators
+Texas Instruments TPS65217 family of regulators
+
+This module is part of the TPS65217. For more details about the whole
+TPS65217 chip see Documentation/devicetree/bindings/mfd/tps65217.txt.
 
 Required properties:
-- compatible: "ti,tps65217"
-- reg: I2C slave address
+- compatible: "ti,tps65217-pmic"
 - regulators: list of regulators provided by this controller, must be named
   after their hardware counterparts: dcdc[1-3] and ldo[1-4]
 - regulators: This is the list of child nodes that specify the regulator
-- 
2.9.3

[PATCH 2/4] ARM: dts: tps65217: Add backlight and pmic device

[Enric Balletbo i Serra]

Support the backlight driver but disable by default and also add the
compatible string for regulators.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
 arch/arm/boot/dts/tps65217.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/tps65217.dtsi b/arch/arm/boot/dts/tps65217.dtsi
index 02de56b..233581f 100644
--- a/arch/arm/boot/dts/tps65217.dtsi
+++ b/arch/arm/boot/dts/tps65217.dtsi
@@ -16,6 +16,11 @@
 	interrupt-controller;
 	#interrupt-cells = <1>;
 
+	backlight {
+		compatible = "ti,tps65217-bl";
+		status = "disabled";
+	};
+
 	charger {
 		compatible = "ti,tps65217-charger";
 		status = "disabled";
@@ -27,6 +32,7 @@
 	};
 
 	regulators {
+		compatible = "ti,tps65217-pmic";
 		#address-cells = <1>;
 		#size-cells = <0>;
 
-- 
2.9.3

[PATCH 3/4] regulator: tps65217: Fix module autoload for devices registered via OF

[Enric Balletbo i Serra]

The driver doesn't have a OF device ID table and so if devices are
registered via OF, module autoloading won't work.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
 drivers/regulator/tps65217-regulator.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/regulator/tps65217-regulator.c b/drivers/regulator/tps65217-regulator.c
index 5324dc9..2c05360 100644
--- a/drivers/regulator/tps65217-regulator.c
+++ b/drivers/regulator/tps65217-regulator.c
@@ -263,9 +263,16 @@ static int tps65217_regulator_probe(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct of_device_id tps65217_regulator_match_table[] = {
+	{ .compatible = "ti,tps65217-pmic", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, tps65217_regulator_match_table);
+
 static struct platform_driver tps65217_regulator_driver = {
 	.driver = {
 		.name = "tps65217-pmic",
+		.of_match_table = tps65217_regulator_match_table,
 	},
 	.probe = tps65217_regulator_probe,
 };
-- 
2.9.3

[PATCH 4/4] mfd: tps65217: Instantiate sub-devices from device tree

[Enric Balletbo i Serra]

The driver boots only via device tree but currently all the MFD sub-devices
are instatiated independently of the device tree configuration, i.e
although tps65217-charger is disabled by default it's instantiated by the
MFD driver.

Instead of register all sub-devices, if the TPS65217 device tree node has a
sub-node enabled, try to instatiate them as MFD sub-devices but not
instantiate sub-nodes that are not enabled.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
 drivers/mfd/tps65217.c       | 56 +++++++-------------------------------------
 include/linux/mfd/tps65217.h |  6 -----
 2 files changed, 9 insertions(+), 53 deletions(-)

diff --git a/drivers/mfd/tps65217.c b/drivers/mfd/tps65217.c
index f769c7d..9effdda 100644
--- a/drivers/mfd/tps65217.c
+++ b/drivers/mfd/tps65217.c
@@ -33,14 +33,7 @@
 #include <linux/mfd/core.h>
 #include <linux/mfd/tps65217.h>
 
-static struct resource charger_resources[] = {
-	DEFINE_RES_IRQ_NAMED(TPS65217_IRQ_AC, "AC"),
-	DEFINE_RES_IRQ_NAMED(TPS65217_IRQ_USB, "USB"),
-};
-
-static struct resource pb_resources[] = {
-	DEFINE_RES_IRQ_NAMED(TPS65217_IRQ_PB, "PB"),
-};
+#define TPS65217_NUM_IRQ	3
 
 static void tps65217_irq_lock(struct irq_data *data)
 {
@@ -86,29 +79,6 @@ static struct irq_chip tps65217_irq_chip = {
 	.irq_disable		= tps65217_irq_disable,
 };
 
-static struct mfd_cell tps65217s[] = {
-	{
-		.name = "tps65217-pmic",
-		.of_compatible = "ti,tps65217-pmic",
-	},
-	{
-		.name = "tps65217-bl",
-		.of_compatible = "ti,tps65217-bl",
-	},
-	{
-		.name = "tps65217-charger",
-		.num_resources = ARRAY_SIZE(charger_resources),
-		.resources = charger_resources,
-		.of_compatible = "ti,tps65217-charger",
-	},
-	{
-		.name = "tps65217-pwrbutton",
-		.num_resources = ARRAY_SIZE(pb_resources),
-		.resources = pb_resources,
-		.of_compatible = "ti,tps65217-pwrbutton",
-	},
-};
-
 static irqreturn_t tps65217_irq_thread(int irq, void *data)
 {
 	struct tps65217 *tps = data;
@@ -359,23 +329,8 @@ static int tps65217_probe(struct i2c_client *client,
 		return ret;
 	}
 
-	if (client->irq) {
+	if (client->irq)
 		tps65217_irq_init(tps, client->irq);
-	} else {
-		int i;
-
-		/* Don't tell children about IRQ resources which won't fire */
-		for (i = 0; i < ARRAY_SIZE(tps65217s); i++)
-			tps65217s[i].num_resources = 0;
-	}
-
-	ret = devm_mfd_add_devices(tps->dev, -1, tps65217s,
-				   ARRAY_SIZE(tps65217s), NULL, 0,
-				   tps->irq_domain);
-	if (ret < 0) {
-		dev_err(tps->dev, "mfd_add_devices failed: %d\n", ret);
-		return ret;
-	}
 
 	ret = tps65217_reg_read(tps, TPS65217_REG_CHIPID, &version);
 	if (ret < 0) {
@@ -384,6 +339,13 @@ static int tps65217_probe(struct i2c_client *client,
 		return ret;
 	}
 
+	ret = of_platform_populate(client->dev.of_node, NULL, NULL,
+				   &client->dev);
+	if (ret) {
+		dev_err(&client->dev, "Failed to register sub-devices\n");
+		return ret;
+	}
+
 	/* Set the PMIC to shutdown on PWR_EN toggle */
 	if (status_off) {
 		ret = tps65217_set_bits(tps, TPS65217_REG_STATUS,
diff --git a/include/linux/mfd/tps65217.h b/include/linux/mfd/tps65217.h
index eac2857..dfc51f5 100644
--- a/include/linux/mfd/tps65217.h
+++ b/include/linux/mfd/tps65217.h
@@ -236,12 +236,6 @@ struct tps65217_bl_pdata {
 	int dft_brightness;
 };
 
-/* Interrupt numbers */
-#define TPS65217_IRQ_USB		0
-#define TPS65217_IRQ_AC			1
-#define TPS65217_IRQ_PB			2
-#define TPS65217_NUM_IRQ		3
-
 /**
  * struct tps65217_board - packages regulator init data
  * @tps65217_regulator_data: regulator initialization values
-- 
2.9.3

Re: [PATCH 4/4] mfd: tps65217: Instantiate sub-devices from device tree

[Grygorii Strashko]

On 06/07/2017 05:32 AM, Enric Balletbo i Serra wrote:
> The driver boots only via device tree but currently all the MFD sub-devices
> are instatiated independently of the device tree configuration, i.e
> although tps65217-charger is disabled by default it's instantiated by the
> MFD driver.
> 
> Instead of register all sub-devices, if the TPS65217 device tree node has a
> sub-node enabled, try to instatiate them as MFD sub-devices but not
> instantiate sub-nodes that are not enabled.
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
>   drivers/mfd/tps65217.c       | 56 +++++++-------------------------------------
>   include/linux/mfd/tps65217.h |  6 -----
>   2 files changed, 9 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/mfd/tps65217.c b/drivers/mfd/tps65217.c
> index f769c7d..9effdda 100644
> --- a/drivers/mfd/tps65217.c
> +++ b/drivers/mfd/tps65217.c
> @@ -33,14 +33,7 @@
>   #include <linux/mfd/core.h>
>   #include <linux/mfd/tps65217.h>
>   
> -static struct resource charger_resources[] = {
> -	DEFINE_RES_IRQ_NAMED(TPS65217_IRQ_AC, "AC"),
> -	DEFINE_RES_IRQ_NAMED(TPS65217_IRQ_USB, "USB"),
> -};
> -
> -static struct resource pb_resources[] = {
> -	DEFINE_RES_IRQ_NAMED(TPS65217_IRQ_PB, "PB"),
> -};

May be I messed smth, but how about interrupts for charger and pwrbutton?

> +#define TPS65217_NUM_IRQ	3
>   
>   static void tps65217_irq_lock(struct irq_data *data)
>   {
> @@ -86,29 +79,6 @@ static struct irq_chip tps65217_irq_chip = {
>   	.irq_disable		= tps65217_irq_disable,
>   };
>   
> -static struct mfd_cell tps65217s[] = {
> -	{
> -		.name = "tps65217-pmic",
> -		.of_compatible = "ti,tps65217-pmic",
> -	},
> -	{
> -		.name = "tps65217-bl",
> -		.of_compatible = "ti,tps65217-bl",
> -	},
> -	{
> -		.name = "tps65217-charger",
> -		.num_resources = ARRAY_SIZE(charger_resources),
> -		.resources = charger_resources,
> -		.of_compatible = "ti,tps65217-charger",
> -	},
> -	{
> -		.name = "tps65217-pwrbutton",
> -		.num_resources = ARRAY_SIZE(pb_resources),
> -		.resources = pb_resources,
> -		.of_compatible = "ti,tps65217-pwrbutton",
> -	},
> -};
> -
>   static irqreturn_t tps65217_irq_thread(int irq, void *data)
>   {
>   	struct tps65217 *tps = data;
> @@ -359,23 +329,8 @@ static int tps65217_probe(struct i2c_client *client,
>   		return ret;
>   	}
>   
> -	if (client->irq) {
> +	if (client->irq)
>   		tps65217_irq_init(tps, client->irq);
> -	} else {
> -		int i;
> -
> -		/* Don't tell children about IRQ resources which won't fire */
> -		for (i = 0; i < ARRAY_SIZE(tps65217s); i++)
> -			tps65217s[i].num_resources = 0;
> -	}
> -
> -	ret = devm_mfd_add_devices(tps->dev, -1, tps65217s,
> -				   ARRAY_SIZE(tps65217s), NULL, 0,
> -				   tps->irq_domain);
> -	if (ret < 0) {
> -		dev_err(tps->dev, "mfd_add_devices failed: %d\n", ret);
> -		return ret;
> -	}

And as I remember there was a request to use mfd_add_devices()
and not of_platform_populate() for mfd sub-devices instantiation.

 
>   
>   	ret = tps65217_reg_read(tps, TPS65217_REG_CHIPID, &version);
>   	if (ret < 0) {
> @@ -384,6 +339,13 @@ static int tps65217_probe(struct i2c_client *client,
>   		return ret;
>   	}
>   
> +	ret = of_platform_populate(client->dev.of_node, NULL, NULL,
> +				   &client->dev);
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to register sub-devices\n");
> +		return ret;
> +	}
> +
>   	/* Set the PMIC to shutdown on PWR_EN toggle */
>   	if (status_off) {
>   		ret = tps65217_set_bits(tps, TPS65217_REG_STATUS,
> diff --git a/include/linux/mfd/tps65217.h b/include/linux/mfd/tps65217.h
> index eac2857..dfc51f5 100644
> --- a/include/linux/mfd/tps65217.h
> +++ b/include/linux/mfd/tps65217.h
> @@ -236,12 +236,6 @@ struct tps65217_bl_pdata {
>   	int dft_brightness;
>   };
>   
> -/* Interrupt numbers */
> -#define TPS65217_IRQ_USB		0
> -#define TPS65217_IRQ_AC			1
> -#define TPS65217_IRQ_PB			2
> -#define TPS65217_NUM_IRQ		3
> -
>   /**
>    * struct tps65217_board - packages regulator init data
>    * @tps65217_regulator_data: regulator initialization values
> 

-- 
regards,
-grygorii

Re: [PATCH 4/4] mfd: tps65217: Instantiate sub-devices from device tree

[Enric Balletbo Serra]

2017-06-07 18:05 GMT+02:00 Grygorii Strashko <grygorii.strashko@ti.com>:
>
>
> On 06/07/2017 05:32 AM, Enric Balletbo i Serra wrote:
>> The driver boots only via device tree but currently all the MFD sub-devices
>> are instatiated independently of the device tree configuration, i.e
>> although tps65217-charger is disabled by default it's instantiated by the
>> MFD driver.
>>
>> Instead of register all sub-devices, if the TPS65217 device tree node has a
>> sub-node enabled, try to instatiate them as MFD sub-devices but not
>> instantiate sub-nodes that are not enabled.
>>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> ---
>>   drivers/mfd/tps65217.c       | 56 +++++++-------------------------------------
>>   include/linux/mfd/tps65217.h |  6 -----
>>   2 files changed, 9 insertions(+), 53 deletions(-)
>>
>> diff --git a/drivers/mfd/tps65217.c b/drivers/mfd/tps65217.c
>> index f769c7d..9effdda 100644
>> --- a/drivers/mfd/tps65217.c
>> +++ b/drivers/mfd/tps65217.c
>> @@ -33,14 +33,7 @@
>>   #include <linux/mfd/core.h>
>>   #include <linux/mfd/tps65217.h>
>>
>> -static struct resource charger_resources[] = {
>> -     DEFINE_RES_IRQ_NAMED(TPS65217_IRQ_AC, "AC"),
>> -     DEFINE_RES_IRQ_NAMED(TPS65217_IRQ_USB, "USB"),
>> -};
>> -
>> -static struct resource pb_resources[] = {
>> -     DEFINE_RES_IRQ_NAMED(TPS65217_IRQ_PB, "PB"),
>> -};
>
> May be I messed smth, but how about interrupts for charger and pwrbutton?
>

I might be wrong but as this driver is DT-only these resources came
from the DT. The driver calls platform_get_irq_byname and then
of_irq_get_byname.

i.e arch/arm/boot/dts/am335x-bone-common.dtsi

&tps {
        charger {
                interrupts = <0>, <1>;
                interrupt-names = "USB", "AC";
                status = "okay";
        };

        pwrbutton {
                interrupts = <2>;
                status = "okay";
        };
};

>> +#define TPS65217_NUM_IRQ     3
>>
>>   static void tps65217_irq_lock(struct irq_data *data)
>>   {
>> @@ -86,29 +79,6 @@ static struct irq_chip tps65217_irq_chip = {
>>       .irq_disable            = tps65217_irq_disable,
>>   };
>>
>> -static struct mfd_cell tps65217s[] = {
>> -     {
>> -             .name = "tps65217-pmic",
>> -             .of_compatible = "ti,tps65217-pmic",
>> -     },
>> -     {
>> -             .name = "tps65217-bl",
>> -             .of_compatible = "ti,tps65217-bl",
>> -     },
>> -     {
>> -             .name = "tps65217-charger",
>> -             .num_resources = ARRAY_SIZE(charger_resources),
>> -             .resources = charger_resources,
>> -             .of_compatible = "ti,tps65217-charger",
>> -     },
>> -     {
>> -             .name = "tps65217-pwrbutton",
>> -             .num_resources = ARRAY_SIZE(pb_resources),
>> -             .resources = pb_resources,
>> -             .of_compatible = "ti,tps65217-pwrbutton",
>> -     },
>> -};
>> -
>>   static irqreturn_t tps65217_irq_thread(int irq, void *data)
>>   {
>>       struct tps65217 *tps = data;
>> @@ -359,23 +329,8 @@ static int tps65217_probe(struct i2c_client *client,
>>               return ret;
>>       }
>>
>> -     if (client->irq) {
>> +     if (client->irq)
>>               tps65217_irq_init(tps, client->irq);
>> -     } else {
>> -             int i;
>> -
>> -             /* Don't tell children about IRQ resources which won't fire */
>> -             for (i = 0; i < ARRAY_SIZE(tps65217s); i++)
>> -                     tps65217s[i].num_resources = 0;
>> -     }
>> -
>> -     ret = devm_mfd_add_devices(tps->dev, -1, tps65217s,
>> -                                ARRAY_SIZE(tps65217s), NULL, 0,
>> -                                tps->irq_domain);
>> -     if (ret < 0) {
>> -             dev_err(tps->dev, "mfd_add_devices failed: %d\n", ret);
>> -             return ret;
>> -     }
>
> And as I remember there was a request to use mfd_add_devices()
> and not of_platform_populate() for mfd sub-devices instantiation.
>

>From what I know or you add the sub-devices via mfd_add_devices or
using the DT with of_platform_populate, and the first is probably the
preferred, but in this specific case this driver is DT-only so IMHO
makes more sense adding via the DT, there are already some bindings
i.e:
 arch/arm/boot/dts/tps65217.dtsi
 arch/arm/boot/dts/am335x-bone-common.dtsi

Let me explain a bit more,  assume that you have another AM335x with
the TPS65217 PMIC but you don't want the charger because is not wired
in your board. Your board DT will include the tps65217.dtsi, in this
file you can see:

&tps {
       ...
        charger {
                compatible = "ti,tps65217-charger";
                status = "disabled";
        };
        ...
};

Seems that's fine but actually not works as expected, the TPS65217 MFD
registers all the sub-devices so the charger is enabled and running
even you have status = "disabled" in your DT. This looks incoherent to
me, hence I replaced the devm_mfd_add_devices for the
of_platform_populate which takes care of the status propriety and only
calls the probe of the sub-devices that match and are enabled (status
= "okay").

>
>>
>>       ret = tps65217_reg_read(tps, TPS65217_REG_CHIPID, &version);
>>       if (ret < 0) {
>> @@ -384,6 +339,13 @@ static int tps65217_probe(struct i2c_client *client,
>>               return ret;
>>       }
>>
>> +     ret = of_platform_populate(client->dev.of_node, NULL, NULL,
>> +                                &client->dev);
>> +     if (ret) {
>> +             dev_err(&client->dev, "Failed to register sub-devices\n");
>> +             return ret;
>> +     }
>> +
>>       /* Set the PMIC to shutdown on PWR_EN toggle */
>>       if (status_off) {
>>               ret = tps65217_set_bits(tps, TPS65217_REG_STATUS,
>> diff --git a/include/linux/mfd/tps65217.h b/include/linux/mfd/tps65217.h
>> index eac2857..dfc51f5 100644
>> --- a/include/linux/mfd/tps65217.h
>> +++ b/include/linux/mfd/tps65217.h
>> @@ -236,12 +236,6 @@ struct tps65217_bl_pdata {
>>       int dft_brightness;
>>   };
>>
>> -/* Interrupt numbers */
>> -#define TPS65217_IRQ_USB             0
>> -#define TPS65217_IRQ_AC                      1
>> -#define TPS65217_IRQ_PB                      2
>> -#define TPS65217_NUM_IRQ             3
>> -
>>   /**
>>    * struct tps65217_board - packages regulator init data
>>    * @tps65217_regulator_data: regulator initialization values
>>
>
> --
> regards,
> -grygorii
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] mfd: tps65217: Instantiate sub-devices from device tree

[Grygorii Strashko]

On 06/08/2017 08:16 AM, Enric Balletbo Serra wrote:
> 2017-06-07 18:05 GMT+02:00 Grygorii Strashko <grygorii.strashko@ti.com>:
>>
>>
>> On 06/07/2017 05:32 AM, Enric Balletbo i Serra wrote:
>>> The driver boots only via device tree but currently all the MFD sub-devices
>>> are instatiated independently of the device tree configuration, i.e
>>> although tps65217-charger is disabled by default it's instantiated by the
>>> MFD driver.
>>>
>>> Instead of register all sub-devices, if the TPS65217 device tree node has a
>>> sub-node enabled, try to instatiate them as MFD sub-devices but not
>>> instantiate sub-nodes that are not enabled.
>>>
>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>> ---
>>>    drivers/mfd/tps65217.c       | 56 +++++++-------------------------------------
>>>    include/linux/mfd/tps65217.h |  6 -----
>>>    2 files changed, 9 insertions(+), 53 deletions(-)
>>>
>>> diff --git a/drivers/mfd/tps65217.c b/drivers/mfd/tps65217.c
>>> index f769c7d..9effdda 100644
>>> --- a/drivers/mfd/tps65217.c
>>> +++ b/drivers/mfd/tps65217.c
>>> @@ -33,14 +33,7 @@
>>>    #include <linux/mfd/core.h>
>>>    #include <linux/mfd/tps65217.h>
>>>
>>> -static struct resource charger_resources[] = {
>>> -     DEFINE_RES_IRQ_NAMED(TPS65217_IRQ_AC, "AC"),
>>> -     DEFINE_RES_IRQ_NAMED(TPS65217_IRQ_USB, "USB"),
>>> -};
>>> -
>>> -static struct resource pb_resources[] = {
>>> -     DEFINE_RES_IRQ_NAMED(TPS65217_IRQ_PB, "PB"),
>>> -};
>>
>> May be I messed smth, but how about interrupts for charger and pwrbutton?
>>
> 
> I might be wrong but as this driver is DT-only these resources came
> from the DT. The driver calls platform_get_irq_byname and then
> of_irq_get_byname.
> 
> i.e arch/arm/boot/dts/am335x-bone-common.dtsi
> 
> &tps {
>          charger {
>                  interrupts = <0>, <1>;
>                  interrupt-names = "USB", "AC";
>                  status = "okay";
>          };
> 
>          pwrbutton {
>                  interrupts = <2>;
>                  status = "okay";
>          };
> };

Sry, but this make no sense - those IRQ configuration is static,
so it should be defined in arch/arm/boot/dts/tps65217.dtsi at least.
But, again, it also not required, because it is strictly static for 
this particular device and so defined in code.
And  platform_get_irq_byname() should perfectly work with mfd_add_devices().

Also not that  tps65217.dtsi included in below files, but 

./arch/arm/boot/dts/am335x-bone-common.dtsi:/include/ "tps65217.dtsi"
./arch/arm/boot/dts/am335x-chilisom.dtsi:/include/ "tps65217.dtsi"
./arch/arm/boot/dts/am335x-nano.dts:#include "tps65217.dtsi"
./arch/arm/boot/dts/am335x-pepper.dts:/include/ "tps65217.dtsi"
./arch/arm/boot/dts/am335x-sl50.dts:#include "tps65217.dtsi"

but IRQs defined only in
./arch/arm/boot/dts/am335x-bone-common.dtsi:		interrupt-names = "USB", "AC";
./arch/arm/boot/dts/am335x-chiliboard.dts:		interrupt-names = "USB", "AC";

So how are other boards working?


> 
>>> +#define TPS65217_NUM_IRQ     3
>>>
>>>    static void tps65217_irq_lock(struct irq_data *data)
>>>    {
>>> @@ -86,29 +79,6 @@ static struct irq_chip tps65217_irq_chip = {
>>>        .irq_disable            = tps65217_irq_disable,
>>>    };
>>>
>>> -static struct mfd_cell tps65217s[] = {
>>> -     {
>>> -             .name = "tps65217-pmic",
>>> -             .of_compatible = "ti,tps65217-pmic",
>>> -     },
>>> -     {
>>> -             .name = "tps65217-bl",
>>> -             .of_compatible = "ti,tps65217-bl",
>>> -     },
>>> -     {
>>> -             .name = "tps65217-charger",
>>> -             .num_resources = ARRAY_SIZE(charger_resources),
>>> -             .resources = charger_resources,
>>> -             .of_compatible = "ti,tps65217-charger",
>>> -     },
>>> -     {
>>> -             .name = "tps65217-pwrbutton",
>>> -             .num_resources = ARRAY_SIZE(pb_resources),
>>> -             .resources = pb_resources,
>>> -             .of_compatible = "ti,tps65217-pwrbutton",
>>> -     },
>>> -};
>>> -
>>>    static irqreturn_t tps65217_irq_thread(int irq, void *data)
>>>    {
>>>        struct tps65217 *tps = data;
>>> @@ -359,23 +329,8 @@ static int tps65217_probe(struct i2c_client *client,
>>>                return ret;
>>>        }
>>>
>>> -     if (client->irq) {
>>> +     if (client->irq)
>>>                tps65217_irq_init(tps, client->irq);
>>> -     } else {
>>> -             int i;
>>> -
>>> -             /* Don't tell children about IRQ resources which won't fire */
>>> -             for (i = 0; i < ARRAY_SIZE(tps65217s); i++)
>>> -                     tps65217s[i].num_resources = 0;
>>> -     }
>>> -
>>> -     ret = devm_mfd_add_devices(tps->dev, -1, tps65217s,
>>> -                                ARRAY_SIZE(tps65217s), NULL, 0,
>>> -                                tps->irq_domain);
>>> -     if (ret < 0) {
>>> -             dev_err(tps->dev, "mfd_add_devices failed: %d\n", ret);
>>> -             return ret;
>>> -     }
>>
>> And as I remember there was a request to use mfd_add_devices()
>> and not of_platform_populate() for mfd sub-devices instantiation.
>>
> 
>  From what I know or you add the sub-devices via mfd_add_devices or
> using the DT with of_platform_populate, and the first is probably the
> preferred, but in this specific case this driver is DT-only so IMHO
> makes more sense adding via the DT, there are already some bindings
> i.e:
>   arch/arm/boot/dts/tps65217.dtsi
>   arch/arm/boot/dts/am335x-bone-common.dtsi
> 
> Let me explain a bit more,  assume that you have another AM335x with
> the TPS65217 PMIC but you don't want the charger because is not wired
> in your board. Your board DT will include the tps65217.dtsi, in this
> file you can see:
> 
> &tps {
>         ...
>          charger {
>                  compatible = "ti,tps65217-charger";
>                  status = "disabled";
>          };
>          ...
> };
> 
> Seems that's fine but actually not works as expected, the TPS65217 MFD
> registers all the sub-devices so the charger is enabled and running
> even you have status = "disabled" in your DT. This looks incoherent to
> me, hence I replaced the devm_mfd_add_devices() for the
> of_platform_populate which takes care of the status propriety and only
> calls the probe of the sub-devices that match and are enabled (status
> = "okay").

I can't find links on corresponding discussions, but mfd_add_devices() is
preferred for MDF devices. Below is one commit i've found. Also you can compare number of
drivers using mfd_add_devices() and of_platform_populate().


regarding your problem - I think, It might be reasonable from your side to propose change to
mfd_add_device() which will take into account 'status = "disabled";' DT property.

---
commit 4531156db726d27e593d35800d43c74be4e393b9
Author: Keerthy <j-keerthy@ti.com>
Date:   Mon Sep 19 13:09:05 2016 +0530

    mfd: tps65218: Use mfd_add_devices instead of of_platform_populate
    
    mfd_add_devices enables parsing device tree nodes without compatibles
    for regulators and gpio modules. Replace of_platform_populate with
    mfd_add_devices. mfd_cell currently is populated with regulators,
    gpio and powerbutton.
    
    Signed-off-by: Keerthy <j-keerthy@ti.com>
    Signed-off-by: Lee Jones <lee.jones@linaro.org>





-- 
regards,
-grygorii

Re: [PATCH 4/4] mfd: tps65217: Instantiate sub-devices from device tree

[Enric Balletbo Serra]

2017-06-08 19:11 GMT+02:00 Grygorii Strashko <grygorii.strashko@ti.com>:
>
>
> On 06/08/2017 08:16 AM, Enric Balletbo Serra wrote:
>> 2017-06-07 18:05 GMT+02:00 Grygorii Strashko <grygorii.strashko@ti.com>:
>>>
>>>
>>> On 06/07/2017 05:32 AM, Enric Balletbo i Serra wrote:
>>>> The driver boots only via device tree but currently all the MFD sub-devices
>>>> are instatiated independently of the device tree configuration, i.e
>>>> although tps65217-charger is disabled by default it's instantiated by the
>>>> MFD driver.
>>>>
>>>> Instead of register all sub-devices, if the TPS65217 device tree node has a
>>>> sub-node enabled, try to instatiate them as MFD sub-devices but not
>>>> instantiate sub-nodes that are not enabled.
>>>>
>>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>>> ---
>>>>    drivers/mfd/tps65217.c       | 56 +++++++-------------------------------------
>>>>    include/linux/mfd/tps65217.h |  6 -----
>>>>    2 files changed, 9 insertions(+), 53 deletions(-)
>>>>
>>>> diff --git a/drivers/mfd/tps65217.c b/drivers/mfd/tps65217.c
>>>> index f769c7d..9effdda 100644
>>>> --- a/drivers/mfd/tps65217.c
>>>> +++ b/drivers/mfd/tps65217.c
>>>> @@ -33,14 +33,7 @@
>>>>    #include <linux/mfd/core.h>
>>>>    #include <linux/mfd/tps65217.h>
>>>>
>>>> -static struct resource charger_resources[] = {
>>>> -     DEFINE_RES_IRQ_NAMED(TPS65217_IRQ_AC, "AC"),
>>>> -     DEFINE_RES_IRQ_NAMED(TPS65217_IRQ_USB, "USB"),
>>>> -};
>>>> -
>>>> -static struct resource pb_resources[] = {
>>>> -     DEFINE_RES_IRQ_NAMED(TPS65217_IRQ_PB, "PB"),
>>>> -};
>>>
>>> May be I messed smth, but how about interrupts for charger and pwrbutton?
>>>
>>
>> I might be wrong but as this driver is DT-only these resources came
>> from the DT. The driver calls platform_get_irq_byname and then
>> of_irq_get_byname.
>>
>> i.e arch/arm/boot/dts/am335x-bone-common.dtsi
>>
>> &tps {
>>          charger {
>>                  interrupts = <0>, <1>;
>>                  interrupt-names = "USB", "AC";
>>                  status = "okay";
>>          };
>>
>>          pwrbutton {
>>                  interrupts = <2>;
>>                  status = "okay";
>>          };
>> };
>
> Sry, but this make no sense - those IRQ configuration is static,
> so it should be defined in arch/arm/boot/dts/tps65217.dtsi at least.

I was describing the state-of-art not what should be. If you mean that
what doesn't make sense have these interrupts portions in the DT and
the resources in the driver I'm completely agree. So we have two
options:

 1) Get rid of the irq resources from tps65217 MFD driver and
configure all with the DT (these patches)
 2) Get rid of the DT portions as doesn't make sense have them in two places.

If we select 2) at least we have the problem that currently all
sub-devices are instantiated and there is no way to disable a
sub-device (the problem I'm trying to solve) hence I proposed 1)

> But, again, it also not required, because it is strictly static for
> this particular device and so defined in code.

DT describes the hardware and I guess is full of static data so I
don't think this would be a problem.

> And  platform_get_irq_byname() should perfectly work with mfd_add_devices().
>

Agree, for a DT-only driver it would check first in the DT
(of_irq_get_byname) and if it fails get the resource with
platform_get_resource_byname. I don't have an answer here about if
it's preferred provide this info in the DT or hard-coded in the
driver.

> Also not that  tps65217.dtsi included in below files, but
>
> ./arch/arm/boot/dts/am335x-bone-common.dtsi:/include/ "tps65217.dtsi"
> ./arch/arm/boot/dts/am335x-chilisom.dtsi:/include/ "tps65217.dtsi"
> ./arch/arm/boot/dts/am335x-nano.dts:#include "tps65217.dtsi"
> ./arch/arm/boot/dts/am335x-pepper.dts:/include/ "tps65217.dtsi"
> ./arch/arm/boot/dts/am335x-sl50.dts:#include "tps65217.dtsi"
>
> but IRQs defined only in
> ./arch/arm/boot/dts/am335x-bone-common.dtsi:            interrupt-names = "USB", "AC";
> ./arch/arm/boot/dts/am335x-chiliboard.dts:              interrupt-names = "USB", "AC";
>
> So how are other boards working?
>

Because on those that the IRQs are not defined it gets the data from
the platform resources. Again a mesh that we need to solve.

>
>>
>>>> +#define TPS65217_NUM_IRQ     3
>>>>
>>>>    static void tps65217_irq_lock(struct irq_data *data)
>>>>    {
>>>> @@ -86,29 +79,6 @@ static struct irq_chip tps65217_irq_chip = {
>>>>        .irq_disable            = tps65217_irq_disable,
>>>>    };
>>>>
>>>> -static struct mfd_cell tps65217s[] = {
>>>> -     {
>>>> -             .name = "tps65217-pmic",
>>>> -             .of_compatible = "ti,tps65217-pmic",
>>>> -     },
>>>> -     {
>>>> -             .name = "tps65217-bl",
>>>> -             .of_compatible = "ti,tps65217-bl",
>>>> -     },
>>>> -     {
>>>> -             .name = "tps65217-charger",
>>>> -             .num_resources = ARRAY_SIZE(charger_resources),
>>>> -             .resources = charger_resources,
>>>> -             .of_compatible = "ti,tps65217-charger",
>>>> -     },
>>>> -     {
>>>> -             .name = "tps65217-pwrbutton",
>>>> -             .num_resources = ARRAY_SIZE(pb_resources),
>>>> -             .resources = pb_resources,
>>>> -             .of_compatible = "ti,tps65217-pwrbutton",
>>>> -     },
>>>> -};
>>>> -
>>>>    static irqreturn_t tps65217_irq_thread(int irq, void *data)
>>>>    {
>>>>        struct tps65217 *tps = data;
>>>> @@ -359,23 +329,8 @@ static int tps65217_probe(struct i2c_client *client,
>>>>                return ret;
>>>>        }
>>>>
>>>> -     if (client->irq) {
>>>> +     if (client->irq)
>>>>                tps65217_irq_init(tps, client->irq);
>>>> -     } else {
>>>> -             int i;
>>>> -
>>>> -             /* Don't tell children about IRQ resources which won't fire */
>>>> -             for (i = 0; i < ARRAY_SIZE(tps65217s); i++)
>>>> -                     tps65217s[i].num_resources = 0;
>>>> -     }
>>>> -
>>>> -     ret = devm_mfd_add_devices(tps->dev, -1, tps65217s,
>>>> -                                ARRAY_SIZE(tps65217s), NULL, 0,
>>>> -                                tps->irq_domain);
>>>> -     if (ret < 0) {
>>>> -             dev_err(tps->dev, "mfd_add_devices failed: %d\n", ret);
>>>> -             return ret;
>>>> -     }
>>>
>>> And as I remember there was a request to use mfd_add_devices()
>>> and not of_platform_populate() for mfd sub-devices instantiation.
>>>
>>
>>  From what I know or you add the sub-devices via mfd_add_devices or
>> using the DT with of_platform_populate, and the first is probably the
>> preferred, but in this specific case this driver is DT-only so IMHO
>> makes more sense adding via the DT, there are already some bindings
>> i.e:
>>   arch/arm/boot/dts/tps65217.dtsi
>>   arch/arm/boot/dts/am335x-bone-common.dtsi
>>
>> Let me explain a bit more,  assume that you have another AM335x with
>> the TPS65217 PMIC but you don't want the charger because is not wired
>> in your board. Your board DT will include the tps65217.dtsi, in this
>> file you can see:
>>
>> &tps {
>>         ...
>>          charger {
>>                  compatible = "ti,tps65217-charger";
>>                  status = "disabled";
>>          };
>>          ...
>> };
>>
>> Seems that's fine but actually not works as expected, the TPS65217 MFD
>> registers all the sub-devices so the charger is enabled and running
>> even you have status = "disabled" in your DT. This looks incoherent to
>> me, hence I replaced the devm_mfd_add_devices() for the
>> of_platform_populate which takes care of the status propriety and only
>> calls the probe of the sub-devices that match and are enabled (status
>> = "okay").
>
> I can't find links on corresponding discussions, but mfd_add_devices() is
> preferred for MDF devices. Below is one commit i've found. Also you can compare number of
> drivers using mfd_add_devices() and of_platform_populate().
>

I don't think this a valid argument, I can also provide you a commit
that does exactly the contrary, replaces mfd_add_devices for
of_platform_populate (commit bb03ffb96c72)

>
> regarding your problem - I think, It might be reasonable from your side to propose change to
> mfd_add_device() which will take into account 'status = "disabled";' DT property.
>

Yes this is another solution to the problem that I thought, modify the
mfd-core to check if a device is available for use by calling
of_device_is_available and only add the sub-devices that are enabled.

Please, let's focus on how to solve the problem. Guess it's clear what
I'm trying to solve. To sum up, I think there are two ways to solve
the problem

 1) Get rid of the irq resources from tps65217 MFD driver and
configure all with the DT and of_platform_populate (these patches)
 2) Get rid of the DT portions as doesn't make sense have them in two places.
     +  2.1) Modify mfd-core to only instantiate the the sub-device is
device has status = "okay"

I don't mind to implement 2 + 2.1 if this is what people thinks is the
best. The *main* purpose of these patches was start this discussion to
find the solution.

I think at this point we need the maintainer's thoughts?

> ---
> commit 4531156db726d27e593d35800d43c74be4e393b9
> Author: Keerthy <j-keerthy@ti.com>
> Date:   Mon Sep 19 13:09:05 2016 +0530
>
>     mfd: tps65218: Use mfd_add_devices instead of of_platform_populate
>
>     mfd_add_devices enables parsing device tree nodes without compatibles
>     for regulators and gpio modules. Replace of_platform_populate with
>     mfd_add_devices. mfd_cell currently is populated with regulators,
>     gpio and powerbutton.
>
>     Signed-off-by: Keerthy <j-keerthy@ti.com>
>     Signed-off-by: Lee Jones <lee.jones@linaro.org>
>
>
>
>
>
> --
> regards,
> -grygorii

Re: [PATCH 4/4] mfd: tps65217: Instantiate sub-devices from device tree

[Javier Martinez Canillas]

On Thu, Jun 8, 2017 at 11:35 PM, Enric Balletbo Serra
<eballetbo@gmail.com> wrote:
> 2017-06-08 19:11 GMT+02:00 Grygorii Strashko <grygorii.strashko@ti.com>:

[snip]

>>>
>>> &tps {
>>>          charger {
>>>                  interrupts = <0>, <1>;
>>>                  interrupt-names = "USB", "AC";
>>>                  status = "okay";
>>>          };
>>>
>>>          pwrbutton {
>>>                  interrupts = <2>;
>>>                  status = "okay";
>>>          };
>>> };
>>
>> Sry, but this make no sense - those IRQ configuration is static,
>> so it should be defined in arch/arm/boot/dts/tps65217.dtsi at least.

Agreed.

>
> I was describing the state-of-art not what should be. If you mean that
> what doesn't make sense have these interrupts portions in the DT and
> the resources in the driver I'm completely agree. So we have two
> options:
>
>  1) Get rid of the irq resources from tps65217 MFD driver and
> configure all with the DT (these patches)
>  2) Get rid of the DT portions as doesn't make sense have them in two places.
>
> If we select 2) at least we have the problem that currently all
> sub-devices are instantiated and there is no way to disable a
> sub-device (the problem I'm trying to solve) hence I proposed 1)
>

There's also an option (3) AFAICT. To do (1) but instead of hardcoding
in the DTS[i], to do it in the charger driver since it seems the
sub-devices are only used for this particular MFD device, and so the
IRQ numbers are always going to be the same (it's already hardcoded in
the MFD driver anyways).

So you can just move TPS65217_IRQ_{AC,USB} to the driver. Not sure
what's general opinion of having drivers calling irq_create_mapping()
to get the virtual IRQ's though...

[snip]

>>
>> I can't find links on corresponding discussions, but mfd_add_devices() is
>> preferred for MDF devices. Below is one commit i've found. Also you can compare number of
>> drivers using mfd_add_devices() and of_platform_populate().

I'm not sure if we should use that as an argument, it may well be
that's just the drivers being half converted to DT (which is pretty
common TBH).

>>
>
> I don't think this a valid argument, I can also provide you a commit
> that does exactly the contrary, replaces mfd_add_devices for
> of_platform_populate (commit bb03ffb96c72)
>
>>

[snip]

>
>> ---
>> commit 4531156db726d27e593d35800d43c74be4e393b9
>> Author: Keerthy <j-keerthy@ti.com>
>> Date:   Mon Sep 19 13:09:05 2016 +0530
>>
>>     mfd: tps65218: Use mfd_add_devices instead of of_platform_populate
>>
>>     mfd_add_devices enables parsing device tree nodes without compatibles
>>     for regulators and gpio modules. Replace of_platform_populate with
>>     mfd_add_devices. mfd_cell currently is populated with regulators,
>>     gpio and powerbutton.
>>

For tps65218 couldn't instead of using mfd_add_devices() for all the
sub-devs, had used of_platform_populate() for the ones that have
device nodes and mfd_add_devices() only for the "tps65218-regulator"?

The commit talks about nodes without compatibles but's actually about
sub-devices without an associated device node. For me it makes sense
to use of_platform_populate() when the MFD has device nodes for their
sub-devices and mfd_add_devices() when DT knows nothing about the
sub-devices.

Best regards,
Javier

Re: [PATCH 4/4] mfd: tps65217: Instantiate sub-devices from device tree

[Grygorii Strashko]

On 06/08/2017 05:30 PM, Javier Martinez Canillas wrote:
> On Thu, Jun 8, 2017 at 11:35 PM, Enric Balletbo Serra
> <eballetbo@gmail.com> wrote:
>> 2017-06-08 19:11 GMT+02:00 Grygorii Strashko <grygorii.strashko@ti.com>:
> 
> [snip]
> 
>>>>
>>>> &tps {
>>>>           charger {
>>>>                   interrupts = <0>, <1>;
>>>>                   interrupt-names = "USB", "AC";
>>>>                   status = "okay";
>>>>           };
>>>>
>>>>           pwrbutton {
>>>>                   interrupts = <2>;
>>>>                   status = "okay";
>>>>           };
>>>> };
>>>
>>> Sry, but this make no sense - those IRQ configuration is static,
>>> so it should be defined in arch/arm/boot/dts/tps65217.dtsi at least.
> 
> Agreed.
> 
>>
>> I was describing the state-of-art not what should be. If you mean that
>> what doesn't make sense have these interrupts portions in the DT and
>> the resources in the driver I'm completely agree. So we have two
>> options:
>>
>>   1) Get rid of the irq resources from tps65217 MFD driver and
>> configure all with the DT (these patches)
>>   2) Get rid of the DT portions as doesn't make sense have them in two places.
>>
>> If we select 2) at least we have the problem that currently all
>> sub-devices are instantiated and there is no way to disable a
>> sub-device (the problem I'm trying to solve) hence I proposed 1)
>>
> 
> There's also an option (3) AFAICT. To do (1) but instead of hardcoding
> in the DTS[i], to do it in the charger driver since it seems the
> sub-devices are only used for this particular MFD device, and so the
> IRQ numbers are always going to be the same (it's already hardcoded in
> the MFD driver anyways).
> 
> So you can just move TPS65217_IRQ_{AC,USB} to the driver. Not sure
> what's general opinion of having drivers calling irq_create_mapping()
> to get the virtual IRQ's though...
> 
> [snip]
> 
>>>
>>> I can't find links on corresponding discussions, but mfd_add_devices() is
>>> preferred for MDF devices. Below is one commit i've found. Also you can compare number of
>>> drivers using mfd_add_devices() and of_platform_populate().
> 
> I'm not sure if we should use that as an argument, it may well be
> that's just the drivers being half converted to DT (which is pretty
> common TBH).
> 
>>>
>>
>> I don't think this a valid argument, I can also provide you a commit
>> that does exactly the contrary, replaces mfd_add_devices for
>> of_platform_populate (commit bb03ffb96c72)
>>
>>>
> 
> [snip]
> 
>>
>>> ---
>>> commit 4531156db726d27e593d35800d43c74be4e393b9
>>> Author: Keerthy <j-keerthy@ti.com>
>>> Date:   Mon Sep 19 13:09:05 2016 +0530
>>>
>>>      mfd: tps65218: Use mfd_add_devices instead of of_platform_populate
>>>
>>>      mfd_add_devices enables parsing device tree nodes without compatibles
>>>      for regulators and gpio modules. Replace of_platform_populate with
>>>      mfd_add_devices. mfd_cell currently is populated with regulators,
>>>      gpio and powerbutton.
>>>
> 
> For tps65218 couldn't instead of using mfd_add_devices() for all the
> sub-devs, had used of_platform_populate() for the ones that have
> device nodes and mfd_add_devices() only for the "tps65218-regulator"?
> 
> The commit talks about nodes without compatibles but's actually about
> sub-devices without an associated device node. For me it makes sense
> to use of_platform_populate() when the MFD has device nodes for their
> sub-devices and mfd_add_devices() when DT knows nothing about the
> sub-devices.

FYI. Below is link discussion I'm referring to between Mark Brown and Andrew F. Davis 
https://lkml.org/lkml/2015/10/22/823
the same - https://groups.google.com/forum/#!topic/linux.kernel/wQsdSpPMroQ




-- 
regards,
-grygorii

Re: [PATCH 4/4] mfd: tps65217: Instantiate sub-devices from device tree

[Javier Martinez Canillas]

Hello Grygorii,

[snip]

>>
>> For tps65218 couldn't instead of using mfd_add_devices() for all the
>> sub-devs, had used of_platform_populate() for the ones that have
>> device nodes and mfd_add_devices() only for the "tps65218-regulator"?
>>
>> The commit talks about nodes without compatibles but's actually about
>> sub-devices without an associated device node. For me it makes sense
>> to use of_platform_populate() when the MFD has device nodes for their
>> sub-devices and mfd_add_devices() when DT knows nothing about the
>> sub-devices.
>
> FYI. Below is link discussion I'm referring to between Mark Brown and Andrew F. Davis
> https://lkml.org/lkml/2015/10/22/823
> the same - https://groups.google.com/forum/#!topic/linux.kernel/wQsdSpPMroQ
>

Thanks a lot for the pointer. There's a subtle difference between the
argument you made and the one that Mark is making in this thread
though.

Because you said (sorry if I misunderstood) that mfd_add_devices()
should be used instead of of_device_populate() even when sub-devices
are described as DT nodes (as is the case in the commit you shared)
while Mark is saying that if the sub-devs IP blocks are part of the
MFD, then it shouldn't be exposed in the DT and be instantiated via
mfd_add_devices() and I absolutely agree with that.

So I was arguing for using of_device_populate() if the sub-devices are
described in the DT. If that makes sense or not to expose the
sub-devices in the DT for this particular driver is a different
discussion and I can't comment on that since I'm not familiar with the
HW.

Best regards,
Javier

Re: [PATCH 4/4] mfd: tps65217: Instantiate sub-devices from device tree

[Enric Balletbo Serra]

Hello Grygorii, Javier,

2017-06-09 2:00 GMT+02:00 Javier Martinez Canillas <javier@dowhile0.org>:
> Hello Grygorii,
>
> [snip]
>
>>>
>>> For tps65218 couldn't instead of using mfd_add_devices() for all the
>>> sub-devs, had used of_platform_populate() for the ones that have
>>> device nodes and mfd_add_devices() only for the "tps65218-regulator"?
>>>
>>> The commit talks about nodes without compatibles but's actually about
>>> sub-devices without an associated device node. For me it makes sense
>>> to use of_platform_populate() when the MFD has device nodes for their
>>> sub-devices and mfd_add_devices() when DT knows nothing about the
>>> sub-devices.
>>
>> FYI. Below is link discussion I'm referring to between Mark Brown and Andrew F. Davis
>> https://lkml.org/lkml/2015/10/22/823
>> the same - https://groups.google.com/forum/#!topic/linux.kernel/wQsdSpPMroQ
>>
>
> Thanks a lot for the pointer. There's a subtle difference between the
> argument you made and the one that Mark is making in this thread
> though.
>
> Because you said (sorry if I misunderstood) that mfd_add_devices()
> should be used instead of of_device_populate() even when sub-devices
> are described as DT nodes (as is the case in the commit you shared)
> while Mark is saying that if the sub-devs IP blocks are part of the
> MFD, then it shouldn't be exposed in the DT and be instantiated via
> mfd_add_devices() and I absolutely agree with that.
>
> So I was arguing for using of_device_populate() if the sub-devices are
> described in the DT. If that makes sense or not to expose the
> sub-devices in the DT for this particular driver is a different
> discussion and I can't comment on that since I'm not familiar with the
> HW.
>

I'm agree with Grygorii here that has no sense describe the static
interrupts resources in the DT here unless DT maintainers prefer have
them (dunno their preferences). OTOH, for the charger we need a way to
disable (or having a mfd propriety or having a DT subnode with the
status). I have a new idea that might be acceptable by Grygorii and
also solve my use case. Let me prepare a second patchset and lets
continue the discussion there?

Best regards,
 Enric

> Best regards,
> Javier

Re: [PATCH 1/4] dt-bindings: tps65217: Update binding documentation.

[Rob Herring]

On Wed, Jun 07, 2017 at 12:32:39PM +0200, Enric Balletbo i Serra wrote:
> This patch adds a new binding documentation for the TPS65217 MFD and
> updates the documentation for all the sub-devices in accordance to get
> each individual sub-driver functioning correctly.

Explain why breaking compatibility is okay.

> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
>  .../bindings/input/tps65218-pwrbutton.txt          |   2 +-
>  .../bindings/leds/backlight/tps65217-backlight.txt |  24 ++---
>  Documentation/devicetree/bindings/mfd/tps65217.txt | 100 +++++++++++++++++++++
>  .../devicetree/bindings/regulator/tps65217.txt     |   8 +-
>  4 files changed, 119 insertions(+), 15 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/mfd/tps65217.txt
> 
> diff --git a/Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt b/Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt
> index 8682ab6..603a3f0 100644
> --- a/Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt
> +++ b/Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt
> @@ -1,7 +1,7 @@
>  Texas Instruments TPS65217 and TPS65218 power button
>  
>  This module is part of the TPS65217/TPS65218. For more details about the whole
> -TPS65217 chip see Documentation/devicetree/bindings/regulator/tps65217.txt.
> +TPS65217 chip see Documentation/devicetree/bindings/mfd/tps65217.txt.
>  
>  This driver provides a simple power button event via an Interrupt.
>  
> diff --git a/Documentation/devicetree/bindings/leds/backlight/tps65217-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/tps65217-backlight.txt
> index 5fb9279..a1bc465 100644
> --- a/Documentation/devicetree/bindings/leds/backlight/tps65217-backlight.txt
> +++ b/Documentation/devicetree/bindings/leds/backlight/tps65217-backlight.txt
> @@ -1,11 +1,13 @@
> -TPS65217 family of regulators
> +Texas Instruments TPS65217 backlight regulator
> +
> +This module is part of the TPS65217. For more details about the whole
> +TPS65217 chip see Documentation/devicetree/bindings/mfd/tps65217.txt.
>  
>  The TPS65217 chip contains a boost converter and current sinks which can be
>  used to drive LEDs for use as backlights.
>  
>  Required properties:
> -- compatible: "ti,tps65217"
> -- reg: I2C slave address
> +- compatible: "ti,tps65217-bl"
>  - backlight: node for specifying WLED1 and WLED2 lines in TPS65217
>  - isel: selection bit, valid values: 1 for ISEL1 (low-level) and 2 for ISEL2 (high-level)
>  - fdim: PWM dimming frequency, valid values: 100, 200, 500, 1000
> @@ -15,13 +17,13 @@ Each regulator is defined using the standard binding for regulators.
>  
>  Example:
>  
> -	tps: tps@24 {
> -		reg = <0x24>;
> -		compatible = "ti,tps65217";
> -		backlight {
> -			isel = <1>;  /* 1 - ISET1, 2 ISET2 */
> -			fdim = <100>; /* TPS65217_BL_FDIM_100HZ */
> -			default-brightness = <50>;
> -		};
> +&tps {
> +	backlight {
> +		compatible = "ti,tps65217-bl";
> +		status = "okay";
> +		isel = <1>;  /* 1 - ISET1, 2 ISET2 */
> +		fdim = <100>; /* TPS65217_BL_FDIM_100HZ */
> +		default-brightness = <50>;
>  	};
> +};
>  
> diff --git a/Documentation/devicetree/bindings/mfd/tps65217.txt b/Documentation/devicetree/bindings/mfd/tps65217.txt
> new file mode 100644
> index 0000000..40c84ba
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/tps65217.txt
> @@ -0,0 +1,100 @@
> +Texas Instruments TPS65217 Single-Chip PMIC for Battery-Powered Systems
> +
> +Required properties:
> +- compatible: "ti,tps65217"
> +- reg: I2C slave address.
> +- interrupt-controller: Marks the device node as an interrupt controller.
> +- #interrupt-cells: The number of cells to describe an IRQ, this should be 1.
> +- backlight: Child node that specify the backlight regulator sub-device. See:
> +	Documentation/devicetree/bindings/leds/backlight/tps65217-backlight.txt
> +- charger: Child node that specify the charger sub-device. See:
> +	Documentation/devicetree/bindings/power/supply/tps65217_charger.txt
> +- pwrbutton: Child node that specify the power button sub-device. See:
> +	Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt
> +- regulators: List of child nodes that specify the regulator initialization
> +	data. See:
> +	Documentation/devicetree/bindings/regulator/tps65217.txt.
> +
> +Optional properties:
> +- ti,pmic-shutdown-controller: Telling the PMIC to shutdown on PWR_EN toggle.
> +
> +Example:
> +
> +	tps: tps@24 {
> +		compatible = "ti,tps65217";
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +
> +		ti,pmic-shutdown-controller;
> +
> +		backlight {
> +			compatible = "ti,tps65217-bl";
> +			status = "disabled";
> +		};
> +
> +		charger {
> +			compatible = "ti,tps65217-charger";
> +			status = "disabled";
> +		};
> +
> +		pwrbutton {
> +			compatible = "ti,tps65217-pwrbutton";
> +			status = "disabled";
> +		};
> +
> +		regulators {
> +			compatible = "ti,tps65217-pmic";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			dcdc1_reg: dcdc1 {
> +				regulator-min-microvolt = <900000>;
> +				regulator-max-microvolt = <1800000>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +			};
> +
> +			dcdc2_reg: dcdc2 {
> +				regulator-min-microvolt = <900000>;
> +				regulator-max-microvolt = <3300000>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +			};
> +
> +			dcdc3_reg: dcc3 {
> +				regulator-min-microvolt = <900000>;
> +				regulator-max-microvolt = <1500000>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +			};
> +
> +			ldo1_reg: ldo1 {
> +				regulator-min-microvolt = <1000000>;
> +				regulator-max-microvolt = <3300000>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +			};
> +
> +			ldo2_reg: ldo2 {
> +				regulator-min-microvolt = <900000>;
> +				regulator-max-microvolt = <3300000>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +			};
> +
> +			ldo3_reg: ldo3 {
> +				regulator-min-microvolt = <1800000>;
> +				regulator-max-microvolt = <3300000>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +			};
> +
> +			ldo4_reg: ldo4 {
> +				regulator-min-microvolt = <1800000>;
> +				regulator-max-microvolt = <3300000>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +			};
> +		};
> +	};
> +
> diff --git a/Documentation/devicetree/bindings/regulator/tps65217.txt b/Documentation/devicetree/bindings/regulator/tps65217.txt
> index 4f05d20..2d9b519 100644
> --- a/Documentation/devicetree/bindings/regulator/tps65217.txt
> +++ b/Documentation/devicetree/bindings/regulator/tps65217.txt
> @@ -1,8 +1,10 @@
> -TPS65217 family of regulators
> +Texas Instruments TPS65217 family of regulators
> +
> +This module is part of the TPS65217. For more details about the whole
> +TPS65217 chip see Documentation/devicetree/bindings/mfd/tps65217.txt.
>  
>  Required properties:
> -- compatible: "ti,tps65217"
> -- reg: I2C slave address
> +- compatible: "ti,tps65217-pmic"
>  - regulators: list of regulators provided by this controller, must be named
>    after their hardware counterparts: dcdc[1-3] and ldo[1-4]
>  - regulators: This is the list of child nodes that specify the regulator

regulators twice? Fix that while you're here.

> -- 
> 2.9.3
> 

Re: [PATCH 1/4] dt-bindings: tps65217: Update binding documentation.

[Enric Balletbo Serra]

Hello Rob,

2017-06-09 16:03 GMT+02:00 Rob Herring <robh@kernel.org>:
> On Wed, Jun 07, 2017 at 12:32:39PM +0200, Enric Balletbo i Serra wrote:
>> This patch adds a new binding documentation for the TPS65217 MFD and
>> updates the documentation for all the sub-devices in accordance to get
>> each individual sub-driver functioning correctly.
>
> Explain why breaking compatibility is okay.
>

We had some discussion in patch 4 that make me rethink a bit all this,
please let me send a new version and continue the discussion there
(now I'm not sure if I'll break the compatibility or not)

But let me ask a question. The TPS65217 MFD has different sub-nodes
(charger, backlight, pwrbutton, regulators) in DT, I suspect the
answer is no, but is it ok that some of them were not described in the
DT because there is nothing to configure?

i.e: Have this because the resources of charger and pwrbutton are
static so can be hard-coded in the driver

&tps {
        compatible = "ti,tps65217";
        interrupt-controller;
        #interrupt-cells = <1>;

        regulators {
                #address-cells = <1>;
                #size-cells = <0>;

                dcdc1_reg: regulator@0 {
                        reg = <0>;
                ...
       };
};

instead of :

&tps {
        compatible = "ti,tps65217";
        interrupt-controller;
        #interrupt-cells = <1>;

        charger {
                compatible = "ti,tps65217-charger";
                interrupts = <0>, <1>;
                interrupt-names = "USB", "AC";
        };

        pwrbutton {
                compatible = "ti,tps65217-pwrbutton";
                interrupts = <2>;
        };

        regulators {
                #address-cells = <1>;
                #size-cells = <0>;

                dcdc1_reg: regulator@0 {
                        reg = <0>;
                ...
       };
};

Best regards,
 Enric

>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> ---
>>  .../bindings/input/tps65218-pwrbutton.txt          |   2 +-
>>  .../bindings/leds/backlight/tps65217-backlight.txt |  24 ++---
>>  Documentation/devicetree/bindings/mfd/tps65217.txt | 100 +++++++++++++++++++++
>>  .../devicetree/bindings/regulator/tps65217.txt     |   8 +-
>>  4 files changed, 119 insertions(+), 15 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/mfd/tps65217.txt
>>
>> diff --git a/Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt b/Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt
>> index 8682ab6..603a3f0 100644
>> --- a/Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt
>> +++ b/Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt
>> @@ -1,7 +1,7 @@
>>  Texas Instruments TPS65217 and TPS65218 power button
>>
>>  This module is part of the TPS65217/TPS65218. For more details about the whole
>> -TPS65217 chip see Documentation/devicetree/bindings/regulator/tps65217.txt.
>> +TPS65217 chip see Documentation/devicetree/bindings/mfd/tps65217.txt.
>>
>>  This driver provides a simple power button event via an Interrupt.
>>
>> diff --git a/Documentation/devicetree/bindings/leds/backlight/tps65217-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/tps65217-backlight.txt
>> index 5fb9279..a1bc465 100644
>> --- a/Documentation/devicetree/bindings/leds/backlight/tps65217-backlight.txt
>> +++ b/Documentation/devicetree/bindings/leds/backlight/tps65217-backlight.txt
>> @@ -1,11 +1,13 @@
>> -TPS65217 family of regulators
>> +Texas Instruments TPS65217 backlight regulator
>> +
>> +This module is part of the TPS65217. For more details about the whole
>> +TPS65217 chip see Documentation/devicetree/bindings/mfd/tps65217.txt.
>>
>>  The TPS65217 chip contains a boost converter and current sinks which can be
>>  used to drive LEDs for use as backlights.
>>
>>  Required properties:
>> -- compatible: "ti,tps65217"
>> -- reg: I2C slave address
>> +- compatible: "ti,tps65217-bl"
>>  - backlight: node for specifying WLED1 and WLED2 lines in TPS65217
>>  - isel: selection bit, valid values: 1 for ISEL1 (low-level) and 2 for ISEL2 (high-level)
>>  - fdim: PWM dimming frequency, valid values: 100, 200, 500, 1000
>> @@ -15,13 +17,13 @@ Each regulator is defined using the standard binding for regulators.
>>
>>  Example:
>>
>> -     tps: tps@24 {
>> -             reg = <0x24>;
>> -             compatible = "ti,tps65217";
>> -             backlight {
>> -                     isel = <1>;  /* 1 - ISET1, 2 ISET2 */
>> -                     fdim = <100>; /* TPS65217_BL_FDIM_100HZ */
>> -                     default-brightness = <50>;
>> -             };
>> +&tps {
>> +     backlight {
>> +             compatible = "ti,tps65217-bl";
>> +             status = "okay";
>> +             isel = <1>;  /* 1 - ISET1, 2 ISET2 */
>> +             fdim = <100>; /* TPS65217_BL_FDIM_100HZ */
>> +             default-brightness = <50>;
>>       };
>> +};
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/tps65217.txt b/Documentation/devicetree/bindings/mfd/tps65217.txt
>> new file mode 100644
>> index 0000000..40c84ba
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/tps65217.txt
>> @@ -0,0 +1,100 @@
>> +Texas Instruments TPS65217 Single-Chip PMIC for Battery-Powered Systems
>> +
>> +Required properties:
>> +- compatible: "ti,tps65217"
>> +- reg: I2C slave address.
>> +- interrupt-controller: Marks the device node as an interrupt controller.
>> +- #interrupt-cells: The number of cells to describe an IRQ, this should be 1.
>> +- backlight: Child node that specify the backlight regulator sub-device. See:
>> +     Documentation/devicetree/bindings/leds/backlight/tps65217-backlight.txt
>> +- charger: Child node that specify the charger sub-device. See:
>> +     Documentation/devicetree/bindings/power/supply/tps65217_charger.txt
>> +- pwrbutton: Child node that specify the power button sub-device. See:
>> +     Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt
>> +- regulators: List of child nodes that specify the regulator initialization
>> +     data. See:
>> +     Documentation/devicetree/bindings/regulator/tps65217.txt.
>> +
>> +Optional properties:
>> +- ti,pmic-shutdown-controller: Telling the PMIC to shutdown on PWR_EN toggle.
>> +
>> +Example:
>> +
>> +     tps: tps@24 {
>> +             compatible = "ti,tps65217";
>> +             interrupt-controller;
>> +             #interrupt-cells = <1>;
>> +
>> +             ti,pmic-shutdown-controller;
>> +
>> +             backlight {
>> +                     compatible = "ti,tps65217-bl";
>> +                     status = "disabled";
>> +             };
>> +
>> +             charger {
>> +                     compatible = "ti,tps65217-charger";
>> +                     status = "disabled";
>> +             };
>> +
>> +             pwrbutton {
>> +                     compatible = "ti,tps65217-pwrbutton";
>> +                     status = "disabled";
>> +             };
>> +
>> +             regulators {
>> +                     compatible = "ti,tps65217-pmic";
>> +                     #address-cells = <1>;
>> +                     #size-cells = <0>;
>> +
>> +                     dcdc1_reg: dcdc1 {
>> +                             regulator-min-microvolt = <900000>;
>> +                             regulator-max-microvolt = <1800000>;
>> +                             regulator-boot-on;
>> +                             regulator-always-on;
>> +                     };
>> +
>> +                     dcdc2_reg: dcdc2 {
>> +                             regulator-min-microvolt = <900000>;
>> +                             regulator-max-microvolt = <3300000>;
>> +                             regulator-boot-on;
>> +                             regulator-always-on;
>> +                     };
>> +
>> +                     dcdc3_reg: dcc3 {
>> +                             regulator-min-microvolt = <900000>;
>> +                             regulator-max-microvolt = <1500000>;
>> +                             regulator-boot-on;
>> +                             regulator-always-on;
>> +                     };
>> +
>> +                     ldo1_reg: ldo1 {
>> +                             regulator-min-microvolt = <1000000>;
>> +                             regulator-max-microvolt = <3300000>;
>> +                             regulator-boot-on;
>> +                             regulator-always-on;
>> +                     };
>> +
>> +                     ldo2_reg: ldo2 {
>> +                             regulator-min-microvolt = <900000>;
>> +                             regulator-max-microvolt = <3300000>;
>> +                             regulator-boot-on;
>> +                             regulator-always-on;
>> +                     };
>> +
>> +                     ldo3_reg: ldo3 {
>> +                             regulator-min-microvolt = <1800000>;
>> +                             regulator-max-microvolt = <3300000>;
>> +                             regulator-boot-on;
>> +                             regulator-always-on;
>> +                     };
>> +
>> +                     ldo4_reg: ldo4 {
>> +                             regulator-min-microvolt = <1800000>;
>> +                             regulator-max-microvolt = <3300000>;
>> +                             regulator-boot-on;
>> +                             regulator-always-on;
>> +                     };
>> +             };
>> +     };
>> +
>> diff --git a/Documentation/devicetree/bindings/regulator/tps65217.txt b/Documentation/devicetree/bindings/regulator/tps65217.txt
>> index 4f05d20..2d9b519 100644
>> --- a/Documentation/devicetree/bindings/regulator/tps65217.txt
>> +++ b/Documentation/devicetree/bindings/regulator/tps65217.txt
>> @@ -1,8 +1,10 @@
>> -TPS65217 family of regulators
>> +Texas Instruments TPS65217 family of regulators
>> +
>> +This module is part of the TPS65217. For more details about the whole
>> +TPS65217 chip see Documentation/devicetree/bindings/mfd/tps65217.txt.
>>
>>  Required properties:
>> -- compatible: "ti,tps65217"
>> -- reg: I2C slave address
>> +- compatible: "ti,tps65217-pmic"
>>  - regulators: list of regulators provided by this controller, must be named
>>    after their hardware counterparts: dcdc[1-3] and ldo[1-4]
>>  - regulators: This is the list of child nodes that specify the regulator
>
> regulators twice? Fix that while you're here.
>

Sure, I can add this change as well.

>> --
>> 2.9.3
>>

Re: [PATCH 1/4] dt-bindings: tps65217: Update binding documentation.

[Rob Herring]

On Fri, Jun 9, 2017 at 5:30 PM, Enric Balletbo Serra
<eballetbo@gmail.com> wrote:
> Hello Rob,
>
> 2017-06-09 16:03 GMT+02:00 Rob Herring <robh@kernel.org>:
>> On Wed, Jun 07, 2017 at 12:32:39PM +0200, Enric Balletbo i Serra wrote:
>>> This patch adds a new binding documentation for the TPS65217 MFD and
>>> updates the documentation for all the sub-devices in accordance to get
>>> each individual sub-driver functioning correctly.
>>
>> Explain why breaking compatibility is okay.
>>
>
> We had some discussion in patch 4 that make me rethink a bit all this,
> please let me send a new version and continue the discussion there
> (now I'm not sure if I'll break the compatibility or not)
>
> But let me ask a question. The TPS65217 MFD has different sub-nodes
> (charger, backlight, pwrbutton, regulators) in DT, I suspect the
> answer is no, but is it ok that some of them were not described in the
> DT because there is nothing to configure?

But you are configuring the interrupts. I'm all for not creating nodes
just for the purpose instantiating a driver (DT is not the only way to
create platform devices) when the parent node is sufficient. We see
both models, but we don't really want a mixture of both ways so I'd
stick with the latter here.

> i.e: Have this because the resources of charger and pwrbutton are
> static so can be hard-coded in the driver
>
> &tps {
>         compatible = "ti,tps65217";
>         interrupt-controller;
>         #interrupt-cells = <1>;
>
>         regulators {
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>
>                 dcdc1_reg: regulator@0 {
>                         reg = <0>;
>                 ...
>        };
> };
>
> instead of :
>
> &tps {
>         compatible = "ti,tps65217";
>         interrupt-controller;
>         #interrupt-cells = <1>;
>
>         charger {
>                 compatible = "ti,tps65217-charger";
>                 interrupts = <0>, <1>;
>                 interrupt-names = "USB", "AC";
>         };
>
>         pwrbutton {
>                 compatible = "ti,tps65217-pwrbutton";
>                 interrupts = <2>;
>         };
>
>         regulators {
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>
>                 dcdc1_reg: regulator@0 {
>                         reg = <0>;
>                 ...
>        };
> };