The commit 6b8e090ecc3d ("regmap: use IS_ERR() to check clk_get()
results") assumes that CLK calls return the error pointer when clock
is not found. However in the current code the described situation
is simply impossible, because the regmap won't be created with
missed clock if requested. The only way when it can be the case is
what the above mentioned commit introduced by itself, when clock is
not provided.
Taking above into consideration, effectively revert the commit
6b8e090ecc3d and while at it, drop unneeded NULL checks since CLK
calls are NULL-aware.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/base/regmap/regmap-mmio.c | 25 +++++++++----------------
1 file changed, 9 insertions(+), 16 deletions(-)
diff --git a/drivers/base/regmap/regmap-mmio.c b/drivers/base/regmap/regmap-mmio.c
index e83a2c3ba95a..e1923506a89a 100644
--- a/drivers/base/regmap/regmap-mmio.c
+++ b/drivers/base/regmap/regmap-mmio.c
@@ -146,16 +146,13 @@ static int regmap_mmio_write(void *context, unsigned int reg, unsigned int val)
struct regmap_mmio_context *ctx = context;
int ret;
- if (!IS_ERR(ctx->clk)) {
- ret = clk_enable(ctx->clk);
- if (ret < 0)
- return ret;
- }
+ ret = clk_enable(ctx->clk);
+ if (ret < 0)
+ return ret;
ctx->reg_write(ctx, reg, val);
- if (!IS_ERR(ctx->clk))
- clk_disable(ctx->clk);
+ clk_disable(ctx->clk);
return 0;
}
@@ -227,16 +224,13 @@ static int regmap_mmio_read(void *context, unsigned int reg, unsigned int *val)
struct regmap_mmio_context *ctx = context;
int ret;
- if (!IS_ERR(ctx->clk)) {
- ret = clk_enable(ctx->clk);
- if (ret < 0)
- return ret;
- }
+ ret = clk_enable(ctx->clk);
+ if (ret < 0)
+ return ret;
*val = ctx->reg_read(ctx, reg);
- if (!IS_ERR(ctx->clk))
- clk_disable(ctx->clk);
+ clk_disable(ctx->clk);
return 0;
}
@@ -245,7 +239,7 @@ static void regmap_mmio_free_context(void *context)
{
struct regmap_mmio_context *ctx = context;
- if (!IS_ERR(ctx->clk) && !ctx->attached_clk) {
+ if (!ctx->attached_clk) {
clk_unprepare(ctx->clk);
clk_put(ctx->clk);
}
@@ -290,7 +284,6 @@ static struct regmap_mmio_context *regmap_mmio_gen_context(struct device *dev,
ctx->regs = regs;
ctx->val_bytes = config->val_bits / 8;
ctx->relaxed_mmio = config->use_relaxed_mmio;
- ctx->clk = ERR_PTR(-ENODEV);
switch (regmap_get_val_endian(dev, ®map_mmio, config)) {
case REGMAP_ENDIAN_DEFAULT:
--
2.35.1
On Fri, Aug 05, 2022 at 11:53:18PM +0300, Andy Shevchenko wrote:
> The commit 6b8e090ecc3d ("regmap: use IS_ERR() to check clk_get()
> results") assumes that CLK calls return the error pointer when clock
> is not found. However in the current code the described situation
> is simply impossible, because the regmap won't be created with
> missed clock if requested. The only way when it can be the case is
> what the above mentioned commit introduced by itself, when clock is
> not provided.
> Taking above into consideration, effectively revert the commit
> 6b8e090ecc3d and while at it, drop unneeded NULL checks since CLK
> calls are NULL-aware.
I don't understand the supposed benefit of this. Yes, the clk API does
currently accept NULL as a valid clock and returns it as a dummy but
explicitly taking advantage of that in the way that this does just feels
more sloppy than the current behaviour.
On Mon, Aug 8, 2022 at 3:28 PM Mark Brown <[email protected]> wrote:
> On Fri, Aug 05, 2022 at 11:53:18PM +0300, Andy Shevchenko wrote:
>
> > The commit 6b8e090ecc3d ("regmap: use IS_ERR() to check clk_get()
> > results") assumes that CLK calls return the error pointer when clock
> > is not found. However in the current code the described situation
> > is simply impossible, because the regmap won't be created with
> > missed clock if requested. The only way when it can be the case is
> > what the above mentioned commit introduced by itself, when clock is
> > not provided.
>
> > Taking above into consideration, effectively revert the commit
> > 6b8e090ecc3d and while at it, drop unneeded NULL checks since CLK
> > calls are NULL-aware.
>
> I don't understand the supposed benefit of this. Yes, the clk API does
> currently accept NULL as a valid clock and returns it as a dummy but
> explicitly taking advantage of that in the way that this does just feels
> more sloppy than the current behaviour.
How? The clock is still checked by NULL by the clock framework. The
above mentioned patch is simply wrong (taking into account modern
behaviour of CCF APIs) and reverting it clears things.
--
With Best Regards,
Andy Shevchenko