[PATCH 0/7] clk: Make clk_free return void

[PATCH 0/7] clk: Make clk_free return void

[Sean Anderson]

clk_free cleans up resources allocated by clk_request et. al. It returns an
error code, but it really shouldn't. Much like regular free(), there is
typically no way to handle an error, and errors from clk_free shouldn't prevent
progress in the rest of the program. Make clk_free (and rfree) return void.


Sean Anderson (7):
  clk: Make rfree return void
  dma: bcm6348: Don't check clk_free
  net: bcm63xx: Don't check clk_free
  phy: bcm63xx: Don't check clk_free
  spi: bcm63xx: Don't check clk_free
  spi: dw: Don't check clk_free
  clk: Make clk_free return void

 drivers/clk/clk-uclass.c       | 15 ++++++---------
 drivers/clk/clk_sandbox.c      |  6 +++---
 drivers/clk/clk_sandbox_test.c |  9 +++------
 drivers/dma/bcm6348-iudma.c    |  6 +-----
 drivers/net/bcm6348-eth.c      |  6 +-----
 drivers/net/bcm6368-eth.c      |  6 +-----
 drivers/phy/bcm6318-usbh-phy.c |  4 +---
 drivers/phy/bcm6348-usbh-phy.c |  4 +---
 drivers/phy/bcm6368-usbh-phy.c |  8 ++------
 drivers/spi/bcm63xx_hsspi.c    |  8 ++------
 drivers/spi/bcm63xx_spi.c      |  4 +---
 drivers/spi/designware_spi.c   |  2 +-
 include/clk-uclass.h           |  8 +++-----
 include/clk.h                  |  8 ++++----
 14 files changed, 30 insertions(+), 64 deletions(-)

-- 
2.34.1

[PATCH 1/7] clk: Make rfree return void

[Sean Anderson]

When freeing a clock there is not much we can do if there is an error, and
most callers do not actually check the return value. Even e.g. checking to
make sure that clk->id is valid should have been done in request() in the
first place (unless someone is messing with the driver behind our back).
Just return void and don't bother returning an error.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

 drivers/clk/clk-uclass.c  | 7 +++----
 drivers/clk/clk_sandbox.c | 6 +++---
 include/clk-uclass.h      | 8 +++-----
 3 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
index fca4b8321a..61f977b661 100644
--- a/drivers/clk/clk-uclass.c
+++ b/drivers/clk/clk-uclass.c
@@ -481,10 +481,9 @@ int clk_free(struct clk *clk)
 		return 0;
 	ops = clk_dev_ops(clk->dev);
 
-	if (!ops->rfree)
-		return 0;
-
-	return ops->rfree(clk);
+	if (ops->rfree)
+		ops->rfree(clk);
+	return 0;
 }
 
 ulong clk_get_rate(struct clk *clk)
diff --git a/drivers/clk/clk_sandbox.c b/drivers/clk/clk_sandbox.c
index 57acf7d855..636914db8c 100644
--- a/drivers/clk/clk_sandbox.c
+++ b/drivers/clk/clk_sandbox.c
@@ -101,15 +101,15 @@ static int sandbox_clk_request(struct clk *clk)
 	return 0;
 }
 
-static int sandbox_clk_free(struct clk *clk)
+static void sandbox_clk_free(struct clk *clk)
 {
 	struct sandbox_clk_priv *priv = dev_get_priv(clk->dev);
 
 	if (clk->id >= SANDBOX_CLK_ID_COUNT)
-		return -EINVAL;
+		return;
 
 	priv->requested[clk->id] = false;
-	return 0;
+	return;
 }
 
 static struct clk_ops sandbox_clk_ops = {
diff --git a/include/clk-uclass.h b/include/clk-uclass.h
index e44f1caf51..65ebff9ed2 100644
--- a/include/clk-uclass.h
+++ b/include/clk-uclass.h
@@ -32,7 +32,7 @@ struct clk_ops {
 	int (*of_xlate)(struct clk *clock,
 			struct ofnode_phandle_args *args);
 	int (*request)(struct clk *clock);
-	int (*rfree)(struct clk *clock);
+	void (*rfree)(struct clk *clock);
 	ulong (*round_rate)(struct clk *clk, ulong rate);
 	ulong (*get_rate)(struct clk *clk);
 	ulong (*set_rate)(struct clk *clk, ulong rate);
@@ -81,11 +81,9 @@ int request(struct clk *clock);
  * rfree() - Free a previously requested clock.
  * @clock:	The clock to free.
  *
- * This is the implementation of the client clk_free() API.
- *
- * Return: 0 if OK, or a negative error code.
+ * Free any resources allocated in request().
  */
-int rfree(struct clk *clock);
+void rfree(struct clk *clock);
 
 /**
  * round_rate() - Adjust a rate to the exact rate a clock can provide.
-- 
2.34.1

[PATCH 2/7] dma: bcm6348: Don't check clk_free

[Sean Anderson]

This function always succeeds, so don't check its return value.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

 drivers/dma/bcm6348-iudma.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/dma/bcm6348-iudma.c b/drivers/dma/bcm6348-iudma.c
index c04aa55cb4..4fc650272d 100644
--- a/drivers/dma/bcm6348-iudma.c
+++ b/drivers/dma/bcm6348-iudma.c
@@ -596,11 +596,7 @@ static int bcm6348_iudma_probe(struct udevice *dev)
 			return ret;
 		}
 
-		ret = clk_free(&clk);
-		if (ret < 0) {
-			pr_err("error freeing clock %d\n", i);
-			return ret;
-		}
+		clk_free(&clk);
 	}
 
 	/* try to perform resets */
-- 
2.34.1

[PATCH 3/7] net: bcm63xx: Don't check clk_free

[Sean Anderson]

This function always succeeds, so don't check its return value.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

 drivers/net/bcm6348-eth.c | 6 +-----
 drivers/net/bcm6368-eth.c | 6 +-----
 2 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/net/bcm6348-eth.c b/drivers/net/bcm6348-eth.c
index aad7b61213..06e0dd74a5 100644
--- a/drivers/net/bcm6348-eth.c
+++ b/drivers/net/bcm6348-eth.c
@@ -461,11 +461,7 @@ static int bcm6348_eth_probe(struct udevice *dev)
 			return ret;
 		}
 
-		ret = clk_free(&clk);
-		if (ret < 0) {
-			pr_err("%s: error freeing clock %d\n", __func__, i);
-			return ret;
-		}
+		clk_free(&clk);
 	}
 
 	/* try to perform resets */
diff --git a/drivers/net/bcm6368-eth.c b/drivers/net/bcm6368-eth.c
index 29abe7fc96..c2a8b9f057 100644
--- a/drivers/net/bcm6368-eth.c
+++ b/drivers/net/bcm6368-eth.c
@@ -546,11 +546,7 @@ static int bcm6368_eth_probe(struct udevice *dev)
 			return ret;
 		}
 
-		ret = clk_free(&clk);
-		if (ret < 0) {
-			pr_err("%s: error freeing clock %d\n", __func__, i);
-			return ret;
-		}
+		clk_free(&clk);
 	}
 
 	/* try to perform resets */
-- 
2.34.1

[PATCH 4/7] phy: bcm63xx: Don't check clk_free

[Sean Anderson]

This function always succeeds, so don't check its return value.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

 drivers/phy/bcm6318-usbh-phy.c | 4 +---
 drivers/phy/bcm6348-usbh-phy.c | 4 +---
 drivers/phy/bcm6368-usbh-phy.c | 8 ++------
 3 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/phy/bcm6318-usbh-phy.c b/drivers/phy/bcm6318-usbh-phy.c
index 60608a55bc..1c10853940 100644
--- a/drivers/phy/bcm6318-usbh-phy.c
+++ b/drivers/phy/bcm6318-usbh-phy.c
@@ -98,9 +98,7 @@ static int bcm6318_usbh_probe(struct udevice *dev)
 	if (ret < 0)
 		return ret;
 
-	ret = clk_free(&clk);
-	if (ret < 0)
-		return ret;
+	clk_free(&clk);
 
 	/* enable power domain */
 	ret = power_domain_get(dev, &pwr_dom);
diff --git a/drivers/phy/bcm6348-usbh-phy.c b/drivers/phy/bcm6348-usbh-phy.c
index 1b6b5ad177..ce6be3d7da 100644
--- a/drivers/phy/bcm6348-usbh-phy.c
+++ b/drivers/phy/bcm6348-usbh-phy.c
@@ -62,9 +62,7 @@ static int bcm6348_usbh_probe(struct udevice *dev)
 	if (ret < 0)
 		return ret;
 
-	ret = clk_free(&clk);
-	if (ret < 0)
-		return ret;
+	clk_free(&clk);
 
 	/* perform reset */
 	ret = reset_get_by_index(dev, 0, &rst_ctl);
