[PATCH 1/4] ARM: dts: tps65217: Add charger interrupts to the common tps65217.dtsi file

[PATCH 1/4] ARM: dts: tps65217: Add charger interrupts to the common tps65217.dtsi file

[Enric Balletbo i Serra]

The interrupt specifiers for USB and AC charger input are static data that
comes from the datasheet, there is no reason to need to define these values
on every board so seem reasonable put this information into the common
tps65217 file.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
---
 arch/arm/boot/dts/am335x-bone-common.dtsi | 2 --
 arch/arm/boot/dts/am335x-chiliboard.dts   | 2 --
 arch/arm/boot/dts/tps65217.dtsi           | 2 ++
 3 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/am335x-bone-common.dtsi b/arch/arm/boot/dts/am335x-bone-common.dtsi
index bf6b26a..bfa4258 100644
--- a/arch/arm/boot/dts/am335x-bone-common.dtsi
+++ b/arch/arm/boot/dts/am335x-bone-common.dtsi
@@ -319,8 +319,6 @@
 	ti,pmic-shutdown-controller;
 
 	charger {
-		interrupts = <0>, <1>;
-		interrupt-names = "USB", "AC";
 		status = "okay";
 	};
 
diff --git a/arch/arm/boot/dts/am335x-chiliboard.dts b/arch/arm/boot/dts/am335x-chiliboard.dts
index d876979..5c25f3a 100644
--- a/arch/arm/boot/dts/am335x-chiliboard.dts
+++ b/arch/arm/boot/dts/am335x-chiliboard.dts
@@ -191,8 +191,6 @@
 	interrupts = <7>; /* NNMI */
 
 	charger {
-		interrupts = <0>, <1>;
-		interrupt-names = "USB", "AC";
 		status = "okay";
 	};
 
diff --git a/arch/arm/boot/dts/tps65217.dtsi b/arch/arm/boot/dts/tps65217.dtsi
index 02de56b..1973159 100644
--- a/arch/arm/boot/dts/tps65217.dtsi
+++ b/arch/arm/boot/dts/tps65217.dtsi
@@ -18,6 +18,8 @@
 
 	charger {
 		compatible = "ti,tps65217-charger";
+		interrupts = <0>, <1>;
+		interrupt-names = "USB", "AC";
 		status = "disabled";
 	};
 
-- 
2.9.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/4] ARM: dts: tps65217: Add power button interrupt to the common tps65217.dtsi file

[Enric Balletbo i Serra]

The interrupt for power button is static data that comes from the
datasheet, there is no reason to need to define this value on every
board so seams reasonable put this information into the common tps65217
file.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
 arch/arm/boot/dts/am335x-bone-common.dtsi | 1 -
 arch/arm/boot/dts/am335x-chiliboard.dts   | 1 -
 arch/arm/boot/dts/tps65217.dtsi           | 1 +
 3 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/am335x-bone-common.dtsi b/arch/arm/boot/dts/am335x-bone-common.dtsi
index bfa4258..89daaeb 100644
--- a/arch/arm/boot/dts/am335x-bone-common.dtsi
+++ b/arch/arm/boot/dts/am335x-bone-common.dtsi
@@ -323,7 +323,6 @@
 	};
 
 	pwrbutton {
-		interrupts = <2>;
 		status = "okay";
 	};
 
diff --git a/arch/arm/boot/dts/am335x-chiliboard.dts b/arch/arm/boot/dts/am335x-chiliboard.dts
index 5c25f3a..59431b2 100644
--- a/arch/arm/boot/dts/am335x-chiliboard.dts
+++ b/arch/arm/boot/dts/am335x-chiliboard.dts
@@ -195,7 +195,6 @@
 	};
 
 	pwrbutton {
-		interrupts = <2>;
 		status = "okay";
 	};
 };
diff --git a/arch/arm/boot/dts/tps65217.dtsi b/arch/arm/boot/dts/tps65217.dtsi
index 1973159..399baaa 100644
--- a/arch/arm/boot/dts/tps65217.dtsi
+++ b/arch/arm/boot/dts/tps65217.dtsi
@@ -25,6 +25,7 @@
 
 	pwrbutton {
 		compatible = "ti,tps65217-pwrbutton";
+		interrupts = <2>;
 		status = "disabled";
 	};
 
