Re: [PATCH v3 02/10] spmi: Linux driver framework for SPMI
On Tue, Oct 29, 2013 at 09:52:15AM -0700, Stephen Boyd wrote:
On 10/28/13 11:12, Josh Cartwright wrote:
diff --git a/drivers/spmi/Kconfig b/drivers/spmi/Kconfig
new file mode 100644
index 000..a03835f
--- /dev/null
+++ b/drivers/spmi/Kconfig
@@ -0,0 +1,9 @@
+#
+# SPMI driver configuration
+#
+menuconfig SPMI
+ bool SPMI support
Can we do tristate?
I don't think there is any reason why we can't do tristate here. I do
foresee in the future, however, that we'll run into ordering/dependency
problems when we get the regulator drivers in place.
I suppose we'll wait until we get there to deal with those.
+ help
+ SPMI (System Power Management Interface) is a two-wire
+ serial interface between baseband and application processors
+ and Power Management Integrated Circuits (PMIC).
diff --git a/drivers/spmi/spmi.c b/drivers/spmi/spmi.c
new file mode 100644
index 000..0780bd4
--- /dev/null
+++ b/drivers/spmi/spmi.c
@@ -0,0 +1,491 @@
+/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+#include linux/kernel.h
+#include linux/errno.h
+#include linux/idr.h
+#include linux/slab.h
+#include linux/module.h
+#include linux/of.h
+#include linux/of_device.h
+#include linux/platform_device.h
+#include linux/spmi.h
+#include linux/module.h
+#include linux/pm_runtime.h
+
+#include dt-bindings/spmi/spmi.h
+
+static DEFINE_MUTEX(ctrl_idr_lock);
+static DEFINE_IDR(ctrl_idr);
+
+static void spmi_dev_release(struct device *dev)
+{
+ struct spmi_device *sdev = to_spmi_device(dev);
+ kfree(sdev);
+}
+
+static struct device_type spmi_dev_type = {
+ .release= spmi_dev_release,
const?
[..]
+static struct device_type spmi_ctrl_type = {
const?
Yep. Thanks.
[..]
+int spmi_register_read(struct spmi_device *sdev, u8 addr, u8 *buf)
+{
+ /* 5-bit register address */
+ if (addr 0x1F)
Perhaps 0x1f should be a #define.
Is 0x1F with the comment above it not clear enough?
+ return -EINVAL;
+
+ return spmi_read_cmd(sdev-ctrl, SPMI_CMD_READ, sdev-usid, addr, 0,
+buf);
+}
+EXPORT_SYMBOL_GPL(spmi_register_read);
+
[...]
+struct spmi_controller *spmi_controller_alloc(struct device *parent,
+ size_t size)
+{
+ struct spmi_controller *ctrl;
+ int id;
+
+ if (WARN_ON(!parent))
+ return NULL;
+
+ ctrl = kzalloc(sizeof(*ctrl) + size, GFP_KERNEL);
+ if (!ctrl)
+ return NULL;
+
+ device_initialize(ctrl-dev);
+ ctrl-dev.type = spmi_ctrl_type;
+ ctrl-dev.bus = spmi_bus_type;
+ ctrl-dev.parent = parent;
+ ctrl-dev.of_node = parent-of_node;
+ spmi_controller_set_drvdata(ctrl, ctrl[1]);
+
+ idr_preload(GFP_KERNEL);
+ mutex_lock(ctrl_idr_lock);
+ id = idr_alloc(ctrl_idr, ctrl, 0, 0, GFP_NOWAIT);
+ mutex_unlock(ctrl_idr_lock);
+ idr_preload_end();
This can just be:
mutex_lock(ctrl_idr_lock);
id = idr_alloc(ctrl_idr, ctrl, 0, 0, GFP_KERNEL);
mutex_unlock(ctrl_idr_lock);
Actually, I'm wondering if it's just easier to leverage the simpler
ida_simple_* APIs instead.
+
+ if (id 0) {
+ dev_err(parent,
+ unable to allocate SPMI controller identifier.\n);
+ spmi_controller_put(ctrl);
+ return NULL;
+ }
+
+ ctrl-nr = id;
+ dev_set_name(ctrl-dev, spmi-%d, id);
+
+ dev_dbg(ctrl-dev, allocated controller 0x%p id %d\n, ctrl, id);
+ return ctrl;
+}
+EXPORT_SYMBOL_GPL(spmi_controller_alloc);
+
+static int of_spmi_register_devices(struct spmi_controller *ctrl)
+{
+ struct device_node *node;
+ int err;
+
+ dev_dbg(ctrl-dev, of_spmi_register_devices\n);
Could be
dev_dbg(ctrl-dev, %s, __func__);
so that function renaming is transparent.
Some of these dev_dbg()'s (like this one) can probably be just be
removed, especially because we have an additional dev_dbg() right below
this one...
+
+ if (!ctrl-dev.of_node)
+ return -ENODEV;
+
+ dev_dbg(ctrl-dev, looping through children\n);
+
+ for_each_available_child_of_node(ctrl-dev.of_node, node) {
+ struct spmi_device *sdev;
+ u32 reg[2];
+
+ dev_dbg(ctrl-dev, adding child %s\n, node-full_name);
+
+ err = of_property_read_u32_array(node, reg, reg, 2);
+
Re: [PATCH v3 02/10] spmi: Linux driver framework for SPMI
On 10/30/13 12:37, Josh Cartwright wrote:
On Tue, Oct 29, 2013 at 09:52:15AM -0700, Stephen Boyd wrote:
On 10/28/13 11:12, Josh Cartwright wrote:
+int spmi_register_read(struct spmi_device *sdev, u8 addr, u8 *buf)
+{
+ /* 5-bit register address */
+ if (addr 0x1F)
Perhaps 0x1f should be a #define.
Is 0x1F with the comment above it not clear enough?
It triggered my 'magic number used twice' alarm. I'm ok with it either way.
+struct spmi_controller *spmi_controller_alloc(struct device *parent,
+ size_t size)
+{
+ struct spmi_controller *ctrl;
+ int id;
+
+ if (WARN_ON(!parent))
+ return NULL;
+
+ ctrl = kzalloc(sizeof(*ctrl) + size, GFP_KERNEL);
+ if (!ctrl)
+ return NULL;
+
+ device_initialize(ctrl-dev);
+ ctrl-dev.type = spmi_ctrl_type;
+ ctrl-dev.bus = spmi_bus_type;
+ ctrl-dev.parent = parent;
+ ctrl-dev.of_node = parent-of_node;
+ spmi_controller_set_drvdata(ctrl, ctrl[1]);
+
+ idr_preload(GFP_KERNEL);
+ mutex_lock(ctrl_idr_lock);
+ id = idr_alloc(ctrl_idr, ctrl, 0, 0, GFP_NOWAIT);
+ mutex_unlock(ctrl_idr_lock);
+ idr_preload_end();
This can just be:
mutex_lock(ctrl_idr_lock);
id = idr_alloc(ctrl_idr, ctrl, 0, 0, GFP_KERNEL);
mutex_unlock(ctrl_idr_lock);
Actually, I'm wondering if it's just easier to leverage the simpler
ida_simple_* APIs instead.
Yes that also works.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line unsubscribe linux-arm-msm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 02/10] spmi: Linux driver framework for SPMI
Couple of high-level comments on the in-kernel API.
On 10/28/2013 07:12 PM, Josh Cartwright wrote:
+#ifdef CONFIG_PM_SLEEP
+static int spmi_pm_suspend(struct device *dev)
+{
+ const struct dev_pm_ops *pm = dev-driver ? dev-driver-pm : NULL;
+
+ if (pm)
+ return pm_generic_suspend(dev);
pm_generic_suspend() checks both dev-driver and dev-driver-pm and returns
0 if either of them is NULL, so there should be no need to wrap the function.
+ else
+ return 0;
+}
+
+static int spmi_pm_resume(struct device *dev)
+{
+ const struct dev_pm_ops *pm = dev-driver ? dev-driver-pm : NULL;
+
+ if (pm)
+ return pm_generic_resume(dev);
Same here
+ else
+ return 0;
+}
+#else
+#define spmi_pm_suspend NULL
+#define spmi_pm_resume NULL
+#endif
[...]
+/**
+ * spmi_controller_remove: Controller tear-down.
+ * @ctrl: controller to be removed.
+ *
+ * Controller added with the above API is torn down using this API.
+ */
+int spmi_controller_remove(struct spmi_controller *ctrl)
The return type should be void. The function can't fail and nobody is going
to check the return value anyway.
+{
+ int dummy;
+
+ if (!ctrl)
+ return -EINVAL;
+
+ dummy = device_for_each_child(ctrl-dev, NULL,
+ spmi_ctrl_remove_device);
+ device_unregister(ctrl-dev);
Should be device_del(). device_unregister() will do both device_del() and
put_device(). But usually you'd want to do something in between like release
resources used by the controller.
+ return 0;
+}
+EXPORT_SYMBOL_GPL(spmi_controller_remove);
+
[...]
+/**
+ * spmi_controller_alloc: Allocate a new SPMI controller
+ * @ctrl: associated controller
+ *
+ * Caller is responsible for either calling spmi_device_add() to add the
+ * newly allocated controller, or calling spmi_device_put() to discard it.
+ */
+struct spmi_device *spmi_device_alloc(struct spmi_controller *ctrl);
+
+static inline void spmi_device_put(struct spmi_device *sdev)
For symmetry reasons it might make sense to call this spmi_device_free().
+{
+ if (sdev)
+ put_device(sdev-dev);
+}
[...]
+#define to_spmi_controller(d) container_of(d, struct spmi_controller, dev)
Should be a inline function for better type safety.
[...]
+static inline void spmi_controller_put(struct spmi_controller *ctrl)
For symmetry reasons it might make sense to call this spmi_controller_free().
+{
+ if (ctrl)
+ put_device(ctrl-dev);
+}
+
[]
+struct spmi_driver {
+ struct device_driver driver;
+ int (*probe)(struct spmi_device *sdev);
+ int (*remove)(struct spmi_device *sdev);
The type of the remove function should be found. The Linux device model
doesn't really allow for device removal to fail.
+ void(*shutdown)(struct spmi_device *sdev);
+ int (*suspend)(struct spmi_device *sdev, pm_message_t pmesg);
+ int (*resume)(struct spmi_device *sdev);
The framework seems to support dev_pm_ops just fine, there should be no need
for legacy suspend/resume callbacks.
+};
+#define to_spmi_driver(d) container_of(d, struct spmi_driver, driver)
Inline function here as well
[...]
--
To unsubscribe from this list: send the line unsubscribe linux-arm-msm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 02/10] spmi: Linux driver framework for SPMI
Hey Lars-
Thanks for the feedback. CC'ing Ivan, since he had the same feedback
regarding the PM callbacks.
On Tue, Oct 29, 2013 at 04:21:28PM +0100, Lars-Peter Clausen wrote:
Couple of high-level comments on the in-kernel API.
On 10/28/2013 07:12 PM, Josh Cartwright wrote:
+#ifdef CONFIG_PM_SLEEP
+static int spmi_pm_suspend(struct device *dev)
+{
+ const struct dev_pm_ops *pm = dev-driver ? dev-driver-pm : NULL;
+
+ if (pm)
+ return pm_generic_suspend(dev);
pm_generic_suspend() checks both dev-driver and dev-driver-pm and returns
0 if either of them is NULL, so there should be no need to wrap the function.
+ else
+ return 0;
+}
+
+static int spmi_pm_resume(struct device *dev)
+{
+ const struct dev_pm_ops *pm = dev-driver ? dev-driver-pm : NULL;
+
+ if (pm)
+ return pm_generic_resume(dev);
Same here
Sounds good. I'll drop these.
+/**
+ * spmi_controller_remove: Controller tear-down.
+ * @ctrl: controller to be removed.
+ *
+ * Controller added with the above API is torn down using this API.
+ */
+int spmi_controller_remove(struct spmi_controller *ctrl)
The return type should be void. The function can't fail and nobody is going
to check the return value anyway.
Alright.
+{
+ int dummy;
+
+ if (!ctrl)
+ return -EINVAL;
+
+ dummy = device_for_each_child(ctrl-dev, NULL,
+ spmi_ctrl_remove_device);
+ device_unregister(ctrl-dev);
Should be device_del(). device_unregister() will do both device_del() and
put_device(). But usually you'd want to do something in between like release
resources used by the controller.
I'm not sure I understand your suggestion here. If put_device() isn't
called here, wouldn't we be leaking the controller? What resources
would I want to be releasing here that aren't released as part of the
controller's release() function?
+ return 0;
+}
+EXPORT_SYMBOL_GPL(spmi_controller_remove);
+
[...]
+/**
+ * spmi_controller_alloc: Allocate a new SPMI controller
+ * @ctrl: associated controller
+ *
+ * Caller is responsible for either calling spmi_device_add() to add the
+ * newly allocated controller, or calling spmi_device_put() to discard it.
+ */
+struct spmi_device *spmi_device_alloc(struct spmi_controller *ctrl);
+
+static inline void spmi_device_put(struct spmi_device *sdev)
For symmetry reasons it might make sense to call this spmi_device_free().
Except that it doesn't necessarily free() the underlying device, so I
find that more confusing.
+{
+ if (sdev)
+ put_device(sdev-dev);
+}
[...]
+#define to_spmi_controller(d) container_of(d, struct spmi_controller, dev)
Should be a inline function for better type safety.
Sounds good. Will change the to_spmi_*() macros.
[...]
+static inline void spmi_controller_put(struct spmi_controller *ctrl)
For symmetry reasons it might make sense to call this spmi_controller_free().
+{
+ if (ctrl)
+ put_device(ctrl-dev);
+}
+
[]
+struct spmi_driver {
+ struct device_driver driver;
+ int (*probe)(struct spmi_device *sdev);
+ int (*remove)(struct spmi_device *sdev);
The type of the remove function should be found. The Linux device model
doesn't really allow for device removal to fail.
+ void(*shutdown)(struct spmi_device *sdev);
+ int (*suspend)(struct spmi_device *sdev, pm_message_t pmesg);
+ int (*resume)(struct spmi_device *sdev);
The framework seems to support dev_pm_ops just fine, there should be no need
for legacy suspend/resume callbacks.
Yep. Will drop.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line unsubscribe linux-arm-msm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 02/10] spmi: Linux driver framework for SPMI
On Tue, Oct 29, 2013 at 05:02:03PM +0200, Ivan T. Ivanov wrote:
On Mon, 2013-10-28 at 13:12 -0500, Josh Cartwright wrote:
From: Kenneth Heitke khei...@codeaurora.org
System Power Management Interface (SPMI) is a specification
developed by the MIPI (Mobile Industry Process Interface) Alliance
optimized for the real time control of Power Management ICs (PMIC).
SPMI is a two-wire serial interface that supports up to 4 master
devices and up to 16 logical slaves.
The framework supports message APIs, multiple busses (1 controller
per bus) and multiple clients/slave devices per controller.
Signed-off-by: Kenneth Heitke khei...@codeaurora.org
Signed-off-by: Michael Bohan mbo...@codeaurora.org
Signed-off-by: Josh Cartwright jo...@codeaurora.org
[..]
+static int spmi_drv_probe(struct device *dev)
+{
+ const struct spmi_driver *sdrv = to_spmi_driver(dev-driver);
+ int err = 0;
+
+ if (sdrv-probe)
+ err = sdrv-probe(to_spmi_device(dev));
+
+ return err;
+}
+
+static int spmi_drv_remove(struct device *dev)
+{
+ const struct spmi_driver *sdrv = to_spmi_driver(dev-driver);
+ int err = 0;
+
+ if (sdrv-remove)
+ err = sdrv-remove(to_spmi_device(dev));
+
+ return err;
+}
+
+static void spmi_drv_shutdown(struct device *dev)
+{
+ const struct spmi_driver *sdrv = to_spmi_driver(dev-driver);
+
+ if (sdrv-shutdown)
If driver for device is not loaded this will cause kernel NULL
pointer dereference.
Indeed. I'll fix this.
+static int of_spmi_register_devices(struct spmi_controller *ctrl)
+{
+ struct device_node *node;
+ int err;
+
+ dev_dbg(ctrl-dev, of_spmi_register_devices\n);
+
+ if (!ctrl-dev.of_node)
+ return -ENODEV;
+
+ dev_dbg(ctrl-dev, looping through children\n);
+
+ for_each_available_child_of_node(ctrl-dev.of_node, node) {
+ struct spmi_device *sdev;
+ u32 reg[2];
+
+ dev_dbg(ctrl-dev, adding child %s\n, node-full_name);
+
+ err = of_property_read_u32_array(node, reg, reg, 2);
+ if (err) {
+ dev_err(ctrl-dev,
+ node %s does not have 'reg' property\n,
+ node-full_name);
+ continue;
Shouldn't this be a fatal error?
Fatal in what way? It is fatal in the sense that this particular child
node is skipped, but other children can still be enumerated. Are you
suggesting that we bail completely when we hit a wrongly-described
child?
+ }
+
+ if (reg[1] != SPMI_USID) {
+ dev_err(ctrl-dev,
+ node %s contains unsupported 'reg' entry\n,
+ node-full_name);
+ continue;
+ }
+
+ if (reg[0] 0xF) {
+ dev_err(ctrl-dev,
+ invalid usid on node %s\n,
+ node-full_name);
+ continue;
+ }
+
+ dev_dbg(ctrl-dev, read usid %02x\n, reg[0]);
+
+ sdev = spmi_device_alloc(ctrl);
+ if (!sdev)
+ continue;
+
+ sdev-dev.of_node = node;
+ sdev-usid = (u8) reg[0];
+
+ err = spmi_device_add(sdev);
+ if (err) {
+ dev_err(sdev-dev,
+ failure adding device. status %d\n, err);
+ spmi_device_put(sdev);
+ continue;
+ }
+ }
+
+ return 0;
+}
+
+int spmi_controller_add(struct spmi_controller *ctrl)
+{
+ int ret;
+
+ /* Can't register until after driver model init */
+ if (WARN_ON(!spmi_bus_type.p))
+ return -EAGAIN;
+
+ ret = device_add(ctrl-dev);
+ if (ret)
+ return ret;
+
+ if (IS_ENABLED(CONFIG_OF))
+ of_spmi_register_devices(ctrl);
Check for error code here?
And do what with it? Maybe instead, I should make
of_spmi_register_devices() return void.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line unsubscribe linux-arm-msm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 02/10] spmi: Linux driver framework for SPMI
On 10/29/13 08:56, Josh Cartwright wrote: +#define to_spmi_controller(d) container_of(d, struct spmi_controller, dev) Should be a inline function for better type safety. Sounds good. Will change the to_spmi_*() macros. I was under the impression that container_of() already does type checking. At least it will ensure that typeof(d) == typeof(dev) in the above example which is about as good as it can get. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation -- To unsubscribe from this list: send the line unsubscribe linux-arm-msm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 02/10] spmi: Linux driver framework for SPMI
On 10/28/13 11:12, Josh Cartwright wrote:
diff --git a/drivers/spmi/Kconfig b/drivers/spmi/Kconfig
new file mode 100644
index 000..a03835f
--- /dev/null
+++ b/drivers/spmi/Kconfig
@@ -0,0 +1,9 @@
+#
+# SPMI driver configuration
+#
+menuconfig SPMI
+ bool SPMI support
Can we do tristate?
+ help
+ SPMI (System Power Management Interface) is a two-wire
+ serial interface between baseband and application processors
+ and Power Management Integrated Circuits (PMIC).
diff --git a/drivers/spmi/spmi.c b/drivers/spmi/spmi.c
new file mode 100644
index 000..0780bd4
--- /dev/null
+++ b/drivers/spmi/spmi.c
@@ -0,0 +1,491 @@
+/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+#include linux/kernel.h
+#include linux/errno.h
+#include linux/idr.h
+#include linux/slab.h
+#include linux/module.h
+#include linux/of.h
+#include linux/of_device.h
+#include linux/platform_device.h
+#include linux/spmi.h
+#include linux/module.h
+#include linux/pm_runtime.h
+
+#include dt-bindings/spmi/spmi.h
+
+static DEFINE_MUTEX(ctrl_idr_lock);
+static DEFINE_IDR(ctrl_idr);
+
+static void spmi_dev_release(struct device *dev)
+{
+ struct spmi_device *sdev = to_spmi_device(dev);
+ kfree(sdev);
+}
+
+static struct device_type spmi_dev_type = {
+ .release= spmi_dev_release,
const?
+};
+
+static void spmi_ctrl_release(struct device *dev)
+{
+ struct spmi_controller *ctrl = to_spmi_controller(dev);
+ mutex_lock(ctrl_idr_lock);
+ idr_remove(ctrl_idr, ctrl-nr);
+ mutex_unlock(ctrl_idr_lock);
+ kfree(ctrl);
+}
+
+static struct device_type spmi_ctrl_type = {
const?
+ .release= spmi_ctrl_release,
+};
+
[...]
+
+/*
+ * register read/write: 5-bit address, 1 byte of data
+ * extended register read/write: 8-bit address, up to 16 bytes of data
+ * extended register read/write long: 16-bit address, up to 8 bytes of data
+ */
+
+int spmi_register_read(struct spmi_device *sdev, u8 addr, u8 *buf)
+{
+ /* 5-bit register address */
+ if (addr 0x1F)
Perhaps 0x1f should be a #define.
+ return -EINVAL;
+
+ return spmi_read_cmd(sdev-ctrl, SPMI_CMD_READ, sdev-usid, addr, 0,
+ buf);
+}
+EXPORT_SYMBOL_GPL(spmi_register_read);
+
[...]
+struct spmi_controller *spmi_controller_alloc(struct device *parent,
+ size_t size)
+{
+ struct spmi_controller *ctrl;
+ int id;
+
+ if (WARN_ON(!parent))
+ return NULL;
+
+ ctrl = kzalloc(sizeof(*ctrl) + size, GFP_KERNEL);
+ if (!ctrl)
+ return NULL;
+
+ device_initialize(ctrl-dev);
+ ctrl-dev.type = spmi_ctrl_type;
+ ctrl-dev.bus = spmi_bus_type;
+ ctrl-dev.parent = parent;
+ ctrl-dev.of_node = parent-of_node;
+ spmi_controller_set_drvdata(ctrl, ctrl[1]);
+
+ idr_preload(GFP_KERNEL);
+ mutex_lock(ctrl_idr_lock);
+ id = idr_alloc(ctrl_idr, ctrl, 0, 0, GFP_NOWAIT);
+ mutex_unlock(ctrl_idr_lock);
+ idr_preload_end();
This can just be:
mutex_lock(ctrl_idr_lock);
id = idr_alloc(ctrl_idr, ctrl, 0, 0, GFP_KERNEL);
mutex_unlock(ctrl_idr_lock);
+
+ if (id 0) {
+ dev_err(parent,
+ unable to allocate SPMI controller identifier.\n);
+ spmi_controller_put(ctrl);
+ return NULL;
+ }
+
+ ctrl-nr = id;
+ dev_set_name(ctrl-dev, spmi-%d, id);
+
+ dev_dbg(ctrl-dev, allocated controller 0x%p id %d\n, ctrl, id);
+ return ctrl;
+}
+EXPORT_SYMBOL_GPL(spmi_controller_alloc);
+
+static int of_spmi_register_devices(struct spmi_controller *ctrl)
+{
+ struct device_node *node;
+ int err;
+
+ dev_dbg(ctrl-dev, of_spmi_register_devices\n);
Could be
dev_dbg(ctrl-dev, %s, __func__);
so that function renaming is transparent.
+
+ if (!ctrl-dev.of_node)
+ return -ENODEV;
+
+ dev_dbg(ctrl-dev, looping through children\n);
+
+ for_each_available_child_of_node(ctrl-dev.of_node, node) {
+ struct spmi_device *sdev;
+ u32 reg[2];
+
+ dev_dbg(ctrl-dev, adding child %s\n, node-full_name);
+
+ err = of_property_read_u32_array(node, reg, reg, 2);
+ if (err) {
+ dev_err(ctrl-dev,
+ node %s does not have 'reg' property\n,
+
Re: [PATCH v3 02/10] spmi: Linux driver framework for SPMI
On 10/29/2013 05:30 PM, Stephen Boyd wrote: On 10/29/13 08:56, Josh Cartwright wrote: +#define to_spmi_controller(d) container_of(d, struct spmi_controller, dev) Should be a inline function for better type safety. Sounds good. Will change the to_spmi_*() macros. I was under the impression that container_of() already does type checking. At least it will ensure that typeof(d) == typeof(dev) in the above example which is about as good as it can get. Well you'll get a warning, but the quality of the warning message is much better when an inline function is used. warning: initialization from incompatible pointer type vs. warning: Passing argument 1 of to_smpi_controller() from incompatible pointer type. Expected struct device * got struct driver * - Lars -- To unsubscribe from this list: send the line unsubscribe linux-arm-msm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 02/10] spmi: Linux driver framework for SPMI
On 10/29/2013 04:56 PM, Josh Cartwright wrote:
+{
+ int dummy;
+
+ if (!ctrl)
+ return -EINVAL;
+
+ dummy = device_for_each_child(ctrl-dev, NULL,
+ spmi_ctrl_remove_device);
+ device_unregister(ctrl-dev);
Should be device_del(). device_unregister() will do both device_del() and
put_device(). But usually you'd want to do something in between like release
resources used by the controller.
I'm not sure I understand your suggestion here. If put_device() isn't
called here, wouldn't we be leaking the controller? What resources
would I want to be releasing here that aren't released as part of the
controller's release() function?
Resources used by the driver implementing the controller. Usually the driver
state struct will be allocated by spmi_controller_alloc() as well. So if you
store resources in that struct, e.g. a clk you first want to unregister the
spmi controller to make sure that the resources are no longer accessed, then
free the resources and finally drop the reference to the controller so the
memory can be freed. E.g.
static int foobar_remove(struct platform_device *pdev)
{
struct spmi_controller *ctrl = platform_get_drvdata(pdev);
struct foobar *foobar = spmi_controller_get_drvdata(ctrl);
spmi_controller_remove(ctrl);
free_irq(foobar-irq)
clk_put(foobar-clk);
...
spmi_controller_put(ctrl);
return 0;
}
+ return 0;
+}
+EXPORT_SYMBOL_GPL(spmi_controller_remove);
+
[...]
+/**
+ * spmi_controller_alloc: Allocate a new SPMI controller
+ * @ctrl: associated controller
+ *
+ * Caller is responsible for either calling spmi_device_add() to add the
+ * newly allocated controller, or calling spmi_device_put() to discard it.
+ */
+struct spmi_device *spmi_device_alloc(struct spmi_controller *ctrl);
+
+static inline void spmi_device_put(struct spmi_device *sdev)
For symmetry reasons it might make sense to call this spmi_device_free().
Except that it doesn't necessarily free() the underlying device, so I
find that more confusing.
Well, for all the driver cares the device has been freed.
--
To unsubscribe from this list: send the line unsubscribe linux-arm-msm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 02/10] spmi: Linux driver framework for SPMI
On Mon, Oct 28, 2013 at 01:12:35PM -0500, Josh Cartwright wrote:
From: Kenneth Heitke khei...@codeaurora.org
System Power Management Interface (SPMI) is a specification
developed by the MIPI (Mobile Industry Process Interface) Alliance
optimized for the real time control of Power Management ICs (PMIC).
SPMI is a two-wire serial interface that supports up to 4 master
devices and up to 16 logical slaves.
The framework supports message APIs, multiple busses (1 controller
per bus) and multiple clients/slave devices per controller.
I haven't read this in depth, but... if you want to support runtime PM
for your spmi devices, then I suggest that you also include the fragments
to setup runtime PM in the bus-level probe handler and clean it up in
the bus-level remove handler.
What that means is doing what PCI, AMBA and similar buses do:
pm_runtime_get_noresume(dev);
pm_runtime_set_active(dev);
pm_runtime_enable(dev);
ret = driver-probe(dev);
if (ret != 0) {
pm_runtime_disable(dev);
pm_runtime_set_suspended(dev);
pm_runtime_put_noidle(dev);
}
and:
pm_runtime_get_sync(dev);
ret = driver-remove(dev);
pm_runtime_put_noidle(dev);
pm_runtime_disable(dev);
pm_runtime_set_suspended(dev);
pm_runtime_put_noidle(dev);
What this means is that your devices get runtime enabled by default,
but they have to do a pm_runtime_put() or similar in their probe
function to benefit from it and a balancing pm_runtime_get() in
their remove method.
The set_active() call above may need to be conditional upon whether
the device really is in a powered up state at that point or not.
Others have made comments on various other issues so I won't repeat
those points here.
--
To unsubscribe from this list: send the line unsubscribe linux-arm-msm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