diff --git a/drivers/phy/bcm6368-usbh-phy.c b/drivers/phy/bcm6368-usbh-phy.c
index 4d3a63faad..d057f1f52e 100644
--- a/drivers/phy/bcm6368-usbh-phy.c
+++ b/drivers/phy/bcm6368-usbh-phy.c
@@ -137,9 +137,7 @@ static int bcm6368_usbh_probe(struct udevice *dev)
 	if (ret < 0)
 		return ret;
 
-	ret = clk_free(&clk);
-	if (ret < 0)
-		return ret;
+	clk_free(&clk);
 
 #if defined(CONFIG_POWER_DOMAIN)
 	/* enable power domain */
@@ -176,9 +174,7 @@ static int bcm6368_usbh_probe(struct udevice *dev)
 		if (ret < 0)
 			return ret;
 
-		ret = clk_free(&clk);
-		if (ret < 0)
-			return ret;
+		clk_free(&clk);
 	}
 
 	mdelay(100);
-- 
2.34.1

[PATCH 5/7] spi: bcm63xx: Don't check clk_free

[Sean Anderson]

This function always succeeds, so don't check its return value.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

 drivers/spi/bcm63xx_hsspi.c | 8 ++------
 drivers/spi/bcm63xx_spi.c   | 4 +---
 2 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/spi/bcm63xx_hsspi.c b/drivers/spi/bcm63xx_hsspi.c
index 85108df565..47002f8b56 100644
--- a/drivers/spi/bcm63xx_hsspi.c
+++ b/drivers/spi/bcm63xx_hsspi.c
@@ -355,9 +355,7 @@ static int bcm63xx_hsspi_probe(struct udevice *dev)
 	if (ret < 0 && ret != -ENOSYS)
 		return ret;
 
-	ret = clk_free(&clk);
-	if (ret < 0 && ret != -ENOSYS)
-		return ret;
+	clk_free(&clk);
 
 	/* get clock rate */
 	ret = clk_get_by_name(dev, "pll", &clk);
@@ -366,9 +364,7 @@ static int bcm63xx_hsspi_probe(struct udevice *dev)
 
 	priv->clk_rate = clk_get_rate(&clk);
 
-	ret = clk_free(&clk);
-	if (ret < 0 && ret != -ENOSYS)
-		return ret;
+	clk_free(&clk);
 
 	/* perform reset */
 	ret = reset_get_by_index(dev, 0, &rst_ctl);
diff --git a/drivers/spi/bcm63xx_spi.c b/drivers/spi/bcm63xx_spi.c
index dd5e62b2fe..0600d56c69 100644
--- a/drivers/spi/bcm63xx_spi.c
+++ b/drivers/spi/bcm63xx_spi.c
@@ -391,9 +391,7 @@ static int bcm63xx_spi_probe(struct udevice *dev)
 	if (ret < 0)
 		return ret;
 
-	ret = clk_free(&clk);
-	if (ret < 0)
-		return ret;
+	clk_free(&clk);
 
 	/* perform reset */
 	ret = reset_get_by_index(dev, 0, &rst_ctl);
-- 
2.34.1

[PATCH 6/7] spi: dw: Don't check clk_free

[Sean Anderson]

This function always succeeds, so don't check its return value.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

 drivers/spi/designware_spi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
index 742121140d..9b9933b439 100644
--- a/drivers/spi/designware_spi.c
+++ b/drivers/spi/designware_spi.c
@@ -718,7 +718,7 @@ static int dw_spi_remove(struct udevice *bus)
 	if (ret)
 		return ret;
 
-	ret = clk_free(&priv->clk);
+	clk_free(&priv->clk);
 	if (ret)
 		return ret;
 #endif
-- 
2.34.1

[PATCH 7/7] clk: Make clk_free return void

[Sean Anderson]

Most callers of this function do not check the return value, and it is
unclear what action they should take if it fails. If a function is freeing
multiple clocks, it should not stop just because the first one failed.
Since the callbacks can no longer fail, just convert the return type to
void.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

 drivers/clk/clk-uclass.c       | 10 ++++------
 drivers/clk/clk_sandbox_test.c |  9 +++------
 include/clk.h                  |  8 ++++----
 3 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
index 61f977b661..5641709244 100644
--- a/drivers/clk/clk-uclass.c
+++ b/drivers/clk/clk-uclass.c
@@ -447,9 +447,7 @@ int clk_release_all(struct clk *clk, int count)
 		if (ret && ret != -ENOSYS)
 			return ret;
 
-		ret = clk_free(&clk[i]);
-		if (ret && ret != -ENOSYS)
-			return ret;
+		clk_free(&clk[i]);
 	}
 
 	return 0;
@@ -472,18 +470,18 @@ int clk_request(struct udevice *dev, struct clk *clk)
 	return ops->request(clk);
 }
 
-int clk_free(struct clk *clk)
+void clk_free(struct clk *clk)
 {
 	const struct clk_ops *ops;
 
 	debug("%s(clk=%p)\n", __func__, clk);
 	if (!clk_valid(clk))
-		return 0;
+		return;
 	ops = clk_dev_ops(clk->dev);
 
 	if (ops->rfree)
 		ops->rfree(clk);
-	return 0;
+	return;
 }
 
 ulong clk_get_rate(struct clk *clk)
diff --git a/drivers/clk/clk_sandbox_test.c b/drivers/clk/clk_sandbox_test.c
index f665fd3cc4..5807a454f3 100644
--- a/drivers/clk/clk_sandbox_test.c
+++ b/drivers/clk/clk_sandbox_test.c
@@ -137,14 +137,11 @@ int sandbox_clk_test_disable_bulk(struct udevice *dev)
 int sandbox_clk_test_free(struct udevice *dev)
 {
 	struct sandbox_clk_test *sbct = dev_get_priv(dev);
-	int i, ret;
+	int i;
 
 	devm_clk_put(dev, sbct->clkps[SANDBOX_CLK_TEST_ID_DEVM1]);
-	for (i = 0; i < SANDBOX_CLK_TEST_NON_DEVM_COUNT; i++) {
-		ret = clk_free(&sbct->clks[i]);
-		if (ret)
-			return ret;
-	}
+	for (i = 0; i < SANDBOX_CLK_TEST_NON_DEVM_COUNT; i++)
+		clk_free(&sbct->clks[i]);
 
 	return 0;
 }
diff --git a/include/clk.h b/include/clk.h
index 33f448eb89..79e7a9551f 100644
--- a/include/clk.h
+++ b/include/clk.h
@@ -396,9 +396,9 @@ int clk_request(struct udevice *dev, struct clk *clk);
  * @clk:	A clock struct that was previously successfully requested by
  *		clk_request/get_by_*().
  *
- * Return: 0 if OK, or a negative error code.
+ * Free resources allocated by clk_request() (or any clk_get_* function).
  */
