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

[Krzysztof Kozlowski <krzk@kernel.org>]

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