[PATCH] iio: adc: pac1921: add missing error return in probe()

[PATCH] iio: adc: pac1921: add missing error return in probe()

[Dan Carpenter]

This error path was intended to return, and not just print an error.  The
current code will lead to an error pointer dereference.

Fixes: 371f778b83cd ("iio: adc: add support for pac1921")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/iio/adc/pac1921.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c
index d04c6685d780..8200a47bdf21 100644
--- a/drivers/iio/adc/pac1921.c
+++ b/drivers/iio/adc/pac1921.c
@@ -1168,8 +1168,8 @@ static int pac1921_probe(struct i2c_client *client)
 
 	priv->regmap = devm_regmap_init_i2c(client, &pac1921_regmap_config);
 	if (IS_ERR(priv->regmap))
-		dev_err_probe(dev, (int)PTR_ERR(priv->regmap),
-			      "Cannot initialize register map\n");
+		return dev_err_probe(dev, (int)PTR_ERR(priv->regmap),
+				     "Cannot initialize register map\n");
 
 	devm_mutex_init(dev, &priv->lock);
 
-- 
2.43.0

Re: [PATCH] iio: adc: pac1921: add missing error return in probe()

[Christophe JAILLET]

Le 08/08/2024 à 21:28, Dan Carpenter a écrit :
> This error path was intended to return, and not just print an error.  The
> current code will lead to an error pointer dereference.
> 
> Fixes: 371f778b83cd ("iio: adc: add support for pac1921")
> Signed-off-by: Dan Carpenter <dan.carpenter-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>   drivers/iio/adc/pac1921.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c
> index d04c6685d780..8200a47bdf21 100644
> --- a/drivers/iio/adc/pac1921.c
> +++ b/drivers/iio/adc/pac1921.c
> @@ -1168,8 +1168,8 @@ static int pac1921_probe(struct i2c_client *client)
>   
>   	priv->regmap = devm_regmap_init_i2c(client, &pac1921_regmap_config);
>   	if (IS_ERR(priv->regmap))
> -		dev_err_probe(dev, (int)PTR_ERR(priv->regmap),
> -			      "Cannot initialize register map\n");
> +		return dev_err_probe(dev, (int)PTR_ERR(priv->regmap),

The (int) is unusual.

CJ

> +				     "Cannot initialize register map\n");
>   
>   	devm_mutex_init(dev, &priv->lock);
>   

Re: [PATCH] iio: adc: pac1921: add missing error return in probe()

[Matteo Martelli]

Christophe JAILLET wrote:
> Le 08/08/2024 à 21:28, Dan Carpenter a écrit :
> > This error path was intended to return, and not just print an error.  The
> > current code will lead to an error pointer dereference.
> > 
> > Fixes: 371f778b83cd ("iio: adc: add support for pac1921")
> > Signed-off-by: Dan Carpenter <dan.carpenter-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > ---
> >   drivers/iio/adc/pac1921.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c
> > index d04c6685d780..8200a47bdf21 100644
> > --- a/drivers/iio/adc/pac1921.c
> > +++ b/drivers/iio/adc/pac1921.c
> > @@ -1168,8 +1168,8 @@ static int pac1921_probe(struct i2c_client *client)
> >   
> >   	priv->regmap = devm_regmap_init_i2c(client, &pac1921_regmap_config);
> >   	if (IS_ERR(priv->regmap))
> > -		dev_err_probe(dev, (int)PTR_ERR(priv->regmap),
> > -			      "Cannot initialize register map\n");
> > +		return dev_err_probe(dev, (int)PTR_ERR(priv->regmap),
> 
> The (int) is unusual.
>
The (int) explicit cast is to address Wconversion warnings since dev_err_probe
takes an int as argument.
> 
> CJ
> 
> > +				     "Cannot initialize register map\n");
> >   
> >   	devm_mutex_init(dev, &priv->lock);
> >   
> 

Thanks,
Matteo Martelli

Re: [PATCH] iio: adc: pac1921: add missing error return in probe()

[Christophe JAILLET]

