From: Tomasz Figa <t.figa@samsung.com>
To: Sylwester Nawrocki <s.nawrocki@samsung.com>
Cc: Mateusz Krawczuk <m.krawczuk@partner.samsung.com>,
        linus.walleij@linaro.org, thomas.abraham@linaro.org,
        kgene.kim@samsung.com, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-samsung-soc@vger.kernel.org
Subject: Re: [PATCH 1/4] pinctrl: Add s5pv210 support to pinctrl-exynos
Date: Tue, 27 Aug 2013 14:26:41 +0200
Message-id: <1882987.W6oKxzvqXu@amdc1227>
Organization: Samsung Poland R&D Center
User-Agent: KMail/4.11 (Linux/3.10.9-gentoo; KDE/4.11.0; x86_64; ; )
In-reply-to: <521C71A1.4070609@samsung.com>
References: <1377595171-31116-1-git-send-email-m.krawczuk@partner.samsung.com>
 <521C71A1.4070609@samsung.com>
MIME-version: 1.0
Content-transfer-encoding: 7Bit
Content-type: text/plain; charset=us-ascii
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1723
Lines: 50

Hi Sylwester,

On Tuesday 27 of August 2013 11:30:09 Sylwester Nawrocki wrote:
> Hi,
> 
> Just a few nits...
> 
> On 08/27/2013 11:19 AM, Mateusz Krawczuk wrote:
> > This patch implements pinctrl for s5pv210 and adds required device tree
> > bindings.
> Would be good to wrap this to not exceed 80 columns.
> 
> > Signed-off-by: Mateusz Krawczuk <m.krawczuk@partner.samsung.com>
> > ---
> 
> [...]
> 
> > diff --git a/drivers/pinctrl/pinctrl-exynos.c
> > b/drivers/pinctrl/pinctrl-exynos.c index a74b3cb..fc3e1d7 100644
> > --- a/drivers/pinctrl/pinctrl-exynos.c
> > +++ b/drivers/pinctrl/pinctrl-exynos.c
> > @@ -660,6 +660,64 @@ static void exynos_pinctrl_resume(struct
> > samsung_pinctrl_drv_data *drvdata)> 
> >  			exynos_pinctrl_resume_bank(drvdata, bank);
> >  
> >  }
> > 
> > +/* pin banks of s5pv210 pin-controller */
> > +static struct samsung_pin_bank s5pv210_pin_banks0[] = {
> 
> Couldn't it be 'static const' ?

This driver relies on this array not being const, as it calculates 
remaining contents of the samsung_pin_bank struct dynamically.

Even though it is far from being elegant, it works correctly, because there 
is always one separate array per each pin controller instance. This could 
be improved by separating the constant part from the variable part or by 
using this array as a template, i.e. marking as __initdata and copying its 
contents, but I couldn't get myself to do that. Feel free to submit 
appropriate patches ;).

Best regards,
Tomasz

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/