[PATCH v2 0/1] power: supply: gpio-charger: Support to disable charger

[PATCH v2 0/1] power: supply: gpio-charger: Support to disable charger

[Stefan Raufhake]

From: Stefan Raufhake <s.raufhake@beckhoff.de>

Hello,
We have a device with a simple built-in UPS, which
is controlled by GPIOs. For the control of the UPS,
we used the “gpio-charger” driver module from the
power-supply. For our usecase we want to support 
enabing and disabling the ups. When the UPS is 
disabled, the device turns off immediately if the
supply power is switched off. When the UPS is
enabled, the device has power as long as the UPS
has enough energy for the device. Now, we are
looking for the best way to implement this
function. This patch contains our first proposal.

Changes since v1:
=================

- Rework of the commit message
- Changed the property name from "charge-disable-gpios" to "enable-gpios".
- Inverted the logic of charge_type to 0 = disable and 1 = enable for the 
charger.


Stefan Raufhake (1):
  power: supply: gpio-charger: Support to disable charger

 .../bindings/power/supply/gpio-charger.yaml   |  6 +++
 drivers/power/supply/gpio-charger.c           | 43 +++++++++++++++++++
 2 files changed, 49 insertions(+)

-- 
2.25.1

[PATCH v2 1/1] power: supply: gpio-charger: Support to disable charger

[Stefan Raufhake]

From: Stefan Raufhake <s.raufhake@beckhoff.de>

Some GPIO-controlled power supplies can be turned off (charging disabled).
Support changing the charging state by setting charge_type to
POWER_SUPPLY_CHARGE_TYPE_STANDARD and disabling charging by setting
charge_type to POWER_SUPPLY_CHARGE_TYPE_NONE. One potential use case for
this is disabling battery backup on a UPS.

Signed-off-by: Stefan Raufhake <s.raufhake@beckhoff.de>
---
 .../bindings/power/supply/gpio-charger.yaml   |  6 +++
 drivers/power/supply/gpio-charger.c           | 43 +++++++++++++++++++
 2 files changed, 49 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml b/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml
index 89f8e2bcb2d7..084520bfc040 100644
--- a/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml
+++ b/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml
@@ -44,6 +44,10 @@ properties:
     maxItems: 32
     description: GPIOs used for current limiting
 
+  enable-gpios:
+    maxItems: 1
+    description: GPIO is used to enable/disable the charger
+
   charge-current-limit-mapping:
     description: List of tuples with current in uA and a GPIO bitmap (in
       this order). The tuples must be provided in descending order of the
@@ -68,6 +72,8 @@ anyOf:
       - charge-status-gpios
   - required:
       - charge-current-limit-gpios
+  - required:
+      - enable-gpios
 
 dependencies:
   charge-current-limit-gpios: [ charge-current-limit-mapping ]
diff --git a/drivers/power/supply/gpio-charger.c b/drivers/power/supply/gpio-charger.c
index 68212b39785b..461fec34904d 100644
--- a/drivers/power/supply/gpio-charger.c
+++ b/drivers/power/supply/gpio-charger.c
@@ -32,6 +32,7 @@ struct gpio_charger {
 	struct power_supply_desc charger_desc;
 	struct gpio_desc *gpiod;
 	struct gpio_desc *charge_status;
+	struct gpio_desc *charge_type;
 
 	struct gpio_descs *current_limit_gpios;
 	struct gpio_mapping *current_limit_map;
@@ -82,6 +83,26 @@ static int set_charge_current_limit(struct gpio_charger *gpio_charger, int val)
 	return 0;
 }
 
+static int gpio_charger_set_charge_type(struct gpio_desc *gpio_charger, int type)
+{
+	int chg_config = 0;
+
+	switch (type) {
+	case POWER_SUPPLY_CHARGE_TYPE_STANDARD:
+		chg_config = 1;
+		break;
+	case POWER_SUPPLY_CHARGE_TYPE_NONE:
+		chg_config = 0;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	gpiod_set_value_cansleep(gpio_charger, chg_config);
+
+	return 0;
+}
+
 static int gpio_charger_get_property(struct power_supply *psy,
 		enum power_supply_property psp, union power_supply_propval *val)
 {
@@ -100,6 +121,13 @@ static int gpio_charger_get_property(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
 		val->intval = gpio_charger->charge_current_limit;
 		break;
+	case POWER_SUPPLY_PROP_CHARGE_TYPE:
+		if (gpiod_get_value_cansleep(gpio_charger->charge_type))
+			val->intval = POWER_SUPPLY_CHARGE_TYPE_STANDARD;
+		else
+			val->intval = POWER_SUPPLY_CHARGE_TYPE_NONE;
+		break;
+
 	default:
 		return -EINVAL;
 	}
@@ -115,6 +143,9 @@ static int gpio_charger_set_property(struct power_supply *psy,
 	switch (psp) {
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
 		return set_charge_current_limit(gpio_charger, val->intval);
+	case POWER_SUPPLY_PROP_CHARGE_TYPE:
+		return gpio_charger_set_charge_type(gpio_charger->charge_type, val->intval);
+	break;
 	default:
 		return -EINVAL;
 	}
@@ -126,6 +157,7 @@ static int gpio_charger_property_is_writeable(struct power_supply *psy,
 					      enum power_supply_property psp)
 {
 	switch (psp) {
+	case POWER_SUPPLY_PROP_CHARGE_TYPE:
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
 		return 1;
 	default:
@@ -246,6 +278,7 @@ static enum power_supply_property gpio_charger_properties[] = {
 	POWER_SUPPLY_PROP_ONLINE,
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
+	POWER_SUPPLY_PROP_CHARGE_TYPE,
 };
 
 static int gpio_charger_probe(struct platform_device *pdev)
@@ -256,6 +289,7 @@ static int gpio_charger_probe(struct platform_device *pdev)
 	struct gpio_charger *gpio_charger;
 	struct power_supply_desc *charger_desc;
 	struct gpio_desc *charge_status;
+	struct gpio_desc *charge_type;
 	int charge_status_irq;
 	int ret;
 	int num_props = 0;
@@ -304,6 +338,15 @@ static int gpio_charger_probe(struct platform_device *pdev)
 		num_props++;
 	}
 
+	charge_type = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_HIGH);
+	if (IS_ERR(charge_type))
+		return PTR_ERR(charge_type);
+	if (charge_type) {
+		gpio_charger->charge_type = charge_type;
+		gpio_charger_properties[num_props] = POWER_SUPPLY_PROP_CHARGE_TYPE;
+		num_props++;
+	}
+
 	charger_desc = &gpio_charger->charger_desc;
 	charger_desc->properties = gpio_charger_properties;
 	charger_desc->num_properties = num_props;
-- 
2.25.1

Re: [PATCH v2 1/1] power: supply: gpio-charger: Support to disable charger

[Krzysztof Kozlowski]

On Tue, Dec 10, 2024 at 09:23:43AM +0000, Stefan Raufhake wrote:
> From: Stefan Raufhake <s.raufhake@beckhoff.de>
> 
> Some GPIO-controlled power supplies can be turned off (charging disabled).
> Support changing the charging state by setting charge_type to
> POWER_SUPPLY_CHARGE_TYPE_STANDARD and disabling charging by setting
> charge_type to POWER_SUPPLY_CHARGE_TYPE_NONE. One potential use case for
> this is disabling battery backup on a UPS.
> 
> Signed-off-by: Stefan Raufhake <s.raufhake@beckhoff.de>
> ---
>  .../bindings/power/supply/gpio-charger.yaml   |  6 +++
>  drivers/power/supply/gpio-charger.c           | 43 +++++++++++++++++++
>  2 files changed, 49 insertions(+)
> 

<form letter>
This is a friendly reminder during the review process.

It seems my or other reviewer's previous comments were not fully
addressed. Maybe the feedback got lost between the quotes, maybe you
just forgot to apply it. Please go back to the previous discussion and
either implement all requested changes or keep discussing them.

Thank you.
</form letter>

> diff --git a/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml b/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml
> index 89f8e2bcb2d7..084520bfc040 100644
> --- a/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml
> +++ b/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml
> @@ -44,6 +44,10 @@ properties:
>      maxItems: 32
>      description: GPIOs used for current limiting
>  
> +  enable-gpios:
> +    maxItems: 1
> +    description: GPIO is used to enable/disable the charger
> +

You did not respond to my comments, nothing improved. Without
explanation based on hardware - which I asked - this is still a no.

Implement and respond fully to previous feedback.

Best regards,
Krzysztof

Re: [PATCH v2 1/1] power: supply: gpio-charger: Support to disable charger

[Stefan Raufhake]

Hallo Krzysztof,

>
> On Tue, Dec 10, 2024 at 09:23:43AM +0000, Stefan Raufhake wrote:
> > From: Stefan Raufhake <s.raufhake@beckhoff.de>
> >
> > Some GPIO-controlled power supplies can be turned off (charging disabled).
> > Support changing the charging state by setting charge_type to
> > POWER_SUPPLY_CHARGE_TYPE_STANDARD and disabling charging by setting
> > charge_type to POWER_SUPPLY_CHARGE_TYPE_NONE. One potential use case for
> > this is disabling battery backup on a UPS.
> >
> > Signed-off-by: Stefan Raufhake <s.raufhake@beckhoff.de>
> > ---
> >  .../bindings/power/supply/gpio-charger.yaml   |  6 +++
> >  drivers/power/supply/gpio-charger.c           | 43 +++++++++++++++++++
> >  2 files changed, 49 insertions(+)
> >
>
> <form letter>
> This is a friendly reminder during the review process.
>
> It seems my or other reviewer's previous comments were not fully
> addressed. Maybe the feedback got lost between the quotes, maybe you
> just forgot to apply it. Please go back to the previous discussion and
> either implement all requested changes or keep discussing them.
>
> Thank you.
> </form letter>

Sorry, it seems I made a mistake during the patch review process. 
Should I reply to your email about version 1 of the patch or only about
version 2? I don't want to make another mistake and open two discussions 
at the same time. 
I hope to do better in the future.

>
> > diff --git a/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml b/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml
> > index 89f8e2bcb2d7..084520bfc040 100644
> > --- a/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml
> > +++ b/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml
> > @@ -44,6 +44,10 @@ properties:
> >      maxItems: 32
> >      description: GPIOs used for current limiting
> >
> > +  enable-gpios:
> > +    maxItems: 1
> > +    description: GPIO is used to enable/disable the charger
> > +
>
> You did not respond to my comments, nothing improved. Without
> explanation based on hardware - which I asked - this is still a no.
>
> Implement and respond fully to previous feedback.
>
> Best regards,
> Krzysztof
>


Sorry, I'm new to this and don't really know what exactly you want for the
hardware description and how best to represent our hardware in dts.
For the gpio power supply, it can basically be any circuit that implements
a "fully charged" GPIO and a "disable ups" GPIO.

We're using a circuit built around the LTC3350 (super capacitor ups chip):
We use this pin to indicate that our UPS is fully charged (once the input
is gone, it's not fully charged anymore):
PFO (pin 38): Power-Fail Status Output. This open-drain output is pulled
low when a power failure has occurred.
 
For the "disable ups" GPIO, we have some external circuitry around the 
LTC3350. I can't share the schematic, but it boils down to "disable usage
of ups" so that the device shuts down immediately when power is lost.
 
We've implemented this in many of our devices, but first we're looking 
at [1] and [2], which we also want to upstream the device trees for.
[1] https://www.beckhoff.com/en-en/products/ipc/embedded-pcs/cx9240-arm-r-cortex-r-a53/cx9240.html
[2] https://www.beckhoff.com/en-en/products/ipc/embedded-pcs/cx8200-arm-r-cortex-r-a53/cx8200.html

For the LTC3350, there is a separate driver posted to the Linux kernel
mail list [3] by another devolper that we would like to use in the future,
but without this gpio, our circuit won't work.
[3] https://lore.kernel.org/all/?q=power%3A+supply%3A+ltc3350-charger

Best regards, 

Stefan 

Re: [PATCH v2 1/1] power: supply: gpio-charger: Support to disable charger

[Krzysztof Kozlowski]

On 13/12/2024 11:28, Stefan Raufhake wrote:
> Hallo Krzysztof,
> 
>>
>> On Tue, Dec 10, 2024 at 09:23:43AM +0000, Stefan Raufhake wrote:
>>> From: Stefan Raufhake <s.raufhake@beckhoff.de>
>>>
>>> Some GPIO-controlled power supplies can be turned off (charging disabled).
>>> Support changing the charging state by setting charge_type to
>>> POWER_SUPPLY_CHARGE_TYPE_STANDARD and disabling charging by setting
>>> charge_type to POWER_SUPPLY_CHARGE_TYPE_NONE. One potential use case for
>>> this is disabling battery backup on a UPS.
>>>
>>> Signed-off-by: Stefan Raufhake <s.raufhake@beckhoff.de>
>>> ---
>>>  .../bindings/power/supply/gpio-charger.yaml   |  6 +++
>>>  drivers/power/supply/gpio-charger.c           | 43 +++++++++++++++++++
>>>  2 files changed, 49 insertions(+)
>>>
>>
>> <form letter>
>> This is a friendly reminder during the review process.
>>
>> It seems my or other reviewer's previous comments were not fully
>> addressed. Maybe the feedback got lost between the quotes, maybe you
>> just forgot to apply it. Please go back to the previous discussion and
>> either implement all requested changes or keep discussing them.
>>
>> Thank you.
>> </form letter>
> 
> Sorry, it seems I made a mistake during the patch review process. 
> Should I reply to your email about version 1 of the patch or only about
> version 2? I don't want to make another mistake and open two discussions 
> at the same time. 
> I hope to do better in the future.
> 
>>
>>> diff --git a/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml b/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml
>>> index 89f8e2bcb2d7..084520bfc040 100644
>>> --- a/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml
>>> +++ b/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml
>>> @@ -44,6 +44,10 @@ properties:
>>>      maxItems: 32
>>>      description: GPIOs used for current limiting
>>>
>>> +  enable-gpios:
>>> +    maxItems: 1
>>> +    description: GPIO is used to enable/disable the charger
>>> +
>>
>> You did not respond to my comments, nothing improved. Without
>> explanation based on hardware - which I asked - this is still a no.
>>
>> Implement and respond fully to previous feedback.
>>
>> Best regards,
>> Krzysztof
>>
> 
> 
> Sorry, I'm new to this and don't really know what exactly you want for the
> hardware description and how best to represent our hardware in dts.
> For the gpio power supply, it can basically be any circuit that implements
> a "fully charged" GPIO and a "disable ups" GPIO.
> 
> We're using a circuit built around the LTC3350 (super capacitor ups chip):
> We use this pin to indicate that our UPS is fully charged (once the input
> is gone, it's not fully charged anymore):
> PFO (pin 38): Power-Fail Status Output. This open-drain output is pulled
> low when a power failure has occurred.
>  
> For the "disable ups" GPIO, we have some external circuitry around the 
> LTC3350. I can't share the schematic, but it boils down to "disable usage
> of ups" so that the device shuts down immediately when power is lost.
>  
> We've implemented this in many of our devices, but first we're looking 
> at [1] and [2], which we also want to upstream the device trees for.
> [1] https://www.beckhoff.com/en-en/products/ipc/embedded-pcs/cx9240-arm-r-cortex-r-a53/cx9240.html
> [2] https://www.beckhoff.com/en-en/products/ipc/embedded-pcs/cx8200-arm-r-cortex-r-a53/cx8200.html
> 
> For the LTC3350, there is a separate driver posted to the Linux kernel
> mail list [3] by another devolper that we would like to use in the future,
> but without this gpio, our circuit won't work.
> [3] https://lore.kernel.org/all/?q=power%3A+supply%3A+ltc3350-charger

This does not address my concerns at all. Read the previous comments -
you are duplicating existing property.


Best regards,
Krzysztof

Re: [PATCH v2 1/1] power: supply: gpio-charger: Support to disable charger

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 5202 bytes --]

Hi,

On Mon, Dec 16, 2024 at 08:30:45AM +0100, Krzysztof Kozlowski wrote:
> On 13/12/2024 11:28, Stefan Raufhake wrote:
> > Hallo Krzysztof,
> > 
> >>
> >> On Tue, Dec 10, 2024 at 09:23:43AM +0000, Stefan Raufhake wrote:
> >>> From: Stefan Raufhake <s.raufhake@beckhoff.de>
> >>>
> >>> Some GPIO-controlled power supplies can be turned off (charging disabled).
> >>> Support changing the charging state by setting charge_type to
> >>> POWER_SUPPLY_CHARGE_TYPE_STANDARD and disabling charging by setting
> >>> charge_type to POWER_SUPPLY_CHARGE_TYPE_NONE. One potential use case for
> >>> this is disabling battery backup on a UPS.
> >>>
> >>> Signed-off-by: Stefan Raufhake <s.raufhake@beckhoff.de>
> >>> ---
> >>>  .../bindings/power/supply/gpio-charger.yaml   |  6 +++
> >>>  drivers/power/supply/gpio-charger.c           | 43 +++++++++++++++++++
> >>>  2 files changed, 49 insertions(+)
> >>>
> >>
> >> <form letter>
> >> This is a friendly reminder during the review process.
> >>
> >> It seems my or other reviewer's previous comments were not fully
> >> addressed. Maybe the feedback got lost between the quotes, maybe you
> >> just forgot to apply it. Please go back to the previous discussion and
> >> either implement all requested changes or keep discussing them.
> >>
> >> Thank you.
> >> </form letter>
> > 
> > Sorry, it seems I made a mistake during the patch review process. 
> > Should I reply to your email about version 1 of the patch or only about
> > version 2? I don't want to make another mistake and open two discussions 
> > at the same time. 
> > I hope to do better in the future.
> > 
> >>
> >>> diff --git a/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml b/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml
> >>> index 89f8e2bcb2d7..084520bfc040 100644
> >>> --- a/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml
> >>> +++ b/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml
> >>> @@ -44,6 +44,10 @@ properties:
> >>>      maxItems: 32
> >>>      description: GPIOs used for current limiting
> >>>
> >>> +  enable-gpios:
> >>> +    maxItems: 1
> >>> +    description: GPIO is used to enable/disable the charger
> >>> +
> >>
> >> You did not respond to my comments, nothing improved. Without
> >> explanation based on hardware - which I asked - this is still a no.
> >>
> >> Implement and respond fully to previous feedback.
> >>
> >> Best regards,
> >> Krzysztof
> >>
> > 
> > 
> > Sorry, I'm new to this and don't really know what exactly you want for the
> > hardware description and how best to represent our hardware in dts.
> > For the gpio power supply, it can basically be any circuit that implements
> > a "fully charged" GPIO and a "disable ups" GPIO.
> > 
> > We're using a circuit built around the LTC3350 (super capacitor ups chip):
> > We use this pin to indicate that our UPS is fully charged (once the input
> > is gone, it's not fully charged anymore):
> > PFO (pin 38): Power-Fail Status Output. This open-drain output is pulled
> > low when a power failure has occurred.
> >  
> > For the "disable ups" GPIO, we have some external circuitry around the 
> > LTC3350. I can't share the schematic, but it boils down to "disable usage
> > of ups" so that the device shuts down immediately when power is lost.
> >  
> > We've implemented this in many of our devices, but first we're looking 
> > at [1] and [2], which we also want to upstream the device trees for.
> > [1] https://www.beckhoff.com/en-en/products/ipc/embedded-pcs/cx9240-arm-r-cortex-r-a53/cx9240.html
> > [2] https://www.beckhoff.com/en-en/products/ipc/embedded-pcs/cx8200-arm-r-cortex-r-a53/cx8200.html
> > 
> > For the LTC3350, there is a separate driver posted to the Linux kernel
> > mail list [3] by another devolper that we would like to use in the future,
> > but without this gpio, our circuit won't work.
> > [3] https://lore.kernel.org/all/?q=power%3A+supply%3A+ltc3350-charger
> 
> This does not address my concerns at all. Read the previous comments -
> you are duplicating existing property.

I think there is some misunderstanding. IIUIC you (Krzysztof) are
referencing the following existing gpios property without any
prefix?

>  gpios:
>    maxItems: 1
>    description: GPIO indicating the charger presence

This informs the operating system, that the charger has been plugged
in (so the GPIO is an input from operating system point of view).

The work from Stefan is not about presence detection, but
controlling if the charging should happen at all (so the GPIO is an
output from operating system point of view). So that's two very
different things.

Technically there is some overlap with another existing property:
charge-current-limit-gpios. I suppose a charger device limited to
0 Microampere is effectively off. But I think its fair to have a
dedicated GPIO for explicit disabling.

If my analysis of the situation is correct, the documentation seems
to be bad. Please suggest better wording :)

P.S.: binding and driver should be send in separate patches.

Greetings,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/1] power: supply: gpio-charger: Support to disable charger

[Krzysztof Kozlowski]

On 19/12/2024 01:58, Sebastian Reichel wrote:
> Hi,
> 
> On Mon, Dec 16, 2024 at 08:30:45AM +0100, Krzysztof Kozlowski wrote:
>> On 13/12/2024 11:28, Stefan Raufhake wrote:
>>> Hallo Krzysztof,
>>>
>>>>
>>>> On Tue, Dec 10, 2024 at 09:23:43AM +0000, Stefan Raufhake wrote:
>>>>> From: Stefan Raufhake <s.raufhake@beckhoff.de>
>>>>>
>>>>> Some GPIO-controlled power supplies can be turned off (charging disabled).
>>>>> Support changing the charging state by setting charge_type to
>>>>> POWER_SUPPLY_CHARGE_TYPE_STANDARD and disabling charging by setting
>>>>> charge_type to POWER_SUPPLY_CHARGE_TYPE_NONE. One potential use case for
>>>>> this is disabling battery backup on a UPS.
>>>>>
>>>>> Signed-off-by: Stefan Raufhake <s.raufhake@beckhoff.de>
>>>>> ---
>>>>>  .../bindings/power/supply/gpio-charger.yaml   |  6 +++
>>>>>  drivers/power/supply/gpio-charger.c           | 43 +++++++++++++++++++
>>>>>  2 files changed, 49 insertions(+)
>>>>>
>>>>
>>>> <form letter>
>>>> This is a friendly reminder during the review process.
>>>>
>>>> It seems my or other reviewer's previous comments were not fully
>>>> addressed. Maybe the feedback got lost between the quotes, maybe you
>>>> just forgot to apply it. Please go back to the previous discussion and
>>>> either implement all requested changes or keep discussing them.
>>>>
>>>> Thank you.
>>>> </form letter>
>>>
>>> Sorry, it seems I made a mistake during the patch review process. 
>>> Should I reply to your email about version 1 of the patch or only about
>>> version 2? I don't want to make another mistake and open two discussions 
>>> at the same time. 
>>> I hope to do better in the future.
>>>
>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml b/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml
>>>>> index 89f8e2bcb2d7..084520bfc040 100644
>>>>> --- a/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml
>>>>> +++ b/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml
>>>>> @@ -44,6 +44,10 @@ properties:
>>>>>      maxItems: 32
>>>>>      description: GPIOs used for current limiting
>>>>>
>>>>> +  enable-gpios:
>>>>> +    maxItems: 1
>>>>> +    description: GPIO is used to enable/disable the charger
>>>>> +
>>>>
>>>> You did not respond to my comments, nothing improved. Without
>>>> explanation based on hardware - which I asked - this is still a no.
>>>>
>>>> Implement and respond fully to previous feedback.
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>>
>>>
>>>
>>> Sorry, I'm new to this and don't really know what exactly you want for the
>>> hardware description and how best to represent our hardware in dts.
>>> For the gpio power supply, it can basically be any circuit that implements
>>> a "fully charged" GPIO and a "disable ups" GPIO.
>>>
>>> We're using a circuit built around the LTC3350 (super capacitor ups chip):
>>> We use this pin to indicate that our UPS is fully charged (once the input
>>> is gone, it's not fully charged anymore):
>>> PFO (pin 38): Power-Fail Status Output. This open-drain output is pulled
>>> low when a power failure has occurred.
>>>  
>>> For the "disable ups" GPIO, we have some external circuitry around the 
>>> LTC3350. I can't share the schematic, but it boils down to "disable usage
>>> of ups" so that the device shuts down immediately when power is lost.
>>>  
>>> We've implemented this in many of our devices, but first we're looking 
>>> at [1] and [2], which we also want to upstream the device trees for.
>>> [1] https://www.beckhoff.com/en-en/products/ipc/embedded-pcs/cx9240-arm-r-cortex-r-a53/cx9240.html
>>> [2] https://www.beckhoff.com/en-en/products/ipc/embedded-pcs/cx8200-arm-r-cortex-r-a53/cx8200.html
>>>
>>> For the LTC3350, there is a separate driver posted to the Linux kernel
>>> mail list [3] by another devolper that we would like to use in the future,
>>> but without this gpio, our circuit won't work.
>>> [3] https://lore.kernel.org/all/?q=power%3A+supply%3A+ltc3350-charger
>>
>> This does not address my concerns at all. Read the previous comments -
>> you are duplicating existing property.
> 
> I think there is some misunderstanding. IIUIC you (Krzysztof) are
> referencing the following existing gpios property without any
> prefix?
> 
>>  gpios:
>>    maxItems: 1
>>    description: GPIO indicating the charger presence
> 
> This informs the operating system, that the charger has been plugged
> in (so the GPIO is an input from operating system point of view).
> 
> The work from Stefan is not about presence detection, but
> controlling if the charging should happen at all (so the GPIO is an
> output from operating system point of view). So that's two very
> different things.

So the gpios and charging status are input GPIOs and this is an output?
If so this seems right, indeed.

> 
> Technically there is some overlap with another existing property:
> charge-current-limit-gpios. I suppose a charger device limited to
> 0 Microampere is effectively off. But I think its fair to have a
> dedicated GPIO for explicit disabling.
> 
> If my analysis of the situation is correct, the documentation seems
> to be bad. Please suggest better wording :)
> 
> P.S.: binding and driver should be send in separate patches.

Yeah, still all my comments should be addressed.


Best regards,
Krzysztof

Re: [PATCH v2 1/1] power: supply: gpio-charger: Support to disable charger

[Stefan Raufhake]

Hallo, 

>
> On 19/12/2024 01:58, Sebastian Reichel wrote:
> > Hi,
> >
> > On Mon, Dec 16, 2024 at 08:30:45AM +0100, Krzysztof Kozlowski wrote:
> >> On 13/12/2024 11:28, Stefan Raufhake wrote:
> >>> Hallo Krzysztof,
> >>>
> >>>>
> >>>> On Tue, Dec 10, 2024 at 09:23:43AM +0000, Stefan Raufhake wrote:
> >>>>> From: Stefan Raufhake <s.raufhake@beckhoff.de>
> >>>>>
> >>>>> Some GPIO-controlled power supplies can be turned off (charging disabled).
> >>>>> Support changing the charging state by setting charge_type to
> >>>>> POWER_SUPPLY_CHARGE_TYPE_STANDARD and disabling charging by setting
> >>>>> charge_type to POWER_SUPPLY_CHARGE_TYPE_NONE. One potential use case for
> >>>>> this is disabling battery backup on a UPS.
> >>>>>
> >>>>> Signed-off-by: Stefan Raufhake <s.raufhake@beckhoff.de>
> >>>>> ---
> >>>>>  .../bindings/power/supply/gpio-charger.yaml   |  6 +++
> >>>>>  drivers/power/supply/gpio-charger.c           | 43 +++++++++++++++++++
> >>>>>  2 files changed, 49 insertions(+)
> >>>>>
> >>>>
> >>>> <form letter>
> >>>> This is a friendly reminder during the review process.
> >>>>
> >>>> It seems my or other reviewer's previous comments were not fully
> >>>> addressed. Maybe the feedback got lost between the quotes, maybe you
> >>>> just forgot to apply it. Please go back to the previous discussion and
> >>>> either implement all requested changes or keep discussing them.
> >>>>
> >>>> Thank you.
> >>>> </form letter>
> >>>
> >>> Sorry, it seems I made a mistake during the patch review process.
> >>> Should I reply to your email about version 1 of the patch or only about
> >>> version 2? I don't want to make another mistake and open two discussions
> >>> at the same time.
> >>> I hope to do better in the future.
> >>>
> >>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml b/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml
> >>>>> index 89f8e2bcb2d7..084520bfc040 100644
> >>>>> --- a/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml
> >>>>> +++ b/Documentation/devicetree/bindings/power/supply/gpio-charger.yaml
> >>>>> @@ -44,6 +44,10 @@ properties:
> >>>>>      maxItems: 32
> >>>>>      description: GPIOs used for current limiting
> >>>>>
> >>>>> +  enable-gpios:
> >>>>> +    maxItems: 1
> >>>>> +    description: GPIO is used to enable/disable the charger
> >>>>> +
> >>>>
> >>>> You did not respond to my comments, nothing improved. Without
> >>>> explanation based on hardware - which I asked - this is still a no.
> >>>>
> >>>> Implement and respond fully to previous feedback.
> >>>>
> >>>> Best regards,
> >>>> Krzysztof
> >>>>
> >>>
> >>>
> >>> Sorry, I'm new to this and don't really know what exactly you want for the
> >>> hardware description and how best to represent our hardware in dts.
> >>> For the gpio power supply, it can basically be any circuit that implements
> >>> a "fully charged" GPIO and a "disable ups" GPIO.
> >>>
> >>> We're using a circuit built around the LTC3350 (super capacitor ups chip):
> >>> We use this pin to indicate that our UPS is fully charged (once the input
> >>> is gone, it's not fully charged anymore):
> >>> PFO (pin 38): Power-Fail Status Output. This open-drain output is pulled
> >>> low when a power failure has occurred.
> >>>
> >>> For the "disable ups" GPIO, we have some external circuitry around the
> >>> LTC3350. I can't share the schematic, but it boils down to "disable usage
> >>> of ups" so that the device shuts down immediately when power is lost.
> >>>
> >>> We've implemented this in many of our devices, but first we're looking
> >>> at [1] and [2], which we also want to upstream the device trees for.
> >>> [1] https://www.beckhoff.com/en-en/products/ipc/embedded-pcs/cx9240-arm-r-cortex-r-a53/cx9240.html
> >>> [2] https://www.beckhoff.com/en-en/products/ipc/embedded-pcs/cx8200-arm-r-cortex-r-a53/cx8200.html
> >>>
> >>> For the LTC3350, there is a separate driver posted to the Linux kernel
> >>> mail list [3] by another devolper that we would like to use in the future,
> >>> but without this gpio, our circuit won't work.
> >>> [3] https://lore.kernel.org/all/?q=power%3A+supply%3A+ltc3350-charger
> >>
> >> This does not address my concerns at all. Read the previous comments -
> >> you are duplicating existing property.
> >
> > I think there is some misunderstanding. IIUIC you (Krzysztof) are
> > referencing the following existing gpios property without any
> > prefix?
> >
> >>  gpios:
> >>    maxItems: 1
> >>    description: GPIO indicating the charger presence
> >
> > This informs the operating system, that the charger has been plugged
> > in (so the GPIO is an input from operating system point of view).
> >
> > The work from Stefan is not about presence detection, but
> > controlling if the charging should happen at all (so the GPIO is an
> > output from operating system point of view). So that's two very
> > different things.
>
> So the gpios and charging status are input GPIOs and this is an output?
> If so this seems right, indeed.
>

Yes, Krzysztof, you see it right. Sebastian described the problem correctly from my point of view.

> >
> > Technically there is some overlap with another existing property:
> > charge-current-limit-gpios. I suppose a charger device limited to
> > 0 Microampere is effectively off. But I think its fair to have a
> > dedicated GPIO for explicit disabling.
> >
> > If my analysis of the situation is correct, the documentation seems
> > to be bad. Please suggest better wording :)

Which part of the documentation is being referred to: the binding, the commit message, or another section? 
Once clarified, I can suggest a better wording in this part of the documentation.

> > P.S.: binding and driver should be send in separate patches.
>

In the next version, I will split the binding and driver into two separate patches.

> Yeah, still all my comments should be addressed.
>

Krzysztof, in the bindings for 'gpio-charger.yaml' (Documentation/devicetree/bindings/power/supply/gpio-charger.yaml), 
is the property name 'enable-gpios' suitable for you, or should I rename it? 
If a rename is needed, which name makes the most sense to you for this function?

>
> Best regards,
> Krzysztof


Best regards,

Stefan

Re: [PATCH v2 1/1] power: supply: gpio-charger: Support to disable charger

[Krzysztof Kozlowski]

On 03/01/2025 09:18, Stefan Raufhake wrote:
>>> to be bad. Please suggest better wording :)
> 
> Which part of the documentation is being referred to: the binding, the commit message, or another section? 
> Once clarified, I can suggest a better wording in this part of the documentation.
> 
>>> P.S.: binding and driver should be send in separate patches.
>>
> 
> In the next version, I will split the binding and driver into two separate patches.
> 
>> Yeah, still all my comments should be addressed.
>>
> 
> Krzysztof, in the bindings for 'gpio-charger.yaml' (Documentation/devicetree/bindings/power/supply/gpio-charger.yaml), 
> is the property name 'enable-gpios' suitable for you, or should I rename it? 
> If a rename is needed, which name makes the most sense to you for this function?

enable-gpios is correct, assuming these is a different GPIO than one
used for "charge-current-limit-gpios" for value of 0, as pointed out by
Sebastian.

Existing example DTS in the binding clearly defines A.11 as
enable-gpios. Maybe that's just coincidence, but I wonder how it would
work for three gpios?

Anyway the example should be then fixed to reflect real intention, e.g.
add enable-gpios and change "<0 0x02>; // 0 mA => GPIO A.11 high" into
some value like 300 mA.

Best regards,
Krzysztof