From mboxrd@z Thu Jan 1 00:00:00 1970 From: boris brezillon Subject: Re: [RFC PATCH 2/3] pinctrl: at91: add support for generic pinconf Date: Mon, 26 Aug 2013 20:45:09 +0200 Message-ID: <521BA235.1090104@overkiz.com> References: <1377379926-11163-1-git-send-email-b.brezillon@overkiz.com> <1377380259-11290-1-git-send-email-b.brezillon@overkiz.com> <20130826175333.GF18627@ns203013.ovh.net> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <20130826175333.GF18627@ns203013.ovh.net> Sender: linux-doc-owner@vger.kernel.org To: Jean-Christophe PLAGNIOL-VILLARD Cc: Rob Herring , Pawel Moll , Mark Rutland , Stephen Warren , Ian Campbell , Rob Landley , Russell King , Linus Walleij , Jiri Kosina , Masanari Iida , Nicolas Ferre , Richard Genoud , Heiko Stuebner , James Hogan , devicetree@vger.kernel.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org List-Id: devicetree@vger.kernel.org Hello Jean-Christophe, Le 26/08/2013 19:53, Jean-Christophe PLAGNIOL-VILLARD a =E9crit : > On 23:37 Sat 24 Aug , Boris BREZILLON wrote: >> Add support for generic pin configuration to pinctrl-at91 driver. >> >> Signed-off-by: Boris BREZILLON >> --- >> .../bindings/pinctrl/atmel,at91-pinctrl.txt | 43 +++- >> drivers/pinctrl/Kconfig | 2 +- >> drivers/pinctrl/pinctrl-at91.c | 265 ++++++++= ++++++++++-- >> 3 files changed, 289 insertions(+), 21 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pi= nctrl.txt b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctr= l.txt >> index 7ccae49..7a7c0c4 100644 >> --- a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.t= xt >> +++ b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.t= xt >> @@ -18,7 +18,9 @@ mode) this pin can work on and the 'config' config= ures various pad settings >> such as pull-up, multi drive, etc. >> =20 >> Required properties for iomux controller: >> -- compatible: "atmel,at91rm9200-pinctrl" >> +- compatible: "atmel,at91rm9200-pinctrl" or "atmel,at91sam9x5-pinct= rl". >> + Add "generic-pinconf" to the compatible string list to use the ge= neric pin >> + configuration syntax. >> - atmel,mux-mask: array of mask (periph per bank) to describe if a= pin can be >> configured in this periph mode. All the periph and bank need to = be describe. >> =20 >> @@ -83,6 +85,11 @@ Required properties for pin configuration node: >> setting. The format is atmel,pins =3D . >> The PERIPH 0 means gpio, PERIPH 1 is periph A, PERIPH 2 is perip= h B... >> PIN_BANK 0 is pioA, PIN_BANK 1 is pioB... >> + Dependending on the presence of the "generic-pinconf" string in t= he >> + compatible property the 4th cell is: >> + * a phandle referencing a generic pin config node (refer to >> + pinctrl-bindings.txt) >> + * an integer defining the pin config (see the following descript= ion) >> =20 >> Bits used for CONFIG: >> PULL_UP (1 << 0): indicate this pin need a pull up. >> @@ -132,6 +139,40 @@ pinctrl@fffff400 { >> }; >> }; >> =20 >> +or >> + >> +pinctrl@fffff400 { >> + #address-cells =3D <1>; >> + #size-cells =3D <1>; >> + ranges; >> + compatible =3D "atmel,at91rm9200-pinctrl", "generic-pinconf", "sim= ple-bus"; > nack your break the backword compatibility > > if we use a old kernel with this new dt nothing will work > as the old kernel will never known the the "generic-pinconf" means an= ything Your're right, I didn't think of this case (old kernel with new dt). > if we want to use generic-pinconf support you *CAN NOT* use atmel,at9= 1rm9200-pinctrl > in the compatible What about using "atmel,at91xx-pinconf" instead of=20 "atmel,at91xx-pinctrl" to notify the generic pinconf compatibility (as done by single pinctrl driver) ? >> + reg =3D <0xfffff400 0x600>; >> + >> + atmel,mux-mask =3D < >> + /* A B */ >> + 0xffffffff 0xffc00c3b /* pioA */ >> + 0xffffffff 0x7fff3ccf /* pioB */ >> + 0xffffffff 0x007fffff /* pioC */ >> + >; >> + >> + pcfg_none: pcfg_none { >> + bias-disable; >> + }; >> + >> + pcfg_pull_up: pcfg_pull_up { >> + bias-pullup; >> + }; >> + >> + /* shared pinctrl settings */ >> + dbgu { >> + pinctrl_dbgu: dbgu-0 { >> + atmel,pins =3D >> + <1 14 0x1 &pcfg_none /* PB14 periph A */ >> + 1 15 0x1 &pcfg_pull_up>; /* PB15 periph A with pullup */ >> + }; >> + }; >> +}; >> + >> dbgu: serial@fffff200 { >> compatible =3D "atmel,at91sam9260-usart"; >> reg =3D <0xfffff200 0x200>; >> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig >> index bdb1a87..55a4f2c 100644 >> --- a/drivers/pinctrl/Kconfig >> +++ b/drivers/pinctrl/Kconfig >> @@ -54,7 +54,7 @@ config PINCTRL_AT91 >> depends on OF >> depends on ARCH_AT91 >> select PINMUX >> - select PINCONF >> + select GENERIC_PINCONF >> help >> Say Y here to enable the at91 pinctrl driver >> =20 >> diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctr= l-at91.c >> index 7cce066..1994dd2 100644 >> --- a/drivers/pinctrl/pinctrl-at91.c >> +++ b/drivers/pinctrl/pinctrl-at91.c >> @@ -23,6 +23,7 @@ >> #include >> #include >> #include >> +#include >> #include >> #include >> /* Since we request GPIOs from ourself */ >> @@ -32,6 +33,7 @@ >> #include >> =20 >> #include "core.h" >> +#include "pinconf.h" >> =20 >> #define MAX_NB_GPIO_PER_BANK 32 >> =20 >> @@ -85,6 +87,21 @@ enum at91_mux { >> AT91_MUX_PERIPH_D =3D 4, >> }; >> =20 >> +struct at91_generic_pinconf { >> + unsigned long *configs; >> + unsigned int nconfigs; >> +}; >> + >> +enum at91_pinconf_type { >> + AT91_PINCONF_NATIVE, >> + AT91_PINCONF_GENERIC, >> +}; >> + >> +union at91_pinconf { >> + unsigned long native; >> + struct at91_generic_pinconf generic; >> +}; >> + >> /** >> * struct at91_pmx_pin - describes an At91 pin mux >> * @bank: the bank of the pin >> @@ -93,10 +110,11 @@ enum at91_mux { >> * @conf: the configuration of the pin: PULL_UP, MULTIDRIVE etc... >> */ >> struct at91_pmx_pin { >> - uint32_t bank; >> - uint32_t pin; >> - enum at91_mux mux; >> - unsigned long conf; >> + uint32_t bank; >> + uint32_t pin; >> + enum at91_mux mux; >> + enum at91_pinconf_type conftype; >> + union at91_pinconf conf; >> }; >> =20 >> /** >> @@ -278,8 +296,16 @@ static int at91_dt_node_to_map(struct pinctrl_d= ev *pctldev, >> new_map[i].type =3D PIN_MAP_TYPE_CONFIGS_PIN; >> new_map[i].data.configs.group_or_pin =3D >> pin_get_name(pctldev, grp->pins[i]); >> - new_map[i].data.configs.configs =3D &grp->pins_conf[i].conf; >> - new_map[i].data.configs.num_configs =3D 1; >> + if (!pctldev->desc->confops->is_generic) { >> + new_map[i].data.configs.configs =3D >> + &grp->pins_conf[i].conf.native; >> + new_map[i].data.configs.num_configs =3D 1; >> + } else { >> + new_map[i].data.configs.configs =3D >> + grp->pins_conf[i].conf.generic.configs; >> + new_map[i].data.configs.num_configs =3D >> + grp->pins_conf[i].conf.generic.nconfigs; >> + } >> } >> =20 >> dev_dbg(pctldev->dev, "maps: function %s group %s num %d\n", >> @@ -325,7 +351,7 @@ static void at91_mux_disable_interrupt(void __io= mem *pio, unsigned mask) >> =20 >> static unsigned at91_mux_get_pullup(void __iomem *pio, unsigned pi= n) >> { >> - return (readl_relaxed(pio + PIO_PUSR) >> pin) & 0x1; >> + return !((readl_relaxed(pio + PIO_PUSR) >> pin) & 0x1); >> } >> =20 >> static void at91_mux_set_pullup(void __iomem *pio, unsigned mask, = bool on) >> @@ -407,6 +433,16 @@ static enum at91_mux at91_mux_get_periph(void _= _iomem *pio, unsigned mask) >> return select + 1; >> } >> =20 >> +static bool at91_mux_get_output(void __iomem *pio, unsigned pin) >> +{ >> + return (readl_relaxed(pio + PIO_OSR) >> pin) & 0x1; >> +} >> + >> +static void at91_mux_set_output(void __iomem *pio, unsigned mask, b= ool is_on) >> +{ >> + __raw_writel(mask, pio + (is_on ? PIO_OER : PIO_ODR)); >> +} >> + >> static bool at91_mux_get_deglitch(void __iomem *pio, unsigned pin) >> { >> return (__raw_readl(pio + PIO_IFSR) >> pin) & 0x1; >> @@ -445,7 +481,7 @@ static void at91_mux_pio3_set_debounce(void __io= mem *pio, unsigned mask, >> =20 >> static bool at91_mux_pio3_get_pulldown(void __iomem *pio, unsigned= pin) >> { >> - return (__raw_readl(pio + PIO_PPDSR) >> pin) & 0x1; >> + return !((__raw_readl(pio + PIO_PPDSR) >> pin) & 0x1); >> } >> =20 >> static void at91_mux_pio3_set_pulldown(void __iomem *pio, unsigned= mask, bool is_on) >> @@ -492,12 +528,17 @@ static struct at91_pinctrl_mux_ops at91sam9x5_= ops =3D { >> static void at91_pin_dbg(const struct device *dev, const struct at= 91_pmx_pin *pin) >> { >> if (pin->mux) { >> - dev_dbg(dev, "pio%c%d configured as periph%c with conf =3D 0x%lu\= n", >> - pin->bank + 'A', pin->pin, pin->mux - 1 + 'A', pin->conf); >> + dev_dbg(dev, "pio%c%d configured as periph%c", >> + pin->bank + 'A', pin->pin, pin->mux - 1 + 'A'); >> } else { >> - dev_dbg(dev, "pio%c%d configured as gpio with conf =3D 0x%lu\n", >> - pin->bank + 'A', pin->pin, pin->conf); >> + dev_dbg(dev, "pio%c%d configured as gpio", >> + pin->bank + 'A', pin->pin); >> } >> + >> + if (pin->conftype =3D=3D AT91_PINCONF_NATIVE) >> + dev_dbg(dev, " with conf =3D 0x%lu\n", pin->conf.native); >> + else >> + dev_dbg(dev, "\n"); > do not change debug output I do not change the debug output for the native pinconf binding, but I=20 cannot print the config as a single interger in hex format if the generic pinconf is used. I must translate each config entry to a "property=3Dvalue" string, or=20 translate the generic config array to a single native config integer. Do you have something easier in mind ? >> } >> =20 >> static int pin_check_config(struct at91_pinctrl *info, const char = *name, >> @@ -782,6 +823,157 @@ static const struct pinconf_ops at91_pinconf_o= ps =3D { >> .pin_config_group_dbg_show =3D at91_pinconf_group_dbg_show, >> }; >> =20 >> +static int at91_generic_pinconf_get(struct pinctrl_dev *pctldev, >> + unsigned pin_id, unsigned long *config) >> +{ >> + struct at91_pinctrl *info =3D pinctrl_dev_get_drvdata(pctldev); >> + enum pin_config_param param =3D pinconf_to_config_param(*config); >> + void __iomem *pio =3D pin_to_controller(info, pin_to_bank(pin_id))= ; >> + unsigned pin =3D pin_id % MAX_NB_GPIO_PER_BANK; >> + int div; >> + >> + switch (param) { >> + case PIN_CONFIG_BIAS_DISABLE: >> + if (at91_mux_get_pullup(pio, pin) && >> + (info->ops->get_pulldown || >> + !info->ops->get_pulldown(pio, pin))) >> + return -EINVAL; >> + >> + *config =3D 0; >> + break; >> + case PIN_CONFIG_BIAS_PULL_UP: >> + if (!at91_mux_get_pullup(pio, pin)) >> + return -EINVAL; >> + >> + *config =3D 0; >> + break; >> + case PIN_CONFIG_BIAS_PULL_DOWN: >> + if (!info->ops->get_pulldown) >> + return -ENOTSUPP; >> + if (!info->ops->get_pulldown(pio, pin)) >> + return -EINVAL; >> + >> + *config =3D 0; >> + break; >> + case PIN_CONFIG_DRIVE_OPEN_DRAIN: >> + if (!at91_mux_get_multidrive(pio, pin)) >> + return -EINVAL; >> + >> + *config =3D 0; >> + break; >> + case PIN_CONFIG_INPUT_SCHMITT_ENABLE: >> + if (!info->ops->get_schmitt_trig) >> + return -ENOTSUPP; >> + >> + if (!(info->ops->get_schmitt_trig(pio, pin))) >> + *config =3D 1; >> + else >> + *config =3D 0; >> + break; >> + case PIN_CONFIG_INPUT_DEBOUNCE: >> + if (!info->ops->get_debounce) >> + return -ENOTSUPP; >> + >> + if (info->ops->get_debounce(pio, pin, &div)) { >> + /* TODO: replace 32768 with clk_get_rate(slck) return */ >> + *config =3D ((div + 1) * 2) * 1000000 / 32768; >> + if (*config > 0xffff) >> + *config =3D 0xffff; >> + } else >> + *config =3D 0; >> + break; >> + case PIN_CONFIG_INPUT_DEGLITCH: >> + if (!info->ops->get_deglitch) >> + return -ENOTSUPP; >> + >> + *config =3D info->ops->get_deglitch(pio, pin); >> + break; >> + case PIN_CONFIG_OUTPUT: >> + if (info->ops->get_periph(pio, pin) !=3D AT91_MUX_GPIO) >> + return -EINVAL; >> + >> + *config =3D at91_mux_get_output(pio, pin); >> + break; >> + default: >> + return -ENOTSUPP; >> + break; >> + } >> + >> + return 0; >> +} >> + >> +static int at91_generic_pinconf_set(struct pinctrl_dev *pctldev, >> + unsigned pin_id, unsigned long config) >> +{ >> + struct at91_pinctrl *info =3D pinctrl_dev_get_drvdata(pctldev); >> + enum pin_config_param param =3D pinconf_to_config_param(config); >> + u16 arg =3D pinconf_to_config_argument(config); >> + u32 div =3D 0; >> + unsigned pin =3D pin_id % MAX_NB_GPIO_PER_BANK; >> + unsigned mask =3D pin_to_mask(pin); >> + void __iomem *pio =3D pin_to_controller(info, pin_to_bank(pin_id))= ; >> + >> + switch (param) { >> + case PIN_CONFIG_BIAS_DISABLE: >> + at91_mux_set_pullup(pio, mask, 0); >> + if (info->ops->set_pulldown) >> + info->ops->set_pulldown(pio, mask, 0); >> + break; >> + case PIN_CONFIG_BIAS_PULL_UP: >> + at91_mux_set_pullup(pio, mask, arg); >> + break; >> + case PIN_CONFIG_BIAS_PULL_DOWN: >> + if (!info->ops->set_pulldown) >> + return -ENOTSUPP; >> + info->ops->set_pulldown(pio, mask, arg); >> + break; >> + case PIN_CONFIG_DRIVE_OPEN_DRAIN: >> + at91_mux_set_multidrive(pio, mask, 1); >> + break; >> + case PIN_CONFIG_INPUT_SCHMITT_ENABLE: >> + if (!info->ops->disable_schmitt_trig) >> + return -ENOTSUPP; >> + if (arg) >> + mask =3D ~mask; >> + info->ops->disable_schmitt_trig(pio, mask); >> + break; >> + case PIN_CONFIG_INPUT_DEBOUNCE: >> + if (!info->ops->set_debounce) >> + return -ENOTSUPP; >> + >> + /* TODO: replace 32768 with clk_get_rate(slck) return */ >> + if (arg) { >> + div =3D (arg * 32768 / (2 * 1000000)); >> + if (div) >> + div--; >> + } >> + info->ops->set_debounce(pio, mask, arg ? 1 : 0, div); >> + break; >> + case PIN_CONFIG_INPUT_DEGLITCH: >> + if (!info->ops->set_deglitch) >> + return -ENOTSUPP; >> + >> + info->ops->set_deglitch(pio, mask, arg ? 1 : 0); >> + break; >> + case PIN_CONFIG_OUTPUT: >> + if (info->ops->get_periph(pio, pin) !=3D AT91_MUX_GPIO) >> + return -EINVAL; >> + at91_mux_set_output(pio, mask, arg); >> + break; >> + default: >> + return -ENOTSUPP; >> + break; >> + } >> + >> + return 0; >> +} >> + >> +static const struct pinconf_ops at91_generic_pinconf_ops =3D { >> + .is_generic =3D true, >> + .pin_config_get =3D at91_generic_pinconf_get, >> + .pin_config_set =3D at91_generic_pinconf_set, >> +}; >> + >> static struct pinctrl_desc at91_pinctrl_desc =3D { >> .pctlops =3D &at91_pctrl_ops, >> .pmxops =3D &at91_pmx_ops, >> @@ -847,6 +1039,7 @@ static int at91_pinctrl_parse_groups(struct dev= ice_node *np, >> int size; >> const __be32 *list; >> int i, j; >> + int err; >> =20 >> dev_dbg(info->dev, "group(%d): %s\n", index, np->name); >> =20 >> @@ -870,21 +1063,48 @@ static int at91_pinctrl_parse_groups(struct d= evice_node *np, >> GFP_KERNEL); >> grp->pins =3D devm_kzalloc(info->dev, grp->npins * sizeof(unsigne= d int), >> GFP_KERNEL); >> - if (!grp->pins_conf || !grp->pins) >> - return -ENOMEM; >> + if (!grp->pins_conf || !grp->pins) { >> + err =3D -ENOMEM; >> + goto out_err; >> + } > why ??? Right, I didn't notice the devm_kzalloc, I thought it was allocated=20 using kzalloc. >> =20 >> for (i =3D 0, j =3D 0; i < size; i +=3D 4, j++) { >> pin->bank =3D be32_to_cpu(*list++); >> pin->pin =3D be32_to_cpu(*list++); >> grp->pins[j] =3D pin->bank * MAX_NB_GPIO_PER_BANK + pin->pin; >> pin->mux =3D be32_to_cpu(*list++); >> - pin->conf =3D be32_to_cpu(*list++); >> + if (at91_pinctrl_desc.confops->is_generic) { >> + struct device_node *np_config; >> + const __be32 *phandle =3D list++; >> + >> + if (!phandle) { >> + err =3D -EINVAL; >> + goto out_err; >> + } >> + np_config =3D >> + of_find_node_by_phandle(be32_to_cpup(phandle)); >> + pin->conftype =3D AT91_PINCONF_GENERIC; >> + err =3D pinconf_generic_parse_dt_config(np_config, >> + &pin->conf.generic.configs, >> + &pin->conf.generic.nconfigs); >> + if (err) >> + goto out_err; >> + >> + } else { >> + pin->conftype =3D AT91_PINCONF_NATIVE; >> + pin->conf.native =3D be32_to_cpu(*list++); >> + } >> =20 >> at91_pin_dbg(info->dev, pin); >> pin++; >> } >> =20 >> return 0; >> + >> +out_err: >> + kfree(grp->pins_conf); >> + kfree(grp->pins); > use devm and drop those kfree Same mistake (devm_kzalloc is already used). I'll drop this part for next version. >> + return err; >> } >> =20 >> static int at91_pinctrl_parse_functions(struct device_node *np, >> @@ -904,10 +1124,10 @@ static int at91_pinctrl_parse_functions(struc= t device_node *np, >> /* Initialise function */ >> func->name =3D np->name; >> func->ngroups =3D of_get_child_count(np); >> - if (func->ngroups <=3D 0) { >> - dev_err(info->dev, "no groups defined\n"); >> - return -EINVAL; >> - } >> + /* This node might be a generic config definition: silently ignore= it */ >> + if (func->ngroups <=3D 0) >> + return 0; >> + >> func->groups =3D devm_kzalloc(info->dev, >> func->ngroups * sizeof(char *), GFP_KERNEL); >> if (!func->groups) >> @@ -930,6 +1150,11 @@ static struct of_device_id at91_pinctrl_of_mat= ch[] =3D { >> { /* sentinel */ } >> }; >> =20 >> +static struct of_device_id at91_pinconf_of_match[] =3D { >> + { .compatible =3D "generic-pinconf" }, >> + { /* sentinel */ } >> +}; >> + >> static int at91_pinctrl_probe_dt(struct platform_device *pdev, >> struct at91_pinctrl *info) >> { >> @@ -945,6 +1170,8 @@ static int at91_pinctrl_probe_dt(struct platfor= m_device *pdev, >> info->dev =3D &pdev->dev; >> info->ops =3D (struct at91_pinctrl_mux_ops *) >> of_match_device(at91_pinctrl_of_match, &pdev->dev)->data; >> + if (of_match_device(at91_pinconf_of_match, &pdev->dev)) >> + at91_pinctrl_desc.confops =3D &at91_generic_pinconf_ops; >> at91_pinctrl_child_count(info, np); >> =20 >> if (info->nbanks < 1) { >> --=20 >> 1.7.9.5 >> Thanks for your review. Best Regards, Boris