RE: [PATCH 1/3] pinctrl: samsung: support ExynosAutov9 SoC pinctrl

From: "Chanho Park" <chanho61.park@samsung.com>
To: "'Krzysztof Kozlowski'" <krzysztof.kozlowski@canonical.com>,
	"'Rob Herring'" <robh+dt@kernel.org>,
	"'Linus Walleij'" <linus.walleij@linaro.org>
Cc: "'Tomasz Figa'" <tomasz.figa@gmail.com>,
	"'Sylwester Nawrocki'" <s.nawrocki@samsung.com>,
	<linux-samsung-soc@vger.kernel.org>, <devicetree@vger.kernel.org>
Subject: RE: [PATCH 1/3] pinctrl: samsung: support ExynosAutov9 SoC pinctrl
Date: Fri, 8 Oct 2021 09:16:13 +0900	[thread overview]
Message-ID: <00d101d7bbd9$b42e27a0$1c8a76e0$@samsung.com> (raw)
In-Reply-To: <a6826964-5949-26b5-fc6b-7b52f7c752af@canonical.com>

Hi Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> Sent: Friday, October 8, 2021 2:55 AM
> To: Chanho Park <chanho61.park@samsung.com>; Rob Herring
> <robh+dt@kernel.org>; Linus Walleij <linus.walleij@linaro.org>
> Cc: Tomasz Figa <tomasz.figa@gmail.com>; Sylwester Nawrocki
> <s.nawrocki@samsung.com>; linux-samsung-soc@vger.kernel.org;
> devicetree@vger.kernel.org
> Subject: Re: [PATCH 1/3] pinctrl: samsung: support ExynosAutov9 SoC
> pinctrl
> 
> On 07/10/2021 14:08, Chanho Park wrote:
> > Add pinctrl data for ExynosAuto v9 SoC.
> >
> > - GPA0, GPA1: 10, External wake up interrupt
> > - GPQ0: 2, XbootLDO, Speedy PMIC I/F
> > - GPB0, GPB1, GPB2, GPB3: 29, I2S 7 CH
> > - GPF0, GPF1, GPF2, GPF3,GPF4, GPF5, GPF6, GPF8: 52, FSYS
> > - GPG0, GPG1, GPG2, GPG3: 25, GPIO x 24, SMPL_INT
> > - GPP0, GPP1, GPP2, GPP3, GPP4, GPP5: 48, USI 12 CH
> >
> > Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> 
> Thanks Chanho for the patches. It's awesome to see this work upstreamed!
> 
> Few comments below.

Thanks for your review.