-- 
2.9.3

Re: [PATCH 2/4] ARM: dts: tps65217: Add power button interrupt to the common tps65217.dtsi file

[Tony Lindgren]

* Enric Balletbo i Serra <enric.balletbo@collabora.com> [170612 14:28]:
> The interrupt for power button is static data that comes from the
> datasheet, there is no reason to need to define this value on every
> board so seams reasonable put this information into the common tps65217
> file.

I think there's a problem with these patches where we no longer know
if the interrupts are wired up on a board. For example, if the USB PHY
VBUS interrupt is not connected, the phy driver needs poll the
cable state.

And on some PMICs some pins can be muxed to GPIO mode, don't
remember if that can be done with tps65217.

Regards,

Tony

Re: [PATCH 2/4] ARM: dts: tps65217: Add power button interrupt to the common tps65217.dtsi file

[Enric Balletbo Serra]

Hi Tony,

2017-06-13 9:35 GMT+02:00 Tony Lindgren <tony@atomide.com>:
> * Enric Balletbo i Serra <enric.balletbo@collabora.com> [170612 14:28]:
>> The interrupt for power button is static data that comes from the
>> datasheet, there is no reason to need to define this value on every
>> board so seams reasonable put this information into the common tps65217
>> file.
>
> I think there's a problem with these patches where we no longer know
> if the interrupts are wired up on a board. For example, if the USB PHY
> VBUS interrupt is not connected, the phy driver needs poll the
> cable state.
>

Hmm, the tps65217 doesn't have a USB PHY like others tps, so not sure
how could affect this patch. Did you see this issue in any board that
uses tps65217 or any am335x based board which commonly uses this pmic?

> And on some PMICs some pins can be muxed to GPIO mode, don't
> remember if that can be done with tps65217.
>

i don't think this is the case of the boards that use tps65217 as the
pins can't be muxed to GPIO mode.

> Regards,
>
> Tony

Regards,
 Enric

Re: [PATCH 2/4] ARM: dts: tps65217: Add power button interrupt to the common tps65217.dtsi file

[Tony Lindgren]

* Enric Balletbo Serra <eballetbo-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [170613 02:12]:
> Hi Tony,
> 
> 2017-06-13 9:35 GMT+02:00 Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>:
> > * Enric Balletbo i Serra <enric.balletbo-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org> [170612 14:28]:
> >> The interrupt for power button is static data that comes from the
> >> datasheet, there is no reason to need to define this value on every
> >> board so seams reasonable put this information into the common tps65217
> >> file.
> >
> > I think there's a problem with these patches where we no longer know
> > if the interrupts are wired up on a board. For example, if the USB PHY
> > VBUS interrupt is not connected, the phy driver needs poll the
> > cable state.
> >
> 
> Hmm, the tps65217 doesn't have a USB PHY like others tps, so not sure
> how could affect this patch. Did you see this issue in any board that
> uses tps65217 or any am335x based board which commonly uses this pmic?

Oh sorry I misread your patch. I thought you're removing
the interrupts from dts, but you're just moving the dts entries
to the common dtsi file :) So seems just fine with me.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] ARM: dts: tps65217: Add power button interrupt to the common tps65217.dtsi file

[Tony Lindgren]

* Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [170613 22:00]:
> * Enric Balletbo Serra <eballetbo-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [170613 02:12]:
> > Hi Tony,
> > 
> > 2017-06-13 9:35 GMT+02:00 Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>:
> > > * Enric Balletbo i Serra <enric.balletbo-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org> [170612 14:28]:
> > >> The interrupt for power button is static data that comes from the
> > >> datasheet, there is no reason to need to define this value on every
> > >> board so seams reasonable put this information into the common tps65217
> > >> file.
> > >
> > > I think there's a problem with these patches where we no longer know
> > > if the interrupts are wired up on a board. For example, if the USB PHY
> > > VBUS interrupt is not connected, the phy driver needs poll the
> > > cable state.
> > >
> > 
> > Hmm, the tps65217 doesn't have a USB PHY like others tps, so not sure
> > how could affect this patch. Did you see this issue in any board that
> > uses tps65217 or any am335x based board which commonly uses this pmic?
> 
> Oh sorry I misread your patch. I thought you're removing
> the interrupts from dts, but you're just moving the dts entries
> to the common dtsi file :) So seems just fine with me.

