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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 <dan.carpenter@xxxxxxxxxx>
> > > Signed-off-by: Phil Edworthy <phil.edworthy@xxxxxxxxxxx>
> > > ---
> > > 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
> > >

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux