pinctrl: rzn1: Fix check for used MDIO bus

This fixes the check for unused mdio bus setting and the following static
checker warning:
 drivers/pinctrl/pinctrl-rzn1.c:198 rzn1_pinctrl_mdio_select()
 warn: always true condition '(ipctl->mdio_func[mdio] >= 0) => (0-u32max >= 0)'

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
---
 drivers/pinctrl/pinctrl-rzn1.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Hi Phil,

On Fri, Oct 12, 2018 at 11:40:36AM +0100, Phil Edworthy wrote:
> This fixes the check for unused mdio bus setting and the following static
> checker warning:
>  drivers/pinctrl/pinctrl-rzn1.c:198 rzn1_pinctrl_mdio_select()
>  warn: always true condition '(ipctl->mdio_func[mdio] >= 0) => (0-u32max >= 0)'
>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> ---
>  drivers/pinctrl/pinctrl-rzn1.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pinctrl/pinctrl-rzn1.c b/drivers/pinctrl/pinctrl-rzn1.c
> index ce05e3a00be2..f688f3a29dfd 100644
> --- a/drivers/pinctrl/pinctrl-rzn1.c
> +++ b/drivers/pinctrl/pinctrl-rzn1.c
> @@ -195,7 +195,7 @@ static void rzn1_hw_set_lock(struct rzn1_pinctrl *ipctl, u8 lock, u8 value)
>  static void rzn1_pinctrl_mdio_select(struct rzn1_pinctrl *ipctl, int mdio,
>  				     u32 func)
>  {
> -	if (ipctl->mdio_func[mdio] >= 0 && ipctl->mdio_func[mdio] != func)
> +	if (ipctl->mdio_func[mdio] != -1 && ipctl->mdio_func[mdio] != func)
>  		dev_warn(ipctl->dev, "conflicting setting for mdio%d!\n", mdio);
>  	ipctl->mdio_func[mdio] = func;
>

MY understanding here is that the static checker complains because you
are comparing a variable of unsigned type to negative values, and
indeed you are treating mdio_func as signed in the driver code.

mdio_func is defined as:

struct rzn1_pinctrl {
        ...
	u32 mdio_func[2];
        ...
};

Then in probe function mdio_func gets intialized as:

	ipctl->mdio_func[0] = -1;
	ipctl->mdio_func[1] = -1;

I think you could safely make mdio_func integers, or either define an
INVALID value ( > 0), intialize them it that value and check against it for
validity.

Thanks
   j


> --
> 2.17.1
>

HI Jacopo,

On 15 October 2018 08:54 jacopo mondi wrote:
> On Fri, Oct 12, 2018 at 11:40:36AM +0100, Phil Edworthy wrote:
> > This fixes the check for unused mdio bus setting and the following
> > static checker warning:
> >  drivers/pinctrl/pinctrl-rzn1.c:198 rzn1_pinctrl_mdio_select()
> >  warn: always true condition '(ipctl->mdio_func[mdio] >= 0) => (0-u32max
> >= 0)'
> >
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> > ---
> >  drivers/pinctrl/pinctrl-rzn1.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/pinctrl/pinctrl-rzn1.c
> > b/drivers/pinctrl/pinctrl-rzn1.c index ce05e3a00be2..f688f3a29dfd
> > 100644
> > --- a/drivers/pinctrl/pinctrl-rzn1.c
> > +++ b/drivers/pinctrl/pinctrl-rzn1.c
> > @@ -195,7 +195,7 @@ static void rzn1_hw_set_lock(struct rzn1_pinctrl
> > *ipctl, u8 lock, u8 value)  static void rzn1_pinctrl_mdio_select(struct
> rzn1_pinctrl *ipctl, int mdio,
> >  				     u32 func)
> >  {
> > -	if (ipctl->mdio_func[mdio] >= 0 && ipctl->mdio_func[mdio] != func)
> > +	if (ipctl->mdio_func[mdio] != -1 && ipctl->mdio_func[mdio] != func)
> >  		dev_warn(ipctl->dev, "conflicting setting for mdio%d!\n",
> mdio);
> >  	ipctl->mdio_func[mdio] = func;
> >
> 
> MY understanding here is that the static checker complains because you are
> comparing a variable of unsigned type to negative values, and indeed you are
> treating mdio_func as signed in the driver code.
> 
> mdio_func is defined as:
> 
> struct rzn1_pinctrl {
>         ...
> 	u32 mdio_func[2];
>         ...
> };
> 
> Then in probe function mdio_func gets intialized as:
> 
> 	ipctl->mdio_func[0] = -1;
> 	ipctl->mdio_func[1] = -1;
> 
> I think you could safely make mdio_func integers, or either define an
> INVALID value ( > 0), intialize them it that value and check against it for
> validity.

You are right, I shouldn't rely on the implicit typecast from -1 to uint.
I'll send an updated patch to fix this.

Thanks
Phil