From: Rob Herring <robherring2@gmail.com>
To: Tim Bird <tim.bird@sonymobile.com>
Cc: "Arnd Bergmann" <arnd@arndb.de>,
"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
linux-arm-msm <linux-arm-msm@vger.kernel.org>,
"Pawel Moll" <pawel.moll@arm.com>,
"Mark Rutland" <mark.rutland@arm.com>,
"Ian Campbell" <ijc+devicetree@hellion.org.uk>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"Andersson, Björn" <Bjorn.Andersson@sonymobile.com>,
"Mark Brown" <broonie@kernel.org>
Subject: Re: [PATCH 1/3] ARM: dts: qcom: Add binding for the qcom coincell charger
Date: Wed, 15 Jul 2015 16:22:45 -0500 [thread overview]
Message-ID: <CAL_Jsq+U7q1=BbRY47b077ys2NWMCTVE5SvgnWW4N2yKUAm95w@mail.gmail.com> (raw)
In-Reply-To: <55A6A55D.8000209@sonymobile.com>
On Wed, Jul 15, 2015 at 1:24 PM, Tim Bird <tim.bird@sonymobile.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@sonymobile.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@sonymobile.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
WARNING: multiple messages have this Message-ID (diff)
From: Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
To: Tim Bird <tim.bird-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
Cc: "Arnd Bergmann" <arnd-r2nGTMty4D4@public.gmane.org>,
"Greg Kroah-Hartman"
<gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>,
"devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
<devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
linux-arm-msm
<linux-arm-msm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
"Pawel Moll" <pawel.moll-5wv7dgnIgG8@public.gmane.org>,
"Mark Rutland" <mark.rutland-5wv7dgnIgG8@public.gmane.org>,
"Ian Campbell"
<ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>,
"linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
<linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
"Andersson,
Björn" <Bjorn.Andersson-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>,
"Mark Brown" <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Subject: Re: [PATCH 1/3] ARM: dts: qcom: Add binding for the qcom coincell charger
Date: Wed, 15 Jul 2015 16:22:45 -0500 [thread overview]
Message-ID: <CAL_Jsq+U7q1=BbRY47b077ys2NWMCTVE5SvgnWW4N2yKUAm95w@mail.gmail.com> (raw)
In-Reply-To: <55A6A55D.8000209-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
On Wed, Jul 15, 2015 at 1:24 PM, Tim Bird <tim.bird-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org> wrote:
> On 07/14/2015 06:07 PM, Rob Herring wrote:
>> On Tue, Jul 14, 2015 at 4:41 PM, Tim Bird <tim.bird-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org> wrote:
>>> On 07/13/2015 08:59 PM, Rob Herring wrote:
>>>> On Mon, Jul 13, 2015 at 6:39 PM, Tim Bird <tim.bird-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2015-07-15 21:23 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-13 23:39 [PATCH 1/3] ARM: dts: qcom: Add binding for the qcom coincell charger Tim Bird
2015-07-13 23:39 ` Tim Bird
2015-07-13 23:39 ` [PATCH 2/3] ARM: qcom: Add coincell charger driver Tim Bird
2015-07-13 23:39 ` Tim Bird
2015-07-13 23:39 ` [PATCH 3/3] ARM: dts: qcom: Add dts changes for qcom coincell charger Tim Bird
2015-07-13 23:39 ` Tim Bird
[not found] ` <CAL_JsqJ4poHe_644aOooAXJqwSvTW-3NPCFJ7LmaBj=wAQQp1w@mail.gmail.com>
[not found] ` <55A58221.7040004@sonymobile.com>
2015-07-15 1:07 ` [PATCH 1/3] ARM: dts: qcom: Add binding for the " Rob Herring
2015-07-15 1:07 ` Rob Herring
2015-07-15 18:24 ` Tim Bird
2015-07-15 21:22 ` Rob Herring [this message]
2015-07-15 21:22 ` Rob Herring
2015-07-15 22:27 ` Tim Bird
2015-07-15 22:27 ` Tim Bird
2015-07-16 0:53 ` Bjorn Andersson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAL_Jsq+U7q1=BbRY47b077ys2NWMCTVE5SvgnWW4N2yKUAm95w@mail.gmail.com' \
--to=robherring2@gmail.com \
--cc=Bjorn.Andersson@sonymobile.com \
--cc=arnd@arndb.de \
--cc=broonie@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=ijc+devicetree@hellion.org.uk \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=pawel.moll@arm.com \
--cc=tim.bird@sonymobile.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.