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

[Tim Bird <tim.bird@sonymobile.com>]

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