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 devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
