On Wed, Jul 15, 2015 at 1:24 PM, Tim Bird <tim.bird@xxxxxxxxxxxxxx> wrote: > On 07/14/2015 06:07 PM, Rob Herring wrote: >> On Tue, Jul 14, 2015 at 4:41 PM, Tim Bird <tim.bird@xxxxxxxxxxxxxx> wrote: >>> On 07/13/2015 08:59 PM, Rob Herring wrote: >>>> On Mon, Jul 13, 2015 at 6:39 PM, Tim Bird <tim.bird@xxxxxxxxxxxxxx> wrote: >>>>> This binding is used to configure the driver for the coincell charger >>>>> found in Qualcomm PMICs. [...] >>>>> +- 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. > >> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