-int clk_free(struct clk *clk);
+void clk_free(struct clk *clk);
 
 /**
  * clk_get_rate() - Get current clock rate.
@@ -545,9 +545,9 @@ static inline int clk_request(struct udevice *dev, struct clk *clk)
 	return -ENOSYS;
 }
 
-static inline int clk_free(struct clk *clk)
+static inline void clk_free(struct clk *clk)
 {
-	return 0;
+	return;
 }
 
 static inline ulong clk_get_rate(struct clk *clk)
-- 
2.34.1

Re: [PATCH 1/7] clk: Make rfree return void

[Simon Glass]

Hi Sean,

On Sat, 15 Jan 2022 at 15:25, Sean Anderson <seanga2@gmail.com> wrote:
>
> When freeing a clock there is not much we can do if there is an error, and
> most callers do not actually check the return value. Even e.g. checking to
> make sure that clk->id is valid should have been done in request() in the
> first place (unless someone is messing with the driver behind our back).
> Just return void and don't bother returning an error.
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
>
>  drivers/clk/clk-uclass.c  | 7 +++----
>  drivers/clk/clk_sandbox.c | 6 +++---
>  include/clk-uclass.h      | 8 +++-----
>  3 files changed, 9 insertions(+), 12 deletions(-)
>

We have the same thing in other places too, but I am a little worried
about removing error checking. We try to avoid checking arguments too
much in U-Boot, due to code-size concerns, so I suppose I agree that
an invalid clk should be caught by a debug assertion rather than a
full check. But with driver model we have generally added an error
return to every uclass method, for consistency and to permit returning
error information if needed.

Regards,
Simon

Re: [PATCH 1/7] clk: Make rfree return void

[Sean Anderson]

On 1/27/22 10:05 AM, Simon Glass wrote:
> Hi Sean,
> 
> On Sat, 15 Jan 2022 at 15:25, Sean Anderson <seanga2@gmail.com> wrote:
>>
>> When freeing a clock there is not much we can do if there is an error, and
>> most callers do not actually check the return value. Even e.g. checking to
>> make sure that clk->id is valid should have been done in request() in the
>> first place (unless someone is messing with the driver behind our back).
>> Just return void and don't bother returning an error.
>>
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>> ---
>>
>>   drivers/clk/clk-uclass.c  | 7 +++----
>>   drivers/clk/clk_sandbox.c | 6 +++---
>>   include/clk-uclass.h      | 8 +++-----
>>   3 files changed, 9 insertions(+), 12 deletions(-)
>>
> 
> We have the same thing in other places too, but I am a little worried
> about removing error checking. We try to avoid checking arguments too
> much in U-Boot, due to code-size concerns, so I suppose I agree that
> an invalid clk should be caught by a debug assertion rather than a
> full check. But with driver model we have generally added an error
> return to every uclass method, for consistency and to permit returning
> error information if needed.
> 
> Regards,
> Simon
> 

So there are a few reasons why I don't think a return value is useful
here. To illustrate this, consider a typical user of the clock API:

	struct clk a, b;

	ret = clk_get_by_name(dev, "a", &a);
	if (ret)
		return ret;

	ret = clk_get_by_name(dev, "b", &b);
	if (ret)
		goto free_a;

	ret = clk_set_rate(&a, 5000000);
	if (ret)
		goto free_b;

	ret = clk_enable(&b);

free_b:
	clk_free(&b);
free_a:
	clk_free(&a);
	return ret;

- Because a and b are "thick pointers" they do not need any cleanup to
   free their own resources. The only cleanup might be if the clock
   driver has allocated something in clk_request (more on this below)
- By the time we call clk_free, the mutable portions of the function
   have already completed. In effect, the function has succeeded,
   regardless of whether clk_free fails. Additionally, we cannot take any
   action if it fails, since we still have to free both clocks.
- clk_free occurs during the error path of the function. Even if it
   errored, we do not want to override the existing error from one of the
   functions doing "real" work.

The last thing is that no clock driver actually does anything in rfree.
The only driver with this function is the sandbox driver. I would like
to remove the function altogether. As I understand it, the existing API
is inspired by the reset drivers, so I would like to review its usage in
the reset subsystem before removing it for the clock subsystem. I also
want to make some changes to how rates and enables/disables are
calculated which might provide a case for rfree. But once that is
complete I think there will be no users still.

--Sean

Re: [PATCH 1/7] clk: Make rfree return void

[Simon Glass]

Hi Sean,

On Thu, 27 Jan 2022 at 08:43, Sean Anderson <seanga2@gmail.com> wrote:
>
> On 1/27/22 10:05 AM, Simon Glass wrote:
> > Hi Sean,
> >
> > On Sat, 15 Jan 2022 at 15:25, Sean Anderson <seanga2@gmail.com> wrote:
> >>
> >> When freeing a clock there is not much we can do if there is an error, and
> >> most callers do not actually check the return value. Even e.g. checking to
> >> make sure that clk->id is valid should have been done in request() in the
> >> first place (unless someone is messing with the driver behind our back).
> >> Just return void and don't bother returning an error.
> >>
> >> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> >> ---
> >>
> >>   drivers/clk/clk-uclass.c  | 7 +++----
> >>   drivers/clk/clk_sandbox.c | 6 +++---
> >>   include/clk-uclass.h      | 8 +++-----
> >>   3 files changed, 9 insertions(+), 12 deletions(-)
> >>
> >
> > We have the same thing in other places too, but I am a little worried
> > about removing error checking. We try to avoid checking arguments too
> > much in U-Boot, due to code-size concerns, so I suppose I agree that
> > an invalid clk should be caught by a debug assertion rather than a
> > full check. But with driver model we have generally added an error
> > return to every uclass method, for consistency and to permit returning
> > error information if needed.
> >
> > Regards,
> > Simon
> >
>
> So there are a few reasons why I don't think a return value is useful
> here. To illustrate this, consider a typical user of the clock API:
>
>         struct clk a, b;
>
>         ret = clk_get_by_name(dev, "a", &a);
>         if (ret)
>                 return ret;
>
>         ret = clk_get_by_name(dev, "b", &b);
>         if (ret)
>                 goto free_a;
>
>         ret = clk_set_rate(&a, 5000000);
>         if (ret)
>                 goto free_b;
>
>         ret = clk_enable(&b);
>
> free_b:
>         clk_free(&b);
> free_a:
>         clk_free(&a);
>         return ret;
>
> - Because a and b are "thick pointers" they do not need any cleanup to
>    free their own resources. The only cleanup might be if the clock
>    driver has allocated something in clk_request (more on this below)
> - By the time we call clk_free, the mutable portions of the function
>    have already completed. In effect, the function has succeeded,
>    regardless of whether clk_free fails. Additionally, we cannot take any
>    action if it fails, since we still have to free both clocks.
> - clk_free occurs during the error path of the function. Even if it
>    errored, we do not want to override the existing error from one of the
>    functions doing "real" work.
>
> The last thing is that no clock driver actually does anything in rfree.
> The only driver with this function is the sandbox driver. I would like
> to remove the function altogether. As I understand it, the existing API
> is inspired by the reset drivers, so I would like to review its usage in
> the reset subsystem before removing it for the clock subsystem. I also
> want to make some changes to how rates and enables/disables are
> calculated which might provide a case for rfree. But once that is
> complete I think there will be no users still.

What does this all look like in Linux?

Regards,
Simon

Re: [PATCH 1/7] clk: Make rfree return void

[Sean Anderson]

On 1/27/22 4:35 PM, Simon Glass wrote:
> Hi Sean,
> 
> On Thu, 27 Jan 2022 at 08:43, Sean Anderson <seanga2@gmail.com> wrote:
>>
>> On 1/27/22 10:05 AM, Simon Glass wrote:
>>> Hi Sean,
>>>
>>> On Sat, 15 Jan 2022 at 15:25, Sean Anderson <seanga2@gmail.com> wrote:
>>>>
>>>> When freeing a clock there is not much we can do if there is an error, and
>>>> most callers do not actually check the return value. Even e.g. checking to
>>>> make sure that clk->id is valid should have been done in request() in the
>>>> first place (unless someone is messing with the driver behind our back).
>>>> Just return void and don't bother returning an error.
>>>>
>>>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>>>> ---
>>>>
>>>>    drivers/clk/clk-uclass.c  | 7 +++----
>>>>    drivers/clk/clk_sandbox.c | 6 +++---
>>>>    include/clk-uclass.h      | 8 +++-----
>>>>    3 files changed, 9 insertions(+), 12 deletions(-)
>>>>
>>>
>>> We have the same thing in other places too, but I am a little worried
>>> about removing error checking. We try to avoid checking arguments too
>>> much in U-Boot, due to code-size concerns, so I suppose I agree that
>>> an invalid clk should be caught by a debug assertion rather than a
>>> full check. But with driver model we have generally added an error
>>> return to every uclass method, for consistency and to permit returning
>>> error information if needed.
>>>
>>> Regards,
>>> Simon
>>>
>>
>> So there are a few reasons why I don't think a return value is useful
>> here. To illustrate this, consider a typical user of the clock API:
>>
>>          struct clk a, b;
>>
>>          ret = clk_get_by_name(dev, "a", &a);
>>          if (ret)
>>                  return ret;
>>
>>          ret = clk_get_by_name(dev, "b", &b);
>>          if (ret)
>>                  goto free_a;
>>
>>          ret = clk_set_rate(&a, 5000000);
>>          if (ret)
>>                  goto free_b;
>>
>>          ret = clk_enable(&b);
>>
>> free_b:
>>          clk_free(&b);
>> free_a:
>>          clk_free(&a);
>>          return ret;
>>
>> - Because a and b are "thick pointers" they do not need any cleanup to
>>     free their own resources. The only cleanup might be if the clock
>>     driver has allocated something in clk_request (more on this below)
>> - By the time we call clk_free, the mutable portions of the function
>>     have already completed. In effect, the function has succeeded,
>>     regardless of whether clk_free fails. Additionally, we cannot take any
>>     action if it fails, since we still have to free both clocks.
>> - clk_free occurs during the error path of the function. Even if it
>>     errored, we do not want to override the existing error from one of the
>>     functions doing "real" work.
>>
>> The last thing is that no clock driver actually does anything in rfree.
>> The only driver with this function is the sandbox driver. I would like
>> to remove the function altogether. As I understand it, the existing API
>> is inspired by the reset drivers, so I would like to review its usage in
>> the reset subsystem before removing it for the clock subsystem. I also
>> want to make some changes to how rates and enables/disables are
>> calculated which might provide a case for rfree. But once that is
>> complete I think there will be no users still.
> 
> What does this all look like in Linux?

Their equivalent (clk_put) returns void, and generally so do most other
cleanup functions, since .device_remove also returns void.

--Sean

Re: [PATCH 1/7] clk: Make rfree return void

[Simon Glass]

Hi Sean,

On Tue, 1 Feb 2022 at 07:49, Sean Anderson <seanga2@gmail.com> wrote:
>
> On 1/27/22 4:35 PM, Simon Glass wrote:
> > Hi Sean,
> >
> > On Thu, 27 Jan 2022 at 08:43, Sean Anderson <seanga2@gmail.com> wrote:
> >>
> >> On 1/27/22 10:05 AM, Simon Glass wrote:
> >>> Hi Sean,
> >>>
> >>> On Sat, 15 Jan 2022 at 15:25, Sean Anderson <seanga2@gmail.com> wrote:
> >>>>
> >>>> When freeing a clock there is not much we can do if there is an error, and
> >>>> most callers do not actually check the return value. Even e.g. checking to
> >>>> make sure that clk->id is valid should have been done in request() in the
> >>>> first place (unless someone is messing with the driver behind our back).
> >>>> Just return void and don't bother returning an error.
> >>>>
> >>>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> >>>> ---
> >>>>
> >>>>    drivers/clk/clk-uclass.c  | 7 +++----
> >>>>    drivers/clk/clk_sandbox.c | 6 +++---
> >>>>    include/clk-uclass.h      | 8 +++-----
> >>>>    3 files changed, 9 insertions(+), 12 deletions(-)
> >>>>
> >>>
> >>> We have the same thing in other places too, but I am a little worried
> >>> about removing error checking. We try to avoid checking arguments too
> >>> much in U-Boot, due to code-size concerns, so I suppose I agree that
> >>> an invalid clk should be caught by a debug assertion rather than a
> >>> full check. But with driver model we have generally added an error
> >>> return to every uclass method, for consistency and to permit returning
> >>> error information if needed.
> >>>
> >>> Regards,
> >>> Simon
> >>>
> >>
> >> So there are a few reasons why I don't think a return value is useful
> >> here. To illustrate this, consider a typical user of the clock API:
> >>
> >>          struct clk a, b;
> >>
> >>          ret = clk_get_by_name(dev, "a", &a);
> >>          if (ret)
> >>                  return ret;
> >>
> >>          ret = clk_get_by_name(dev, "b", &b);
> >>          if (ret)
> >>                  goto free_a;
> >>
> >>          ret = clk_set_rate(&a, 5000000);
> >>          if (ret)
> >>                  goto free_b;
> >>
> >>          ret = clk_enable(&b);
> >>
> >> free_b:
> >>          clk_free(&b);
> >> free_a:
> >>          clk_free(&a);
> >>          return ret;
> >>
> >> - Because a and b are "thick pointers" they do not need any cleanup to
> >>     free their own resources. The only cleanup might be if the clock
> >>     driver has allocated something in clk_request (more on this below)
> >> - By the time we call clk_free, the mutable portions of the function
> >>     have already completed. In effect, the function has succeeded,
> >>     regardless of whether clk_free fails. Additionally, we cannot take any
> >>     action if it fails, since we still have to free both clocks.
> >> - clk_free occurs during the error path of the function. Even if it
> >>     errored, we do not want to override the existing error from one of the
> >>     functions doing "real" work.
> >>
> >> The last thing is that no clock driver actually does anything in rfree.
> >> The only driver with this function is the sandbox driver. I would like
> >> to remove the function altogether. As I understand it, the existing API
> >> is inspired by the reset drivers, so I would like to review its usage in
> >> the reset subsystem before removing it for the clock subsystem. I also
> >> want to make some changes to how rates and enables/disables are
> >> calculated which might provide a case for rfree. But once that is
> >> complete I think there will be no users still.
> >
> > What does this all look like in Linux?
>
> Their equivalent (clk_put) returns void, and generally so do most other
> cleanup functions, since .device_remove also returns void.

We really cannot ignore errors from device_remove().

Anyway I think what you say about the 'thick pointer' makes sense. But
my expectation was that removing a clock might turn off a clock above
it in the tree, for example.

Regards,
Simon

Re: [PATCH 1/7] clk: Make rfree return void

[Sean Anderson]

On 2/1/22 10:59 PM, Simon Glass wrote:
> Hi Sean,
> 
> On Tue, 1 Feb 2022 at 07:49, Sean Anderson <seanga2@gmail.com> wrote:
>>
>> On 1/27/22 4:35 PM, Simon Glass wrote:
>>> Hi Sean,
>>>
>>> On Thu, 27 Jan 2022 at 08:43, Sean Anderson <seanga2@gmail.com> wrote:
>>>>
>>>> On 1/27/22 10:05 AM, Simon Glass wrote:
>>>>> Hi Sean,
>>>>>
>>>>> On Sat, 15 Jan 2022 at 15:25, Sean Anderson <seanga2@gmail.com> wrote:
>>>>>>
>>>>>> When freeing a clock there is not much we can do if there is an error, and
>>>>>> most callers do not actually check the return value. Even e.g. checking to
>>>>>> make sure that clk->id is valid should have been done in request() in the
>>>>>> first place (unless someone is messing with the driver behind our back).
>>>>>> Just return void and don't bother returning an error.
>>>>>>
>>>>>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>>>>>> ---
>>>>>>
>>>>>>     drivers/clk/clk-uclass.c  | 7 +++----
>>>>>>     drivers/clk/clk_sandbox.c | 6 +++---
>>>>>>     include/clk-uclass.h      | 8 +++-----
>>>>>>     3 files changed, 9 insertions(+), 12 deletions(-)
>>>>>>
>>>>>
>>>>> We have the same thing in other places too, but I am a little worried
>>>>> about removing error checking. We try to avoid checking arguments too
>>>>> much in U-Boot, due to code-size concerns, so I suppose I agree that
>>>>> an invalid clk should be caught by a debug assertion rather than a
>>>>> full check. But with driver model we have generally added an error
>>>>> return to every uclass method, for consistency and to permit returning
>>>>> error information if needed.
>>>>>
>>>>> Regards,
>>>>> Simon
>>>>>
>>>>
>>>> So there are a few reasons why I don't think a return value is useful
>>>> here. To illustrate this, consider a typical user of the clock API:
>>>>
>>>>           struct clk a, b;
>>>>
>>>>           ret = clk_get_by_name(dev, "a", &a);
>>>>           if (ret)
>>>>                   return ret;
>>>>
>>>>           ret = clk_get_by_name(dev, "b", &b);
>>>>           if (ret)
>>>>                   goto free_a;
>>>>
>>>>           ret = clk_set_rate(&a, 5000000);
>>>>           if (ret)
>>>>                   goto free_b;
>>>>
>>>>           ret = clk_enable(&b);
>>>>
>>>> free_b:
>>>>           clk_free(&b);
>>>> free_a:
>>>>           clk_free(&a);
>>>>           return ret;
>>>>
>>>> - Because a and b are "thick pointers" they do not need any cleanup to
>>>>      free their own resources. The only cleanup might be if the clock
>>>>      driver has allocated something in clk_request (more on this below)
>>>> - By the time we call clk_free, the mutable portions of the function
>>>>      have already completed. In effect, the function has succeeded,
>>>>      regardless of whether clk_free fails. Additionally, we cannot take any
>>>>      action if it fails, since we still have to free both clocks.
>>>> - clk_free occurs during the error path of the function. Even if it
>>>>      errored, we do not want to override the existing error from one of the
>>>>      functions doing "real" work.
>>>>
>>>> The last thing is that no clock driver actually does anything in rfree.
>>>> The only driver with this function is the sandbox driver. I would like
>>>> to remove the function altogether. As I understand it, the existing API
>>>> is inspired by the reset drivers, so I would like to review its usage in
>>>> the reset subsystem before removing it for the clock subsystem. I also
>>>> want to make some changes to how rates and enables/disables are
>>>> calculated which might provide a case for rfree. But once that is
>>>> complete I think there will be no users still.
>>>
>>> What does this all look like in Linux?
>>
>> Their equivalent (clk_put) returns void, and generally so do most other
>> cleanup functions, since .device_remove also returns void.
> 
> We really cannot ignore errors from device_remove().

Once you are at device_remove, all the users are gone and it's up to the
device to clean up after itself. And often there is nothing we can do
once remove is called. As you yourself say in device_remove,

	/* We can't put the children back */