Le 09/08/2024 à 09:31, Matteo Martelli a écrit :
> Christophe JAILLET wrote:
>> Le 08/08/2024 à 21:28, Dan Carpenter a écrit :
>>> This error path was intended to return, and not just print an error.  The
>>> current code will lead to an error pointer dereference.
>>>
>>> Fixes: 371f778b83cd ("iio: adc: add support for pac1921")
>>> Signed-off-by: Dan Carpenter <dan.carpenter-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>> ---
>>>    drivers/iio/adc/pac1921.c | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c
>>> index d04c6685d780..8200a47bdf21 100644
>>> --- a/drivers/iio/adc/pac1921.c
>>> +++ b/drivers/iio/adc/pac1921.c
>>> @@ -1168,8 +1168,8 @@ static int pac1921_probe(struct i2c_client *client)
>>>    
>>>    	priv->regmap = devm_regmap_init_i2c(client, &pac1921_regmap_config);
>>>    	if (IS_ERR(priv->regmap))
>>> -		dev_err_probe(dev, (int)PTR_ERR(priv->regmap),
>>> -			      "Cannot initialize register map\n");
>>> +		return dev_err_probe(dev, (int)PTR_ERR(priv->regmap),
>>
>> The (int) is unusual.
>>
> The (int) explicit cast is to address Wconversion warnings since dev_err_probe
> takes an int as argument.

Ok, but:

1) With the cast removed, on my x86_64:
	$ make CFLAGS="-Wconversion" drivers/iio/adc/pac1921.o

doesn't generate any error.

2)
	$ it grep dev_err_probe.*\)PTR_ERR | wc -l
	2

	$ it grep dev_err_probe.*PTR_ERR | wc -l
	1948
So, should the cast be needed, maybe another fix could make sense?

CJ

>>
>> CJ
>>
>>> +				     "Cannot initialize register map\n");
>>>    
>>>    	devm_mutex_init(dev, &priv->lock);
>>>    
>>
> 
> Thanks,
> Matteo Martelli
> 

Re: [PATCH] iio: adc: pac1921: add missing error return in probe()

[Matteo Martelli]

Christophe JAILLET wrote:
> Le 09/08/2024 à 09:31, Matteo Martelli a écrit :
> > Christophe JAILLET wrote:
> >> Le 08/08/2024 à 21:28, Dan Carpenter a écrit :
> >>> This error path was intended to return, and not just print an error.  The
> >>> current code will lead to an error pointer dereference.
> >>>
> >>> Fixes: 371f778b83cd ("iio: adc: add support for pac1921")
> >>> Signed-off-by: Dan Carpenter <dan.carpenter-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >>> ---
> >>>    drivers/iio/adc/pac1921.c | 4 ++--
> >>>    1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c
> >>> index d04c6685d780..8200a47bdf21 100644
> >>> --- a/drivers/iio/adc/pac1921.c
> >>> +++ b/drivers/iio/adc/pac1921.c
> >>> @@ -1168,8 +1168,8 @@ static int pac1921_probe(struct i2c_client *client)
> >>>    
> >>>    	priv->regmap = devm_regmap_init_i2c(client, &pac1921_regmap_config);
> >>>    	if (IS_ERR(priv->regmap))
> >>> -		dev_err_probe(dev, (int)PTR_ERR(priv->regmap),
> >>> -			      "Cannot initialize register map\n");
> >>> +		return dev_err_probe(dev, (int)PTR_ERR(priv->regmap),
> >>
> >> The (int) is unusual.
> >>
> > The (int) explicit cast is to address Wconversion warnings since dev_err_probe
> > takes an int as argument.
> 
> Ok, but:
> 
> 1) With the cast removed, on my x86_64:
> 	$ make CFLAGS="-Wconversion" drivers/iio/adc/pac1921.o
> 
> doesn't generate any error.
> 
I can't reproduce the warning in that way either, but maybe CFLAGS gets
overridden in that case because with the following method I can see the
warning:

$ print "CFLAGS_pac1921.o := -Wconversion" >> drivers/iio/adc/Makefile
$ print "CONFIG_IIO=y\nCONFIG_PAC1921=y" >> arch/x86/configs/x86_64_defconfig
$ sed -i 's/CONFIG_WERROR=y/CONFIG_WERROR=n/g' arch/x86/configs/x86_64_defconfig
$ make x86_64_defconfig
$ make -j7