Applying patches 1 and 2 of this series into omap-for-v4.14/dt
thanks.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/4] mfd: tps65217: remove duplicated interrupt resources.

[Enric Balletbo i Serra]

I don't think it makes sense to have the interrupt resources for charger
and power button in two different places, the driver and the DT binding.
That's confusing so remove the ones from the mfd driver in favour of
having the interrupt resources only described in the DT. Having the
resources in DT may help if there is or will be a similar pmic with
different resource allocation.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
 drivers/mfd/tps65217.c | 22 +---------------------
 1 file changed, 1 insertion(+), 21 deletions(-)

diff --git a/drivers/mfd/tps65217.c b/drivers/mfd/tps65217.c
index f769c7d..fd83bae 100644
--- a/drivers/mfd/tps65217.c
+++ b/drivers/mfd/tps65217.c
@@ -33,15 +33,6 @@
 #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"),
-};
-
 static void tps65217_irq_lock(struct irq_data *data)
 {
 	struct tps65217 *tps = irq_data_get_irq_chip_data(data);
@@ -97,14 +88,10 @@ static struct mfd_cell tps65217s[] = {
 	},
 	{
 		.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",
 	},
 };
@@ -359,15 +346,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,
-- 
2.9.3

Re: [PATCH 3/4] mfd: tps65217: remove duplicated interrupt resources.

[Grygorii Strashko]

On 06/12/2017 04:24 PM, Enric Balletbo i Serra wrote:
> I don't think it makes sense to have the interrupt resources for charger
> and power button in two different places, the driver and the DT binding.
> That's confusing so remove the ones from the mfd driver in favour of
> having the interrupt resources only described in the DT. Having the
> resources in DT may help if there is or will be a similar pmic with
> different resource allocation.

Wouldn't this break DT compatibility? Old DTs do not contain IRQ resources
and so they work only because of IRQ definitions in code.

> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
> ---
>   drivers/mfd/tps65217.c | 22 +---------------------
>   1 file changed, 1 insertion(+), 21 deletions(-)
> 
> diff --git a/drivers/mfd/tps65217.c b/drivers/mfd/tps65217.c
> index f769c7d..fd83bae 100644
> --- a/drivers/mfd/tps65217.c
> +++ b/drivers/mfd/tps65217.c
> @@ -33,15 +33,6 @@
>   #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"),
> -};
> -
>   static void tps65217_irq_lock(struct irq_data *data)
>   {
>   	struct tps65217 *tps = irq_data_get_irq_chip_data(data);
> @@ -97,14 +88,10 @@ static struct mfd_cell tps65217s[] = {
>   	},
>   	{
>   		.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",
>   	},
>   };
> @@ -359,15 +346,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,
> 

-- 
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] mfd: tps65217: remove duplicated interrupt resources.

[Enric Balletbo Serra]

Hi Grygorii,

2017-06-13 0:41 GMT+02:00 Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>:
>
>
> On 06/12/2017 04:24 PM, Enric Balletbo i Serra wrote:
>> I don't think it makes sense to have the interrupt resources for charger
>> and power button in two different places, the driver and the DT binding.
>> That's confusing so remove the ones from the mfd driver in favour of
>> having the interrupt resources only described in the DT. Having the
>> resources in DT may help if there is or will be a similar pmic with
>> different resource allocation.
>
> Wouldn't this break DT compatibility? Old DTs do not contain IRQ resources
> and so they work only because of IRQ definitions in code.
>

I don't think so, the DT binding [1] and [2] says that the interrupts
proprieties are required, so the bindings that that doesn't have these
proprieties are wrong. Also I suspect that the binding that doesn't
define the interrupts expect the hw block disabled as the status =
"disabled" was defined, so I don't think was his intention have i.e
the charger active.

[1] https://www.kernel.org/doc/Documentation/devicetree/bindings/power/supply/tps65217_charger.txt
[2] https://www.kernel.org/doc/Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt

