[U-Boot,PATCHv1,14/14] reset: remove request and free functions

Message ID 1523731900-4675-15-git-send-email-dinguyen@kernel.org
State New
Delegated to: Tom Rini
Headers show
Series
  • reset: remove request and free functions
Related show

Commit Message

Dinh Nguyen April 14, 2018, 6:51 p.m.
The request and free reset functions are not really used for any useful
purpose but for debugging. We can safely remove them.

Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
---
 drivers/reset/reset-uclass.c | 28 ----------------------------
 include/reset-uclass.h       | 21 ---------------------
 include/reset.h              | 23 -----------------------
 3 files changed, 72 deletions(-)

Comments

Simon Glass April 16, 2018, 6:43 p.m. | #1
+Stephen for comment

Hi Dinh,

On 14 April 2018 at 12:51, Dinh Nguyen <dinguyen@kernel.org> wrote:
> The request and free reset functions are not really used for any useful
> purpose but for debugging. We can safely remove them.

The API is set to line up with clocks. I think in general we do want
to be able to request and free these devices, just as we do for GPIOs.
What is the goal of removing these methods?

Regards,
Simon
Stephen Warren April 16, 2018, 6:51 p.m. | #2
On 04/16/2018 12:43 PM, Simon Glass wrote:
> +Stephen for comment
> 
> Hi Dinh,
> 
> On 14 April 2018 at 12:51, Dinh Nguyen <dinguyen@kernel.org> wrote:
>> The request and free reset functions are not really used for any useful
>> purpose but for debugging. We can safely remove them.
> 
> The API is set to line up with clocks. I think in general we do want
> to be able to request and free these devices, just as we do for GPIOs.
> What is the goal of removing these methods?

Many of the request methods do in fact do something; they check the 
validity of the reset ID so that check doesn't need to be duplicated 
everywhere. Even ignoring that, any resource management API should have 
explicit request/free APIs so that lifetime can be tracked if needed.
Dinh Nguyen April 16, 2018, 8:38 p.m. | #3
On 04/16/2018 01:43 PM, Simon Glass wrote:
> +Stephen for comment
> 
> Hi Dinh,
> 
> On 14 April 2018 at 12:51, Dinh Nguyen <dinguyen@kernel.org> wrote:
>> The request and free reset functions are not really used for any useful
>> purpose but for debugging. We can safely remove them.
> 
> The API is set to line up with clocks. I think in general we do want
> to be able to request and free these devices, just as we do for GPIOs.
> What is the goal of removing these methods?
> 

It was a suggestion from Marek. I just didn't see any useful code
implemented in any of the request/free functions.

Dinh
Dinh Nguyen April 16, 2018, 8:41 p.m. | #4
On 04/16/2018 01:51 PM, Stephen Warren wrote:
> On 04/16/2018 12:43 PM, Simon Glass wrote:
>> +Stephen for comment
>>
>> Hi Dinh,
>>
>> On 14 April 2018 at 12:51, Dinh Nguyen <dinguyen@kernel.org> wrote:
>>> The request and free reset functions are not really used for any useful
>>> purpose but for debugging. We can safely remove them.
>>
>> The API is set to line up with clocks. I think in general we do want
>> to be able to request and free these devices, just as we do for GPIOs.
>> What is the goal of removing these methods?
> 
> Many of the request methods do in fact do something; they check the
> validity of the reset ID so that check doesn't need to be duplicated
> everywhere. Even ignoring that, any resource management API should have
> explicit request/free APIs so that lifetime can be tracked if needed.

Agreed, that the checks were in some of the request functions, but the
majority did not do any checks, just a debug() statement. All of the
platforms that did the checks, I just moved them to reset_assert and
reset_deassert.

Dinh
Simon Glass April 17, 2018, 11:21 p.m. | #5
H Dinh,

On 16 April 2018 at 16:41, Dinh Nguyen <dinguyen@kernel.org> wrote:
>
>
> On 04/16/2018 01:51 PM, Stephen Warren wrote:
>> On 04/16/2018 12:43 PM, Simon Glass wrote:
>>> +Stephen for comment
>>>
>>> Hi Dinh,
>>>
>>> On 14 April 2018 at 12:51, Dinh Nguyen <dinguyen@kernel.org> wrote:
>>>> The request and free reset functions are not really used for any useful
>>>> purpose but for debugging. We can safely remove them.
>>>
>>> The API is set to line up with clocks. I think in general we do want
>>> to be able to request and free these devices, just as we do for GPIOs.
>>> What is the goal of removing these methods?
>>
>> Many of the request methods do in fact do something; they check the
>> validity of the reset ID so that check doesn't need to be duplicated
>> everywhere. Even ignoring that, any resource management API should have
>> explicit request/free APIs so that lifetime can be tracked if needed.
>
> Agreed, that the checks were in some of the request functions, but the
> majority did not do any checks, just a debug() statement. All of the
> platforms that did the checks, I just moved them to reset_assert and
> reset_deassert.

