Re: [PATCH 13/13] dt: power: bq24257-charger: Cover additional devices

[Krzysztof Kozlowski <k.kozlowski@samsung.com>]

On 04.09.2015 01:09, Andreas Dannenberg wrote:
> On Thu, Sep 03, 2015 at 10:57:12AM +0900, Krzysztof Kozlowski wrote:
>> On 03.09.2015 10:47, Andreas Dannenberg wrote:
>>> On Thu, Sep 03, 2015 at 10:31:20AM +0900, Krzysztof Kozlowski wrote:
>>>> I still don't get why you need extra "ti,pg-gpio-disable" property. It
>>>> could work like this:
>>>> 1. Get rid of "ti,pg-gpio-disable" and make "pg-gpio" optional.
>>>> 2. If it is present, use it.
>>>> 3. If it is not present, use software based approach (the same as
>>>> setting "ti,pg-gpio-disable" previously).
>>>>
>>>> Would that work?
>>>
>>> Similar to the earlier comment - the idea was to have it more explicit.
>>> Plus, the original driver would hard-fail when the "pg-gpio" pin is not
>>> provided (since it was essential for that driver's ability to function).
>>> If I change to automatically fallback to the SW solution such an error
>>> case would in theory not be 100% backward compatible. I could however
>>> more clearly indicate/log which method is being used (dev_info) so
>>> somebody rummaging through the logs would still see it. Will simplify.
>>
>> Right, the kernel's backward compatibility has to be preserved. However
>> for bindings I am not sure.
> 
> Hi Krzysztof. Please see pages 3-5 in the below PDF, that's what I had
> in mind. Maybe I mis-interpreted it or there is newer information.
> http://events.linuxfoundation.org/sites/events/files/slides/petazzoni-dt-as-stable-abi-fairy-tale.pdf

I think our case is not covered there. I am talking about backward
compatibility of DTB with older kernels, not about backward
compatibility of kernel.

AFAIR this is not a requirement so you can change the behaviour of DTB
(backward compatibility of kernel is fully preserved).

> 
> With the proposed changes, while the existing bindings (strings) would
> stay the same, the failure behavior in one specific use case would be
> slightly different. The question is, where should we draw the line for
> compatibility? I would argue in this case the general usage is not
> affected so it should be OK making the change/simplification you propose
> (getting rid of "ti,pg-gpio-disable"). 

If we don't care about using newer DTB with older kernel then there is
no compatibility line to draw. You can safely change the behaviour.

>  
>> Let's assume booting on device with bq24257.
>> 1. Old kernel booted with a DTB which doesn't have "pg-gpio" would print
>> error (probe would fail).
>> 2. New kernel booted in the same situation (1) would assume GPIOs have
>> to be disabled.
>>
>> 3. Old kernel booted with your previous solution (DTB containing both
>> "pg-gpio" and "ti,pg-gpio-disable") would work fine ignoring the
>> "disable" property.
>> 4. New kernel in the same situation (3) would disable GPIOs.
>>
>> There is a difference between (1) and (3). New DTB is not backward
>> compatible with old kernels for existing bq24257 devices.
>>
>> Am I understanding this correctly?
> 
> Well almost. I think the difference between case 1 and 3 isn't really an
> issue from a use case point of view. The case I was trying to describe
> is as follows (let's call it case 5):
> 
> 5. An old DTB not containing "pg-gpio" (or with that property being
> misconfigured) is booted with the new Kernel. The boot will succeed as
> the SW approach for PG detection gets invoked automatically.
> 
> The difference now is between (1) and (5). If in (5) somebody specifies
> "pg-gpio" they would want to explicitly use this pin and be notified if
> the respective setup fails. My proposal earlier for this was to output
> through dev_info() during driver probe whether an actual GPIO pin is
> being used for power-good detection or whether the fall-back SW
> algorithm has been invoked. This way, at least there is a notification.

I think I understand. IMHO if pg-gpio is specified and it fails (like
GPIO is invalid) the kernel should not switch to SW mode. This should be
all-or-nothing and described in a specific way: iff pg-gpio is missing
then SW mode would be used because that was the intention of developer.

Best regards,
Krzysztof