> 
> > ---
> >  .../bindings/pinctrl/samsung-pinctrl.txt      |   1 +
> >  .../pinctrl/samsung/pinctrl-exynos-arm64.c    | 108 ++++++++++++++++++
> >  drivers/pinctrl/samsung/pinctrl-samsung.c     |   2 +
> >  drivers/pinctrl/samsung/pinctrl-samsung.h     |   1 +
> >  4 files changed, 112 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
> > b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
> > index e7a1b1880375..b8b475967ff9 100644
> > --- a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
> > +++ b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
> > @@ -23,6 +23,7 @@ Required Properties:
> >    - "samsung,exynos5433-pinctrl": for Exynos5433 compatible pin-
> controller.
> >    - "samsung,exynos7-pinctrl": for Exynos7 compatible pin-controller.
> >    - "samsung,exynos850-pinctrl": for Exynos850 compatible pin-controller.
> > +  - "samsung,exynosautov9-pinctrl": for ExynosAutov9 compatible pin-
> controller.
> >
> >  - reg: Base address of the pin controller hardware module and length of
> >    the address space it occupies.
> > diff --git a/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> > b/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> > index fe5f6046fbd5..3bf18e844402 100644
> > --- a/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> > +++ b/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> > @@ -538,3 +538,111 @@ const struct samsung_pinctrl_of_match_data
> exynos850_of_data __initconst = {
> >  	.ctrl		= exynos850_pin_ctrl,
> >  	.num_ctrl	= ARRAY_SIZE(exynos850_pin_ctrl),
> >  };
> > +
> > +/* pin banks of exynosautov9 pin-controller 0 (ALIVE) */ static
> > +struct samsung_pin_bank_data exynosautov9_pin_banks0[] = {
> 
> This and below should be static const and __initconst.

Okay. I'll put both.

> 
> > +	EXYNOS850_PIN_BANK_EINTW(8, 0x000, "gpa0", 0x00),
> > +	EXYNOS850_PIN_BANK_EINTW(2, 0x020, "gpa1", 0x04),
> > +	EXYNOS850_PIN_BANK_EINTN(2, 0x040, "gpq0"), };
> > +
> > +/* pin banks of exynosautov9 pin-controller 1 (AUD) */ static struct
> > +samsung_pin_bank_data exynosautov9_pin_banks1[] = {
> > +	EXYNOS850_PIN_BANK_EINTG(5, 0x000, "gpb0", 0x00),
> > +	EXYNOS850_PIN_BANK_EINTG(8, 0x020, "gpb1", 0x04),
> > +	EXYNOS850_PIN_BANK_EINTG(8, 0x040, "gpb2", 0x08),
> > +	EXYNOS850_PIN_BANK_EINTG(8, 0x060, "gpb3", 0x0C), };
> > +
> > +/* pin banks of exynosautov9 pin-controller 2 (FSYS0) */ static
> > +struct samsung_pin_bank_data exynosautov9_pin_banks2[] = {
> > +	EXYNOS850_PIN_BANK_EINTG(6, 0x000, "gpf0", 0x00),
> > +	EXYNOS850_PIN_BANK_EINTG(6, 0x020, "gpf1", 0x04), };
> > +
> > +/* pin banks of exynosautov9 pin-controller 3 (FSYS1) */ static
> > +struct samsung_pin_bank_data exynosautov9_pin_banks3[] = {
> > +	EXYNOS850_PIN_BANK_EINTG(6, 0x000, "gpf8", 0x00), };
> > +
> > +/* pin banks of exynosautov9 pin-controller 4 (FSYS2) */ static
> > +struct samsung_pin_bank_data exynosautov9_pin_banks4[] = {
> > +	EXYNOS850_PIN_BANK_EINTG(4, 0x000, "gpf2", 0x00),
> > +	EXYNOS850_PIN_BANK_EINTG(8, 0x020, "gpf3", 0x04),
> > +	EXYNOS850_PIN_BANK_EINTG(7, 0x040, "gpf4", 0x08),
> > +	EXYNOS850_PIN_BANK_EINTG(8, 0x060, "gpf5", 0x0C),
> > +	EXYNOS850_PIN_BANK_EINTG(7, 0x080, "gpf6", 0x10), };
> > +
> > +/* pin banks of exynosautov9 pin-controller 5 (PERIC0) */ static
> > +struct samsung_pin_bank_data exynosautov9_pin_banks5[] = {
> > +	EXYNOS850_PIN_BANK_EINTG(8, 0x000, "gpp0", 0x00),
> > +	EXYNOS850_PIN_BANK_EINTG(8, 0x020, "gpp1", 0x04),
> > +	EXYNOS850_PIN_BANK_EINTG(8, 0x040, "gpp2", 0x08),
> > +	EXYNOS850_PIN_BANK_EINTG(5, 0x060, "gpg0", 0x0C), };
> > +
> > +/* pin banks of exynosautov9 pin-controller 6 (PERIC1) */ static
> > +struct samsung_pin_bank_data exynosautov9_pin_banks6[] = {
> > +	EXYNOS850_PIN_BANK_EINTG(8, 0x000, "gpp3", 0x00),
> > +	EXYNOS850_PIN_BANK_EINTG(8, 0x020, "gpp4", 0x04),
> > +	EXYNOS850_PIN_BANK_EINTG(8, 0x040, "gpp5", 0x08),
> > +	EXYNOS850_PIN_BANK_EINTG(8, 0x060, "gpg1", 0x0C),
> > +	EXYNOS850_PIN_BANK_EINTG(8, 0x080, "gpg2", 0x10),
> > +	EXYNOS850_PIN_BANK_EINTG(4, 0x0A0, "gpg3", 0x14), };
> > +
> > +const struct samsung_pin_ctrl exynosautov9_pin_ctrl[] = {
> 
> __initconst at the end, please.

Will do as well.

Best Regards,
Chanho Park