> -----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-tegra" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