>>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
>> ---
>>   drivers/mfd/tps65217.c | 22 +---------------------
>>   1 file changed, 1 insertion(+), 21 deletions(-)
>>
>> diff --git a/drivers/mfd/tps65217.c b/drivers/mfd/tps65217.c
>> index f769c7d..fd83bae 100644
>> --- a/drivers/mfd/tps65217.c
>> +++ b/drivers/mfd/tps65217.c
>> @@ -33,15 +33,6 @@
>>   #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"),
>> -};
>> -
>>   static void tps65217_irq_lock(struct irq_data *data)
>>   {
>>       struct tps65217 *tps = irq_data_get_irq_chip_data(data);
>> @@ -97,14 +88,10 @@ static struct mfd_cell tps65217s[] = {
>>       },
>>       {
>>               .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",
>>       },
>>   };
>> @@ -359,15 +346,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,
>>
>
> --
> regards,
> -grygorii
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Best regards,
 Enric
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] mfd: tps65217: remove duplicated interrupt resources.

[Grygorii Strashko]

On 06/13/2017 03:18 AM, Enric Balletbo Serra wrote:
> Hi Grygorii,
> 
> 2017-06-13 0:41 GMT+02:00 Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>:
>>
>>
>> On 06/12/2017 04:24 PM, Enric Balletbo i Serra wrote:
>>> I don't think it makes sense to have the interrupt resources for charger
>>> and power button in two different places, the driver and the DT binding.
>>> That's confusing so remove the ones from the mfd driver in favour of
>>> having the interrupt resources only described in the DT. Having the
>>> resources in DT may help if there is or will be a similar pmic with
>>> different resource allocation.
>>
>> Wouldn't this break DT compatibility? Old DTs do not contain IRQ resources
>> and so they work only because of IRQ definitions in code.
>>
> 
> I don't think so, the DT binding [1] and [2] says that the interrupts
> proprieties are required, so the bindings that that doesn't have these
> proprieties are wrong. Also I suspect that the binding that doesn't
> define the interrupts expect the hw block disabled as the status =
> "disabled" was defined, so I don't think was his intention have i.e
> the charger active.
> 
> [1] https://www.kernel.org/doc/Documentation/devicetree/bindings/power/supply/tps65217_charger.txt
> [2] https://www.kernel.org/doc/Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt

Ok. fair enough.

Anyway, personally I would prefer to remove irqs from DT, but as both 
points are valid - its up to maintainers to decide.

-- 
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4] power: supply: tps65217: able to disable the charger block.

[Enric Balletbo i Serra]

The TPS65217 charger is enabled by default, but by default the DT binding
sets the charger as disabled, so currently all the devices that include
the tps65217 binding have the charger enabled. This patch adds a check in
the probe function of the tps65217 charger and disables it if the device
status is disabled.

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

diff --git a/drivers/power/supply/tps65217_charger.c b/drivers/power/supply/tps65217_charger.c
index 1f52340..55308f4 100644
--- a/drivers/power/supply/tps65217_charger.c
+++ b/drivers/power/supply/tps65217_charger.c
@@ -203,6 +203,23 @@ static int tps65217_charger_probe(struct platform_device *pdev)
 	int ret;
 	int i;
 
+	/*
+	 * By default the charger is enabled but if device is disabled stop
+	 * the charger accordingly to the configuration.
+	 */
+	if (!of_device_is_available(pdev->dev.of_node)) {
+		dev_dbg(&pdev->dev, "charger disabled\n");
+		ret = tps65217_clear_bits(tps, TPS65217_REG_CHGCONFIG1,
+					  TPS65217_CHGCONFIG1_CHG_EN,
+					  TPS65217_PROTECT_NONE);
+		if (ret) {
+			dev_err(&pdev->dev, "Error writing in reg 0x%x: %d\n",
+				TPS65217_REG_CHGCONFIG1, ret);
+			return ret;
+		}
+		return -ENODEV;
+	}
+
 	charger = devm_kzalloc(&pdev->dev, sizeof(*charger), GFP_KERNEL);
 	if (!charger)
 		return -ENOMEM;
-- 
2.9.3