OK. Please can resend the series without removing those methods?

Regards,
Simon

Patch

diff --git a/drivers/reset/reset-uclass.c b/drivers/reset/reset-uclass.c
index 9a5c9c9..24dd48c 100644
--- a/drivers/reset/reset-uclass.c
+++ b/drivers/reset/reset-uclass.c
@@ -72,12 +72,6 @@  int reset_get_by_index(struct udevice *dev, int index,
 		return ret;
 	}
 
-	ret = ops->request(reset_ctl);
-	if (ret) {
-		debug("ops->request() failed: %d\n", ret);
-		return ret;
-	}
-
 	return 0;
 }
 
@@ -133,24 +127,6 @@  int reset_get_by_name(struct udevice *dev, const char *name,
 	return reset_get_by_index(dev, index, reset_ctl);
 }
 
-int reset_request(struct reset_ctl *reset_ctl)
-{
-	struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
-
-	debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
-
-	return ops->request(reset_ctl);
-}
-
-int reset_free(struct reset_ctl *reset_ctl)
-{
-	struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
-
-	debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
-
-	return ops->free(reset_ctl);
-}
-
 int reset_assert(struct reset_ctl *reset_ctl)
 {
 	struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
@@ -209,10 +185,6 @@  int reset_release_all(struct reset_ctl *reset_ctl, int count)
 		ret = reset_assert(&reset_ctl[i]);
 		if (ret)
 			return ret;
-
-		ret = reset_free(&reset_ctl[i]);
-		if (ret)
-			return ret;
 	}
 
 	return 0;
diff --git a/include/reset-uclass.h b/include/reset-uclass.h
index 50fbeb1..a6301cf 100644
--- a/include/reset-uclass.h
+++ b/include/reset-uclass.h
@@ -40,27 +40,6 @@  struct reset_ops {
 	int (*of_xlate)(struct reset_ctl *reset_ctl,
 			struct ofnode_phandle_args *args);
 	/**
-	 * request - Request a translated reset control.
-	 *
-	 * The reset core calls this function as the second step in
-	 * implementing a client's reset_get_by_*() call, following a
-	 * successful xxx_xlate() call.
-	 *
-	 * @reset_ctl:	The reset control struct to request; this has been
-	 *		filled in by a previoux xxx_xlate() function call.
-	 * @return 0 if OK, or a negative error code.
-	 */
-	int (*request)(struct reset_ctl *reset_ctl);
-	/**
-	 * free - Free a previously requested reset control.
-	 *
-	 * This is the implementation of the client reset_free() API.
-	 *
-	 * @reset_ctl:	The reset control to free.
-	 * @return 0 if OK, or a negative error code.
-	 */
-	int (*free)(struct reset_ctl *reset_ctl);
-	/**
 	 * rst_assert - Assert a reset signal.
 	 *
 	 * Note: This function is named rst_assert not assert to avoid
diff --git a/include/reset.h b/include/reset.h
index d38f176..505de77 100644
--- a/include/reset.h
+++ b/include/reset.h
@@ -134,24 +134,6 @@  int reset_get_by_name(struct udevice *dev, const char *name,
 		      struct reset_ctl *reset_ctl);
 
 /**
- * reset_request - Request a reset signal.
- *
- * @reset_ctl:	A reset control struct.
- *
- * @return 0 if OK, or a negative error code.
- */
-int reset_request(struct reset_ctl *reset_ctl);
-
-/**
- * reset_free - Free a previously requested reset signal.
- *
- * @reset_ctl:	A reset control struct that was previously successfully
- *		requested by reset_get_by_*().
- * @return 0 if OK, or a negative error code.
- */
-int reset_free(struct reset_ctl *reset_ctl);
-
-/**
  * reset_assert - Assert a reset signal.
  *
  * This function will assert the specified reset signal, thus resetting the
@@ -254,11 +236,6 @@  static inline int reset_get_by_name(struct udevice *dev, const char *name,
 	return -ENOTSUPP;
 }
 
-static inline int reset_free(struct reset_ctl *reset_ctl)
-{
-	return 0;
-}
-
 static inline int reset_assert(struct reset_ctl *reset_ctl)
 {
 	return 0;