drivers/iio/adc/pac1921.c: In function ‘pac1921_probe’:
drivers/iio/adc/pac1921.c:1171:36: warning: conversion from ‘long int’ to ‘int’ may change value [-Wconversion]
 1171 |                 dev_err_probe(dev, PTR_ERR(priv->regmap),
      |                                    ^~~~~~~~~~~~~~~~~~~~~

Built with gcc version: gcc version 14.1.1 20240522 (GCC)

Same thing building for aarch64 with gcc version 12.2.0 (Debian 12.2.0-14)

> 2)
> 	$ it grep dev_err_probe.*\)PTR_ERR | wc -l
> 	2
> 
> 	$ it grep dev_err_probe.*PTR_ERR | wc -l
> 	1948
> So, should the cast be needed, maybe another fix could make sense?
>
It could be assigned to the ret value if that would be preferred:
	if (IS_ERR(priv->regmap)) {
		ret = (int)PTR_ERR(priv->regmap);
		return dev_err_probe(dev, ret, "Cannot initialize register map\n");
	}

Otherwise a more generic approach could be to let PTR_ERR directly cast to
(int). I would say that if it is always called after an IS_ERR() it should be
safe to cast to (int) since the latter should guarantee the pointer value is
inside int size boundaries. The similar PTR_ERR_OR_ZERO also casts (implicitly)
to int but it also checks for IS_ERR before the cast.
Maybe another solution could be introducing a new macro that does the cast but
before it checks the ptr with IS_ERR(), I came up with the following even
though it doesn't look very idiomatic:

#define WITH_PTR_ERR(ret, ptr) if (IS_ERR(ptr) && (ret = (int)PTR_ERR(ptr)))
...
static int pac1921_probe(struct i2c_client *client)
{
        ...
	WITH_PTR_ERR(ret, priv->regmap) {
		return dev_err_probe(dev, ret, "Cannot initialize register map\n");
	}
}

Maybe there is already some similar use case?

Anyway, if in general it is preferred to avoid the explicit cast despite the
Wconversion warning I would be fine with it.

> CJ
> 

Thanks,
Matteo Martelli

Re: [PATCH] iio: adc: pac1921: add missing error return in probe()

[Matteo Martelli]

Matteo Martelli wrote:
> Christophe JAILLET wrote:
> > Le 09/08/2024 à 09:31, Matteo Martelli a écrit :
> > > Christophe JAILLET wrote:
> > >> Le 08/08/2024 à 21:28, Dan Carpenter a écrit :
> > >>> This error path was intended to return, and not just print an error.  The
> > >>> current code will lead to an error pointer dereference.
> > >>>
> > >>> Fixes: 371f778b83cd ("iio: adc: add support for pac1921")
> > >>> Signed-off-by: Dan Carpenter <dan.carpenter-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > >>> ---
> > >>>    drivers/iio/adc/pac1921.c | 4 ++--
> > >>>    1 file changed, 2 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c
> > >>> index d04c6685d780..8200a47bdf21 100644
> > >>> --- a/drivers/iio/adc/pac1921.c
> > >>> +++ b/drivers/iio/adc/pac1921.c
> > >>> @@ -1168,8 +1168,8 @@ static int pac1921_probe(struct i2c_client *client)
> > >>>    
> > >>>    	priv->regmap = devm_regmap_init_i2c(client, &pac1921_regmap_config);
> > >>>    	if (IS_ERR(priv->regmap))
> > >>> -		dev_err_probe(dev, (int)PTR_ERR(priv->regmap),
> > >>> -			      "Cannot initialize register map\n");
> > >>> +		return dev_err_probe(dev, (int)PTR_ERR(priv->regmap),
> > >>
> > >> The (int) is unusual.
> > >>
> > > The (int) explicit cast is to address Wconversion warnings since dev_err_probe
> > > takes an int as argument.
> > 
> > Ok, but:
> > 
> > 1) With the cast removed, on my x86_64:
> > 	$ make CFLAGS="-Wconversion" drivers/iio/adc/pac1921.o
> > 
> > doesn't generate any error.
> > 
> I can't reproduce the warning in that way either, but maybe CFLAGS gets
> overridden in that case because with the following method I can see the
> warning:
> 
> $ print "CFLAGS_pac1921.o := -Wconversion" >> drivers/iio/adc/Makefile
> $ print "CONFIG_IIO=y\nCONFIG_PAC1921=y" >> arch/x86/configs/x86_64_defconfig
> $ sed -i 's/CONFIG_WERROR=y/CONFIG_WERROR=n/g' arch/x86/configs/x86_64_defconfig
> $ make x86_64_defconfig
> $ make -j7
> 
> drivers/iio/adc/pac1921.c: In function ‘pac1921_probe’:
> drivers/iio/adc/pac1921.c:1171:36: warning: conversion from ‘long int’ to ‘int’ may change value [-Wconversion]
>  1171 |                 dev_err_probe(dev, PTR_ERR(priv->regmap),
>       |                                    ^~~~~~~~~~~~~~~~~~~~~
> 
> Built with gcc version: gcc version 14.1.1 20240522 (GCC)
> 
> Same thing building for aarch64 with gcc version 12.2.0 (Debian 12.2.0-14)
> 
> > 2)
> > 	$ it grep dev_err_probe.*\)PTR_ERR | wc -l
> > 	2
> > 
> > 	$ it grep dev_err_probe.*PTR_ERR | wc -l
> > 	1948
> > So, should the cast be needed, maybe another fix could make sense?
> >
> It could be assigned to the ret value if that would be preferred:
> 	if (IS_ERR(priv->regmap)) {
> 		ret = (int)PTR_ERR(priv->regmap);
> 		return dev_err_probe(dev, ret, "Cannot initialize register map\n");
> 	}
>
> Otherwise a more generic approach could be to let PTR_ERR directly cast to
> (int). I would say that if it is always called after an IS_ERR() it should be
> safe to cast to (int) since the latter should guarantee the pointer value is
> inside int size boundaries. The similar PTR_ERR_OR_ZERO also casts (implicitly)
> to int but it also checks for IS_ERR before the cast.
> Maybe another solution could be introducing a new macro that does the cast but
> before it checks the ptr with IS_ERR(), I came up with the following even
> though it doesn't look very idiomatic:
> 
> #define WITH_PTR_ERR(ret, ptr) if (IS_ERR(ptr) && (ret = (int)PTR_ERR(ptr)))
> ...
> static int pac1921_probe(struct i2c_client *client)
> {
>         ...
> 	WITH_PTR_ERR(ret, priv->regmap) {
> 		return dev_err_probe(dev, ret, "Cannot initialize register map\n");
> 	}
> }
> 
> Maybe there is already some similar use case?
> 
> Anyway, if in general it is preferred to avoid the explicit cast despite the
> Wconversion warning I would be fine with it.
>

