[PATCH] pinctrl: rzn1: Fix check for used MDIO bus

[PATCH] pinctrl: rzn1: Fix check for used MDIO bus

[Phil Edworthy]

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>
---
v3:
 - Split fixing the return var when calling of_get_child_count() into
   a separate patch.
v2:
 - Don't rely on rely on the implicit typecast from -1 to uint
---
 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 57886dcff53d..a04235e3bec4 100644
--- a/drivers/pinctrl/pinctrl-rzn1.c
+++ b/drivers/pinctrl/pinctrl-rzn1.c
@@ -112,7 +112,7 @@ struct rzn1_pinctrl {
 	struct rzn1_pinctrl_regs __iomem *lev2;
 	u32 lev1_protect_phys;
 	u32 lev2_protect_phys;
-	u32 mdio_func[2];
+	int mdio_func[2];
 
 	struct rzn1_pin_group *groups;
 	unsigned int ngroups;
-- 
2.17.1

[PATCH] pinctrl: rzn1: Fix of_get_child_count() error check

[Phil Edworthy]

If we assign the result of of_get_child_count() to an unsigned int,
the code will not detect any errors. Therefore assign it to an int
instead.

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 a04235e3bec4..cc0e5aa9128a 100644
--- a/drivers/pinctrl/pinctrl-rzn1.c
+++ b/drivers/pinctrl/pinctrl-rzn1.c
@@ -810,8 +810,8 @@ static int rzn1_pinctrl_probe_dt(struct platform_device *pdev,
 	struct device_node *np = pdev->dev.of_node;
 	struct device_node *child;
 	unsigned int maxgroups = 0;
-	unsigned int nfuncs = 0;
 	unsigned int i = 0;
+	int nfuncs = 0;
 	int ret;
 
 	nfuncs = of_get_child_count(np);
-- 
2.17.1

Re: [PATCH] pinctrl: rzn1: Fix of_get_child_count() error check

[Geert Uytterhoeven]

On Fri, Nov 23, 2018 at 11:54 AM Phil Edworthy
<phil.edworthy@renesas.com> wrote:
> If we assign the result of of_get_child_count() to an unsigned int,
> the code will not detect any errors. Therefore assign it to an int
> instead.

Note that currently of_get_child_count() never returns a negative
error code. But it does return int, not unsigned int.

> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in sh-pfc-for-v4.21.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] pinctrl: rzn1: Fix check for used MDIO bus

[Geert Uytterhoeven]

On Fri, Nov 23, 2018 at 11:54 AM Phil Edworthy
<phil.edworthy@renesas.com> 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>
> ---
> v3:
>  - Split fixing the return var when calling of_get_child_count() into
>    a separate patch.
> v2:
>  - Don't rely on rely on the implicit typecast from -1 to uint

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in sh-pfc-for-v4.21.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH] pinctrl: rzn1: Fix check for used MDIO bus

[Phil Edworthy]

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)'

It also fixes the return var when calling of_get_child_count()

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
---
v2:
 - Don't rely on rely on the implicit typecast from -1 to uint
---
 drivers/pinctrl/pinctrl-rzn1.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-rzn1.c b/drivers/pinctrl/pinctrl-rzn1.c
