Hi Jacopo,
On 15 October 2018 16:12 jacopo mondi wrote:
> On Mon, Oct 15, 2018 at 04:01:47PM +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)'
> >
> > It also fixes the return var when calling of_get_child_count()
> >
>
> Not really, since you skip the assignement if return value is <= 0:
>
> nfuncs = of_get_child_count(np);
> if (nfuncs <= 0)
> return 0;
>
> ipctl->nfunctions = nfuncs;
>
> This seems more likely to be here to make 'rzn1_pmx_get_funcs_count()'
> happy, as it returns a signed integer, but since nfunctions is only assigned
> if
> 'nfuncs' > 0, then the cast is safe there.
>
> I would keep this unsigned, and rather return an error if
> 'of_get_child_count()' returns an error of any sort in 'probe_dt()', otherwise
> assign 'ipctl->nfunctions = nfuncs;' unconditionally and return immediately if
> nfuncs == 0. This makes sure nfunctions is initialized even if there are no
> functions registered.
Ok, I get what you're saying, though nfunctions will always be initialised due
to allocation with devm_kzalloc.
> Anyway, small issues, this just doesn't belong to this patch, but it's likely
> not
> to cause any harm I guess.
True... should I leave this patch as is or respin again?
Thanks
Phil
> > Reported-by: Dan Carpenter <dan.carpen...@oracle.com>
> > Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
> > ---
> > v2:
> > - Don't use implicit type conversion.
> > - Fix type of return var when calling of_get_child_count().
> > ---
> > drivers/pinctrl/pinctrl-rzn1.c | 10 +++++-----
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/pinctrl/pinctrl-rzn1.c
> > b/drivers/pinctrl/pinctrl-rzn1.c index ce05e3a00be2..4998463c54a0
> > 100644
> > --- a/drivers/pinctrl/pinctrl-rzn1.c
> > +++ b/drivers/pinctrl/pinctrl-rzn1.c
> > @@ -112,13 +112,13 @@ 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;
> >
> > struct rzn1_pmx_func *functions;
> > - unsigned int nfunctions;
> > + int nfunctions;
> > };
> >
> > #define RZN1_PINS_PROP "pinmux"
> > @@ -195,9 +195,9 @@ 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] >= 0 && ipctl->mdio_func[mdio] !=
> > +(int)func)
> > dev_warn(ipctl->dev, "conflicting setting for mdio%d!\n",
> mdio);
> > - ipctl->mdio_func[mdio] = func;
> > + ipctl->mdio_func[mdio] = (int)func;
> >
> > dev_dbg(ipctl->dev, "setting mdio%d to %u\n", mdio, func);
> >
> > @@ -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
> >
