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