lkml.org 
[lkml]   [2013]   [Apr]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRE: [PATCH 1/2] pinctrl: tegra: add suspend-resume support
    > -----Original Message-----
    > From: Thierry Reding [mailto:thierry.reding@avionic-design.de]
    > Sent: Wednesday, April 24, 2013 12:14 AM
    > To: Bibek Basu
    > Cc: linus.walleij@linaro.org; swarren@wwwdotorg.org; linux-
    > tegra@vger.kernel.org; linux-kernel@vger.kernel.org; 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


    \
     
     \ /
      Last update: 2013-04-28 15:41    [W:2.980 / U:0.068 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site