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