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

[Krzysztof Kozlowski <krzk@kernel.org>]

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