diff mbox

reset: generate reset_control_get variants with macro expansion

Message ID 1469077523-23163-1-git-send-email-yamada.masahiro@socionext.com
State New
Headers show

Commit Message

Masahiro Yamada July 21, 2016, 5:05 a.m. UTC 
The recent update in the reset subsystem requires all reset consumers
to be explicit about the requested reset lines; _explicit or _shared.
This effectively doubled the number of reset_control_get variants.
Also, we already had _optional variants.

We see some pattern in the reset_control_get APIs.

There are 6 base functions in terms of function prototype:
  reset_control_get
  reset_control_get_by_index
  of_reset_control_get
  of_reset_control_get_by_index
  devm_reset_control_get
  devm_reset_control_get_by_index

Each of them has 4 variants with the following suffixes:
  _exclusive
  _shared
  _optional_exclusive
  _optional_shared

The exhaustive set of functions (6 * 4) can be generated with macro
expansion.  It will mitigate the pain of maintaining proliferating
APIs.

I merged similar comment blocks into two, for functions in core.c
because copy-paste work for similar comment blocks is error-prone.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

---

 drivers/reset/core.c  |  36 ++++++
 include/linux/reset.h | 306 +++++++++-----------------------------------------
 2 files changed, 86 insertions(+), 256 deletions(-)

-- 
1.9.1

Comments

Masahiro Yamada July 21, 2016, 9:53 a.m. UTC | #1 
Hi Philipp,


2016-07-21 17:41 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
> Hi Masahiro,

>

> Am Donnerstag, den 21.07.2016, 14:05 +0900 schrieb Masahiro Yamada:

>> The recent update in the reset subsystem requires all reset consumers

>> to be explicit about the requested reset lines; _explicit or _shared.

>> This effectively doubled the number of reset_control_get variants.

>> Also, we already had _optional variants.

>>

>> We see some pattern in the reset_control_get APIs.

>>

>> There are 6 base functions in terms of function prototype:

>>   reset_control_get

>>   reset_control_get_by_index

>>   of_reset_control_get

>>   of_reset_control_get_by_index

>>   devm_reset_control_get

>>   devm_reset_control_get_by_index

>>

>> Each of them has 4 variants with the following suffixes:

>>   _exclusive

>>   _shared

>>   _optional_exclusive

>>   _optional_shared

>>

>> The exhaustive set of functions (6 * 4) can be generated with macro

>> expansion.  It will mitigate the pain of maintaining proliferating

>> APIs.

>>

>> I merged similar comment blocks into two, for functions in core.c

>> because copy-paste work for similar comment blocks is error-prone.

>>

>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

>

> Thank you for the patch, but while I'm all for reducing unnecessary

> duplication, I do not like this change for two reasons:

> First, the macro generated API functions can not be found by name using

> tools like ctags or grep anymore.



Linux does this here and there to
not repeat similar function defines.
This is an improvement as we have done from time to time.

For ctags, scripts/tags.sh exists for that purpose, doesn't it?

For example,
commit e0e5070b20e01f0321f97db4e4e174f3f6b49e50
converted func defines and adjusted tags.sh at the same time.



> And secondly, we would now have detailed kerneldoc comments for two

> functions that are never called directly, but the public facing API

> itself is completely without documentation. I wouldn't mind removing

> duplicated documentation paragraphs though, for example by referencing

> reset_control_get_* from the of_reset_control_get_* kerneldoc comments.


Linux is not always kind enough to document every public API.

As you see, we generally do not document
static inline wrappers.

In our case, they are all simply derived from two base functions.
Documenting the two is enough.


I need to expand drivers/usb/host/ehci-platform.c
to support multiple reset lines, so I really want
devm_reset_control_get_optional_shared_by_index() supported.

(So, probably
devm_reset_control_get_exclusive_by_index
devm_reset_control_get_shared_by_index
devm_reset_control_get_optional_exclusive_by_index
as well)

If you are unhappy with this patch,
I will send an alternative one,
which adds 4 more functions in a stupid and straightforward way.


> I think when Lee suggested the API change, I initially leaned towards a

> single entry point with flags, similar to the irq request API:

>   reset_control_get(..., RSTF_EXCLUSIVE)

>   reset_control_get(..., RSTF_SHARED)

>   reset_control_get(..., RSTF_OPTIONAL | RSTF_EXCLUSIVE)

