Re: [PATCH 1/3] ARM: dts: qcom: Add binding for the qcom coincell charger

On Wed, Jul 15, 2015 at 1:24 PM, Tim Bird [off-list ref] wrote:

On 07/14/2015 06:07 PM, Rob Herring wrote:

quoted

On Tue, Jul 14, 2015 at 4:41 PM, Tim Bird [off-list ref] wrote:

quoted

On 07/13/2015 08:59 PM, Rob Herring wrote:

quoted

On Mon, Jul 13, 2015 at 6:39 PM, Tim Bird [off-list ref] wrote:

quoted

This binding is used to configure the driver for the coincell charger
found in Qualcomm PMICs.

[...]

quoted

quoted

quoted

quoted

+- qcom,charge-enable:
+       Usage: optional
+       Value type: <u32> or <none>
+       Definition: definining this property, with an optional non-zero
+               value, enables charging

I'm not sure that this belongs in DT. Don't you want to enable
charging when plugged in perhaps or at some voltage threshold?

In practice this is never changed at runtime. It's only set at kernel boot.
The main use of this is to override (either on or off) whatever the firmware
did.

If your firmware and dtb are separate from your kernel, then ... (well
you know where I'm headed :) ).

Sorry, I have no idea how the sentence would end, so I think I'm missing
where you are headed.

dtbs should be separate from the kernel and part of the firmware. I'm
certain you recall those discussions or have sucessfully blocked them
from memory. If the dtb is part of the firmware, then changing the dtb
to change the kernel's handling of this would not make a lot of sense.

I was going to say if you want to change what firmware did, then you
could just do it from userspace. A delay from kernel boot to userspace
init would not matter here. However, if you have no other reason for
having a userspace interface, that probably isn't worth it and it is
fine as is.

quoted

If we do keep this, I think it should be a disable property with not
present being the default and enabling charging. Also, it only needs
to be bool (i.e. no value).

Are you suggesting something like this, then?

  - qcom,charger-disable:
       Usage: optional
       Value type: <none>

s/<none>/boolean/

But otherwise, yes this looks fine.

       Definition: defining this property disables charging

The logic would be as follows:
 - if the developer wants to just use the firmware settings, then
  the kernel would just not define this dts node at all, and nothing
  would change on kernel boot

Well, the kernel doesn't decide dts settings, but yes I agree that
removing or disabling the node would disable any kernel control.

 - if the developers want to change the settings, either turning off
  the charger, or specifying desired settings, then they define
  the appropriate attributes.

I'm OK with that.

I am too.

It would make no sense to define rset and vset values when this
is defined.  Should I note that somewhere in the binding doc?

They are somewhat don't care unless changing them has some side
effect. I'll leave it up to you.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html