Really the only sensible thing is to print an error and continue booting
if possible.

And of course no clock drivers actually use this function anyway, nor do
(all but 5) users check it.

> Anyway I think what you say about the 'thick pointer' makes sense. But
> my expectation was that removing a clock might turn off a clock above
> it in the tree, for example.

No, this just frees resources (as is documented). If you want to turn
off a clock, you have to call clk_disable. In fact, a very common use
case is just like the example above, where the consmer frees the clock
after enabling it.

(This is also why clk->enable_count/rate are basically useless for
anything other than CCF clocks)

--Sean

Re: [PATCH 1/7] clk: Make rfree return void

[Simon Glass]

Hi Sean,

On Tue, 1 Feb 2022 at 21:24, Sean Anderson <seanga2@gmail.com> wrote:
>
> On 2/1/22 10:59 PM, Simon Glass wrote:
> > Hi Sean,
> >
> > On Tue, 1 Feb 2022 at 07:49, Sean Anderson <seanga2@gmail.com> wrote:
> >>
> >> On 1/27/22 4:35 PM, Simon Glass wrote:
> >>> Hi Sean,
> >>>
> >>> On Thu, 27 Jan 2022 at 08:43, Sean Anderson <seanga2@gmail.com> wrote:
> >>>>
> >>>> On 1/27/22 10:05 AM, Simon Glass wrote:
> >>>>> Hi Sean,
> >>>>>
> >>>>> On Sat, 15 Jan 2022 at 15:25, Sean Anderson <seanga2@gmail.com> wrote:
> >>>>>>
> >>>>>> When freeing a clock there is not much we can do if there is an error, and
> >>>>>> most callers do not actually check the return value. Even e.g. checking to
> >>>>>> make sure that clk->id is valid should have been done in request() in the
> >>>>>> first place (unless someone is messing with the driver behind our back).
> >>>>>> Just return void and don't bother returning an error.
> >>>>>>
> >>>>>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> >>>>>> ---
> >>>>>>
> >>>>>>     drivers/clk/clk-uclass.c  | 7 +++----
> >>>>>>     drivers/clk/clk_sandbox.c | 6 +++---
> >>>>>>     include/clk-uclass.h      | 8 +++-----
> >>>>>>     3 files changed, 9 insertions(+), 12 deletions(-)
> >>>>>>
> >>>>>
> >>>>> We have the same thing in other places too, but I am a little worried
> >>>>> about removing error checking. We try to avoid checking arguments too
> >>>>> much in U-Boot, due to code-size concerns, so I suppose I agree that
> >>>>> an invalid clk should be caught by a debug assertion rather than a
> >>>>> full check. But with driver model we have generally added an error
> >>>>> return to every uclass method, for consistency and to permit returning
> >>>>> error information if needed.
> >>>>>
> >>>>> Regards,
> >>>>> Simon
> >>>>>
> >>>>
> >>>> So there are a few reasons why I don't think a return value is useful
> >>>> here. To illustrate this, consider a typical user of the clock API:
> >>>>
> >>>>           struct clk a, b;
> >>>>
> >>>>           ret = clk_get_by_name(dev, "a", &a);
> >>>>           if (ret)
> >>>>                   return ret;
> >>>>
> >>>>           ret = clk_get_by_name(dev, "b", &b);
> >>>>           if (ret)
> >>>>                   goto free_a;
> >>>>
> >>>>           ret = clk_set_rate(&a, 5000000);
> >>>>           if (ret)
> >>>>                   goto free_b;
> >>>>
> >>>>           ret = clk_enable(&b);
> >>>>
> >>>> free_b:
> >>>>           clk_free(&b);
> >>>> free_a:
> >>>>           clk_free(&a);
> >>>>           return ret;
> >>>>
> >>>> - Because a and b are "thick pointers" they do not need any cleanup to
> >>>>      free their own resources. The only cleanup might be if the clock
> >>>>      driver has allocated something in clk_request (more on this below)
> >>>> - By the time we call clk_free, the mutable portions of the function
> >>>>      have already completed. In effect, the function has succeeded,
> >>>>      regardless of whether clk_free fails. Additionally, we cannot take any
> >>>>      action if it fails, since we still have to free both clocks.
> >>>> - clk_free occurs during the error path of the function. Even if it
> >>>>      errored, we do not want to override the existing error from one of the
> >>>>      functions doing "real" work.
> >>>>
> >>>> The last thing is that no clock driver actually does anything in rfree.
> >>>> The only driver with this function is the sandbox driver. I would like
> >>>> to remove the function altogether. As I understand it, the existing API
> >>>> is inspired by the reset drivers, so I would like to review its usage in
> >>>> the reset subsystem before removing it for the clock subsystem. I also
> >>>> want to make some changes to how rates and enables/disables are
> >>>> calculated which might provide a case for rfree. But once that is
> >>>> complete I think there will be no users still.
> >>>
> >>> What does this all look like in Linux?
> >>
> >> Their equivalent (clk_put) returns void, and generally so do most other
> >> cleanup functions, since .device_remove also returns void.
> >
> > We really cannot ignore errors from device_remove().
>
> Once you are at device_remove, all the users are gone and it's up to the
> device to clean up after itself. And often there is nothing we can do
> once remove is called. As you yourself say in device_remove,
>
>         /* We can't put the children back */