Adding another simple alternative I didn't think of before:
        ...
	ret = PTR_ERR_OR_ZERO(priv->regmap);
	if (ret)
		return dev_err_probe(dev, ret, "Cannot initialize register map\n");

The warning would still be produced due to the implicit cast inside
PTR_ERR_OR_ZERO but it could be fixed for all users with an explicit cast if
there will be interest in future to do so.

Also used a bit around:
grep -R -A 3 'ret = PTR_ERR_OR_ZERO' . | grep -e '_err.*(' | wc -l
69

Thanks,
Matteo Martelli

Re: [PATCH] iio: adc: pac1921: add missing error return in probe()

[Dan Carpenter]

Just delete the -Wconversion?  I'm not really familiar that option but it sounds
an even worse version of -Wsign-compare.

https://staticthinking.wordpress.com/2023/07/25/wsign-compare-is-garbage/

regards,
dan carpenter

Re: [PATCH] iio: adc: pac1921: add missing error return in probe()

[Dan Carpenter]

On Fri, Aug 09, 2024 at 09:31:43AM +0200, Matteo Martelli wrote:
> Christophe JAILLET wrote:
> > Le 08/08/2024 à 21:28, Dan Carpenter a écrit :
> > > This error path was intended to return, and not just print an error.  The
> > > current code will lead to an error pointer dereference.
> > > 
> > > Fixes: 371f778b83cd ("iio: adc: add support for pac1921")
> > > Signed-off-by: Dan Carpenter <dan.carpenter-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > > ---
> > >   drivers/iio/adc/pac1921.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c
> > > index d04c6685d780..8200a47bdf21 100644
> > > --- a/drivers/iio/adc/pac1921.c
> > > +++ b/drivers/iio/adc/pac1921.c
> > > @@ -1168,8 +1168,8 @@ static int pac1921_probe(struct i2c_client *client)
> > >   
> > >   	priv->regmap = devm_regmap_init_i2c(client, &pac1921_regmap_config);
> > >   	if (IS_ERR(priv->regmap))
> > > -		dev_err_probe(dev, (int)PTR_ERR(priv->regmap),
> > > -			      "Cannot initialize register map\n");
> > > +		return dev_err_probe(dev, (int)PTR_ERR(priv->regmap),
> > 
> > The (int) is unusual.
> >
> The (int) explicit cast is to address Wconversion warnings since dev_err_probe
> takes an int as argument.

I don't want to remove the int because it's unrelated, but Christophe is right
that the int is unusual.  We really would want to discourage that.

regards,
dan carpenter

Re: [PATCH] iio: adc: pac1921: add missing error return in probe()

[Jonathan Cameron]

On Fri, 9 Aug 2024 18:18:13 +0300
Dan Carpenter <dan.carpenter@linaro.org> wrote:

> On Fri, Aug 09, 2024 at 09:31:43AM +0200, Matteo Martelli wrote:
> > Christophe JAILLET wrote:  
> > > Le 08/08/2024 à 21:28, Dan Carpenter a écrit :  
> > > > This error path was intended to return, and not just print an error.  The
> > > > current code will lead to an error pointer dereference.
> > > > 
> > > > Fixes: 371f778b83cd ("iio: adc: add support for pac1921")
> > > > Signed-off-by: Dan Carpenter <dan.carpenter-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > > > ---
> > > >   drivers/iio/adc/pac1921.c | 4 ++--
> > > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c
> > > > index d04c6685d780..8200a47bdf21 100644
> > > > --- a/drivers/iio/adc/pac1921.c
> > > > +++ b/drivers/iio/adc/pac1921.c
> > > > @@ -1168,8 +1168,8 @@ static int pac1921_probe(struct i2c_client *client)
> > > >   
> > > >   	priv->regmap = devm_regmap_init_i2c(client, &pac1921_regmap_config);
> > > >   	if (IS_ERR(priv->regmap))
> > > > -		dev_err_probe(dev, (int)PTR_ERR(priv->regmap),
> > > > -			      "Cannot initialize register map\n");
> > > > +		return dev_err_probe(dev, (int)PTR_ERR(priv->regmap),  
> > > 
> > > The (int) is unusual.
> > >  
> > The (int) explicit cast is to address Wconversion warnings since dev_err_probe
> > takes an int as argument.  
> 
> I don't want to remove the int because it's unrelated, but Christophe is right
> that the int is unusual.  We really would want to discourage that.

Applied, but I'd ideally like a follow up patch removing the int and the
couple of similar instances from this driver.  Anyone want to spin one?

Thanks,

Jonathan

> 
> regards,
> dan carpenter
> 

Re: [PATCH] iio: adc: pac1921: add missing error return in probe()

[Matteo Martelli]

Quoting Jonathan Cameron (2024-08-10 12:35:18)
> On Fri, 9 Aug 2024 18:18:13 +0300
> Dan Carpenter <dan.carpenter@linaro.org> wrote:
> 
> > On Fri, Aug 09, 2024 at 09:31:43AM +0200, Matteo Martelli wrote:
> > > Christophe JAILLET wrote:  
> > > > Le 08/08/2024 à 21:28, Dan Carpenter a écrit :  
> > > > > This error path was intended to return, and not just print an error.  The
> > > > > current code will lead to an error pointer dereference.
> > > > > 
> > > > > Fixes: 371f778b83cd ("iio: adc: add support for pac1921")
> > > > > Signed-off-by: Dan Carpenter <dan.carpenter-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > > > > ---
> > > > >   drivers/iio/adc/pac1921.c | 4 ++--
> > > > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c
> > > > > index d04c6685d780..8200a47bdf21 100644
> > > > > --- a/drivers/iio/adc/pac1921.c
> > > > > +++ b/drivers/iio/adc/pac1921.c
> > > > > @@ -1168,8 +1168,8 @@ static int pac1921_probe(struct i2c_client *client)
> > > > >   
> > > > >         priv->regmap = devm_regmap_init_i2c(client, &pac1921_regmap_config);
> > > > >         if (IS_ERR(priv->regmap))
> > > > > -               dev_err_probe(dev, (int)PTR_ERR(priv->regmap),
> > > > > -                             "Cannot initialize register map\n");
> > > > > +               return dev_err_probe(dev, (int)PTR_ERR(priv->regmap),  
> > > > 
> > > > The (int) is unusual.
> > > >  
> > > The (int) explicit cast is to address Wconversion warnings since dev_err_probe
> > > takes an int as argument.  
> > 
> > I don't want to remove the int because it's unrelated, but Christophe is right
> > that the int is unusual.  We really would want to discourage that.
> 
> Applied, but I'd ideally like a follow up patch removing the int and the
> couple of similar instances from this driver.  Anyone want to spin one?
> 

I can spin the patch, but at this point I am wondering whether I should remove all
the unnecessary explicit casts that I put to address Wconversion
and Wsign-compare warnings. If that's the general approach to help readability I
am totally fine with it.

> Thanks,
> 
> Jonathan
> 

Thanks,
Matteo Martelli

Re: [PATCH] iio: adc: pac1921: add missing error return in probe()

[Jonathan Cameron]

On Wed, 11 Sep 2024 11:32:04 +0200
Matteo Martelli <matteomartelli3@gmail.com> wrote:

> Quoting Jonathan Cameron (2024-08-10 12:35:18)
> > On Fri, 9 Aug 2024 18:18:13 +0300
> > Dan Carpenter <dan.carpenter@linaro.org> wrote:
> >   
> > > On Fri, Aug 09, 2024 at 09:31:43AM +0200, Matteo Martelli wrote:  
> > > > Christophe JAILLET wrote:    
> > > > > Le 08/08/2024 à 21:28, Dan Carpenter a écrit :    
> > > > > > This error path was intended to return, and not just print an error.  The
> > > > > > current code will lead to an error pointer dereference.
> > > > > > 
> > > > > > Fixes: 371f778b83cd ("iio: adc: add support for pac1921")
> > > > > > Signed-off-by: Dan Carpenter <dan.carpenter-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > > > > > ---
> > > > > >   drivers/iio/adc/pac1921.c | 4 ++--
> > > > > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c
> > > > > > index d04c6685d780..8200a47bdf21 100644
> > > > > > --- a/drivers/iio/adc/pac1921.c
> > > > > > +++ b/drivers/iio/adc/pac1921.c
> > > > > > @@ -1168,8 +1168,8 @@ static int pac1921_probe(struct i2c_client *client)
> > > > > >   
> > > > > >         priv->regmap = devm_regmap_init_i2c(client, &pac1921_regmap_config);
> > > > > >         if (IS_ERR(priv->regmap))
> > > > > > -               dev_err_probe(dev, (int)PTR_ERR(priv->regmap),
> > > > > > -                             "Cannot initialize register map\n");
> > > > > > +               return dev_err_probe(dev, (int)PTR_ERR(priv->regmap),    
> > > > > 
> > > > > The (int) is unusual.
> > > > >    
> > > > The (int) explicit cast is to address Wconversion warnings since dev_err_probe
> > > > takes an int as argument.    
> > > 
> > > I don't want to remove the int because it's unrelated, but Christophe is right
> > > that the int is unusual.  We really would want to discourage that.  
> > 
> > Applied, but I'd ideally like a follow up patch removing the int and the
> > couple of similar instances from this driver.  Anyone want to spin one?
> >   
> 
> I can spin the patch, but at this point I am wondering whether I should remove all
> the unnecessary explicit casts that I put to address Wconversion
> and Wsign-compare warnings. If that's the general approach to help readability I
> am totally fine with it.

I think it is probably sensible to do so as mostly we know the value ranges
etc so they don't matter.

Jonathan

> 
> > Thanks,
> > 
> > Jonathan
> >   
> 
> Thanks,
> Matteo Martelli

Re: [PATCH] iio: adc: pac1921: add missing error return in probe()

[Matteo Martelli]

Dan Carpenter wrote:
> This error path was intended to return, and not just print an error.  The
> current code will lead to an error pointer dereference.
> 
> Fixes: 371f778b83cd ("iio: adc: add support for pac1921")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>  drivers/iio/adc/pac1921.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c
> index d04c6685d780..8200a47bdf21 100644
> --- a/drivers/iio/adc/pac1921.c
> +++ b/drivers/iio/adc/pac1921.c
> @@ -1168,8 +1168,8 @@ static int pac1921_probe(struct i2c_client *client)
>  
>  	priv->regmap = devm_regmap_init_i2c(client, &pac1921_regmap_config);
>  	if (IS_ERR(priv->regmap))
> -		dev_err_probe(dev, (int)PTR_ERR(priv->regmap),
> -			      "Cannot initialize register map\n");
> +		return dev_err_probe(dev, (int)PTR_ERR(priv->regmap),
> +				     "Cannot initialize register map\n");
>  
>  	devm_mutex_init(dev, &priv->lock);
>  
> -- 
> 2.43.0
> 

The intent was indeed to return the error. Thanks for catching it.
Acked-by: Matteo Martelli <matteomartelli3@gmail.com>

Thanks,
Matteo Martelli