On 07/15/2015 02:22 PM, Rob Herring wrote: > 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. Ah yes, those discussions. :-) Having dtbs come from firmware is not on the horizon yet for projects I'm working on, so I haven't really considered the ramifications. > 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. Indeed. > 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. OK - these are indeed "don't care" in that case. I probably don't have to explain in the binding doc that adjusting settings for disabled hardware doesn't make sense. Thanks again for the quick feedback. -- Tim -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