Well this assumes that device_remove() is actually removing the
device, not just disabling DMA, etc.

>
> Really the only sensible thing is to print an error and continue booting
> if possible.
>
> And of course no clock drivers actually use this function anyway, nor do
> (all but 5) users check it.
>
> > Anyway I think what you say about the 'thick pointer' makes sense. But
> > my expectation was that removing a clock might turn off a clock above
> > it in the tree, for example.
>
> No, this just frees resources (as is documented). If you want to turn
> off a clock, you have to call clk_disable. In fact, a very common use
> case is just like the example above, where the consmer frees the clock
> after enabling it.
>
> (This is also why clk->enable_count/rate are basically useless for
> anything other than CCF clocks)

How about a clock provided by an audio codec on an I2C bus? Should
clk_free() do anything in that case? I assume not. I think the
compelling part of your argument is that it is a  'think pointer' and
disable does nothing. So can you update clk_rfree() etc. to document
what is allowed to be done in that function?

Reviewed-by: Simon Glass <sjg@chromium.org>

Regards,
Simon

Re: [PATCH 1/7] clk: Make rfree return void

[Sean Anderson]

On 2/26/22 1:36 PM, Simon Glass wrote:
> Hi Sean,
> 
> On Tue, 1 Feb 2022 at 21:24, Sean Anderson <seanga2@gmail.com> wrote:
>>
>> On 2/1/22 10:59 PM, Simon Glass wrote:
>>> Hi Sean,
>>>
>>> On Tue, 1 Feb 2022 at 07:49, Sean Anderson <seanga2@gmail.com> wrote:
>>>>
>>>> On 1/27/22 4:35 PM, Simon Glass wrote:
>>>>> Hi Sean,
>>>>>
>>>>> On Thu, 27 Jan 2022 at 08:43, Sean Anderson <seanga2@gmail.com> wrote:
>>>>>>
>>>>>> On 1/27/22 10:05 AM, Simon Glass wrote:
>>>>>>> Hi Sean,
>>>>>>>
>>>>>>> On Sat, 15 Jan 2022 at 15:25, Sean Anderson <seanga2@gmail.com> wrote:
>>>>>>>>
>>>>>>>> When freeing a clock there is not much we can do if there is an error, and
>>>>>>>> most callers do not actually check the return value. Even e.g. checking to
>>>>>>>> make sure that clk->id is valid should have been done in request() in the
>>>>>>>> first place (unless someone is messing with the driver behind our back).
>>>>>>>> Just return void and don't bother returning an error.
>>>>>>>>
>>>>>>>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>>>>>>>> ---
>>>>>>>>
>>>>>>>>      drivers/clk/clk-uclass.c  | 7 +++----
>>>>>>>>      drivers/clk/clk_sandbox.c | 6 +++---
>>>>>>>>      include/clk-uclass.h      | 8 +++-----
>>>>>>>>      3 files changed, 9 insertions(+), 12 deletions(-)
>>>>>>>>
>>>>>>>
>>>>>>> We have the same thing in other places too, but I am a little worried
>>>>>>> about removing error checking. We try to avoid checking arguments too
>>>>>>> much in U-Boot, due to code-size concerns, so I suppose I agree that
>>>>>>> an invalid clk should be caught by a debug assertion rather than a
>>>>>>> full check. But with driver model we have generally added an error
>>>>>>> return to every uclass method, for consistency and to permit returning
>>>>>>> error information if needed.
>>>>>>>
>>>>>>> Regards,
>>>>>>> Simon
>>>>>>>
>>>>>>
>>>>>> So there are a few reasons why I don't think a return value is useful
>>>>>> here. To illustrate this, consider a typical user of the clock API:
>>>>>>
>>>>>>            struct clk a, b;
>>>>>>
>>>>>>            ret = clk_get_by_name(dev, "a", &a);
>>>>>>            if (ret)
>>>>>>                    return ret;
>>>>>>
>>>>>>            ret = clk_get_by_name(dev, "b", &b);
>>>>>>            if (ret)
>>>>>>                    goto free_a;
>>>>>>
>>>>>>            ret = clk_set_rate(&a, 5000000);
>>>>>>            if (ret)
>>>>>>                    goto free_b;
>>>>>>
>>>>>>            ret = clk_enable(&b);
>>>>>>
>>>>>> free_b:
>>>>>>            clk_free(&b);
>>>>>> free_a:
>>>>>>            clk_free(&a);
>>>>>>            return ret;
>>>>>>
>>>>>> - Because a and b are "thick pointers" they do not need any cleanup to
>>>>>>       free their own resources. The only cleanup might be if the clock
>>>>>>       driver has allocated something in clk_request (more on this below)
>>>>>> - By the time we call clk_free, the mutable portions of the function
>>>>>>       have already completed. In effect, the function has succeeded,
>>>>>>       regardless of whether clk_free fails. Additionally, we cannot take any
>>>>>>       action if it fails, since we still have to free both clocks.
>>>>>> - clk_free occurs during the error path of the function. Even if it
>>>>>>       errored, we do not want to override the existing error from one of the
>>>>>>       functions doing "real" work.
>>>>>>
>>>>>> The last thing is that no clock driver actually does anything in rfree.
>>>>>> The only driver with this function is the sandbox driver. I would like
>>>>>> to remove the function altogether. As I understand it, the existing API
>>>>>> is inspired by the reset drivers, so I would like to review its usage in
>>>>>> the reset subsystem before removing it for the clock subsystem. I also
>>>>>> want to make some changes to how rates and enables/disables are
>>>>>> calculated which might provide a case for rfree. But once that is
>>>>>> complete I think there will be no users still.
>>>>>
>>>>> What does this all look like in Linux?
>>>>
>>>> Their equivalent (clk_put) returns void, and generally so do most other
>>>> cleanup functions, since .device_remove also returns void.
>>>
>>> We really cannot ignore errors from device_remove().
>>
>> Once you are at device_remove, all the users are gone and it's up to the
>> device to clean up after itself. And often there is nothing we can do
>> once remove is called. As you yourself say in device_remove,
>>
>>          /* We can't put the children back */
> 
> Well this assumes that device_remove() is actually removing the
> device, not just disabling DMA, etc.
> 
>>
>> Really the only sensible thing is to print an error and continue booting
>> if possible.
>>
>> And of course no clock drivers actually use this function anyway, nor do
>> (all but 5) users check it.
>>
>>> Anyway I think what you say about the 'thick pointer' makes sense. But
>>> my expectation was that removing a clock might turn off a clock above
>>> it in the tree, for example.
>>
>> No, this just frees resources (as is documented). If you want to turn
>> off a clock, you have to call clk_disable. In fact, a very common use
>> case is just like the example above, where the consmer frees the clock
>> after enabling it.
>>
>> (This is also why clk->enable_count/rate are basically useless for
>> anything other than CCF clocks)
> 
> How about a clock provided by an audio codec on an I2C bus? Should
> clk_free() do anything in that case? I assume not. I think the
> compelling part of your argument is that it is a  'think pointer' and
> disable does nothing. So can you update clk_rfree() etc. to document
> what is allowed to be done in that function?

