diff mbox series

[11/17] soc: ti: pruss: add pruss_get()/put() API

Message ID 1542886753-17625-12-git-send-email-rogerq@ti.com (mailing list archive)
State New, archived
Headers show
Series Add support for TI PRU ICSS | expand

Commit Message

Roger Quadros Nov. 22, 2018, 11:39 a.m. UTC 
From: Tero Kristo <t-kristo@ti.com>

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 <t-kristo@ti.com>
[s-anna@ti.com: various fixups and cleanups]
Signed-off-by: Suman Anna <s-anna@ti.com>
---
 drivers/soc/ti/pruss.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pruss.h  | 10 +++++++++
 2 files changed, 67 insertions(+)

Comments

kernel test robot Nov. 23, 2018, 2:47 a.m. UTC | #1 
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
Arnd Bergmann Nov. 23, 2018, 8:20 a.m. UTC | #2 
On Thu, Nov 22, 2018 at 12:41 PM Roger Quadros <rogerq@ti.com> 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
Tero Kristo Nov. 23, 2018, 8:58 a.m. UTC | #3 
On 23/11/2018 10:20, Arnd Bergmann wrote:
> On Thu, Nov 22, 2018 at 12:41 PM Roger Quadros <rogerq@ti.com> 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
Roger Quadros Nov. 23, 2018, 9:40 a.m. UTC | #4 
On 23/11/18 10:20, Arnd Bergmann wrote:
> On Thu, Nov 22, 2018 at 12:41 PM Roger Quadros <rogerq@ti.com> 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
Roger Quadros Nov. 23, 2018, 9:41 a.m. UTC | #5 
On 23/11/18 04:47, kbuild test robot wrote:
> 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);
>                                    ^~~~~

This was my bad due to re-ordering of patches. Fixed now.

cheers,
-roger

> --
>    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
>
David Lechner Nov. 26, 2018, 9:18 p.m. UTC | #6 
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;
diff mbox series

Patch

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)