index 57886dcff53d..cc0e5aa9128a 100644
--- a/drivers/pinctrl/pinctrl-rzn1.c
+++ b/drivers/pinctrl/pinctrl-rzn1.c
@@ -112,7 +112,7 @@ struct rzn1_pinctrl {
 	struct rzn1_pinctrl_regs __iomem *lev2;
 	u32 lev1_protect_phys;
 	u32 lev2_protect_phys;
-	u32 mdio_func[2];
+	int mdio_func[2];
 
 	struct rzn1_pin_group *groups;
 	unsigned int ngroups;
@@ -810,8 +810,8 @@ static int rzn1_pinctrl_probe_dt(struct platform_device *pdev,
 	struct device_node *np = pdev->dev.of_node;
 	struct device_node *child;
 	unsigned int maxgroups = 0;
-	unsigned int nfuncs = 0;
 	unsigned int i = 0;
+	int nfuncs = 0;
 	int ret;
 
 	nfuncs = of_get_child_count(np);
-- 
2.17.1

Re: [PATCH] pinctrl: rzn1: Fix check for used MDIO bus

[Simon Horman]

On Mon, Nov 19, 2018 at 04:18:38PM +0000, 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)'
> 
> It also fixes the return var when calling of_get_child_count()
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> ---
> v2:
>  - Don't rely on rely on the implicit typecast from -1 to uint
> ---
>  drivers/pinctrl/pinctrl-rzn1.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-rzn1.c b/drivers/pinctrl/pinctrl-rzn1.c
> index 57886dcff53d..cc0e5aa9128a 100644
> --- a/drivers/pinctrl/pinctrl-rzn1.c
> +++ b/drivers/pinctrl/pinctrl-rzn1.c
> @@ -112,7 +112,7 @@ struct rzn1_pinctrl {
>  	struct rzn1_pinctrl_regs __iomem *lev2;
>  	u32 lev1_protect_phys;
>  	u32 lev2_protect_phys;
> -	u32 mdio_func[2];
> +	int mdio_func[2];

Hi Phil,

rzn1_pinctrl_mdio_select() assigns values of type u32 to elements
of mdio_func. Perhaps that warrants cleaning up too?

>  
>  	struct rzn1_pin_group *groups;
>  	unsigned int ngroups;
> @@ -810,8 +810,8 @@ static int rzn1_pinctrl_probe_dt(struct platform_device *pdev,
>  	struct device_node *np = pdev->dev.of_node;
>  	struct device_node *child;
>  	unsigned int maxgroups = 0;
> -	unsigned int nfuncs = 0;
>  	unsigned int i = 0;
> +	int nfuncs = 0;
>  	int ret;
>  
>  	nfuncs = of_get_child_count(np);
> -- 
> 2.17.1
> 

RE: [PATCH] pinctrl: rzn1: Fix check for used MDIO bus

[Phil Edworthy]

Hi Simon,

On 22 November 2018 14:09 Simon Horman wrote:
> On Mon, Nov 19, 2018 at 04:18:38PM +0000, 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)'
> >
> > It also fixes the return var when calling of_get_child_count()
> >
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> > ---
> > v2:
> >  - Don't rely on rely on the implicit typecast from -1 to uint
> > ---
> >  drivers/pinctrl/pinctrl-rzn1.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pinctrl/pinctrl-rzn1.c
> > b/drivers/pinctrl/pinctrl-rzn1.c index 57886dcff53d..cc0e5aa9128a
> > 100644
> > --- a/drivers/pinctrl/pinctrl-rzn1.c
> > +++ b/drivers/pinctrl/pinctrl-rzn1.c
> > @@ -112,7 +112,7 @@ struct rzn1_pinctrl {
> >  	struct rzn1_pinctrl_regs __iomem *lev2;
> >  	u32 lev1_protect_phys;
> >  	u32 lev2_protect_phys;
> > -	u32 mdio_func[2];
> > +	int mdio_func[2];
> 
> Hi Phil,
> 
> rzn1_pinctrl_mdio_select() assigns values of type u32 to elements of
> mdio_func. Perhaps that warrants cleaning up too?
The source of the 'u32 func' arg is ultimately u32 values read from DT.
The code already ensures that these cannot be bigger than 7, so is fine
I think.

Thanks
Phil


> >  	struct rzn1_pin_group *groups;
> >  	unsigned int ngroups;
> > @@ -810,8 +810,8 @@ static int rzn1_pinctrl_probe_dt(struct
> platform_device *pdev,
> >  	struct device_node *np = pdev->dev.of_node;
> >  	struct device_node *child;
> >  	unsigned int maxgroups = 0;
> > -	unsigned int nfuncs = 0;
> >  	unsigned int i = 0;
> > +	int nfuncs = 0;
> >  	int ret;
> >
> >  	nfuncs = of_get_child_count(np);
> > --
> > 2.17.1
> >

Re: [PATCH] pinctrl: rzn1: Fix check for used MDIO bus

[Geert Uytterhoeven]

Hi Phil,

Thanks for your patch!

On Mon, Nov 19, 2018 at 5:18 PM Phil Edworthy <phil.edworthy@renesas.com> 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)'
>
> It also fixes the return var when calling of_get_child_count()

I think this should be a separate patch.

> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

BTW, I have a question about rzn1_pinctrl_mdio_select():

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)
                dev_warn(ipctl->dev, "conflicting setting for mdio%d!\n", mdio);
        ipctl->mdio_func[mdio] = func;

        dev_dbg(ipctl->dev, "setting mdio%d to %u\n", mdio, func);

        writel(func, &ipctl->lev2->l2_mdio[mdio]);
}

The check warns the user if it overrides an already initialized MDIO function
with a different value.
However, there is no method to uninitialize (reset to -1) mdio_func[], to
avoid getting the warning.

For a use case, I was thinking about a DT overlay that would cause the
MDIO function to be initialized on loading, and needs to uninitialize the
MDIO function on removing.

Perhaps that is very unlikely or even impossible, given the function of the
pins controlled by the MDIO function?

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH] pinctrl: rzn1: Fix check for used MDIO bus