The ideal case would be if you wanted to allocate some per-struct-clk
data. Then, the correct place to free it would be rfree. But no one
does this, and if they did it would probably be better to free things
in remove.

Actually... no one in clk, reset, or power-domain does anything with
rfree. So I am inclined to just remove it altogether.

--Sean

> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Regards,
> Simon
> 

Re: [PATCH 1/7] clk: Make rfree return void

[Simon Glass]

Hi Sean,

On Sun, 27 Feb 2022 at 12:38, Sean Anderson <seanga2@gmail.com> wrote:
>
> On 2/26/22 1:36 PM, Simon Glass wrote:
> > Hi Sean,
> >
> > On Tue, 1 Feb 2022 at 21:24, Sean Anderson <seanga2@gmail.com> wrote:
> >>
> >> On 2/1/22 10:59 PM, Simon Glass wrote:
> >>> Hi Sean,
> >>>
> >>> On Tue, 1 Feb 2022 at 07:49, Sean Anderson <seanga2@gmail.com> wrote:
> >>>>
> >>>> On 1/27/22 4:35 PM, Simon Glass wrote:
> >>>>> Hi Sean,
> >>>>>
> >>>>> On Thu, 27 Jan 2022 at 08:43, Sean Anderson <seanga2@gmail.com> wrote:
> >>>>>>
> >>>>>> On 1/27/22 10:05 AM, Simon Glass wrote:
> >>>>>>> Hi Sean,
> >>>>>>>
> >>>>>>> On Sat, 15 Jan 2022 at 15:25, Sean Anderson <seanga2@gmail.com> wrote:
> >>>>>>>>
> >>>>>>>> When freeing a clock there is not much we can do if there is an error, and
> >>>>>>>> most callers do not actually check the return value. Even e.g. checking to
> >>>>>>>> make sure that clk->id is valid should have been done in request() in the
> >>>>>>>> first place (unless someone is messing with the driver behind our back).
> >>>>>>>> Just return void and don't bother returning an error.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> >>>>>>>> ---
> >>>>>>>>
> >>>>>>>>      drivers/clk/clk-uclass.c  | 7 +++----
> >>>>>>>>      drivers/clk/clk_sandbox.c | 6 +++---
> >>>>>>>>      include/clk-uclass.h      | 8 +++-----
> >>>>>>>>      3 files changed, 9 insertions(+), 12 deletions(-)
> >>>>>>>>
> >>>>>>>
> >>>>>>> We have the same thing in other places too, but I am a little worried
> >>>>>>> about removing error checking. We try to avoid checking arguments too
> >>>>>>> much in U-Boot, due to code-size concerns, so I suppose I agree that
> >>>>>>> an invalid clk should be caught by a debug assertion rather than a
> >>>>>>> full check. But with driver model we have generally added an error
> >>>>>>> return to every uclass method, for consistency and to permit returning
> >>>>>>> error information if needed.
> >>>>>>>
> >>>>>>> Regards,
> >>>>>>> Simon
> >>>>>>>
> >>>>>>
> >>>>>> So there are a few reasons why I don't think a return value is useful
> >>>>>> here. To illustrate this, consider a typical user of the clock API:
> >>>>>>
> >>>>>>            struct clk a, b;
> >>>>>>
> >>>>>>            ret = clk_get_by_name(dev, "a", &a);
> >>>>>>            if (ret)
> >>>>>>                    return ret;
> >>>>>>
> >>>>>>            ret = clk_get_by_name(dev, "b", &b);
> >>>>>>            if (ret)
> >>>>>>                    goto free_a;
> >>>>>>
> >>>>>>            ret = clk_set_rate(&a, 5000000);
> >>>>>>            if (ret)
> >>>>>>                    goto free_b;
> >>>>>>
> >>>>>>            ret = clk_enable(&b);
> >>>>>>
> >>>>>> free_b:
> >>>>>>            clk_free(&b);
> >>>>>> free_a:
> >>>>>>            clk_free(&a);
> >>>>>>            return ret;
> >>>>>>
> >>>>>> - Because a and b are "thick pointers" they do not need any cleanup to
> >>>>>>       free their own resources. The only cleanup might be if the clock
> >>>>>>       driver has allocated something in clk_request (more on this below)
> >>>>>> - By the time we call clk_free, the mutable portions of the function
> >>>>>>       have already completed. In effect, the function has succeeded,
> >>>>>>       regardless of whether clk_free fails. Additionally, we cannot take any
> >>>>>>       action if it fails, since we still have to free both clocks.
> >>>>>> - clk_free occurs during the error path of the function. Even if it
> >>>>>>       errored, we do not want to override the existing error from one of the
> >>>>>>       functions doing "real" work.
> >>>>>>
> >>>>>> The last thing is that no clock driver actually does anything in rfree.
> >>>>>> The only driver with this function is the sandbox driver. I would like
> >>>>>> to remove the function altogether. As I understand it, the existing API
> >>>>>> is inspired by the reset drivers, so I would like to review its usage in
> >>>>>> the reset subsystem before removing it for the clock subsystem. I also
> >>>>>> want to make some changes to how rates and enables/disables are
> >>>>>> calculated which might provide a case for rfree. But once that is
> >>>>>> complete I think there will be no users still.
> >>>>>
> >>>>> What does this all look like in Linux?
> >>>>
> >>>> Their equivalent (clk_put) returns void, and generally so do most other
> >>>> cleanup functions, since .device_remove also returns void.
> >>>
> >>> We really cannot ignore errors from device_remove().
> >>
> >> Once you are at device_remove, all the users are gone and it's up to the
> >> device to clean up after itself. And often there is nothing we can do
> >> once remove is called. As you yourself say in device_remove,
> >>
> >>          /* We can't put the children back */
> >
> > Well this assumes that device_remove() is actually removing the
> > device, not just disabling DMA, etc.
> >
> >>
> >> Really the only sensible thing is to print an error and continue booting
> >> if possible.
> >>
> >> And of course no clock drivers actually use this function anyway, nor do
> >> (all but 5) users check it.
> >>
> >>> Anyway I think what you say about the 'thick pointer' makes sense. But
> >>> my expectation was that removing a clock might turn off a clock above
> >>> it in the tree, for example.
> >>
> >> No, this just frees resources (as is documented). If you want to turn
> >> off a clock, you have to call clk_disable. In fact, a very common use
> >> case is just like the example above, where the consmer frees the clock
> >> after enabling it.
> >>
> >> (This is also why clk->enable_count/rate are basically useless for
> >> anything other than CCF clocks)
> >
> > How about a clock provided by an audio codec on an I2C bus? Should
> > clk_free() do anything in that case? I assume not. I think the
> > compelling part of your argument is that it is a  'think pointer' and
> > disable does nothing. So can you update clk_rfree() etc. to document
> > what is allowed to be done in that function?
>
> The ideal case would be if you wanted to allocate some per-struct-clk
> data. Then, the correct place to free it would be rfree. But no one
> does this, and if they did it would probably be better to free things
> in remove.
>
> Actually... no one in clk, reset, or power-domain does anything with
> rfree. So I am inclined to just remove it altogether.