>   reset_control_get(..., RSTF_OPTIONAL | RSTF_SHARED)

> On the other hand, with that API users could forget the flags or use

> incompatible combinations, which is impossible with the current API.


Right.

I agree that having dedicated functions is a better idea than flags.

-- 
Best Regards
Masahiro Yamada
Lee Jones July 21, 2016, 12:06 p.m. UTC | #2 
On Thu, 21 Jul 2016, Masahiro Yamada wrote:
> 2016-07-21 17:41 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:

> > Am Donnerstag, den 21.07.2016, 14:05 +0900 schrieb Masahiro Yamada:

> >> The recent update in the reset subsystem requires all reset consumers

> >> to be explicit about the requested reset lines; _explicit or _shared.

> >> This effectively doubled the number of reset_control_get variants.

> >> Also, we already had _optional variants.

> >>

> >> We see some pattern in the reset_control_get APIs.

> >>

> >> There are 6 base functions in terms of function prototype:

> >>   reset_control_get

> >>   reset_control_get_by_index

> >>   of_reset_control_get

> >>   of_reset_control_get_by_index

> >>   devm_reset_control_get

> >>   devm_reset_control_get_by_index

> >>

> >> Each of them has 4 variants with the following suffixes:

> >>   _exclusive

> >>   _shared

> >>   _optional_exclusive

> >>   _optional_shared

> >>

> >> The exhaustive set of functions (6 * 4) can be generated with macro

> >> expansion.  It will mitigate the pain of maintaining proliferating

> >> APIs.

> >>

> >> I merged similar comment blocks into two, for functions in core.c

> >> because copy-paste work for similar comment blocks is error-prone.

> >>

> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>


[...]

I did entertain several ways of reducing this API myself, so I know
where you are coming from but ...

> > I think when Lee suggested the API change, I initially leaned towards a

> > single entry point with flags, similar to the irq request API:

> >   reset_control_get(..., RSTF_EXCLUSIVE)

> >   reset_control_get(..., RSTF_SHARED)

> >   reset_control_get(..., RSTF_OPTIONAL | RSTF_EXCLUSIVE)

> >   reset_control_get(..., RSTF_OPTIONAL | RSTF_SHARED)

> > On the other hand, with that API users could forget the flags or use

> > incompatible combinations, which is impossible with the current API.


This is the age-old 'lines-of-code over {grep,read,maintain}ability'
argument, and I'm afraid to say that this patch, although saves some
lines, the {grep,read,maintain}ability takes too much of a big hit
IMHO.

As a developer, I find it very annoying when I can't directly grep for
functions that have been written in a 'clever' (read 'obfuscated')
way.  And since this is a) a header file and b) all of the verboseness
is in a single/central place, it's better left as it is.

I'd entertain the FLAGS idea, since this is very common in the kernel
and users/developers know their way around those already, but MACRO
driven function names, no way!

> If you are unhappy with this patch,

> I will send an alternative one,

> which adds 4 more functions in a stupid and straightforward way.


Yes, 'simple' and straightforward is what we're looking for and is the
way forward.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
diff mbox

Patch

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index e21251d..dd536c3 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -257,6 +257,30 @@  static void __reset_control_put(struct reset_control *rstc)
 	kfree(rstc);
 }
 
+/**
+ * __of_reset_control_get - Lookup and obtain a reference to a reset controller.
+ * @node: device to be reset by the controller
+ * @id: optional reset line name
+ * @index: index of the reset line (valid iif @id is NULL)
+ * @shared: shareability of reset control
+ *
+ * Returns a struct reset_control or IS_ERR() condition containing errno.
+ * The target reset line can be specified by either @id or @index.
+ *
+ * Set the @shared flag for use with reset-controls which are shared between
+ * multiple hardware blocks. When a reset-control is shared, the behavior of
+ * reset_control_assert / deassert is changed; the reset-core will keep track
+ * of a deassert_count and only (re-)assert the reset after reset_control_assert
+ * has been called as many times as reset_control_deassert was called. Calling
+ * reset_control_assert without first calling reset_control_deassert is not
+ * allowed on a shared reset control. Calling reset_control_reset is also not
+ * allowed on a shared reset control. Also see the remark about shared reset
+ * controls in the reset_control_assert docs.
+ *
+ * If the @shared flag is not set, this function gets an exclusive reference to
+ * a reset controller; if this function is called more then once for the same
+ * reset_control it will return -EBUSY.
+ */
 struct reset_control *__of_reset_control_get(struct device_node *node,
 				     const char *id, int index, int shared)
 {
@@ -337,6 +361,18 @@  static void devm_reset_control_release(struct device *dev, void *res)
 	reset_control_put(*(struct reset_control **)res);
 }
 
