Re: [RFC PATCH 2/3] pinctrl: at91: add support for generic pinconf

From: Jean-Christophe PLAGNIOL-VILLARD
Date: Mon Aug 26 2013 - 19:51:27 EST


On 23:37 Sat 24 Aug , Boris BREZILLON wrote:
> Add support for generic pin configuration to pinctrl-at91 driver.
>
> Signed-off-by: Boris BREZILLON <b.brezillon@xxxxxxxxxxx>
> ---
> .../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-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
> index 7ccae49..7a7c0c4 100644
> --- a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt
> @@ -18,7 +18,9 @@ mode) this pin can work on and the 'config' configures various pad settings
> such as pull-up, multi drive, etc.
>
> Required properties for iomux controller:
> -- compatible: "atmel,at91rm9200-pinctrl"
> +- compatible: "atmel,at91rm9200-pinctrl" or "atmel,at91sam9x5-pinctrl".
> + Add "generic-pinconf" to the compatible string list to use the generic 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.
>
> @@ -83,6 +85,11 @@ Required properties for pin configuration node:
> setting. The format is atmel,pins = <PIN_BANK PIN_BANK_NUM PERIPH CONFIG>.
> The PERIPH 0 means gpio, PERIPH 1 is periph A, PERIPH 2 is periph B...
> PIN_BANK 0 is pioA, PIN_BANK 1 is pioB...
> + Dependending on the presence of the "generic-pinconf" string in the
> + 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 description)
>
> Bits used for CONFIG:
> PULL_UP (1 << 0): indicate this pin need a pull up.
> @@ -132,6 +139,40 @@ pinctrl@fffff400 {
> };
> };
>
> +or
> +
> +pinctrl@fffff400 {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> + compatible = "atmel,at91rm9200-pinctrl", "generic-pinconf", "simple-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 anything

if we want to use generic-pinconf support you *CAN NOT* use atmel,at91rm9200-pinctrl
in the compatible
> + reg = <0xfffff400 0x600>;
> +
> + atmel,mux-mask = <
> + /* 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 =
> + <1 14 0x1 &pcfg_none /* PB14 periph A */
> + 1 15 0x1 &pcfg_pull_up>; /* PB15 periph A with pullup */
> + };
> + };
> +};
> +
> dbgu: serial@fffff200 {
> compatible = "atmel,at91sam9260-usart";
> reg = <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
>
> diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
> index 7cce066..1994dd2 100644
> --- a/drivers/pinctrl/pinctrl-at91.c
> +++ b/drivers/pinctrl/pinctrl-at91.c
> @@ -23,6 +23,7 @@
> #include <linux/gpio.h>
> #include <linux/pinctrl/machine.h>
> #include <linux/pinctrl/pinconf.h>
> +#include <linux/pinctrl/pinconf-generic.h>
> #include <linux/pinctrl/pinctrl.h>
> #include <linux/pinctrl/pinmux.h>
> /* Since we request GPIOs from ourself */
> @@ -32,6 +33,7 @@
> #include <mach/at91_pio.h>
>
> #include "core.h"
> +#include "pinconf.h"
>
> #define MAX_NB_GPIO_PER_BANK 32
>
> @@ -85,6 +87,21 @@ enum at91_mux {
> AT91_MUX_PERIPH_D = 4,
> };
>
> +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;
> };
>
> /**
> @@ -278,8 +296,16 @@ static int at91_dt_node_to_map(struct pinctrl_dev *pctldev,
> new_map[i].type = PIN_MAP_TYPE_CONFIGS_PIN;
> new_map[i].data.configs.group_or_pin =
> pin_get_name(pctldev, grp->pins[i]);
> - new_map[i].data.configs.configs = &grp->pins_conf[i].conf;
> - new_map[i].data.configs.num_configs = 1;
> + if (!pctldev->desc->confops->is_generic) {
> + new_map[i].data.configs.configs =
> + &grp->pins_conf[i].conf.native;
> + new_map[i].data.configs.num_configs = 1;
> + } else {
> + new_map[i].data.configs.configs =
> + grp->pins_conf[i].conf.generic.configs;
> + new_map[i].data.configs.num_configs =
> + grp->pins_conf[i].conf.generic.nconfigs;
> + }
> }
>
> dev_dbg(pctldev->dev, "maps: function %s group %s num %d\n",
> @@ -325,7 +351,7 @@ static void at91_mux_disable_interrupt(void __iomem *pio, unsigned mask)
>
> static unsigned at91_mux_get_pullup(void __iomem *pio, unsigned pin)
> {
> - return (readl_relaxed(pio + PIO_PUSR) >> pin) & 0x1;
> + return !((readl_relaxed(pio + PIO_PUSR) >> pin) & 0x1);
> }
>
> 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;
> }
>
> +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, bool 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 __iomem *pio, unsigned mask,
>
> 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);
> }
>
> 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 = {
> static void at91_pin_dbg(const struct device *dev, const struct at91_pmx_pin *pin)
> {
> if (pin->mux) {
> - dev_dbg(dev, "pio%c%d configured as periph%c with conf = 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 = 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 == AT91_PINCONF_NATIVE)
> + dev_dbg(dev, " with conf = 0x%lu\n", pin->conf.native);
> + else
> + dev_dbg(dev, "\n");

do not change debug output
> }
>
> static int pin_check_config(struct at91_pinctrl *info, const char *name,
> @@ -782,6 +823,157 @@ static const struct pinconf_ops at91_pinconf_ops = {
> .pin_config_group_dbg_show = at91_pinconf_group_dbg_show,
> };
>
> +static int at91_generic_pinconf_get(struct pinctrl_dev *pctldev,
> + unsigned pin_id, unsigned long *config)
> +{
> + struct at91_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
> + enum pin_config_param param = pinconf_to_config_param(*config);
> + void __iomem *pio = pin_to_controller(info, pin_to_bank(pin_id));
> + unsigned pin = 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 = 0;
> + break;
> + case PIN_CONFIG_BIAS_PULL_UP:
> + if (!at91_mux_get_pullup(pio, pin))
> + return -EINVAL;
> +
> + *config = 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 = 0;
> + break;
> + case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> + if (!at91_mux_get_multidrive(pio, pin))
> + return -EINVAL;
> +
> + *config = 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 = 1;
> + else
> + *config = 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 = ((div + 1) * 2) * 1000000 / 32768;
> + if (*config > 0xffff)
> + *config = 0xffff;
> + } else
> + *config = 0;
> + break;
> + case PIN_CONFIG_INPUT_DEGLITCH:
> + if (!info->ops->get_deglitch)
> + return -ENOTSUPP;
> +
> + *config = info->ops->get_deglitch(pio, pin);
> + break;
> + case PIN_CONFIG_OUTPUT:
> + if (info->ops->get_periph(pio, pin) != AT91_MUX_GPIO)
> + return -EINVAL;
> +
> + *config = 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 = pinctrl_dev_get_drvdata(pctldev);
> + enum pin_config_param param = pinconf_to_config_param(config);
> + u16 arg = pinconf_to_config_argument(config);
> + u32 div = 0;
> + unsigned pin = pin_id % MAX_NB_GPIO_PER_BANK;
> + unsigned mask = pin_to_mask(pin);
> + void __iomem *pio = 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 = ~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 = (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) != 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 = {
> + .is_generic = true,
> + .pin_config_get = at91_generic_pinconf_get,
> + .pin_config_set = at91_generic_pinconf_set,
> +};
> +
> static struct pinctrl_desc at91_pinctrl_desc = {
> .pctlops = &at91_pctrl_ops,
> .pmxops = &at91_pmx_ops,
> @@ -847,6 +1039,7 @@ static int at91_pinctrl_parse_groups(struct device_node *np,
> int size;
> const __be32 *list;
> int i, j;
> + int err;
>
> dev_dbg(info->dev, "group(%d): %s\n", index, np->name);
>
> @@ -870,21 +1063,48 @@ static int at91_pinctrl_parse_groups(struct device_node *np,
> GFP_KERNEL);
> grp->pins = devm_kzalloc(info->dev, grp->npins * sizeof(unsigned int),
> GFP_KERNEL);
> - if (!grp->pins_conf || !grp->pins)
> - return -ENOMEM;
> + if (!grp->pins_conf || !grp->pins) {
> + err = -ENOMEM;
> + goto out_err;
> + }

why ???
>
> for (i = 0, j = 0; i < size; i += 4, j++) {
> pin->bank = be32_to_cpu(*list++);
> pin->pin = be32_to_cpu(*list++);
> grp->pins[j] = pin->bank * MAX_NB_GPIO_PER_BANK + pin->pin;
> pin->mux = be32_to_cpu(*list++);
> - pin->conf = be32_to_cpu(*list++);
> + if (at91_pinctrl_desc.confops->is_generic) {
> + struct device_node *np_config;
> + const __be32 *phandle = list++;
> +
> + if (!phandle) {
> + err = -EINVAL;
> + goto out_err;
> + }
> + np_config =
> + of_find_node_by_phandle(be32_to_cpup(phandle));
> + pin->conftype = AT91_PINCONF_GENERIC;
> + err = pinconf_generic_parse_dt_config(np_config,
> + &pin->conf.generic.configs,
> + &pin->conf.generic.nconfigs);
> + if (err)
> + goto out_err;
> +
> + } else {
> + pin->conftype = AT91_PINCONF_NATIVE;
> + pin->conf.native = be32_to_cpu(*list++);
> + }
>
> at91_pin_dbg(info->dev, pin);
> pin++;
> }
>
> return 0;
> +
> +out_err:
> + kfree(grp->pins_conf);
> + kfree(grp->pins);
use devm and drop those kfree
> + return err;
> }
>
> static int at91_pinctrl_parse_functions(struct device_node *np,
> @@ -904,10 +1124,10 @@ static int at91_pinctrl_parse_functions(struct device_node *np,
> /* Initialise function */
> func->name = np->name;
> func->ngroups = of_get_child_count(np);
> - if (func->ngroups <= 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 <= 0)
> + return 0;
> +
> func->groups = 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_match[] = {
> { /* sentinel */ }
> };
>
> +static struct of_device_id at91_pinconf_of_match[] = {
> + { .compatible = "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 platform_device *pdev,
> info->dev = &pdev->dev;
> info->ops = (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 = &at91_generic_pinconf_ops;
> at91_pinctrl_child_count(info, np);
>
> if (info->nbanks < 1) {
> --
> 1.7.9.5
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/