On 07/15/2015 12:44 PM, Stephen Boyd wrote:

On 07/15/2015 12:08 PM, Tim Bird wrote:

quoted

On 07/14/2015 06:11 PM, Stephen Boyd wrote:

quoted

On 07/14/2015 04:26 PM, Tim Bird wrote:

quoted

   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.

quoted

quoted

+	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.

[...]

quoted

quoted

quoted

+	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.

quoted

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! :-)

[...]

quoted

quoted

quoted

+	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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html