Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1664246 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/4] ARM: dts: tps65217: Add charger interrupts to the common tps65217.dtsi file Enric Balletbo i Serra <enric.balletbo@collabora.com> - 2017-06-12 23:30 +0200
    [PATCH 3/4] mfd: tps65217: remove duplicated interrupt resources. Enric Balletbo i Serra <enric.balletbo@collabora.com> - 2017-06-12 23:30 +0200
      Re: [PATCH 3/4] mfd: tps65217: remove duplicated interrupt resources. Grygorii Strashko <grygorii.strashko@ti.com> - 2017-06-13 00:50 +0200
        Re: [PATCH 3/4] mfd: tps65217: remove duplicated interrupt resources. Enric Balletbo Serra <eballetbo@gmail.com> - 2017-06-13 10:20 +0200
          Re: [PATCH 3/4] mfd: tps65217: remove duplicated interrupt resources. Grygorii Strashko <grygorii.strashko@ti.com> - 2017-06-14 01:10 +0200

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

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@collabora.com>
---
 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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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@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,
> 

-- 
regards,
-grygorii

[toc] | [prev] | [next] | [standalone]

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

Hi Grygorii,

2017-06-13 0:41 GMT+02:00 Grygorii Strashko <grygorii.strashko@ti.com>:
>
>
> 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@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,
>>
>
> --
> 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

Best regards,
 Enric

[toc] | [prev] | [next] | [standalone]

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

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@ti.com>:
>>
>>
>> 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web