Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1605893 > unrolled thread
| Started by | Gregory CLEMENT <gregory.clement@free-electrons.com> |
|---|---|
| First post | 2017-03-21 19:40 +0100 |
| Last post | 2017-03-22 12:50 +0100 |
| Articles | 7 — 2 participants |
Back to article view | Back to linux.kernel
[PATCH v2 0/7] Hi, Gregory CLEMENT <gregory.clement@free-electrons.com> - 2017-03-21 19:40 +0100
[PATCH v2 3/7] pinctrl: armada-37xx: Add pin controller support for Armada 37xx Gregory CLEMENT <gregory.clement@free-electrons.com> - 2017-03-21 19:40 +0100
Re: [PATCH v2 3/7] pinctrl: armada-37xx: Add pin controller support for Armada 37xx Linus Walleij <linus.walleij@linaro.org> - 2017-03-27 11:30 +0200
[PATCH v2 5/7] pinctrl: aramda-37xx: Add irqchip support Gregory CLEMENT <gregory.clement@free-electrons.com> - 2017-03-21 19:40 +0100
Re: [PATCH v2 5/7] pinctrl: aramda-37xx: Add irqchip support Linus Walleij <linus.walleij@linaro.org> - 2017-03-27 11:20 +0200
[PATCH v2 0/7] Add support for pinctrl/gpio on Armada 37xx Was Re: [PATCH v2 0/7] Hi, Gregory CLEMENT <gregory.clement@free-electrons.com> - 2017-03-21 22:00 +0100
Re: [PATCH v2 0/7] Add support for pinctrl/gpio on Armada 37xx Was Re: [PATCH v2 0/7] Hi, Gregory CLEMENT <gregory.clement@free-electrons.com> - 2017-03-22 12:50 +0100
| From | Gregory CLEMENT <gregory.clement@free-electrons.com> |
|---|---|
| Date | 2017-03-21 19:40 +0100 |
| Subject | [PATCH v2 0/7] Hi, |
| Message-ID | <tnwRj-779-5@gated-at.bofh.it> |
this series add support for the pin and gpio controllers present on the Armada 37xx SoCs. Each Armada 37xx SoC comes 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. The gpio controller is also capable to handle interrupt from gpio. Gregory Gregory CLEMENT (7): pinctrl: dt-bindings: Add documentation for Armada 37xx pin controllers arm64: marvell: enable the Armada 37xx pinctrl driver pinctrl: armada-37xx: Add pin controller support for Armada 37xx pinctrl: armada-37xx: Add gpio support pinctrl: aramda-37xx: Add irqchip support ARM64: dts: marvell: Add pinctrl nodes for Armada 3700 ARM64: dts: marvell: armada37xx: add pinctrl definition Documentation/devicetree/bindings/clock/armada3700-xtal-clock.txt | 7 +- Documentation/devicetree/bindings/pinctrl/marvell,armada-37xx-pinctrl.txt | 143 +++++++++++- arch/arm64/Kconfig.platforms | 2 +- arch/arm64/boot/dts/marvell/armada-3720-db.dts | 8 +- arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 71 +++++- drivers/pinctrl/Makefile | 2 +- drivers/pinctrl/mvebu/Kconfig | 7 +- drivers/pinctrl/mvebu/Makefile | 3 +- drivers/pinctrl/mvebu/pinctrl-armada-37xx.c | 918 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 9 files changed, 1153 insertions(+), 8 deletions(-) create mode 100644 Documentation/devicetree/bindings/pinctrl/marvell,armada-37xx-pinctrl.txt create mode 100644 drivers/pinctrl/mvebu/pinctrl-armada-37xx.c base-commit: c1ae3cfa0e89fa1a7ecc4c99031f5e9ae99d9201 -- git-series 0.9.1
[toc] | [next] | [standalone]
| From | Gregory CLEMENT <gregory.clement@free-electrons.com> |
|---|---|
| Date | 2017-03-21 19:40 +0100 |
| Subject | [PATCH v2 3/7] pinctrl: armada-37xx: Add pin controller support for Armada 37xx |
| Message-ID | <tnwRk-779-35@gated-at.bofh.it> |
| In reply to | #1605893 |
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>
---
drivers/pinctrl/Makefile | 2 +-
drivers/pinctrl/mvebu/Kconfig | 7 +-
drivers/pinctrl/mvebu/Makefile | 3 +-
drivers/pinctrl/mvebu/pinctrl-armada-37xx.c | 605 +++++++++++++++++++++-
4 files changed, 615 insertions(+), 2 deletions(-)
create mode 100644 drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index a251f439626f..95080811f36f 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -44,7 +44,7 @@ obj-y += bcm/
obj-$(CONFIG_PINCTRL_BERLIN) += berlin/
obj-y += freescale/
obj-$(CONFIG_X86) += intel/
-obj-$(CONFIG_PINCTRL_MVEBU) += mvebu/
+obj-y += mvebu/
obj-y += nomadik/
obj-$(CONFIG_PINCTRL_PXA) += pxa/
obj-$(CONFIG_ARCH_QCOM) += qcom/
diff --git a/drivers/pinctrl/mvebu/Kconfig b/drivers/pinctrl/mvebu/Kconfig
index 170602407c0d..5bade32d3089 100644
--- a/drivers/pinctrl/mvebu/Kconfig
+++ b/drivers/pinctrl/mvebu/Kconfig
@@ -39,3 +39,10 @@ config PINCTRL_ORION
select PINCTRL_MVEBU
endif
+
+config PINCTRL_ARMADA_37XX
+ bool
+ select GENERIC_PINCONF
+ select MFD_SYSCON
+ select PINCONF
+ select PINMUX
diff --git a/drivers/pinctrl/mvebu/Makefile b/drivers/pinctrl/mvebu/Makefile
index 18270cd5ea43..60c245a60f39 100644
--- a/drivers/pinctrl/mvebu/Makefile
+++ b/drivers/pinctrl/mvebu/Makefile
@@ -1,4 +1,4 @@
-obj-y += pinctrl-mvebu.o
+obj-$(CONFIG_PINCTRL_MVEBU) += pinctrl-mvebu.o
obj-$(CONFIG_PINCTRL_DOVE) += pinctrl-dove.o
obj-$(CONFIG_PINCTRL_KIRKWOOD) += pinctrl-kirkwood.o
obj-$(CONFIG_PINCTRL_ARMADA_370) += pinctrl-armada-370.o
@@ -6,4 +6,5 @@ obj-$(CONFIG_PINCTRL_ARMADA_375) += pinctrl-armada-375.o
obj-$(CONFIG_PINCTRL_ARMADA_38X) += pinctrl-armada-38x.o
obj-$(CONFIG_PINCTRL_ARMADA_39X) += pinctrl-armada-39x.o
obj-$(CONFIG_PINCTRL_ARMADA_XP) += pinctrl-armada-xp.o
+obj-$(CONFIG_PINCTRL_ARMADA_37XX) += pinctrl-armada-37xx.o
obj-$(CONFIG_PINCTRL_ORION) += pinctrl-orion.o
diff --git a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
new file mode 100644
index 000000000000..98cde04e07e1
--- /dev/null
+++ b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
@@ -0,0 +1,605 @@
+/*
+ * Marvell 37xx SoC pinctrl driver
+ *
+ * Copyright (C) 2017 Marvell
+ *
+ * Gregory CLEMENT <gregory.clement@free-electrons.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2 or later. This program is licensed "as is"
+ * without any warranty of any kind, whether express or implied.
+ */
+
+#include <linux/mfd/syscon.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#include "../pinctrl-utils.h"
+
+#define OUTPUT_EN 0x0
+#define OUTPUT_CTL 0x20
+#define SELECTION 0x30
+
+static int global_pin;
+
+#define NB_FUNCS 2
+#define GPIO_PER_REG 32
+
+/**
+ * struct armada_37xx_pin_group: represents group of pins of a pinmux function.
+ * The pins of a pinmux groups are composed of one or two groups of contiguous
+ * pins.
+ * @name: Name of the pin group, used to lookup the group.
+ * @start_pins: Index of the first pin of the main range of pins belonging to
+ * the group
+ * @npins: Number of pins included in the first range
+ * @reg_mask: Bit mask matching the group in the selection register
+ * @extra_pins: Index of the first pin of the optional second range of pins
+ * belonging to the group
+ * @npins: Number of pins included in the second optional range
+ * @funcs: A list of pinmux functions that can be selected for this group.
+ * @pins: Total number pins included in the group
+ */
+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;
+};
+
+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;
+};
+
+#define PIN_GRP(_name, _start, _nr, _mask, _func1, _func2) \
+ { \
+ .name = _name, \
+ .start_pin = _start, \
+ .npins = _nr, \
+ .reg_mask = _mask, \
+ .funcs = {_func1, _func2} \
+ }
+
+#define PIN_GRP_GPIO(_name, _start, _nr, _mask, _func1) \
+ { \
+ .name = _name, \
+ .start_pin = _start, \
+ .npins = _nr, \
+ .reg_mask = _mask, \
+ .funcs = {_func1, "gpio"} \
+ }
+
+#define PIN_GRP_EXTRA(_name, _start, _nr, _mask, _start2, _nr2, _f1, _f2) \
+ { \
+ .name = _name, \
+ .start_pin = _start, \
+ .npins = _nr, \
+ .reg_mask = _mask, \
+ .extra_pin = _start2, \
+ .extra_npins = _nr2, \
+ .funcs = {_f1, _f2} \
+ }
+
+static struct armada_37xx_pin_group armada_37xx_nb_groups[] = {
+ PIN_GRP_GPIO("jtag", 20, 5, BIT(0), "jtag"),
+ PIN_GRP_GPIO("sdio0", 8, 3, BIT(1), "sdio"),
+ PIN_GRP_GPIO("emmc_nb", 27, 9, BIT(2), "emmc"),
+ PIN_GRP_GPIO("pwm0", 11, 1, BIT(3), "pwm"),
+ PIN_GRP_GPIO("pwm1", 12, 1, BIT(4), "pwm"),
+ PIN_GRP_GPIO("pwm2", 13, 1, BIT(5), "pwm"),
+ PIN_GRP_GPIO("pwm3", 14, 1, BIT(6), "pwm"),
+ PIN_GRP_GPIO("pmic1", 17, 1, BIT(7), "pmic"),
+ PIN_GRP_GPIO("pmic0", 16, 1, BIT(8), "pmic"),
+ PIN_GRP_GPIO("i2c2", 2, 2, BIT(9), "i2c"),
+ PIN_GRP_GPIO("i2c1", 0, 2, BIT(10), "i2c"),
+ PIN_GRP_GPIO("spi_cs1", 17, 1, BIT(12), "spi"),
+ PIN_GRP_GPIO("spi_cs2", 18, 1, BIT(13), "spi"),
+ PIN_GRP_GPIO("spi_cs3", 19, 1, BIT(14), "spi"),
+ PIN_GRP_GPIO("onewire", 4, 1, BIT(16), "onewire"),
+ PIN_GRP_GPIO("uart1", 25, 2, BIT(17), "uart"),
+ PIN_GRP_GPIO("spi_quad", 15, 2, BIT(18), "spi"),
+ PIN_GRP_EXTRA("uart_2", 9, 2, BIT(19), 18, 2, "gpio", "uart"),
+};
+
+static struct armada_37xx_pin_group armada_37xx_sb_groups[] = {
+ PIN_GRP_GPIO("usb32_drvvbus0", 0, 1, BIT(0), "drvbus"),
+ PIN_GRP_GPIO("usb2_drvvbus1", 1, 1, BIT(1), "drvbus"),
+ PIN_GRP_GPIO("sdio_sb", 24, 5, BIT(2), "sdio"),
+ PIN_GRP_EXTRA("rgmii", 6, 14, BIT(3), 23, 1, "mii", "gpio"),
+ PIN_GRP_GPIO("pcie1", 3, 2, BIT(4), "pcie"),
+ PIN_GRP_GPIO("ptp", 20, 3, BIT(5), "ptp"),
+ PIN_GRP("ptp_clk", 21, 1, BIT(6), "ptp", "mii"),
+ PIN_GRP("ptp_trig", 22, 1, BIT(7), "ptp", "mii"),
+ PIN_GRP("mii_col", 23, 1, BIT(8), "mii", "mii_err"),
+};
+
+struct armada_37xx_pin_data armada_37xx_pin_nb = {
+ .nr_pins = 36,
+ .name = "GPIO1",
+ .groups = armada_37xx_nb_groups,
+ .ngroups = ARRAY_SIZE(armada_37xx_nb_groups),
+};
+
+struct armada_37xx_pin_data armada_37xx_pin_sb = {
+ .nr_pins = 29,
+ .name = "GPIO2",
+ .groups = armada_37xx_sb_groups,
+ .ngroups = ARRAY_SIZE(armada_37xx_sb_groups),
+};
+
+static int armada_37xx_get_func_reg(struct armada_37xx_pin_group *grp,
+ const char *func)
+{
+ int f;
+
+ for (f = 0; f < NB_FUNCS; f++)
+ if (!strcmp(grp->funcs[f], func))
+ return f;
+
+ return -ENOTSUPP;
+}
+
+static struct armada_37xx_pin_group *armada_37xx_find_next_grp_by_pin(
+ struct armada_37xx_pinctrl *info, int pin, int *grp)
+{
+ while (*grp < info->ngroups) {
+ struct armada_37xx_pin_group *group = &info->groups[*grp];
+ int j;
+
+ *grp = *grp + 1;
+ for (j = 0; j < (group->npins + group->extra_npins); j++)
+ if (group->pins[j] == pin)
+ return group;
+ }
+ return NULL;
+}
+
+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,
+};
+
+static int armada_37xx_get_groups_count(struct pinctrl_dev *pctldev)
+{
+ struct armada_37xx_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+
+ return info->ngroups;
+}
+
+static const char *armada_37xx_get_group_name(struct pinctrl_dev *pctldev,
+ unsigned int group)
+{
+ struct armada_37xx_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+
+ return info->groups[group].name;
+}
+
+static int armada_37xx_get_group_pins(struct pinctrl_dev *pctldev,
+ unsigned int selector,
+ const unsigned int **pins,
+ unsigned int *npins)
+{
+ struct armada_37xx_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+
+ if (selector >= info->ngroups)
+ return -EINVAL;
+
+ *pins = info->groups[selector].pins;
+ *npins = info->groups[selector].npins;
+
+ return 0;
+}
+
+static const struct pinctrl_ops armada_37xx_pctrl_ops = {
+ .get_groups_count = armada_37xx_get_groups_count,
+ .get_group_name = armada_37xx_get_group_name,
+ .get_group_pins = armada_37xx_get_group_pins,
+ .dt_node_to_map = pinconf_generic_dt_node_to_map_group,
+ .dt_free_map = pinctrl_utils_free_map,
+};
+
+/*
+ * Pinmux_ops handling
+ */
+
+static int armada_37xx_pmx_get_funcs_count(struct pinctrl_dev *pctldev)
+{
+ struct armada_37xx_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+
+ return info->nfuncs;
+}
+
+static const char *armada_37xx_pmx_get_func_name(struct pinctrl_dev *pctldev,
+ unsigned int selector)
+{
+ struct armada_37xx_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+
+ return info->funcs[selector].name;
+}
+
+static int armada_37xx_pmx_get_groups(struct pinctrl_dev *pctldev,
+ unsigned int selector,
+ const char * const **groups,
+ unsigned int * const num_groups)
+{
+ struct armada_37xx_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+
+ *groups = info->funcs[selector].groups;
+ *num_groups = info->funcs[selector].ngroups;
+
+ return 0;
+}
+
+static int armada_37xx_pmx_set_by_name(struct pinctrl_dev *pctldev,
+ const char *name,
+ struct armada_37xx_pin_group *grp)
+{
+ struct armada_37xx_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+ unsigned int reg = SELECTION;
+ unsigned int mask = grp->reg_mask;
+ int ret, val;
+
+ dev_dbg(info->dev, "enable function %s group %s\n",
+ name, grp->name);
+
+ ret = armada_37xx_get_func_reg(grp, name);
+
+ if (ret < 0)
+ return ret;
+
+ val = ret ? mask : 0;
+
+ regmap_update_bits(info->regmap, reg, mask, val);
+
+ return 0;
+}
+
+static int armada_37xx_pmx_set(struct pinctrl_dev *pctldev,
+ unsigned int selector,
+ unsigned int group)
+{
+
+ struct armada_37xx_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+ struct armada_37xx_pin_group *grp = &info->groups[group];
+ const char *name = info->funcs[selector].name;
+
+ return armada_37xx_pmx_set_by_name(pctldev, name, grp);
+}
+
+static int armada_37xx_pmx_direction_input(struct armada_37xx_pinctrl *info,
+ unsigned int offset)
+{
+ unsigned int reg = OUTPUT_EN;
+ unsigned int mask;
+
+ if (offset >= GPIO_PER_REG) {
+ offset -= GPIO_PER_REG;
+ reg += sizeof(u32);
+ }
+ mask = BIT(offset);
+
+ return regmap_update_bits(info->regmap, reg, mask, 0);
+}
+
+
+
+static int armada_37xx_pmx_direction_output(struct armada_37xx_pinctrl *info,
+ unsigned int offset, int value)
+{
+ unsigned int reg = OUTPUT_EN;
+ unsigned int mask;
+
+ if (offset >= GPIO_PER_REG) {
+ offset -= GPIO_PER_REG;
+ reg += sizeof(u32);
+ }
+ mask = BIT(offset);
+
+ return regmap_update_bits(info->regmap, reg, mask, mask);
+}
+
+static int armada_37xx_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
+ struct pinctrl_gpio_range *range,
+ unsigned int offset, bool input)
+{
+ struct armada_37xx_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+
+ dev_dbg(info->dev, "gpio_direction for pin %u as %s-%d to %s\n",
+ offset, range->name, offset, input ? "input" : "output");
+
+ if (input)
+ armada_37xx_pmx_direction_input(info, offset);
+ else
+ armada_37xx_pmx_direction_output(info, offset, 0);
+
+ return 0;
+}
+
+static int armada_37xx_gpio_request_enable(struct pinctrl_dev *pctldev,
+ struct pinctrl_gpio_range *range,
+ unsigned int offset)
+{
+ struct armada_37xx_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+ struct armada_37xx_pin_group *group;
+ int grp = 0;
+
+ dev_dbg(info->dev, "requesting gpio %d\n", offset);
+
+ while ((group = armada_37xx_find_next_grp_by_pin(info, offset, &grp)))
+ armada_37xx_pmx_set_by_name(pctldev, "gpio", group);
+
+ return 0;
+}
+
+static const struct pinmux_ops armada_37xx_pmx_ops = {
+ .get_functions_count = armada_37xx_pmx_get_funcs_count,
+ .get_function_name = armada_37xx_pmx_get_func_name,
+ .get_function_groups = armada_37xx_pmx_get_groups,
+ .set_mux = armada_37xx_pmx_set,
+ .gpio_request_enable = armada_37xx_gpio_request_enable,
+ .gpio_set_direction = armada_37xx_pmx_gpio_set_direction,
+};
+
+static int armada_37xx_add_function(struct armada_37xx_pmx_func *funcs,
+ int *funcsize, const char *name)
+{
+ int i = 0;
+
+ if (*funcsize <= 0)
+ return -EOVERFLOW;
+
+ while (funcs->ngroups) {
+ /* function already there */
+ if (strcmp(funcs->name, name) == 0) {
+ funcs->ngroups++;
+
+ return -EEXIST;
+ }
+ funcs++;
+ i++;
+ }
+
+ /* append new unique function */
+ funcs->name = name;
+ funcs->ngroups = 1;
+ (*funcsize)--;
+
+ return 0;
+}
+
+static int armada_37xx_fill_group(struct armada_37xx_pinctrl *info, int base)
+{
+ int n, num = 0, funcsize = info->data->nr_pins;
+
+ for (n = 0; n < info->ngroups; n++) {
+ struct armada_37xx_pin_group *grp = &info->groups[n];
+ int i, j, f;
+
+ grp->pins = devm_kzalloc(info->dev,
+ (grp->npins + grp->extra_npins) *
+ sizeof(*grp->pins), GFP_KERNEL);
+ if (!grp->pins)
+ return -ENOMEM;
+
+ for (i = 0; i < grp->npins; i++)
+ grp->pins[i] = grp->start_pin + base + i;
+
+ for (j = 0; j < grp->extra_npins; j++)
+ grp->pins[i+j] = grp->extra_pin + base + j;
+
+ for (f = 0; f < NB_FUNCS; f++) {
+ int ret;
+ /* check for unique functions and count groups */
+ ret = armada_37xx_add_function(info->funcs, &funcsize,
+ grp->funcs[f]);
+ if (ret == -EOVERFLOW)
+ dev_err(info->dev,
+ "More functions than pins(%d)\n",
+ info->data->nr_pins);
+ if (ret < 0)
+ continue;
+ num++;
+ }
+ }
+
+ info->nfuncs = num;
+
+ return 0;
+}
+
+static int armada_37xx_fill_func(struct armada_37xx_pinctrl *info)
+{
+ struct armada_37xx_pmx_func *funcs = info->funcs;
+ int n;
+
+ for (n = 0; n < info->nfuncs; n++) {
+ const char *name = funcs[n].name;
+ const char **groups;
+ int g;
+
+ funcs[n].groups = devm_kzalloc(info->dev, funcs[n].ngroups *
+ sizeof(*(funcs[n].groups)),
+ GFP_KERNEL);
+ if (!funcs[n].groups)
+ return -ENOMEM;
+
+ groups = funcs[n].groups;
+
+ for (g = 0; g < info->ngroups; g++) {
+ struct armada_37xx_pin_group *gp = &info->groups[g];
+ int f;
+
+ for (f = 0; f < NB_FUNCS; f++) {
+ if (strcmp(gp->funcs[f], name) == 0) {
+ *groups = gp->name;
+ groups++;
+ }
+ }
+ }
+ }
+ return 0;
+}
+
+static int armada_37xx_pinctrl_register(struct platform_device *pdev,
+ struct armada_37xx_pinctrl *info)
+{
+ struct armada_37xx_pin_data *pin_data = info->data;
+ struct pinctrl_desc *ctrldesc = &info->pctl;
+ struct pinctrl_pin_desc *pindesc, *pdesc;
+ int base_pin = global_pin;
+ int pin, ret;
+
+ info->groups = pin_data->groups;
+ info->ngroups = pin_data->ngroups;
+
+ ctrldesc->name = "armada_37xx-pinctrl";
+ ctrldesc->owner = THIS_MODULE;
+ ctrldesc->pctlops = &armada_37xx_pctrl_ops;
+ ctrldesc->pmxops = &armada_37xx_pmx_ops;
+ ctrldesc->confops = &armada_37xx_pinconf_ops;
+
+ pindesc = devm_kzalloc(&pdev->dev, sizeof(*pindesc) *
+ pin_data->nr_pins, GFP_KERNEL);
+ if (!pindesc)
+ return -ENOMEM;
+
+ ctrldesc->pins = pindesc;
+ ctrldesc->npins = pin_data->nr_pins;
+
+ pdesc = pindesc;
+ for (pin = 0; pin < pin_data->nr_pins; pin++, global_pin++) {
+ pdesc->number = global_pin;
+ pdesc->name = kasprintf(GFP_KERNEL, "%s-%d",
+ pin_data->name, pin);
+ pdesc++;
+ }
+
+ /*
+ * we allocate functions for number of pins and hope there are
+ * fewer unique functions than pins available
+ */
+ info->funcs = devm_kzalloc(&pdev->dev, pin_data->nr_pins *
+ sizeof(struct armada_37xx_pmx_func), GFP_KERNEL);
+ if (!info->funcs)
+ return -ENOMEM;
+
+
+ ret = armada_37xx_fill_group(info, base_pin);
+ if (ret)
+ return ret;
+
+ ret = armada_37xx_fill_func(info);
+ if (ret)
+ return ret;
+
+ info->pctl_dev = devm_pinctrl_register(&pdev->dev, ctrldesc, info);
+ if (IS_ERR(info->pctl_dev)) {
+ dev_err(&pdev->dev, "could not register pinctrl driver\n");
+ return PTR_ERR(info->pctl_dev);
+ }
+
+ return 0;
+}
+
+static const struct of_device_id armada_37xx_pinctrl_of_match[] = {
+ {
+ .compatible = "marvell,armada3710-sb-pinctrl",
+ .data = (void *)&armada_37xx_pin_sb,
+ },
+ {
+ .compatible = "marvell,armada3710-nb-pinctrl",
+ .data = (void *)&armada_37xx_pin_nb,
+ },
+ { },
+};
+
+static int armada_37xx_pinctrl_probe(struct platform_device *pdev)
+{
+ const struct of_device_id *match;
+ struct armada_37xx_pinctrl *info;
+ struct device *dev = &pdev->dev;
+ struct device_node *np = dev->of_node;
+ struct regmap *regmap;
+ int ret;
+
+ info = devm_kzalloc(dev, sizeof(struct armada_37xx_pinctrl),
+ GFP_KERNEL);
+ if (!info)
+ return -ENOMEM;
+
+ info->dev = dev;
+
+ regmap = syscon_node_to_regmap(np);
+ if (IS_ERR(regmap)) {
+ dev_err(&pdev->dev, "cannot get regmap\n");
+ return PTR_ERR(regmap);
+ }
+ info->regmap = regmap;
+
+ match = of_match_node(armada_37xx_pinctrl_of_match, np);
+ info->data = (struct armada_37xx_pin_data *)match->data;
+
+ ret = armada_37xx_pinctrl_register(pdev, info);
+ if (ret)
+ return ret;
+
+ platform_set_drvdata(pdev, info);
+
+ return 0;
+}
+
+static struct platform_driver armada_37xx_pinctrl_driver = {
+ .driver = {
+ .name = "armada-37xx-pinctrl",
+ .of_match_table = armada_37xx_pinctrl_of_match,
+ },
+ .probe = armada_37xx_pinctrl_probe,
+};
+
+builtin_platform_driver(armada_37xx_pinctrl_driver);
--
git-series 0.9.1
[toc] | [prev] | [next] | [standalone]
| From | Linus Walleij <linus.walleij@linaro.org> |
|---|---|
| Date | 2017-03-27 11:30 +0200 |
| Subject | Re: [PATCH v2 3/7] pinctrl: armada-37xx: Add pin controller support for Armada 37xx |
| Message-ID | <tpz8l-5M6-13@gated-at.bofh.it> |
| In reply to | #1605897 |
On Tue, Mar 21, 2017 at 7:28 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.
Interesting!
> +static int armada_37xx_pmx_direction_input(struct armada_37xx_pinctrl *info,
> + unsigned int offset)
> +{
> + unsigned int reg = OUTPUT_EN;
> + unsigned int mask;
> +
> + if (offset >= GPIO_PER_REG) {
> + offset -= GPIO_PER_REG;
> + reg += sizeof(u32);
> + }
> + mask = BIT(offset);
> +
> + return regmap_update_bits(info->regmap, reg, mask, 0);
> +}
> +
> +
> +
A bit of excess whitespace, OK nitpicking.
Then this stuff:
> +static int armada_37xx_add_function(struct armada_37xx_pmx_func *funcs,
> + int *funcsize, const char *name)
> +{
> + int i = 0;
> +
> + if (*funcsize <= 0)
> + return -EOVERFLOW;
> +
> + while (funcs->ngroups) {
> + /* function already there */
> + if (strcmp(funcs->name, name) == 0) {
> + funcs->ngroups++;
> +
> + return -EEXIST;
> + }
> + funcs++;
> + i++;
> + }
> +
> + /* append new unique function */
> + funcs->name = name;
> + funcs->ngroups = 1;
> + (*funcsize)--;
> +
> + return 0;
> +}
> +
> +static int armada_37xx_fill_group(struct armada_37xx_pinctrl *info, int base)
> +{
> + int n, num = 0, funcsize = info->data->nr_pins;
> +
> + for (n = 0; n < info->ngroups; n++) {
> + struct armada_37xx_pin_group *grp = &info->groups[n];
> + int i, j, f;
> +
> + grp->pins = devm_kzalloc(info->dev,
> + (grp->npins + grp->extra_npins) *
> + sizeof(*grp->pins), GFP_KERNEL);
> + if (!grp->pins)
> + return -ENOMEM;
> +
> + for (i = 0; i < grp->npins; i++)
> + grp->pins[i] = grp->start_pin + base + i;
> +
> + for (j = 0; j < grp->extra_npins; j++)
> + grp->pins[i+j] = grp->extra_pin + base + j;
> +
> + for (f = 0; f < NB_FUNCS; f++) {
> + int ret;
> + /* check for unique functions and count groups */
> + ret = armada_37xx_add_function(info->funcs, &funcsize,
> + grp->funcs[f]);
> + if (ret == -EOVERFLOW)
> + dev_err(info->dev,
> + "More functions than pins(%d)\n",
> + info->data->nr_pins);
> + if (ret < 0)
> + continue;
> + num++;
> + }
> + }
> +
> + info->nfuncs = num;
> +
> + return 0;
> +}
> +
> +static int armada_37xx_fill_func(struct armada_37xx_pinctrl *info)
> +{
> + struct armada_37xx_pmx_func *funcs = info->funcs;
> + int n;
> +
> + for (n = 0; n < info->nfuncs; n++) {
> + const char *name = funcs[n].name;
> + const char **groups;
> + int g;
> +
> + funcs[n].groups = devm_kzalloc(info->dev, funcs[n].ngroups *
> + sizeof(*(funcs[n].groups)),
> + GFP_KERNEL);
> + if (!funcs[n].groups)
> + return -ENOMEM;
> +
> + groups = funcs[n].groups;
> +
> + for (g = 0; g < info->ngroups; g++) {
> + struct armada_37xx_pin_group *gp = &info->groups[g];
> + int f;
> +
> + for (f = 0; f < NB_FUNCS; f++) {
> + if (strcmp(gp->funcs[f], name) == 0) {
> + *groups = gp->name;
> + groups++;
> + }
> + }
> + }
> + }
> + return 0;
> +}
I would be happy if you add kerneldoc to these functions and explain
what they do. Because I don't get it. I guess they are filling in the data
structures but yeah. Hard to follow.
> + match = of_match_node(armada_37xx_pinctrl_of_match, np);
> + info->data = (struct armada_37xx_pin_data *)match->data;
Use of_device_get_match_data()
> +static struct platform_driver armada_37xx_pinctrl_driver = {
> + .driver = {
> + .name = "armada-37xx-pinctrl",
> + .of_match_table = armada_37xx_pinctrl_of_match,
> + },
> + .probe = armada_37xx_pinctrl_probe,
> +};
> +
> +builtin_platform_driver(armada_37xx_pinctrl_driver);
It almost looks like your could use builting_platform_driver_probe() actually,
and tag the costly initfunctions with __init so they get dicarded after probe.
Yours,
Linus Walleij
[toc] | [prev] | [next] | [standalone]
| From | Gregory CLEMENT <gregory.clement@free-electrons.com> |
|---|---|
| Date | 2017-03-21 19:40 +0100 |
| Subject | [PATCH v2 5/7] pinctrl: aramda-37xx: Add irqchip support |
| Message-ID | <tnwRk-779-47@gated-at.bofh.it> |
| In reply to | #1605893 |
The Armada 37xx SoCs can handle interrupt through GPIO. However it can
only manage the edge ones.
The way the interrupt are managed are classical so we can use the generic
interrupt chip model.
The only unusual "feature" is that many interrupts are connected to the
parent interrupt controller. But we do not take advantage of this and use
the chained irq with all of them.
Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
drivers/pinctrl/mvebu/pinctrl-armada-37xx.c | 223 ++++++++++++++++++++-
1 file changed, 216 insertions(+), 7 deletions(-)
diff --git a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
index e41e2d02aca7..5d3af27f1ba9 100644
--- a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
+++ b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
@@ -13,7 +13,9 @@
#include <linux/gpio/driver.h>
#include <linux/mfd/syscon.h>
#include <linux/of.h>
+#include <linux/of_address.h>
#include <linux/of_device.h>
+#include <linux/of_irq.h>
#include <linux/pinctrl/pinconf-generic.h>
#include <linux/pinctrl/pinconf.h>
#include <linux/pinctrl/pinctrl.h>
@@ -30,6 +32,11 @@
#define OUTPUT_CTL 0x20
#define SELECTION 0x30
+#define IRQ_EN 0x0
+#define IRQ_POL 0x08
+#define IRQ_STATUS 0x10
+#define IRQ_WKUP 0x18
+
static int global_pin;
#define NB_FUNCS 2
@@ -76,9 +83,12 @@ struct armada_37xx_pmx_func {
struct armada_37xx_pinctrl {
struct regmap *regmap;
+ void __iomem *base;
struct armada_37xx_pin_data *data;
struct device *dev;
struct gpio_chip gpio_chip;
+ struct irq_chip irq_chip;
+ spinlock_t irq_lock;
struct pinctrl_desc pctl;
struct pinctrl_dev *pctl_dev;
struct armada_37xx_pin_group *groups;
@@ -316,7 +326,7 @@ static int armada_37xx_pmx_set(struct pinctrl_dev *pctldev,
return armada_37xx_pmx_set_by_name(pctldev, name, grp);
}
-static inline void aramda_37xx_update_reg(unsigned int *reg,
+static inline void armada_37xx_update_reg(unsigned int *reg,
unsigned int offset)
{
/* We never have more than 2 registers */
@@ -326,6 +336,14 @@ static inline void aramda_37xx_update_reg(unsigned int *reg,
}
}
+static inline void armada_37xx_irq_update_reg(unsigned int *reg,
+ struct irq_data *d)
+{
+ int offset = irqd_to_hwirq(d);
+
+ armada_37xx_update_reg(reg, offset);
+}
+
static int armada_37xx_gpio_direction_input(struct gpio_chip *chip,
unsigned int offset)
{
@@ -333,7 +351,7 @@ static int armada_37xx_gpio_direction_input(struct gpio_chip *chip,
unsigned int reg = OUTPUT_EN;
unsigned int mask;
- aramda_37xx_update_reg(®, offset);
+ armada_37xx_update_reg(®, offset);
mask = BIT(offset);
return regmap_update_bits(info->regmap, reg, mask, 0);
@@ -346,7 +364,7 @@ static int armada_37xx_gpio_get_direction(struct gpio_chip *chip,
unsigned int reg = OUTPUT_EN;
unsigned int val, mask;
- aramda_37xx_update_reg(®, offset);
+ armada_37xx_update_reg(®, offset);
mask = BIT(offset);
regmap_read(info->regmap, reg, &val);
@@ -361,7 +379,7 @@ static int armada_37xx_gpio_direction_output(struct gpio_chip *chip,
unsigned int reg = OUTPUT_EN;
unsigned int mask;
- aramda_37xx_update_reg(®, offset);
+ armada_37xx_update_reg(®, offset);
mask = BIT(offset);
return regmap_update_bits(info->regmap, reg, mask, mask);
@@ -373,7 +391,7 @@ static int armada_37xx_gpio_get(struct gpio_chip *chip, unsigned int offset)
unsigned int reg = INPUT_VAL;
unsigned int val, mask;
- aramda_37xx_update_reg(®, offset);
+ armada_37xx_update_reg(®, offset);
mask = BIT(offset);
regmap_read(info->regmap, reg, &val);
@@ -388,7 +406,7 @@ static void armada_37xx_gpio_set(struct gpio_chip *chip, unsigned int offset,
unsigned int reg = OUTPUT_VAL;
unsigned int mask, val;
- aramda_37xx_update_reg(®, offset);
+ armada_37xx_update_reg(®, offset);
mask = BIT(offset);
val = value ? mask : 0;
@@ -449,6 +467,194 @@ static const struct gpio_chip armada_37xx_gpiolib_chip = {
.owner = THIS_MODULE,
};
+void armada_37xx_irq_ack(struct irq_data *d)
+{
+ struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+ struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
+ u32 reg = IRQ_STATUS, mask = d->mask;
+ unsigned long flags;
+
+ armada_37xx_irq_update_reg(®, d);
+ spin_lock_irqsave(&info->irq_lock, flags);
+ writel(mask, info->base + reg);
+ spin_unlock_irqrestore(&info->irq_lock, flags);
+}
+
+void armada_37xx_irq_mask(struct irq_data *d)
+{
+ struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+ struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
+ u32 val, reg = IRQ_EN, mask = d->mask;
+ unsigned long flags;
+
+ armada_37xx_irq_update_reg(®, d);
+ spin_lock_irqsave(&info->irq_lock, flags);
+ val = readl(info->base + reg);
+ writel(val & ~mask, info->base + reg);
+ spin_unlock_irqrestore(&info->irq_lock, flags);
+}
+
+void armada_37xx_irq_unmask(struct irq_data *d)
+{
+ struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+ struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
+ u32 val, reg = IRQ_EN, mask = d->mask;
+ unsigned long flags;
+
+ armada_37xx_irq_update_reg(®, d);
+ spin_lock_irqsave(&info->irq_lock, flags);
+ val = readl(info->base + reg);
+ writel(val | mask, info->base + reg);
+ spin_unlock_irqrestore(&info->irq_lock, flags);
+}
+
+static int armada_37xx_irq_set_wake(struct irq_data *d, unsigned int on)
+{
+ struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+ struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
+ u32 val, reg = IRQ_WKUP, mask = d->mask;
+ unsigned long flags;
+
+ armada_37xx_irq_update_reg(®, d);
+ spin_lock_irqsave(&info->irq_lock, flags);
+ val = readl(info->base + reg);
+ if (on)
+ val |= mask;
+ else
+ val &= ~mask;
+ writel(val, info->base + reg);
+ spin_unlock_irqrestore(&info->irq_lock, flags);
+
+ return 0;
+}
+
+static int armada_37xx_irq_set_type(struct irq_data *d, unsigned int type)
+{
+ struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+ struct armada_37xx_pinctrl *info = gpiochip_get_data(chip);
+ u32 val, reg = IRQ_POL, mask = d->mask;
+ unsigned long flags;
+
+ spin_lock_irqsave(&info->irq_lock, flags);
+ armada_37xx_irq_update_reg(®, d);
+ val = readl(info->base + reg);
+ switch (type) {
+ case IRQ_TYPE_EDGE_RISING:
+ val &= ~mask;
+ break;
+ case IRQ_TYPE_EDGE_FALLING:
+ val |= mask;
+ break;
+ default:
+ spin_unlock_irqrestore(&info->irq_lock, flags);
+ return -EINVAL;
+ }
+ writel(val, info->base + reg);
+ spin_unlock_irqrestore(&info->irq_lock, flags);
+
+ return 0;
+}
+
+
+static void armada_37xx_irq_handler(struct irq_desc *desc)
+{
+ struct gpio_chip *gc = irq_desc_get_handler_data(desc);
+ struct irq_chip *chip = irq_desc_get_chip(desc);
+ struct armada_37xx_pinctrl *info = gpiochip_get_data(gc);
+ struct irq_domain *d = gc->irqdomain;
+ int i;
+
+ chained_irq_enter(chip, desc);
+ for (i = 0; i <= d->revmap_size / GPIO_PER_REG; i++) {
+ u32 status;
+ unsigned long flags;
+
+ spin_lock_irqsave(&info->irq_lock, flags);
+ status = readl_relaxed(info->base + IRQ_STATUS + 4 * i);
+ /* Manage only the interrupt that was enabled */
+ status &= readl_relaxed(info->base + IRQ_EN + 4 * i);
+ spin_unlock_irqrestore(&info->irq_lock, flags);
+ while (status) {
+ u32 hwirq = ffs(status) - 1;
+ u32 virq = irq_linear_revmap(d, hwirq +
+ i * GPIO_PER_REG);
+
+ generic_handle_irq(virq);
+ status &= ~(1 << hwirq);
+ }
+ }
+ chained_irq_exit(chip, desc);
+}
+
+static int armada_37xx_irqchip_register(struct platform_device *pdev,
+ struct armada_37xx_pinctrl *info)
+{
+ struct device_node *np = info->dev->of_node;
+ int nrirqs = info->data->nr_pins;
+ struct gpio_chip *gc = &info->gpio_chip;
+ struct irq_chip *irqchip = &info->irq_chip;
+ struct resource res;
+ int ret, i, nr_irq_parent;
+
+ for_each_child_of_node(info->dev->of_node, np) {
+ if (of_find_property(np, "gpio-controller", NULL)) {
+ ret = 0;
+ break;
+ }
+ };
+ if (ret)
+ return ret;
+
+ nr_irq_parent = of_irq_count(np);
+ spin_lock_init(&info->irq_lock);
+
+ if (!nr_irq_parent) {
+ dev_err(&pdev->dev, "Invalid or no IRQ\n");
+ return 0;
+ }
+
+ if (of_address_to_resource(info->dev->of_node, 1, &res)) {
+ dev_err(info->dev, "cannot find IO resource\n");
+ return -ENOENT;
+ }
+
+ info->base = devm_ioremap_resource(info->dev, &res);
+ if (IS_ERR(info->base))
+ return PTR_ERR(info->base);
+
+ irqchip->irq_ack = armada_37xx_irq_ack;
+ irqchip->irq_mask = armada_37xx_irq_mask;
+ irqchip->irq_unmask = armada_37xx_irq_unmask;
+ irqchip->irq_set_wake = armada_37xx_irq_set_wake;
+ irqchip->irq_set_type = armada_37xx_irq_set_type;
+ irqchip->name = info->data->name;
+
+ ret = gpiochip_irqchip_add(gc, irqchip, 0,
+ handle_level_irq, IRQ_TYPE_NONE);
+ if (ret) {
+ dev_info(&pdev->dev, "could not add irqchip\n");
+ return ret;
+ }
+
+ for (i = 0; i < nrirqs; i++) {
+ struct irq_data *d = irq_get_irq_data(gc->irq_base + i);
+
+ d->mask = 1 << (i % GPIO_PER_REG);
+ }
+
+ for (i = 0; i < nr_irq_parent; i++) {
+ int irq = irq_of_parse_and_map(np, i);
+
+ if (irq < 0)
+ continue;
+
+ gpiochip_set_chained_irqchip(gc, irqchip, irq,
+ armada_37xx_irq_handler);
+ }
+
+ return 0;
+}
+
static int armada_37xx_gpiochip_register(struct platform_device *pdev,
struct armada_37xx_pinctrl *info)
{
@@ -477,6 +683,9 @@ static int armada_37xx_gpiochip_register(struct platform_device *pdev,
ret = gpiochip_add_data(gc, info);
if (ret)
return ret;
+ ret = armada_37xx_irqchip_register(pdev, info);
+ if (ret)
+ return ret;
return 0;
}
@@ -518,7 +727,7 @@ static int armada_37xx_fill_group(struct armada_37xx_pinctrl *info, int base)
grp->pins = devm_kzalloc(info->dev,
(grp->npins + grp->extra_npins) *
- sizeof(*grp->pins), GFP_KERNEL);
+ sizeof(*grp->pins), GFP_KERNEL);
if (!grp->pins)
return -ENOMEM;
--
git-series 0.9.1
[toc] | [prev] | [next] | [standalone]
| From | Linus Walleij <linus.walleij@linaro.org> |
|---|---|
| Date | 2017-03-27 11:20 +0200 |
| Subject | Re: [PATCH v2 5/7] pinctrl: aramda-37xx: Add irqchip support |
| Message-ID | <tpyYG-5I2-11@gated-at.bofh.it> |
| In reply to | #1605900 |
On Tue, Mar 21, 2017 at 7:28 PM, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:
> The Armada 37xx SoCs can handle interrupt through GPIO. However it can
> only manage the edge ones.
>
> The way the interrupt are managed are classical so we can use the generic
> interrupt chip model.
>
> The only unusual "feature" is that many interrupts are connected to the
> parent interrupt controller. But we do not take advantage of this and use
> the chained irq with all of them.
>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
You need something in your Kconfig
doing select GPIOLIB_IRQCHIP unless there is
something I miss here.
> +#define IRQ_EN 0x0
> +#define IRQ_POL 0x08
> +#define IRQ_STATUS 0x10
This just cries out to me that there is a register 0x0c
and I bet it handles edges vs levels so you could also implement
level IRQs. Am I right?
> - aramda_37xx_update_reg(®, offset);
> + armada_37xx_update_reg(®, offset);
(...)
> - aramda_37xx_update_reg(®, offset);
> + armada_37xx_update_reg(®, offset);
These spelling fixes, do not do them in this patch, fix the first
patch adding them
instead. It's super-confusing. Applies everywhere.
> +static void armada_37xx_irq_handler(struct irq_desc *desc)
> +{
> + struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> + struct irq_chip *chip = irq_desc_get_chip(desc);
> + struct armada_37xx_pinctrl *info = gpiochip_get_data(gc);
> + struct irq_domain *d = gc->irqdomain;
> + int i;
> +
> + chained_irq_enter(chip, desc);
> + for (i = 0; i <= d->revmap_size / GPIO_PER_REG; i++) {
> + u32 status;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&info->irq_lock, flags);
> + status = readl_relaxed(info->base + IRQ_STATUS + 4 * i);
> + /* Manage only the interrupt that was enabled */
> + status &= readl_relaxed(info->base + IRQ_EN + 4 * i);
> + spin_unlock_irqrestore(&info->irq_lock, flags);
> + while (status) {
> + u32 hwirq = ffs(status) - 1;
> + u32 virq = irq_linear_revmap(d, hwirq +
> + i * GPIO_PER_REG);
Use irq_find_mapping() instead please.
> + generic_handle_irq(virq);
> + status &= ~(1 << hwirq);
Why not status &= ~BIT(hwirq);
> + }
> + }
> + chained_irq_exit(chip, desc);
Apart from that nice, it re-reads status on every iteration which is
good.
> +static int armada_37xx_irqchip_register(struct platform_device *pdev,
> + struct armada_37xx_pinctrl *info)
> +{
> + struct device_node *np = info->dev->of_node;
> + int nrirqs = info->data->nr_pins;
> + struct gpio_chip *gc = &info->gpio_chip;
> + struct irq_chip *irqchip = &info->irq_chip;
> + struct resource res;
> + int ret, i, nr_irq_parent;
> +
> + for_each_child_of_node(info->dev->of_node, np) {
> + if (of_find_property(np, "gpio-controller", NULL)) {
> + ret = 0;
> + break;
> + }
> + };
Now there is this thing again looping over the nodes.
> + if (ret)
> + return ret;
ret may be used uninitialized here, if you loop over all nodes
and do not find any "gpio-controller".
The static code checks will just scream about this.
(Please fix in the other patch as well if present there.)
> + nr_irq_parent = of_irq_count(np);
> + spin_lock_init(&info->irq_lock);
> +
> + if (!nr_irq_parent) {
> + dev_err(&pdev->dev, "Invalid or no IRQ\n");
> + return 0;
> + }
What if it is > 1? That doesn't seem to work but will pass this
check silently.
> + ret = gpiochip_irqchip_add(gc, irqchip, 0,
> + handle_level_irq, IRQ_TYPE_NONE);
If you also set up the handler in .set_type() you can assign
handle_bad_irq() here and let .set_type set the right handler
as e.g. drivers/gpio/gpio-pl061.c.
> + for (i = 0; i < nrirqs; i++) {
> + struct irq_data *d = irq_get_irq_data(gc->irq_base + i);
> +
> + d->mask = 1 << (i % GPIO_PER_REG);
> + }
What is this? It looks like a big hack. At least put in a fat
comment about what is going on and why.
> + for (i = 0; i < nr_irq_parent; i++) {
> + int irq = irq_of_parse_and_map(np, i);
I think gpiochip_irqchip_add() will do this for you already,
as it calls irq_create_mapping() for all offsets which will call
irq_of_parse_and_map() am I right?
> +
> + if (irq < 0)
> + continue;
> +
> + gpiochip_set_chained_irqchip(gc, irqchip, irq,
> + armada_37xx_irq_handler);
> + }
So only this statement for each IRQ should be all right.
I think this driver needs a bit of tinkering and refining.
Yours,
Linus Walleij
[toc] | [prev] | [next] | [standalone]
| From | Gregory CLEMENT <gregory.clement@free-electrons.com> |
|---|---|
| Date | 2017-03-21 22:00 +0100 |
| Subject | [PATCH v2 0/7] Add support for pinctrl/gpio on Armada 37xx Was Re: [PATCH v2 0/7] Hi, |
| Message-ID | <tnz2O-8X-7@gated-at.bofh.it> |
| In reply to | #1605893 |
Hi, Obviously this cover letter is wrong. Actually I forgot to commit it in git series. Here it is the correct one: After several months here it is the second version of the series adding support for the pin and gpio controllers present on the Armada 37xx SoCs. Each Armada 37xx SoC comes 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. The gpio controller is also capable to handle interrupt from gpio. In the second version several changes has been done: - Update binding documentation making clear that mfd and syscon must be used (patch 1). - Split the fist patch adding pin controller support for Armada 37xx in arm64 part (for kconfig) and pinctrl part (patch 2 and 3) - Add MFD_SYSCON dependency (patch 3) - Add kerneldoc for the armada_37xx_pin_group struct (patch 3) - Rename _add_function() to armada_37xx_add_function() (patch 3) - Use an inline function to update the reg offset (patch 4) - Rename gpiolib_register to gpiochip_register (patch 4) - Add a comment about the two registers limit (patch 4) - Add explicit gpio node in the device tree (patch 4) - Convert the driver to use GPIOLIB_IRQCHIP (patch 5) - Add a critical section when accessing the hardware registers (patch 5) - Use the gpio sub-node (patch 5) With these change most of the comment of the first revision have been addressed. Thanks, Gregory Gregory CLEMENT (7): pinctrl: dt-bindings: Add documentation for Armada 37xx pin controllers arm64: marvell: enable the Armada 37xx pinctrl driver pinctrl: armada-37xx: Add pin controller support for Armada 37xx pinctrl: armada-37xx: Add gpio support pinctrl: aramda-37xx: Add irqchip support ARM64: dts: marvell: Add pinctrl nodes for Armada 3700 ARM64: dts: marvell: armada37xx: add pinctrl definition Documentation/devicetree/bindings/clock/armada3700-xtal-clock.txt | 7 +- Documentation/devicetree/bindings/pinctrl/marvell,armada-37xx-pinctrl.txt | 143 +++++++++++- arch/arm64/Kconfig.platforms | 2 +- arch/arm64/boot/dts/marvell/armada-3720-db.dts | 8 +- arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 71 +++++- drivers/pinctrl/Makefile | 2 +- drivers/pinctrl/mvebu/Kconfig | 7 +- drivers/pinctrl/mvebu/Makefile | 3 +- drivers/pinctrl/mvebu/pinctrl-armada-37xx.c | 918 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 9 files changed, 1153 insertions(+), 8 deletions(-) create mode 100644 Documentation/devicetree/bindings/pinctrl/marvell,armada-37xx-pinctrl.txt create mode 100644 drivers/pinctrl/mvebu/pinctrl-armada-37xx.c Sorry for the noise, Gregory On mar., mars 21 2017, Gregory CLEMENT <gregory.clement@free-electrons.com> wrote: > this series add support for the pin and gpio controllers present on > the Armada 37xx SoCs. > > Each Armada 37xx SoC comes 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. > > The gpio controller is also capable to handle interrupt from gpio. > > Gregory > > Gregory CLEMENT (7): > pinctrl: dt-bindings: Add documentation for Armada 37xx pin controllers > arm64: marvell: enable the Armada 37xx pinctrl driver > pinctrl: armada-37xx: Add pin controller support for Armada 37xx > pinctrl: armada-37xx: Add gpio support > pinctrl: aramda-37xx: Add irqchip support > ARM64: dts: marvell: Add pinctrl nodes for Armada 3700 > ARM64: dts: marvell: armada37xx: add pinctrl definition > > Documentation/devicetree/bindings/clock/armada3700-xtal-clock.txt | 7 +- > Documentation/devicetree/bindings/pinctrl/marvell,armada-37xx-pinctrl.txt | 143 +++++++++++- > arch/arm64/Kconfig.platforms | 2 +- > arch/arm64/boot/dts/marvell/armada-3720-db.dts | 8 +- > arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 71 +++++- > drivers/pinctrl/Makefile | 2 +- > drivers/pinctrl/mvebu/Kconfig | 7 +- > drivers/pinctrl/mvebu/Makefile | 3 +- > drivers/pinctrl/mvebu/pinctrl-armada-37xx.c | 918 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- > 9 files changed, 1153 insertions(+), 8 deletions(-) > create mode 100644 Documentation/devicetree/bindings/pinctrl/marvell,armada-37xx-pinctrl.txt > create mode 100644 drivers/pinctrl/mvebu/pinctrl-armada-37xx.c > > base-commit: c1ae3cfa0e89fa1a7ecc4c99031f5e9ae99d9201 > -- > git-series 0.9.1 -- Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com
[toc] | [prev] | [next] | [standalone]
| From | Gregory CLEMENT <gregory.clement@free-electrons.com> |
|---|---|
| Date | 2017-03-22 12:50 +0100 |
| Subject | Re: [PATCH v2 0/7] Add support for pinctrl/gpio on Armada 37xx Was Re: [PATCH v2 0/7] Hi, |
| Message-ID | <tnMW5-1La-13@gated-at.bofh.it> |
| In reply to | #1605993 |
Hi Linus, On mar., mars 21 2017, Gregory CLEMENT <gregory.clement@free-electrons.com> wrote: > Hi, > > Obviously this cover letter is wrong. Actually I forgot to commit it in > git series. Here it is the correct one: > > After several months here it is the second version of the series adding > support for the pin and gpio controllers present on the Armada 37xx > SoCs. As the first version was sent in December here it is a link to it: http://lists.infradead.org/pipermail/linux-arm-kernel/2016-December/475216.html Also I am going to answer to your review to point what I have done in this new series. Gregory > > Each Armada 37xx SoC comes 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. > > The gpio controller is also capable to handle interrupt from gpio. > > In the second version several changes has been done: > > - Update binding documentation making clear that mfd and syscon must > be used (patch 1). > > - Split the fist patch adding pin controller support for Armada 37xx > in arm64 part (for kconfig) and pinctrl part (patch 2 and 3) > > - Add MFD_SYSCON dependency (patch 3) > > - Add kerneldoc for the armada_37xx_pin_group struct (patch 3) > > - Rename _add_function() to armada_37xx_add_function() (patch 3) > > - Use an inline function to update the reg offset (patch 4) > > - Rename gpiolib_register to gpiochip_register (patch 4) > > - Add a comment about the two registers limit (patch 4) > > - Add explicit gpio node in the device tree (patch 4) > > - Convert the driver to use GPIOLIB_IRQCHIP (patch 5) > > - Add a critical section when accessing the hardware registers (patch 5) > > - Use the gpio sub-node (patch 5) > > With these change most of the comment of the first revision have been addressed. > > Thanks, > > Gregory > > Gregory CLEMENT (7): > pinctrl: dt-bindings: Add documentation for Armada 37xx pin controllers > arm64: marvell: enable the Armada 37xx pinctrl driver > pinctrl: armada-37xx: Add pin controller support for Armada 37xx > pinctrl: armada-37xx: Add gpio support > pinctrl: aramda-37xx: Add irqchip support > ARM64: dts: marvell: Add pinctrl nodes for Armada 3700 > ARM64: dts: marvell: armada37xx: add pinctrl definition > > Documentation/devicetree/bindings/clock/armada3700-xtal-clock.txt | 7 +- > Documentation/devicetree/bindings/pinctrl/marvell,armada-37xx-pinctrl.txt | 143 +++++++++++- > arch/arm64/Kconfig.platforms | 2 +- > arch/arm64/boot/dts/marvell/armada-3720-db.dts | 8 +- > arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 71 +++++- > drivers/pinctrl/Makefile | 2 +- > drivers/pinctrl/mvebu/Kconfig | 7 +- > drivers/pinctrl/mvebu/Makefile | 3 +- > drivers/pinctrl/mvebu/pinctrl-armada-37xx.c | 918 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- > 9 files changed, 1153 insertions(+), 8 deletions(-) > create mode 100644 Documentation/devicetree/bindings/pinctrl/marvell,armada-37xx-pinctrl.txt > create mode 100644 drivers/pinctrl/mvebu/pinctrl-armada-37xx.c > > Sorry for the noise, > > Gregory > > > On mar., mars 21 2017, Gregory CLEMENT <gregory.clement@free-electrons.com> wrote: > >> this series add support for the pin and gpio controllers present on >> the Armada 37xx SoCs. >> >> Each Armada 37xx SoC comes 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. >> >> The gpio controller is also capable to handle interrupt from gpio. >> >> Gregory >> >> Gregory CLEMENT (7): >> pinctrl: dt-bindings: Add documentation for Armada 37xx pin controllers >> arm64: marvell: enable the Armada 37xx pinctrl driver >> pinctrl: armada-37xx: Add pin controller support for Armada 37xx >> pinctrl: armada-37xx: Add gpio support >> pinctrl: aramda-37xx: Add irqchip support >> ARM64: dts: marvell: Add pinctrl nodes for Armada 3700 >> ARM64: dts: marvell: armada37xx: add pinctrl definition >> >> Documentation/devicetree/bindings/clock/armada3700-xtal-clock.txt | 7 +- >> Documentation/devicetree/bindings/pinctrl/marvell,armada-37xx-pinctrl.txt | 143 +++++++++++- >> arch/arm64/Kconfig.platforms | 2 +- >> arch/arm64/boot/dts/marvell/armada-3720-db.dts | 8 +- >> arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 71 +++++- >> drivers/pinctrl/Makefile | 2 +- >> drivers/pinctrl/mvebu/Kconfig | 7 +- >> drivers/pinctrl/mvebu/Makefile | 3 +- >> drivers/pinctrl/mvebu/pinctrl-armada-37xx.c | 918 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- >> 9 files changed, 1153 insertions(+), 8 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/pinctrl/marvell,armada-37xx-pinctrl.txt >> create mode 100644 drivers/pinctrl/mvebu/pinctrl-armada-37xx.c >> >> base-commit: c1ae3cfa0e89fa1a7ecc4c99031f5e9ae99d9201 >> -- >> git-series 0.9.1 > > -- > Gregory Clement, Free Electrons > Kernel, drivers, real-time and embedded Linux > development, consulting, training and support. > http://free-electrons.com > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web