RE: [PATCH 1/2] pinctrl: tegra: add suspend-resume support

[Bibek Basu]

> -----Original Message-----
> From: Thierry Reding [mailto:thierry.reding@xxxxxxxxxxxxxxxxx]
> Sent: Wednesday, April 24, 2013 12:14 AM
> To: Bibek Basu
> Cc: linus.walleij@xxxxxxxxxx; swarren@xxxxxxxxxxxxx; linux-
> tegra@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Pritesh Raithatha
> Subject: Re: [PATCH 1/2] pinctrl: tegra: add suspend-resume support
> 
> * PGP Signed by an unknown key
> 
> > diff --git a/drivers/pinctrl/pinctrl-tegra.c
> > b/drivers/pinctrl/pinctrl-tegra.c
> [...]
> > @@ -41,6 +42,8 @@ struct tegra_pmx {
> >
> >  	int nbanks;
> >  	void __iomem **regs;
> > +	int *regs_size;
> 
> Perhaps this should be unsigned int *. The values stored in this array will
> never be negative, right?
Agree. 
> 
> >  int tegra_pinctrl_probe(struct platform_device *pdev,
> >  			const struct tegra_pinctrl_soc_data *soc_data)  {
> > -	struct tegra_pmx *pmx;
> >  	struct resource *res;
> > -	int i;
> > +	struct tegra_pmx *pmx;
> > +	int i, pg_data_size = 0;
> 
> There's a needless move of the pmx variable declaration here.
Agree
> 
> > @@ -735,6 +769,21 @@ int tegra_pinctrl_probe(struct platform_device
> *pdev,
> >  		return -ENODEV;
> >  	}
> >
> > +if (IS_ENABLED(CONFIG_PM_SLEEP))
> > +	pmx->regs_size = devm_kzalloc(&pdev->dev,
> > +				pmx->nbanks * sizeof(*(pmx->regs_size)),
> > +				GFP_KERNEL);
> > +	if (!pmx->regs_size) {
> > +		dev_err(&pdev->dev, "Can't alloc regs pointer\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	pmx->pg_data = devm_kzalloc(&pdev->dev, pg_data_size,
> GFP_KERNEL);
> > +	if (!pmx->pg_data) {
> > +		dev_err(&pdev->dev, "Can't alloc pingroup data pointer\n");
> > +		return -ENODEV;
> > +	}
> 
> I don't think this works the way you expect it to. The line
> 
> 	if (IS_ENABLED(CONFIG_PM_SLEEP))
> 
> is a standard conditional and therefore needs to be properly indented and
> use { and } to delimit the block.
> 
My Bad. Will fix
> > @@ -756,6 +805,9 @@ int tegra_pinctrl_probe(struct platform_device
> *pdev,
> >  			dev_err(&pdev->dev, "Couldn't ioremap regs %d\n",
> i);
> >  			return -ENODEV;
> >  		}
> > +
> > +if (IS_ENABLED(CONFIG_PM_SLEEP))
> > +		pmx->regs_size[i] = resource_size(res);
> 
> In this case it will actually work as expected, but the if () should be properly
> indented.
> 
> Thierry
> 
Thanks for the review
Bibek
> * Unknown Key
> * 0x7F3EB3A1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/