[Phil Edworthy]

Hi Geert,

On 23 November 2018 09:41 Geert Uytterhoeven wrote:
> Subject: Re: [PATCH] pinctrl: rzn1: Fix check for used MDIO bus
> On Mon, Nov 19, 2018 at 5:18 PM 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)'
> >
> > It also fixes the return var when calling of_get_child_count()
> 
> I think this should be a separate patch.
Ok, I'll split them.


> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> BTW, I have a question about rzn1_pinctrl_mdio_select():
> 
> 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)
>                 dev_warn(ipctl->dev, "conflicting setting for mdio%d!\n", mdio);
>         ipctl->mdio_func[mdio] = func;
> 
>         dev_dbg(ipctl->dev, "setting mdio%d to %u\n", mdio, func);
> 
>         writel(func, &ipctl->lev2->l2_mdio[mdio]); }
> 
> The check warns the user if it overrides an already initialized MDIO function
> with a different value.
> However, there is no method to uninitialize (reset to -1) mdio_func[], to
> avoid getting the warning.
> 
> For a use case, I was thinking about a DT overlay that would cause the MDIO
> function to be initialized on loading, and needs to uninitialize the MDIO
> function on removing.
> 
> Perhaps that is very unlikely or even impossible, given the function of the
> pins controlled by the MDIO function?
I hadn't considered that DT overlay possibility...
Since this MDIO muxing selects one of several different IP blocks as the
MDIO master, I guess it could happen. However, this is pretty unlikely!

I can't see any way via the pinctrl_ops or pinconf_ops to 'undo' a pin
setting, how would this work?
If a DT overlay causes remove() then probe() to be called again, the driver
resets mdio_func[] in probe(), so it'll work.

Thanks!
Phil

Re: [PATCH] pinctrl: rzn1: Fix check for used MDIO bus

[Geert Uytterhoeven]

Hi Phil,

On Fri, Nov 23, 2018 at 11:07 AM Phil Edworthy
<phil.edworthy@renesas.com> wrote:
> On 23 November 2018 09:41 Geert Uytterhoeven wrote:
> > Subject: Re: [PATCH] pinctrl: rzn1: Fix check for used MDIO bus
> > On Mon, Nov 19, 2018 at 5:18 PM 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)'
> > >
> > > It also fixes the return var when calling of_get_child_count()
> >
> > I think this should be a separate patch.
> Ok, I'll split them.

Thanks!

> > BTW, I have a question about rzn1_pinctrl_mdio_select():
> >
> > 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)
> >                 dev_warn(ipctl->dev, "conflicting setting for mdio%d!\n", mdio);
> >         ipctl->mdio_func[mdio] = func;
> >
> >         dev_dbg(ipctl->dev, "setting mdio%d to %u\n", mdio, func);
> >
> >         writel(func, &ipctl->lev2->l2_mdio[mdio]); }
> >
> > The check warns the user if it overrides an already initialized MDIO function
> > with a different value.
> > However, there is no method to uninitialize (reset to -1) mdio_func[], to
> > avoid getting the warning.
> >
> > For a use case, I was thinking about a DT overlay that would cause the MDIO
> > function to be initialized on loading, and needs to uninitialize the MDIO
> > function on removing.
> >
> > Perhaps that is very unlikely or even impossible, given the function of the
> > pins controlled by the MDIO function?
> I hadn't considered that DT overlay possibility...
> Since this MDIO muxing selects one of several different IP blocks as the
> MDIO master, I guess it could happen. However, this is pretty unlikely!
>
> I can't see any way via the pinctrl_ops or pinconf_ops to 'undo' a pin
> setting, how would this work?

