Message-Id: <20250103081836.4499-1-raufhakestefan@gmail.com>
Date: Fri,  3 Jan 2025 08:18:36 +0000
From: Stefan Raufhake <raufhakestefan@...il.com>
To: krzk@...nel.org,
	sebastian.reichel@...labora.com
Cc: sre@...nel.org,
	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

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

Powered by blists - more mailing lists