Date:	Wed, 15 Jul 2015 16:22:45 -0500
From:	Rob Herring <robherring2@...il.com>
To:	Tim Bird <tim.bird@...ymobile.com>
Cc:	Arnd Bergmann <arnd@...db.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	linux-arm-msm <linux-arm-msm@...r.kernel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Andersson, Björn 
	<Bjorn.Andersson@...ymobile.com>, Mark Brown <broonie@...nel.org>
Subject: 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 <tim.bird@...ymobile.com> wrote:
> On 07/14/2015 06:07 PM, Rob Herring wrote:
>> On Tue, Jul 14, 2015 at 4:41 PM, Tim Bird <tim.bird@...ymobile.com> wrote:
>>> On 07/13/2015 08:59 PM, Rob Herring wrote:
>>>> On Mon, Jul 13, 2015 at 6:39 PM, Tim Bird <tim.bird@...ymobile.com> 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 linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists