Hi Stephen,
2016-04-14 9:33 GMT+09:00 Stephen Boyd <sboyd@xxxxxxxxxxxxxx>:
On 04/08, Masahiro Yamada wrote:
This makes our driver programming life easier.
For example, let's see drivers/tty/serial/8250/8250_of.c
The "clock-frequency" DT property takes precedence over "clocks" property.
So, it is valid to probe the driver with a NULL pointer for info->clk.
if (of_property_read_u32(np, "clock-frequency", &clk)) {
/* Get clk rate through clk driver if present */
info->clk = devm_clk_get(&ofdev->dev, NULL);
if (IS_ERR(info->clk)) {
dev_warn(&ofdev->dev,
"clk or clock-frequency not defined\n");
return PTR_ERR(info->clk);
}
ret = clk_prepare_enable(info->clk);
if (ret < 0)
return ret;
clk = clk_get_rate(info->clk);
}
As a result, we need to make sure the clk pointer is valid
before calling clk_disable_unprepare().
If we could support pointer checking in callees, we would be able to
clean-up lots of clock consumers.
I'm not sure if you meant to use that example for the error
pointer case? It bails out if clk_get() returns an error pointer.
I'm all for a no-op in clk_disable()/unprepare() when the pointer
is NULL. But when it's an error pointer the driver should be
handling it and bail out before it would ever call enable/prepare
on it or disable/unprepare.
Let me explain my original idea.
We do various initialization in a probe method,
so we (OK, I) sometimes want to split init code
into some helper function(s) like this:
static int foo_clk_init(struct platform_device *pdev,
struct foo_priv *priv)
{
int ret;
priv->clk = devm_clk_get(&pdev->dev, NULL); /* case 1 */
if (IS_ERR(priv->clk)) {
dev_err(&pdev->dev, "falied to get clk\n");
return PTR_ERR(priv->clk);
}
ret = clk_prepare_enable(priv->clk); /* case 2 */
if (ret < 0) {
dev_err(&pdev->dev, "falied to enable clk\n");
return ret;
}
priv->clk_rate = clk_get_rate(priv->clk); /* case 3 */
if (!priv->clk_rate) {
dev_err(&pdev->dev, "clk rate should not be zero\n");
return -EINVAL;
}
[ do something ]
return 0;
}
static int foo_probe(struct platform_device *pdev)
{
[memory allocation, OF parse, various init.... ]
ret = foo_clk_init(pdev, priv);
if (ret < 0)
goto err;
ret = foo_blahblah_init(pdev, priv) /* case 4 */
ir (ret < 0)
goto err;
[ more initialization ... ]
return 0;
err:
clk_disable_unprepare(priv->clk);
return ret;
}
There are some failure paths in this example.
[1] If case 1 fails, priv->clk contains an error pointer.
We should not do clk_disable_unprepare().
[2] If case 2 fails, priv->clk contains a valid pointer,
but we should not do clk_disable_unprepare().
[3] If case 3 fails, priv->clk contains a valid pointer,
and we should do clk_disable_unprepare().
[4] If case 4 fails, priv->clk contains a valid pointer,
and we should do clk_disable_unprepare().
My difficulty is that [1]-[3] are contained in one helper function.
(A real example is drivers/i2c/busses/i2c-uniphier.c)
If foo_clk_init() fails for reason [1],
I want clk_disable_unprepare() to just return.
(This is my original intention of this patch.)
If foo_clk_init() fails for reason [3],
I want clk_disable_unprepare() to do its job.
OK, now I notice another problem in my code;
if foo_clk_init() fails for reason [2],
clk_disable() WARN's due to zero enable_count.
if (WARN_ON(core->enable_count == 0))
return;
Perhaps, I got screwed up by splitting clock init stuff
into a helper function.
--
Best Regards
Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
