| Message ID | 98afbc28-3366-459e-bd01-f77cf1a67fe9@moroto.mountain |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: sun6i-p2wi: Fix an error message in probe() | expand |
On Tue, Jun 27, 2023 at 01:59:20PM +0200, Andi Shyti wrote: > Hi Dan, > > On Tue, Jun 27, 2023 at 10:12:36AM +0300, Dan Carpenter wrote: > > The "ret" variable is uninitialized. It was the "p2wi->rstc" variable > > that was intended. We can also use the %pe string format to print the > > error code name instead of just the number. > > > > Fixes: 75ff8a340a81 ("i2c: sun6i-p2wi: Use devm_clk_get_enabled()") > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > --- > > drivers/i2c/busses/i2c-sun6i-p2wi.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/i2c/busses/i2c-sun6i-p2wi.c b/drivers/i2c/busses/i2c-sun6i-p2wi.c > > index ad8270cdbd3e..fa6020dced59 100644 > > --- a/drivers/i2c/busses/i2c-sun6i-p2wi.c > > +++ b/drivers/i2c/busses/i2c-sun6i-p2wi.c > > @@ -250,7 +250,8 @@ static int p2wi_probe(struct platform_device *pdev) > > > > p2wi->rstc = devm_reset_control_get_exclusive(dev, NULL); > > if (IS_ERR(p2wi->rstc)) { > > - dev_err(dev, "failed to retrieve reset controller: %d\n", ret); > > + dev_err(dev, "failed to retrieve reset controller: %pe\n", > > + p2wi->rstc); > > Yes, good catch! Thanks! But I think we want to print the error > value here, so I think it should be: > > - dev_err(dev, "failed to retrieve reset controller: %d\n", ret); > + dev_err(dev, "failed to retrieve reset controller: %d\n", > + PTR_ERR(p2wi->rstc)); > The %pe which I changed it to is a cool new thing that prints: failed to retrieve reset controller: -EINVAL\n We should create a similar %e printk format that works for ints instead of error pointers. But instead of that you have people who cast error codes to pointers just to get the %pe functionality. And other people who make suggestions (this is the catagory that I'm in) but are too lazy to do the actual work. regards, dan carpenter
Hi Dan, On Tue, Jun 27, 2023 at 03:08:53PM +0300, Dan Carpenter wrote: > On Tue, Jun 27, 2023 at 01:59:20PM +0200, Andi Shyti wrote: > > Hi Dan, > > > > On Tue, Jun 27, 2023 at 10:12:36AM +0300, Dan Carpenter wrote: > > > The "ret" variable is uninitialized. It was the "p2wi->rstc" variable > > > that was intended. We can also use the %pe string format to print the > > > error code name instead of just the number. > > > > > > Fixes: 75ff8a340a81 ("i2c: sun6i-p2wi: Use devm_clk_get_enabled()") > > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > > --- > > > drivers/i2c/busses/i2c-sun6i-p2wi.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/i2c/busses/i2c-sun6i-p2wi.c b/drivers/i2c/busses/i2c-sun6i-p2wi.c > > > index ad8270cdbd3e..fa6020dced59 100644 > > > --- a/drivers/i2c/busses/i2c-sun6i-p2wi.c > > > +++ b/drivers/i2c/busses/i2c-sun6i-p2wi.c > > > @@ -250,7 +250,8 @@ static int p2wi_probe(struct platform_device *pdev) > > > > > > p2wi->rstc = devm_reset_control_get_exclusive(dev, NULL); > > > if (IS_ERR(p2wi->rstc)) { > > > - dev_err(dev, "failed to retrieve reset controller: %d\n", ret); > > > + dev_err(dev, "failed to retrieve reset controller: %pe\n", > > > + p2wi->rstc); > > > > Yes, good catch! Thanks! But I think we want to print the error > > value here, so I think it should be: > > > > - dev_err(dev, "failed to retrieve reset controller: %d\n", ret); > > + dev_err(dev, "failed to retrieve reset controller: %d\n", > > + PTR_ERR(p2wi->rstc)); > > > > The %pe which I changed it to is a cool new thing that prints: > > failed to retrieve reset controller: -EINVAL\n oh... that's right! Sorry, I didn't know about it! Then, definitely: Reviewed-by: Andi Shyti <andi.shyti@kernel.org> > We should create a similar %e printk format that works for ints instead > of error pointers. But instead of that you have people who cast error > codes to pointers just to get the %pe functionality. And other people > who make suggestions (this is the catagory that I'm in) but are too lazy > to do the actual work. Ahaha... that's right! It's indeed a nice feature to have. Let me see if I manage to scratch a few hours out of my time. Andi
On Tue, Jun 27, 2023 at 10:12:36AM +0300, Dan Carpenter wrote: > The "ret" variable is uninitialized. It was the "p2wi->rstc" variable > that was intended. We can also use the %pe string format to print the > error code name instead of just the number. > > Fixes: 75ff8a340a81 ("i2c: sun6i-p2wi: Use devm_clk_get_enabled()") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> Applied to for-current, thanks!
diff --git a/drivers/i2c/busses/i2c-sun6i-p2wi.c b/drivers/i2c/busses/i2c-sun6i-p2wi.c index ad8270cdbd3e..fa6020dced59 100644 --- a/drivers/i2c/busses/i2c-sun6i-p2wi.c +++ b/drivers/i2c/busses/i2c-sun6i-p2wi.c @@ -250,7 +250,8 @@ static int p2wi_probe(struct platform_device *pdev) p2wi->rstc = devm_reset_control_get_exclusive(dev, NULL); if (IS_ERR(p2wi->rstc)) { - dev_err(dev, "failed to retrieve reset controller: %d\n", ret); + dev_err(dev, "failed to retrieve reset controller: %pe\n", + p2wi->rstc); return PTR_ERR(p2wi->rstc); }
The "ret" variable is uninitialized. It was the "p2wi->rstc" variable that was intended. We can also use the %pe string format to print the error code name instead of just the number. Fixes: 75ff8a340a81 ("i2c: sun6i-p2wi: Use devm_clk_get_enabled()") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- drivers/i2c/busses/i2c-sun6i-p2wi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)