From: Tim Bird <tim.bird@sonymobile.com>
To: Rob Herring <robherring2@gmail.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 15:27:40 -0700 [thread overview]
Message-ID: <55A6DE5C.7020005@sonymobile.com> (raw)
In-Reply-To: <CAL_Jsq+U7q1=BbRY47b077ys2NWMCTVE5SvgnWW4N2yKUAm95w@mail.gmail.com>
On 07/15/2015 02:22 PM, Rob Herring wrote:
> 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.
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
WARNING: multiple messages have this Message-ID (diff)
From: Tim Bird <tim.bird-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
To: Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@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 15:27:40 -0700 [thread overview]
Message-ID: <55A6DE5C.7020005@sonymobile.com> (raw)
In-Reply-To: <CAL_Jsq+U7q1=BbRY47b077ys2NWMCTVE5SvgnWW4N2yKUAm95w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 07/15/2015 02:22 PM, Rob Herring wrote:
> 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.
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 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 22:27 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
2015-07-15 21:22 ` Rob Herring
2015-07-15 22:27 ` Tim Bird [this message]
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=55A6DE5C.7020005@sonymobile.com \
--to=tim.bird@sonymobile.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=robherring2@gmail.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.