From: Linus Walleij <linus.walleij@linaro.org> To: Gregory CLEMENT <gregory.clement@free-electrons.com> Cc: "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>, Jason Cooper <jason@lakedaemon.net>, Andrew Lunn <andrew@lunn.ch>, Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>, Thomas Petazzoni <thomas.petazzoni@free-electrons.com>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, Nadav Haklai <nadavh@marvell.com>, Victor Gu <xigu@marvell.com>, Omri Itach <omrii@marvell.com>, Marcin Wojtas <mw@semihalf.com>, Wilson Ding <dingwei@marvell.com>, Hua Jing <jinghua@marvell.com>, Terry Zhou <bjzhou@marvell.com> Subject: Re: [PATCH 2/6] pinctrl: armada-37xx: Add pin controller support for Armada 37xx Date: Fri, 30 Dec 2016 09:44:49 +0100 [thread overview] Message-ID: <CACRpkdbnfkb8tDtjzbQdhg8JxV1S5FdFjPx7B8NAT+nkeUO=Lw@mail.gmail.com> (raw) In-Reply-To: <20161222172501.16121-3-gregory.clement@free-electrons.com> On Thu, Dec 22, 2016 at 6:24 PM, Gregory CLEMENT <gregory.clement@free-electrons.com> wrote: > The Armada 37xx SoC come with 2 pin controllers: one on the south > bridge (managing 28 pins) and one on the north bridge (managing 36 pins). > > At the hardware level the controller configure the pins by group and not > pin by pin. This constraint is reflected in the design of the driver: > only the group related functions are implemented. > > Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> Overall this looks good. > diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms > index 715ef1256838..0786e3e0f5c6 100644 > --- a/arch/arm64/Kconfig.platforms > +++ b/arch/arm64/Kconfig.platforms > @@ -105,6 +105,8 @@ config ARCH_MVEBU > select ARMADA_37XX_CLK > select MVEBU_ODMI > select MVEBU_PIC > + select PINCTRL > + select PINCTRL_ARMADA_37XX Do you already select MFD_SYSCON? It seems to be required. I can't merge patches to ARM SoC and prefer not to. Split this into a separate patch for ARM SoC in the Armada/Marvell tree. > -obj-$(CONFIG_PINCTRL_MVEBU) += mvebu/ > +obj-y += mvebu/ Just make sure everything is guarded with proper symbols. > +config PINCTRL_ARMADA_37XX > + bool > + select PINMUX > + select PINCONF > + select GENERIC_PINCONF Nice! > +struct armada_37xx_pin_group { > + const char *name; > + unsigned int start_pin; > + unsigned int npins; > + u32 reg_mask; > + unsigned int extra_pin; > + unsigned int extra_npins; > + const char *funcs[NB_FUNCS]; > + unsigned int *pins; > +}; I would prefer if you add some kerneldoc to this struct. Especially the extra_pin things are not evident so explain this in detail here. > +struct armada_37xx_pin_data { > + u8 nr_pins; > + char *name; > + struct armada_37xx_pin_group *groups; > + int ngroups; > +}; > + > +struct armada_37xx_pmx_func { > + const char *name; > + const char **groups; > + unsigned int ngroups; > +}; > + > +struct armada_37xx_pinctrl { > + struct regmap *regmap; > + struct armada_37xx_pin_data *data; > + struct device *dev; > + struct pinctrl_desc pctl; > + struct pinctrl_dev *pctl_dev; > + struct armada_37xx_pin_group *groups; > + unsigned int ngroups; > + struct armada_37xx_pmx_func *funcs; > + unsigned int nfuncs; > +}; You do not need to document these. They are self-explanatory. > +static int armada_37xx_pin_config_group_get(struct pinctrl_dev *pctldev, > + unsigned int selector, unsigned long *config) > +{ > + return -ENOTSUPP; > +} > + > +static int armada_37xx_pin_config_group_set(struct pinctrl_dev *pctldev, > + unsigned int selector, unsigned long *configs, > + unsigned int num_configs) > +{ > + return -ENOTSUPP; > +} > + > +static struct pinconf_ops armada_37xx_pinconf_ops = { > + .is_generic = true, > + .pin_config_group_get = armada_37xx_pin_config_group_get, > + .pin_config_group_set = armada_37xx_pin_config_group_set, > +}; Don't we support just leaving group set/get uninitialized? Too bad in that case. > +static int _add_function(struct armada_37xx_pmx_func *funcs, int *funcsize, > + const char *name) No _foo opaque underscore prefix please. Git this a reasonable name like armada_37xx_add_function() or so. Apart from that it looks good. Yours, Linus Walleij
WARNING: multiple messages have this Message-ID (diff)
From: linus.walleij@linaro.org (Linus Walleij) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 2/6] pinctrl: armada-37xx: Add pin controller support for Armada 37xx Date: Fri, 30 Dec 2016 09:44:49 +0100 [thread overview] Message-ID: <CACRpkdbnfkb8tDtjzbQdhg8JxV1S5FdFjPx7B8NAT+nkeUO=Lw@mail.gmail.com> (raw) In-Reply-To: <20161222172501.16121-3-gregory.clement@free-electrons.com> On Thu, Dec 22, 2016 at 6:24 PM, Gregory CLEMENT <gregory.clement@free-electrons.com> wrote: > The Armada 37xx SoC come with 2 pin controllers: one on the south > bridge (managing 28 pins) and one on the north bridge (managing 36 pins). > > At the hardware level the controller configure the pins by group and not > pin by pin. This constraint is reflected in the design of the driver: > only the group related functions are implemented. > > Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> Overall this looks good. > diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms > index 715ef1256838..0786e3e0f5c6 100644 > --- a/arch/arm64/Kconfig.platforms > +++ b/arch/arm64/Kconfig.platforms > @@ -105,6 +105,8 @@ config ARCH_MVEBU > select ARMADA_37XX_CLK > select MVEBU_ODMI > select MVEBU_PIC > + select PINCTRL > + select PINCTRL_ARMADA_37XX Do you already select MFD_SYSCON? It seems to be required. I can't merge patches to ARM SoC and prefer not to. Split this into a separate patch for ARM SoC in the Armada/Marvell tree. > -obj-$(CONFIG_PINCTRL_MVEBU) += mvebu/ > +obj-y += mvebu/ Just make sure everything is guarded with proper symbols. > +config PINCTRL_ARMADA_37XX > + bool > + select PINMUX > + select PINCONF > + select GENERIC_PINCONF Nice! > +struct armada_37xx_pin_group { > + const char *name; > + unsigned int start_pin; > + unsigned int npins; > + u32 reg_mask; > + unsigned int extra_pin; > + unsigned int extra_npins; > + const char *funcs[NB_FUNCS]; > + unsigned int *pins; > +}; I would prefer if you add some kerneldoc to this struct. Especially the extra_pin things are not evident so explain this in detail here. > +struct armada_37xx_pin_data { > + u8 nr_pins; > + char *name; > + struct armada_37xx_pin_group *groups; > + int ngroups; > +}; > + > +struct armada_37xx_pmx_func { > + const char *name; > + const char **groups; > + unsigned int ngroups; > +}; > + > +struct armada_37xx_pinctrl { > + struct regmap *regmap; > + struct armada_37xx_pin_data *data; > + struct device *dev; > + struct pinctrl_desc pctl; > + struct pinctrl_dev *pctl_dev; > + struct armada_37xx_pin_group *groups; > + unsigned int ngroups; > + struct armada_37xx_pmx_func *funcs; > + unsigned int nfuncs; > +}; You do not need to document these. They are self-explanatory. > +static int armada_37xx_pin_config_group_get(struct pinctrl_dev *pctldev, > + unsigned int selector, unsigned long *config) > +{ > + return -ENOTSUPP; > +} > + > +static int armada_37xx_pin_config_group_set(struct pinctrl_dev *pctldev, > + unsigned int selector, unsigned long *configs, > + unsigned int num_configs) > +{ > + return -ENOTSUPP; > +} > + > +static struct pinconf_ops armada_37xx_pinconf_ops = { > + .is_generic = true, > + .pin_config_group_get = armada_37xx_pin_config_group_get, > + .pin_config_group_set = armada_37xx_pin_config_group_set, > +}; Don't we support just leaving group set/get uninitialized? Too bad in that case. > +static int _add_function(struct armada_37xx_pmx_func *funcs, int *funcsize, > + const char *name) No _foo opaque underscore prefix please. Git this a reasonable name like armada_37xx_add_function() or so. Apart from that it looks good. Yours, Linus Walleij
next prev parent reply other threads:[~2016-12-30 8:44 UTC|newest] Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-12-22 17:24 [PATCH 0/6] Add support for pinctrl/gpio on Armada 37xx Gregory CLEMENT 2016-12-22 17:24 ` Gregory CLEMENT 2016-12-22 17:24 ` [PATCH 1/6] pinctrl: dt-bindings: Add documentation for Armada 37xx pin controllers Gregory CLEMENT 2016-12-22 17:24 ` Gregory CLEMENT 2016-12-30 8:35 ` Linus Walleij 2016-12-30 8:35 ` Linus Walleij 2017-03-22 11:42 ` Gregory CLEMENT 2017-03-22 11:42 ` Gregory CLEMENT 2016-12-22 17:24 ` [PATCH 2/6] pinctrl: armada-37xx: Add pin controller support for Armada 37xx Gregory CLEMENT 2016-12-22 17:24 ` Gregory CLEMENT 2016-12-30 8:44 ` Linus Walleij [this message] 2016-12-30 8:44 ` Linus Walleij 2017-03-22 11:47 ` Gregory CLEMENT 2017-03-22 11:47 ` Gregory CLEMENT 2016-12-22 17:24 ` [PATCH 3/6] pinctrl: armada-37xx: Add gpio support Gregory CLEMENT 2016-12-22 17:24 ` Gregory CLEMENT 2016-12-30 8:51 ` Linus Walleij 2016-12-30 8:51 ` Linus Walleij 2017-03-22 11:54 ` Gregory CLEMENT 2017-03-22 11:54 ` Gregory CLEMENT 2017-03-23 10:28 ` Linus Walleij 2017-03-23 10:28 ` Linus Walleij 2017-03-23 14:47 ` Gregory CLEMENT 2017-03-23 14:47 ` Gregory CLEMENT 2016-12-22 17:24 ` [PATCH 4/6] pinctrl: aramda-37xx: Add irqchip support Gregory CLEMENT 2016-12-22 17:24 ` Gregory CLEMENT 2016-12-30 8:58 ` Linus Walleij 2016-12-30 8:58 ` Linus Walleij 2017-03-22 12:02 ` Gregory CLEMENT 2017-03-22 12:02 ` Gregory CLEMENT 2017-03-23 10:36 ` Linus Walleij 2017-03-23 10:36 ` Linus Walleij 2017-03-23 14:41 ` Gregory CLEMENT 2017-03-23 14:41 ` Gregory CLEMENT 2016-12-22 17:25 ` [PATCH 5/6] ARM64: dts: marvell: Add pinctrl nodes for Armada 3700 Gregory CLEMENT 2016-12-22 17:25 ` Gregory CLEMENT 2016-12-22 17:25 ` [PATCH 6/6] ARM64: dts: marvell: armada37xx: add pinctrl definition Gregory CLEMENT 2016-12-22 17:25 ` Gregory CLEMENT 2016-12-30 9:00 ` Linus Walleij 2016-12-30 9:00 ` Linus Walleij
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CACRpkdbnfkb8tDtjzbQdhg8JxV1S5FdFjPx7B8NAT+nkeUO=Lw@mail.gmail.com' \ --to=linus.walleij@linaro.org \ --cc=andrew@lunn.ch \ --cc=bjzhou@marvell.com \ --cc=dingwei@marvell.com \ --cc=gregory.clement@free-electrons.com \ --cc=jason@lakedaemon.net \ --cc=jinghua@marvell.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-gpio@vger.kernel.org \ --cc=mw@semihalf.com \ --cc=nadavh@marvell.com \ --cc=omrii@marvell.com \ --cc=sebastian.hesselbarth@gmail.com \ --cc=thomas.petazzoni@free-electrons.com \ --cc=xigu@marvell.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.