Actually the pinctrl core wouldn't undo the configuration on DT overlay unload,
but would just do the new configuration when loading the new DT overlay.
Hence that would print the warning, but work regardless.
Only for GPIOs there would be an undo step (freeing the requested GPIO).

> If a DT overlay causes remove() then probe() to be called again, the driver
> resets mdio_func[] in probe(), so it'll work.

Typically a DT overlay would only control I/O devices, not the actual pinctrl
device, so the pin controller driver's .probe() wouldn't be called.

But I agree it's unlikely and rare, and would still work. And probably you do
want to keep the warning.  DT overlays are still experimental, as there's no
upstream support for loading a random DT overlay at runtime.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH] pinctrl: rzn1: Fix check for used MDIO bus

[Phil Edworthy]

Hi Geert,

On 23 November 2018 10:16 Geert Uytterhoeven wrote:
> On Fri, Nov 23, 2018 at 11:07 AM Phil Edworthy wrote:
> > On 23 November 2018 09:41 Geert Uytterhoeven wrote:
> > > Subject: Re: [PATCH] pinctrl: rzn1: Fix check for used MDIO bus On
> > > Mon, Nov 19, 2018 at 5:18 PM 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)'
> > > >
> > > > It also fixes the return var when calling of_get_child_count()
> > >
> > > I think this should be a separate patch.
> > Ok, I'll split them.
> 
> Thanks!
> 
> > > BTW, I have a question about rzn1_pinctrl_mdio_select():
> > >
> > > 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)
> > >                 dev_warn(ipctl->dev, "conflicting setting for mdio%d!\n", mdio);
> > >         ipctl->mdio_func[mdio] = func;
> > >
> > >         dev_dbg(ipctl->dev, "setting mdio%d to %u\n", mdio, func);
> > >
> > >         writel(func, &ipctl->lev2->l2_mdio[mdio]); }
> > >
> > > The check warns the user if it overrides an already initialized MDIO
> > > function with a different value.
> > > However, there is no method to uninitialize (reset to -1)
> > > mdio_func[], to avoid getting the warning.
> > >
> > > For a use case, I was thinking about a DT overlay that would cause
> > > the MDIO function to be initialized on loading, and needs to
> > > uninitialize the MDIO function on removing.
> > >
> > > Perhaps that is very unlikely or even impossible, given the function
> > > of the pins controlled by the MDIO function?
> > I hadn't considered that DT overlay possibility...
> > Since this MDIO muxing selects one of several different IP blocks as
> > the MDIO master, I guess it could happen. However, this is pretty unlikely!
> >
> > I can't see any way via the pinctrl_ops or pinconf_ops to 'undo' a pin
> > setting, how would this work?
> 
> Actually the pinctrl core wouldn't undo the configuration on DT overlay
> unload, but would just do the new configuration when loading the new DT
> overlay.
> Hence that would print the warning, but work regardless.
> Only for GPIOs there would be an undo step (freeing the requested GPIO).
> 
> > If a DT overlay causes remove() then probe() to be called again, the
> > driver resets mdio_func[] in probe(), so it'll work.
> 
> Typically a DT overlay would only control I/O devices, not the actual pinctrl
> device, so the pin controller driver's .probe() wouldn't be called.
> 
> But I agree it's unlikely and rare, and would still work. And probably you do
> want to keep the warning.  DT overlays are still experimental, as there's no
> upstream support for loading a random DT overlay at runtime.

Ok, I'll leave it as it is then. If it ever becomes an issue, we can look again.

Thanks
Phil

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds