Hi Phil, On Tue, Oct 16, 2018 at 08:04:53AM +0000, Phil Edworthy wrote: > 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. >
Right, sorry for missing that. > > 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? I think you should handle the case where of_get_child_count() returns an error, but in a follow up patch. You can leave this one as it is, even if that change is imho not necessary. Thanks j > > Thanks > Phil > > > > Reported-by: Dan Carpenter <[email protected]> > > > Signed-off-by: Phil Edworthy <[email protected]> > > > --- > > > 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 > > >
signature.asc
Description: PGP signature