Well, I suppose it is easy enough to add later, if needed. What does
Linux use this for?


>
> --Sean
>
> > Reviewed-by: Simon Glass <sjg@chromium.org>
>

Regards,
SImon

Re: [PATCH 1/7] clk: Make rfree return void

[Sean Anderson]

On 3/1/22 9:58 AM, Simon Glass wrote:
> Hi Sean,
> 
> On Sun, 27 Feb 2022 at 12:38, Sean Anderson <seanga2@gmail.com> wrote:
>>
>> On 2/26/22 1:36 PM, Simon Glass wrote:
>>> Hi Sean,
>>>
>>> On Tue, 1 Feb 2022 at 21:24, Sean Anderson <seanga2@gmail.com> wrote:
>>>>
>>>> On 2/1/22 10:59 PM, Simon Glass wrote:
>>>>> Hi Sean,
>>>>>
>>>>> On Tue, 1 Feb 2022 at 07:49, Sean Anderson <seanga2@gmail.com> wrote:
>>>>>>
>>>>>> On 1/27/22 4:35 PM, Simon Glass wrote:
>>>>>>> Hi Sean,
>>>>>>>
>>>>>>> On Thu, 27 Jan 2022 at 08:43, Sean Anderson <seanga2@gmail.com> wrote:
>>>>>>>>
>>>>>>>> On 1/27/22 10:05 AM, Simon Glass wrote:
>>>>>>>>> Hi Sean,
>>>>>>>>>
>>>>>>>>> On Sat, 15 Jan 2022 at 15:25, Sean Anderson <seanga2@gmail.com> wrote:
>>>>>>>>>>
>>>>>>>>>> When freeing a clock there is not much we can do if there is an error, and
>>>>>>>>>> most callers do not actually check the return value. Even e.g. checking to
>>>>>>>>>> make sure that clk->id is valid should have been done in request() in the
>>>>>>>>>> first place (unless someone is messing with the driver behind our back).
>>>>>>>>>> Just return void and don't bother returning an error.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>>>>>>>>>> ---
>>>>>>>>>>
>>>>>>>>>>       drivers/clk/clk-uclass.c  | 7 +++----
>>>>>>>>>>       drivers/clk/clk_sandbox.c | 6 +++---
>>>>>>>>>>       include/clk-uclass.h      | 8 +++-----
>>>>>>>>>>       3 files changed, 9 insertions(+), 12 deletions(-)
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> We have the same thing in other places too, but I am a little worried
>>>>>>>>> about removing error checking. We try to avoid checking arguments too
>>>>>>>>> much in U-Boot, due to code-size concerns, so I suppose I agree that
>>>>>>>>> an invalid clk should be caught by a debug assertion rather than a
>>>>>>>>> full check. But with driver model we have generally added an error
>>>>>>>>> return to every uclass method, for consistency and to permit returning
>>>>>>>>> error information if needed.
>>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>> Simon
>>>>>>>>>
>>>>>>>>
>>>>>>>> So there are a few reasons why I don't think a return value is useful
>>>>>>>> here. To illustrate this, consider a typical user of the clock API:
>>>>>>>>
>>>>>>>>             struct clk a, b;
>>>>>>>>
>>>>>>>>             ret = clk_get_by_name(dev, "a", &a);
>>>>>>>>             if (ret)
>>>>>>>>                     return ret;
>>>>>>>>
>>>>>>>>             ret = clk_get_by_name(dev, "b", &b);
>>>>>>>>             if (ret)
>>>>>>>>                     goto free_a;
>>>>>>>>
>>>>>>>>             ret = clk_set_rate(&a, 5000000);
>>>>>>>>             if (ret)
>>>>>>>>                     goto free_b;
>>>>>>>>
>>>>>>>>             ret = clk_enable(&b);
>>>>>>>>
>>>>>>>> free_b:
>>>>>>>>             clk_free(&b);
>>>>>>>> free_a:
>>>>>>>>             clk_free(&a);
>>>>>>>>             return ret;
>>>>>>>>
>>>>>>>> - Because a and b are "thick pointers" they do not need any cleanup to
>>>>>>>>        free their own resources. The only cleanup might be if the clock
>>>>>>>>        driver has allocated something in clk_request (more on this below)
>>>>>>>> - By the time we call clk_free, the mutable portions of the function
>>>>>>>>        have already completed. In effect, the function has succeeded,
>>>>>>>>        regardless of whether clk_free fails. Additionally, we cannot take any
>>>>>>>>        action if it fails, since we still have to free both clocks.
>>>>>>>> - clk_free occurs during the error path of the function. Even if it
>>>>>>>>        errored, we do not want to override the existing error from one of the
>>>>>>>>        functions doing "real" work.
>>>>>>>>
>>>>>>>> The last thing is that no clock driver actually does anything in rfree.
>>>>>>>> The only driver with this function is the sandbox driver. I would like
>>>>>>>> to remove the function altogether. As I understand it, the existing API
>>>>>>>> is inspired by the reset drivers, so I would like to review its usage in
>>>>>>>> the reset subsystem before removing it for the clock subsystem. I also
>>>>>>>> want to make some changes to how rates and enables/disables are
>>>>>>>> calculated which might provide a case for rfree. But once that is
>>>>>>>> complete I think there will be no users still.
>>>>>>>
>>>>>>> What does this all look like in Linux?
>>>>>>
>>>>>> Their equivalent (clk_put) returns void, and generally so do most other
>>>>>> cleanup functions, since .device_remove also returns void.
>>>>>
>>>>> We really cannot ignore errors from device_remove().
>>>>
>>>> Once you are at device_remove, all the users are gone and it's up to the
>>>> device to clean up after itself. And often there is nothing we can do
>>>> once remove is called. As you yourself say in device_remove,
>>>>
>>>>           /* We can't put the children back */
>>>
>>> Well this assumes that device_remove() is actually removing the
>>> device, not just disabling DMA, etc.
>>>
>>>>
>>>> Really the only sensible thing is to print an error and continue booting
>>>> if possible.
>>>>
>>>> And of course no clock drivers actually use this function anyway, nor do
>>>> (all but 5) users check it.
>>>>
>>>>> Anyway I think what you say about the 'thick pointer' makes sense. But
>>>>> my expectation was that removing a clock might turn off a clock above
>>>>> it in the tree, for example.
>>>>
>>>> No, this just frees resources (as is documented). If you want to turn
>>>> off a clock, you have to call clk_disable. In fact, a very common use
>>>> case is just like the example above, where the consmer frees the clock
>>>> after enabling it.
>>>>
>>>> (This is also why clk->enable_count/rate are basically useless for
>>>> anything other than CCF clocks)
>>>
>>> How about a clock provided by an audio codec on an I2C bus? Should
>>> clk_free() do anything in that case? I assume not. I think the
>>> compelling part of your argument is that it is a  'think pointer' and
>>> disable does nothing. So can you update clk_rfree() etc. to document
>>> what is allowed to be done in that function?
>>
>> The ideal case would be if you wanted to allocate some per-struct-clk
>> data. Then, the correct place to free it would be rfree. But no one
>> does this, and if they did it would probably be better to free things
>> in remove.
>>
>> Actually... no one in clk, reset, or power-domain does anything with
>> rfree. So I am inclined to just remove it altogether.
> 
> Well, I suppose it is easy enough to add later, if needed. What does
> Linux use this for?

As far as I can tell, Linux doesn't have request/free. There's
of_clk_provider->get(), but that corresponds more to of_xlate.
Similarly, there's reset_controller_dev->of_xlate(), but no request/free.

--Sean

Re: [PATCH 1/7] clk: Make rfree return void

[Simon Glass]

HI Sean,

