linux-kernel - Re: [PATCH v2 2/3] ARM: qcom: Add coincell charger driver

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] [day] [month] [year] [list]

Date:	Wed, 15 Jul 2015 15:45:47 -0700
From:	Tim Bird <tim.bird@...ymobile.com>
To:	Stephen Boyd <sboyd@...eaurora.org>
CC:	"arnd@...db.de" <arnd@...db.de>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
	"robh+dt@...nel.org" <robh+dt@...nel.org>,
	"pawel.moll@....com" <pawel.moll@....com>,
	"mark.rutland@....com" <mark.rutland@....com>,
	"ijc+devicetree@...lion.org.uk" <ijc+devicetree@...lion.org.uk>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Andersson, Björn" 
	<Bjorn.Andersson@...ymobile.com>
Subject: Re: [PATCH v2 2/3] ARM: qcom: Add coincell charger driver



On 07/15/2015 12:44 PM, Stephen Boyd wrote:
> On 07/15/2015 12:08 PM, Tim Bird wrote:
>>
>> On 07/14/2015 06:11 PM, Stephen Boyd wrote:
>>> On 07/14/2015 04:26 PM, Tim Bird wrote:
>>>
>>>>    3 files changed, 166 insertions(+)
>>>>    create mode 100644 drivers/misc/qcom-coincell.c
>>>>
>>>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>>>> index 42c3852..0909869 100644
>>>> --- a/drivers/misc/Kconfig
>>>> +++ b/drivers/misc/Kconfig
>>>> @@ -271,6 +271,17 @@ config HP_ILO
>>>>    	  To compile this driver as a module, choose M here: the
>>>>    	  module will be called hpilo.
>>>>    
>>>> +config QCOM_COINCELL
>>>> +	tristate "Qualcomm coincell charger support"
>>>> +	depends on OF
>>> It looks like it would compile fine without OF, so can we drop this
>>> dependency? Or make it into
>>>
>>>    depends on MFD_SPMI_PMIC || COMPILE_TEST
>>>
>>> ?
>> I think I had CONFIG_OF off one time, and I spent the better
>> part of the afternoon trying to figure out why the driver wasn't
>> loading.  So it compiles but doesn't actually work.
>> But I think a dependency on MFD_SPMI_PMIC solves this issue.
>> So, OK on the second suggestion.
>>
>>>> +	select REGMAP
> 
> This config wouldn't be necessary either then because it would be 
> selected implicitly by the SPMI parent driver.

OK.  I seem to recall having problems with this with an earlier kernel
version, but it looks like the parent does indeed have a "select REMAP_SPMI"
now.  So I'll get rid of this here.

[...]
>>>> +	return rc;
>>>> +}
>>>> +
>>>> +static int qcom_coincell_probe(struct platform_device *pdev)
>>>> +{
>>>> +	struct device_node *node = pdev->dev.of_node;
>>>> +	struct qcom_coincell *chgr;
>>>> +	u32 rset, vset, enable;
>>>> +	int rc;
>>>> +
>>>> +	if (!node) {
>>>> +		dev_err(&pdev->dev, "%s: device node missing\n", __func__);
>>>> +		return -ENODEV;
>>>> +	}
>>> Does this happen?
>> Probably not any more.  The only way this device gets initialized now
>> is via OF operations.  This code was forward-ported from when this driver
>> also operated as a platform device.  In the current situation, I don't
>> know of a way for the kernel to get here if of_node is missing
>> (but I'm not an OF expert, and I didn't want to start using
>> a NULL of_node.)
>>
>> What does of_property_read...() do with a NULL node?
> 
> I'm pretty sure it returns success or nothing when the node is NULL.
> 
>>
>> I'm a little leery of taking this check out, but if you think it's
>> OK I'm fine doing it.
> 
> I'll fix any problems with the removal of the check :)

LOL. It's a deal! :-)

[...]
>>>> +	if (rc)
>>>> +		enable = 0;
>>>> +
>>>> +	rc = qcom_coincell_chgr_config(chgr, rset, vset, enable);
>>>> +
>>>> +	return rc;
> 
> This could be simplified to a return qcom_coincell_chrg_config() too.

OK

> Also, do we even need the chgr structure allocated anywhere besides on 
> the stack? It seems that it will be memory that's just lying around for 
> no use after probe.

Nope. And Agreed.  I'll change this.

Thanks for the help!
 -- 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/