From: Tero Kristo <[email protected]>
Add two new get and put API, pruss_get() and pruss_put(), to the
PRUSS platform driver to allow client drivers to request a handle
to a PRUSS device. This handle will be used by client drivers to
request various operations of the PRUSS platform driver through
additional API that will be added in the following patches.
The pruss_get() function returns the pruss handle corresponding
to a PRUSS device referenced by a PRU remoteproc instance. The
pruss_put() is the complimentary function to pruss_get().
Signed-off-by: Tero Kristo <[email protected]>
[[email protected]: various fixups and cleanups]
Signed-off-by: Suman Anna <[email protected]>
---
drivers/soc/ti/pruss.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/pruss.h | 10 +++++++++
2 files changed, 67 insertions(+)
diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
index c2271c4..90ee5b9 100644
--- a/drivers/soc/ti/pruss.c
+++ b/drivers/soc/ti/pruss.c
@@ -13,10 +13,67 @@
#include <linux/of_address.h>
#include <linux/of_device.h>
#include <linux/pruss.h>
+#include <linux/remoteproc.h>
#include "pruss.h"
/**
+ * pruss_get() - get the pruss for a given PRU remoteproc
+ * @rproc: remoteproc handle of a PRU instance
+ *
+ * Finds the parent pruss device for a PRU given the @rproc handle of the
+ * PRU remote processor. This function increments the pruss device's refcount,
+ * so always use pruss_put() to decrement it back once pruss isn't needed
+ * anymore.
+ *
+ * Returns the pruss handle on success, and an ERR_PTR on failure using one
+ * of the following error values
+ * -EINVAL if invalid parameter
+ * -ENODEV if PRU device or PRUSS device is not found
+ */
+struct pruss *pruss_get(struct rproc *rproc)
+{
+ struct pruss *pruss;
+ struct device *dev;
+ struct platform_device *ppdev;
+
+ if (IS_ERR_OR_NULL(rproc))
+ return ERR_PTR(-EINVAL);
+
+ dev = &rproc->dev;
+ if (!dev->parent)
+ return ERR_PTR(-ENODEV);
+
+ /* rudimentary check to make sure rproc handle is for a PRU */
+ if (!strstr(dev_name(dev->parent), "pru"))
+ return ERR_PTR(-ENODEV);
+
+ ppdev = to_platform_device(dev->parent->parent);
+ pruss = platform_get_drvdata(ppdev);
+ if (pruss)
+ get_device(pruss->dev);
+
+ return pruss ? pruss : ERR_PTR(-ENODEV);
+}
+EXPORT_SYMBOL_GPL(pruss_get);
+
+/**
+ * pruss_put() - decrement pruss device's usecount
+ * @pruss: pruss handle
+ *
+ * Complimentary function for pruss_get(). Needs to be called
+ * after the PRUSS is used, and only if the pruss_get() succeeds.
+ */
+void pruss_put(struct pruss *pruss)
+{
+ if (IS_ERR_OR_NULL(pruss))
+ return;
+
+ put_device(pruss->dev);
+}
+EXPORT_SYMBOL_GPL(pruss_put);
+
+/**
* pruss_request_mem_region() - request a memory resource
* @pruss: the pruss instance
* @mem_id: the memory resource id
diff --git a/include/linux/pruss.h b/include/linux/pruss.h
index 768b698..b9135d6 100644
--- a/include/linux/pruss.h
+++ b/include/linux/pruss.h
@@ -36,6 +36,9 @@ struct pruss;
#if IS_ENABLED(CONFIG_TI_PRUSS)
+struct pruss *pruss_get(struct rproc *rproc);
+void pruss_put(struct pruss *pruss);
+
int pruss_request_mem_region(struct pruss *pruss, enum pruss_mem mem_id,
struct pruss_mem_region *region);
int pruss_release_mem_region(struct pruss *pruss,
@@ -45,6 +48,13 @@ int pruss_intc_trigger(unsigned int irq);
#else
+static inline struct pruss *pruss_get(struct rproc *rproc)
+{
+ return ERR_PTR(-ENOTSUPP);
+}
+
+static inline void pruss_put(struct pruss *pruss) { }
+
static inline int pruss_request_mem_region(struct pruss *pruss,
enum pruss_mem mem_id,
struct pruss_mem_region *region)
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Hi Tero,
I love your patch! Yet something to improve:
[auto build test ERROR on keystone/next]
[also build test ERROR on v4.20-rc3 next-20181122]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Roger-Quadros/Add-support-for-TI-PRU-ICSS/20181123-083903
base: https://git.kernel.org/pub/scm/linux/kernel/git/ssantosh/linux-keystone.git next
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=arm
All error/warnings (new ones prefixed by >>):
In file included from drivers/soc/ti/pruss_intc.c:16:0:
>> include/linux/pruss.h:39:32: warning: 'struct rproc' declared inside parameter list will not be visible outside of this definition or declaration
struct pruss *pruss_get(struct rproc *rproc);
^~~~~
--
In file included from drivers/soc/ti/pruss.c:15:0:
>> include/linux/pruss.h:39:32: warning: 'struct rproc' declared inside parameter list will not be visible outside of this definition or declaration
struct pruss *pruss_get(struct rproc *rproc);
^~~~~
>> drivers/soc/ti/pruss.c:34:15: error: conflicting types for 'pruss_get'
struct pruss *pruss_get(struct rproc *rproc)
^~~~~~~~~
In file included from drivers/soc/ti/pruss.c:15:0:
include/linux/pruss.h:39:15: note: previous declaration of 'pruss_get' was here
struct pruss *pruss_get(struct rproc *rproc);
^~~~~~~~~
In file included from include/linux/linkage.h:7:0,
from include/linux/kernel.h:7,
from include/linux/list.h:9,
from include/linux/kobject.h:19,
from include/linux/device.h:16,
from include/linux/dma-mapping.h:7,
from drivers/soc/ti/pruss.c:10:
drivers/soc/ti/pruss.c:58:19: error: conflicting types for 'pruss_get'
EXPORT_SYMBOL_GPL(pruss_get);
^
include/linux/export.h:79:21: note: in definition of macro '___EXPORT_SYMBOL'
extern typeof(sym) sym; \
^~~
>> drivers/soc/ti/pruss.c:58:1: note: in expansion of macro 'EXPORT_SYMBOL_GPL'
EXPORT_SYMBOL_GPL(pruss_get);
^~~~~~~~~~~~~~~~~
In file included from drivers/soc/ti/pruss.c:15:0:
include/linux/pruss.h:39:15: note: previous declaration of 'pruss_get' was here
struct pruss *pruss_get(struct rproc *rproc);
^~~~~~~~~
--
In file included from drivers/soc//ti/pruss.c:15:0:
>> include/linux/pruss.h:39:32: warning: 'struct rproc' declared inside parameter list will not be visible outside of this definition or declaration
struct pruss *pruss_get(struct rproc *rproc);
^~~~~
drivers/soc//ti/pruss.c:34:15: error: conflicting types for 'pruss_get'
struct pruss *pruss_get(struct rproc *rproc)
^~~~~~~~~
In file included from drivers/soc//ti/pruss.c:15:0:
include/linux/pruss.h:39:15: note: previous declaration of 'pruss_get' was here
struct pruss *pruss_get(struct rproc *rproc);
^~~~~~~~~
In file included from include/linux/linkage.h:7:0,
from include/linux/kernel.h:7,
from include/linux/list.h:9,
from include/linux/kobject.h:19,
from include/linux/device.h:16,
from include/linux/dma-mapping.h:7,
from drivers/soc//ti/pruss.c:10:
drivers/soc//ti/pruss.c:58:19: error: conflicting types for 'pruss_get'
EXPORT_SYMBOL_GPL(pruss_get);
^
include/linux/export.h:79:21: note: in definition of macro '___EXPORT_SYMBOL'
extern typeof(sym) sym; \
^~~
drivers/soc//ti/pruss.c:58:1: note: in expansion of macro 'EXPORT_SYMBOL_GPL'
EXPORT_SYMBOL_GPL(pruss_get);
^~~~~~~~~~~~~~~~~
In file included from drivers/soc//ti/pruss.c:15:0:
include/linux/pruss.h:39:15: note: previous declaration of 'pruss_get' was here
struct pruss *pruss_get(struct rproc *rproc);
^~~~~~~~~
vim +/pruss_get +34 drivers/soc/ti/pruss.c
> 10 #include <linux/dma-mapping.h>
11 #include <linux/module.h>
12 #include <linux/io.h>
13 #include <linux/of_address.h>
14 #include <linux/of_device.h>
> 15 #include <linux/pruss.h>
16 #include <linux/remoteproc.h>
17
18 #include "pruss.h"
19
20 /**
21 * pruss_get() - get the pruss for a given PRU remoteproc
22 * @rproc: remoteproc handle of a PRU instance
23 *
24 * Finds the parent pruss device for a PRU given the @rproc handle of the
25 * PRU remote processor. This function increments the pruss device's refcount,
26 * so always use pruss_put() to decrement it back once pruss isn't needed
27 * anymore.
28 *
29 * Returns the pruss handle on success, and an ERR_PTR on failure using one
30 * of the following error values
31 * -EINVAL if invalid parameter
32 * -ENODEV if PRU device or PRUSS device is not found
33 */
> 34 struct pruss *pruss_get(struct rproc *rproc)
35 {
36 struct pruss *pruss;
37 struct device *dev;
38 struct platform_device *ppdev;
39
40 if (IS_ERR_OR_NULL(rproc))
41 return ERR_PTR(-EINVAL);
42
43 dev = &rproc->dev;
44 if (!dev->parent)
45 return ERR_PTR(-ENODEV);
46
47 /* rudimentary check to make sure rproc handle is for a PRU */
48 if (!strstr(dev_name(dev->parent), "pru"))
49 return ERR_PTR(-ENODEV);
50
51 ppdev = to_platform_device(dev->parent->parent);
52 pruss = platform_get_drvdata(ppdev);
53 if (pruss)
54 get_device(pruss->dev);
55
56 return pruss ? pruss : ERR_PTR(-ENODEV);
57 }
> 58 EXPORT_SYMBOL_GPL(pruss_get);
59
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Thu, Nov 22, 2018 at 12:41 PM Roger Quadros <[email protected]> wrote:
> +
> + if (IS_ERR_OR_NULL(rproc))
> + return ERR_PTR(-EINVAL);
Any usage of IS_ERR_OR_NULL() tends to be an indication of a badly
designed API. Please change this to allow only one of the two to be passed
around.
Arnd
On 23/11/2018 10:20, Arnd Bergmann wrote:
> On Thu, Nov 22, 2018 at 12:41 PM Roger Quadros <[email protected]> wrote:
>
>> +
>> + if (IS_ERR_OR_NULL(rproc))
>> + return ERR_PTR(-EINVAL);
>
>
> Any usage of IS_ERR_OR_NULL() tends to be an indication of a badly
> designed API. Please change this to allow only one of the two to be passed
> around.
Should this be deprecated then?
-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 23/11/18 10:20, Arnd Bergmann wrote:
> On Thu, Nov 22, 2018 at 12:41 PM Roger Quadros <[email protected]> wrote:
>
>> +
>> + if (IS_ERR_OR_NULL(rproc))
>> + return ERR_PTR(-EINVAL);
>
>
> Any usage of IS_ERR_OR_NULL() tends to be an indication of a badly
> designed API. Please change this to allow only one of the two to be passed
> around.
The user of this API has to call pru_rproc_get() which never returns NULL.
So IS_ERR() should suffice here.
>
> Arnd
>
cheers,
-roger
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 11/22/18 5:39 AM, Roger Quadros wrote:
> + pruss = platform_get_drvdata(ppdev);
> + if (pruss)
> + get_device(pruss->dev);
> +
> + return pruss ? pruss : ERR_PTR(-ENODEV);
> +}
This might be a bit easier to follow if we handle the error
path first:
if (!pruss)
return ERR_PTR(-ENODEV);
get_device(pruss->dev);
return pruss;