From: Eric Anholt <eric@anholt.net> To: Matheus Castello <matheus@castello.eng.br>, stefan.wahren@i2se.com, linus.walleij@linaro.org Cc: linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, Matheus Castello <matheus@castello.eng.br> Subject: Re: [PATCH v2 2/3] pinctrl: bcm2835: Add support for generic pinctrl binding Date: Mon, 05 Mar 2018 15:21:26 -0800 [thread overview] Message-ID: <87d10irtvt.fsf@anholt.net> (raw) In-Reply-To: <1520216960-19880-3-git-send-email-matheus@castello.eng.br> [-- Attachment #1: Type: text/plain, Size: 6711 bytes --] Matheus Castello <matheus@castello.eng.br> writes: > To keep driver up to date we add generic pinctrl binding support, which covers > the features used in this driver and has additional node properties that this > SoC has compatibility, so enabling future implementations of these properties > without the need to create new node properties in the device trees. > > The logic of this change maintain the old brcm legacy binding support in order > to keep the ABI stable. > > Signed-off-by: Matheus Castello <matheus@castello.eng.br> > --- > > A brief explanation of what I did: > > Add pinconf-generic header for use the defines and pinctrl-generic API. > > Add dt-bindings pinctrl bcm2835 header to use functions selections and > pulls definitions, which functions definitions where duplicated in the > enum bcm2835_fsel, I removed the duplicate defines from enum. > > In the bcm2835_pctl_dt_node_to_map_pull I used the generic macro for > pack the legacy param and arguments, since it will be unpacked along with > generic properties that is packed with this same macro. > > In bcm2835_pctl_dt_node_to_map I thougt it was better, and simpler, to use > pinctrl-generic parse code instead of parsing it inside the driver, so code > first check for generic binding parse, if something is parsed then it is > assumed that are using the new generic style, and when nothing is found then > parse continues to search for legacy properties. > > In the bcm2835_pinconf_set was changed the unpack legacy by the generic, and > was added a switch for the parameter tests, since pinctrl generic uses 3 > properties to define the states of the pull instead of one with arguments, that > was the reason too that bcm2835_pull_config_set function was added, for reuse > the code that set state of pull. > > drivers/pinctrl/bcm/pinctrl-bcm2835.c | 83 +++++++++++++++++++++++------------ > 1 file changed, 54 insertions(+), 29 deletions(-) > > diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c > index 785c366..755ea90 100644 > --- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c > +++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c > @@ -36,11 +36,13 @@ > #include <linux/pinctrl/pinconf.h> > #include <linux/pinctrl/pinctrl.h> > #include <linux/pinctrl/pinmux.h> > +#include <linux/pinctrl/pinconf-generic.h> > #include <linux/platform_device.h> > #include <linux/seq_file.h> > #include <linux/slab.h> > #include <linux/spinlock.h> > #include <linux/types.h> > +#include <dt-bindings/pinctrl/bcm2835.h> > > #define MODULE_NAME "pinctrl-bcm2835" > #define BCM2835_NUM_GPIOS 54 > @@ -213,14 +215,6 @@ static const char * const bcm2835_gpio_groups[] = { > }; > > enum bcm2835_fsel { > - BCM2835_FSEL_GPIO_IN = 0, > - BCM2835_FSEL_GPIO_OUT = 1, > - BCM2835_FSEL_ALT0 = 4, > - BCM2835_FSEL_ALT1 = 5, > - BCM2835_FSEL_ALT2 = 6, > - BCM2835_FSEL_ALT3 = 7, > - BCM2835_FSEL_ALT4 = 3, > - BCM2835_FSEL_ALT5 = 2, > BCM2835_FSEL_COUNT = 8, > BCM2835_FSEL_MASK = 0x7, > }; > @@ -714,7 +708,7 @@ static int bcm2835_pctl_dt_node_to_map_pull(struct bcm2835_pinctrl *pc, > configs = kzalloc(sizeof(*configs), GFP_KERNEL); > if (!configs) > return -ENOMEM; > - configs[0] = BCM2835_PINCONF_PACK(BCM2835_PINCONF_PARAM_PULL, pull); > + configs[0] = PIN_CONF_PACKED(BCM2835_PINCONF_PARAM_PULL, pull); > > map->type = PIN_MAP_TYPE_CONFIGS_PIN; > map->data.configs.group_or_pin = bcm2835_gpio_pins[pin].name; > @@ -736,6 +730,12 @@ static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev, > int i, err; > u32 pin, func, pull; > > + /* Check for generic binding in this node */ > + err = pinconf_generic_dt_node_to_map_all(pctldev, np, map, num_maps); > + if (err || *num_maps) > + return err; > + > + /* Generic binding did not find anything continue with legacy parse */ > pins = of_find_property(np, "brcm,pins", NULL); > if (!pins) { > dev_err(pc->dev, "%pOF: missing brcm,pins property\n", np); > @@ -917,37 +917,62 @@ static int bcm2835_pinconf_get(struct pinctrl_dev *pctldev, > return -ENOTSUPP; > } > > +static void bcm2835_pull_config_set(struct bcm2835_pinctrl *pc, > + unsigned pin, unsigned arg) > +{ > + u32 off, bit; > + > + off = GPIO_REG_OFFSET(pin); > + bit = GPIO_REG_SHIFT(pin); > + > + bcm2835_gpio_wr(pc, GPPUD, arg & 3); > + /* > + * BCM2835 datasheet say to wait 150 cycles, but not of what. > + * But the VideoCore firmware delay for this operation > + * based nearly on the same amount of VPU cycles and this clock > + * runs at 250 MHz. > + */ > + udelay(1); > + bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), BIT(bit)); > + udelay(1); > + bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), 0); > +} > + > static int bcm2835_pinconf_set(struct pinctrl_dev *pctldev, > unsigned pin, unsigned long *configs, > unsigned num_configs) > { > struct bcm2835_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev); > - enum bcm2835_pinconf_param param; > - u16 arg; > - u32 off, bit; > + u16 param, arg; > int i; > > for (i = 0; i < num_configs; i++) { > - param = BCM2835_PINCONF_UNPACK_PARAM(configs[i]); > - arg = BCM2835_PINCONF_UNPACK_ARG(configs[i]); > + param = configs[i] & 0xffUL; > + arg = configs[i] >> 8; If you're removing the usage of BCM2835_PINCONF_PACK() and UNPACK, let's delete those macros, too. Also, it looks like there's "pinconf_to_config_param()" for unpacking, instead of rolling your own. Other than that, it looks good to me. I'll let Linus comment on whether this is handling the generic properties correctly. > > - if (param != BCM2835_PINCONF_PARAM_PULL) > - return -EINVAL; > + switch (param) { > + /* Set legacy brcm,pull */ > + case BCM2835_PINCONF_PARAM_PULL: > + bcm2835_pull_config_set(pc, pin, arg); > + break; > > - off = GPIO_REG_OFFSET(pin); > - bit = GPIO_REG_SHIFT(pin); > + /* Set pull generic bindings */ > + case PIN_CONFIG_BIAS_DISABLE: > + bcm2835_pull_config_set(pc, pin, BCM2835_PUD_OFF); > + break; > > - bcm2835_gpio_wr(pc, GPPUD, arg & 3); > - /* > - * BCM2835 datasheet say to wait 150 cycles, but not of what. > - * But the VideoCore firmware delay for this operation > - * based nearly on the same amount of VPU cycles and this clock > - * runs at 250 MHz. > - */ > - udelay(1); > - bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), BIT(bit)); > - udelay(1); > - bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), 0); > + case PIN_CONFIG_BIAS_PULL_DOWN: > + bcm2835_pull_config_set(pc, pin, BCM2835_PUD_DOWN); > + break; > + > + case PIN_CONFIG_BIAS_PULL_UP: > + bcm2835_pull_config_set(pc, pin, BCM2835_PUD_UP); > + break; > + > + default: > + return -EINVAL; > + > + } /* switch param type */ > } /* for each config */ > > return 0; > -- > 2.7.4 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: Eric Anholt <eric@anholt.net> To: stefan.wahren@i2se.com, linus.walleij@linaro.org Cc: linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, Matheus Castello <matheus@castello.eng.br> Subject: Re: [PATCH v2 2/3] pinctrl: bcm2835: Add support for generic pinctrl binding Date: Mon, 05 Mar 2018 15:21:26 -0800 [thread overview] Message-ID: <87d10irtvt.fsf@anholt.net> (raw) In-Reply-To: <1520216960-19880-3-git-send-email-matheus@castello.eng.br> [-- Attachment #1: Type: text/plain, Size: 6711 bytes --] Matheus Castello <matheus@castello.eng.br> writes: > To keep driver up to date we add generic pinctrl binding support, which covers > the features used in this driver and has additional node properties that this > SoC has compatibility, so enabling future implementations of these properties > without the need to create new node properties in the device trees. > > The logic of this change maintain the old brcm legacy binding support in order > to keep the ABI stable. > > Signed-off-by: Matheus Castello <matheus@castello.eng.br> > --- > > A brief explanation of what I did: > > Add pinconf-generic header for use the defines and pinctrl-generic API. > > Add dt-bindings pinctrl bcm2835 header to use functions selections and > pulls definitions, which functions definitions where duplicated in the > enum bcm2835_fsel, I removed the duplicate defines from enum. > > In the bcm2835_pctl_dt_node_to_map_pull I used the generic macro for > pack the legacy param and arguments, since it will be unpacked along with > generic properties that is packed with this same macro. > > In bcm2835_pctl_dt_node_to_map I thougt it was better, and simpler, to use > pinctrl-generic parse code instead of parsing it inside the driver, so code > first check for generic binding parse, if something is parsed then it is > assumed that are using the new generic style, and when nothing is found then > parse continues to search for legacy properties. > > In the bcm2835_pinconf_set was changed the unpack legacy by the generic, and > was added a switch for the parameter tests, since pinctrl generic uses 3 > properties to define the states of the pull instead of one with arguments, that > was the reason too that bcm2835_pull_config_set function was added, for reuse > the code that set state of pull. > > drivers/pinctrl/bcm/pinctrl-bcm2835.c | 83 +++++++++++++++++++++++------------ > 1 file changed, 54 insertions(+), 29 deletions(-) > > diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c > index 785c366..755ea90 100644 > --- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c > +++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c > @@ -36,11 +36,13 @@ > #include <linux/pinctrl/pinconf.h> > #include <linux/pinctrl/pinctrl.h> > #include <linux/pinctrl/pinmux.h> > +#include <linux/pinctrl/pinconf-generic.h> > #include <linux/platform_device.h> > #include <linux/seq_file.h> > #include <linux/slab.h> > #include <linux/spinlock.h> > #include <linux/types.h> > +#include <dt-bindings/pinctrl/bcm2835.h> > > #define MODULE_NAME "pinctrl-bcm2835" > #define BCM2835_NUM_GPIOS 54 > @@ -213,14 +215,6 @@ static const char * const bcm2835_gpio_groups[] = { > }; > > enum bcm2835_fsel { > - BCM2835_FSEL_GPIO_IN = 0, > - BCM2835_FSEL_GPIO_OUT = 1, > - BCM2835_FSEL_ALT0 = 4, > - BCM2835_FSEL_ALT1 = 5, > - BCM2835_FSEL_ALT2 = 6, > - BCM2835_FSEL_ALT3 = 7, > - BCM2835_FSEL_ALT4 = 3, > - BCM2835_FSEL_ALT5 = 2, > BCM2835_FSEL_COUNT = 8, > BCM2835_FSEL_MASK = 0x7, > }; > @@ -714,7 +708,7 @@ static int bcm2835_pctl_dt_node_to_map_pull(struct bcm2835_pinctrl *pc, > configs = kzalloc(sizeof(*configs), GFP_KERNEL); > if (!configs) > return -ENOMEM; > - configs[0] = BCM2835_PINCONF_PACK(BCM2835_PINCONF_PARAM_PULL, pull); > + configs[0] = PIN_CONF_PACKED(BCM2835_PINCONF_PARAM_PULL, pull); > > map->type = PIN_MAP_TYPE_CONFIGS_PIN; > map->data.configs.group_or_pin = bcm2835_gpio_pins[pin].name; > @@ -736,6 +730,12 @@ static int bcm2835_pctl_dt_node_to_map(struct pinctrl_dev *pctldev, > int i, err; > u32 pin, func, pull; > > + /* Check for generic binding in this node */ > + err = pinconf_generic_dt_node_to_map_all(pctldev, np, map, num_maps); > + if (err || *num_maps) > + return err; > + > + /* Generic binding did not find anything continue with legacy parse */ > pins = of_find_property(np, "brcm,pins", NULL); > if (!pins) { > dev_err(pc->dev, "%pOF: missing brcm,pins property\n", np); > @@ -917,37 +917,62 @@ static int bcm2835_pinconf_get(struct pinctrl_dev *pctldev, > return -ENOTSUPP; > } > > +static void bcm2835_pull_config_set(struct bcm2835_pinctrl *pc, > + unsigned pin, unsigned arg) > +{ > + u32 off, bit; > + > + off = GPIO_REG_OFFSET(pin); > + bit = GPIO_REG_SHIFT(pin); > + > + bcm2835_gpio_wr(pc, GPPUD, arg & 3); > + /* > + * BCM2835 datasheet say to wait 150 cycles, but not of what. > + * But the VideoCore firmware delay for this operation > + * based nearly on the same amount of VPU cycles and this clock > + * runs at 250 MHz. > + */ > + udelay(1); > + bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), BIT(bit)); > + udelay(1); > + bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), 0); > +} > + > static int bcm2835_pinconf_set(struct pinctrl_dev *pctldev, > unsigned pin, unsigned long *configs, > unsigned num_configs) > { > struct bcm2835_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev); > - enum bcm2835_pinconf_param param; > - u16 arg; > - u32 off, bit; > + u16 param, arg; > int i; > > for (i = 0; i < num_configs; i++) { > - param = BCM2835_PINCONF_UNPACK_PARAM(configs[i]); > - arg = BCM2835_PINCONF_UNPACK_ARG(configs[i]); > + param = configs[i] & 0xffUL; > + arg = configs[i] >> 8; If you're removing the usage of BCM2835_PINCONF_PACK() and UNPACK, let's delete those macros, too. Also, it looks like there's "pinconf_to_config_param()" for unpacking, instead of rolling your own. Other than that, it looks good to me. I'll let Linus comment on whether this is handling the generic properties correctly. > > - if (param != BCM2835_PINCONF_PARAM_PULL) > - return -EINVAL; > + switch (param) { > + /* Set legacy brcm,pull */ > + case BCM2835_PINCONF_PARAM_PULL: > + bcm2835_pull_config_set(pc, pin, arg); > + break; > > - off = GPIO_REG_OFFSET(pin); > - bit = GPIO_REG_SHIFT(pin); > + /* Set pull generic bindings */ > + case PIN_CONFIG_BIAS_DISABLE: > + bcm2835_pull_config_set(pc, pin, BCM2835_PUD_OFF); > + break; > > - bcm2835_gpio_wr(pc, GPPUD, arg & 3); > - /* > - * BCM2835 datasheet say to wait 150 cycles, but not of what. > - * But the VideoCore firmware delay for this operation > - * based nearly on the same amount of VPU cycles and this clock > - * runs at 250 MHz. > - */ > - udelay(1); > - bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), BIT(bit)); > - udelay(1); > - bcm2835_gpio_wr(pc, GPPUDCLK0 + (off * 4), 0); > + case PIN_CONFIG_BIAS_PULL_DOWN: > + bcm2835_pull_config_set(pc, pin, BCM2835_PUD_DOWN); > + break; > + > + case PIN_CONFIG_BIAS_PULL_UP: > + bcm2835_pull_config_set(pc, pin, BCM2835_PUD_UP); > + break; > + > + default: > + return -EINVAL; > + > + } /* switch param type */ > } /* for each config */ > > return 0; > -- > 2.7.4 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --]
next prev parent reply other threads:[~2018-03-05 23:21 UTC|newest] Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-02-22 17:44 [PATCH] pinctrl: bc2835: Add brcm,level property Matheus Castello 2018-02-23 14:54 ` Eric Anholt 2018-02-23 15:58 ` Matheus Castello 2018-03-02 10:14 ` Linus Walleij 2018-03-03 14:21 ` Stefan Wahren 2018-03-05 2:29 ` [PATCH v2 0/3] pinctrl: bcm2835: " Matheus Castello 2018-03-05 2:29 ` [PATCH v2 1/3] pinctrl: bcm2835: switch to GENERIC_PINCONF Matheus Castello 2018-03-07 11:51 ` Stefan Wahren 2018-03-05 2:29 ` [PATCH v2 2/3] pinctrl: bcm2835: Add support for generic pinctrl binding Matheus Castello 2018-03-05 23:21 ` Eric Anholt [this message] 2018-03-05 23:21 ` Eric Anholt 2018-03-07 3:28 ` Matheus Castello 2018-03-05 2:29 ` [PATCH v2 3/3] pinctrl: bcm2835: Add support for output-low output-high properties Matheus Castello 2018-03-07 11:58 ` [PATCH v2 0/3] pinctrl: bcm2835: Add brcm,level property Stefan Wahren 2018-03-08 0:12 ` Matheus Castello 2018-03-08 8:00 ` Stefan Wahren 2018-03-08 19:47 ` Matheus Castello 2018-03-09 4:16 ` [PATCH v3 " Matheus Castello 2018-03-09 4:16 ` [PATCH v3 1/3] pinctrl: bcm2835: Add support for generic pinctrl binding Matheus Castello 2018-03-09 4:16 ` [PATCH v3 2/3] pinctrl: bcm2835: Add support for output-low output-high properties Matheus Castello 2018-03-09 4:16 ` [PATCH v3 3/3] pinctrl: bcm2835: Update docs about generic pinctrl bindings support Matheus Castello 2018-03-09 11:46 ` Stefan Wahren 2018-03-09 17:13 ` [PATCH v4 0/3] pinctrl: bcm2835: Add generic pinctrl support Matheus Castello 2018-03-09 17:13 ` [PATCH v4 1/3] dt-bindings: pinctrl: bcm2835-gpio: " Matheus Castello 2018-03-18 12:48 ` Rob Herring 2018-03-18 15:23 ` Matheus Castello 2018-04-11 4:58 ` [PATCH v5 0/3] pinctrl: bcm2835: " Matheus Castello 2018-04-11 4:58 ` [PATCH v5 1/3] dt-bindings: pinctrl: bcm2835-gpio: " Matheus Castello 2018-04-11 16:02 ` Eric Anholt 2018-04-11 16:02 ` Eric Anholt 2018-04-16 15:06 ` Rob Herring 2018-04-26 12:21 ` Linus Walleij 2018-04-11 4:58 ` [PATCH v5 2/3] pinctrl: bcm2835: Add support for generic pinctrl binding Matheus Castello 2018-04-21 10:30 ` Stefan Wahren 2018-04-22 12:44 ` Stefan Wahren 2018-04-26 12:24 ` Linus Walleij 2018-04-11 4:58 ` [PATCH v5 3/3] pinctrl: bcm2835: Add support for output-low output-high properties Matheus Castello 2018-04-26 12:25 ` Linus Walleij 2018-04-29 19:28 ` [PATCH v6 0/3] bcm2835: Add generic pinctrl support Matheus Castello 2018-04-29 19:28 ` [PATCH v6 2/3] pinctrl: bcm2835: Add support for generic pinctrl binding Matheus Castello 2018-04-29 19:28 ` [PATCH v6 3/3] pinctrl: bcm2835: Add support for output-low output-high properties Matheus Castello 2018-05-01 0:42 ` [PATCH v7 0/3] bcm2835: Add generic pinctrl support Matheus Castello 2018-05-01 0:42 ` [PATCH v7 2/3] pinctrl: bcm2835: Add support for generic pinctrl binding Matheus Castello 2018-05-03 17:38 ` Stefan Wahren 2018-05-12 9:08 ` Stefan Wahren 2018-05-16 12:00 ` Linus Walleij 2018-05-01 0:42 ` [PATCH v7 3/3] pinctrl: bcm2835: Add support for output-low output-high properties Matheus Castello 2018-05-16 12:02 ` Linus Walleij 2018-03-09 17:13 ` [PATCH v4 2/3] pinctrl: bcm2835: Add support for generic pinctrl binding Matheus Castello 2018-03-12 18:03 ` Eric Anholt 2018-03-12 18:03 ` Eric Anholt 2018-03-09 17:14 ` [PATCH v4 3/3] pinctrl: bcm2835: Add support for output-low output-high properties Matheus Castello
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=87d10irtvt.fsf@anholt.net \ --to=eric@anholt.net \ --cc=devicetree@vger.kernel.org \ --cc=linus.walleij@linaro.org \ --cc=linux-kernel@vger.kernel.org \ --cc=matheus@castello.eng.br \ --cc=stefan.wahren@i2se.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.