On Wed, 16 Mar 2022 at 10:18, Sean Anderson <seanga2@gmail.com> wrote:
>
> On 3/1/22 9:58 AM, Simon Glass wrote:
> > Hi Sean,
> >
> > On Sun, 27 Feb 2022 at 12:38, Sean Anderson <seanga2@gmail.com> wrote:
> >>
> >> On 2/26/22 1:36 PM, Simon Glass wrote:
> >>> Hi Sean,
> >>>
> >>> On Tue, 1 Feb 2022 at 21:24, Sean Anderson <seanga2@gmail.com> wrote:
> >>>>
> >>>> On 2/1/22 10:59 PM, Simon Glass wrote:
> >>>>> Hi Sean,
> >>>>>
> >>>>> On Tue, 1 Feb 2022 at 07:49, Sean Anderson <seanga2@gmail.com> wrote:
> >>>>>>
> >>>>>> On 1/27/22 4:35 PM, Simon Glass wrote:
> >>>>>>> Hi Sean,
> >>>>>>>
> >>>>>>> On Thu, 27 Jan 2022 at 08:43, Sean Anderson <seanga2@gmail.com> wrote:
> >>>>>>>>
> >>>>>>>> On 1/27/22 10:05 AM, Simon Glass wrote:
> >>>>>>>>> Hi Sean,
> >>>>>>>>>
> >>>>>>>>> On Sat, 15 Jan 2022 at 15:25, Sean Anderson <seanga2@gmail.com> wrote:
> >>>>>>>>>>
> >>>>>>>>>> When freeing a clock there is not much we can do if there is an error, and
> >>>>>>>>>> most callers do not actually check the return value. Even e.g. checking to
> >>>>>>>>>> make sure that clk->id is valid should have been done in request() in the
> >>>>>>>>>> first place (unless someone is messing with the driver behind our back).
> >>>>>>>>>> Just return void and don't bother returning an error.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> >>>>>>>>>> ---
> >>>>>>>>>>
> >>>>>>>>>>       drivers/clk/clk-uclass.c  | 7 +++----
> >>>>>>>>>>       drivers/clk/clk_sandbox.c | 6 +++---
> >>>>>>>>>>       include/clk-uclass.h      | 8 +++-----
> >>>>>>>>>>       3 files changed, 9 insertions(+), 12 deletions(-)
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> We have the same thing in other places too, but I am a little worried
> >>>>>>>>> about removing error checking. We try to avoid checking arguments too
> >>>>>>>>> much in U-Boot, due to code-size concerns, so I suppose I agree that
> >>>>>>>>> an invalid clk should be caught by a debug assertion rather than a
> >>>>>>>>> full check. But with driver model we have generally added an error
> >>>>>>>>> return to every uclass method, for consistency and to permit returning
> >>>>>>>>> error information if needed.
> >>>>>>>>>
> >>>>>>>>> Regards,
> >>>>>>>>> Simon
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> So there are a few reasons why I don't think a return value is useful
> >>>>>>>> here. To illustrate this, consider a typical user of the clock API:
> >>>>>>>>
> >>>>>>>>             struct clk a, b;
> >>>>>>>>
> >>>>>>>>             ret = clk_get_by_name(dev, "a", &a);
> >>>>>>>>             if (ret)
> >>>>>>>>                     return ret;
> >>>>>>>>
> >>>>>>>>             ret = clk_get_by_name(dev, "b", &b);
> >>>>>>>>             if (ret)
> >>>>>>>>                     goto free_a;
> >>>>>>>>
> >>>>>>>>             ret = clk_set_rate(&a, 5000000);
> >>>>>>>>             if (ret)
> >>>>>>>>                     goto free_b;
> >>>>>>>>
> >>>>>>>>             ret = clk_enable(&b);
> >>>>>>>>
> >>>>>>>> free_b:
> >>>>>>>>             clk_free(&b);
> >>>>>>>> free_a:
> >>>>>>>>             clk_free(&a);
> >>>>>>>>             return ret;
> >>>>>>>>
> >>>>>>>> - Because a and b are "thick pointers" they do not need any cleanup to
> >>>>>>>>        free their own resources. The only cleanup might be if the clock
> >>>>>>>>        driver has allocated something in clk_request (more on this below)
> >>>>>>>> - By the time we call clk_free, the mutable portions of the function
> >>>>>>>>        have already completed. In effect, the function has succeeded,
> >>>>>>>>        regardless of whether clk_free fails. Additionally, we cannot take any
> >>>>>>>>        action if it fails, since we still have to free both clocks.
> >>>>>>>> - clk_free occurs during the error path of the function. Even if it
> >>>>>>>>        errored, we do not want to override the existing error from one of the
> >>>>>>>>        functions doing "real" work.
> >>>>>>>>
> >>>>>>>> The last thing is that no clock driver actually does anything in rfree.
> >>>>>>>> The only driver with this function is the sandbox driver. I would like
> >>>>>>>> to remove the function altogether. As I understand it, the existing API
> >>>>>>>> is inspired by the reset drivers, so I would like to review its usage in
> >>>>>>>> the reset subsystem before removing it for the clock subsystem. I also
> >>>>>>>> want to make some changes to how rates and enables/disables are
> >>>>>>>> calculated which might provide a case for rfree. But once that is
> >>>>>>>> complete I think there will be no users still.
> >>>>>>>
> >>>>>>> What does this all look like in Linux?
> >>>>>>
> >>>>>> Their equivalent (clk_put) returns void, and generally so do most other
> >>>>>> cleanup functions, since .device_remove also returns void.
> >>>>>
> >>>>> We really cannot ignore errors from device_remove().
> >>>>
> >>>> Once you are at device_remove, all the users are gone and it's up to the
> >>>> device to clean up after itself. And often there is nothing we can do
> >>>> once remove is called. As you yourself say in device_remove,
> >>>>
> >>>>           /* We can't put the children back */
> >>>
> >>> Well this assumes that device_remove() is actually removing the
> >>> device, not just disabling DMA, etc.
> >>>
> >>>>
> >>>> Really the only sensible thing is to print an error and continue booting
> >>>> if possible.
> >>>>
> >>>> And of course no clock drivers actually use this function anyway, nor do
> >>>> (all but 5) users check it.
> >>>>
> >>>>> Anyway I think what you say about the 'thick pointer' makes sense. But
> >>>>> my expectation was that removing a clock might turn off a clock above
> >>>>> it in the tree, for example.
> >>>>
> >>>> No, this just frees resources (as is documented). If you want to turn
> >>>> off a clock, you have to call clk_disable. In fact, a very common use
> >>>> case is just like the example above, where the consmer frees the clock
> >>>> after enabling it.
> >>>>
> >>>> (This is also why clk->enable_count/rate are basically useless for
> >>>> anything other than CCF clocks)
> >>>
> >>> How about a clock provided by an audio codec on an I2C bus? Should
> >>> clk_free() do anything in that case? I assume not. I think the
> >>> compelling part of your argument is that it is a  'think pointer' and
> >>> disable does nothing. So can you update clk_rfree() etc. to document
> >>> what is allowed to be done in that function?
> >>
> >> The ideal case would be if you wanted to allocate some per-struct-clk
> >> data. Then, the correct place to free it would be rfree. But no one
> >> does this, and if they did it would probably be better to free things
> >> in remove.
> >>
> >> Actually... no one in clk, reset, or power-domain does anything with
> >> rfree. So I am inclined to just remove it altogether.
> >
> > Well, I suppose it is easy enough to add later, if needed. What does
> > Linux use this for?
>
> As far as I can tell, Linux doesn't have request/free. There's
> of_clk_provider->get(), but that corresponds more to of_xlate.
> Similarly, there's reset_controller_dev->of_xlate(), but no request/free.

OK, sounds good then.

Regards,
Simon

Re: [PATCH 0/7] clk: Make clk_free return void

[Sean Anderson]

On Sat, 15 Jan 2022 17:24:57 -0500, Sean Anderson wrote:
> clk_free cleans up resources allocated by clk_request et. al. It returns an
> error code, but it really shouldn't. Much like regular free(), there is
> typically no way to handle an error, and errors from clk_free shouldn't prevent
> progress in the rest of the program. Make clk_free (and rfree) return void.
> 
> 
> Sean Anderson (7):
>   clk: Make rfree return void
>   dma: bcm6348: Don't check clk_free
>   net: bcm63xx: Don't check clk_free
>   phy: bcm63xx: Don't check clk_free
>   spi: bcm63xx: Don't check clk_free
>   spi: dw: Don't check clk_free
>   clk: Make clk_free return void
> 
> [...]

Applied, thanks!

[1/7] clk: Make rfree return void
      commit: 276d446757e462c210768eb0bbd48450ae254f51
[2/7] dma: bcm6348: Don't check clk_free
      commit: 454af567edc0b02842c83aaf1a60bbcb766af0cd
[3/7] net: bcm63xx: Don't check clk_free
      commit: b2e0889abacfd453131359156fe279996727d95b
[4/7] phy: bcm63xx: Don't check clk_free
      commit: ad20358c7462159d5f9012facba9dec1e197aaca
[5/7] spi: bcm63xx: Don't check clk_free
      commit: dfdb227c3d018983f37cc97fe003e231a81a46ea
[6/7] spi: dw: Don't check clk_free
      commit: 3cbdd4cab951b8bd3f2e76066e6911f9780c4eb1
[7/7] clk: Make clk_free return void
      commit: ac15e789caecec19d29ee9c5869305d3c3ddfb42

Best regards,
-- 
Sean Anderson <seanga2@gmail.com>