Message-ID: <xmjsvdyp63clrjkef4up7k4cgk5wfyldbij5ycsb7kumitnypi@pz6jpyyz45rv>
Date: Thu, 19 Dec 2024 01:58:21 +0100
From: Sebastian Reichel <sebastian.reichel@...labora.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Stefan Raufhake <raufhakestefan@...il.com>, linux-pm@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, s.raufhake@...khoff.com, 
	s.dirkwinkel@...khoff.com, s.raufhake@...khoff.de, robh@...nel.org, krzk+dt@...nel.org, 
	conor+dt@...nel.org
Subject: Re: [PATCH v2 1/1] power: supply: gpio-charger: Support to disable
 charger

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@...khoff.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@...khoff.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

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists