linux-kernel - Re: [PATCH 1/3] ARM: dts: qcom: Add binding for the qcom coincell charger

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <55A6DE5C.7020005@sonymobile.com>
Date:	Wed, 15 Jul 2015 15:27:40 -0700
From:	Tim Bird <tim.bird@...ymobile.com>
To:	Rob Herring <robherring2@...il.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 07/15/2015 02:22 PM, Rob Herring wrote:
> 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.

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-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/