Re: [PATCH] net: add Qualcomm IPC router
On Tue, Dec 15, 2015 at 10:01:14PM +0100, David Miller wrote: > From: Bjorn Andersson > Date: Fri, 11 Dec 2015 12:41:59 -0800 > > > +static unsigned int qrtr_local_nid = 1; > > +module_param_named(node_id, qrtr_local_nid, uint, S_IRUGO); > > +MODULE_PARM_DESC(idVendor, "Local Node Identifier"); > > Module parameters suck. > > Allow the user to choose this dynamically. You have roughtly two choices. > > 1) Subvert the 'protocol' field passed to ->create() and use that, it is >being ignored otherwise. > > 2) Put it into the socket address for bind(). So each socket can have its own node id? That doesn't seem right. The way these node ids are assigned is by a system designer (in this case Qualcomm). The ARM, Linux CPU is always node 1, the audio DSP is always node 5, etc. Anyone with the knowhow could reassign these numbers, but there's no reason to have them be dynamic during runtime. Additionally, allowing dynamic assignment would require code to prevent id duplication for known remote nodes, as well as to deal with cases in which remote node discovery happens after local sockets have acquired that node's id. Maybe the first socket created needs CAP_NET_ADMIN, and uses the 'protocol' field to set the node id? Ugh. Gross. We could hardcode the value in kconfig, but that seems like a worse solution than a module parameter. I'm open to further suggestions. -Courtney -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net: add Qualcomm IPC router
On Fri, Dec 11, 2015 at 09:41:59PM +0100, Bjorn Andersson wrote: > From: Courtney Cavin > > Add an implementation of Qualcomm's IPC router protocol, used to > communicate with service providing remote processors. > > Signed-off-by: Courtney Cavin > --- [...] > +static int qrtr_sendmsg(struct socket *sock, struct msghdr *msg, size_t len) > +{ > + DECLARE_SOCKADDR(struct sockaddr_qrtr *, addr, msg->msg_name); > + int (*enqueue_fn)(struct qrtr_node *, struct sk_buff *); > + struct qrtr_sock *ipc = qrtr_sk(sock->sk); > + struct sock *sk = sock->sk; > + struct qrtr_node *node; > + struct qrtr_hdr *hdr; > + struct sk_buff *skb; > + size_t plen; > + int rc; > + > + if (msg->msg_flags & ~(MSG_DONTWAIT)) > + return -EINVAL; > + > + if (len > 65535) > + return -EMSGSIZE; > + > + lock_sock(sk); > + > + if (addr) { > + if (msg->msg_namelen < sizeof(*addr)) { > + release_sock(sk); > + return -EINVAL; > + } > + > + if (addr->sq_family != AF_QIPCRTR) { > + release_sock(sk); > + return -EINVAL; > + } > + > + rc = qrtr_autobind(sock); > + if (rc) { > + release_sock(sk); > + return rc; > + } > + } else if (sk->sk_state == TCP_ESTABLISHED) { > + addr = >peer; > + } else { > + release_sock(sk); > + return -ENOTCONN; > + } > + > + node = NULL; > + if (addr->sq_node == QRTR_NODE_BCAST) { > + enqueue_fn = qrtr_bcast_enqueue; > + } else if (addr->sq_node == 0 || addr->sq_node == ipc->us.sq_node) { 'addr->sq_node == 0' should be removed from this if-condition. Zero is a valid node id. Clients needing the local address can use getsockname(2). > + enqueue_fn = qrtr_local_enqueue; > + } else { > + enqueue_fn = qrtr_node_enqueue; > + node = qrtr_node_lookup(addr->sq_node); > + if (!node) { > + release_sock(sk); > + return -ECONNRESET; > + } > + } -Courtney -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net: add Qualcomm IPC router
On Fri, Dec 11, 2015 at 09:41:59PM +0100, Bjorn Andersson wrote: > From: Courtney Cavin <courtney.ca...@sonymobile.com> > > Add an implementation of Qualcomm's IPC router protocol, used to > communicate with service providing remote processors. > > Signed-off-by: Courtney Cavin <courtney.ca...@sonymobile.com> > --- [...] > +static int qrtr_sendmsg(struct socket *sock, struct msghdr *msg, size_t len) > +{ > + DECLARE_SOCKADDR(struct sockaddr_qrtr *, addr, msg->msg_name); > + int (*enqueue_fn)(struct qrtr_node *, struct sk_buff *); > + struct qrtr_sock *ipc = qrtr_sk(sock->sk); > + struct sock *sk = sock->sk; > + struct qrtr_node *node; > + struct qrtr_hdr *hdr; > + struct sk_buff *skb; > + size_t plen; > + int rc; > + > + if (msg->msg_flags & ~(MSG_DONTWAIT)) > + return -EINVAL; > + > + if (len > 65535) > + return -EMSGSIZE; > + > + lock_sock(sk); > + > + if (addr) { > + if (msg->msg_namelen < sizeof(*addr)) { > + release_sock(sk); > + return -EINVAL; > + } > + > + if (addr->sq_family != AF_QIPCRTR) { > + release_sock(sk); > + return -EINVAL; > + } > + > + rc = qrtr_autobind(sock); > + if (rc) { > + release_sock(sk); > + return rc; > + } > + } else if (sk->sk_state == TCP_ESTABLISHED) { > + addr = >peer; > + } else { > + release_sock(sk); > + return -ENOTCONN; > + } > + > + node = NULL; > + if (addr->sq_node == QRTR_NODE_BCAST) { > + enqueue_fn = qrtr_bcast_enqueue; > + } else if (addr->sq_node == 0 || addr->sq_node == ipc->us.sq_node) { 'addr->sq_node == 0' should be removed from this if-condition. Zero is a valid node id. Clients needing the local address can use getsockname(2). > + enqueue_fn = qrtr_local_enqueue; > + } else { > + enqueue_fn = qrtr_node_enqueue; > + node = qrtr_node_lookup(addr->sq_node); > + if (!node) { > + release_sock(sk); > + return -ECONNRESET; > + } > + } -Courtney -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] net: add Qualcomm IPC router
On Tue, Dec 15, 2015 at 10:01:14PM +0100, David Miller wrote: > From: Bjorn Andersson> Date: Fri, 11 Dec 2015 12:41:59 -0800 > > > +static unsigned int qrtr_local_nid = 1; > > +module_param_named(node_id, qrtr_local_nid, uint, S_IRUGO); > > +MODULE_PARM_DESC(idVendor, "Local Node Identifier"); > > Module parameters suck. > > Allow the user to choose this dynamically. You have roughtly two choices. > > 1) Subvert the 'protocol' field passed to ->create() and use that, it is >being ignored otherwise. > > 2) Put it into the socket address for bind(). So each socket can have its own node id? That doesn't seem right. The way these node ids are assigned is by a system designer (in this case Qualcomm). The ARM, Linux CPU is always node 1, the audio DSP is always node 5, etc. Anyone with the knowhow could reassign these numbers, but there's no reason to have them be dynamic during runtime. Additionally, allowing dynamic assignment would require code to prevent id duplication for known remote nodes, as well as to deal with cases in which remote node discovery happens after local sockets have acquired that node's id. Maybe the first socket created needs CAP_NET_ADMIN, and uses the 'protocol' field to set the node id? Ugh. Gross. We could hardcode the value in kconfig, but that seems like a worse solution than a module parameter. I'm open to further suggestions. -Courtney -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: qcom-spmi-pmic: Add support for more chips versions
On Fri, Nov 07, 2014 at 04:40:52PM +0100, Ivan T. Ivanov wrote: > On Fri, 2014-11-07 at 17:33 +0200, Ivan T. Ivanov wrote: > > On Thu, 2014-11-06 at 08:55 -0800, Bjorn Andersson wrote: > > > On Wed, Nov 5, 2014 at 11:54 PM, Ivan T. Ivanov > > > wrote: > > > > On Wed, 2014-11-05 at 17:36 -0800, Bjorn Andersson wrote: [...] > > > > > Could you be a little bit more elaborate on what you're trying to do > > > > > and which child devices that might be? > > > > > > > > For example ADC drivers are required temperature compensation based > > > > on PMIC variant and chip manufacturer. > > > > > > > > > > I see, is that compensation of any practical value? Or is the > > > compensation of academic proportions? > > > > It depends of what you mean by academic :-). Attached file have test > > application which dump difference between non compensated and compensated > > values for different temperature, manufacture and input value. > > [...] > Forgot to add. PMIC subtype and version are used also in charger and BMS > drivers to workaround hardware issues. All of the blocks on the PM8x41 series have their own version numbers. There's no need to look at the chip revision. In fact, the SMBB (charger) documentation (80-NA555-12) specifically refers to the SMBB_MISC block revision registers as the method for determining the hardware version. The "qpnp-charger" SMBB driver in the CAF 3.4 & 3.10 trees utilizes block specific revision numbers, as do the "qpnp-bms" BMS driver, the "qpnp-adc-current" IADC driver, and the "qpnp-adc-voltage" VADC driver. The revision of the PMIC itself should be completely irrelevant to any of the software interfacing with it. -Courtney -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mfd: qcom-spmi-pmic: Add support for more chips versions
On Fri, Nov 07, 2014 at 04:40:52PM +0100, Ivan T. Ivanov wrote: On Fri, 2014-11-07 at 17:33 +0200, Ivan T. Ivanov wrote: On Thu, 2014-11-06 at 08:55 -0800, Bjorn Andersson wrote: On Wed, Nov 5, 2014 at 11:54 PM, Ivan T. Ivanov iiva...@mm-sol.com wrote: On Wed, 2014-11-05 at 17:36 -0800, Bjorn Andersson wrote: [...] Could you be a little bit more elaborate on what you're trying to do and which child devices that might be? For example ADC drivers are required temperature compensation based on PMIC variant and chip manufacturer. I see, is that compensation of any practical value? Or is the compensation of academic proportions? It depends of what you mean by academic :-). Attached file have test application which dump difference between non compensated and compensated values for different temperature, manufacture and input value. [...] Forgot to add. PMIC subtype and version are used also in charger and BMS drivers to workaround hardware issues. All of the blocks on the PM8x41 series have their own version numbers. There's no need to look at the chip revision. In fact, the SMBB (charger) documentation (80-NA555-12) specifically refers to the SMBB_MISC block revision registers as the method for determining the hardware version. The qpnp-charger SMBB driver in the CAF 3.4 3.10 trees utilizes block specific revision numbers, as do the qpnp-bms BMS driver, the qpnp-adc-current IADC driver, and the qpnp-adc-voltage VADC driver. The revision of the PMIC itself should be completely irrelevant to any of the software interfacing with it. -Courtney -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] hwspinlock/msm: Add support for Qualcomm MSM HW Mutex block
On Sat, Aug 30, 2014 at 02:14:07AM +0200, Stephen Boyd wrote: > On 08/29/14 16:41, Courtney Cavin wrote: > > On Sat, Aug 30, 2014 at 01:14:23AM +0200, Bjorn Andersson wrote: > >> From: Kumar Gala > >> > >> Add driver for Qualcomm MSM Hardware Mutex block that exists on > >> newer Qualcomm SoCs. > >> > >> Cc: Jeffrey Hugo > >> Cc: Eric Holmberg > >> Cc: Courtney Cavin > >> Signed-off-by: Kumar Gala > >> [bjorn: added pm_runtime calls, from Courtney, > >>added sfpb-mutex compatible, > >>updated DT binding documentation formatting] > >> Signed-off-by: Bjorn Andersson > >> --- > > [...] > >> diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig > >> index 3612cb5..2cd39e2 100644 > >> --- a/drivers/hwspinlock/Kconfig > >> +++ b/drivers/hwspinlock/Kconfig > >> @@ -8,6 +8,17 @@ config HWSPINLOCK > >> > >> menu "Hardware Spinlock drivers" > >> > >> +config HWSPINLOCK_MSM > >> + tristate "MSM Hardware Spinlock device" > >> + depends on ARCH_QCOM > > This should also depend on OF, as it won't compile or work without it. > > Doesn't ARCH_QCOM imply OF? ARCH_MULTIPLATFORM has a select USE_OF. Hrm. Apparently. -Courtney -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] hwspinlock/msm: Add support for Qualcomm MSM HW Mutex block
On Sat, Aug 30, 2014 at 01:14:23AM +0200, Bjorn Andersson wrote: > From: Kumar Gala > > Add driver for Qualcomm MSM Hardware Mutex block that exists on > newer Qualcomm SoCs. > > Cc: Jeffrey Hugo > Cc: Eric Holmberg > Cc: Courtney Cavin > Signed-off-by: Kumar Gala > [bjorn: added pm_runtime calls, from Courtney, > added sfpb-mutex compatible, > updated DT binding documentation formatting] > Signed-off-by: Bjorn Andersson > --- [...] > diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig > index 3612cb5..2cd39e2 100644 > --- a/drivers/hwspinlock/Kconfig > +++ b/drivers/hwspinlock/Kconfig > @@ -8,6 +8,17 @@ config HWSPINLOCK > > menu "Hardware Spinlock drivers" > > +config HWSPINLOCK_MSM > + tristate "MSM Hardware Spinlock device" > + depends on ARCH_QCOM This should also depend on OF, as it won't compile or work without it. > + select HWSPINLOCK > + help > + Say y here to support the MSM Hardware Mutex functionality, which > + provides a synchronisation mechanism for the various processors on > + the SoC. > + > + If unsure, say N. > + > config HWSPINLOCK_OMAP > tristate "OMAP Hardware Spinlock device" > depends on ARCH_OMAP4 || SOC_OMAP5 || SOC_DRA7XX || SOC_AM33XX || > SOC_AM43XX [...] > +static const struct of_device_id msm_hwspinlock_of_match[] = { > + { .compatible = "qcom,sfpb-mutex", .data = (void *)0x4 }, > + { .compatible = "qcom,tcsr-mutex", .data = (void *)0x80 }, > + { }, > +}; MODULE_DEVICE_TABLE(of, msm_hwspinlock_of_match); ? [...] > +static struct platform_driver msm_hwspinlock_driver = { > + .probe = msm_hwspinlock_probe, > + .remove = msm_hwspinlock_remove, > + .driver = { > + .name = "msm_hwspinlock", > + .owner = THIS_MODULE, No need, as: #define platform_driver_register(drv) \ __platform_driver_register(drv, THIS_MODULE) extern int __platform_driver_register(struct platform_driver *, struct module *); > + .of_match_table = msm_hwspinlock_of_match, > + }, > +}; Otherwise, looks fine. -Courtney -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] hwspinlock/msm: Add support for Qualcomm MSM HW Mutex block
On Sat, Aug 30, 2014 at 01:14:23AM +0200, Bjorn Andersson wrote: From: Kumar Gala ga...@codeaurora.org Add driver for Qualcomm MSM Hardware Mutex block that exists on newer Qualcomm SoCs. Cc: Jeffrey Hugo jh...@codeaurora.org Cc: Eric Holmberg eholm...@codeaurora.org Cc: Courtney Cavin courtney.ca...@sonymobile.com Signed-off-by: Kumar Gala ga...@codeaurora.org [bjorn: added pm_runtime calls, from Courtney, added sfpb-mutex compatible, updated DT binding documentation formatting] Signed-off-by: Bjorn Andersson bjorn.anders...@sonymobile.com --- [...] diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig index 3612cb5..2cd39e2 100644 --- a/drivers/hwspinlock/Kconfig +++ b/drivers/hwspinlock/Kconfig @@ -8,6 +8,17 @@ config HWSPINLOCK menu Hardware Spinlock drivers +config HWSPINLOCK_MSM + tristate MSM Hardware Spinlock device + depends on ARCH_QCOM This should also depend on OF, as it won't compile or work without it. + select HWSPINLOCK + help + Say y here to support the MSM Hardware Mutex functionality, which + provides a synchronisation mechanism for the various processors on + the SoC. + + If unsure, say N. + config HWSPINLOCK_OMAP tristate OMAP Hardware Spinlock device depends on ARCH_OMAP4 || SOC_OMAP5 || SOC_DRA7XX || SOC_AM33XX || SOC_AM43XX [...] +static const struct of_device_id msm_hwspinlock_of_match[] = { + { .compatible = qcom,sfpb-mutex, .data = (void *)0x4 }, + { .compatible = qcom,tcsr-mutex, .data = (void *)0x80 }, + { }, +}; MODULE_DEVICE_TABLE(of, msm_hwspinlock_of_match); ? [...] +static struct platform_driver msm_hwspinlock_driver = { + .probe = msm_hwspinlock_probe, + .remove = msm_hwspinlock_remove, + .driver = { + .name = msm_hwspinlock, + .owner = THIS_MODULE, No need, as: #define platform_driver_register(drv) \ __platform_driver_register(drv, THIS_MODULE) extern int __platform_driver_register(struct platform_driver *, struct module *); + .of_match_table = msm_hwspinlock_of_match, + }, +}; Otherwise, looks fine. -Courtney -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] hwspinlock/msm: Add support for Qualcomm MSM HW Mutex block
On Sat, Aug 30, 2014 at 02:14:07AM +0200, Stephen Boyd wrote: On 08/29/14 16:41, Courtney Cavin wrote: On Sat, Aug 30, 2014 at 01:14:23AM +0200, Bjorn Andersson wrote: From: Kumar Gala ga...@codeaurora.org Add driver for Qualcomm MSM Hardware Mutex block that exists on newer Qualcomm SoCs. Cc: Jeffrey Hugo jh...@codeaurora.org Cc: Eric Holmberg eholm...@codeaurora.org Cc: Courtney Cavin courtney.ca...@sonymobile.com Signed-off-by: Kumar Gala ga...@codeaurora.org [bjorn: added pm_runtime calls, from Courtney, added sfpb-mutex compatible, updated DT binding documentation formatting] Signed-off-by: Bjorn Andersson bjorn.anders...@sonymobile.com --- [...] diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig index 3612cb5..2cd39e2 100644 --- a/drivers/hwspinlock/Kconfig +++ b/drivers/hwspinlock/Kconfig @@ -8,6 +8,17 @@ config HWSPINLOCK menu Hardware Spinlock drivers +config HWSPINLOCK_MSM + tristate MSM Hardware Spinlock device + depends on ARCH_QCOM This should also depend on OF, as it won't compile or work without it. Doesn't ARCH_QCOM imply OF? ARCH_MULTIPLATFORM has a select USE_OF. Hrm. Apparently. -Courtney -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] mfd: pm8x41: add support for Qualcomm 8x41 PMICs
On Fri, May 09, 2014 at 02:45:30PM +0200, Ivan T. Ivanov wrote: [...] > > > > > > > > I'm thinking we should probably have a generic compatible entry as well, > > > > "qcom,pmic-qpnp" or similar. We should still specify in the binding > > > > that PMIC slaves specify a version-specific string as well as the > > > > generic string. That is, a slave should have: > > > > > > > > compatible = "qcom,pm8841", "qcom,pmic-qpnp"; > > > > > > > > ...in case we would ever need to differentiate in the future. > > > > > > > > (I recall that in a previous version I had done this, but I don't > > > > remember why I had changed it..) > > > > > > I gave this some thought but came to the conclusion that there is no > > > benefit of adding a generic compatible to a new binding. Please clarify > > > a use-case where this would be ... useful. > > > > Having a generic compatible entry allows for easily supporting new PMICs > > without having to add yet another vacuous entry in the ID table. In > > this case I think it's perfectly acceptable given that this driver isn't > > really defining a programming model for a specific device, but rather > > acting much more like a bus. > > > > Requiring a specific PMIC listed before a generic one allows us an > > escape hatch in the future if for some reason we need to add a quirk for > > a specific PMIC. > > Is there a conclusion on this issue? I am voting for generic name :-) > "qcom,pm-qpnp". Josh and I have discussed this offline, and I think we have come to the conclusion that this should be a generic driver with only a generic binding. The current proposed name is "spmi-ext", as there is specific functional relation to Qualcomm, PMICs or QPNP. Further, the binding documentation should be specific to pm8[89]41 as 'mfd/pm8x41.txt', and should contain the compatibles: - "qcom,pm8941", "spmi-ext" - "qcom,pm8841", "spmi-ext" This naming has been discussed to death, so a few more shed color suggestions can't possibly hurt. > Further complication is that several sub function drivers expect to > runtime detect the exact version of the controller ("qcom, qpnp-iadc", > "qcom, qpnp-vadc", "qcom, qpnp-linear-charger"). This is realized by the > exported function of the driver "qcom, qpnp-revid". Would it be good > idea to merge qpnp-revid and "qcom,pm-qpnp" driver? Each block within the PMICs have--undocumented--version registers, so a global version number is not particularly useful. A good example of this is the ADC code [1], as you mentioned. -Courtney [1] https://www.codeaurora.org/cgit/quic/la/kernel/msm-3.10/tree/drivers/thermal/qpnp-adc-tm.c#n469 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] mfd: pm8x41: add support for Qualcomm 8x41 PMICs
On Fri, May 09, 2014 at 02:45:30PM +0200, Ivan T. Ivanov wrote: [...] I'm thinking we should probably have a generic compatible entry as well, qcom,pmic-qpnp or similar. We should still specify in the binding that PMIC slaves specify a version-specific string as well as the generic string. That is, a slave should have: compatible = qcom,pm8841, qcom,pmic-qpnp; ...in case we would ever need to differentiate in the future. (I recall that in a previous version I had done this, but I don't remember why I had changed it..) I gave this some thought but came to the conclusion that there is no benefit of adding a generic compatible to a new binding. Please clarify a use-case where this would be ... useful. Having a generic compatible entry allows for easily supporting new PMICs without having to add yet another vacuous entry in the ID table. In this case I think it's perfectly acceptable given that this driver isn't really defining a programming model for a specific device, but rather acting much more like a bus. Requiring a specific PMIC listed before a generic one allows us an escape hatch in the future if for some reason we need to add a quirk for a specific PMIC. Is there a conclusion on this issue? I am voting for generic name :-) qcom,pm-qpnp. Josh and I have discussed this offline, and I think we have come to the conclusion that this should be a generic driver with only a generic binding. The current proposed name is spmi-ext, as there is specific functional relation to Qualcomm, PMICs or QPNP. Further, the binding documentation should be specific to pm8[89]41 as 'mfd/pm8x41.txt', and should contain the compatibles: - qcom,pm8941, spmi-ext - qcom,pm8841, spmi-ext This naming has been discussed to death, so a few more shed color suggestions can't possibly hurt. Further complication is that several sub function drivers expect to runtime detect the exact version of the controller (qcom, qpnp-iadc, qcom, qpnp-vadc, qcom, qpnp-linear-charger). This is realized by the exported function of the driver qcom, qpnp-revid. Would it be good idea to merge qpnp-revid and qcom,pm-qpnp driver? Each block within the PMICs have--undocumented--version registers, so a global version number is not particularly useful. A good example of this is the ADC code [1], as you mentioned. -Courtney [1] https://www.codeaurora.org/cgit/quic/la/kernel/msm-3.10/tree/drivers/thermal/qpnp-adc-tm.c#n469 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] mfd: pm8x41: add support for Qualcomm 8x41 PMICs
On Sat, Apr 26, 2014 at 02:28:06AM +0200, Frank Rowand wrote: > On 4/23/2014 6:19 AM, Ivan T. Ivanov wrote: [...] > >> +static int pm8x41_probe(struct spmi_device *sdev) > >> +{ > >> + struct regmap *regmap; > >> + > >> + regmap = devm_regmap_init_spmi_ext(sdev, _regmap_config); > >> + if (IS_ERR(regmap)) { > >> + dev_dbg(>dev, "regmap creation failed.\n"); > >> + return PTR_ERR(regmap); > >> + } > >> + > >> + return of_platform_populate(sdev->dev.of_node, NULL, NULL, >dev); > > > > I think that this will not going to work. For example in this particular > > case, both controllers have "qcom,qpnp-revid" peripheral which is > > located at offset 0x100. > > > > And the result is: > > > > [0.963944] sysfs: cannot create duplicate filename > > '/bus/platform/devices/100.revid' > > The duplicate filename error is because pm8x41_probe() is calling > of_platform_populate() > directly. > > If that call is removed then there is no attempt to create a revid file in > /sys/bus/platform/devices. If I add your pm8841@4 node to my dts file, then > the 100.revid file is properly created at > > /sys/firmware/devicetree/base/soc/qcom,spmi@fc4cf000/pm8841@4/qcom,revid@100 > > and no attempt is made to create /sys/bus/platform/devices/100.revid > > This appears to be correct to me, because I do not think revid should be > created as > a platform device. > Wait, what? That's the entire point of this driver. [...] > > Any suggestions? > > Remove of_platform_populate() from pm8x41_probe(). Do you know of any > other reason it can not be removed? I do! If you remove it, the entire driver becomes a useless pile of nothing! The PMICs targeted by this driver are mostly made up of independent blocks which should be able to be written as standalone device drivers. The design of this driver is to create a simple bus-like layer between SPMI and these independent blocks. of_platform_populate() is what makes it work. -Courtney -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] mfd: pm8x41: add support for Qualcomm 8x41 PMICs
On Sat, Apr 26, 2014 at 02:28:06AM +0200, Frank Rowand wrote: On 4/23/2014 6:19 AM, Ivan T. Ivanov wrote: [...] +static int pm8x41_probe(struct spmi_device *sdev) +{ + struct regmap *regmap; + + regmap = devm_regmap_init_spmi_ext(sdev, pm8x41_regmap_config); + if (IS_ERR(regmap)) { + dev_dbg(sdev-dev, regmap creation failed.\n); + return PTR_ERR(regmap); + } + + return of_platform_populate(sdev-dev.of_node, NULL, NULL, sdev-dev); I think that this will not going to work. For example in this particular case, both controllers have qcom,qpnp-revid peripheral which is located at offset 0x100. And the result is: [0.963944] sysfs: cannot create duplicate filename '/bus/platform/devices/100.revid' The duplicate filename error is because pm8x41_probe() is calling of_platform_populate() directly. If that call is removed then there is no attempt to create a revid file in /sys/bus/platform/devices. If I add your pm8841@4 node to my dts file, then the 100.revid file is properly created at /sys/firmware/devicetree/base/soc/qcom,spmi@fc4cf000/pm8841@4/qcom,revid@100 and no attempt is made to create /sys/bus/platform/devices/100.revid This appears to be correct to me, because I do not think revid should be created as a platform device. Wait, what? That's the entire point of this driver. [...] Any suggestions? Remove of_platform_populate() from pm8x41_probe(). Do you know of any other reason it can not be removed? I do! If you remove it, the entire driver becomes a useless pile of nothing! The PMICs targeted by this driver are mostly made up of independent blocks which should be able to be written as standalone device drivers. The design of this driver is to create a simple bus-like layer between SPMI and these independent blocks. of_platform_populate() is what makes it work. -Courtney -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] mfd: pm8x41: add support for Qualcomm 8x41 PMICs
On Wed, Apr 23, 2014 at 11:46:26PM +0200, Josh Cartwright wrote: > On Tue, Apr 22, 2014 at 05:31:49PM -0700, Courtney Cavin wrote: > > From: Josh Cartwright > > > > The Qualcomm 8941 and 8841 PMICs are components used with the Snapdragon > > 800 series SoC family. This driver exists largely as a glue mfd component, > > it exists to be an owner of an SPMI regmap for children devices > > described in device tree. > > > > Signed-off-by: Josh Cartwright > > Signed-off-by: Courtney Cavin > > Hey Courtney- > > Thanks for picking this up! Well, I've been poking you about it for months now, so I figured I'd stop being annoying, and start being productive. > > One thing that I had meant to do is rename this thing. Nothing about > this is PM8841/PM8941 specific at all. It should apply equally to all > Qualcomm's PMICs which implement QPNP. > > Perhaps a better name would be "qcom-pmic-qpnp". What's a QPNP? Really. I've heard you speak about it before as being a definition of the register layout for interrupts, but I have no documentation on it. I would argue here from my understanding that this driver isn't specific to QPNP either. With that in mind we could just go with "qcom-pmic-spmi". In fact just "spmi-ext" would not be incorrect, as this driver has little to do with PMICs at all. My point here is that we can easily make this into something very generic, but that only causes problems in the future when it's not "generic enough", and we have to add quirks. If in the future Qualcomm releases a pm8A41, and it's qpnp, but not spmi, or spmi, but not 'ext', then we need to either change this driver dramatically, or write a new one. I like keeping this driver name specific to what we know it supports. We can rename it in the future if deemed appropriate, but I'd rather not make it something that which turns out to be wrong at some later point. Having said all of this, I have no doubt that your HW engineers will find a way to break our interpretation of their naming scheme... once again. > > [..] > > +++ b/drivers/mfd/pm8x41.c > > @@ -0,0 +1,63 @@ > > +/* Copyright (c) 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 > > +#include > > +#include > > +#include > > +#include > > + > > +static const struct regmap_config pm8x41_regmap_config = { > > + .reg_bits = 16, > > + .val_bits = 8, > > + .max_register = 0x, > > +}; > > This reminds me. David Collins (CC'd) noticed that there are usecases > where peripheral drivers will need to be accessing registers from atomic > context, so we should probably be setting .fast_io in the SPMI > regmap_bus structures, but we can tackle that when we get there. Hrm. Please comment on this David. I would like to see a solid use-case before even considering that. > > + > > +static int pm8x41_remove_child(struct device *dev, void *unused) > > +{ > > + platform_device_unregister(to_platform_device(dev)); > > + return 0; > > +} > > + > > +static void pm8x41_remove(struct spmi_device *sdev) > > +{ > > + device_for_each_child(>dev, NULL, pm8x41_remove_child); > > +} > > + > > +static int pm8x41_probe(struct spmi_device *sdev) > > +{ > > + struct regmap *regmap; > > + > > + regmap = devm_regmap_init_spmi_ext(sdev, _regmap_config); > > + if (IS_ERR(regmap)) { > > + dev_dbg(>dev, "regmap creation failed.\n"); > > + return PTR_ERR(regmap); > > + } > > + > > + return of_platform_populate(sdev->dev.of_node, NULL, NULL, >dev); > > +} > > + > > +static const struct of_device_id pm8x41_id_table[] = { > > + { .compatible = "qcom,pm8841", }, > > + { .compatible = "qcom,pm8941", }, > > + {}, > > +}; > > +MODULE_DEVICE_TABLE(of, pm8x41_id_table); > > I'm thinking we should probably have a generic compatible entry as well, > "qcom,pmic-qpnp" or similar. We should still specify in the binding > that PMIC slaves specify a version-specific string as well as
Re: [PATCH 1/2] mfd: pm8x41: add support for Qualcomm 8x41 PMICs
On Wed, Apr 23, 2014 at 10:34:32PM +0200, Ivan T. Ivanov wrote: > > On Wed, 2014-04-23 at 11:16 -0700, Courtney Cavin wrote: > > On Wed, Apr 23, 2014 at 03:19:28PM +0200, Ivan T. Ivanov wrote: > > > > > > Hi, > > > > > > On Tue, 2014-04-22 at 17:31 -0700, Courtney Cavin wrote: > > > > From: Josh Cartwright > > > > > > > > The Qualcomm 8941 and 8841 PMICs are components used with the Snapdragon > > > > 800 series SoC family. This driver exists largely as a glue mfd > > > > component, > > > > it exists to be an owner of an SPMI regmap for children devices > > > > described in device tree. > > > > > > > > > > Thanks. This is exactly what I have planed to do :-) > > > > > > > Sorry if I usurped your work! > > Noting to worry. I just was surprised how close it is to my vision ;-). > Well, you might notice how extremely close it is to what Josh posted in October in his SPMI series [1], so I can't take any credit there. > > > > > > Signed-off-by: Josh Cartwright > > > > Signed-off-by: Courtney Cavin > > > > --- > > > > drivers/mfd/Kconfig | 13 +++ > > > > drivers/mfd/Makefile | 1 + > > > > drivers/mfd/pm8x41.c | 63 > > > > > > > > 3 files changed, 77 insertions(+) > > > > create mode 100644 drivers/mfd/pm8x41.c > > > > > > > > > > > > > > > > > + > > > > +static int pm8x41_probe(struct spmi_device *sdev) > > > > +{ > > > > + struct regmap *regmap; > > > > + > > > > + regmap = devm_regmap_init_spmi_ext(sdev, _regmap_config); > > > > + if (IS_ERR(regmap)) { > > > > + dev_dbg(>dev, "regmap creation failed.\n"); > > > > + return PTR_ERR(regmap); > > > > + } > > > > + > > > > + return of_platform_populate(sdev->dev.of_node, NULL, NULL, > > > > >dev); > > > > > > I think that this will not going to work. For example in this particular > > > case, both controllers have "qcom,qpnp-revid" peripheral which is > > > located at offset 0x100. > > > > > > And the result is: > > > > > > [0.963944] sysfs: cannot create duplicate filename > > > '/bus/platform/devices/100.revid' > > > > > [...] > > > > > > Any suggestions? > > > > That's expected behavior actually. You have two nodes in DT named the > > same thing and at the same address. This error is due to the fact that > > all devices are put in '/bus/platform/devices/' with a name made from > > the unit address and name specified in DT. There's no other unique > > information used to differentiate the devices. > > > > If you simply change the names in DT, it works. > > Sure, it will work. But they are part of different address spaces. > Why we should add, IMHO, artificial requirement that names should > be unique? Is it possible to prefix child nodes with parent device > address? As side note, why they should be registered on the platform > bus at all? To be honest I don't have solution. I agree, and it would appear that the ePAPR does as well. Feel free to send patches! > > [...] qcom,qpnp-revid 100.qcom,pm8841-revid: PM8841 v2.0 options: 0, 0, 2, 2 > > [...] qcom,qpnp-revid 100.qcom,pm8941-revid: PM8941 v3.0 options: 2, 0, 0, 0 > > > > Whether this should be "fixed" in the device/bus/sysfs core, I don't > > know, but it isn't specifically an issue with this driver, and there's > > little-to-nothing I can do to fix it here. > > > > -Courtney -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] mfd: pm8x41: add support for Qualcomm 8x41 PMICs
On Wed, Apr 23, 2014 at 03:19:28PM +0200, Ivan T. Ivanov wrote: > > Hi, > > On Tue, 2014-04-22 at 17:31 -0700, Courtney Cavin wrote: > > From: Josh Cartwright > > > > The Qualcomm 8941 and 8841 PMICs are components used with the Snapdragon > > 800 series SoC family. This driver exists largely as a glue mfd component, > > it exists to be an owner of an SPMI regmap for children devices > > described in device tree. > > > > Thanks. This is exactly what I have planed to do :-) > Sorry if I usurped your work! > > Signed-off-by: Josh Cartwright > > Signed-off-by: Courtney Cavin > > --- > > drivers/mfd/Kconfig | 13 +++ > > drivers/mfd/Makefile | 1 + > > drivers/mfd/pm8x41.c | 63 > > > > 3 files changed, 77 insertions(+) > > create mode 100644 drivers/mfd/pm8x41.c > > > > > > > + > > +static int pm8x41_probe(struct spmi_device *sdev) > > +{ > > + struct regmap *regmap; > > + > > + regmap = devm_regmap_init_spmi_ext(sdev, _regmap_config); > > + if (IS_ERR(regmap)) { > > + dev_dbg(>dev, "regmap creation failed.\n"); > > + return PTR_ERR(regmap); > > + } > > + > > + return of_platform_populate(sdev->dev.of_node, NULL, NULL, >dev); > > I think that this will not going to work. For example in this particular > case, both controllers have "qcom,qpnp-revid" peripheral which is > located at offset 0x100. > > And the result is: > > [0.963944] sysfs: cannot create duplicate filename > '/bus/platform/devices/100.revid' > [...] > > Any suggestions? That's expected behavior actually. You have two nodes in DT named the same thing and at the same address. This error is due to the fact that all devices are put in '/bus/platform/devices/' with a name made from the unit address and name specified in DT. There's no other unique information used to differentiate the devices. If you simply change the names in DT, it works. Example follows: qcom,pm8941@0 { compatible = "qcom,pm8941"; reg = <0x0 SPMI_USID>; #address-cells = <1>; #size-cells = <0>; qcom,pm8941-revid@100 { compatible = "qcom,qpnp-revid"; reg = <0x100>; }; }; qcom,pm8841@4 { compatible = "qcom,pm8841"; reg = <0x4 SPMI_USID>; #address-cells = <1>; #size-cells = <0>; qcom,pm8841-revid@100 { compatible = "qcom,qpnp-revid"; reg = <0x100>; }; }; [...] qcom,qpnp-revid 100.qcom,pm8841-revid: PM8841 v2.0 options: 0, 0, 2, 2 [...] qcom,qpnp-revid 100.qcom,pm8941-revid: PM8941 v3.0 options: 2, 0, 0, 0 Whether this should be "fixed" in the device/bus/sysfs core, I don't know, but it isn't specifically an issue with this driver, and there's little-to-nothing I can do to fix it here. -Courtney -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] mfd: pm8x41: add support for Qualcomm 8x41 PMICs
On Wed, Apr 23, 2014 at 12:50:55PM +0200, Lee Jones wrote: > > From: Josh Cartwright > > > > The Qualcomm 8941 and 8841 PMICs are components used with the Snapdragon > > 800 series SoC family. This driver exists largely as a glue mfd component, > > it exists to be an owner of an SPMI regmap for children devices > > described in device tree. > > > > Signed-off-by: Josh Cartwright > > Signed-off-by: Courtney Cavin > > --- > > drivers/mfd/Kconfig | 13 +++ > > drivers/mfd/Makefile | 1 + > > drivers/mfd/pm8x41.c | 63 > > > > 3 files changed, 77 insertions(+) > > create mode 100644 drivers/mfd/pm8x41.c > > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > > index 3383412..f5ff799 100644 > > --- a/drivers/mfd/Kconfig > > +++ b/drivers/mfd/Kconfig > > @@ -502,6 +502,19 @@ config MFD_PM8921_CORE > > Say M here if you want to include support for PM8921 chip as a module. > > This will build a module called "pm8921-core". > > > > +config MFD_PM8X41 > > + bool "Qualcomm PM8x41 PMIC" Hrm. This should be tristate. > > + depends on ARCH_QCOM > > depends on OF? Yea, that's probably best. > > + select REGMAP_SPMI > > + help > > + This enables basic support for the Qualcomm 8941 and 8841 PMICs. > > + These PMICs are currently used with the Snapdragon 800 series of > > + SoCs. Note, that this will only be useful paired with descriptions > > + of the independent functions as children nodes in the device tree. > > + > > + Say M here if you want to include support for the PM8x41 series as a > > + module. The module will be called "pm8x41". > > + > > config MFD_RDC321X > > tristate "RDC R-321x southbridge" > > select MFD_CORE > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > > index 2851275..f0df41d 100644 > > --- a/drivers/mfd/Makefile > > +++ b/drivers/mfd/Makefile > > @@ -151,6 +151,7 @@ obj-$(CONFIG_MFD_SI476X_CORE) += si476x-core.o > > obj-$(CONFIG_MFD_CS5535) += cs5535-mfd.o > > obj-$(CONFIG_MFD_OMAP_USB_HOST)+= omap-usb-host.o omap-usb-tll.o > > obj-$(CONFIG_MFD_PM8921_CORE) += pm8921-core.o ssbi.o > > +obj-$(CONFIG_MFD_PM8X41) += pm8x41.o > > obj-$(CONFIG_TPS65911_COMPARATOR) += tps65911-comparator.o > > obj-$(CONFIG_MFD_TPS65090) += tps65090.o > > obj-$(CONFIG_MFD_AAT2870_CORE) += aat2870-core.o > > diff --git a/drivers/mfd/pm8x41.c b/drivers/mfd/pm8x41.c > > new file mode 100644 > > index 000..c85e0d6 > > --- /dev/null > > +++ b/drivers/mfd/pm8x41.c > > @@ -0,0 +1,63 @@ > > +/* Copyright (c) 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 > > +#include > > +#include > > +#include > > +#include > > + > > +static const struct regmap_config pm8x41_regmap_config = { > > + .reg_bits = 16, > > + .val_bits = 8, > > + .max_register = 0x, > > I've never seen this many registers registered before. > > Are you sure you want to regmap the entire bank? > Quite sure. The pm8941 has blocks at each 0x100, up to 0xE700. Additionally, from what I've heard, there are some undocumented (for me at least) blocks higher than that. > > +}; > > + > > +static int pm8x41_remove_child(struct device *dev, void *unused) > > +{ > > + platform_device_unregister(to_platform_device(dev)); > > + return 0; > > +} > > + > > +static void pm8x41_remove(struct spmi_device *sdev) > > +{ > > + device_for_each_child(>dev, NULL, pm8x41_remove_child); > > +} > > Nit: It's strange to see the .remove above the .probe. > Really? Alright, I can move it. > > +static int pm8x41_probe(struct spmi_device *sdev) > > +{ > > + struct regmap *regmap; > > + > > + regmap = devm_regmap_init_spmi_ext(sdev, _regmap_config); > > + if (IS_ERR(regmap)) { > > +
Re: [PATCH 1/2] mfd: pm8x41: add support for Qualcomm 8x41 PMICs
On Wed, Apr 23, 2014 at 12:50:55PM +0200, Lee Jones wrote: From: Josh Cartwright jo...@codeaurora.org The Qualcomm 8941 and 8841 PMICs are components used with the Snapdragon 800 series SoC family. This driver exists largely as a glue mfd component, it exists to be an owner of an SPMI regmap for children devices described in device tree. Signed-off-by: Josh Cartwright jo...@codeaurora.org Signed-off-by: Courtney Cavin courtney.ca...@sonymobile.com --- drivers/mfd/Kconfig | 13 +++ drivers/mfd/Makefile | 1 + drivers/mfd/pm8x41.c | 63 3 files changed, 77 insertions(+) create mode 100644 drivers/mfd/pm8x41.c diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index 3383412..f5ff799 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -502,6 +502,19 @@ config MFD_PM8921_CORE Say M here if you want to include support for PM8921 chip as a module. This will build a module called pm8921-core. +config MFD_PM8X41 + bool Qualcomm PM8x41 PMIC Hrm. This should be tristate. + depends on ARCH_QCOM depends on OF? Yea, that's probably best. + select REGMAP_SPMI + help + This enables basic support for the Qualcomm 8941 and 8841 PMICs. + These PMICs are currently used with the Snapdragon 800 series of + SoCs. Note, that this will only be useful paired with descriptions + of the independent functions as children nodes in the device tree. + + Say M here if you want to include support for the PM8x41 series as a + module. The module will be called pm8x41. + config MFD_RDC321X tristate RDC R-321x southbridge select MFD_CORE diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index 2851275..f0df41d 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -151,6 +151,7 @@ obj-$(CONFIG_MFD_SI476X_CORE) += si476x-core.o obj-$(CONFIG_MFD_CS5535) += cs5535-mfd.o obj-$(CONFIG_MFD_OMAP_USB_HOST)+= omap-usb-host.o omap-usb-tll.o obj-$(CONFIG_MFD_PM8921_CORE) += pm8921-core.o ssbi.o +obj-$(CONFIG_MFD_PM8X41) += pm8x41.o obj-$(CONFIG_TPS65911_COMPARATOR) += tps65911-comparator.o obj-$(CONFIG_MFD_TPS65090) += tps65090.o obj-$(CONFIG_MFD_AAT2870_CORE) += aat2870-core.o diff --git a/drivers/mfd/pm8x41.c b/drivers/mfd/pm8x41.c new file mode 100644 index 000..c85e0d6 --- /dev/null +++ b/drivers/mfd/pm8x41.c @@ -0,0 +1,63 @@ +/* Copyright (c) 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/module.h +#include linux/spmi.h +#include linux/regmap.h +#include linux/of_platform.h + +static const struct regmap_config pm8x41_regmap_config = { + .reg_bits = 16, + .val_bits = 8, + .max_register = 0x, I've never seen this many registers registered before. Are you sure you want to regmap the entire bank? Quite sure. The pm8941 has blocks at each 0x100, up to 0xE700. Additionally, from what I've heard, there are some undocumented (for me at least) blocks higher than that. +}; + +static int pm8x41_remove_child(struct device *dev, void *unused) +{ + platform_device_unregister(to_platform_device(dev)); + return 0; +} + +static void pm8x41_remove(struct spmi_device *sdev) +{ + device_for_each_child(sdev-dev, NULL, pm8x41_remove_child); +} Nit: It's strange to see the .remove above the .probe. Really? Alright, I can move it. +static int pm8x41_probe(struct spmi_device *sdev) +{ + struct regmap *regmap; + + regmap = devm_regmap_init_spmi_ext(sdev, pm8x41_regmap_config); + if (IS_ERR(regmap)) { + dev_dbg(sdev-dev, regmap creation failed.\n); This is not debug, it's an error. Indeed. + return PTR_ERR(regmap); + } + + return of_platform_populate(sdev-dev.of_node, NULL, NULL, sdev-dev); +} + +static const struct of_device_id pm8x41_id_table[] = { + { .compatible = qcom,pm8841, }, + { .compatible = qcom,pm8941, }, + {}, +}; +MODULE_DEVICE_TABLE(of, pm8x41_id_table); + +static struct spmi_driver pm8x41_driver = { + .probe = pm8x41_probe, + .remove = pm8x41_remove, + .driver = { + .name = pm8x41, + .of_match_table = pm8x41_id_table, of_match_ptr() Ok. + }, +}; +module_spmi_driver(pm8x41_driver); Thanks
Re: [PATCH 1/2] mfd: pm8x41: add support for Qualcomm 8x41 PMICs
On Wed, Apr 23, 2014 at 03:19:28PM +0200, Ivan T. Ivanov wrote: Hi, On Tue, 2014-04-22 at 17:31 -0700, Courtney Cavin wrote: From: Josh Cartwright jo...@codeaurora.org The Qualcomm 8941 and 8841 PMICs are components used with the Snapdragon 800 series SoC family. This driver exists largely as a glue mfd component, it exists to be an owner of an SPMI regmap for children devices described in device tree. Thanks. This is exactly what I have planed to do :-) Sorry if I usurped your work! Signed-off-by: Josh Cartwright jo...@codeaurora.org Signed-off-by: Courtney Cavin courtney.ca...@sonymobile.com --- drivers/mfd/Kconfig | 13 +++ drivers/mfd/Makefile | 1 + drivers/mfd/pm8x41.c | 63 3 files changed, 77 insertions(+) create mode 100644 drivers/mfd/pm8x41.c snip + +static int pm8x41_probe(struct spmi_device *sdev) +{ + struct regmap *regmap; + + regmap = devm_regmap_init_spmi_ext(sdev, pm8x41_regmap_config); + if (IS_ERR(regmap)) { + dev_dbg(sdev-dev, regmap creation failed.\n); + return PTR_ERR(regmap); + } + + return of_platform_populate(sdev-dev.of_node, NULL, NULL, sdev-dev); I think that this will not going to work. For example in this particular case, both controllers have qcom,qpnp-revid peripheral which is located at offset 0x100. And the result is: [0.963944] sysfs: cannot create duplicate filename '/bus/platform/devices/100.revid' [...] Any suggestions? That's expected behavior actually. You have two nodes in DT named the same thing and at the same address. This error is due to the fact that all devices are put in '/bus/platform/devices/' with a name made from the unit address and name specified in DT. There's no other unique information used to differentiate the devices. If you simply change the names in DT, it works. Example follows: qcom,pm8941@0 { compatible = qcom,pm8941; reg = 0x0 SPMI_USID; #address-cells = 1; #size-cells = 0; qcom,pm8941-revid@100 { compatible = qcom,qpnp-revid; reg = 0x100; }; }; qcom,pm8841@4 { compatible = qcom,pm8841; reg = 0x4 SPMI_USID; #address-cells = 1; #size-cells = 0; qcom,pm8841-revid@100 { compatible = qcom,qpnp-revid; reg = 0x100; }; }; [...] qcom,qpnp-revid 100.qcom,pm8841-revid: PM8841 v2.0 options: 0, 0, 2, 2 [...] qcom,qpnp-revid 100.qcom,pm8941-revid: PM8941 v3.0 options: 2, 0, 0, 0 Whether this should be fixed in the device/bus/sysfs core, I don't know, but it isn't specifically an issue with this driver, and there's little-to-nothing I can do to fix it here. -Courtney -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] mfd: pm8x41: add support for Qualcomm 8x41 PMICs
On Wed, Apr 23, 2014 at 10:34:32PM +0200, Ivan T. Ivanov wrote: On Wed, 2014-04-23 at 11:16 -0700, Courtney Cavin wrote: On Wed, Apr 23, 2014 at 03:19:28PM +0200, Ivan T. Ivanov wrote: Hi, On Tue, 2014-04-22 at 17:31 -0700, Courtney Cavin wrote: From: Josh Cartwright jo...@codeaurora.org The Qualcomm 8941 and 8841 PMICs are components used with the Snapdragon 800 series SoC family. This driver exists largely as a glue mfd component, it exists to be an owner of an SPMI regmap for children devices described in device tree. Thanks. This is exactly what I have planed to do :-) Sorry if I usurped your work! Noting to worry. I just was surprised how close it is to my vision ;-). Well, you might notice how extremely close it is to what Josh posted in October in his SPMI series [1], so I can't take any credit there. Signed-off-by: Josh Cartwright jo...@codeaurora.org Signed-off-by: Courtney Cavin courtney.ca...@sonymobile.com --- drivers/mfd/Kconfig | 13 +++ drivers/mfd/Makefile | 1 + drivers/mfd/pm8x41.c | 63 3 files changed, 77 insertions(+) create mode 100644 drivers/mfd/pm8x41.c snip + +static int pm8x41_probe(struct spmi_device *sdev) +{ + struct regmap *regmap; + + regmap = devm_regmap_init_spmi_ext(sdev, pm8x41_regmap_config); + if (IS_ERR(regmap)) { + dev_dbg(sdev-dev, regmap creation failed.\n); + return PTR_ERR(regmap); + } + + return of_platform_populate(sdev-dev.of_node, NULL, NULL, sdev-dev); I think that this will not going to work. For example in this particular case, both controllers have qcom,qpnp-revid peripheral which is located at offset 0x100. And the result is: [0.963944] sysfs: cannot create duplicate filename '/bus/platform/devices/100.revid' [...] Any suggestions? That's expected behavior actually. You have two nodes in DT named the same thing and at the same address. This error is due to the fact that all devices are put in '/bus/platform/devices/' with a name made from the unit address and name specified in DT. There's no other unique information used to differentiate the devices. If you simply change the names in DT, it works. Sure, it will work. But they are part of different address spaces. Why we should add, IMHO, artificial requirement that names should be unique? Is it possible to prefix child nodes with parent device address? As side note, why they should be registered on the platform bus at all? To be honest I don't have solution. I agree, and it would appear that the ePAPR does as well. Feel free to send patches! [...] qcom,qpnp-revid 100.qcom,pm8841-revid: PM8841 v2.0 options: 0, 0, 2, 2 [...] qcom,qpnp-revid 100.qcom,pm8941-revid: PM8941 v3.0 options: 2, 0, 0, 0 Whether this should be fixed in the device/bus/sysfs core, I don't know, but it isn't specifically an issue with this driver, and there's little-to-nothing I can do to fix it here. -Courtney -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] mfd: pm8x41: add support for Qualcomm 8x41 PMICs
On Wed, Apr 23, 2014 at 11:46:26PM +0200, Josh Cartwright wrote: On Tue, Apr 22, 2014 at 05:31:49PM -0700, Courtney Cavin wrote: From: Josh Cartwright jo...@codeaurora.org The Qualcomm 8941 and 8841 PMICs are components used with the Snapdragon 800 series SoC family. This driver exists largely as a glue mfd component, it exists to be an owner of an SPMI regmap for children devices described in device tree. Signed-off-by: Josh Cartwright jo...@codeaurora.org Signed-off-by: Courtney Cavin courtney.ca...@sonymobile.com Hey Courtney- Thanks for picking this up! Well, I've been poking you about it for months now, so I figured I'd stop being annoying, and start being productive. One thing that I had meant to do is rename this thing. Nothing about this is PM8841/PM8941 specific at all. It should apply equally to all Qualcomm's PMICs which implement QPNP. Perhaps a better name would be qcom-pmic-qpnp. What's a QPNP? Really. I've heard you speak about it before as being a definition of the register layout for interrupts, but I have no documentation on it. I would argue here from my understanding that this driver isn't specific to QPNP either. With that in mind we could just go with qcom-pmic-spmi. In fact just spmi-ext would not be incorrect, as this driver has little to do with PMICs at all. My point here is that we can easily make this into something very generic, but that only causes problems in the future when it's not generic enough, and we have to add quirks. If in the future Qualcomm releases a pm8A41, and it's qpnp, but not spmi, or spmi, but not 'ext', then we need to either change this driver dramatically, or write a new one. I like keeping this driver name specific to what we know it supports. We can rename it in the future if deemed appropriate, but I'd rather not make it something that which turns out to be wrong at some later point. Having said all of this, I have no doubt that your HW engineers will find a way to break our interpretation of their naming scheme... once again. [..] +++ b/drivers/mfd/pm8x41.c @@ -0,0 +1,63 @@ +/* Copyright (c) 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/module.h +#include linux/spmi.h +#include linux/regmap.h +#include linux/of_platform.h + +static const struct regmap_config pm8x41_regmap_config = { + .reg_bits = 16, + .val_bits = 8, + .max_register = 0x, +}; This reminds me. David Collins (CC'd) noticed that there are usecases where peripheral drivers will need to be accessing registers from atomic context, so we should probably be setting .fast_io in the SPMI regmap_bus structures, but we can tackle that when we get there. Hrm. Please comment on this David. I would like to see a solid use-case before even considering that. + +static int pm8x41_remove_child(struct device *dev, void *unused) +{ + platform_device_unregister(to_platform_device(dev)); + return 0; +} + +static void pm8x41_remove(struct spmi_device *sdev) +{ + device_for_each_child(sdev-dev, NULL, pm8x41_remove_child); +} + +static int pm8x41_probe(struct spmi_device *sdev) +{ + struct regmap *regmap; + + regmap = devm_regmap_init_spmi_ext(sdev, pm8x41_regmap_config); + if (IS_ERR(regmap)) { + dev_dbg(sdev-dev, regmap creation failed.\n); + return PTR_ERR(regmap); + } + + return of_platform_populate(sdev-dev.of_node, NULL, NULL, sdev-dev); +} + +static const struct of_device_id pm8x41_id_table[] = { + { .compatible = qcom,pm8841, }, + { .compatible = qcom,pm8941, }, + {}, +}; +MODULE_DEVICE_TABLE(of, pm8x41_id_table); I'm thinking we should probably have a generic compatible entry as well, qcom,pmic-qpnp or similar. We should still specify in the binding that PMIC slaves specify a version-specific string as well as the generic string. That is, a slave should have: compatible = qcom,pm8841, qcom,pmic-qpnp; ...in case we would ever need to differentiate in the future. (I recall that in a previous version I had done this, but I don't remember why I had changed it..) I gave this some thought but came to the conclusion that there is no benefit of adding a generic compatible to a new binding. Please clarify a use-case where this would be ... useful. Thanks for the review! -Courtney -- To unsubscribe from this list: send the line unsubscribe
[PATCH 1/2] mfd: pm8x41: add support for Qualcomm 8x41 PMICs
From: Josh Cartwright The Qualcomm 8941 and 8841 PMICs are components used with the Snapdragon 800 series SoC family. This driver exists largely as a glue mfd component, it exists to be an owner of an SPMI regmap for children devices described in device tree. Signed-off-by: Josh Cartwright Signed-off-by: Courtney Cavin --- drivers/mfd/Kconfig | 13 +++ drivers/mfd/Makefile | 1 + drivers/mfd/pm8x41.c | 63 3 files changed, 77 insertions(+) create mode 100644 drivers/mfd/pm8x41.c diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index 3383412..f5ff799 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -502,6 +502,19 @@ config MFD_PM8921_CORE Say M here if you want to include support for PM8921 chip as a module. This will build a module called "pm8921-core". +config MFD_PM8X41 + bool "Qualcomm PM8x41 PMIC" + depends on ARCH_QCOM + select REGMAP_SPMI + help + This enables basic support for the Qualcomm 8941 and 8841 PMICs. + These PMICs are currently used with the Snapdragon 800 series of + SoCs. Note, that this will only be useful paired with descriptions + of the independent functions as children nodes in the device tree. + + Say M here if you want to include support for the PM8x41 series as a + module. The module will be called "pm8x41". + config MFD_RDC321X tristate "RDC R-321x southbridge" select MFD_CORE diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index 2851275..f0df41d 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -151,6 +151,7 @@ obj-$(CONFIG_MFD_SI476X_CORE) += si476x-core.o obj-$(CONFIG_MFD_CS5535) += cs5535-mfd.o obj-$(CONFIG_MFD_OMAP_USB_HOST)+= omap-usb-host.o omap-usb-tll.o obj-$(CONFIG_MFD_PM8921_CORE) += pm8921-core.o ssbi.o +obj-$(CONFIG_MFD_PM8X41) += pm8x41.o obj-$(CONFIG_TPS65911_COMPARATOR) += tps65911-comparator.o obj-$(CONFIG_MFD_TPS65090) += tps65090.o obj-$(CONFIG_MFD_AAT2870_CORE) += aat2870-core.o diff --git a/drivers/mfd/pm8x41.c b/drivers/mfd/pm8x41.c new file mode 100644 index 000..c85e0d6 --- /dev/null +++ b/drivers/mfd/pm8x41.c @@ -0,0 +1,63 @@ +/* Copyright (c) 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 +#include +#include +#include +#include + +static const struct regmap_config pm8x41_regmap_config = { + .reg_bits = 16, + .val_bits = 8, + .max_register = 0x, +}; + +static int pm8x41_remove_child(struct device *dev, void *unused) +{ + platform_device_unregister(to_platform_device(dev)); + return 0; +} + +static void pm8x41_remove(struct spmi_device *sdev) +{ + device_for_each_child(>dev, NULL, pm8x41_remove_child); +} + +static int pm8x41_probe(struct spmi_device *sdev) +{ + struct regmap *regmap; + + regmap = devm_regmap_init_spmi_ext(sdev, _regmap_config); + if (IS_ERR(regmap)) { + dev_dbg(>dev, "regmap creation failed.\n"); + return PTR_ERR(regmap); + } + + return of_platform_populate(sdev->dev.of_node, NULL, NULL, >dev); +} + +static const struct of_device_id pm8x41_id_table[] = { + { .compatible = "qcom,pm8841", }, + { .compatible = "qcom,pm8941", }, + {}, +}; +MODULE_DEVICE_TABLE(of, pm8x41_id_table); + +static struct spmi_driver pm8x41_driver = { + .probe = pm8x41_probe, + .remove = pm8x41_remove, + .driver = { + .name = "pm8x41", + .of_match_table = pm8x41_id_table, + }, +}; +module_spmi_driver(pm8x41_driver); -- 1.8.1.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] mfd: pm8x41: document device tree bindings
From: Josh Cartwright Document the bindings used to describe the Qualcomm 8x41 PMICs. Signed-off-by: Josh Cartwright Signed-off-by: Courtney Cavin --- Documentation/devicetree/bindings/mfd/pm8x41.txt | 34 1 file changed, 34 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/pm8x41.txt diff --git a/Documentation/devicetree/bindings/mfd/pm8x41.txt b/Documentation/devicetree/bindings/mfd/pm8x41.txt new file mode 100644 index 000..b865a4f --- /dev/null +++ b/Documentation/devicetree/bindings/mfd/pm8x41.txt @@ -0,0 +1,34 @@ +Qualcomm PM8841 and PM8941 PMIC multi-function device bindings + +The PM8x41 PMICs are used with the Qualcomm Snapdragon 800 series SoCs, and are +interfaced to the chip via the SPMI (System Power Management Interface) bus. +Support for multiple independent functions are implemented by splitting the +16-bit SPMI slave address space into 256 smaller fixed-size regions, 256 bytes +each. A function can consume one or more of these fixed-size register regions. + +Required properties: +- compatible: Must be one of: + "qcom,pm8841" + "qcom,pm8941" +- reg: Specifies the SPMI USID slave address for this device; + See bindings/spmi/spmi.txt +- #address-cells = <1> +- #size-cells = <0> + +Each child node represents a function of the PM8x41. Each child 'reg' entry +describes an offset within the USID slave address where the region starts. + +Example: + +pm8941@0 { + compatible = "qcom,pm8941"; + reg = <0x0 SPMI_USID>; + + #address-cells = <1>; + #size-cells = <0>; + + rtc@6000 { + compatible = "..."; + reg = <0x6000 0x6100>; + }; +}; -- 1.8.1.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/2] mfd: pm8x41: add support for Qualcomm 8x41 PMICs
From: Josh Cartwright jo...@codeaurora.org The Qualcomm 8941 and 8841 PMICs are components used with the Snapdragon 800 series SoC family. This driver exists largely as a glue mfd component, it exists to be an owner of an SPMI regmap for children devices described in device tree. Signed-off-by: Josh Cartwright jo...@codeaurora.org Signed-off-by: Courtney Cavin courtney.ca...@sonymobile.com --- drivers/mfd/Kconfig | 13 +++ drivers/mfd/Makefile | 1 + drivers/mfd/pm8x41.c | 63 3 files changed, 77 insertions(+) create mode 100644 drivers/mfd/pm8x41.c diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index 3383412..f5ff799 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -502,6 +502,19 @@ config MFD_PM8921_CORE Say M here if you want to include support for PM8921 chip as a module. This will build a module called pm8921-core. +config MFD_PM8X41 + bool Qualcomm PM8x41 PMIC + depends on ARCH_QCOM + select REGMAP_SPMI + help + This enables basic support for the Qualcomm 8941 and 8841 PMICs. + These PMICs are currently used with the Snapdragon 800 series of + SoCs. Note, that this will only be useful paired with descriptions + of the independent functions as children nodes in the device tree. + + Say M here if you want to include support for the PM8x41 series as a + module. The module will be called pm8x41. + config MFD_RDC321X tristate RDC R-321x southbridge select MFD_CORE diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index 2851275..f0df41d 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -151,6 +151,7 @@ obj-$(CONFIG_MFD_SI476X_CORE) += si476x-core.o obj-$(CONFIG_MFD_CS5535) += cs5535-mfd.o obj-$(CONFIG_MFD_OMAP_USB_HOST)+= omap-usb-host.o omap-usb-tll.o obj-$(CONFIG_MFD_PM8921_CORE) += pm8921-core.o ssbi.o +obj-$(CONFIG_MFD_PM8X41) += pm8x41.o obj-$(CONFIG_TPS65911_COMPARATOR) += tps65911-comparator.o obj-$(CONFIG_MFD_TPS65090) += tps65090.o obj-$(CONFIG_MFD_AAT2870_CORE) += aat2870-core.o diff --git a/drivers/mfd/pm8x41.c b/drivers/mfd/pm8x41.c new file mode 100644 index 000..c85e0d6 --- /dev/null +++ b/drivers/mfd/pm8x41.c @@ -0,0 +1,63 @@ +/* Copyright (c) 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/module.h +#include linux/spmi.h +#include linux/regmap.h +#include linux/of_platform.h + +static const struct regmap_config pm8x41_regmap_config = { + .reg_bits = 16, + .val_bits = 8, + .max_register = 0x, +}; + +static int pm8x41_remove_child(struct device *dev, void *unused) +{ + platform_device_unregister(to_platform_device(dev)); + return 0; +} + +static void pm8x41_remove(struct spmi_device *sdev) +{ + device_for_each_child(sdev-dev, NULL, pm8x41_remove_child); +} + +static int pm8x41_probe(struct spmi_device *sdev) +{ + struct regmap *regmap; + + regmap = devm_regmap_init_spmi_ext(sdev, pm8x41_regmap_config); + if (IS_ERR(regmap)) { + dev_dbg(sdev-dev, regmap creation failed.\n); + return PTR_ERR(regmap); + } + + return of_platform_populate(sdev-dev.of_node, NULL, NULL, sdev-dev); +} + +static const struct of_device_id pm8x41_id_table[] = { + { .compatible = qcom,pm8841, }, + { .compatible = qcom,pm8941, }, + {}, +}; +MODULE_DEVICE_TABLE(of, pm8x41_id_table); + +static struct spmi_driver pm8x41_driver = { + .probe = pm8x41_probe, + .remove = pm8x41_remove, + .driver = { + .name = pm8x41, + .of_match_table = pm8x41_id_table, + }, +}; +module_spmi_driver(pm8x41_driver); -- 1.8.1.5 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] mfd: pm8x41: document device tree bindings
From: Josh Cartwright jo...@codeaurora.org Document the bindings used to describe the Qualcomm 8x41 PMICs. Signed-off-by: Josh Cartwright jo...@codeaurora.org Signed-off-by: Courtney Cavin courtney.ca...@sonymobile.com --- Documentation/devicetree/bindings/mfd/pm8x41.txt | 34 1 file changed, 34 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/pm8x41.txt diff --git a/Documentation/devicetree/bindings/mfd/pm8x41.txt b/Documentation/devicetree/bindings/mfd/pm8x41.txt new file mode 100644 index 000..b865a4f --- /dev/null +++ b/Documentation/devicetree/bindings/mfd/pm8x41.txt @@ -0,0 +1,34 @@ +Qualcomm PM8841 and PM8941 PMIC multi-function device bindings + +The PM8x41 PMICs are used with the Qualcomm Snapdragon 800 series SoCs, and are +interfaced to the chip via the SPMI (System Power Management Interface) bus. +Support for multiple independent functions are implemented by splitting the +16-bit SPMI slave address space into 256 smaller fixed-size regions, 256 bytes +each. A function can consume one or more of these fixed-size register regions. + +Required properties: +- compatible: Must be one of: + qcom,pm8841 + qcom,pm8941 +- reg: Specifies the SPMI USID slave address for this device; + See bindings/spmi/spmi.txt +- #address-cells = 1 +- #size-cells = 0 + +Each child node represents a function of the PM8x41. Each child 'reg' entry +describes an offset within the USID slave address where the region starts. + +Example: + +pm8941@0 { + compatible = qcom,pm8941; + reg = 0x0 SPMI_USID; + + #address-cells = 1; + #size-cells = 0; + + rtc@6000 { + compatible = ...; + reg = 0x6000 0x6100; + }; +}; -- 1.8.1.5 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/9] crypto: qce: Add core driver implementation
On Tue, Apr 08, 2014 at 06:26:44PM +0200, Stanimir Varbanov wrote: > On 04/04/2014 02:38 AM, Courtney Cavin wrote: > > On Thu, Apr 03, 2014 at 06:17:58PM +0200, Stanimir Varbanov wrote: > >> This adds core driver files. The core part is implementing a > >> platform driver probe and remove callbaks, the probe enables > >> clocks, checks crypto version, initialize and request dma > >> channels, create done tasklet and work queue and finally > >> register the algorithms into crypto subsystem. > >> > >> Signed-off-by: Stanimir Varbanov > >> --- > >> drivers/crypto/qce/core.c | 333 > >> ++ > >> drivers/crypto/qce/core.h | 69 ++ > >> 2 files changed, 402 insertions(+) > >> create mode 100644 drivers/crypto/qce/core.c > >> create mode 100644 drivers/crypto/qce/core.h > >> > >> diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c > > [...] > >> +static struct qce_algo_ops qce_ops[] = { > >> + { > >> + .type = CRYPTO_ALG_TYPE_ABLKCIPHER, > >> + .register_alg = qce_ablkcipher_register, > >> + }, > >> + { > >> + .type = CRYPTO_ALG_TYPE_AHASH, > >> + .register_alg = qce_ahash_register, > >> + }, > >> +}; > >> + > >> +static void qce_unregister_algs(struct qce_device *qce) > >> +{ > >> + struct qce_alg_template *tmpl, *n; > >> + > >> + list_for_each_entry_safe(tmpl, n, >alg_list, entry) { > >> + if (tmpl->crypto_alg_type == CRYPTO_ALG_TYPE_AHASH) > >> + crypto_unregister_ahash(>alg.ahash); > >> + else > >> + crypto_unregister_alg(>alg.crypto); > >> + > >> + list_del(>entry); > >> + kfree(tmpl); > > > > I find this whole memory/list management to be very disorganised. > > ops->register_alg() is supposed to allocate this item--more precisely, > > multiple items--using something that must be able to be kfree'd > > directly, register it with the crypto core, and put it on this list > > manually. Here we unregister/remove/free this in the core. Josh's > > recommendation of a unregister_alg callback might help, but it all > > remains a bit unclear with register_alg/unregister_alg managing X > > algorithms per call. > > > > Additionally, above you have qce_ops, which clearly defines the > > operations for specific algorithms types/groups, which in later patches > > are shown to be seperated out into independent implementations. > > > > From what I can tell, this seems to be a framework with built-in yet > > independent crypto implementations which call the crypto API directly. > > > > It would be more logical to me if this was seperated out into a > > "library/core" API, with the individual implementations as platform > > drivers of their own. Then they can register with the core, managing > > memory how they please. > > > > What am I missing? > > > > No, you have not miss nothing. > > OK I see your point. I made few changes in the core, killed the alg_list > and its manipulation function and added a .unregister_algs operation. > Now every type of algorithm will handle all core crypto api functions > itself. Also I'm using devm_kzalloc() in .register_algs when allocating > memory for qce_alg_template structures to avoid kfree(). The callbacks > async_req_queue/done are now embedded in qce_device structure and they > are invoked directly from algorithm implementations. Thus I have > separated the interfaces: functions implemented in core part of the > driver and struct qce_algo_ops having the function pointers implemented > by every type of algorithm. > > If you don't have some objections I can send out a version 2. Well, I'd have to see the code to understand clearly what you are describing here, but the mention of devm_kzalloc() concerns me. The only device which I currently see to which this allocation could be associated is the single platform_device in the core. Associating the memory with the core gets rid of the explicit call to kfree() by the core, but doesn't rearrange the way the memory is actually managed. If you have changed it so that each algorithm "block" has its own device, then this would seem more reasonable, but I don't see that in the explanation you provided. -Courtney -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/9] crypto: qce: Add core driver implementation
On Tue, Apr 08, 2014 at 06:26:44PM +0200, Stanimir Varbanov wrote: On 04/04/2014 02:38 AM, Courtney Cavin wrote: On Thu, Apr 03, 2014 at 06:17:58PM +0200, Stanimir Varbanov wrote: This adds core driver files. The core part is implementing a platform driver probe and remove callbaks, the probe enables clocks, checks crypto version, initialize and request dma channels, create done tasklet and work queue and finally register the algorithms into crypto subsystem. Signed-off-by: Stanimir Varbanov svarba...@mm-sol.com --- drivers/crypto/qce/core.c | 333 ++ drivers/crypto/qce/core.h | 69 ++ 2 files changed, 402 insertions(+) create mode 100644 drivers/crypto/qce/core.c create mode 100644 drivers/crypto/qce/core.h diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c [...] +static struct qce_algo_ops qce_ops[] = { + { + .type = CRYPTO_ALG_TYPE_ABLKCIPHER, + .register_alg = qce_ablkcipher_register, + }, + { + .type = CRYPTO_ALG_TYPE_AHASH, + .register_alg = qce_ahash_register, + }, +}; + +static void qce_unregister_algs(struct qce_device *qce) +{ + struct qce_alg_template *tmpl, *n; + + list_for_each_entry_safe(tmpl, n, qce-alg_list, entry) { + if (tmpl-crypto_alg_type == CRYPTO_ALG_TYPE_AHASH) + crypto_unregister_ahash(tmpl-alg.ahash); + else + crypto_unregister_alg(tmpl-alg.crypto); + + list_del(tmpl-entry); + kfree(tmpl); I find this whole memory/list management to be very disorganised. ops-register_alg() is supposed to allocate this item--more precisely, multiple items--using something that must be able to be kfree'd directly, register it with the crypto core, and put it on this list manually. Here we unregister/remove/free this in the core. Josh's recommendation of a unregister_alg callback might help, but it all remains a bit unclear with register_alg/unregister_alg managing X algorithms per call. Additionally, above you have qce_ops, which clearly defines the operations for specific algorithms types/groups, which in later patches are shown to be seperated out into independent implementations. From what I can tell, this seems to be a framework with built-in yet independent crypto implementations which call the crypto API directly. It would be more logical to me if this was seperated out into a library/core API, with the individual implementations as platform drivers of their own. Then they can register with the core, managing memory how they please. What am I missing? No, you have not miss nothing. OK I see your point. I made few changes in the core, killed the alg_list and its manipulation function and added a .unregister_algs operation. Now every type of algorithm will handle all core crypto api functions itself. Also I'm using devm_kzalloc() in .register_algs when allocating memory for qce_alg_template structures to avoid kfree(). The callbacks async_req_queue/done are now embedded in qce_device structure and they are invoked directly from algorithm implementations. Thus I have separated the interfaces: functions implemented in core part of the driver and struct qce_algo_ops having the function pointers implemented by every type of algorithm. If you don't have some objections I can send out a version 2. Well, I'd have to see the code to understand clearly what you are describing here, but the mention of devm_kzalloc() concerns me. The only device which I currently see to which this allocation could be associated is the single platform_device in the core. Associating the memory with the core gets rid of the explicit call to kfree() by the core, but doesn't rearrange the way the memory is actually managed. If you have changed it so that each algorithm block has its own device, then this would seem more reasonable, but I don't see that in the explanation you provided. -Courtney -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/9] crypto: qce: Add dma and sg helpers
On Fri, Apr 04, 2014 at 03:07:13PM +0200, Stanimir Varbanov wrote: > >> diff --git a/drivers/crypto/qce/dma.h b/drivers/crypto/qce/dma.h > >> new file mode 100644 > >> index ..932b02fd8f25 > >> --- /dev/null > >> +++ b/drivers/crypto/qce/dma.h > >> @@ -0,0 +1,57 @@ > >> +/* > >> + * Copyright (c) 2011-2014, 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. > >> + */ > >> + > >> +#ifndef _DMA_H_ > >> +#define _DMA_H_ > >> + > >> +#define QCE_AUTHIV_REGS_CNT 16 > >> +#define QCE_AUTH_BYTECOUNT_REGS_CNT 4 > >> +#define QCE_CNTRIV_REGS_CNT 4 > >> + > >> +/* result dump format */ > >> +struct qce_result_dump { > >> + u32 auth_iv[QCE_AUTHIV_REGS_CNT]; > >> + u32 auth_byte_count[QCE_AUTH_BYTECOUNT_REGS_CNT]; > >> + u32 encr_cntr_iv[QCE_CNTRIV_REGS_CNT]; > >> + u32 status; > >> + u32 status2; > >> +}; > >> + > >> +#define QCE_IGNORE_BUF_SZ (2 * QCE_BAM_BURST_SIZE) > > > > QCE_BAM_BURST_SIZE is defined in common.h in 6/9. Either that file > > needs to be included from this one, or the definition needs to be moved. > > I decided to not include any files in driver private headers. Thus I > include the private header files in relevant c files in order. Actually, that is exactly what I was trying to indicate as undesirable. Please modify this so that each individual header file doesn't require you to include another file for usage. -Courtney -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/9] crypto: qce: Add dma and sg helpers
On Fri, Apr 04, 2014 at 03:07:13PM +0200, Stanimir Varbanov wrote: diff --git a/drivers/crypto/qce/dma.h b/drivers/crypto/qce/dma.h new file mode 100644 index ..932b02fd8f25 --- /dev/null +++ b/drivers/crypto/qce/dma.h @@ -0,0 +1,57 @@ +/* + * Copyright (c) 2011-2014, 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. + */ + +#ifndef _DMA_H_ +#define _DMA_H_ + +#define QCE_AUTHIV_REGS_CNT 16 +#define QCE_AUTH_BYTECOUNT_REGS_CNT 4 +#define QCE_CNTRIV_REGS_CNT 4 + +/* result dump format */ +struct qce_result_dump { + u32 auth_iv[QCE_AUTHIV_REGS_CNT]; + u32 auth_byte_count[QCE_AUTH_BYTECOUNT_REGS_CNT]; + u32 encr_cntr_iv[QCE_CNTRIV_REGS_CNT]; + u32 status; + u32 status2; +}; + +#define QCE_IGNORE_BUF_SZ (2 * QCE_BAM_BURST_SIZE) QCE_BAM_BURST_SIZE is defined in common.h in 6/9. Either that file needs to be included from this one, or the definition needs to be moved. I decided to not include any files in driver private headers. Thus I include the private header files in relevant c files in order. Actually, that is exactly what I was trying to indicate as undesirable. Please modify this so that each individual header file doesn't require you to include another file for usage. -Courtney -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/9] crypto: qce: Add core driver implementation
On Thu, Apr 03, 2014 at 06:17:58PM +0200, Stanimir Varbanov wrote: > This adds core driver files. The core part is implementing a > platform driver probe and remove callbaks, the probe enables > clocks, checks crypto version, initialize and request dma > channels, create done tasklet and work queue and finally > register the algorithms into crypto subsystem. > > Signed-off-by: Stanimir Varbanov > --- > drivers/crypto/qce/core.c | 333 > ++ > drivers/crypto/qce/core.h | 69 ++ > 2 files changed, 402 insertions(+) > create mode 100644 drivers/crypto/qce/core.c > create mode 100644 drivers/crypto/qce/core.h > > diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c [...] > +static struct qce_algo_ops qce_ops[] = { > + { > + .type = CRYPTO_ALG_TYPE_ABLKCIPHER, > + .register_alg = qce_ablkcipher_register, > + }, > + { > + .type = CRYPTO_ALG_TYPE_AHASH, > + .register_alg = qce_ahash_register, > + }, > +}; > + > +static void qce_unregister_algs(struct qce_device *qce) > +{ > + struct qce_alg_template *tmpl, *n; > + > + list_for_each_entry_safe(tmpl, n, >alg_list, entry) { > + if (tmpl->crypto_alg_type == CRYPTO_ALG_TYPE_AHASH) > + crypto_unregister_ahash(>alg.ahash); > + else > + crypto_unregister_alg(>alg.crypto); > + > + list_del(>entry); > + kfree(tmpl); I find this whole memory/list management to be very disorganised. ops->register_alg() is supposed to allocate this item--more precisely, multiple items--using something that must be able to be kfree'd directly, register it with the crypto core, and put it on this list manually. Here we unregister/remove/free this in the core. Josh's recommendation of a unregister_alg callback might help, but it all remains a bit unclear with register_alg/unregister_alg managing X algorithms per call. Additionally, above you have qce_ops, which clearly defines the operations for specific algorithms types/groups, which in later patches are shown to be seperated out into independent implementations. >From what I can tell, this seems to be a framework with built-in yet independent crypto implementations which call the crypto API directly. It would be more logical to me if this was seperated out into a "library/core" API, with the individual implementations as platform drivers of their own. Then they can register with the core, managing memory how they please. What am I missing? > + } > +} > + > +static int qce_register_algs(struct qce_device *qce) > +{ > + struct qce_algo_ops *ops; > + int i, rc = -ENODEV; > + > + for (i = 0; i < ARRAY_SIZE(qce_ops); i++) { > + ops = _ops[i]; > + ops->async_req_queue = qce_async_request_queue; > + ops->async_req_done = qce_async_request_done; > + rc = ops->register_alg(qce, ops); > + if (rc) > + break; > + } > + > + if (rc) > + qce_unregister_algs(qce); > + > + return rc; > +} -Courtney -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/9] crypto: qce: Add dma and sg helpers
On Thu, Apr 03, 2014 at 06:18:00PM +0200, Stanimir Varbanov wrote: > This adds dmaengine and sg-list helper functions used by > other parts of the crypto driver. > > Signed-off-by: Stanimir Varbanov > --- > drivers/crypto/qce/dma.c | 201 > +++ > drivers/crypto/qce/dma.h | 57 ++ > 2 files changed, 258 insertions(+) > create mode 100644 drivers/crypto/qce/dma.c > create mode 100644 drivers/crypto/qce/dma.h More nitpicking, because everyone loves that > > diff --git a/drivers/crypto/qce/dma.c b/drivers/crypto/qce/dma.c > new file mode 100644 > index ..1bad958db2f9 > --- /dev/null > +++ b/drivers/crypto/qce/dma.c > @@ -0,0 +1,201 @@ [...] > +int qce_dma_request(struct device *dev, struct qce_dma_data *dma) > +{ > + unsigned int memsize; > + void *va; > + int ret; > + > + dma->txchan = dma_request_slave_channel_reason(dev, "tx"); > + if (IS_ERR(dma->txchan)) { > + ret = PTR_ERR(dma->txchan); > + return ret; You can just return PTR_ERR(dma->txchan) here, no need to set 'ret'. > + } > + > + dma->rxchan = dma_request_slave_channel_reason(dev, "rx"); > + if (IS_ERR(dma->rxchan)) { > + ret = PTR_ERR(dma->rxchan); > + goto error_rx; > + } > + > + memsize = QCE_RESULT_BUF_SZ + QCE_IGNORE_BUF_SZ; > + va = kzalloc(memsize, GFP_KERNEL); 'memsize' is only used here. Just pass 'QCE_RESULT_BUF_SZ + QCE_IGNORE_BUF_SZ' directly to kzalloc(). Additionally, is there some particular reason why we need to zero this memory? > + if (!va) { > + ret = -ENOMEM; > + goto error_nomem; > + } > + > + dma->result_buf = va; Is there some requirement that we don't set dma->result_buf on error? If not, we can discard the 'va' variable as well. > + dma->ignore_buf = dma->result_buf + QCE_RESULT_BUF_SZ; > + > + return 0; > +error_nomem: > + if (!IS_ERR(dma->rxchan)) > + dma_release_channel(dma->rxchan); > +error_rx: > + if (!IS_ERR(dma->txchan)) > + dma_release_channel(dma->txchan); > + return ret; > +} > + > +void qce_dma_release(struct qce_dma_data *dma) > +{ > + dma_release_channel(dma->txchan); > + dma_release_channel(dma->rxchan); > + kfree(dma->result_buf); > +} > + > +int qce_mapsg(struct device *dev, struct scatterlist *sg, unsigned int nents, > + enum dma_data_direction dir, bool chained) > +{ > + int err; > + > + if (chained) { > + while (sg) { > + err = dma_map_sg(dev, sg, 1, dir); > + if (!err) > + goto error; > + sg = scatterwalk_sg_next(sg); > + } > + } else { > + err = dma_map_sg(dev, sg, nents, dir); > + if (!err) > + goto error; > + } > + > + return nents; > +error: > + return -EFAULT; No need for this label, as there's no cleanup. Just return -EFAULT directly on error. > +} [...] > +struct scatterlist * > +qce_sgtable_add(struct sg_table *sgt, struct scatterlist *new_sgl) > +{ > + struct scatterlist *sg = sgt->sgl, *sg_last = NULL; > + > + while (sg) { > + if (!sg_page(sg)) > + break; > + sg = sg_next(sg); > + } > + > + if (!sg) > + goto error; > + > + while (new_sgl && sg) { > + sg_set_page(sg, sg_page(new_sgl), new_sgl->length, > + new_sgl->offset); > + sg_last = sg; > + sg = sg_next(sg); > + new_sgl = sg_next(new_sgl); > + } > + > + if (new_sgl) > + goto error; > + > + return sg_last; > +error: > + return ERR_PTR(-EINVAL); No need for this label, as there's no cleanup. Just return ERR_PTR(-EINVAL) directly on error. > +} > + > +static int qce_dma_prep_sg(struct dma_chan *chan, struct scatterlist *sg, > +int nents, unsigned long flags, > +enum dma_transfer_direction dir, > +dma_async_tx_callback cb, void *cb_param) > +{ > + struct dma_async_tx_descriptor *desc; > + > + if (!sg || !nents) > + return -EINVAL; > + > + desc = dmaengine_prep_slave_sg(chan, sg, nents, dir, flags); > + if (!desc) > + return -EINVAL; > + > + desc->callback = cb; > + desc->callback_param = cb_param; > + dmaengine_submit(desc); Do we not care if there is an error here? dma_cookie_t cookie; ... cookie = dmaengine_submit(desc); return dma_submit_error(cookie); > + return 0; > +} [...] > diff --git a/drivers/crypto/qce/dma.h b/drivers/crypto/qce/dma.h > new file mode 100644 > index ..932b02fd8f25 > --- /dev/null > +++ b/drivers/crypto/qce/dma.h > @@ -0,0 +1,57 @@ > +/* > + * Copyright (c) 2011-2014, The Linux Foundation. All rights reserved. > + * > + * This program is free
Re: [PATCH 3/9] crypto: qce: Add dma and sg helpers
On Thu, Apr 03, 2014 at 06:18:00PM +0200, Stanimir Varbanov wrote: This adds dmaengine and sg-list helper functions used by other parts of the crypto driver. Signed-off-by: Stanimir Varbanov svarba...@mm-sol.com --- drivers/crypto/qce/dma.c | 201 +++ drivers/crypto/qce/dma.h | 57 ++ 2 files changed, 258 insertions(+) create mode 100644 drivers/crypto/qce/dma.c create mode 100644 drivers/crypto/qce/dma.h More nitpicking, because everyone loves that diff --git a/drivers/crypto/qce/dma.c b/drivers/crypto/qce/dma.c new file mode 100644 index ..1bad958db2f9 --- /dev/null +++ b/drivers/crypto/qce/dma.c @@ -0,0 +1,201 @@ [...] +int qce_dma_request(struct device *dev, struct qce_dma_data *dma) +{ + unsigned int memsize; + void *va; + int ret; + + dma-txchan = dma_request_slave_channel_reason(dev, tx); + if (IS_ERR(dma-txchan)) { + ret = PTR_ERR(dma-txchan); + return ret; You can just return PTR_ERR(dma-txchan) here, no need to set 'ret'. + } + + dma-rxchan = dma_request_slave_channel_reason(dev, rx); + if (IS_ERR(dma-rxchan)) { + ret = PTR_ERR(dma-rxchan); + goto error_rx; + } + + memsize = QCE_RESULT_BUF_SZ + QCE_IGNORE_BUF_SZ; + va = kzalloc(memsize, GFP_KERNEL); 'memsize' is only used here. Just pass 'QCE_RESULT_BUF_SZ + QCE_IGNORE_BUF_SZ' directly to kzalloc(). Additionally, is there some particular reason why we need to zero this memory? + if (!va) { + ret = -ENOMEM; + goto error_nomem; + } + + dma-result_buf = va; Is there some requirement that we don't set dma-result_buf on error? If not, we can discard the 'va' variable as well. + dma-ignore_buf = dma-result_buf + QCE_RESULT_BUF_SZ; + + return 0; +error_nomem: + if (!IS_ERR(dma-rxchan)) + dma_release_channel(dma-rxchan); +error_rx: + if (!IS_ERR(dma-txchan)) + dma_release_channel(dma-txchan); + return ret; +} + +void qce_dma_release(struct qce_dma_data *dma) +{ + dma_release_channel(dma-txchan); + dma_release_channel(dma-rxchan); + kfree(dma-result_buf); +} + +int qce_mapsg(struct device *dev, struct scatterlist *sg, unsigned int nents, + enum dma_data_direction dir, bool chained) +{ + int err; + + if (chained) { + while (sg) { + err = dma_map_sg(dev, sg, 1, dir); + if (!err) + goto error; + sg = scatterwalk_sg_next(sg); + } + } else { + err = dma_map_sg(dev, sg, nents, dir); + if (!err) + goto error; + } + + return nents; +error: + return -EFAULT; No need for this label, as there's no cleanup. Just return -EFAULT directly on error. +} [...] +struct scatterlist * +qce_sgtable_add(struct sg_table *sgt, struct scatterlist *new_sgl) +{ + struct scatterlist *sg = sgt-sgl, *sg_last = NULL; + + while (sg) { + if (!sg_page(sg)) + break; + sg = sg_next(sg); + } + + if (!sg) + goto error; + + while (new_sgl sg) { + sg_set_page(sg, sg_page(new_sgl), new_sgl-length, + new_sgl-offset); + sg_last = sg; + sg = sg_next(sg); + new_sgl = sg_next(new_sgl); + } + + if (new_sgl) + goto error; + + return sg_last; +error: + return ERR_PTR(-EINVAL); No need for this label, as there's no cleanup. Just return ERR_PTR(-EINVAL) directly on error. +} + +static int qce_dma_prep_sg(struct dma_chan *chan, struct scatterlist *sg, +int nents, unsigned long flags, +enum dma_transfer_direction dir, +dma_async_tx_callback cb, void *cb_param) +{ + struct dma_async_tx_descriptor *desc; + + if (!sg || !nents) + return -EINVAL; + + desc = dmaengine_prep_slave_sg(chan, sg, nents, dir, flags); + if (!desc) + return -EINVAL; + + desc-callback = cb; + desc-callback_param = cb_param; + dmaengine_submit(desc); Do we not care if there is an error here? dma_cookie_t cookie; ... cookie = dmaengine_submit(desc); return dma_submit_error(cookie); + return 0; +} [...] diff --git a/drivers/crypto/qce/dma.h b/drivers/crypto/qce/dma.h new file mode 100644 index ..932b02fd8f25 --- /dev/null +++ b/drivers/crypto/qce/dma.h @@ -0,0 +1,57 @@ +/* + * Copyright (c) 2011-2014, 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
Re: [PATCH 1/9] crypto: qce: Add core driver implementation
On Thu, Apr 03, 2014 at 06:17:58PM +0200, Stanimir Varbanov wrote: This adds core driver files. The core part is implementing a platform driver probe and remove callbaks, the probe enables clocks, checks crypto version, initialize and request dma channels, create done tasklet and work queue and finally register the algorithms into crypto subsystem. Signed-off-by: Stanimir Varbanov svarba...@mm-sol.com --- drivers/crypto/qce/core.c | 333 ++ drivers/crypto/qce/core.h | 69 ++ 2 files changed, 402 insertions(+) create mode 100644 drivers/crypto/qce/core.c create mode 100644 drivers/crypto/qce/core.h diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c [...] +static struct qce_algo_ops qce_ops[] = { + { + .type = CRYPTO_ALG_TYPE_ABLKCIPHER, + .register_alg = qce_ablkcipher_register, + }, + { + .type = CRYPTO_ALG_TYPE_AHASH, + .register_alg = qce_ahash_register, + }, +}; + +static void qce_unregister_algs(struct qce_device *qce) +{ + struct qce_alg_template *tmpl, *n; + + list_for_each_entry_safe(tmpl, n, qce-alg_list, entry) { + if (tmpl-crypto_alg_type == CRYPTO_ALG_TYPE_AHASH) + crypto_unregister_ahash(tmpl-alg.ahash); + else + crypto_unregister_alg(tmpl-alg.crypto); + + list_del(tmpl-entry); + kfree(tmpl); I find this whole memory/list management to be very disorganised. ops-register_alg() is supposed to allocate this item--more precisely, multiple items--using something that must be able to be kfree'd directly, register it with the crypto core, and put it on this list manually. Here we unregister/remove/free this in the core. Josh's recommendation of a unregister_alg callback might help, but it all remains a bit unclear with register_alg/unregister_alg managing X algorithms per call. Additionally, above you have qce_ops, which clearly defines the operations for specific algorithms types/groups, which in later patches are shown to be seperated out into independent implementations. From what I can tell, this seems to be a framework with built-in yet independent crypto implementations which call the crypto API directly. It would be more logical to me if this was seperated out into a library/core API, with the individual implementations as platform drivers of their own. Then they can register with the core, managing memory how they please. What am I missing? + } +} + +static int qce_register_algs(struct qce_device *qce) +{ + struct qce_algo_ops *ops; + int i, rc = -ENODEV; + + for (i = 0; i ARRAY_SIZE(qce_ops); i++) { + ops = qce_ops[i]; + ops-async_req_queue = qce_async_request_queue; + ops-async_req_done = qce_async_request_done; + rc = ops-register_alg(qce, ops); + if (rc) + break; + } + + if (rc) + qce_unregister_algs(qce); + + return rc; +} -Courtney -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] thermal: add generic IIO channel thermal sensor driver
On Thu, Feb 27, 2014 at 07:53:34AM +0100, Zhang Rui wrote: > On Wed, 2014-02-05 at 17:43 -0800, Courtney Cavin wrote: > > This driver is a generic method for using IIO ADC channels as thermal > > sensors. > > > > Signed-off-by: Courtney Cavin > > Eduardo, > > what do you think of this patch? Apparently not very much. Oh well. Sans further input, I will submit a new series with the following changes: - Fix some return values in probe(). - Change DT binding - Submit as seperate patch in series - Reword/rename to represent a thermistor - Remove specific references to IIO, where appropriate -Courtney -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] thermal: add generic IIO channel thermal sensor driver
On Thu, Feb 27, 2014 at 07:53:34AM +0100, Zhang Rui wrote: On Wed, 2014-02-05 at 17:43 -0800, Courtney Cavin wrote: This driver is a generic method for using IIO ADC channels as thermal sensors. Signed-off-by: Courtney Cavin courtney.ca...@sonymobile.com Eduardo, what do you think of this patch? Apparently not very much. Oh well. Sans further input, I will submit a new series with the following changes: - Fix some return values in probe(). - Change DT binding - Submit as seperate patch in series - Reword/rename to represent a thermistor - Remove specific references to IIO, where appropriate -Courtney -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/3] usb: chipidea: msm: Add device tree binding information
On Wed, Feb 19, 2014 at 04:43:22PM +0100, Ivan T. Ivanov wrote: > > Hi, > > On Tue, 2014-02-18 at 13:26 -0800, Courtney Cavin wrote: > > On Tue, Feb 18, 2014 at 02:21:19PM +0100, Ivan T. Ivanov wrote: > > > From: "Ivan T. Ivanov" > > > > > > Document device tree binding information as required by > > > the Qualcomm USB controller. > > > > > > Signed-off-by: Ivan T. Ivanov > > > --- > > > .../devicetree/bindings/usb/msm-hsusb.txt | 17 > > > + > > > > Although you mentioned to Josh that this is intended for "non-standard" > > Chipidea properties, I don't see any other than requiring that 'dr_mode' > > must be "peripheral". It would seem that this should all be integrated > > into a ci3xxx.txt. > > Hm, there is no ci3xxx.txt. The closest match is ci-hdrc-imx.txt. > So it could be ci-hdrc-qcom.txt or my preferred name qcom,ci-hdrc.txt? Sorry, I was referring to ci13xxx-imx.txt, which was apparently moved to ci-hdrc-imx.txt. I was recommending to merge the two into one 'ci13xxx.txt', as this binding seems to be a new compatible for the same basic chip. Now perhaps 'ci-hdrc.txt'. Although I agree with Josh that this name should be changed, and I think either of your two suggestions would be acceptable, I would like to at least discuss the possibility of actually merging the two in this series. Comments? -Courtney -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/3] usb: chipidea: msm: Add device tree binding information
On Wed, Feb 19, 2014 at 04:43:22PM +0100, Ivan T. Ivanov wrote: Hi, On Tue, 2014-02-18 at 13:26 -0800, Courtney Cavin wrote: On Tue, Feb 18, 2014 at 02:21:19PM +0100, Ivan T. Ivanov wrote: From: Ivan T. Ivanov iiva...@mm-sol.com Document device tree binding information as required by the Qualcomm USB controller. Signed-off-by: Ivan T. Ivanov iiva...@mm-sol.com --- .../devicetree/bindings/usb/msm-hsusb.txt | 17 + Although you mentioned to Josh that this is intended for non-standard Chipidea properties, I don't see any other than requiring that 'dr_mode' must be peripheral. It would seem that this should all be integrated into a ci3xxx.txt. Hm, there is no ci3xxx.txt. The closest match is ci-hdrc-imx.txt. So it could be ci-hdrc-qcom.txt or my preferred name qcom,ci-hdrc.txt? Sorry, I was referring to ci13xxx-imx.txt, which was apparently moved to ci-hdrc-imx.txt. I was recommending to merge the two into one 'ci13xxx.txt', as this binding seems to be a new compatible for the same basic chip. Now perhaps 'ci-hdrc.txt'. Although I agree with Josh that this name should be changed, and I think either of your two suggestions would be acceptable, I would like to at least discuss the possibility of actually merging the two in this series. Comments? -Courtney -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/3] usb: chipidea: msm: Add device tree support
On Tue, Feb 18, 2014 at 07:31:55PM +0100, Sergei Shtylyov wrote: > On 02/18/2014 08:14 PM, Ivan T. Ivanov wrote: > > >>> From: "Ivan T. Ivanov" > > >>> Allows controller to be specified via device tree. > >>> Pass PHY phandle specified in DT to core driver. > > >>> Signed-off-by: Ivan T. Ivanov > >>> --- > >>>drivers/usb/chipidea/ci_hdrc_msm.c | 23 ++- > >>>1 file changed, 22 insertions(+), 1 deletion(-) > > >> You also need to describe the binding you're creating in > >> Documentation/devicetree/bindings/usb/.txt. > > > Did you check "[PATCH v2 1/3]"? > > Did you send it to 'linux-usb'? I just didn't get the patch. > (Typically, the bindings are described in the same patch the DT support is > added to the driver bu YMMV, of course.) Although I would personally agree that this is the most logical method, it would appear that the DT guys disagree with us [1]. Lately, they seem to have either given up or are otherwise preoccupied, so perhaps you can still slip a few patches by them. ;) -Courtney [1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/submitting-patches.txt -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/3] usb: chipidea: msm: Add device tree binding information
On Tue, Feb 18, 2014 at 02:21:19PM +0100, Ivan T. Ivanov wrote: > From: "Ivan T. Ivanov" > > Document device tree binding information as required by > the Qualcomm USB controller. > > Signed-off-by: Ivan T. Ivanov > --- > .../devicetree/bindings/usb/msm-hsusb.txt | 17 + Although you mentioned to Josh that this is intended for "non-standard" Chipidea properties, I don't see any other than requiring that 'dr_mode' must be "peripheral". It would seem that this should all be integrated into a ci3xxx.txt. > 1 file changed, 17 insertions(+) > > diff --git a/Documentation/devicetree/bindings/usb/msm-hsusb.txt > b/Documentation/devicetree/bindings/usb/msm-hsusb.txt > index 5ea26c6..d4e7e41 100644 > --- a/Documentation/devicetree/bindings/usb/msm-hsusb.txt > +++ b/Documentation/devicetree/bindings/usb/msm-hsusb.txt > @@ -15,3 +15,20 @@ Example EHCI controller device node: > usb-phy = <_otg>; > }; > > +CI13xxx (Chipidea) USB controllers > + > +Required properties: > +- compatible: should contain "qcom,ci-hdrc" Is there nothing more specific you could put here? Maybe a hardware revision, or something? > +- reg: offset and length of the register set in the memory map > +- interrupts: interrupt-specifier for the controller interrupt. > +- usb-phy: phandle for the PHY device > +- dr_mode: Sould be "peripheral" s/Sould/should/ -Courtney -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv3 2/6] mailbox: Introduce a new common API
On Tue, Feb 18, 2014 at 08:06:55AM +0100, Jassi Brar wrote: > On 18 February 2014 06:22, Courtney Cavin > wrote: > > On Sat, Feb 15, 2014 at 07:25:27PM +0100, Jassi Brar wrote: > > >> +request_token_t ipc_send_message(void *channel, void *mssg) > >> +{ > >> + struct ipc_chan *chan = (struct ipc_chan *)channel; > >> + request_token_t t; > >> + > >> + if (!chan || !chan->assigned) > >> + return 0; > > > > 0 is not an error in my book. Please make this more obvious, or define > > something which categorizes this as an error. > > > The returned value is index+1 of the slot assigned for message. Yeah > we could mention that in kerneldoc for the api or make it something <0 Please do. > >> +void *ipc_request_channel(struct ipc_client *cl) > >> +{ [...] > >> + spin_lock_irqsave(>lock, flags); > >> + chan->msg_free = 0; > >> + chan->msg_count = 0; > >> + chan->active_req = NULL; > >> + chan->rxcb = cl->rxcb; > >> + chan->txcb = cl->txcb; > >> + chan->cl_id = cl->cl_id; > >> + chan->assigned = true; > >> + chan->tx_block = cl->tx_block; > > > > Copying all of this data is unnecessary, and prone to error. Just point > > to 'cl' in the client struct. > > > That did occur to me. But I chose to have controller specify its > attributes from lower layer and the client from upper layer. The api > basically works on channels... which get their > functionality/limitations from controllers+clients and save them in > their own structure. That also helps 'submit message and moveon' type > clients. I understand how this currently works, but I see no reason why one couldn't simply point to the client struct here, instead of duplicating the client definition, and copying the data. I don't see an argument for fire-and-forget clients here. The lifetime of the callback and private data must persist, so why not the containing struct? > >> + if (!cl->tx_tout) > >> + chan->tx_tout = msecs_to_jiffies(360); > > > > An hour!? Arbitrary, and extremely long. Either this should be > > indefinite, or there should be a reasonably small value here. > > > Ideally the client _should_ specify the timeout. Here the default 1hr > is infinity, roughly speaking. > What value do you suggest? An hour is by definition uncountably far from infinity. The fact that this may seem like it is infinite in a test setup indicates that this will cause problems in the future, when/if the timeout occurs. I suggest using wait_for_completion() if this is 0, or alternatively a timeout short enough to trigger easily via testing (e.g. 1 second). > >> +int ipc_links_register(struct ipc_controller *ipc) > >> +{ > >> + int i, num_links, txdone; > >> + struct ipc_chan *chan; > >> + struct ipc_con *con; > >> + > >> + /* Sanity check */ > >> + if (!ipc || !ipc->ops) > > > > You probably want to check for !ipc->links here too, as you immediately > > dereference it. > > > Yeah, right. Thanks. > > >> + return -EINVAL; > >> + > >> + for (i = 0; ipc->links[i]; i++) > > > > So you require a NULL node at the end? Why not have a nlinks/num_links > > in the controller struct? It would save having to count here. > > > Rather I find that noisy. Why add variables that we can do without? > Especially when the variable would be used just once. Just like the marker link you need at the end of your array? It's more flexible, and easier to understand from an API perspective. > >> + > >> + chan = (void *)con + sizeof(*con); > >> + for (i = 0; i < num_links; i++) { > >> + chan[i].con = con; > >> + chan[i].assigned = false; > >> + chan[i].link_ops = ipc->ops; > >> + chan[i].link = ipc->links[i]; > >> + chan[i].txdone_method = txdone; > >> + chan[i].link->api_priv = [i]; > >> + spin_lock_init([i].lock); > >> + BLOCKING_INIT_NOTIFIER_HEAD([i].avail); > >> + list_add_tail([i].node, >channels); > > > > Why not have all of this data exposed in the link definition, so yo
Re: [PATCHv3 2/6] mailbox: Introduce a new common API
On Tue, Feb 18, 2014 at 08:06:55AM +0100, Jassi Brar wrote: On 18 February 2014 06:22, Courtney Cavin courtney.ca...@sonymobile.com wrote: On Sat, Feb 15, 2014 at 07:25:27PM +0100, Jassi Brar wrote: +request_token_t ipc_send_message(void *channel, void *mssg) +{ + struct ipc_chan *chan = (struct ipc_chan *)channel; + request_token_t t; + + if (!chan || !chan-assigned) + return 0; 0 is not an error in my book. Please make this more obvious, or define something which categorizes this as an error. The returned value is index+1 of the slot assigned for message. Yeah we could mention that in kerneldoc for the api or make it something 0 Please do. +void *ipc_request_channel(struct ipc_client *cl) +{ [...] + spin_lock_irqsave(chan-lock, flags); + chan-msg_free = 0; + chan-msg_count = 0; + chan-active_req = NULL; + chan-rxcb = cl-rxcb; + chan-txcb = cl-txcb; + chan-cl_id = cl-cl_id; + chan-assigned = true; + chan-tx_block = cl-tx_block; Copying all of this data is unnecessary, and prone to error. Just point to 'cl' in the client struct. That did occur to me. But I chose to have controller specify its attributes from lower layer and the client from upper layer. The api basically works on channels... which get their functionality/limitations from controllers+clients and save them in their own structure. That also helps 'submit message and moveon' type clients. I understand how this currently works, but I see no reason why one couldn't simply point to the client struct here, instead of duplicating the client definition, and copying the data. I don't see an argument for fire-and-forget clients here. The lifetime of the callback and private data must persist, so why not the containing struct? + if (!cl-tx_tout) + chan-tx_tout = msecs_to_jiffies(360); An hour!? Arbitrary, and extremely long. Either this should be indefinite, or there should be a reasonably small value here. Ideally the client _should_ specify the timeout. Here the default 1hr is infinity, roughly speaking. What value do you suggest? An hour is by definition uncountably far from infinity. The fact that this may seem like it is infinite in a test setup indicates that this will cause problems in the future, when/if the timeout occurs. I suggest using wait_for_completion() if this is 0, or alternatively a timeout short enough to trigger easily via testing (e.g. 1 second). +int ipc_links_register(struct ipc_controller *ipc) +{ + int i, num_links, txdone; + struct ipc_chan *chan; + struct ipc_con *con; + + /* Sanity check */ + if (!ipc || !ipc-ops) You probably want to check for !ipc-links here too, as you immediately dereference it. Yeah, right. Thanks. + return -EINVAL; + + for (i = 0; ipc-links[i]; i++) So you require a NULL node at the end? Why not have a nlinks/num_links in the controller struct? It would save having to count here. Rather I find that noisy. Why add variables that we can do without? Especially when the variable would be used just once. Just like the marker link you need at the end of your array? It's more flexible, and easier to understand from an API perspective. + + chan = (void *)con + sizeof(*con); + for (i = 0; i num_links; i++) { + chan[i].con = con; + chan[i].assigned = false; + chan[i].link_ops = ipc-ops; + chan[i].link = ipc-links[i]; + chan[i].txdone_method = txdone; + chan[i].link-api_priv = chan[i]; + spin_lock_init(chan[i].lock); + BLOCKING_INIT_NOTIFIER_HEAD(chan[i].avail); + list_add_tail(chan[i].node, con-channels); Why not have all of this data exposed in the link definition, so you don't need this list, and can represent it as a pre-populated array? Because the api choose to work only in terms of ipc channels. Infact in previous revision there was just one global pool of channels that the api managed. Suman Anna insisted we manage channels in controllers so it makes searching easier. Legacy reasons is not a good argument here. Clearly this code knows about the relationship between a client and a controller. Additionally, it has all the data it needs provided by the controller in an array. Why not continue using that array? + snprintf(chan[i].name, 16, %s, ipc-links[i]-link_name); + } + + mutex_lock(con_mutex); + list_add_tail(con-node, ipc_cons); + mutex_unlock(con_mutex); + + return 0; You should have
Re: [PATCH v2 1/3] usb: chipidea: msm: Add device tree binding information
On Tue, Feb 18, 2014 at 02:21:19PM +0100, Ivan T. Ivanov wrote: From: Ivan T. Ivanov iiva...@mm-sol.com Document device tree binding information as required by the Qualcomm USB controller. Signed-off-by: Ivan T. Ivanov iiva...@mm-sol.com --- .../devicetree/bindings/usb/msm-hsusb.txt | 17 + Although you mentioned to Josh that this is intended for non-standard Chipidea properties, I don't see any other than requiring that 'dr_mode' must be peripheral. It would seem that this should all be integrated into a ci3xxx.txt. 1 file changed, 17 insertions(+) diff --git a/Documentation/devicetree/bindings/usb/msm-hsusb.txt b/Documentation/devicetree/bindings/usb/msm-hsusb.txt index 5ea26c6..d4e7e41 100644 --- a/Documentation/devicetree/bindings/usb/msm-hsusb.txt +++ b/Documentation/devicetree/bindings/usb/msm-hsusb.txt @@ -15,3 +15,20 @@ Example EHCI controller device node: usb-phy = usb_otg; }; +CI13xxx (Chipidea) USB controllers + +Required properties: +- compatible: should contain qcom,ci-hdrc Is there nothing more specific you could put here? Maybe a hardware revision, or something? +- reg: offset and length of the register set in the memory map +- interrupts: interrupt-specifier for the controller interrupt. +- usb-phy: phandle for the PHY device +- dr_mode: Sould be peripheral s/Sould/should/ -Courtney -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/3] usb: chipidea: msm: Add device tree support
On Tue, Feb 18, 2014 at 07:31:55PM +0100, Sergei Shtylyov wrote: On 02/18/2014 08:14 PM, Ivan T. Ivanov wrote: From: Ivan T. Ivanov iiva...@mm-sol.com Allows controller to be specified via device tree. Pass PHY phandle specified in DT to core driver. Signed-off-by: Ivan T. Ivanov iiva...@mm-sol.com --- drivers/usb/chipidea/ci_hdrc_msm.c | 23 ++- 1 file changed, 22 insertions(+), 1 deletion(-) You also need to describe the binding you're creating in Documentation/devicetree/bindings/usb/file.txt. Did you check [PATCH v2 1/3]? Did you send it to 'linux-usb'? I just didn't get the patch. (Typically, the bindings are described in the same patch the DT support is added to the driver bu YMMV, of course.) Although I would personally agree that this is the most logical method, it would appear that the DT guys disagree with us [1]. Lately, they seem to have either given up or are otherwise preoccupied, so perhaps you can still slip a few patches by them. ;) -Courtney [1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/submitting-patches.txt -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv3 2/6] mailbox: Introduce a new common API
On Sat, Feb 15, 2014 at 07:25:27PM +0100, Jassi Brar wrote: > Introduce common framework for client/protocol drivers and > controller drivers of Inter-Processor-Communication (IPC). > > Client driver developers should have a look at > include/linux/mailbox_client.h to understand the part of > the API exposed to client drivers. > Similarly controller driver developers should have a look > at include/linux/mailbox_controller.h > > Signed-off-by: Jassi Brar > --- > drivers/mailbox/Makefile | 4 + > drivers/mailbox/mailbox.c | 534 > + > include/linux/mailbox.h| 17 ++ > include/linux/mailbox_client.h | 87 ++ > include/linux/mailbox_controller.h | 102 +++ > 5 files changed, 744 insertions(+) > create mode 100644 drivers/mailbox/mailbox.c > create mode 100644 include/linux/mailbox.h > create mode 100644 include/linux/mailbox_client.h > create mode 100644 include/linux/mailbox_controller.h > > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c > new file mode 100644 > index 000..3082f08 > --- /dev/null > +++ b/drivers/mailbox/mailbox.c > @@ -0,0 +1,534 @@ > +/* No copyright? > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ [...] > +request_token_t ipc_send_message(void *channel, void *mssg) > +{ > + struct ipc_chan *chan = (struct ipc_chan *)channel; > + request_token_t t; > + > + if (!chan || !chan->assigned) > + return 0; 0 is not an error in my book. Please make this more obvious, or define something which categorizes this as an error. > + > + t = _add_to_rbuf(chan, mssg); > + if (!t) > + pr_err("Try increasing MBOX_TX_QUEUE_LEN\n"); > + > + _msg_submit(chan); > + > + if (chan->txdone_method == TXDONE_BY_POLL) > + poll_txdone((unsigned long)chan->con); > + > + if (chan->tx_block && chan->active_req) { > + int ret; > + init_completion(>tx_complete); > + ret = wait_for_completion_timeout(>tx_complete, > + chan->tx_tout); > + if (ret == 0) { > + t = 0; > + tx_tick(chan, XFER_ERR); > + } > + } > + > + return t; > +} [...] > +void *ipc_request_channel(struct ipc_client *cl) > +{ > + struct ipc_chan *chan; > + struct ipc_con *con; > + unsigned long flags; > + char *con_name; > + int len, ret; > + > + con_name = cl->chan_name; > + len = strcspn(cl->chan_name, ":"); > + > + ret = 0; > + mutex_lock(_mutex); > + list_for_each_entry(con, _cons, node) > + if (!strncmp(con->name, con_name, len)) { > + ret = 1; > + break; > + } > + mutex_unlock(_mutex); > + > + if (!ret) { > + pr_info("Channel(%s) not found!\n", cl->chan_name); > + return NULL; ERR_PTR(-ENODEV) would be better. > + } > + > + ret = 0; > + list_for_each_entry(chan, >channels, node) { > + if (!strcmp(con_name + len + 1, chan->name) > + && !chan->assigned) { Move the !chan->assigned check first, so we can skip the strcmp if not needed. > + spin_lock_irqsave(>lock, flags); > + chan->msg_free = 0; > + chan->msg_count = 0; > + chan->active_req = NULL; > + chan->rxcb = cl->rxcb; > + chan->txcb = cl->txcb; > + chan->cl_id = cl->cl_id; > + chan->assigned = true; > + chan->tx_block = cl->tx_block; Copying all of this data is unnecessary, and prone to error. Just point to 'cl' in the client struct. > + if (!cl->tx_tout) > + chan->tx_tout = msecs_to_jiffies(360); An hour!? Arbitrary, and extremely long. Either this should be indefinite, or there should be a reasonably small value here. > + else > + chan->tx_tout = msecs_to_jiffies(cl->tx_tout); > + if (chan->txdone_method == TXDONE_BY_POLL > + && cl->knows_txdone) > + chan->txdone_method |= TXDONE_BY_ACK; > + spin_unlock_irqrestore(>lock, flags); > + ret = 1; > + break; > + } > + } > + > + if (!ret) { > + pr_err("Unable to assign mailbox(%s)\n", cl->chan_name); > + return NULL; ERR_PTR(-EBUSY) if busy, ERR_PTR(-ENODEV) if unavailable. > + } > + > + ret =
Re: [PATCHv3 2/6] mailbox: Introduce a new common API
On Sat, Feb 15, 2014 at 07:25:27PM +0100, Jassi Brar wrote: Introduce common framework for client/protocol drivers and controller drivers of Inter-Processor-Communication (IPC). Client driver developers should have a look at include/linux/mailbox_client.h to understand the part of the API exposed to client drivers. Similarly controller driver developers should have a look at include/linux/mailbox_controller.h Signed-off-by: Jassi Brar jaswinder.si...@linaro.org --- drivers/mailbox/Makefile | 4 + drivers/mailbox/mailbox.c | 534 + include/linux/mailbox.h| 17 ++ include/linux/mailbox_client.h | 87 ++ include/linux/mailbox_controller.h | 102 +++ 5 files changed, 744 insertions(+) create mode 100644 drivers/mailbox/mailbox.c create mode 100644 include/linux/mailbox.h create mode 100644 include/linux/mailbox_client.h create mode 100644 include/linux/mailbox_controller.h diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c new file mode 100644 index 000..3082f08 --- /dev/null +++ b/drivers/mailbox/mailbox.c @@ -0,0 +1,534 @@ +/* No copyright? + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ [...] +request_token_t ipc_send_message(void *channel, void *mssg) +{ + struct ipc_chan *chan = (struct ipc_chan *)channel; + request_token_t t; + + if (!chan || !chan-assigned) + return 0; 0 is not an error in my book. Please make this more obvious, or define something which categorizes this as an error. + + t = _add_to_rbuf(chan, mssg); + if (!t) + pr_err(Try increasing MBOX_TX_QUEUE_LEN\n); + + _msg_submit(chan); + + if (chan-txdone_method == TXDONE_BY_POLL) + poll_txdone((unsigned long)chan-con); + + if (chan-tx_block chan-active_req) { + int ret; + init_completion(chan-tx_complete); + ret = wait_for_completion_timeout(chan-tx_complete, + chan-tx_tout); + if (ret == 0) { + t = 0; + tx_tick(chan, XFER_ERR); + } + } + + return t; +} [...] +void *ipc_request_channel(struct ipc_client *cl) +{ + struct ipc_chan *chan; + struct ipc_con *con; + unsigned long flags; + char *con_name; + int len, ret; + + con_name = cl-chan_name; + len = strcspn(cl-chan_name, :); + + ret = 0; + mutex_lock(con_mutex); + list_for_each_entry(con, ipc_cons, node) + if (!strncmp(con-name, con_name, len)) { + ret = 1; + break; + } + mutex_unlock(con_mutex); + + if (!ret) { + pr_info(Channel(%s) not found!\n, cl-chan_name); + return NULL; ERR_PTR(-ENODEV) would be better. + } + + ret = 0; + list_for_each_entry(chan, con-channels, node) { + if (!strcmp(con_name + len + 1, chan-name) +!chan-assigned) { Move the !chan-assigned check first, so we can skip the strcmp if not needed. + spin_lock_irqsave(chan-lock, flags); + chan-msg_free = 0; + chan-msg_count = 0; + chan-active_req = NULL; + chan-rxcb = cl-rxcb; + chan-txcb = cl-txcb; + chan-cl_id = cl-cl_id; + chan-assigned = true; + chan-tx_block = cl-tx_block; Copying all of this data is unnecessary, and prone to error. Just point to 'cl' in the client struct. + if (!cl-tx_tout) + chan-tx_tout = msecs_to_jiffies(360); An hour!? Arbitrary, and extremely long. Either this should be indefinite, or there should be a reasonably small value here. + else + chan-tx_tout = msecs_to_jiffies(cl-tx_tout); + if (chan-txdone_method == TXDONE_BY_POLL +cl-knows_txdone) + chan-txdone_method |= TXDONE_BY_ACK; + spin_unlock_irqrestore(chan-lock, flags); + ret = 1; + break; + } + } + + if (!ret) { + pr_err(Unable to assign mailbox(%s)\n, cl-chan_name); + return NULL; ERR_PTR(-EBUSY) if busy, ERR_PTR(-ENODEV) if unavailable. + } + + ret = chan-link_ops-startup(chan-link, cl-link_data); + if (ret) { + pr_err(Unable to startup the
Re: [RFC 1/6] mailbox: add core framework
On Fri, Feb 14, 2014 at 08:48:25PM +0100, Arnd Bergmann wrote: > On Wednesday 12 February 2014, Courtney Cavin wrote: > > On Tue, Feb 11, 2014 at 09:35:01AM +0100, Arnd Bergmann wrote: > > > On Monday 10 February 2014 16:23:48 Courtney Cavin wrote: > > > Then again, I think that the context management stuff is the exception as > > well, > > and I think that can/should also be handled in a higher level. Regardless, > > I > > went ahead and drafted the async flags idea out anyway, so here's some > > pseudo-code. I also tried to shoe-horn in 'peek', and you can see how that > > turns out. Let me know if this is something like what you had in mind. > > The async implementation looks good to me, assuming we actually need both > sync and async operations, which I can't tell for sure. Yea, I would like some further input on that specifically. I have added Linus Walleij and Jassi Brar, who have had good input on mailboxes in the past, and somehow I missed in this series. > For the peek operation, it wouldn't work for the ethernet case, which > has to call it from atomic context in net_rx_action. It wouldn't work if the mbox is not requested with MBOX_ASYNC, but otherwise that should be fine, as it would just peek into the kfifo. That doesn't seem like a desirable method for ethernet use-case though, as it ends up being two extra copies. > > /** > > * so this is where this lock makes things difficult, as this function > > * might_sleep(), but only really because of the lock. Either we can > > * remove the lock and force the adapter to do its own locking > > * spinlock-style, or we can accept the sleep here, which seems a bit > > * stupid in a peek function. Neither option is good. Additionally, > > * there's no guarantee that the adapter doesn't operate over a bus > > * which itself might_sleep(), exacerbating the problem. > > */ > > mutex_lock(>adapter->lock); > > rc = mbox->adapter->ops->peek_message(mbox->adapter, mbox->chan, msg); > > mutex_lock(>adapter->lock); > > If we decide that peek() must not sleep, any driver that operates on a > slow bus could just always report "no data" here. Yes indeed, or it could just not implement peek, which seems reasonable. > Moving the locking into the mbox driver here sounds appropriate. I don't really like doing that for the entirety of the mbox core, as it makes the simple adapters harder to write properly. Since peek is not a typical use-case, perhaps we could remove the locking for just peek, and have a Big Fat Warning in the description of how to properly implement it? > Arnd Thanks for the input! -Courtney -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/6] mailbox: add core framework
On Fri, Feb 14, 2014 at 08:48:25PM +0100, Arnd Bergmann wrote: On Wednesday 12 February 2014, Courtney Cavin wrote: On Tue, Feb 11, 2014 at 09:35:01AM +0100, Arnd Bergmann wrote: On Monday 10 February 2014 16:23:48 Courtney Cavin wrote: Then again, I think that the context management stuff is the exception as well, and I think that can/should also be handled in a higher level. Regardless, I went ahead and drafted the async flags idea out anyway, so here's some pseudo-code. I also tried to shoe-horn in 'peek', and you can see how that turns out. Let me know if this is something like what you had in mind. The async implementation looks good to me, assuming we actually need both sync and async operations, which I can't tell for sure. Yea, I would like some further input on that specifically. I have added Linus Walleij and Jassi Brar, who have had good input on mailboxes in the past, and somehow I missed in this series. For the peek operation, it wouldn't work for the ethernet case, which has to call it from atomic context in net_rx_action. It wouldn't work if the mbox is not requested with MBOX_ASYNC, but otherwise that should be fine, as it would just peek into the kfifo. That doesn't seem like a desirable method for ethernet use-case though, as it ends up being two extra copies. /** * so this is where this lock makes things difficult, as this function * might_sleep(), but only really because of the lock. Either we can * remove the lock and force the adapter to do its own locking * spinlock-style, or we can accept the sleep here, which seems a bit * stupid in a peek function. Neither option is good. Additionally, * there's no guarantee that the adapter doesn't operate over a bus * which itself might_sleep(), exacerbating the problem. */ mutex_lock(mbox-adapter-lock); rc = mbox-adapter-ops-peek_message(mbox-adapter, mbox-chan, msg); mutex_lock(mbox-adapter-lock); If we decide that peek() must not sleep, any driver that operates on a slow bus could just always report no data here. Yes indeed, or it could just not implement peek, which seems reasonable. Moving the locking into the mbox driver here sounds appropriate. I don't really like doing that for the entirety of the mbox core, as it makes the simple adapters harder to write properly. Since peek is not a typical use-case, perhaps we could remove the locking for just peek, and have a Big Fat Warning in the description of how to properly implement it? Arnd Thanks for the input! -Courtney -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/6] mailbox: add core framework
On Tue, Feb 11, 2014 at 09:35:01AM +0100, Arnd Bergmann wrote: > On Monday 10 February 2014 16:23:48 Courtney Cavin wrote: > > > While I'm not sure the dislike of notifiers entirely justifies not using > > them here, where they seem to make sense, I can understand that they > > might not fully implement what we need to expose. > > I think we need to look at a few more examples of things that people > want to with the framework to come up with a good interface. It's > possible that we end up with multiple alternative notification > methods, but it would be good to come up with one that works well > for everyone. > > > Regarding handlers running in IRQ context: currently the API is designed > > to do just that. From the use-cases I've found, most message handlers > > simply queue something to happen at a later point. This is logical, as > > the callback will be async, so you'll need to swap contexts or add locks > > in your consumer anyway. > > Right. You may also have some handlers that need to run with extreme > latency constraints, so we need at least the option of getting the > callback from hardirq context, or possibly from softirq/tasklet > as in the dmaengine case. > > > The dma engine is large and confused, so I'm not sure entirely which > > part you are refering to. I've looked at having async completion going > > both ways, but what I see every time is code complication in both the > > adapter and in the consumers in the simple use-case. It doesn't really > > make sense to make an API which makes things so generic that it becomes > > hard to use. What I tried to follow here when designing the API was > > what I saw in the actual implementations, not what was future-proof: > > - Message receive callbacks may be called from IRQ context > > - Message send implementations may sleep > > I can imagine cases where you want to send messages from softirq > context, or from the same context in which you received the incoming > mail, so it would be good to have the API flexible enough to deal > with that. As a first step, always allowing send to sleep seems > fine. Alternatively, we could have a 'sync' flag in the send > API, to choose between "arrange this message to be sent out as > soon as possible, but don't sleep" and "send this message and > make sure it has arrived at the other end" as you do now. Although I'm not sure there's currently a requirement for this, I can see that this could be needed in the future. > > What I can do is try to alleviate implementation efforts of future > > requirements--as honestly, we can't really say exactly what they are--by > > turning the messages into structs themselves, so at a later point flags, > > ack callbacks, and rainbows can be added. I can then stop using > > notifiers, and re-invent that functionality with a mbox_ prefix. > > I don't think there is a point in reimplementing notifiers under a > different name. The question is rather how we want to deal with > the 'multiple listener' case. If this case is the exception rather > than the rule, I'd prefer making the callback API handle only > single listeners and adding higher-level abstractions for the > cases where we do need multiple listeners, but it really depends > on what people need. I wasn't actually planning on directly ripping-off notifiers under a new name. Rather, as switching to message structs is probably a good idea anyway, I don't think the notifier API properly represents that,... the void pointer is a bit vague. It could be that it would turn out as a wrapper around notifiers. As you said though, I do think this is the exception, so I'm fine with the single callback idea, as long as Rob and Suman agree that this will be suitable for their use-cases. Then again, I think that the context management stuff is the exception as well, and I think that can/should also be handled in a higher level. Regardless, I went ahead and drafted the async flags idea out anyway, so here's some pseudo-code. I also tried to shoe-horn in 'peek', and you can see how that turns out. Let me know if this is something like what you had in mind. enum { MBOX_ASYNC = BIT(0), }; struct mbox_adapter_ops { ... int (*put_message)(struct mbox_adapter *, struct mbox_channel *, const struct mbox_message *msg) int (*peek_message)(struct mbox_adapter *, struct mbox_channel *, struct mbox_message *msg) }; struct mbox; #define MBOX_FIFO_SIZE PAGE_SZ /* or not? */ struct mbox_channel { ... unsigned long flags; /* MBOX_ASYNC, for now */ struct mbox *consumer; struct kfifo_rec_ptr_1 rx_fifo; /** * probably should be allocat
Re: [RFC 1/6] mailbox: add core framework
On Tue, Feb 11, 2014 at 09:35:01AM +0100, Arnd Bergmann wrote: On Monday 10 February 2014 16:23:48 Courtney Cavin wrote: While I'm not sure the dislike of notifiers entirely justifies not using them here, where they seem to make sense, I can understand that they might not fully implement what we need to expose. I think we need to look at a few more examples of things that people want to with the framework to come up with a good interface. It's possible that we end up with multiple alternative notification methods, but it would be good to come up with one that works well for everyone. Regarding handlers running in IRQ context: currently the API is designed to do just that. From the use-cases I've found, most message handlers simply queue something to happen at a later point. This is logical, as the callback will be async, so you'll need to swap contexts or add locks in your consumer anyway. Right. You may also have some handlers that need to run with extreme latency constraints, so we need at least the option of getting the callback from hardirq context, or possibly from softirq/tasklet as in the dmaengine case. The dma engine is large and confused, so I'm not sure entirely which part you are refering to. I've looked at having async completion going both ways, but what I see every time is code complication in both the adapter and in the consumers in the simple use-case. It doesn't really make sense to make an API which makes things so generic that it becomes hard to use. What I tried to follow here when designing the API was what I saw in the actual implementations, not what was future-proof: - Message receive callbacks may be called from IRQ context - Message send implementations may sleep I can imagine cases where you want to send messages from softirq context, or from the same context in which you received the incoming mail, so it would be good to have the API flexible enough to deal with that. As a first step, always allowing send to sleep seems fine. Alternatively, we could have a 'sync' flag in the send API, to choose between arrange this message to be sent out as soon as possible, but don't sleep and send this message and make sure it has arrived at the other end as you do now. Although I'm not sure there's currently a requirement for this, I can see that this could be needed in the future. What I can do is try to alleviate implementation efforts of future requirements--as honestly, we can't really say exactly what they are--by turning the messages into structs themselves, so at a later point flags, ack callbacks, and rainbows can be added. I can then stop using notifiers, and re-invent that functionality with a mbox_ prefix. I don't think there is a point in reimplementing notifiers under a different name. The question is rather how we want to deal with the 'multiple listener' case. If this case is the exception rather than the rule, I'd prefer making the callback API handle only single listeners and adding higher-level abstractions for the cases where we do need multiple listeners, but it really depends on what people need. I wasn't actually planning on directly ripping-off notifiers under a new name. Rather, as switching to message structs is probably a good idea anyway, I don't think the notifier API properly represents that,... the void pointer is a bit vague. It could be that it would turn out as a wrapper around notifiers. As you said though, I do think this is the exception, so I'm fine with the single callback idea, as long as Rob and Suman agree that this will be suitable for their use-cases. Then again, I think that the context management stuff is the exception as well, and I think that can/should also be handled in a higher level. Regardless, I went ahead and drafted the async flags idea out anyway, so here's some pseudo-code. I also tried to shoe-horn in 'peek', and you can see how that turns out. Let me know if this is something like what you had in mind. enum { MBOX_ASYNC = BIT(0), }; struct mbox_adapter_ops { ... int (*put_message)(struct mbox_adapter *, struct mbox_channel *, const struct mbox_message *msg) int (*peek_message)(struct mbox_adapter *, struct mbox_channel *, struct mbox_message *msg) }; struct mbox; #define MBOX_FIFO_SIZE PAGE_SZ /* or not? */ struct mbox_channel { ... unsigned long flags; /* MBOX_ASYNC, for now */ struct mbox *consumer; struct kfifo_rec_ptr_1 rx_fifo; /** * probably should be allocated only if user needs to call * mbox_put_message with MBOX_ASYNC, instead of statically. */ STRUCT_KFIFO_REC_1(MBOX_FIFO_SIZE) tx_fifo; struct work_struct rx_work; struct work_struct tx_work; } struct mbox { struct mbox_channel *chan; struct mbox_adapter *adapter
Re: [RFC 1/6] mailbox: add core framework
On Mon, Feb 10, 2014 at 09:45:07PM +0100, Rob Herring wrote: > On Mon, Feb 10, 2014 at 1:59 PM, Courtney Cavin > wrote: > > On Mon, Feb 10, 2014 at 08:09:34PM +0100, Josh Cartwright wrote: > >> On Mon, Feb 10, 2014 at 11:52:05AM -0600, Rob Herring wrote: > >> > On Mon, Feb 10, 2014 at 8:11 AM, Arnd Bergmann wrote: > >> > > On Friday 07 February 2014 16:50:14 Courtney Cavin wrote: > >> [..] > >> > >> +int mbox_channel_notify(struct mbox_channel *chan, > >> > >> + const void *data, unsigned int len) > >> > >> +{ > >> > >> + return atomic_notifier_call_chain(>notifier, len, (void > >> > >> *)data); > >> > >> +} > >> > >> +EXPORT_SYMBOL(mbox_channel_notify); > >> > > > >> > > What is the reason to use a notifier chain here? Isn't a simple > >> > > callback function pointer enough? I would expect that each mailbox > >> > > can have exactly one consumer, not multiple ones. > >> > > >> > It probably can be a callback, but there can be multiple consumers. It > >> > was only a notifier on the pl320 as there was no framework at the time > >> > and to avoid creating custom interfaces between drivers. On highbank > >> > for example, we can asynchronously receive the events for temperature > >> > change, power off, and reset. So either there needs to be an event > >> > demux somewhere or callbacks have to return whether they handled an > >> > event or not. > >> > >> I'm not familiar with highbank IPC, but with these requirements should > >> the mailbox core even bother with asynchronous notifier chain? It > >> sounds like a better fit might be for the mailbox core to implement a > >> proper adapter-specific irqdomain and used a chained irq handler to > >> demux (or have consumers request with IRQF_SHARED in the shared case). > > > > Although modeling this using irqdomains makes sense for the notification > > bit, and would probably suit most adapters, there's the issue of data > > being passed around which doesn't quite fit. "Ok, I have mail... where > > is it?" Did you have something in mind for that? > > > > Frankly, I don't see the notifier chain as being extraneous or > > over-complicated here core-wise or implementation-wise, and unless I > > understand Rob incorrectly, should suit the existing use-cases. Am I > > missing something? > > Well, I think notifiers are not liked very much. I don't know that irq > handlers would be the right answer either as these are not h/w events > really and we may not want handlers to run in irq context. I would say > a callback similar to how the dma engine framework works is the right > answer. On the send side, you may want to have completion callbacks as > well. While I'm not sure the dislike of notifiers entirely justifies not using them here, where they seem to make sense, I can understand that they might not fully implement what we need to expose. Regarding handlers running in IRQ context: currently the API is designed to do just that. From the use-cases I've found, most message handlers simply queue something to happen at a later point. This is logical, as the callback will be async, so you'll need to swap contexts or add locks in your consumer anyway. The dma engine is large and confused, so I'm not sure entirely which part you are refering to. I've looked at having async completion going both ways, but what I see every time is code complication in both the adapter and in the consumers in the simple use-case. It doesn't really make sense to make an API which makes things so generic that it becomes hard to use. What I tried to follow here when designing the API was what I saw in the actual implementations, not what was future-proof: - Message receive callbacks may be called from IRQ context - Message send implementations may sleep I think that these allowances enable the simple use-case to be very easy to write, and the more complex use-cases still possible--albiet sometimes at a higher level. What I can do is try to alleviate implementation efforts of future requirements--as honestly, we can't really say exactly what they are--by turning the messages into structs themselves, so at a later point flags, ack callbacks, and rainbows can be added. I can then stop using notifiers, and re-invent that functionality with a mbox_ prefix. Comments? -Courtney -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] spi: Add Qualcomm QUP SPI controller support
On Mon, Feb 10, 2014 at 08:41:44PM +0100, Ivan T. Ivanov wrote: > > Hi, > > On Mon, 2014-02-10 at 11:47 -0600, Andy Gross wrote: > > On Mon, Feb 10, 2014 at 06:55:02PM +0200, Ivan T. Ivanov wrote: > > > > [] > > > > > > > > Bail here? > > > > > > > > > > I don't know. What will be the consequences if controller continue to > > > > > operate on its default rate? > > > > > > > > > > > > > It is unclear. But if you can't set the rate that is configured or if > > > > there is > > > > a misconfiguration, it's probably better to exit the probe and catch it > > > > here. > > > > > > > > > My preference is to delay clock speed change till first > > > SPI transfer. And use wherever transfer itself mandate. > > > > > > > That works. My only concern is that it might be nice to catch a > > configuration > > problem early rather than wait for the SPI transfer to fail continuously. > > If developer is skilled enough to know which version controller is, > (s)he will be able to put the right frequency constrain here :-) A developer doesn't have to have much skill at all to copy-paste DT configurations around and muck with numbers I agree with Andy here, early validation is a good idea here, at the very least, some sanity checks. -Courtney -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/6] mailbox: add core framework
On Mon, Feb 10, 2014 at 08:09:34PM +0100, Josh Cartwright wrote: > On Mon, Feb 10, 2014 at 11:52:05AM -0600, Rob Herring wrote: > > On Mon, Feb 10, 2014 at 8:11 AM, Arnd Bergmann wrote: > > > On Friday 07 February 2014 16:50:14 Courtney Cavin wrote: > [..] > > >> +int mbox_channel_notify(struct mbox_channel *chan, > > >> + const void *data, unsigned int len) > > >> +{ > > >> + return atomic_notifier_call_chain(>notifier, len, (void > > >> *)data); > > >> +} > > >> +EXPORT_SYMBOL(mbox_channel_notify); > > > > > > What is the reason to use a notifier chain here? Isn't a simple > > > callback function pointer enough? I would expect that each mailbox > > > can have exactly one consumer, not multiple ones. > > > > It probably can be a callback, but there can be multiple consumers. It > > was only a notifier on the pl320 as there was no framework at the time > > and to avoid creating custom interfaces between drivers. On highbank > > for example, we can asynchronously receive the events for temperature > > change, power off, and reset. So either there needs to be an event > > demux somewhere or callbacks have to return whether they handled an > > event or not. > > I'm not familiar with highbank IPC, but with these requirements should > the mailbox core even bother with asynchronous notifier chain? It > sounds like a better fit might be for the mailbox core to implement a > proper adapter-specific irqdomain and used a chained irq handler to > demux (or have consumers request with IRQF_SHARED in the shared case). Although modeling this using irqdomains makes sense for the notification bit, and would probably suit most adapters, there's the issue of data being passed around which doesn't quite fit. "Ok, I have mail... where is it?" Did you have something in mind for that? Frankly, I don't see the notifier chain as being extraneous or over-complicated here core-wise or implementation-wise, and unless I understand Rob incorrectly, should suit the existing use-cases. Am I missing something? -Courtney -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 3/6] mailbox: pl320: migrate to mbox framework
On Mon, Feb 10, 2014 at 07:28:54PM +0100, Rob Herring wrote: > On Fri, Feb 7, 2014 at 6:50 PM, Courtney Cavin > wrote: > > We don't remove the legacy methods here, but we mark them as deprecated > > in the hopes that people with the ability to properly test modifications > > can adapt its users. > > The DT for highbank is pretty much fixed at this point. So adopting > this will need a way to register without DT. Unfortunately, I don't > have access to h/w either ATM. That's fine. The lookup table stuff (see mbox_channel_lookup() and mbox_add_table()) should solve this. Hopefully we'll find someone with a test-setup for this. > I should note that this driver is very much highbank specific and not > really a generic pl320 driver. The pl320 has up to 8 mailboxes and 8 > interrupts. How it is used from there is a software decision. I've > never seen any other user, but it could be done quite differently from > how it is used in highbank. In the case of highbank, we assigned a tx > and rx mailbox. While both the management core and linux side have all > 8 interrupts wired up, we have assigned an interrupt to each side. I > suppose you could have the interrupt tied to each mailbox, but really > they are unrelated in the pl320 as each mailbox message could have > multiple targets (interrupts). Probably splitting this between a pl320 > lib and platform specific drivers would be the right split if there > are ever other users. I'm not exactly sure I follow, and I probably should lookup the pl320 spec, but from the way you describe it and the simplicity of the existing implementation, it seems to me that one could easily implement this using DT to decribe how the hardware is hooked up. > > - ipc_irq = adev->irq[0]; > > - ret = request_irq(ipc_irq, ipc_handler, 0, dev_name(>dev), > > NULL); > > + pl->adapter.dev = >dev; > > + pl->adapter.ops = _mbox_ops; > > + pl->adapter.nchannels = 1; > > Shouldn't this be 2? The 2 channels here are not a single > bi-directional channel in any way. They are completely independent and > have unrelated events. For example we originally defined having 3 > mailboxes where we had 2 tx mailboxes for fast and slow messages, but > we ultimately decided everything could be a single tx mailbox. Event > completion is handled synchronously via the pl320's handshake > mechanism. I'd imagine you could have a protocol where you have async > completions via an rx mailbox instead. I generally see a mailbox as having both rx & tx, and I thought that this was what this driver was attempting to model, so I implemented it that way. If the channels are completely independent, there's no reason why we can't model this as one rx, and one tx. The proposed API for this should be suitable. If I understand what you mean, "event completion" in this case is an ACK for each event. Async event ACKing sounds pretty dirty, and I don't really want to touch on that in the core implementation, but there should be nothing stopping you from exposing a mailbox which uses .put_message() to ACK an RX event. > Rob I'm glad you found this, as apparently I got your email address all wrong. Thanks for the comments! -Courtney -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv2 2/2] arm: Get rid of meminfo
On Mon, Feb 10, 2014 at 04:25:34AM +0100, Laura Abbott wrote: > On 2/6/2014 6:09 PM, Courtney Cavin wrote: > > On Wed, Feb 05, 2014 at 01:02:31AM +0100, Laura Abbott wrote: > >> memblock is now fully integrated into the kernel and is the prefered > >> method for tracking memory. Rather than reinvent the wheel with > >> meminfo, migrate to using memblock directly instead of meminfo as > >> an intermediate. > >> > >> Signed-off-by: Laura Abbott > > [...] > >> diff --git a/arch/arm/mach-pxa/spitz.c b/arch/arm/mach-pxa/spitz.c > >> index 0b11c1a..51d814e 100644 > >> --- a/arch/arm/mach-pxa/spitz.c > >> +++ b/arch/arm/mach-pxa/spitz.c > >> @@ -32,6 +32,7 @@ > >> #include > >> #include > >> #include > >> +#include > >> > >> #include > >> #include > >> @@ -971,13 +972,9 @@ static void __init spitz_init(void) > >> spitz_i2c_init(); > >> } > >> > >> -static void __init spitz_fixup(struct tag *tags, char **cmdline, > >> - struct meminfo *mi) > >> +static void __init spitz_fixup(struct tag *tags, char **cmdline) > >> { > >> - sharpsl_save_param(); > >> - mi->nr_banks = 1; > >> - mi->bank[0].start = 0xa000; > >> - mi->bank[0].size = (64*1024*1024); > >> + memblock_addr(0xa000, SZ_64M); > > > > memblock_add() ? > Yes, that was a typo on my part. I'll send out a v3 with collected acks. > So, I also just noticed that you specifically remove the call to sharpsl_save_param() here. I'm presuming that this is also an error, as you haven't removed it from any of the other pxa fixups, and it seems unrelated to the change. -Courtney -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/6] mailbox: add core framework
On Mon, Feb 10, 2014 at 03:11:00PM +0100, Arnd Bergmann wrote: > On Friday 07 February 2014 16:50:14 Courtney Cavin wrote: > > The mailbox drivers are fragmented, and some implement their own core. > > Unify the drivers and implement common functionality in a framework. > > > > Signed-off-by: Courtney Cavin > > This seems pretty cool overall, great to see someone getting at it@ I'm glad to hear that there's some interest! > > +static void of_mbox_adapter_add(struct mbox_adapter *adap) > > +{ > > + if (!adap->dev) > > + return; > > + > > + if (!adap->of_xlate) { > > + adap->of_xlate = of_mbox_simple_xlate; > > + adap->of_n_cells = 1; > > + } > > + > > + of_node_get(adap->dev->of_node); > > +} > > You should probably check if of_n_cells matches the device node > #mbox-cells value, otherwise the xlate function will get confused. Ok. I was under the impression that the adapter implementations would add something like that, but I see no reason why putting it here would hurt. > > + > > + mutex_lock(_lock); > > + list_add(>list, _adapters); > > + > > + of_mbox_adapter_add(adap); > > + mutex_unlock(_lock); > > + > > + return 0; > > +} > > +EXPORT_SYMBOL(mbox_adapter_add); > > Please use EXPORT_SYMBOL_GPL here and elsewhere. Ok. > > +/** > > + * mbox_channel_notify() - notify the core that a channel has a message > > + * @chan: the channel which has data > > + * @data: the location of said data > > + * @len: the length of specified data > > + * > > + * This function may be called from interrupt/no-sleep context. > > + */ > > +int mbox_channel_notify(struct mbox_channel *chan, > > + const void *data, unsigned int len) > > +{ > > + return atomic_notifier_call_chain(>notifier, len, (void *)data); > > +} > > +EXPORT_SYMBOL(mbox_channel_notify); > > What is the reason to use a notifier chain here? Isn't a simple > callback function pointer enough? I would expect that each mailbox > can have exactly one consumer, not multiple ones. Mostly because I didn't see a reason not to. While a callback function (and private data) would probably be sufficient, I don't see a specific reason why a mailbox cannot have multiple consumers, and the API currently is designed around that concept. > > +/** > > + * mbox_add_table() - add a lookup table for adapter consumers > > + * @table: array of consumers to register > > + * @num: number of consumers in array > > + */ > > +void __init mbox_add_table(struct mbox_lookup *table, unsigned int num) > > +{ > > + mutex_lock(_lookup_lock); > > + while (num--) { > > + if (table->provider && (table->dev_id || table->con_id)) > > + list_add_tail(>list, _lookup_list); > > + table++; > > + } > > + mutex_unlock(_lookup_lock); > > +} > > +EXPORT_SYMBOL(mbox_add_table); > > I don't understand this part of the API. Why do you need a separate > lookup table here? Isn't that what the DT lookup does already? It is. The lookup/table stuff here is specifically for non-DT-based mailboxes. > > +/** > > + * mbox_request() - lookup and request a MBOX channel > > + * @dev: device for channel consumer > > + * @con_id: consumer name > > + * @nb: notifier block used for receiving messages > > + * > > + * The notifier is called as atomic on new messages, so you may not sleep > > + * in the notifier callback function. > > + */ > > +struct mbox *mbox_request(struct device *dev, const char *con_id, > > + struct notifier_block *nb) > > +{ > > + struct mbox_adapter *adap; > > + struct mbox_channel *chan; > > + struct mbox *mbox; > > + int index = 0; > > + > > + if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node) > > + return of_mbox_request(dev->of_node, con_id, nb); > > What use case do you have in mind for !CONFIG_OF? None particularly, except for the existing implementations in drivers/mailbox. I simply presumed it wouldn't hurt to implement lookup tables similar to those the pwm core. > > +/** > > + * struct mbox_adapter_ops - MBOX adapter operations > > + * @put_message: hook for putting messages in the channels MBOX > > + * @request: optional hook for requesting an MBOX channel > > + * @release: optional hook for releasing an MBOX channel > > + * @owner: helps prevent removal of modules exporting active MBOX channels > > + */ > > +struct mbox_adapter_ops
Re: [RFC 1/6] mailbox: add core framework
On Mon, Feb 10, 2014 at 03:11:00PM +0100, Arnd Bergmann wrote: On Friday 07 February 2014 16:50:14 Courtney Cavin wrote: The mailbox drivers are fragmented, and some implement their own core. Unify the drivers and implement common functionality in a framework. Signed-off-by: Courtney Cavin courtney.ca...@sonymobile.com This seems pretty cool overall, great to see someone getting at it@ I'm glad to hear that there's some interest! +static void of_mbox_adapter_add(struct mbox_adapter *adap) +{ + if (!adap-dev) + return; + + if (!adap-of_xlate) { + adap-of_xlate = of_mbox_simple_xlate; + adap-of_n_cells = 1; + } + + of_node_get(adap-dev-of_node); +} You should probably check if of_n_cells matches the device node #mbox-cells value, otherwise the xlate function will get confused. Ok. I was under the impression that the adapter implementations would add something like that, but I see no reason why putting it here would hurt. + + mutex_lock(mbox_lock); + list_add(adap-list, mbox_adapters); + + of_mbox_adapter_add(adap); + mutex_unlock(mbox_lock); + + return 0; +} +EXPORT_SYMBOL(mbox_adapter_add); Please use EXPORT_SYMBOL_GPL here and elsewhere. Ok. +/** + * mbox_channel_notify() - notify the core that a channel has a message + * @chan: the channel which has data + * @data: the location of said data + * @len: the length of specified data + * + * This function may be called from interrupt/no-sleep context. + */ +int mbox_channel_notify(struct mbox_channel *chan, + const void *data, unsigned int len) +{ + return atomic_notifier_call_chain(chan-notifier, len, (void *)data); +} +EXPORT_SYMBOL(mbox_channel_notify); What is the reason to use a notifier chain here? Isn't a simple callback function pointer enough? I would expect that each mailbox can have exactly one consumer, not multiple ones. Mostly because I didn't see a reason not to. While a callback function (and private data) would probably be sufficient, I don't see a specific reason why a mailbox cannot have multiple consumers, and the API currently is designed around that concept. +/** + * mbox_add_table() - add a lookup table for adapter consumers + * @table: array of consumers to register + * @num: number of consumers in array + */ +void __init mbox_add_table(struct mbox_lookup *table, unsigned int num) +{ + mutex_lock(mbox_lookup_lock); + while (num--) { + if (table-provider (table-dev_id || table-con_id)) + list_add_tail(table-list, mbox_lookup_list); + table++; + } + mutex_unlock(mbox_lookup_lock); +} +EXPORT_SYMBOL(mbox_add_table); I don't understand this part of the API. Why do you need a separate lookup table here? Isn't that what the DT lookup does already? It is. The lookup/table stuff here is specifically for non-DT-based mailboxes. +/** + * mbox_request() - lookup and request a MBOX channel + * @dev: device for channel consumer + * @con_id: consumer name + * @nb: notifier block used for receiving messages + * + * The notifier is called as atomic on new messages, so you may not sleep + * in the notifier callback function. + */ +struct mbox *mbox_request(struct device *dev, const char *con_id, + struct notifier_block *nb) +{ + struct mbox_adapter *adap; + struct mbox_channel *chan; + struct mbox *mbox; + int index = 0; + + if (IS_ENABLED(CONFIG_OF) dev dev-of_node) + return of_mbox_request(dev-of_node, con_id, nb); What use case do you have in mind for !CONFIG_OF? None particularly, except for the existing implementations in drivers/mailbox. I simply presumed it wouldn't hurt to implement lookup tables similar to those the pwm core. +/** + * struct mbox_adapter_ops - MBOX adapter operations + * @put_message: hook for putting messages in the channels MBOX + * @request: optional hook for requesting an MBOX channel + * @release: optional hook for releasing an MBOX channel + * @owner: helps prevent removal of modules exporting active MBOX channels + */ +struct mbox_adapter_ops { + int (*put_message)(struct mbox_adapter *, struct mbox_channel *, + const void *, unsigned int); + int (*request)(struct mbox_adapter *, struct mbox_channel *); + int (*release)(struct mbox_adapter *, struct mbox_channel *); + struct module *owner; +}; I think we will need a peek_message() callback for the upcoming QMTM driver, to allow client drivers to get a message out before the mailbox driver gets an IRQ. This will be used for IRQ mitigation in the network driver. Eeek! I'm not very fond of 'peek' functions, but I guess I can see a reason for IRQ mitigation here. I still cannot help but to try to think my way out of implementing peek. What would be the callback flow here? There's
Re: [PATCHv2 2/2] arm: Get rid of meminfo
On Mon, Feb 10, 2014 at 04:25:34AM +0100, Laura Abbott wrote: On 2/6/2014 6:09 PM, Courtney Cavin wrote: On Wed, Feb 05, 2014 at 01:02:31AM +0100, Laura Abbott wrote: memblock is now fully integrated into the kernel and is the prefered method for tracking memory. Rather than reinvent the wheel with meminfo, migrate to using memblock directly instead of meminfo as an intermediate. Signed-off-by: Laura Abbott lau...@codeaurora.org [...] diff --git a/arch/arm/mach-pxa/spitz.c b/arch/arm/mach-pxa/spitz.c index 0b11c1a..51d814e 100644 --- a/arch/arm/mach-pxa/spitz.c +++ b/arch/arm/mach-pxa/spitz.c @@ -32,6 +32,7 @@ #include linux/io.h #include linux/module.h #include linux/reboot.h +#include linux/memblock.h #include asm/setup.h #include asm/mach-types.h @@ -971,13 +972,9 @@ static void __init spitz_init(void) spitz_i2c_init(); } -static void __init spitz_fixup(struct tag *tags, char **cmdline, - struct meminfo *mi) +static void __init spitz_fixup(struct tag *tags, char **cmdline) { - sharpsl_save_param(); - mi-nr_banks = 1; - mi-bank[0].start = 0xa000; - mi-bank[0].size = (64*1024*1024); + memblock_addr(0xa000, SZ_64M); memblock_add() ? Yes, that was a typo on my part. I'll send out a v3 with collected acks. So, I also just noticed that you specifically remove the call to sharpsl_save_param() here. I'm presuming that this is also an error, as you haven't removed it from any of the other pxa fixups, and it seems unrelated to the change. -Courtney -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 3/6] mailbox: pl320: migrate to mbox framework
On Mon, Feb 10, 2014 at 07:28:54PM +0100, Rob Herring wrote: On Fri, Feb 7, 2014 at 6:50 PM, Courtney Cavin courtney.ca...@sonymobile.com wrote: We don't remove the legacy methods here, but we mark them as deprecated in the hopes that people with the ability to properly test modifications can adapt its users. The DT for highbank is pretty much fixed at this point. So adopting this will need a way to register without DT. Unfortunately, I don't have access to h/w either ATM. That's fine. The lookup table stuff (see mbox_channel_lookup() and mbox_add_table()) should solve this. Hopefully we'll find someone with a test-setup for this. I should note that this driver is very much highbank specific and not really a generic pl320 driver. The pl320 has up to 8 mailboxes and 8 interrupts. How it is used from there is a software decision. I've never seen any other user, but it could be done quite differently from how it is used in highbank. In the case of highbank, we assigned a tx and rx mailbox. While both the management core and linux side have all 8 interrupts wired up, we have assigned an interrupt to each side. I suppose you could have the interrupt tied to each mailbox, but really they are unrelated in the pl320 as each mailbox message could have multiple targets (interrupts). Probably splitting this between a pl320 lib and platform specific drivers would be the right split if there are ever other users. I'm not exactly sure I follow, and I probably should lookup the pl320 spec, but from the way you describe it and the simplicity of the existing implementation, it seems to me that one could easily implement this using DT to decribe how the hardware is hooked up. - ipc_irq = adev-irq[0]; - ret = request_irq(ipc_irq, ipc_handler, 0, dev_name(adev-dev), NULL); + pl-adapter.dev = adev-dev; + pl-adapter.ops = pl320_mbox_ops; + pl-adapter.nchannels = 1; Shouldn't this be 2? The 2 channels here are not a single bi-directional channel in any way. They are completely independent and have unrelated events. For example we originally defined having 3 mailboxes where we had 2 tx mailboxes for fast and slow messages, but we ultimately decided everything could be a single tx mailbox. Event completion is handled synchronously via the pl320's handshake mechanism. I'd imagine you could have a protocol where you have async completions via an rx mailbox instead. I generally see a mailbox as having both rx tx, and I thought that this was what this driver was attempting to model, so I implemented it that way. If the channels are completely independent, there's no reason why we can't model this as one rx, and one tx. The proposed API for this should be suitable. If I understand what you mean, event completion in this case is an ACK for each event. Async event ACKing sounds pretty dirty, and I don't really want to touch on that in the core implementation, but there should be nothing stopping you from exposing a mailbox which uses .put_message() to ACK an RX event. Rob I'm glad you found this, as apparently I got your email address all wrong. Thanks for the comments! -Courtney -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/6] mailbox: add core framework
On Mon, Feb 10, 2014 at 08:09:34PM +0100, Josh Cartwright wrote: On Mon, Feb 10, 2014 at 11:52:05AM -0600, Rob Herring wrote: On Mon, Feb 10, 2014 at 8:11 AM, Arnd Bergmann a...@arndb.de wrote: On Friday 07 February 2014 16:50:14 Courtney Cavin wrote: [..] +int mbox_channel_notify(struct mbox_channel *chan, + const void *data, unsigned int len) +{ + return atomic_notifier_call_chain(chan-notifier, len, (void *)data); +} +EXPORT_SYMBOL(mbox_channel_notify); What is the reason to use a notifier chain here? Isn't a simple callback function pointer enough? I would expect that each mailbox can have exactly one consumer, not multiple ones. It probably can be a callback, but there can be multiple consumers. It was only a notifier on the pl320 as there was no framework at the time and to avoid creating custom interfaces between drivers. On highbank for example, we can asynchronously receive the events for temperature change, power off, and reset. So either there needs to be an event demux somewhere or callbacks have to return whether they handled an event or not. I'm not familiar with highbank IPC, but with these requirements should the mailbox core even bother with asynchronous notifier chain? It sounds like a better fit might be for the mailbox core to implement a proper adapter-specific irqdomain and used a chained irq handler to demux (or have consumers request with IRQF_SHARED in the shared case). Although modeling this using irqdomains makes sense for the notification bit, and would probably suit most adapters, there's the issue of data being passed around which doesn't quite fit. Ok, I have mail... where is it? Did you have something in mind for that? Frankly, I don't see the notifier chain as being extraneous or over-complicated here core-wise or implementation-wise, and unless I understand Rob incorrectly, should suit the existing use-cases. Am I missing something? -Courtney -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] spi: Add Qualcomm QUP SPI controller support
On Mon, Feb 10, 2014 at 08:41:44PM +0100, Ivan T. Ivanov wrote: Hi, On Mon, 2014-02-10 at 11:47 -0600, Andy Gross wrote: On Mon, Feb 10, 2014 at 06:55:02PM +0200, Ivan T. Ivanov wrote: [] Bail here? I don't know. What will be the consequences if controller continue to operate on its default rate? It is unclear. But if you can't set the rate that is configured or if there is a misconfiguration, it's probably better to exit the probe and catch it here. My preference is to delay clock speed change till first SPI transfer. And use wherever transfer itself mandate. That works. My only concern is that it might be nice to catch a configuration problem early rather than wait for the SPI transfer to fail continuously. If developer is skilled enough to know which version controller is, (s)he will be able to put the right frequency constrain here :-) A developer doesn't have to have much skill at all to copy-paste DT configurations around and muck with numbers I agree with Andy here, early validation is a good idea here, at the very least, some sanity checks. -Courtney -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 1/6] mailbox: add core framework
On Mon, Feb 10, 2014 at 09:45:07PM +0100, Rob Herring wrote: On Mon, Feb 10, 2014 at 1:59 PM, Courtney Cavin courtney.ca...@sonymobile.com wrote: On Mon, Feb 10, 2014 at 08:09:34PM +0100, Josh Cartwright wrote: On Mon, Feb 10, 2014 at 11:52:05AM -0600, Rob Herring wrote: On Mon, Feb 10, 2014 at 8:11 AM, Arnd Bergmann a...@arndb.de wrote: On Friday 07 February 2014 16:50:14 Courtney Cavin wrote: [..] +int mbox_channel_notify(struct mbox_channel *chan, + const void *data, unsigned int len) +{ + return atomic_notifier_call_chain(chan-notifier, len, (void *)data); +} +EXPORT_SYMBOL(mbox_channel_notify); What is the reason to use a notifier chain here? Isn't a simple callback function pointer enough? I would expect that each mailbox can have exactly one consumer, not multiple ones. It probably can be a callback, but there can be multiple consumers. It was only a notifier on the pl320 as there was no framework at the time and to avoid creating custom interfaces between drivers. On highbank for example, we can asynchronously receive the events for temperature change, power off, and reset. So either there needs to be an event demux somewhere or callbacks have to return whether they handled an event or not. I'm not familiar with highbank IPC, but with these requirements should the mailbox core even bother with asynchronous notifier chain? It sounds like a better fit might be for the mailbox core to implement a proper adapter-specific irqdomain and used a chained irq handler to demux (or have consumers request with IRQF_SHARED in the shared case). Although modeling this using irqdomains makes sense for the notification bit, and would probably suit most adapters, there's the issue of data being passed around which doesn't quite fit. Ok, I have mail... where is it? Did you have something in mind for that? Frankly, I don't see the notifier chain as being extraneous or over-complicated here core-wise or implementation-wise, and unless I understand Rob incorrectly, should suit the existing use-cases. Am I missing something? Well, I think notifiers are not liked very much. I don't know that irq handlers would be the right answer either as these are not h/w events really and we may not want handlers to run in irq context. I would say a callback similar to how the dma engine framework works is the right answer. On the send side, you may want to have completion callbacks as well. While I'm not sure the dislike of notifiers entirely justifies not using them here, where they seem to make sense, I can understand that they might not fully implement what we need to expose. Regarding handlers running in IRQ context: currently the API is designed to do just that. From the use-cases I've found, most message handlers simply queue something to happen at a later point. This is logical, as the callback will be async, so you'll need to swap contexts or add locks in your consumer anyway. The dma engine is large and confused, so I'm not sure entirely which part you are refering to. I've looked at having async completion going both ways, but what I see every time is code complication in both the adapter and in the consumers in the simple use-case. It doesn't really make sense to make an API which makes things so generic that it becomes hard to use. What I tried to follow here when designing the API was what I saw in the actual implementations, not what was future-proof: - Message receive callbacks may be called from IRQ context - Message send implementations may sleep I think that these allowances enable the simple use-case to be very easy to write, and the more complex use-cases still possible--albiet sometimes at a higher level. What I can do is try to alleviate implementation efforts of future requirements--as honestly, we can't really say exactly what they are--by turning the messages into structs themselves, so at a later point flags, ack callbacks, and rainbows can be added. I can then stop using notifiers, and re-invent that functionality with a mbox_ prefix. Comments? -Courtney -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC 2/6] mailbox: document bindings
Signed-off-by: Courtney Cavin --- .../devicetree/bindings/mailbox/mailbox.txt| 44 ++ 1 file changed, 44 insertions(+) create mode 100644 Documentation/devicetree/bindings/mailbox/mailbox.txt diff --git a/Documentation/devicetree/bindings/mailbox/mailbox.txt b/Documentation/devicetree/bindings/mailbox/mailbox.txt new file mode 100644 index 000..846eb49 --- /dev/null +++ b/Documentation/devicetree/bindings/mailbox/mailbox.txt @@ -0,0 +1,44 @@ +Binding documentation for mailbox providers and consumers +-- + +Mailbox providers may be represented by any node in a device tree. These +nodes are designated as mailbox providers. Consumers can then use a phandle +to a mailbox provider, along with channel specifier information in order to +get a mailbox. + +MAILBOX PROVIDERS + +#mbox-cells: + Usage: required + Type: u32 + Desc: Number of cells in a mailbox specifier; Typically 1 for nodes + which only need a channel index. + + +Example: + mailbox: mailbox { + #mbox-cells = <1>; + ... + }; + + +MAILBOX CONSUMERS + +mbox: + Usage: required + Type: < [phandle] [mailbox-specifier] > + Desc: List of phandle and mailbox specifier pairs, matching provider's + #mbox-cells property + +mbox-names: + Usage: optional + Type: string array + Desc: List of mailbox input name strings sorted in the same order as + the mbox property. Consumer drivers should use mbox-names + to match mailbox input names with mailbox specifiers. + +Example: + consumer { + mbox-names = "comms"; + mbox = < 0>; + }; -- 1.8.1.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC 1/6] mailbox: add core framework
The mailbox drivers are fragmented, and some implement their own core. Unify the drivers and implement common functionality in a framework. Signed-off-by: Courtney Cavin --- drivers/mailbox/Makefile | 1 + drivers/mailbox/core.c | 573 +++ include/linux/mbox.h | 175 +++ 3 files changed, 749 insertions(+) create mode 100644 drivers/mailbox/core.c create mode 100644 include/linux/mbox.h diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index e0facb3..53712ed 100644 --- a/drivers/mailbox/Makefile +++ b/drivers/mailbox/Makefile @@ -1,3 +1,4 @@ +obj-$(CONFIG_MAILBOX) += core.o obj-$(CONFIG_PL320_MBOX) += pl320-ipc.o obj-$(CONFIG_OMAP_MBOX)+= omap-mailbox.o diff --git a/drivers/mailbox/core.c b/drivers/mailbox/core.c new file mode 100644 index 000..0dc865e --- /dev/null +++ b/drivers/mailbox/core.c @@ -0,0 +1,573 @@ +/* + * Generic mailbox implementation + * + * Copyright (C) 2014 Sony Mobile Communications, AB. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope 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 +#include +#include +#include + +static DEFINE_MUTEX(mbox_lock); +static LIST_HEAD(mbox_adapters); + +static DEFINE_MUTEX(mbox_lookup_lock); +static LIST_HEAD(mbox_lookup_list); + +static int __mbox_adapter_request(struct mbox_adapter *adap, + struct mbox_channel *chan) +{ + int rc; + + if (chan->users > 0) { + chan->users++; + return 0; + } + + if (!try_module_get(adap->ops->owner)) + return -ENODEV; + + if (adap->ops->request) { + rc = adap->ops->request(adap, chan); + if (rc) { + module_put(adap->ops->owner); + return rc; + } + } + + chan->users++; + + return 0; +} + +static void __mbox_adapter_release(struct mbox_adapter *adap, + struct mbox_channel *chan) +{ + if (!adap || !chan) + return; + + if (chan->users == 0) { + dev_err(adap->dev, "device already released\n"); + return; + } + + chan->users--; + if (chan->users > 0) + return; + + if (adap->ops->release) + adap->ops->release(adap, chan); + module_put(adap->ops->owner); +} + +static struct mbox_channel * +mbox_adapter_request_channel(struct mbox_adapter *adap, unsigned int index) +{ + struct mbox_channel *chan; + int rc; + + if (!adap || index >= adap->nchannels) + return ERR_PTR(-EINVAL); + + mutex_lock(>lock); + chan = >channels[index]; + + rc = __mbox_adapter_request(adap, chan); + if (rc) + chan = ERR_PTR(rc); + mutex_unlock(>lock); + + return chan; +} + +static void mbox_adapter_release_channel(struct mbox_adapter *adap, + struct mbox_channel *chan) +{ + if (!adap || !chan) + return; + + mutex_lock(>lock); + __mbox_adapter_release(adap, chan); + mutex_unlock(>lock); +} + +static int of_mbox_simple_xlate(struct mbox_adapter *adap, + const struct of_phandle_args *args) +{ + if (adap->of_n_cells < 1) + return -EINVAL; + if (args->args[0] >= adap->nchannels) + return -EINVAL; + + return args->args[0]; +} + +static struct mbox_adapter *of_node_to_mbox_adapter(struct device_node *np) +{ + struct mbox_adapter *adap; + + mutex_lock(_lock); + list_for_each_entry(adap, _adapters, list) { + if (adap->dev && adap->dev->of_node == np) { + mutex_unlock(_lock); + return adap; + } + } + mutex_unlock(_lock); + + return ERR_PTR(-EPROBE_DEFER); +} + +static void of_mbox_adapter_add(struct mbox_adapter *adap) +{ + if (!adap->dev) + return; + + if (!adap->of_xlate) { + adap->of_xlate = of_mbox_simple_xlate; + adap->of_n_cells = 1; + } + + of_node_get(adap->dev->of_node); +} + +static void of_mbox_adapter_remove(struct mbox_adapter *adap) +{ + if (!adap->dev) + return; + of_node_put(adap->dev->of_node); +} + +static struct mbox_channel * +of_mbox_adapter_request_channel(struct device_node *np, const
[RFC 6/6] mailbox: omap2+: move to common mbox framework
Signed-off-by: Courtney Cavin --- drivers/mailbox/Kconfig | 1 - drivers/mailbox/mailbox-omap2.c | 315 +--- 2 files changed, 132 insertions(+), 184 deletions(-) diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index ae6b09b..a592a5a 100644 --- a/drivers/mailbox/Kconfig +++ b/drivers/mailbox/Kconfig @@ -27,7 +27,6 @@ config OMAP1_MBOX config OMAP2PLUS_MBOX tristate "OMAP2+ Mailbox framework support" depends on ARCH_OMAP2PLUS - depends on BROKEN help Mailbox implementation for OMAP family chips with hardware for interprocessor communication involving DSP, IVA1.0 and IVA2 in diff --git a/drivers/mailbox/mailbox-omap2.c b/drivers/mailbox/mailbox-omap2.c index 42d2b89..7ddde19 100644 --- a/drivers/mailbox/mailbox-omap2.c +++ b/drivers/mailbox/mailbox-omap2.c @@ -18,8 +18,8 @@ #include #include #include - -#include "omap-mbox.h" +#include +#include #define MAILBOX_REVISION 0x000 #define MAILBOX_MESSAGE(m) (0x040 + 4 * (m)) @@ -42,192 +42,165 @@ #define MBOX_NR_REGS (MBOX_REG_SIZE / sizeof(u32)) #define OMAP4_MBOX_NR_REGS (OMAP4_MBOX_REG_SIZE / sizeof(u32)) -static void __iomem *mbox_base; - struct omap_mbox2_fifo { unsigned long msg; unsigned long fifo_stat; unsigned long msg_stat; }; +struct omap2_mbox; + struct omap_mbox2_priv { + struct omap2_mbox *mbox; + int irq; + struct omap_mbox2_fifo tx_fifo; struct omap_mbox2_fifo rx_fifo; unsigned long irqenable; unsigned long irqstatus; u32 newmsg_bit; u32 notfull_bit; - u32 ctx[OMAP4_MBOX_NR_REGS]; unsigned long irqdisable; u32 intr_type; }; -static inline unsigned int mbox_read_reg(size_t ofs) -{ - return __raw_readl(mbox_base + ofs); -} +struct omap2_mbox { + struct mbox_adapter adapter; + struct completion completion; + void __iomem *base; + atomic_t active; + struct omap_mbox2_priv *priv; +}; -static inline void mbox_write_reg(u32 val, size_t ofs) +static inline unsigned int mbox_read_reg(void __iomem *base, size_t ofs) { - __raw_writel(val, mbox_base + ofs); + return __raw_readl(base + ofs); } -/* Mailbox H/W preparations */ -static int omap2_mbox_startup(struct omap_mbox *mbox) +static inline void mbox_write_reg(void __iomem *base, u32 val, size_t ofs) { - u32 l; - - pm_runtime_enable(mbox->dev->parent); - pm_runtime_get_sync(mbox->dev->parent); - - l = mbox_read_reg(MAILBOX_REVISION); - pr_debug("omap mailbox rev %d.%d\n", (l & 0xf0) >> 4, (l & 0x0f)); - - return 0; + __raw_writel(val, base + ofs); } -static void omap2_mbox_shutdown(struct omap_mbox *mbox) +static int omap2_mbox_request(struct mbox_adapter *adap, + struct mbox_channel *chan) { - pm_runtime_put_sync(mbox->dev->parent); - pm_runtime_disable(mbox->dev->parent); -} + struct omap_mbox2_priv *p; + struct omap2_mbox *mbox; + u32 enable; -/* Mailbox FIFO handle functions */ -static mbox_msg_t omap2_mbox_fifo_read(struct omap_mbox *mbox) -{ - struct omap_mbox2_fifo *fifo = - &((struct omap_mbox2_priv *)mbox->priv)->rx_fifo; - return (mbox_msg_t) mbox_read_reg(fifo->msg); -} + mbox = container_of(adap, struct omap2_mbox, adapter); + p = >priv[chan->id]; -static void omap2_mbox_fifo_write(struct omap_mbox *mbox, mbox_msg_t msg) -{ - struct omap_mbox2_fifo *fifo = - &((struct omap_mbox2_priv *)mbox->priv)->tx_fifo; - mbox_write_reg(msg, fifo->msg); -} + if (atomic_inc_return(>active) == 1) { + pm_runtime_enable(adap->dev); + pm_runtime_get_sync(adap->dev); + } -static int omap2_mbox_fifo_empty(struct omap_mbox *mbox) -{ - struct omap_mbox2_fifo *fifo = - &((struct omap_mbox2_priv *)mbox->priv)->rx_fifo; - return (mbox_read_reg(fifo->msg_stat) == 0); -} + enable = mbox_read_reg(mbox->base, p->irqenable); + enable |= p->notfull_bit | p->newmsg_bit; + mbox_write_reg(mbox->base, enable, p->irqenable); -static int omap2_mbox_fifo_full(struct omap_mbox *mbox) -{ - struct omap_mbox2_fifo *fifo = - &((struct omap_mbox2_priv *)mbox->priv)->tx_fifo; - return mbox_read_reg(fifo->fifo_stat); + return 0; } -/* Mailbox IRQ handle functions */ -static void omap2_mbox_enable_irq(struct omap_mbox *mbox, omap_mbox_irq_t irq) +static int omap2_mbox_release(struct mbox_adapter *adap, + struct mbox_channel *chan) { - struct omap_mbox2_priv *p = mbox->priv; - u32 l, bit = (irq == IRQ_TX) ? p->notfull_bit : p-&g
[RFC 4/6] mailbox: omap: remove omap-specific framework
This framework is no longer needed, and the users should move over to the new common framework. Mark the existing implementations as broken, and deprecate the api, as a stop-gap. Signed-off-by: Courtney Cavin --- drivers/mailbox/Kconfig| 19 +- drivers/mailbox/Makefile | 1 - drivers/mailbox/omap-mailbox.c | 469 - drivers/mailbox/omap-mbox.h| 67 -- include/linux/omap-mailbox.h | 45 +++- 5 files changed, 40 insertions(+), 561 deletions(-) delete mode 100644 drivers/mailbox/omap-mailbox.c delete mode 100644 drivers/mailbox/omap-mbox.h diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index c8b5c13..6befc6e 100644 --- a/drivers/mailbox/Kconfig +++ b/drivers/mailbox/Kconfig @@ -16,17 +16,10 @@ config PL320_MBOX Management Engine, primarily for cpufreq. Say Y here if you want to use the PL320 IPCM support. -config OMAP_MBOX - tristate - help - This option is selected by any OMAP architecture specific mailbox - driver such as CONFIG_OMAP1_MBOX or CONFIG_OMAP2PLUS_MBOX. This - enables the common OMAP mailbox framework code. - config OMAP1_MBOX tristate "OMAP1 Mailbox framework support" depends on ARCH_OMAP1 - select OMAP_MBOX + depends on BROKEN help Mailbox implementation for OMAP chips with hardware for interprocessor communication involving DSP in OMAP1. Say Y here @@ -35,19 +28,11 @@ config OMAP1_MBOX config OMAP2PLUS_MBOX tristate "OMAP2+ Mailbox framework support" depends on ARCH_OMAP2PLUS - select OMAP_MBOX + depends on BROKEN help Mailbox implementation for OMAP family chips with hardware for interprocessor communication involving DSP, IVA1.0 and IVA2 in OMAP2/3; or IPU, IVA HD and DSP in OMAP4/5. Say Y here if you want to use OMAP2+ Mailbox framework support. -config OMAP_MBOX_KFIFO_SIZE - int "Mailbox kfifo default buffer size (bytes)" - depends on OMAP2PLUS_MBOX || OMAP1_MBOX - default 256 - help - Specify the default size of mailbox's kfifo buffers (bytes). - This can also be changed at runtime (via the mbox_kfifo_size - module parameter). endif diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index 53712ed..c8e51a0 100644 --- a/drivers/mailbox/Makefile +++ b/drivers/mailbox/Makefile @@ -1,7 +1,6 @@ obj-$(CONFIG_MAILBOX) += core.o obj-$(CONFIG_PL320_MBOX) += pl320-ipc.o -obj-$(CONFIG_OMAP_MBOX)+= omap-mailbox.o obj-$(CONFIG_OMAP1_MBOX) += mailbox_omap1.o mailbox_omap1-objs := mailbox-omap1.o obj-$(CONFIG_OMAP2PLUS_MBOX) += mailbox_omap2.o diff --git a/drivers/mailbox/omap-mailbox.c b/drivers/mailbox/omap-mailbox.c deleted file mode 100644 index d79a646..000 --- a/drivers/mailbox/omap-mailbox.c +++ /dev/null @@ -1,469 +0,0 @@ -/* - * OMAP mailbox driver - * - * Copyright (C) 2006-2009 Nokia Corporation. All rights reserved. - * - * Contact: Hiroshi DOYU - * - * This program is free software; you can redistribute it and/or - * modify it under the terms of the GNU General Public License - * 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. - * - * You should have received a copy of the GNU General Public License - * along with this program; if not, write to the Free Software - * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA - * 02110-1301 USA - * - */ - -#include -#include -#include -#include -#include -#include -#include -#include -#include - -#include "omap-mbox.h" - -static struct omap_mbox **mboxes; - -static int mbox_configured; -static DEFINE_MUTEX(mbox_configured_lock); - -static unsigned int mbox_kfifo_size = CONFIG_OMAP_MBOX_KFIFO_SIZE; -module_param(mbox_kfifo_size, uint, S_IRUGO); -MODULE_PARM_DESC(mbox_kfifo_size, "Size of omap's mailbox kfifo (bytes)"); - -/* Mailbox FIFO handle functions */ -static inline mbox_msg_t mbox_fifo_read(struct omap_mbox *mbox) -{ - return mbox->ops->fifo_read(mbox); -} -static inline void mbox_fifo_write(struct omap_mbox *mbox, mbox_msg_t msg) -{ - mbox->ops->fifo_write(mbox, msg); -} -static inline int mbox_fifo_empty(struct omap_mbox *mbox) -{ - return mbox->ops->fifo_empty(mbox); -} -static inline int mbox_fifo_full(struct omap_mbox *mbox) -{ - return mbox->ops->fifo_full(mbox); -} - -/* Mailbox IRQ handle functions */ -static inline void ack_mbox_irq(struct omap_mbox *mbox, omap_mbox_irq_t irq) -{ - if (mbox->ops->ack_irq) - mbox->ops->ack_irq(mbo
[RFC 3/6] mailbox: pl320: migrate to mbox framework
We don't remove the legacy methods here, but we mark them as deprecated in the hopes that people with the ability to properly test modifications can adapt its users. Signed-off-by: Courtney Cavin --- drivers/mailbox/pl320-ipc.c | 258 ++-- include/linux/mailbox.h | 29 - 2 files changed, 225 insertions(+), 62 deletions(-) diff --git a/drivers/mailbox/pl320-ipc.c b/drivers/mailbox/pl320-ipc.c index d873cba..b8da247 100644 --- a/drivers/mailbox/pl320-ipc.c +++ b/drivers/mailbox/pl320-ipc.c @@ -15,7 +15,6 @@ */ #include #include -#include #include #include #include @@ -27,6 +26,7 @@ #include #include +#include #define IPCMxSOURCE(m) ((m) * 0x40) #define IPCMxDSET(m) (((m) * 0x40) + 0x004) @@ -50,131 +50,162 @@ #define A9_SOURCE 1 #define M3_SOURCE 0 -static void __iomem *ipc_base; -static int ipc_irq; -static DEFINE_MUTEX(ipc_m1_lock); -static DECLARE_COMPLETION(ipc_completion); -static ATOMIC_NOTIFIER_HEAD(ipc_notifier); +struct pl320 { + struct mbox_adapter adapter; + void __iomem *base; + int irq; + struct completion completion; +}; -static inline void set_destination(int source, int mbox) +static inline void set_destination(struct pl320 *pl, int source, int mbox) { - __raw_writel(CHAN_MASK(source), ipc_base + IPCMxDSET(mbox)); - __raw_writel(CHAN_MASK(source), ipc_base + IPCMxMSET(mbox)); + __raw_writel(CHAN_MASK(source), pl->base + IPCMxDSET(mbox)); + __raw_writel(CHAN_MASK(source), pl->base + IPCMxMSET(mbox)); } -static inline void clear_destination(int source, int mbox) +static inline void clear_destination(struct pl320 *pl, int source, int mbox) { - __raw_writel(CHAN_MASK(source), ipc_base + IPCMxDCLEAR(mbox)); - __raw_writel(CHAN_MASK(source), ipc_base + IPCMxMCLEAR(mbox)); + __raw_writel(CHAN_MASK(source), pl->base + IPCMxDCLEAR(mbox)); + __raw_writel(CHAN_MASK(source), pl->base + IPCMxMCLEAR(mbox)); } -static void __ipc_send(int mbox, u32 *data) +static void __ipc_send(struct pl320 *pl, int mbox, const u32 *data) { int i; for (i = 0; i < 7; i++) - __raw_writel(data[i], ipc_base + IPCMxDR(mbox, i)); - __raw_writel(0x1, ipc_base + IPCMxSEND(mbox)); + __raw_writel(data[i], pl->base + IPCMxDR(mbox, i)); + __raw_writel(0x1, pl->base + IPCMxSEND(mbox)); } -static u32 __ipc_rcv(int mbox, u32 *data) +static u32 __ipc_rcv(struct pl320 *pl, int mbox, u32 *data) { int i; for (i = 0; i < 7; i++) - data[i] = __raw_readl(ipc_base + IPCMxDR(mbox, i)); + data[i] = __raw_readl(pl->base + IPCMxDR(mbox, i)); return data[1]; } /* blocking implmentation from the A9 side, not usuable in interrupts! */ -int pl320_ipc_transmit(u32 *data) +static int pl320_ipc_put_message(struct mbox_adapter *adap, + struct mbox_channel *chan, const void *data, unsigned int len) { + struct pl320 *pl; + u32 repl[7]; int ret; - mutex_lock(_m1_lock); + if (len != 28 || chan->id != 0) + return -EINVAL; - init_completion(_completion); - __ipc_send(IPC_TX_MBOX, data); - ret = wait_for_completion_timeout(_completion, + pl = container_of(adap, struct pl320, adapter); + reinit_completion(>completion); + __ipc_send(pl, IPC_TX_MBOX, data); + ret = wait_for_completion_timeout(>completion, msecs_to_jiffies(1000)); - if (ret == 0) { - ret = -ETIMEDOUT; - goto out; - } + if (ret == 0) + return -ETIMEDOUT; + + ret = __ipc_rcv(pl, IPC_TX_MBOX, repl); - ret = __ipc_rcv(IPC_TX_MBOX, data); -out: - mutex_unlock(_m1_lock); return ret; } -EXPORT_SYMBOL_GPL(pl320_ipc_transmit); static irqreturn_t ipc_handler(int irq, void *dev) { + struct pl320 *pl = dev; u32 irq_stat; u32 data[7]; - irq_stat = __raw_readl(ipc_base + IPCMMIS(1)); + irq_stat = __raw_readl(pl->base + IPCMMIS(1)); if (irq_stat & MBOX_MASK(IPC_TX_MBOX)) { - __raw_writel(0, ipc_base + IPCMxSEND(IPC_TX_MBOX)); - complete(_completion); + __raw_writel(0, pl->base + IPCMxSEND(IPC_TX_MBOX)); + complete(>completion); } if (irq_stat & MBOX_MASK(IPC_RX_MBOX)) { - __ipc_rcv(IPC_RX_MBOX, data); - atomic_notifier_call_chain(_notifier, data[0], data + 1); - __raw_writel(2, ipc_base + IPCMxSEND(IPC_RX_MBOX)); + __ipc_rcv(pl, IPC_RX_MBOX, data); + mbox_channel_notify(>adapter.channels[0], data, 28); + __raw_writel(2, pl->base + IPCMxSEND(IPC_RX_MBOX)); } return IRQ_HANDLED; }
[RFC 5/6] mailbox: omap1: move to common mbox framework
Signed-off-by: Courtney Cavin --- drivers/mailbox/Kconfig | 1 - drivers/mailbox/mailbox-omap1.c | 153 +++- 2 files changed, 73 insertions(+), 81 deletions(-) diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index 6befc6e..ae6b09b 100644 --- a/drivers/mailbox/Kconfig +++ b/drivers/mailbox/Kconfig @@ -19,7 +19,6 @@ config PL320_MBOX config OMAP1_MBOX tristate "OMAP1 Mailbox framework support" depends on ARCH_OMAP1 - depends on BROKEN help Mailbox implementation for OMAP chips with hardware for interprocessor communication involving DSP in OMAP1. Say Y here diff --git a/drivers/mailbox/mailbox-omap1.c b/drivers/mailbox/mailbox-omap1.c index 9001b76..474890d 100644 --- a/drivers/mailbox/mailbox-omap1.c +++ b/drivers/mailbox/mailbox-omap1.c @@ -12,10 +12,10 @@ #include #include #include +#include +#include #include -#include "omap-mbox.h" - #define MAILBOX_ARM2DSP1 0x00 #define MAILBOX_ARM2DSP1b 0x04 #define MAILBOX_DSP2ARM1 0x08 @@ -26,8 +26,6 @@ #define MAILBOX_DSP2ARM1_Flag 0x1c #define MAILBOX_DSP2ARM2_Flag 0x20 -static void __iomem *mbox_base; - struct omap_mbox1_fifo { unsigned long cmd; unsigned long data; @@ -39,90 +37,70 @@ struct omap_mbox1_priv { struct omap_mbox1_fifo rx_fifo; }; -static inline int mbox_read_reg(size_t ofs) +struct omap1_mbox { + struct mbox_adapter adapter; + struct omap_mbox1_priv priv; + void __iomem *base; + int irq; +}; + +static inline int mbox_read_reg(void __iomem *base, size_t ofs) { - return __raw_readw(mbox_base + ofs); + return __raw_readw(base + ofs); } -static inline void mbox_write_reg(u32 val, size_t ofs) +static inline void mbox_write_reg(void __iomem *base, u32 val, size_t ofs) { - __raw_writew(val, mbox_base + ofs); + __raw_writew(val, base + ofs); } -/* msg */ -static mbox_msg_t omap1_mbox_fifo_read(struct omap_mbox *mbox) +static int omap1_mbox_put_message(struct mbox_adapter *adap, + struct mbox_channel *chan, const void *data, unsigned int len) + { - struct omap_mbox1_fifo *fifo = - &((struct omap_mbox1_priv *)mbox->priv)->rx_fifo; - mbox_msg_t msg; + struct omap_mbox1_fifo *fifo; + struct omap1_mbox *mbox; + u32 msg; + int i; - msg = mbox_read_reg(fifo->data); - msg |= ((mbox_msg_t) mbox_read_reg(fifo->cmd)) << 16; + if (len != sizeof(msg)) + return -EINVAL; - return msg; -} + msg = ((u32 *)data)[0]; + mbox = container_of(adap, struct omap1_mbox, adapter); + fifo = >priv.tx_fifo; -static void -omap1_mbox_fifo_write(struct omap_mbox *mbox, mbox_msg_t msg) -{ - struct omap_mbox1_fifo *fifo = - &((struct omap_mbox1_priv *)mbox->priv)->tx_fifo; + /* wait for available space */ + for (i = 0; i < 100 && mbox_read_reg(mbox->base, fifo->flag); ++i) + usleep_range(10, 20); + if (i == 100) + return -ETIMEDOUT; - mbox_write_reg(msg & 0x, fifo->data); - mbox_write_reg(msg >> 16, fifo->cmd); -} + mbox_write_reg(mbox->base, msg & 0x, fifo->data); + mbox_write_reg(mbox->base, msg >> 16, fifo->cmd); -static int omap1_mbox_fifo_empty(struct omap_mbox *mbox) -{ return 0; } -static int omap1_mbox_fifo_full(struct omap_mbox *mbox) +static irqreturn_t omap1_mbox_irq(int irq, void *dev) { - struct omap_mbox1_fifo *fifo = - &((struct omap_mbox1_priv *)mbox->priv)->rx_fifo; + struct omap1_mbox *mbox = dev; + struct omap_mbox1_fifo *fifo; + u32 msg; - return mbox_read_reg(fifo->flag); -} + fifo = >priv.rx_fifo; + msg = mbox_read_reg(mbox->base, fifo->data); + msg |= ((u32)mbox_read_reg(mbox->base, fifo->cmd)) << 16; -/* irq */ -static void -omap1_mbox_enable_irq(struct omap_mbox *mbox, omap_mbox_irq_t irq) -{ - if (irq == IRQ_RX) - enable_irq(mbox->irq); -} + mbox_channel_notify(>adapter.channels[0], , sizeof(msg)); -static void -omap1_mbox_disable_irq(struct omap_mbox *mbox, omap_mbox_irq_t irq) -{ - if (irq == IRQ_RX) - disable_irq(mbox->irq); + return IRQ_HANDLED; } -static int -omap1_mbox_is_irq(struct omap_mbox *mbox, omap_mbox_irq_t irq) -{ - if (irq == IRQ_TX) - return 0; - return 1; -} - -static struct omap_mbox_ops omap1_mbox_ops = { - .type = OMAP_MBOX_TYPE1, - .fifo_read = omap1_mbox_fifo_read, - .fifo_write = omap1_mbox_fifo_write, - .fifo_empty = omap1_mbox_fifo_empty, - .fifo_full = omap1_mbox_
[RFC 0/6] mailbox: add common framework and port drivers
There is currently no common framework for mailbox drivers, so this is my attempt to come up with something suitable. There seems to be a need for making this generic, so I have attempted to do just that. Most of this is modeled pretty strongly after the pwm core, with some influences from the clock core. Looking at the existing use-cases, and some new ones, it would appear that the requirements here are rather simple. We need essentially two things for consumers: - put_message - callback for receiving messages The code currently uses atomic notifiers for callbacks. The common omap core deals with fifos and work-queues in order to escape atomic contexts, but from what I can see, this is unneeded. I am also of the opinion that the contexts can be much better managed in the drivers which are working with these contexts, rather than generically. Hopefully this will be suitable for the plethora of other drivers around the kernel which implement mailboxes, as well. In any case, I'm rather interested to see what the rest of the world thinks. Keep in mind that while the pl320 & omap code should compile, I don't currently have a platform on which I can perform proper testing. I also removed the context save/restore code from omap2 mailbox support, because I think it should be able to be done via driver suspend/resume, but haven't done a full investigation just yet. I'm also aware that breaking omap, just to fix it again probably isn't the best course of action, and I'm open to suggestions. -Courtney Courtney Cavin (6): mailbox: add core framework mailbox: document bindings mailbox: pl320: migrate to mbox framework mailbox: omap: remove omap-specific framework mailbox: omap1: move to common mbox framework mailbox: omap2+: move to common mbox framework .../devicetree/bindings/mailbox/mailbox.txt| 44 ++ drivers/mailbox/Kconfig| 17 - drivers/mailbox/Makefile | 2 +- drivers/mailbox/core.c | 573 + drivers/mailbox/mailbox-omap1.c| 153 +++--- drivers/mailbox/mailbox-omap2.c| 315 +-- drivers/mailbox/omap-mailbox.c | 469 - drivers/mailbox/omap-mbox.h| 67 --- drivers/mailbox/pl320-ipc.c| 258 +++--- include/linux/mailbox.h| 29 +- include/linux/mbox.h | 175 +++ include/linux/omap-mailbox.h | 45 +- 12 files changed, 1261 insertions(+), 886 deletions(-) create mode 100644 Documentation/devicetree/bindings/mailbox/mailbox.txt create mode 100644 drivers/mailbox/core.c delete mode 100644 drivers/mailbox/omap-mailbox.c delete mode 100644 drivers/mailbox/omap-mbox.h create mode 100644 include/linux/mbox.h -- 1.8.1.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC 0/6] mailbox: add common framework and port drivers
There is currently no common framework for mailbox drivers, so this is my attempt to come up with something suitable. There seems to be a need for making this generic, so I have attempted to do just that. Most of this is modeled pretty strongly after the pwm core, with some influences from the clock core. Looking at the existing use-cases, and some new ones, it would appear that the requirements here are rather simple. We need essentially two things for consumers: - put_message - callback for receiving messages The code currently uses atomic notifiers for callbacks. The common omap core deals with fifos and work-queues in order to escape atomic contexts, but from what I can see, this is unneeded. I am also of the opinion that the contexts can be much better managed in the drivers which are working with these contexts, rather than generically. Hopefully this will be suitable for the plethora of other drivers around the kernel which implement mailboxes, as well. In any case, I'm rather interested to see what the rest of the world thinks. Keep in mind that while the pl320 omap code should compile, I don't currently have a platform on which I can perform proper testing. I also removed the context save/restore code from omap2 mailbox support, because I think it should be able to be done via driver suspend/resume, but haven't done a full investigation just yet. I'm also aware that breaking omap, just to fix it again probably isn't the best course of action, and I'm open to suggestions. -Courtney Courtney Cavin (6): mailbox: add core framework mailbox: document bindings mailbox: pl320: migrate to mbox framework mailbox: omap: remove omap-specific framework mailbox: omap1: move to common mbox framework mailbox: omap2+: move to common mbox framework .../devicetree/bindings/mailbox/mailbox.txt| 44 ++ drivers/mailbox/Kconfig| 17 - drivers/mailbox/Makefile | 2 +- drivers/mailbox/core.c | 573 + drivers/mailbox/mailbox-omap1.c| 153 +++--- drivers/mailbox/mailbox-omap2.c| 315 +-- drivers/mailbox/omap-mailbox.c | 469 - drivers/mailbox/omap-mbox.h| 67 --- drivers/mailbox/pl320-ipc.c| 258 +++--- include/linux/mailbox.h| 29 +- include/linux/mbox.h | 175 +++ include/linux/omap-mailbox.h | 45 +- 12 files changed, 1261 insertions(+), 886 deletions(-) create mode 100644 Documentation/devicetree/bindings/mailbox/mailbox.txt create mode 100644 drivers/mailbox/core.c delete mode 100644 drivers/mailbox/omap-mailbox.c delete mode 100644 drivers/mailbox/omap-mbox.h create mode 100644 include/linux/mbox.h -- 1.8.1.5 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC 3/6] mailbox: pl320: migrate to mbox framework
We don't remove the legacy methods here, but we mark them as deprecated in the hopes that people with the ability to properly test modifications can adapt its users. Signed-off-by: Courtney Cavin courtney.ca...@sonymobile.com --- drivers/mailbox/pl320-ipc.c | 258 ++-- include/linux/mailbox.h | 29 - 2 files changed, 225 insertions(+), 62 deletions(-) diff --git a/drivers/mailbox/pl320-ipc.c b/drivers/mailbox/pl320-ipc.c index d873cba..b8da247 100644 --- a/drivers/mailbox/pl320-ipc.c +++ b/drivers/mailbox/pl320-ipc.c @@ -15,7 +15,6 @@ */ #include linux/types.h #include linux/err.h -#include linux/delay.h #include linux/export.h #include linux/io.h #include linux/interrupt.h @@ -27,6 +26,7 @@ #include linux/amba/bus.h #include linux/mailbox.h +#include linux/mbox.h #define IPCMxSOURCE(m) ((m) * 0x40) #define IPCMxDSET(m) (((m) * 0x40) + 0x004) @@ -50,131 +50,162 @@ #define A9_SOURCE 1 #define M3_SOURCE 0 -static void __iomem *ipc_base; -static int ipc_irq; -static DEFINE_MUTEX(ipc_m1_lock); -static DECLARE_COMPLETION(ipc_completion); -static ATOMIC_NOTIFIER_HEAD(ipc_notifier); +struct pl320 { + struct mbox_adapter adapter; + void __iomem *base; + int irq; + struct completion completion; +}; -static inline void set_destination(int source, int mbox) +static inline void set_destination(struct pl320 *pl, int source, int mbox) { - __raw_writel(CHAN_MASK(source), ipc_base + IPCMxDSET(mbox)); - __raw_writel(CHAN_MASK(source), ipc_base + IPCMxMSET(mbox)); + __raw_writel(CHAN_MASK(source), pl-base + IPCMxDSET(mbox)); + __raw_writel(CHAN_MASK(source), pl-base + IPCMxMSET(mbox)); } -static inline void clear_destination(int source, int mbox) +static inline void clear_destination(struct pl320 *pl, int source, int mbox) { - __raw_writel(CHAN_MASK(source), ipc_base + IPCMxDCLEAR(mbox)); - __raw_writel(CHAN_MASK(source), ipc_base + IPCMxMCLEAR(mbox)); + __raw_writel(CHAN_MASK(source), pl-base + IPCMxDCLEAR(mbox)); + __raw_writel(CHAN_MASK(source), pl-base + IPCMxMCLEAR(mbox)); } -static void __ipc_send(int mbox, u32 *data) +static void __ipc_send(struct pl320 *pl, int mbox, const u32 *data) { int i; for (i = 0; i 7; i++) - __raw_writel(data[i], ipc_base + IPCMxDR(mbox, i)); - __raw_writel(0x1, ipc_base + IPCMxSEND(mbox)); + __raw_writel(data[i], pl-base + IPCMxDR(mbox, i)); + __raw_writel(0x1, pl-base + IPCMxSEND(mbox)); } -static u32 __ipc_rcv(int mbox, u32 *data) +static u32 __ipc_rcv(struct pl320 *pl, int mbox, u32 *data) { int i; for (i = 0; i 7; i++) - data[i] = __raw_readl(ipc_base + IPCMxDR(mbox, i)); + data[i] = __raw_readl(pl-base + IPCMxDR(mbox, i)); return data[1]; } /* blocking implmentation from the A9 side, not usuable in interrupts! */ -int pl320_ipc_transmit(u32 *data) +static int pl320_ipc_put_message(struct mbox_adapter *adap, + struct mbox_channel *chan, const void *data, unsigned int len) { + struct pl320 *pl; + u32 repl[7]; int ret; - mutex_lock(ipc_m1_lock); + if (len != 28 || chan-id != 0) + return -EINVAL; - init_completion(ipc_completion); - __ipc_send(IPC_TX_MBOX, data); - ret = wait_for_completion_timeout(ipc_completion, + pl = container_of(adap, struct pl320, adapter); + reinit_completion(pl-completion); + __ipc_send(pl, IPC_TX_MBOX, data); + ret = wait_for_completion_timeout(pl-completion, msecs_to_jiffies(1000)); - if (ret == 0) { - ret = -ETIMEDOUT; - goto out; - } + if (ret == 0) + return -ETIMEDOUT; + + ret = __ipc_rcv(pl, IPC_TX_MBOX, repl); - ret = __ipc_rcv(IPC_TX_MBOX, data); -out: - mutex_unlock(ipc_m1_lock); return ret; } -EXPORT_SYMBOL_GPL(pl320_ipc_transmit); static irqreturn_t ipc_handler(int irq, void *dev) { + struct pl320 *pl = dev; u32 irq_stat; u32 data[7]; - irq_stat = __raw_readl(ipc_base + IPCMMIS(1)); + irq_stat = __raw_readl(pl-base + IPCMMIS(1)); if (irq_stat MBOX_MASK(IPC_TX_MBOX)) { - __raw_writel(0, ipc_base + IPCMxSEND(IPC_TX_MBOX)); - complete(ipc_completion); + __raw_writel(0, pl-base + IPCMxSEND(IPC_TX_MBOX)); + complete(pl-completion); } if (irq_stat MBOX_MASK(IPC_RX_MBOX)) { - __ipc_rcv(IPC_RX_MBOX, data); - atomic_notifier_call_chain(ipc_notifier, data[0], data + 1); - __raw_writel(2, ipc_base + IPCMxSEND(IPC_RX_MBOX)); + __ipc_rcv(pl, IPC_RX_MBOX, data); + mbox_channel_notify(pl-adapter.channels[0], data, 28
[RFC 5/6] mailbox: omap1: move to common mbox framework
Signed-off-by: Courtney Cavin courtney.ca...@sonymobile.com --- drivers/mailbox/Kconfig | 1 - drivers/mailbox/mailbox-omap1.c | 153 +++- 2 files changed, 73 insertions(+), 81 deletions(-) diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index 6befc6e..ae6b09b 100644 --- a/drivers/mailbox/Kconfig +++ b/drivers/mailbox/Kconfig @@ -19,7 +19,6 @@ config PL320_MBOX config OMAP1_MBOX tristate OMAP1 Mailbox framework support depends on ARCH_OMAP1 - depends on BROKEN help Mailbox implementation for OMAP chips with hardware for interprocessor communication involving DSP in OMAP1. Say Y here diff --git a/drivers/mailbox/mailbox-omap1.c b/drivers/mailbox/mailbox-omap1.c index 9001b76..474890d 100644 --- a/drivers/mailbox/mailbox-omap1.c +++ b/drivers/mailbox/mailbox-omap1.c @@ -12,10 +12,10 @@ #include linux/module.h #include linux/interrupt.h #include linux/platform_device.h +#include linux/delay.h +#include linux/mbox.h #include linux/io.h -#include omap-mbox.h - #define MAILBOX_ARM2DSP1 0x00 #define MAILBOX_ARM2DSP1b 0x04 #define MAILBOX_DSP2ARM1 0x08 @@ -26,8 +26,6 @@ #define MAILBOX_DSP2ARM1_Flag 0x1c #define MAILBOX_DSP2ARM2_Flag 0x20 -static void __iomem *mbox_base; - struct omap_mbox1_fifo { unsigned long cmd; unsigned long data; @@ -39,90 +37,70 @@ struct omap_mbox1_priv { struct omap_mbox1_fifo rx_fifo; }; -static inline int mbox_read_reg(size_t ofs) +struct omap1_mbox { + struct mbox_adapter adapter; + struct omap_mbox1_priv priv; + void __iomem *base; + int irq; +}; + +static inline int mbox_read_reg(void __iomem *base, size_t ofs) { - return __raw_readw(mbox_base + ofs); + return __raw_readw(base + ofs); } -static inline void mbox_write_reg(u32 val, size_t ofs) +static inline void mbox_write_reg(void __iomem *base, u32 val, size_t ofs) { - __raw_writew(val, mbox_base + ofs); + __raw_writew(val, base + ofs); } -/* msg */ -static mbox_msg_t omap1_mbox_fifo_read(struct omap_mbox *mbox) +static int omap1_mbox_put_message(struct mbox_adapter *adap, + struct mbox_channel *chan, const void *data, unsigned int len) + { - struct omap_mbox1_fifo *fifo = - ((struct omap_mbox1_priv *)mbox-priv)-rx_fifo; - mbox_msg_t msg; + struct omap_mbox1_fifo *fifo; + struct omap1_mbox *mbox; + u32 msg; + int i; - msg = mbox_read_reg(fifo-data); - msg |= ((mbox_msg_t) mbox_read_reg(fifo-cmd)) 16; + if (len != sizeof(msg)) + return -EINVAL; - return msg; -} + msg = ((u32 *)data)[0]; + mbox = container_of(adap, struct omap1_mbox, adapter); + fifo = mbox-priv.tx_fifo; -static void -omap1_mbox_fifo_write(struct omap_mbox *mbox, mbox_msg_t msg) -{ - struct omap_mbox1_fifo *fifo = - ((struct omap_mbox1_priv *)mbox-priv)-tx_fifo; + /* wait for available space */ + for (i = 0; i 100 mbox_read_reg(mbox-base, fifo-flag); ++i) + usleep_range(10, 20); + if (i == 100) + return -ETIMEDOUT; - mbox_write_reg(msg 0x, fifo-data); - mbox_write_reg(msg 16, fifo-cmd); -} + mbox_write_reg(mbox-base, msg 0x, fifo-data); + mbox_write_reg(mbox-base, msg 16, fifo-cmd); -static int omap1_mbox_fifo_empty(struct omap_mbox *mbox) -{ return 0; } -static int omap1_mbox_fifo_full(struct omap_mbox *mbox) +static irqreturn_t omap1_mbox_irq(int irq, void *dev) { - struct omap_mbox1_fifo *fifo = - ((struct omap_mbox1_priv *)mbox-priv)-rx_fifo; + struct omap1_mbox *mbox = dev; + struct omap_mbox1_fifo *fifo; + u32 msg; - return mbox_read_reg(fifo-flag); -} + fifo = mbox-priv.rx_fifo; + msg = mbox_read_reg(mbox-base, fifo-data); + msg |= ((u32)mbox_read_reg(mbox-base, fifo-cmd)) 16; -/* irq */ -static void -omap1_mbox_enable_irq(struct omap_mbox *mbox, omap_mbox_irq_t irq) -{ - if (irq == IRQ_RX) - enable_irq(mbox-irq); -} + mbox_channel_notify(mbox-adapter.channels[0], msg, sizeof(msg)); -static void -omap1_mbox_disable_irq(struct omap_mbox *mbox, omap_mbox_irq_t irq) -{ - if (irq == IRQ_RX) - disable_irq(mbox-irq); + return IRQ_HANDLED; } -static int -omap1_mbox_is_irq(struct omap_mbox *mbox, omap_mbox_irq_t irq) -{ - if (irq == IRQ_TX) - return 0; - return 1; -} - -static struct omap_mbox_ops omap1_mbox_ops = { - .type = OMAP_MBOX_TYPE1, - .fifo_read = omap1_mbox_fifo_read, - .fifo_write = omap1_mbox_fifo_write, - .fifo_empty = omap1_mbox_fifo_empty, - .fifo_full = omap1_mbox_fifo_full, - .enable_irq = omap1_mbox_enable_irq
[RFC 6/6] mailbox: omap2+: move to common mbox framework
Signed-off-by: Courtney Cavin courtney.ca...@sonymobile.com --- drivers/mailbox/Kconfig | 1 - drivers/mailbox/mailbox-omap2.c | 315 +--- 2 files changed, 132 insertions(+), 184 deletions(-) diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index ae6b09b..a592a5a 100644 --- a/drivers/mailbox/Kconfig +++ b/drivers/mailbox/Kconfig @@ -27,7 +27,6 @@ config OMAP1_MBOX config OMAP2PLUS_MBOX tristate OMAP2+ Mailbox framework support depends on ARCH_OMAP2PLUS - depends on BROKEN help Mailbox implementation for OMAP family chips with hardware for interprocessor communication involving DSP, IVA1.0 and IVA2 in diff --git a/drivers/mailbox/mailbox-omap2.c b/drivers/mailbox/mailbox-omap2.c index 42d2b89..7ddde19 100644 --- a/drivers/mailbox/mailbox-omap2.c +++ b/drivers/mailbox/mailbox-omap2.c @@ -18,8 +18,8 @@ #include linux/io.h #include linux/pm_runtime.h #include linux/platform_data/mailbox-omap.h - -#include omap-mbox.h +#include linux/interrupt.h +#include linux/mbox.h #define MAILBOX_REVISION 0x000 #define MAILBOX_MESSAGE(m) (0x040 + 4 * (m)) @@ -42,192 +42,165 @@ #define MBOX_NR_REGS (MBOX_REG_SIZE / sizeof(u32)) #define OMAP4_MBOX_NR_REGS (OMAP4_MBOX_REG_SIZE / sizeof(u32)) -static void __iomem *mbox_base; - struct omap_mbox2_fifo { unsigned long msg; unsigned long fifo_stat; unsigned long msg_stat; }; +struct omap2_mbox; + struct omap_mbox2_priv { + struct omap2_mbox *mbox; + int irq; + struct omap_mbox2_fifo tx_fifo; struct omap_mbox2_fifo rx_fifo; unsigned long irqenable; unsigned long irqstatus; u32 newmsg_bit; u32 notfull_bit; - u32 ctx[OMAP4_MBOX_NR_REGS]; unsigned long irqdisable; u32 intr_type; }; -static inline unsigned int mbox_read_reg(size_t ofs) -{ - return __raw_readl(mbox_base + ofs); -} +struct omap2_mbox { + struct mbox_adapter adapter; + struct completion completion; + void __iomem *base; + atomic_t active; + struct omap_mbox2_priv *priv; +}; -static inline void mbox_write_reg(u32 val, size_t ofs) +static inline unsigned int mbox_read_reg(void __iomem *base, size_t ofs) { - __raw_writel(val, mbox_base + ofs); + return __raw_readl(base + ofs); } -/* Mailbox H/W preparations */ -static int omap2_mbox_startup(struct omap_mbox *mbox) +static inline void mbox_write_reg(void __iomem *base, u32 val, size_t ofs) { - u32 l; - - pm_runtime_enable(mbox-dev-parent); - pm_runtime_get_sync(mbox-dev-parent); - - l = mbox_read_reg(MAILBOX_REVISION); - pr_debug(omap mailbox rev %d.%d\n, (l 0xf0) 4, (l 0x0f)); - - return 0; + __raw_writel(val, base + ofs); } -static void omap2_mbox_shutdown(struct omap_mbox *mbox) +static int omap2_mbox_request(struct mbox_adapter *adap, + struct mbox_channel *chan) { - pm_runtime_put_sync(mbox-dev-parent); - pm_runtime_disable(mbox-dev-parent); -} + struct omap_mbox2_priv *p; + struct omap2_mbox *mbox; + u32 enable; -/* Mailbox FIFO handle functions */ -static mbox_msg_t omap2_mbox_fifo_read(struct omap_mbox *mbox) -{ - struct omap_mbox2_fifo *fifo = - ((struct omap_mbox2_priv *)mbox-priv)-rx_fifo; - return (mbox_msg_t) mbox_read_reg(fifo-msg); -} + mbox = container_of(adap, struct omap2_mbox, adapter); + p = mbox-priv[chan-id]; -static void omap2_mbox_fifo_write(struct omap_mbox *mbox, mbox_msg_t msg) -{ - struct omap_mbox2_fifo *fifo = - ((struct omap_mbox2_priv *)mbox-priv)-tx_fifo; - mbox_write_reg(msg, fifo-msg); -} + if (atomic_inc_return(mbox-active) == 1) { + pm_runtime_enable(adap-dev); + pm_runtime_get_sync(adap-dev); + } -static int omap2_mbox_fifo_empty(struct omap_mbox *mbox) -{ - struct omap_mbox2_fifo *fifo = - ((struct omap_mbox2_priv *)mbox-priv)-rx_fifo; - return (mbox_read_reg(fifo-msg_stat) == 0); -} + enable = mbox_read_reg(mbox-base, p-irqenable); + enable |= p-notfull_bit | p-newmsg_bit; + mbox_write_reg(mbox-base, enable, p-irqenable); -static int omap2_mbox_fifo_full(struct omap_mbox *mbox) -{ - struct omap_mbox2_fifo *fifo = - ((struct omap_mbox2_priv *)mbox-priv)-tx_fifo; - return mbox_read_reg(fifo-fifo_stat); + return 0; } -/* Mailbox IRQ handle functions */ -static void omap2_mbox_enable_irq(struct omap_mbox *mbox, omap_mbox_irq_t irq) +static int omap2_mbox_release(struct mbox_adapter *adap, + struct mbox_channel *chan) { - struct omap_mbox2_priv *p = mbox-priv; - u32 l, bit = (irq == IRQ_TX) ? p-notfull_bit : p-newmsg_bit; + struct omap_mbox2_priv *p; + struct omap2_mbox *mbox
[RFC 4/6] mailbox: omap: remove omap-specific framework
This framework is no longer needed, and the users should move over to the new common framework. Mark the existing implementations as broken, and deprecate the api, as a stop-gap. Signed-off-by: Courtney Cavin courtney.ca...@sonymobile.com --- drivers/mailbox/Kconfig| 19 +- drivers/mailbox/Makefile | 1 - drivers/mailbox/omap-mailbox.c | 469 - drivers/mailbox/omap-mbox.h| 67 -- include/linux/omap-mailbox.h | 45 +++- 5 files changed, 40 insertions(+), 561 deletions(-) delete mode 100644 drivers/mailbox/omap-mailbox.c delete mode 100644 drivers/mailbox/omap-mbox.h diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index c8b5c13..6befc6e 100644 --- a/drivers/mailbox/Kconfig +++ b/drivers/mailbox/Kconfig @@ -16,17 +16,10 @@ config PL320_MBOX Management Engine, primarily for cpufreq. Say Y here if you want to use the PL320 IPCM support. -config OMAP_MBOX - tristate - help - This option is selected by any OMAP architecture specific mailbox - driver such as CONFIG_OMAP1_MBOX or CONFIG_OMAP2PLUS_MBOX. This - enables the common OMAP mailbox framework code. - config OMAP1_MBOX tristate OMAP1 Mailbox framework support depends on ARCH_OMAP1 - select OMAP_MBOX + depends on BROKEN help Mailbox implementation for OMAP chips with hardware for interprocessor communication involving DSP in OMAP1. Say Y here @@ -35,19 +28,11 @@ config OMAP1_MBOX config OMAP2PLUS_MBOX tristate OMAP2+ Mailbox framework support depends on ARCH_OMAP2PLUS - select OMAP_MBOX + depends on BROKEN help Mailbox implementation for OMAP family chips with hardware for interprocessor communication involving DSP, IVA1.0 and IVA2 in OMAP2/3; or IPU, IVA HD and DSP in OMAP4/5. Say Y here if you want to use OMAP2+ Mailbox framework support. -config OMAP_MBOX_KFIFO_SIZE - int Mailbox kfifo default buffer size (bytes) - depends on OMAP2PLUS_MBOX || OMAP1_MBOX - default 256 - help - Specify the default size of mailbox's kfifo buffers (bytes). - This can also be changed at runtime (via the mbox_kfifo_size - module parameter). endif diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index 53712ed..c8e51a0 100644 --- a/drivers/mailbox/Makefile +++ b/drivers/mailbox/Makefile @@ -1,7 +1,6 @@ obj-$(CONFIG_MAILBOX) += core.o obj-$(CONFIG_PL320_MBOX) += pl320-ipc.o -obj-$(CONFIG_OMAP_MBOX)+= omap-mailbox.o obj-$(CONFIG_OMAP1_MBOX) += mailbox_omap1.o mailbox_omap1-objs := mailbox-omap1.o obj-$(CONFIG_OMAP2PLUS_MBOX) += mailbox_omap2.o diff --git a/drivers/mailbox/omap-mailbox.c b/drivers/mailbox/omap-mailbox.c deleted file mode 100644 index d79a646..000 --- a/drivers/mailbox/omap-mailbox.c +++ /dev/null @@ -1,469 +0,0 @@ -/* - * OMAP mailbox driver - * - * Copyright (C) 2006-2009 Nokia Corporation. All rights reserved. - * - * Contact: Hiroshi DOYU hiroshi.d...@nokia.com - * - * This program is free software; you can redistribute it and/or - * modify it under the terms of the GNU General Public License - * 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. - * - * You should have received a copy of the GNU General Public License - * along with this program; if not, write to the Free Software - * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA - * 02110-1301 USA - * - */ - -#include linux/interrupt.h -#include linux/spinlock.h -#include linux/mutex.h -#include linux/delay.h -#include linux/slab.h -#include linux/kfifo.h -#include linux/err.h -#include linux/notifier.h -#include linux/module.h - -#include omap-mbox.h - -static struct omap_mbox **mboxes; - -static int mbox_configured; -static DEFINE_MUTEX(mbox_configured_lock); - -static unsigned int mbox_kfifo_size = CONFIG_OMAP_MBOX_KFIFO_SIZE; -module_param(mbox_kfifo_size, uint, S_IRUGO); -MODULE_PARM_DESC(mbox_kfifo_size, Size of omap's mailbox kfifo (bytes)); - -/* Mailbox FIFO handle functions */ -static inline mbox_msg_t mbox_fifo_read(struct omap_mbox *mbox) -{ - return mbox-ops-fifo_read(mbox); -} -static inline void mbox_fifo_write(struct omap_mbox *mbox, mbox_msg_t msg) -{ - mbox-ops-fifo_write(mbox, msg); -} -static inline int mbox_fifo_empty(struct omap_mbox *mbox) -{ - return mbox-ops-fifo_empty(mbox); -} -static inline int mbox_fifo_full(struct omap_mbox *mbox) -{ - return mbox-ops-fifo_full(mbox); -} - -/* Mailbox IRQ handle functions */ -static inline void ack_mbox_irq(struct omap_mbox *mbox, omap_mbox_irq_t irq
[RFC 1/6] mailbox: add core framework
The mailbox drivers are fragmented, and some implement their own core. Unify the drivers and implement common functionality in a framework. Signed-off-by: Courtney Cavin courtney.ca...@sonymobile.com --- drivers/mailbox/Makefile | 1 + drivers/mailbox/core.c | 573 +++ include/linux/mbox.h | 175 +++ 3 files changed, 749 insertions(+) create mode 100644 drivers/mailbox/core.c create mode 100644 include/linux/mbox.h diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index e0facb3..53712ed 100644 --- a/drivers/mailbox/Makefile +++ b/drivers/mailbox/Makefile @@ -1,3 +1,4 @@ +obj-$(CONFIG_MAILBOX) += core.o obj-$(CONFIG_PL320_MBOX) += pl320-ipc.o obj-$(CONFIG_OMAP_MBOX)+= omap-mailbox.o diff --git a/drivers/mailbox/core.c b/drivers/mailbox/core.c new file mode 100644 index 000..0dc865e --- /dev/null +++ b/drivers/mailbox/core.c @@ -0,0 +1,573 @@ +/* + * Generic mailbox implementation + * + * Copyright (C) 2014 Sony Mobile Communications, AB. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope 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/module.h +#include linux/device.h +#include linux/slab.h +#include linux/mbox.h + +static DEFINE_MUTEX(mbox_lock); +static LIST_HEAD(mbox_adapters); + +static DEFINE_MUTEX(mbox_lookup_lock); +static LIST_HEAD(mbox_lookup_list); + +static int __mbox_adapter_request(struct mbox_adapter *adap, + struct mbox_channel *chan) +{ + int rc; + + if (chan-users 0) { + chan-users++; + return 0; + } + + if (!try_module_get(adap-ops-owner)) + return -ENODEV; + + if (adap-ops-request) { + rc = adap-ops-request(adap, chan); + if (rc) { + module_put(adap-ops-owner); + return rc; + } + } + + chan-users++; + + return 0; +} + +static void __mbox_adapter_release(struct mbox_adapter *adap, + struct mbox_channel *chan) +{ + if (!adap || !chan) + return; + + if (chan-users == 0) { + dev_err(adap-dev, device already released\n); + return; + } + + chan-users--; + if (chan-users 0) + return; + + if (adap-ops-release) + adap-ops-release(adap, chan); + module_put(adap-ops-owner); +} + +static struct mbox_channel * +mbox_adapter_request_channel(struct mbox_adapter *adap, unsigned int index) +{ + struct mbox_channel *chan; + int rc; + + if (!adap || index = adap-nchannels) + return ERR_PTR(-EINVAL); + + mutex_lock(adap-lock); + chan = adap-channels[index]; + + rc = __mbox_adapter_request(adap, chan); + if (rc) + chan = ERR_PTR(rc); + mutex_unlock(adap-lock); + + return chan; +} + +static void mbox_adapter_release_channel(struct mbox_adapter *adap, + struct mbox_channel *chan) +{ + if (!adap || !chan) + return; + + mutex_lock(adap-lock); + __mbox_adapter_release(adap, chan); + mutex_unlock(adap-lock); +} + +static int of_mbox_simple_xlate(struct mbox_adapter *adap, + const struct of_phandle_args *args) +{ + if (adap-of_n_cells 1) + return -EINVAL; + if (args-args[0] = adap-nchannels) + return -EINVAL; + + return args-args[0]; +} + +static struct mbox_adapter *of_node_to_mbox_adapter(struct device_node *np) +{ + struct mbox_adapter *adap; + + mutex_lock(mbox_lock); + list_for_each_entry(adap, mbox_adapters, list) { + if (adap-dev adap-dev-of_node == np) { + mutex_unlock(mbox_lock); + return adap; + } + } + mutex_unlock(mbox_lock); + + return ERR_PTR(-EPROBE_DEFER); +} + +static void of_mbox_adapter_add(struct mbox_adapter *adap) +{ + if (!adap-dev) + return; + + if (!adap-of_xlate) { + adap-of_xlate = of_mbox_simple_xlate; + adap-of_n_cells = 1; + } + + of_node_get(adap-dev-of_node); +} + +static void of_mbox_adapter_remove(struct mbox_adapter *adap) +{ + if (!adap-dev) + return; + of_node_put(adap-dev-of_node); +} + +static struct mbox_channel * +of_mbox_adapter_request_channel(struct device_node *np, const char *con_id) +{ + struct of_phandle_args args; + struct mbox_adapter *adap; + struct
[RFC 2/6] mailbox: document bindings
Signed-off-by: Courtney Cavin courtney.ca...@sonymobile.com --- .../devicetree/bindings/mailbox/mailbox.txt| 44 ++ 1 file changed, 44 insertions(+) create mode 100644 Documentation/devicetree/bindings/mailbox/mailbox.txt diff --git a/Documentation/devicetree/bindings/mailbox/mailbox.txt b/Documentation/devicetree/bindings/mailbox/mailbox.txt new file mode 100644 index 000..846eb49 --- /dev/null +++ b/Documentation/devicetree/bindings/mailbox/mailbox.txt @@ -0,0 +1,44 @@ +Binding documentation for mailbox providers and consumers +-- + +Mailbox providers may be represented by any node in a device tree. These +nodes are designated as mailbox providers. Consumers can then use a phandle +to a mailbox provider, along with channel specifier information in order to +get a mailbox. + +MAILBOX PROVIDERS + +#mbox-cells: + Usage: required + Type: u32 + Desc: Number of cells in a mailbox specifier; Typically 1 for nodes + which only need a channel index. + + +Example: + mailbox: mailbox { + #mbox-cells = 1; + ... + }; + + +MAILBOX CONSUMERS + +mbox: + Usage: required + Type: [phandle] [mailbox-specifier] + Desc: List of phandle and mailbox specifier pairs, matching provider's + #mbox-cells property + +mbox-names: + Usage: optional + Type: string array + Desc: List of mailbox input name strings sorted in the same order as + the mbox property. Consumer drivers should use mbox-names + to match mailbox input names with mailbox specifiers. + +Example: + consumer { + mbox-names = comms; + mbox = mailbox 0; + }; -- 1.8.1.5 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv2 2/2] arm: Get rid of meminfo
On Wed, Feb 05, 2014 at 01:02:31AM +0100, Laura Abbott wrote: > memblock is now fully integrated into the kernel and is the prefered > method for tracking memory. Rather than reinvent the wheel with > meminfo, migrate to using memblock directly instead of meminfo as > an intermediate. > > Signed-off-by: Laura Abbott [...] > diff --git a/arch/arm/mach-pxa/spitz.c b/arch/arm/mach-pxa/spitz.c > index 0b11c1a..51d814e 100644 > --- a/arch/arm/mach-pxa/spitz.c > +++ b/arch/arm/mach-pxa/spitz.c > @@ -32,6 +32,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -971,13 +972,9 @@ static void __init spitz_init(void) > spitz_i2c_init(); > } > > -static void __init spitz_fixup(struct tag *tags, char **cmdline, > - struct meminfo *mi) > +static void __init spitz_fixup(struct tag *tags, char **cmdline) > { > - sharpsl_save_param(); > - mi->nr_banks = 1; > - mi->bank[0].start = 0xa000; > - mi->bank[0].size = (64*1024*1024); > + memblock_addr(0xa000, SZ_64M); memblock_add() ? -Courtney -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv2 2/2] arm: Get rid of meminfo
On Wed, Feb 05, 2014 at 01:02:31AM +0100, Laura Abbott wrote: memblock is now fully integrated into the kernel and is the prefered method for tracking memory. Rather than reinvent the wheel with meminfo, migrate to using memblock directly instead of meminfo as an intermediate. Signed-off-by: Laura Abbott lau...@codeaurora.org [...] diff --git a/arch/arm/mach-pxa/spitz.c b/arch/arm/mach-pxa/spitz.c index 0b11c1a..51d814e 100644 --- a/arch/arm/mach-pxa/spitz.c +++ b/arch/arm/mach-pxa/spitz.c @@ -32,6 +32,7 @@ #include linux/io.h #include linux/module.h #include linux/reboot.h +#include linux/memblock.h #include asm/setup.h #include asm/mach-types.h @@ -971,13 +972,9 @@ static void __init spitz_init(void) spitz_i2c_init(); } -static void __init spitz_fixup(struct tag *tags, char **cmdline, - struct meminfo *mi) +static void __init spitz_fixup(struct tag *tags, char **cmdline) { - sharpsl_save_param(); - mi-nr_banks = 1; - mi-bank[0].start = 0xa000; - mi-bank[0].size = (64*1024*1024); + memblock_addr(0xa000, SZ_64M); memblock_add() ? -Courtney -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] thermal: add generic IIO channel thermal sensor driver
This driver is a generic method for using IIO ADC channels as thermal sensors. Signed-off-by: Courtney Cavin --- .../devicetree/bindings/thermal/iio-thermal.txt| 46 + drivers/thermal/Kconfig| 13 ++ drivers/thermal/Makefile | 1 + drivers/thermal/iio_thermal.c | 207 + 4 files changed, 267 insertions(+) create mode 100644 Documentation/devicetree/bindings/thermal/iio-thermal.txt create mode 100644 drivers/thermal/iio_thermal.c diff --git a/Documentation/devicetree/bindings/thermal/iio-thermal.txt b/Documentation/devicetree/bindings/thermal/iio-thermal.txt new file mode 100644 index 000..3be11b6 --- /dev/null +++ b/Documentation/devicetree/bindings/thermal/iio-thermal.txt @@ -0,0 +1,46 @@ +Generic IIO channel thermal sensor bindings + +compatible: + Usage: required + Type: string + Desc: compatible string, must be "iio-thermal" + +conversion-method: + Usage: required + Type: string + Desc: How to convert IIO voltage values to temperature, one of: + "interpolation" - interpolate between values in lookup table + "scalar" - use values as multiplier and divisor + +conversion-values: + Usage: required + Type: u32 array, 2-tuples + Desc: lookup table for conversion, for conversion-method: + "interpolation" - 2-tuples of < uV mK >; micro-volts to + milli-kelvin; table must ascend + "scalar" - single scalar 2-tuple as < M D >; where: + mK = uV * M / D + +io-channels: + Usage: required + Type: prop-encoded-array + Desc: See bindings/iio/iio-bindings.txt; must be a voltage channel + +Example: + +vadc: some_vadc { + compatible = "..."; + #io-channel-cells = <1>; +}; + +iio-thermal { + compatible = "iio-thermal"; + io-channels = < 0>; + conversion-method = "interpolation"; + conversion-values = < + 3 398200 +385000 318200 + 1738000 233200 + >; +}; + diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig index 35c0664..f83a8e8 100644 --- a/drivers/thermal/Kconfig +++ b/drivers/thermal/Kconfig @@ -114,6 +114,19 @@ config THERMAL_EMULATION because userland can easily disable the thermal policy by simply flooding this sysfs node with low temperature values. +config IIO_THERMAL + tristate "Temperature sensor driver for generic IIO channels" + depends on IIO + depends on THERMAL_OF + help + Support for generic IIO channels, such as ADCs. This driver allows + you to expose an IIO voltage channel as a thermal sensor. This is + implemented as a thermal sensor, not a thermal zone, and thus + requires DT defined thermal infrastructure in order to be useful. + + If you aren't sure that you need this support, or haven't configured + a thermal infrastructure in device tree, you should say 'N' here. + config IMX_THERMAL tristate "Temperature sensor driver for Freescale i.MX SoCs" depends on CPU_THERMAL diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile index 54e4ec9..0ee2c92 100644 --- a/drivers/thermal/Makefile +++ b/drivers/thermal/Makefile @@ -25,6 +25,7 @@ obj-y += samsung/ obj-$(CONFIG_DOVE_THERMAL) += dove_thermal.o obj-$(CONFIG_DB8500_THERMAL) += db8500_thermal.o obj-$(CONFIG_ARMADA_THERMAL) += armada_thermal.o +obj-$(CONFIG_IIO_THERMAL) += iio_thermal.o obj-$(CONFIG_IMX_THERMAL) += imx_thermal.o obj-$(CONFIG_DB8500_CPUFREQ_COOLING) += db8500_cpufreq_cooling.o obj-$(CONFIG_INTEL_POWERCLAMP) += intel_powerclamp.o diff --git a/drivers/thermal/iio_thermal.c b/drivers/thermal/iio_thermal.c new file mode 100644 index 000..df21dbc --- /dev/null +++ b/drivers/thermal/iio_thermal.c @@ -0,0 +1,207 @@ +/* + * An IIO channel based thermal sensor driver + * + * Copyright (C) 2014 Sony Mobile Communications, AB + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * 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 +#include +#include +#include +#include +#include +#include + +#define MILLIKELVIN_0C 273150 + +struct iio_thermal_conv { + s32 *values; + int ntuple; + long (*convert)(const struct iio_thermal_conv *, int val); +}; + +s
[PATCH] thermal: add generic IIO channel thermal sensor driver
This driver is a generic method for using IIO ADC channels as thermal sensors. Signed-off-by: Courtney Cavin courtney.ca...@sonymobile.com --- .../devicetree/bindings/thermal/iio-thermal.txt| 46 + drivers/thermal/Kconfig| 13 ++ drivers/thermal/Makefile | 1 + drivers/thermal/iio_thermal.c | 207 + 4 files changed, 267 insertions(+) create mode 100644 Documentation/devicetree/bindings/thermal/iio-thermal.txt create mode 100644 drivers/thermal/iio_thermal.c diff --git a/Documentation/devicetree/bindings/thermal/iio-thermal.txt b/Documentation/devicetree/bindings/thermal/iio-thermal.txt new file mode 100644 index 000..3be11b6 --- /dev/null +++ b/Documentation/devicetree/bindings/thermal/iio-thermal.txt @@ -0,0 +1,46 @@ +Generic IIO channel thermal sensor bindings + +compatible: + Usage: required + Type: string + Desc: compatible string, must be iio-thermal + +conversion-method: + Usage: required + Type: string + Desc: How to convert IIO voltage values to temperature, one of: + interpolation - interpolate between values in lookup table + scalar - use values as multiplier and divisor + +conversion-values: + Usage: required + Type: u32 array, 2-tuples + Desc: lookup table for conversion, for conversion-method: + interpolation - 2-tuples of uV mK ; micro-volts to + milli-kelvin; table must ascend + scalar - single scalar 2-tuple as M D ; where: + mK = uV * M / D + +io-channels: + Usage: required + Type: prop-encoded-array + Desc: See bindings/iio/iio-bindings.txt; must be a voltage channel + +Example: + +vadc: some_vadc { + compatible = ...; + #io-channel-cells = 1; +}; + +iio-thermal { + compatible = iio-thermal; + io-channels = vadc 0; + conversion-method = interpolation; + conversion-values = + 3 398200 +385000 318200 + 1738000 233200 + ; +}; + diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig index 35c0664..f83a8e8 100644 --- a/drivers/thermal/Kconfig +++ b/drivers/thermal/Kconfig @@ -114,6 +114,19 @@ config THERMAL_EMULATION because userland can easily disable the thermal policy by simply flooding this sysfs node with low temperature values. +config IIO_THERMAL + tristate Temperature sensor driver for generic IIO channels + depends on IIO + depends on THERMAL_OF + help + Support for generic IIO channels, such as ADCs. This driver allows + you to expose an IIO voltage channel as a thermal sensor. This is + implemented as a thermal sensor, not a thermal zone, and thus + requires DT defined thermal infrastructure in order to be useful. + + If you aren't sure that you need this support, or haven't configured + a thermal infrastructure in device tree, you should say 'N' here. + config IMX_THERMAL tristate Temperature sensor driver for Freescale i.MX SoCs depends on CPU_THERMAL diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile index 54e4ec9..0ee2c92 100644 --- a/drivers/thermal/Makefile +++ b/drivers/thermal/Makefile @@ -25,6 +25,7 @@ obj-y += samsung/ obj-$(CONFIG_DOVE_THERMAL) += dove_thermal.o obj-$(CONFIG_DB8500_THERMAL) += db8500_thermal.o obj-$(CONFIG_ARMADA_THERMAL) += armada_thermal.o +obj-$(CONFIG_IIO_THERMAL) += iio_thermal.o obj-$(CONFIG_IMX_THERMAL) += imx_thermal.o obj-$(CONFIG_DB8500_CPUFREQ_COOLING) += db8500_cpufreq_cooling.o obj-$(CONFIG_INTEL_POWERCLAMP) += intel_powerclamp.o diff --git a/drivers/thermal/iio_thermal.c b/drivers/thermal/iio_thermal.c new file mode 100644 index 000..df21dbc --- /dev/null +++ b/drivers/thermal/iio_thermal.c @@ -0,0 +1,207 @@ +/* + * An IIO channel based thermal sensor driver + * + * Copyright (C) 2014 Sony Mobile Communications, AB + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * 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/module.h +#include linux/platform_device.h +#include linux/iio/iio.h +#include linux/iio/consumer.h +#include linux/thermal.h +#include linux/err.h +#include linux/of.h + +#define MILLIKELVIN_0C 273150 + +struct iio_thermal_conv { + s32 *values; + int ntuple; + long (*convert)(const struct iio_thermal_conv *, int val); +}; + +struct iio_thermal
Re: [PATCH v7 2/2] mmc: sdhci-msm: Initial support for MSM chipsets
On Thu, Jan 30, 2014 at 06:21:11PM +0100, Georgi Djakov wrote: > Hi, > > Apologies for the delayed reply. > > On 12/09/2013 07:00 PM, Courtney Cavin wrote: > > On Wed, Nov 06, 2013 at 04:56:45PM +0100, Georgi Djakov wrote: > >> This platform driver adds the initial support of Secure > >> Digital Host Controller Interface compliant controller > >> found in Qualcomm MSM chipsets. > >> > >> Signed-off-by: Georgi Djakov [...] > > > > I couldn't get v6 running without the 5 quirks you submitted in [1]. > > Aren't these also attributes of the controller itself? If so, shouldn't they > > be part of this patch series, and included here? > > > > Most of the quirks are for older hardware revisions. On what board/chip > are you trying to run it? I will post v8 shortly. Could you try with it > please? I see. This was initially checked on a Rev 1 SoC. I will test v8 on a Rev 2. -Courtney -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7 2/2] mmc: sdhci-msm: Initial support for MSM chipsets
On Thu, Jan 30, 2014 at 06:21:11PM +0100, Georgi Djakov wrote: Hi, Apologies for the delayed reply. On 12/09/2013 07:00 PM, Courtney Cavin wrote: On Wed, Nov 06, 2013 at 04:56:45PM +0100, Georgi Djakov wrote: This platform driver adds the initial support of Secure Digital Host Controller Interface compliant controller found in Qualcomm MSM chipsets. Signed-off-by: Georgi Djakov gdja...@mm-sol.com [...] I couldn't get v6 running without the 5 quirks you submitted in [1]. Aren't these also attributes of the controller itself? If so, shouldn't they be part of this patch series, and included here? Most of the quirks are for older hardware revisions. On what board/chip are you trying to run it? I will post v8 shortly. Could you try with it please? I see. This was initially checked on a Rev 1 SoC. I will test v8 on a Rev 2. -Courtney -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 5/6] spmi: document the PMIC arbiter SPMI bindings
On Tue, Jan 14, 2014 at 07:41:39PM +0100, Josh Cartwright wrote: > Signed-off-by: Josh Cartwright > --- > .../bindings/spmi/qcom,spmi-pmic-arb.txt | 46 > ++ > 1 file changed, 46 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt > > diff --git a/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt > b/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt > new file mode 100644 > index 000..9e50cb3 > --- /dev/null > +++ b/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt > @@ -0,0 +1,46 @@ > +Qualcomm SPMI Controller (PMIC Arbiter) > + > +The SPMI PMIC Arbiter is found on the Snapdragon 800 Series. It is an SPMI > +controller with wrapping arbitration logic to allow for multiple on-chip > +devices to control a single SPMI master. > + > +The PMIC Arbiter can also act as an interrupt controller, providing > interrupts > +to slave devices. > + > +See spmi.txt for the generic SPMI controller binding requirements for child > +nodes. > + > +Required properties: > +- compatible : should be "qcom,spmi-pmic-arb". > +- reg-names : should be "core", "intr", "cnfg" > +- reg : register specifiers, must contain 3 entries, in the follow order: > + core registers, interrupt controller registers, configuration registers As far as I can tell, patch 3/6 doesn't require these to be in any order, as it uses 'reg-names' to fetch the values. Perhaps the following: reg-names : must contain: "core" - core registers "intr" - interrupt controller registers "cnfg" - configuration registers reg : A list of address + size pairs for the regs listed in reg-names > +- #address-cells : must be set to 2 > +- #size-cells : must be set to 0 > +- qcom,ee : indicates the active Execution Environment identifier (0-5) > +- qcom,channel : which of the PMIC Arb provided channels to use for accesses > (0-5) These two are new Is it expected that they should be required? > +- interrupt-controller : indicates the PMIC arbiter is an interrupt > controller Although it's probably fairly understood that this is a boolean, it wouldn't hurt to mention that here. It might also be worth referencing devicetree/binding/interrupt-controller/interrupts.txt in your opening blurb. > +- #interrupt-cells = <4>: interrupts are specified as a 4-tuple: Nitpick: This '= ' differs from the above 'must be set to X' format. > +cell 1: slave ID for the requested interrupt (0-15) > +cell 2: peripheral ID for requested interrupt (0-255) > +cell 3: the requested peripheral interrupt (0-7) > +cell 4: interrupt flags indicating level-sense information, as defined in > +dt-bindings/interrupt-controller/irq.h [...] -Courtney -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 4/6] spmi: pmic_arb: add support for interrupt handling
On Tue, Jan 14, 2014 at 07:41:38PM +0100, Josh Cartwright wrote: > The Qualcomm PMIC Arbiter, in addition to being a basic SPMI controller, > also implements interrupt handling for slave devices. Note, this is > outside the scope of SPMI, as SPMI leaves interrupt handling completely > unspecified. > > Extend the driver to provide a irq_chip implementation and chained irq > handling which allows for these interrupts to be used. > > Signed-off-by: Josh Cartwright > --- > drivers/spmi/spmi-pmic-arb.c | 393 > ++- > 1 file changed, 391 insertions(+), 2 deletions(-) > Yay! Good to see this series. > diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c > index 16083cd..32bed54 100644 > --- a/drivers/spmi/spmi-pmic-arb.c > +++ b/drivers/spmi/spmi-pmic-arb.c > @@ -13,6 +13,9 @@ > #include > #include > #include > +#include > +#include > +#include > #include > #include > #include > @@ -103,6 +106,14 @@ enum pmic_arb_cmd_op_code { > * @cnfg: address of the PMIC Arbiter configuration registers. > * @lock: lock to synchronize accesses. > * @channel: which channel to use for accesses. > + * @irq: PMIC ARB interrupt. > + * @ee:the current Execution Environment > + * @min_apid: minimum APID (used for bounding IRQ search) > + * @max_apid: maximum APID > + * @mapping_table: in-memory copy of PPID -> APID mapping table. > + * @domain:irq domain object for PMIC IRQ domain > + * @spmic: SPMI controller object > + * @apid_to_ppid: cached mapping from APID to PPID > */ > struct spmi_pmic_arb_dev { > void __iomem*base; > @@ -110,6 +121,14 @@ struct spmi_pmic_arb_dev { > void __iomem*cnfg; > spinlock_t lock; > u8 channel; > + unsigned intirq; > + u8 ee; > + u8 min_apid; > + u8 max_apid; > + u32 mapping_table[SPMI_MAPPING_TABLE_LEN]; > + struct irq_domain *domain; > + struct spmi_controller *spmic; > + u16 apid_to_ppid[256]; > }; > > static inline u32 pmic_arb_base_read(struct spmi_pmic_arb_dev *dev, u32 > offset) > @@ -314,12 +333,333 @@ static int pmic_arb_write_cmd(struct spmi_controller > *ctrl, u8 opc, u8 sid, > return rc; > } > > +enum qpnpint_regs { > + QPNPINT_REG_RT_STS = 0x10, > + QPNPINT_REG_SET_TYPE= 0x11, > + QPNPINT_REG_POLARITY_HIGH = 0x12, > + QPNPINT_REG_POLARITY_LOW= 0x13, > + QPNPINT_REG_LATCHED_CLR = 0x14, > + QPNPINT_REG_EN_SET = 0x15, > + QPNPINT_REG_EN_CLR = 0x16, > + QPNPINT_REG_LATCHED_STS = 0x18, > +}; > + > +struct spmi_pmic_arb_qpnpint_type { > + u8 type; /* 1 -> edge */ > + u8 polarity_high; > + u8 polarity_low; > +} __packed; > + While the rest of this driver uses 'pmic' or 'spmi_pmic', this patch adds 'qpnpint'. Can we please just leave the software fabricated name 'qpnp' out of any changes, as it isn't in any hardware spec? Perhaps 'pmic_int' or something along those lines? > +/* Simplified accessor functions for irqchip callbacks */ > +static void qpnpint_spmi_write(struct irq_data *d, u8 reg, void *buf, > + size_t len) [...] -Courtney -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 4/6] spmi: pmic_arb: add support for interrupt handling
On Tue, Jan 14, 2014 at 07:41:38PM +0100, Josh Cartwright wrote: The Qualcomm PMIC Arbiter, in addition to being a basic SPMI controller, also implements interrupt handling for slave devices. Note, this is outside the scope of SPMI, as SPMI leaves interrupt handling completely unspecified. Extend the driver to provide a irq_chip implementation and chained irq handling which allows for these interrupts to be used. Signed-off-by: Josh Cartwright jo...@codeaurora.org --- drivers/spmi/spmi-pmic-arb.c | 393 ++- 1 file changed, 391 insertions(+), 2 deletions(-) Yay! Good to see this series. diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c index 16083cd..32bed54 100644 --- a/drivers/spmi/spmi-pmic-arb.c +++ b/drivers/spmi/spmi-pmic-arb.c @@ -13,6 +13,9 @@ #include linux/err.h #include linux/interrupt.h #include linux/io.h +#include linux/irqchip/chained_irq.h +#include linux/irqdomain.h +#include linux/irq.h #include linux/kernel.h #include linux/module.h #include linux/of.h @@ -103,6 +106,14 @@ enum pmic_arb_cmd_op_code { * @cnfg: address of the PMIC Arbiter configuration registers. * @lock: lock to synchronize accesses. * @channel: which channel to use for accesses. + * @irq: PMIC ARB interrupt. + * @ee:the current Execution Environment + * @min_apid: minimum APID (used for bounding IRQ search) + * @max_apid: maximum APID + * @mapping_table: in-memory copy of PPID - APID mapping table. + * @domain:irq domain object for PMIC IRQ domain + * @spmic: SPMI controller object + * @apid_to_ppid: cached mapping from APID to PPID */ struct spmi_pmic_arb_dev { void __iomem*base; @@ -110,6 +121,14 @@ struct spmi_pmic_arb_dev { void __iomem*cnfg; spinlock_t lock; u8 channel; + unsigned intirq; + u8 ee; + u8 min_apid; + u8 max_apid; + u32 mapping_table[SPMI_MAPPING_TABLE_LEN]; + struct irq_domain *domain; + struct spmi_controller *spmic; + u16 apid_to_ppid[256]; }; static inline u32 pmic_arb_base_read(struct spmi_pmic_arb_dev *dev, u32 offset) @@ -314,12 +333,333 @@ static int pmic_arb_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid, return rc; } +enum qpnpint_regs { + QPNPINT_REG_RT_STS = 0x10, + QPNPINT_REG_SET_TYPE= 0x11, + QPNPINT_REG_POLARITY_HIGH = 0x12, + QPNPINT_REG_POLARITY_LOW= 0x13, + QPNPINT_REG_LATCHED_CLR = 0x14, + QPNPINT_REG_EN_SET = 0x15, + QPNPINT_REG_EN_CLR = 0x16, + QPNPINT_REG_LATCHED_STS = 0x18, +}; + +struct spmi_pmic_arb_qpnpint_type { + u8 type; /* 1 - edge */ + u8 polarity_high; + u8 polarity_low; +} __packed; + While the rest of this driver uses 'pmic' or 'spmi_pmic', this patch adds 'qpnpint'. Can we please just leave the software fabricated name 'qpnp' out of any changes, as it isn't in any hardware spec? Perhaps 'pmic_int' or something along those lines? +/* Simplified accessor functions for irqchip callbacks */ +static void qpnpint_spmi_write(struct irq_data *d, u8 reg, void *buf, + size_t len) [...] -Courtney -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 5/6] spmi: document the PMIC arbiter SPMI bindings
On Tue, Jan 14, 2014 at 07:41:39PM +0100, Josh Cartwright wrote: Signed-off-by: Josh Cartwright jo...@codeaurora.org --- .../bindings/spmi/qcom,spmi-pmic-arb.txt | 46 ++ 1 file changed, 46 insertions(+) create mode 100644 Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt diff --git a/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt b/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt new file mode 100644 index 000..9e50cb3 --- /dev/null +++ b/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt @@ -0,0 +1,46 @@ +Qualcomm SPMI Controller (PMIC Arbiter) + +The SPMI PMIC Arbiter is found on the Snapdragon 800 Series. It is an SPMI +controller with wrapping arbitration logic to allow for multiple on-chip +devices to control a single SPMI master. + +The PMIC Arbiter can also act as an interrupt controller, providing interrupts +to slave devices. + +See spmi.txt for the generic SPMI controller binding requirements for child +nodes. + +Required properties: +- compatible : should be qcom,spmi-pmic-arb. +- reg-names : should be core, intr, cnfg +- reg : register specifiers, must contain 3 entries, in the follow order: + core registers, interrupt controller registers, configuration registers As far as I can tell, patch 3/6 doesn't require these to be in any order, as it uses 'reg-names' to fetch the values. Perhaps the following: reg-names : must contain: core - core registers intr - interrupt controller registers cnfg - configuration registers reg : A list of address + size pairs for the regs listed in reg-names +- #address-cells : must be set to 2 +- #size-cells : must be set to 0 +- qcom,ee : indicates the active Execution Environment identifier (0-5) +- qcom,channel : which of the PMIC Arb provided channels to use for accesses (0-5) These two are new Is it expected that they should be required? +- interrupt-controller : indicates the PMIC arbiter is an interrupt controller Although it's probably fairly understood that this is a boolean, it wouldn't hurt to mention that here. It might also be worth referencing devicetree/binding/interrupt-controller/interrupts.txt in your opening blurb. +- #interrupt-cells = 4: interrupts are specified as a 4-tuple: Nitpick: This '= X' differs from the above 'must be set to X' format. +cell 1: slave ID for the requested interrupt (0-15) +cell 2: peripheral ID for requested interrupt (0-255) +cell 3: the requested peripheral interrupt (0-7) +cell 4: interrupt flags indicating level-sense information, as defined in +dt-bindings/interrupt-controller/irq.h [...] -Courtney -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 4/7] mfd: ssbi: Add regmap read/write helpers
On Wed, Jan 08, 2014 at 07:37:47PM +0100, Stephen Boyd wrote: > Add read and write helper functions that the pm8921-core driver > can use to read and write ssbi regsiters via a "no-bus" regmap. > Nit: s/regsiters/registers/ > Cc: Mark Brown > Signed-off-by: Stephen Boyd > --- > include/linux/ssbi.h | 13 + > 1 file changed, 13 insertions(+) > > diff --git a/include/linux/ssbi.h b/include/linux/ssbi.h > index bcbb642a7641..3ffc02e479d2 100644 > --- a/include/linux/ssbi.h > +++ b/include/linux/ssbi.h > @@ -20,4 +20,17 @@ > int ssbi_write(struct device *dev, u16 addr, const u8 *buf, int len); > int ssbi_read(struct device *dev, u16 addr, u8 *buf, int len); > > +static inline int > +ssbi_reg_read(void *context, unsigned int reg, unsigned int *val) > +{ > + *val = 0; > + return ssbi_read(context, reg, (u8 *)val, 1); > +} > + > +static inline int > +ssbi_reg_write(void *context, unsigned int reg, unsigned int val) > +{ > + return ssbi_write(context, reg, (u8 *), 1); > +} These functions are endian specific and just generally ugly. I understand that these functions may make the ssbi regmap code cleaner, but that's not really a good excuse for functions which by themselves look horribly broken. If these are really needed, perhaps something like the following would be acceptable? +static inline int +ssbi_reg_read(void *context, unsigned int reg, unsigned int *val) +{ + int rc; + u8 b; + rc = ssbi_read(context, reg, , 1); + if (rc == 1) + *val = b; + return rc; +} + +static inline int +ssbi_reg_write(void *context, unsigned int reg, unsigned int val) +{ + u8 b = val; + return ssbi_write(context, reg, , 1); +} -Courtney -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 3/6] ARM: Add Krait L2 accessor functions
On Mon, Dec 30, 2013 at 09:14:14PM +0100, Stephen Boyd wrote: > Krait CPUs have a handful of L2 cache controller registers that > live behind a cp15 based indirection register. First you program > the indirection register (l2cpselr) to point the L2 'window' > register (l2cpdr) at what you want to read/write. Then you > read/write the 'window' register to do what you want. The > l2cpselr register is not banked per-cpu so we must lock around > accesses to it to prevent other CPUs from re-pointing l2cpdr > underneath us. > > Cc: Mark Rutland > Cc: Russell King > Signed-off-by: Stephen Boyd > --- > arch/arm/common/Kconfig | 3 ++ > arch/arm/common/Makefile | 1 + > arch/arm/common/krait-l2-accessors.c | 58 > +++ > arch/arm/include/asm/krait-l2-accessors.h | 20 +++ > 4 files changed, 82 insertions(+) > create mode 100644 arch/arm/common/krait-l2-accessors.c > create mode 100644 arch/arm/include/asm/krait-l2-accessors.h [...] > + > +extern void set_l2_indirect_reg(u32 addr, u32 val); > +extern u32 get_l2_indirect_reg(u32 addr); As these are Krait specific, please rename the functions to reflect this. -Courtney -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 3/6] ARM: Add Krait L2 accessor functions
On Mon, Dec 30, 2013 at 09:14:14PM +0100, Stephen Boyd wrote: Krait CPUs have a handful of L2 cache controller registers that live behind a cp15 based indirection register. First you program the indirection register (l2cpselr) to point the L2 'window' register (l2cpdr) at what you want to read/write. Then you read/write the 'window' register to do what you want. The l2cpselr register is not banked per-cpu so we must lock around accesses to it to prevent other CPUs from re-pointing l2cpdr underneath us. Cc: Mark Rutland mark.rutl...@arm.com Cc: Russell King li...@arm.linux.org.uk Signed-off-by: Stephen Boyd sb...@codeaurora.org --- arch/arm/common/Kconfig | 3 ++ arch/arm/common/Makefile | 1 + arch/arm/common/krait-l2-accessors.c | 58 +++ arch/arm/include/asm/krait-l2-accessors.h | 20 +++ 4 files changed, 82 insertions(+) create mode 100644 arch/arm/common/krait-l2-accessors.c create mode 100644 arch/arm/include/asm/krait-l2-accessors.h [...] + +extern void set_l2_indirect_reg(u32 addr, u32 val); +extern u32 get_l2_indirect_reg(u32 addr); As these are Krait specific, please rename the functions to reflect this. -Courtney -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 4/7] mfd: ssbi: Add regmap read/write helpers
On Wed, Jan 08, 2014 at 07:37:47PM +0100, Stephen Boyd wrote: Add read and write helper functions that the pm8921-core driver can use to read and write ssbi regsiters via a no-bus regmap. Nit: s/regsiters/registers/ Cc: Mark Brown broo...@kernel.org Signed-off-by: Stephen Boyd sb...@codeaurora.org --- include/linux/ssbi.h | 13 + 1 file changed, 13 insertions(+) diff --git a/include/linux/ssbi.h b/include/linux/ssbi.h index bcbb642a7641..3ffc02e479d2 100644 --- a/include/linux/ssbi.h +++ b/include/linux/ssbi.h @@ -20,4 +20,17 @@ int ssbi_write(struct device *dev, u16 addr, const u8 *buf, int len); int ssbi_read(struct device *dev, u16 addr, u8 *buf, int len); +static inline int +ssbi_reg_read(void *context, unsigned int reg, unsigned int *val) +{ + *val = 0; + return ssbi_read(context, reg, (u8 *)val, 1); +} + +static inline int +ssbi_reg_write(void *context, unsigned int reg, unsigned int val) +{ + return ssbi_write(context, reg, (u8 *)val, 1); +} These functions are endian specific and just generally ugly. I understand that these functions may make the ssbi regmap code cleaner, but that's not really a good excuse for functions which by themselves look horribly broken. If these are really needed, perhaps something like the following would be acceptable? +static inline int +ssbi_reg_read(void *context, unsigned int reg, unsigned int *val) +{ + int rc; + u8 b; + rc = ssbi_read(context, reg, b, 1); + if (rc == 1) + *val = b; + return rc; +} + +static inline int +ssbi_reg_write(void *context, unsigned int reg, unsigned int val) +{ + u8 b = val; + return ssbi_write(context, reg, b, 1); +} -Courtney -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 7/8] mfd: pm8921: Migrate to irqdomains
On Wed, Dec 11, 2013 at 12:35:22AM +0100, Stephen Boyd wrote: > Convert this driver to use irqdomains so that the PMIC's child > devices can be converted to devicetree. > > Signed-off-by: Stephen Boyd > --- > drivers/mfd/pm8921-core.c | 186 > ++ > include/linux/mfd/pm8xxx/irq.h| 59 > include/linux/mfd/pm8xxx/pm8921.h | 30 -- > 3 files changed, 66 insertions(+), 209 deletions(-) > delete mode 100644 include/linux/mfd/pm8xxx/irq.h > delete mode 100644 include/linux/mfd/pm8xxx/pm8921.h > [...] > @@ -282,17 +278,14 @@ static struct irq_chip pm8xxx_irq_chip = { > * RETURNS: > * an int indicating the value read on that line > */ > -int pm8xxx_get_irq_stat(struct pm_irq_chip *chip, int irq) > +static int pm8xxx_get_irq_stat(struct pm_irq_chip *chip, int irq) > { > int pmirq, rc; > u8 block, bits, bit; > unsigned long flags; > + struct irq_data *irq_data = irq_get_irq_data(irq); > > - if (chip == NULL || irq < chip->irq_base || > - irq >= chip->irq_base + chip->num_irqs) > - return -EINVAL; > - > - pmirq = irq - chip->irq_base; > + pmirq = irq_data->hwirq; > > block = pmirq / 8; > bit = pmirq % 8; > @@ -322,64 +315,55 @@ bail_out: > } > EXPORT_SYMBOL_GPL(pm8xxx_get_irq_stat); Surely this isn't needed anymore, since the function is now static. [...] > +static int pm8xxx_irq_init(struct platform_device *pdev, unsigned int irq, > + unsigned int nirqs) 'nirqs' seems to always be 256. Is there a benefit to keeping this dynamic? > +{ > + struct pm_irq_chip *chip; > + > + chip = devm_kzalloc(>dev, sizeof(*chip) + sizeof(u8) * nirqs, > + GFP_KERNEL); > + if (!chip) > + return -ENOMEM; > > - chip->dev = dev; > - chip->devirq = devirq; > - chip->irq_base = pdata->irq_base; > - chip->num_irqs = pdata->irq_cdata.nirqs; > + chip->dev = >dev; > + chip->num_irqs = nirqs; > chip->num_blocks = DIV_ROUND_UP(chip->num_irqs, 8); > chip->num_masters = DIV_ROUND_UP(chip->num_blocks, 8); > spin_lock_init(>pm_irq_lock); > > - for (pmirq = 0; pmirq < chip->num_irqs; pmirq++) { -Courtney -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 7/8] mfd: pm8921: Migrate to irqdomains
On Wed, Dec 11, 2013 at 12:35:22AM +0100, Stephen Boyd wrote: Convert this driver to use irqdomains so that the PMIC's child devices can be converted to devicetree. Signed-off-by: Stephen Boyd sb...@codeaurora.org --- drivers/mfd/pm8921-core.c | 186 ++ include/linux/mfd/pm8xxx/irq.h| 59 include/linux/mfd/pm8xxx/pm8921.h | 30 -- 3 files changed, 66 insertions(+), 209 deletions(-) delete mode 100644 include/linux/mfd/pm8xxx/irq.h delete mode 100644 include/linux/mfd/pm8xxx/pm8921.h [...] @@ -282,17 +278,14 @@ static struct irq_chip pm8xxx_irq_chip = { * RETURNS: * an int indicating the value read on that line */ -int pm8xxx_get_irq_stat(struct pm_irq_chip *chip, int irq) +static int pm8xxx_get_irq_stat(struct pm_irq_chip *chip, int irq) { int pmirq, rc; u8 block, bits, bit; unsigned long flags; + struct irq_data *irq_data = irq_get_irq_data(irq); - if (chip == NULL || irq chip-irq_base || - irq = chip-irq_base + chip-num_irqs) - return -EINVAL; - - pmirq = irq - chip-irq_base; + pmirq = irq_data-hwirq; block = pmirq / 8; bit = pmirq % 8; @@ -322,64 +315,55 @@ bail_out: } EXPORT_SYMBOL_GPL(pm8xxx_get_irq_stat); Surely this isn't needed anymore, since the function is now static. [...] +static int pm8xxx_irq_init(struct platform_device *pdev, unsigned int irq, + unsigned int nirqs) 'nirqs' seems to always be 256. Is there a benefit to keeping this dynamic? +{ + struct pm_irq_chip *chip; + + chip = devm_kzalloc(pdev-dev, sizeof(*chip) + sizeof(u8) * nirqs, + GFP_KERNEL); + if (!chip) + return -ENOMEM; - chip-dev = dev; - chip-devirq = devirq; - chip-irq_base = pdata-irq_base; - chip-num_irqs = pdata-irq_cdata.nirqs; + chip-dev = pdev-dev; + chip-num_irqs = nirqs; chip-num_blocks = DIV_ROUND_UP(chip-num_irqs, 8); chip-num_masters = DIV_ROUND_UP(chip-num_blocks, 8); spin_lock_init(chip-pm_irq_lock); - for (pmirq = 0; pmirq chip-num_irqs; pmirq++) { -Courtney -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH CFT] ARM:MSM: Enable ARM_PATCH_PHYS_VIRT and AUTO_ZRELADDR default
On Mon, Dec 09, 2013 at 10:40:50AM +0100, panchaxari wrote: > ARM_PATCH_PHYS_VIRT and AUTO_ZRELADDR have been enabled as default configs > to MSM platform > > Introduction of PHYS_VIRT config as default would enable phy-to-virt and > virt-to-phy translation function at boot and module loading time > and enforce dynamic reallocation of memory. AUTO_ZRELADDR config would > enable calculation of kernel load address at run time. > > PHYS_VIRT config is mutually exclusive to XIP_KERNEL, XIP_KERNEL is used in > systems with NOR flash devices, and ZRELADDR config is mutually exclusive > to ZBOOT_ROM. > > CFT::Call For Testing > > Requesting maintainers of MSM platforms to evaluate the changes on the board > and comment, as I dont have the board for testing and also requesting an ACK > > Signed-off-by: panchaxari > Cc: David Brown > Cc: Daniel Walker > Cc: Bryan Huntsman > Cc: Russell King > Cc: Linus Walleij > Cc: linux-arm-...@vger.kernel.org > Cc: linux-arm-ker...@lists.infradead.org > Cc: linux-kernel@vger.kernel.org We run with these enabled on our development boards already. Tested-by: Courtney Cavin -Courtney -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7 2/2] mmc: sdhci-msm: Initial support for MSM chipsets
On Wed, Nov 06, 2013 at 04:56:45PM +0100, Georgi Djakov wrote: > This platform driver adds the initial support of Secure > Digital Host Controller Interface compliant controller > found in Qualcomm MSM chipsets. > > Signed-off-by: Georgi Djakov [...] > +static int sdhci_msm_probe(struct platform_device *pdev) > +{ > + struct sdhci_host *host; > + struct sdhci_pltfm_host *pltfm_host; > + struct sdhci_msm_host *msm_host; > + struct resource *core_memres = NULL; > + int ret, dead; > + u16 host_version; > + u32 irq_status, irq_ctl; > + > + if (!pdev->dev.of_node) { > + dev_err(>dev, "No device tree data\n"); > + return -ENOENT; > + } > + > + msm_host = devm_kzalloc(>dev, sizeof(*msm_host), GFP_KERNEL); > + if (!msm_host) > + return -ENOMEM; > + > + msm_host->sdhci_msm_pdata.ops = _msm_ops; > + host = sdhci_pltfm_init(pdev, _host->sdhci_msm_pdata, 0); > + if (IS_ERR(host)) { > + dev_err(mmc_dev(host->mmc), "sdhci_pltfm_init error\n"); > + return PTR_ERR(host); > + } > + > + pltfm_host = sdhci_priv(host); > + pltfm_host->priv = msm_host; > + msm_host->mmc = host->mmc; > + msm_host->pdev = pdev; > + > + ret = mmc_of_parse(host->mmc); Can we please add a call to sdhci_get_of_property(pdev) somewhere around here too? > + if (ret) { > + dev_err(>dev, "failed parsing mmc device tree\n"); > + goto pltfm_free; > + } > + > + ret = sdhci_msm_populate_pdata(>dev, _host->pdata); > + if (ret) { > + dev_err(>dev, "DT parsing error\n"); > + goto pltfm_free; > + } > + > + /* Setup SDCC bus voter clock. */ > + msm_host->bus_clk = devm_clk_get(>dev, "bus"); > + if (!IS_ERR(msm_host->bus_clk)) { > + /* Vote for max. clk rate for max. performance */ > + ret = clk_set_rate(msm_host->bus_clk, INT_MAX); > + if (ret) > + goto pltfm_free; > + ret = clk_prepare_enable(msm_host->bus_clk); > + if (ret) > + goto pltfm_free; > + } > + > + /* Setup main peripheral bus clock */ > + msm_host->pclk = devm_clk_get(>dev, "iface"); > + if (!IS_ERR(msm_host->pclk)) { > + ret = clk_prepare_enable(msm_host->pclk); > + if (ret) { > + dev_err(>dev, > + "Main peripheral clock setup fail (%d)\n", > + ret); > + goto bus_clk_disable; > + } > + } > + > + /* Setup SDC MMC clock */ > + msm_host->clk = devm_clk_get(>dev, "core"); > + if (IS_ERR(msm_host->clk)) { > + ret = PTR_ERR(msm_host->clk); > + dev_err(>dev, "SDC MMC clock setup fail (%d)\n", ret); > + goto pclk_disable; > + } > + > + ret = clk_prepare_enable(msm_host->clk); > + if (ret) > + goto pclk_disable; > + > + /* Setup regulators */ > + ret = sdhci_msm_vreg_init(>dev, _host->pdata); > + if (ret) { > + if (ret != -EPROBE_DEFER) > + dev_err(>dev, "Regulator setup fail (%d)\n", > ret); > + goto clk_disable; > + } > + > + core_memres = platform_get_resource_byname(pdev, > + IORESOURCE_MEM, > "core_mem"); > + msm_host->core_mem = devm_ioremap_resource(>dev, core_memres); > + > + if (IS_ERR(msm_host->core_mem)) { > + dev_err(>dev, "Failed to remap registers\n"); > + ret = PTR_ERR(msm_host->core_mem); > + goto vreg_disable; > + } > + > + /* Reset the core and Enable SDHC mode */ > + writel_relaxed(readl_relaxed(msm_host->core_mem + CORE_POWER) | > + CORE_SW_RST, msm_host->core_mem + CORE_POWER); > + > + /* SW reset can take upto 10HCLK + 15MCLK cycles. (min 40us) */ > + usleep_range(1000, 5000); > + if (readl(msm_host->core_mem + CORE_POWER) & CORE_SW_RST) { > + dev_err(>dev, "Stuck in reset\n"); > + ret = -ETIMEDOUT; > + goto vreg_disable; > + } > + > + /* Set HC_MODE_EN bit in HC_MODE register */ > + writel_relaxed(HC_MODE_EN, (msm_host->core_mem + CORE_HC_MODE)); > + > + /* > +* CORE_SW_RST above may trigger power irq if previous status of > PWRCTL > +* was either BUS_ON or IO_HIGH_V. So before we enable the power irq > +* interrupt in GIC (by registering the interrupt handler), we need to > +* ensure that any pending power irq interrupt status is acknowledged > +* otherwise power irq interrupt handler would be fired prematurely. > +*/ > + irq_status =
Re: [PATCH v7 2/2] mmc: sdhci-msm: Initial support for MSM chipsets
On Wed, Nov 06, 2013 at 04:56:45PM +0100, Georgi Djakov wrote: This platform driver adds the initial support of Secure Digital Host Controller Interface compliant controller found in Qualcomm MSM chipsets. Signed-off-by: Georgi Djakov gdja...@mm-sol.com [...] +static int sdhci_msm_probe(struct platform_device *pdev) +{ + struct sdhci_host *host; + struct sdhci_pltfm_host *pltfm_host; + struct sdhci_msm_host *msm_host; + struct resource *core_memres = NULL; + int ret, dead; + u16 host_version; + u32 irq_status, irq_ctl; + + if (!pdev-dev.of_node) { + dev_err(pdev-dev, No device tree data\n); + return -ENOENT; + } + + msm_host = devm_kzalloc(pdev-dev, sizeof(*msm_host), GFP_KERNEL); + if (!msm_host) + return -ENOMEM; + + msm_host-sdhci_msm_pdata.ops = sdhci_msm_ops; + host = sdhci_pltfm_init(pdev, msm_host-sdhci_msm_pdata, 0); + if (IS_ERR(host)) { + dev_err(mmc_dev(host-mmc), sdhci_pltfm_init error\n); + return PTR_ERR(host); + } + + pltfm_host = sdhci_priv(host); + pltfm_host-priv = msm_host; + msm_host-mmc = host-mmc; + msm_host-pdev = pdev; + + ret = mmc_of_parse(host-mmc); Can we please add a call to sdhci_get_of_property(pdev) somewhere around here too? + if (ret) { + dev_err(pdev-dev, failed parsing mmc device tree\n); + goto pltfm_free; + } + + ret = sdhci_msm_populate_pdata(pdev-dev, msm_host-pdata); + if (ret) { + dev_err(pdev-dev, DT parsing error\n); + goto pltfm_free; + } + + /* Setup SDCC bus voter clock. */ + msm_host-bus_clk = devm_clk_get(pdev-dev, bus); + if (!IS_ERR(msm_host-bus_clk)) { + /* Vote for max. clk rate for max. performance */ + ret = clk_set_rate(msm_host-bus_clk, INT_MAX); + if (ret) + goto pltfm_free; + ret = clk_prepare_enable(msm_host-bus_clk); + if (ret) + goto pltfm_free; + } + + /* Setup main peripheral bus clock */ + msm_host-pclk = devm_clk_get(pdev-dev, iface); + if (!IS_ERR(msm_host-pclk)) { + ret = clk_prepare_enable(msm_host-pclk); + if (ret) { + dev_err(pdev-dev, + Main peripheral clock setup fail (%d)\n, + ret); + goto bus_clk_disable; + } + } + + /* Setup SDC MMC clock */ + msm_host-clk = devm_clk_get(pdev-dev, core); + if (IS_ERR(msm_host-clk)) { + ret = PTR_ERR(msm_host-clk); + dev_err(pdev-dev, SDC MMC clock setup fail (%d)\n, ret); + goto pclk_disable; + } + + ret = clk_prepare_enable(msm_host-clk); + if (ret) + goto pclk_disable; + + /* Setup regulators */ + ret = sdhci_msm_vreg_init(pdev-dev, msm_host-pdata); + if (ret) { + if (ret != -EPROBE_DEFER) + dev_err(pdev-dev, Regulator setup fail (%d)\n, ret); + goto clk_disable; + } + + core_memres = platform_get_resource_byname(pdev, + IORESOURCE_MEM, core_mem); + msm_host-core_mem = devm_ioremap_resource(pdev-dev, core_memres); + + if (IS_ERR(msm_host-core_mem)) { + dev_err(pdev-dev, Failed to remap registers\n); + ret = PTR_ERR(msm_host-core_mem); + goto vreg_disable; + } + + /* Reset the core and Enable SDHC mode */ + writel_relaxed(readl_relaxed(msm_host-core_mem + CORE_POWER) | + CORE_SW_RST, msm_host-core_mem + CORE_POWER); + + /* SW reset can take upto 10HCLK + 15MCLK cycles. (min 40us) */ + usleep_range(1000, 5000); + if (readl(msm_host-core_mem + CORE_POWER) CORE_SW_RST) { + dev_err(pdev-dev, Stuck in reset\n); + ret = -ETIMEDOUT; + goto vreg_disable; + } + + /* Set HC_MODE_EN bit in HC_MODE register */ + writel_relaxed(HC_MODE_EN, (msm_host-core_mem + CORE_HC_MODE)); + + /* +* CORE_SW_RST above may trigger power irq if previous status of PWRCTL +* was either BUS_ON or IO_HIGH_V. So before we enable the power irq +* interrupt in GIC (by registering the interrupt handler), we need to +* ensure that any pending power irq interrupt status is acknowledged +* otherwise power irq interrupt handler would be fired prematurely. +*/ + irq_status = readl_relaxed(msm_host-core_mem + CORE_PWRCTL_STATUS); + writel_relaxed(irq_status, (msm_host-core_mem +
Re: [PATCH CFT] ARM:MSM: Enable ARM_PATCH_PHYS_VIRT and AUTO_ZRELADDR default
On Mon, Dec 09, 2013 at 10:40:50AM +0100, panchaxari wrote: ARM_PATCH_PHYS_VIRT and AUTO_ZRELADDR have been enabled as default configs to MSM platform Introduction of PHYS_VIRT config as default would enable phy-to-virt and virt-to-phy translation function at boot and module loading time and enforce dynamic reallocation of memory. AUTO_ZRELADDR config would enable calculation of kernel load address at run time. PHYS_VIRT config is mutually exclusive to XIP_KERNEL, XIP_KERNEL is used in systems with NOR flash devices, and ZRELADDR config is mutually exclusive to ZBOOT_ROM. CFT::Call For Testing Requesting maintainers of MSM platforms to evaluate the changes on the board and comment, as I dont have the board for testing and also requesting an ACK Signed-off-by: panchaxari panchaxari.prasannamur...@linaro.org Cc: David Brown dav...@codeaurora.org Cc: Daniel Walker dwal...@fifo99.com Cc: Bryan Huntsman bry...@codeaurora.org Cc: Russell King li...@arm.linux.org.uk Cc: Linus Walleij linus.wall...@linaro.org Cc: linux-arm-...@vger.kernel.org Cc: linux-arm-ker...@lists.infradead.org Cc: linux-kernel@vger.kernel.org We run with these enabled on our development boards already. Tested-by: Courtney Cavin courtney.ca...@sonymobile.com -Courtney -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] regmap: make sure we unlock on failure in regmap_bulk_write
Signed-off-by: Courtney Cavin --- drivers/base/regmap/regmap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c index 9c021d9..d1a9141 100644 --- a/drivers/base/regmap/regmap.c +++ b/drivers/base/regmap/regmap.c @@ -1549,7 +1549,7 @@ int regmap_bulk_write(struct regmap *map, unsigned int reg, const void *val, val + (i * val_bytes), val_bytes); if (ret != 0) - return ret; + goto out; } } else { ret = _regmap_raw_write(map, reg, wval, val_bytes * val_count); -- 1.8.1.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] regmap: make sure we unlock on failure in regmap_bulk_write
Signed-off-by: Courtney Cavin courtney.ca...@sonymobile.com --- drivers/base/regmap/regmap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c index 9c021d9..d1a9141 100644 --- a/drivers/base/regmap/regmap.c +++ b/drivers/base/regmap/regmap.c @@ -1549,7 +1549,7 @@ int regmap_bulk_write(struct regmap *map, unsigned int reg, const void *val, val + (i * val_bytes), val_bytes); if (ret != 0) - return ret; + goto out; } } else { ret = _regmap_raw_write(map, reg, wval, val_bytes * val_count); -- 1.8.1.5 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/