+/**
+ * __devm_reset_control_get - resource managed __of_reset_control_get()
+ * @dev: device to be reset by the controller
+ * @id: optional reset line name
+ * @index: index of the reset line (valid iif @id is NULL)
+ * @shared: shareability of reset control
+ *
+ * Managed __of_reset_control_get(). For reset controllers returned from this
+ * function, reset_control_put() is called automatically on driver detach.
+ *
+ * See __of_reset_control_get() for more information.
+ */
 struct reset_control *__devm_reset_control_get(struct device *dev,
 				     const char *id, int index, int shared)
 {
diff --git a/include/linux/reset.h b/include/linux/reset.h
index cb7db61..5894f0f 100644
--- a/include/linux/reset.h
+++ b/include/linux/reset.h
@@ -83,262 +83,56 @@  static inline struct reset_control *__devm_reset_control_get(
 
 #endif /* CONFIG_RESET_CONTROLLER */
 
-/**
- * reset_control_get_exclusive - Lookup and obtain an exclusive reference
- *                               to a reset controller.
- * @dev: device to be reset by the controller
- * @id: reset line name
- *
- * Returns a struct reset_control or IS_ERR() condition containing errno.
- * If this function is called more then once for the same reset_control it will
- * return -EBUSY.
- *
- * See reset_control_get_shared for details on shared references to
- * reset-controls.
- *
- * Use of id names is optional.
- */
-static inline struct reset_control *
-__must_check reset_control_get_exclusive(struct device *dev, const char *id)
-{
-#ifndef CONFIG_RESET_CONTROLLER
-	WARN_ON(1);
-#endif
-	return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, 0);
-}
-
-/**
- * reset_control_get_shared - Lookup and obtain a shared reference to a
- *                            reset controller.
- * @dev: device to be reset by the controller
- * @id: reset line name
- *
- * Returns a struct reset_control or IS_ERR() condition containing errno.
- * This function is intended for use with reset-controls which are shared
- * between hardware-blocks.
- *
- * When a reset-control is shared, the behavior of reset_control_assert /
- * deassert is changed, the reset-core will keep track of a deassert_count
- * and only (re-)assert the reset after reset_control_assert has been called
- * as many times as reset_control_deassert was called. Also see the remark
- * about shared reset-controls in the reset_control_assert docs.
- *
- * Calling reset_control_assert without first calling reset_control_deassert
- * is not allowed on a shared reset control. Calling reset_control_reset is
- * also not allowed on a shared reset control.
- *
- * Use of id names is optional.
- */
-static inline struct reset_control *reset_control_get_shared(
-					struct device *dev, const char *id)
-{
-#ifndef CONFIG_RESET_CONTROLLER
-	WARN_ON(1);
-#endif
-	return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, 1);
-}
-
-static inline struct reset_control *reset_control_get_optional_exclusive(
-					struct device *dev, const char *id)
-{
-	return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, 0);
-}
-
-static inline struct reset_control *reset_control_get_optional_shared(
-					struct device *dev, const char *id)
-{
-	return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, 1);
-}
-
-/**
- * of_reset_control_get_exclusive - Lookup and obtain an exclusive reference
- *                                  to a reset controller.
- * @node: device to be reset by the controller
- * @id: reset line name
- *
- * Returns a struct reset_control or IS_ERR() condition containing errno.
- *
- * Use of id names is optional.
- */
-static inline struct reset_control *of_reset_control_get_exclusive(
-				struct device_node *node, const char *id)
-{
-#ifndef CONFIG_RESET_CONTROLLER
-	WARN_ON(1);
-#endif
-	return __of_reset_control_get(node, id, 0, 0);
-}
-
-/**
- * of_reset_control_get_shared - Lookup and obtain an shared reference
- *                               to a reset controller.
- * @node: device to be reset by the controller
- * @id: reset line name
- *
- * When a reset-control is shared, the behavior of reset_control_assert /
- * deassert is changed, the reset-core will keep track of a deassert_count
- * and only (re-)assert the reset after reset_control_assert has been called
- * as many times as reset_control_deassert was called. Also see the remark
- * about shared reset-controls in the reset_control_assert docs.
- *
- * Calling reset_control_assert without first calling reset_control_deassert
- * is not allowed on a shared reset control. Calling reset_control_reset is
- * also not allowed on a shared reset control.
- * Returns a struct reset_control or IS_ERR() condition containing errno.
- *
- * Use of id names is optional.
- */
-static inline struct reset_control *of_reset_control_get_shared(
-				struct device_node *node, const char *id)
-{
-#ifndef CONFIG_RESET_CONTROLLER
-	WARN_ON(1);
-#endif
-	return __of_reset_control_get(node, id, 0, 1);
-}
-
-/**
- * of_reset_control_get_exclusive_by_index - Lookup and obtain an exclusive
- *                                           reference to a reset controller
- *                                           by index.
- * @node: device to be reset by the controller
- * @index: index of the reset controller
- *
- * This is to be used to perform a list of resets for a device or power domain
- * in whatever order. Returns a struct reset_control or IS_ERR() condition
- * containing errno.
- */
-static inline struct reset_control *of_reset_control_get_exclusive_by_index(
-					struct device_node *node, int index)
-{
-#ifndef CONFIG_RESET_CONTROLLER
-	WARN_ON(1);
-#endif
-	return __of_reset_control_get(node, NULL, index, 0);
-}
-
-/**
- * of_reset_control_get_shared_by_index - Lookup and obtain an shared
- *                                        reference to a reset controller
- *                                        by index.
- * @node: device to be reset by the controller
- * @index: index of the reset controller
- *
- * When a reset-control is shared, the behavior of reset_control_assert /
- * deassert is changed, the reset-core will keep track of a deassert_count
- * and only (re-)assert the reset after reset_control_assert has been called
- * as many times as reset_control_deassert was called. Also see the remark
- * about shared reset-controls in the reset_control_assert docs.
- *
- * Calling reset_control_assert without first calling reset_control_deassert
- * is not allowed on a shared reset control. Calling reset_control_reset is
- * also not allowed on a shared reset control.
- * Returns a struct reset_control or IS_ERR() condition containing errno.
- *
- * This is to be used to perform a list of resets for a device or power domain
- * in whatever order. Returns a struct reset_control or IS_ERR() condition
- * containing errno.
- */
-static inline struct reset_control *of_reset_control_get_shared_by_index(
-					struct device_node *node, int index)
-{
-#ifndef CONFIG_RESET_CONTROLLER
-	WARN_ON(1);
-#endif
-	return __of_reset_control_get(node, NULL, index, 1);
-}
-
-/**
- * devm_reset_control_get_exclusive - resource managed
- *                                    reset_control_get_exclusive()
- * @dev: device to be reset by the controller
- * @id: reset line name
- *
- * Managed reset_control_get_exclusive(). For reset controllers returned
- * from this function, reset_control_put() is called automatically on driver
- * detach.
- *
- * See reset_control_get_exclusive() for more information.
- */
-static inline struct reset_control *
-__must_check devm_reset_control_get_exclusive(struct device *dev,
-					      const char *id)
-{
-#ifndef CONFIG_RESET_CONTROLLER
-	WARN_ON(1);
-#endif
-	return __devm_reset_control_get(dev, id, 0, 0);
-}
-
-/**
- * devm_reset_control_get_shared - resource managed reset_control_get_shared()
- * @dev: device to be reset by the controller
- * @id: reset line name
- *
- * Managed reset_control_get_shared(). For reset controllers returned from
- * this function, reset_control_put() is called automatically on driver detach.
- * See reset_control_get_shared() for more information.
- */
-static inline struct reset_control *devm_reset_control_get_shared(
-					struct device *dev, const char *id)
-{
-#ifndef CONFIG_RESET_CONTROLLER
-	WARN_ON(1);
-#endif
-	return __devm_reset_control_get(dev, id, 0, 1);
-}
-
-static inline struct reset_control *devm_reset_control_get_optional_exclusive(
-					struct device *dev, const char *id)
-{
-	return __devm_reset_control_get(dev, id, 0, 0);
-}
-
-static inline struct reset_control *devm_reset_control_get_optional_shared(
-					struct device *dev, const char *id)
-{
-	return __devm_reset_control_get(dev, id, 0, 1);
-}
-
-/**
- * devm_reset_control_get_exclusive_by_index - resource managed
- *                                             reset_control_get_exclusive()
- * @dev: device to be reset by the controller
- * @index: index of the reset controller
- *
- * Managed reset_control_get_exclusive(). For reset controllers returned from
- * this function, reset_control_put() is called automatically on driver
- * detach.
- *
- * See reset_control_get_exclusive() for more information.
- */
-static inline struct reset_control *
-devm_reset_control_get_exclusive_by_index(struct device *dev, int index)
-{
-#ifndef CONFIG_RESET_CONTROLLER
-	WARN_ON(1);
-#endif
-	return __devm_reset_control_get(dev, NULL, index, 0);
-}
-
-/**
- * devm_reset_control_get_shared_by_index - resource managed
- * reset_control_get_shared
- * @dev: device to be reset by the controller
- * @index: index of the reset controller
- *
- * Managed reset_control_get_shared(). For reset controllers returned from
- * this function, reset_control_put() is called automatically on driver detach.
- * See reset_control_get_shared() for more information.
- */
-static inline struct reset_control *
-devm_reset_control_get_shared_by_index(struct device *dev, int index)
-{
-#ifndef CONFIG_RESET_CONTROLLER
-	WARN_ON(1);
-#endif
-	return __devm_reset_control_get(dev, NULL, index, 1);
-}
+#define GENERATE_RESET_CONTROL_GET_FUNCS(optional, shared, suffix)	\
+static inline struct reset_control * __must_check			\
+reset_control_get_ ## suffix(struct device *dev, const char *id)	\
+{									\
+	WARN_ON(!IS_ENABLED(CONFIG_RESET_CONTROLLER) && !optional);	\
+	return __of_reset_control_get(dev ? dev->of_node : NULL,	\
+				      id, 0, shared);			\
+}									\
+									\
+static inline struct reset_control * __must_check			\
+reset_control_get_ ## suffix ## _by_index(struct device *dev, int index)\
+{									\
+	WARN_ON(!IS_ENABLED(CONFIG_RESET_CONTROLLER) && !optional);	\
+	return __of_reset_control_get(dev ? dev->of_node : NULL,	\
+				      NULL, index, shared);		\
+}									\
+									\
+static inline struct reset_control * __must_check			\
+of_reset_control_get_ ## suffix(struct device_node *node, const char *id)\
+{									\
+	WARN_ON(!IS_ENABLED(CONFIG_RESET_CONTROLLER) && !optional);	\
+	return __of_reset_control_get(node, id, 0, shared);		\
+}									\
+									\
+static inline struct reset_control * __must_check			\
+of_reset_control_get_ ## suffix ## _by_index(struct device_node *node,	\
+					     int index)			\
+{									\
+	WARN_ON(!IS_ENABLED(CONFIG_RESET_CONTROLLER) && !optional);	\
+	return __of_reset_control_get(node, NULL, index, shared);	\
+}									\
+									\
+static inline struct reset_control * __must_check			\
+devm_reset_control_get_ ## suffix(struct device *dev, const char *id)	\
+{									\
+	WARN_ON(!IS_ENABLED(CONFIG_RESET_CONTROLLER) && !optional);	\
+	return __devm_reset_control_get(dev, id, 0, shared);		\
+}									\
+									\
+static inline struct reset_control * __must_check			\
+devm_reset_control_get_ ## suffix ## _by_index(struct device *dev, int index)\
+{									\
+	WARN_ON(!IS_ENABLED(CONFIG_RESET_CONTROLLER) && !optional);	\
+	return __devm_reset_control_get(dev, 0, index, shared);		\
+}
+
+GENERATE_RESET_CONTROL_GET_FUNCS(0, 0, exclusive)
+GENERATE_RESET_CONTROL_GET_FUNCS(0, 1, shared)
+GENERATE_RESET_CONTROL_GET_FUNCS(1, 0, optional_exclusive)
+GENERATE_RESET_CONTROL_GET_FUNCS(1, 1, optional_shared)
 
 /*
  * TEMPORARY calls to use during transition: