[PATCH v2 0/2] gpio: brcm: XGS iProc GPIO driver

[PATCH v2 0/2] gpio: brcm: XGS iProc GPIO driver

[Chris Packham]

This is ported this from Broadcom's XLDK (now heavily modified). There seem to
be 3 different IP blocks for 3 separate banks of GPIOs in the iProc chips.

I've dropped everything except support for the Chip Common A GPIO
controller because the other blocks actually seem to be supportable with
other drivers. The driver itself is halfway between pinctrl-nsp-gpio.c
and pinctrl-iproc-gpio.c.

Chris Packham (2):
  dt-bindings: gpio: brcm: Add bindings for xgs-iproc
  gpio: Add xgs-iproc driver

 .../bindings/gpio/brcm,xgs-iproc.yaml         |  83 +++++
 drivers/gpio/Kconfig                          |   9 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-xgs-iproc.c                 | 301 ++++++++++++++++++
 4 files changed, 394 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml
 create mode 100644 drivers/gpio/gpio-xgs-iproc.c

-- 
2.23.0

[PATCH v2 1/2] dt-bindings: gpio: brcm: Add bindings for xgs-iproc

[Chris Packham]

This GPIO controller is present on a number of Broadcom switch ASICs
with integrated SoCs. It is similar to the nsp-gpio and iproc-gpio
blocks but different enough to require a separate driver.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    Changes in v2:
    - Document as DT schema
    - Include ngpios, #gpio-cells and gpio-controller properties

 .../bindings/gpio/brcm,xgs-iproc.yaml         | 83 +++++++++++++++++++
 1 file changed, 83 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml

diff --git a/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml b/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml
new file mode 100644
index 000000000000..71998551209e
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml
@@ -0,0 +1,83 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/brcm,xgs-iproc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Broadcom XGS iProc GPIO controller
+
+maintainers:
+  - Chris Packham <chris.packham@alliedtelesis.co.nz>
+
+description: |
+  This controller is the Chip Common A GPIO present on a number of Broadcom
+  switch ASICs with integrated SoCs.
+
+properties:
+  compatible:
+    enum:
+      - brcm,iproc-gpio-cca
+
+  reg:
+    minItems: 2
+    maxItems: 2
+    description:
+      The first region defines the base I/O address containing
+      the GPIO controller registers. The second region defines
+      the I/O address containing the Chip Common A interrupt
+      registers.
+
+  gpio-controller: true
+
+  '#gpio-cells':
+      const: 2
+
+  ngpios:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 32
+
+  interrupt-controller:
+    type: boolean
+
+  '#interrupt-cells':
+    const: 2
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - "#gpio-cells"
+  - gpio-controller
+
+allOf:
+ - if:
+     properties:
+       interrupt-controller:
+         contains:
+           const: true
+   then:
+     required:
+       - interrupts
+       - '#interrupt-cells'
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    gpio@18000060 {
+        compatible = "brcm,iproc-gpio-cca";
+        #gpio-cells = <2>;
+        reg = <0x18000060 0x50>,
+              <0x18000000 0x50>;
+        ngpios = <12>;
+        gpio-controller;
+        interrupt-controller;
+        #interrupt-cells = <2>;
+        interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_HIGH>;
+    };
+
+
+...
-- 
2.23.0

[PATCH v2 2/2] gpio: Add xgs-iproc driver

[Chris Packham]

This driver supports the Chip Common A GPIO controller present on a
number of Broadcom switch ASICs with integrated SoCs. The controller is
similar to the pinctrl-nsp-gpio and pinctrl-iproc-gpio blocks but
different enough that a separate driver is required.

This has been ported from Broadcom's XLDK 5.0.3 retaining only the CCA
support (pinctrl-iproc-gpio covers CCB).

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    Changes in v2:
    - use more of the generic infrastructure for gpio chips
    - handling the root interrupt is still done manually due to sharing with uart0.

 drivers/gpio/Kconfig          |   9 +
 drivers/gpio/Makefile         |   1 +
 drivers/gpio/gpio-xgs-iproc.c | 301 ++++++++++++++++++++++++++++++++++
 3 files changed, 311 insertions(+)
 create mode 100644 drivers/gpio/gpio-xgs-iproc.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 38e096e6925f..4b3c0f8397d7 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -156,6 +156,15 @@ config GPIO_BRCMSTB
 	help
 	  Say yes here to enable GPIO support for Broadcom STB (BCM7XXX) SoCs.
 
+config GPIO_XGS_IPROC
+	tristate "BRCM XGS iProc GPIO support"
+	depends on OF_GPIO && (ARCH_BCM_IPROC || COMPILE_TEST)
+	select GPIO_GENERIC
+	select GPIOLIB_IRQCHIP
+	default ARCH_BCM_IPROC
+	help
+	  Say yes here to enable GPIO support for Broadcom XGS iProc SoCs.
+
 config GPIO_CADENCE
 	tristate "Cadence GPIO support"
 	depends on OF_GPIO
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index d2fd19c15bae..3783c3d43fbe 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -37,6 +37,7 @@ obj-$(CONFIG_GPIO_BCM_KONA)		+= gpio-bcm-kona.o
 obj-$(CONFIG_GPIO_BD70528)		+= gpio-bd70528.o
 obj-$(CONFIG_GPIO_BD9571MWV)		+= gpio-bd9571mwv.o
 obj-$(CONFIG_GPIO_BRCMSTB)		+= gpio-brcmstb.o
+obj-$(CONFIG_GPIO_XGS_IPROC)		+= gpio-xgs-iproc.o
 obj-$(CONFIG_GPIO_BT8XX)		+= gpio-bt8xx.o
 obj-$(CONFIG_GPIO_CADENCE)		+= gpio-cadence.o
 obj-$(CONFIG_GPIO_CLPS711X)		+= gpio-clps711x.o
diff --git a/drivers/gpio/gpio-xgs-iproc.c b/drivers/gpio/gpio-xgs-iproc.c
new file mode 100644
index 000000000000..a0277acf9369
--- /dev/null
+++ b/drivers/gpio/gpio-xgs-iproc.c
@@ -0,0 +1,301 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2017 Broadcom Corporation
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/irq.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/of_device.h>
+#include <linux/of_address.h>
+#include <linux/of_gpio.h>
+
+#define CCA_INT_F_GPIOINT		BIT(0)
+#define CCA_INT_STS			0x20
+#define CCA_INT_MASK			0x24
+
+#define GPIO_CCA_DIN			0x0
+#define GPIO_CCA_DOUT			0x4
+#define GPIO_CCA_OUT_EN			0x8
+#define GPIO_CCA_INT_LEVEL		0x10
+#define GPIO_CCA_INT_LEVEL_MASK		0x14
+#define GPIO_CCA_INT_EVENT		0x18
+#define GPIO_CCA_INT_EVENT_MASK		0x1C
+#define GPIO_CCA_INT_EDGE		0x24
+
+struct iproc_gpio_chip {
+	struct irq_chip irqchip;
+	struct gpio_chip gc;
+	spinlock_t lock;
+	struct device *dev;
+	void __iomem *base;
+	void __iomem *intr;
+};
+
+static inline struct iproc_gpio_chip *
+to_iproc_gpio(struct gpio_chip *gc)
+{
+	return container_of(gc, struct iproc_gpio_chip, gc);
+}
+
+static void iproc_gpio_irq_ack(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct iproc_gpio_chip *chip = to_iproc_gpio(gc);
+	int pin = d->hwirq;
+	u32 irq = d->irq;
+	u32 irq_type, event_status = 0;
+
+	irq_type = irq_get_trigger_type(irq);
+	if (irq_type & IRQ_TYPE_EDGE_BOTH) {
+		event_status |= BIT(pin);
+		writel(event_status, chip->base + GPIO_CCA_INT_EVENT);
+	}
+}
+
+static void iproc_gpio_irq_unmask(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct iproc_gpio_chip *chip = to_iproc_gpio(gc);
+	int pin = d->hwirq;
+	u32 irq = d->irq;
+	u32 int_mask, irq_type, event_mask;
+
+	irq_type = irq_get_trigger_type(irq);
+	event_mask = readl(chip->base + GPIO_CCA_INT_EVENT_MASK);
+	int_mask = readl(chip->base + GPIO_CCA_INT_LEVEL_MASK);
+
+	if (irq_type & IRQ_TYPE_EDGE_BOTH) {
+		event_mask |= 1 << pin;
+		writel(event_mask, chip->base + GPIO_CCA_INT_EVENT_MASK);
+	} else {
+		int_mask |= 1 << pin;
+		writel(int_mask, chip->base + GPIO_CCA_INT_LEVEL_MASK);
+	}
+}
+
+static void iproc_gpio_irq_mask(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct iproc_gpio_chip *chip = to_iproc_gpio(gc);
+	int pin = d->hwirq;
+	u32 irq = d->irq;
+	u32 irq_type, int_mask, event_mask;
+
+	irq_type = irq_get_trigger_type(irq);
+	event_mask = readl(chip->base + GPIO_CCA_INT_EVENT_MASK);
+	int_mask = readl(chip->base + GPIO_CCA_INT_LEVEL_MASK);
+
+	if (irq_type & IRQ_TYPE_EDGE_BOTH) {
+		event_mask &= ~BIT(pin);
+		writel(event_mask, chip->base + GPIO_CCA_INT_EVENT_MASK);
+	} else {
+		int_mask &= ~BIT(pin);
+		writel(int_mask, chip->base + GPIO_CCA_INT_LEVEL_MASK);
+	}
+}
+
+
+static int iproc_gpio_irq_set_type(struct irq_data *d, u32 type)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct iproc_gpio_chip *chip = to_iproc_gpio(gc);
+	int pin = d->hwirq;
+	u32 irq = d->irq;
+	u32 event_pol, int_pol;
+
+
+	switch (type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_EDGE_RISING:
+		event_pol = readl(chip->base + GPIO_CCA_INT_EDGE);
+		event_pol &= ~BIT(pin);
+		writel(event_pol, chip->base + GPIO_CCA_INT_EDGE);
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		event_pol = readl(chip->base + GPIO_CCA_INT_EDGE);
+		event_pol |= BIT(pin);
+		writel(event_pol, chip->base + GPIO_CCA_INT_EDGE);
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		int_pol = readl(chip->base + GPIO_CCA_INT_LEVEL);
+		int_pol &= ~BIT(pin);
+		writel(int_pol, chip->base + GPIO_CCA_INT_LEVEL);
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		int_pol = readl(chip->base + GPIO_CCA_INT_LEVEL);
+		int_pol |= BIT(pin);
+		writel(int_pol, chip->base + GPIO_CCA_INT_LEVEL);
+		break;
+	default:
+		/* should not come here */
+		return -EINVAL;
+	}
+
+	if (type & IRQ_TYPE_LEVEL_MASK)
+		irq_set_handler_locked(irq_get_irq_data(irq), handle_level_irq);
+	else if (type & IRQ_TYPE_EDGE_BOTH)
+		irq_set_handler_locked(irq_get_irq_data(irq), handle_edge_irq);
+
+	return 0;
+}
+
+static irqreturn_t iproc_gpio_irq_handler(int irq, void *data)
+{
+	struct gpio_chip *gc = (struct gpio_chip *)data;
+	struct iproc_gpio_chip *chip = to_iproc_gpio(gc);
+	int bit;
+	unsigned long int_bits = 0;
+	u32 int_status;
+
+	/* go through the entire GPIOs and handle all interrupts */
+	int_status = readl(chip->intr + CCA_INT_STS);
+	if (int_status & CCA_INT_F_GPIOINT) {
+		u32 event, level;
+
+		/* Get level and edge interrupts */
+		event = readl(chip->base + GPIO_CCA_INT_EVENT_MASK);
+		event &= readl(chip->base + GPIO_CCA_INT_EVENT);
+		level = readl(chip->base + GPIO_CCA_DIN);
+		level ^= readl(chip->base + GPIO_CCA_INT_LEVEL);
+		level &= readl(chip->base + GPIO_CCA_INT_LEVEL_MASK);
+		int_bits = level | event;
+
+		for_each_set_bit(bit, &int_bits, gc->ngpio)
+			generic_handle_irq(
+				irq_linear_revmap(gc->irq.domain, bit));
+	}
+
+	return  int_bits ? IRQ_HANDLED : IRQ_NONE;
+}
+
+static int iproc_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *dn = pdev->dev.of_node;
+	struct iproc_gpio_chip *chip;
+	u32 num_gpios;
+	int irq, ret;
+
+	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	chip->dev = dev;
+	platform_set_drvdata(pdev, chip);
+
+	chip->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(chip->base))
+		return PTR_ERR(chip->base);
+
+	ret = bgpio_init(&chip->gc, dev, 4,
+			 chip->base + GPIO_CCA_DIN,
+			 chip->base + GPIO_CCA_DOUT,
+			 NULL,
+			 chip->base + GPIO_CCA_OUT_EN,
+			 NULL,
+			 0);
+	if (ret) {
+		dev_err(dev, "unable to init GPIO chip\n");
+		return ret;
+	}
+
+	chip->gc.label = dev_name(dev);
+	if (of_property_read_u32(dn, "ngpios", &num_gpios))
+		chip->gc.ngpio = num_gpios;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq > 0) {
+		struct gpio_irq_chip *girq;
+		struct irq_chip *irqc;
+		u32 val;
+
+		irqc = &chip->irqchip;
+		irqc->name = dev_name(dev);
+		irqc->irq_ack = iproc_gpio_irq_ack;
+		irqc->irq_mask = iproc_gpio_irq_mask;
+		irqc->irq_unmask = iproc_gpio_irq_unmask;
+		irqc->irq_set_type = iproc_gpio_irq_set_type;
+
+		chip->intr = devm_platform_ioremap_resource(pdev, 1);
+		if (IS_ERR(chip->intr))
+			return PTR_ERR(chip->intr);
+
+		/* Enable GPIO interrupts for CCA GPIO */
+		val = readl(chip->intr + CCA_INT_MASK);
+		val |= CCA_INT_F_GPIOINT;
+		writel(val, chip->intr + CCA_INT_MASK);
+
+		/*
+		 * Directly request the irq here instead of passing
+		 * a flow-handler to gpiochip_set_chained_irqchip,
+		 * because the irq is shared.
+		 */
+		ret = devm_request_irq(dev, irq, iproc_gpio_irq_handler,
+				       IRQF_SHARED, chip->gc.label, &chip->gc);
+		if (ret) {
+			dev_err(dev, "Fail to request IRQ%d: %d\n", irq, ret);
+			return ret;
+		}
+
+		girq = &chip->gc.irq;
+		girq->chip =  irqc;
+		/* This will let us handle the parent IRQ in the driver */
+		girq->parent_handler = NULL;
+		girq->num_parents = 0;
+		girq->parents = NULL;
+		girq->default_type = IRQ_TYPE_NONE;
+		girq->handler = handle_simple_irq;
+	}
+
+	ret = devm_gpiochip_add_data(dev, &chip->gc, chip);
+	if (ret) {
+		dev_err(dev, "unable to add GPIO chip\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int __exit iproc_gpio_remove(struct platform_device *pdev)
+{
+	struct iproc_gpio_chip *chip;
+
+	chip = platform_get_drvdata(pdev);
+	if (!chip)
+		return -ENODEV;
+
+	if (chip->intr) {
+		u32 val;
+
+		val = readl(chip->intr + CCA_INT_MASK);
+		val &= ~CCA_INT_F_GPIOINT;
+		writel(val, chip->intr + CCA_INT_MASK);
+	}
+
+	return 0;
+}
+
+static const struct of_device_id bcm_iproc_gpio_of_match[] __initconst = {
+	{ .compatible = "brcm,iproc-gpio-cca" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, bcm_iproc_gpio_of_match);
+
+static struct platform_driver bcm_iproc_gpio_driver = {
+	.driver = {
+		.name = "iproc-xgs-gpio",
+		.owner = THIS_MODULE,
+		.of_match_table = bcm_iproc_gpio_of_match,
+	},
+	.probe = iproc_gpio_probe,
+	.remove = iproc_gpio_remove,
+};
+
+module_platform_driver(bcm_iproc_gpio_driver);
+
+MODULE_DESCRIPTION("XGS IPROC GPIO driver");
+MODULE_LICENSE("GPL v2");
-- 
2.23.0

Re: [PATCH v2 2/2] gpio: Add xgs-iproc driver

[Bartosz Golaszewski]

czw., 17 paź 2019 o 05:11 Chris Packham
<chris.packham@alliedtelesis.co.nz> napisał(a):
>
> This driver supports the Chip Common A GPIO controller present on a
> number of Broadcom switch ASICs with integrated SoCs. The controller is
> similar to the pinctrl-nsp-gpio and pinctrl-iproc-gpio blocks but
> different enough that a separate driver is required.
>
> This has been ported from Broadcom's XLDK 5.0.3 retaining only the CCA
> support (pinctrl-iproc-gpio covers CCB).
>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
>
> Notes:
>     Changes in v2:
>     - use more of the generic infrastructure for gpio chips
>     - handling the root interrupt is still done manually due to sharing with uart0.
>
>  drivers/gpio/Kconfig          |   9 +
>  drivers/gpio/Makefile         |   1 +
>  drivers/gpio/gpio-xgs-iproc.c | 301 ++++++++++++++++++++++++++++++++++
>  3 files changed, 311 insertions(+)
>  create mode 100644 drivers/gpio/gpio-xgs-iproc.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 38e096e6925f..4b3c0f8397d7 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -156,6 +156,15 @@ config GPIO_BRCMSTB
>         help
>           Say yes here to enable GPIO support for Broadcom STB (BCM7XXX) SoCs.
>
> +config GPIO_XGS_IPROC
> +       tristate "BRCM XGS iProc GPIO support"
> +       depends on OF_GPIO && (ARCH_BCM_IPROC || COMPILE_TEST)
> +       select GPIO_GENERIC
> +       select GPIOLIB_IRQCHIP
> +       default ARCH_BCM_IPROC
> +       help
> +         Say yes here to enable GPIO support for Broadcom XGS iProc SoCs.
> +
>  config GPIO_CADENCE
>         tristate "Cadence GPIO support"
>         depends on OF_GPIO
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index d2fd19c15bae..3783c3d43fbe 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -37,6 +37,7 @@ obj-$(CONFIG_GPIO_BCM_KONA)           += gpio-bcm-kona.o
>  obj-$(CONFIG_GPIO_BD70528)             += gpio-bd70528.o
>  obj-$(CONFIG_GPIO_BD9571MWV)           += gpio-bd9571mwv.o
>  obj-$(CONFIG_GPIO_BRCMSTB)             += gpio-brcmstb.o
> +obj-$(CONFIG_GPIO_XGS_IPROC)           += gpio-xgs-iproc.o
>  obj-$(CONFIG_GPIO_BT8XX)               += gpio-bt8xx.o
>  obj-$(CONFIG_GPIO_CADENCE)             += gpio-cadence.o
>  obj-$(CONFIG_GPIO_CLPS711X)            += gpio-clps711x.o
> diff --git a/drivers/gpio/gpio-xgs-iproc.c b/drivers/gpio/gpio-xgs-iproc.c
> new file mode 100644
> index 000000000000..a0277acf9369
> --- /dev/null
> +++ b/drivers/gpio/gpio-xgs-iproc.c
> @@ -0,0 +1,301 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2017 Broadcom Corporation
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/ioport.h>
> +#include <linux/of_device.h>
> +#include <linux/of_address.h>
> +#include <linux/of_gpio.h>

None of these of_ headers seem to be needed.

> +
> +#define CCA_INT_F_GPIOINT              BIT(0)
> +#define CCA_INT_STS                    0x20
> +#define CCA_INT_MASK                   0x24
> +
> +#define GPIO_CCA_DIN                   0x0
> +#define GPIO_CCA_DOUT                  0x4
> +#define GPIO_CCA_OUT_EN                        0x8
> +#define GPIO_CCA_INT_LEVEL             0x10
> +#define GPIO_CCA_INT_LEVEL_MASK                0x14
> +#define GPIO_CCA_INT_EVENT             0x18
> +#define GPIO_CCA_INT_EVENT_MASK                0x1C
> +#define GPIO_CCA_INT_EDGE              0x24

Please use a common prefix for all symbols.

> +
> +struct iproc_gpio_chip {
> +       struct irq_chip irqchip;
> +       struct gpio_chip gc;
> +       spinlock_t lock;

You're not using this lock anywhere.

> +       struct device *dev;
> +       void __iomem *base;
> +       void __iomem *intr;
> +};
> +
> +static inline struct iproc_gpio_chip *
> +to_iproc_gpio(struct gpio_chip *gc)
> +{
> +       return container_of(gc, struct iproc_gpio_chip, gc);
> +}
> +
> +static void iproc_gpio_irq_ack(struct irq_data *d)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct iproc_gpio_chip *chip = to_iproc_gpio(gc);
> +       int pin = d->hwirq;
> +       u32 irq = d->irq;
> +       u32 irq_type, event_status = 0;
> +
> +       irq_type = irq_get_trigger_type(irq);
> +       if (irq_type & IRQ_TYPE_EDGE_BOTH) {
> +               event_status |= BIT(pin);
> +               writel(event_status, chip->base + GPIO_CCA_INT_EVENT);
> +       }
> +}
> +
> +static void iproc_gpio_irq_unmask(struct irq_data *d)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct iproc_gpio_chip *chip = to_iproc_gpio(gc);
> +       int pin = d->hwirq;
> +       u32 irq = d->irq;
> +       u32 int_mask, irq_type, event_mask;
> +
> +       irq_type = irq_get_trigger_type(irq);
> +       event_mask = readl(chip->base + GPIO_CCA_INT_EVENT_MASK);

Do you really need to use the non-relaxed versions of readl() and writel()?

> +       int_mask = readl(chip->base + GPIO_CCA_INT_LEVEL_MASK);
> +
> +       if (irq_type & IRQ_TYPE_EDGE_BOTH) {
> +               event_mask |= 1 << pin;
> +               writel(event_mask, chip->base + GPIO_CCA_INT_EVENT_MASK);
> +       } else {
> +               int_mask |= 1 << pin;
> +               writel(int_mask, chip->base + GPIO_CCA_INT_LEVEL_MASK);
> +       }
> +}
> +
> +static void iproc_gpio_irq_mask(struct irq_data *d)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct iproc_gpio_chip *chip = to_iproc_gpio(gc);
> +       int pin = d->hwirq;
> +       u32 irq = d->irq;
> +       u32 irq_type, int_mask, event_mask;
> +
> +       irq_type = irq_get_trigger_type(irq);
> +       event_mask = readl(chip->base + GPIO_CCA_INT_EVENT_MASK);
> +       int_mask = readl(chip->base + GPIO_CCA_INT_LEVEL_MASK);
> +
> +       if (irq_type & IRQ_TYPE_EDGE_BOTH) {
> +               event_mask &= ~BIT(pin);
> +               writel(event_mask, chip->base + GPIO_CCA_INT_EVENT_MASK);
> +       } else {
> +               int_mask &= ~BIT(pin);
> +               writel(int_mask, chip->base + GPIO_CCA_INT_LEVEL_MASK);
> +       }
> +}
> +
> +
> +static int iproc_gpio_irq_set_type(struct irq_data *d, u32 type)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct iproc_gpio_chip *chip = to_iproc_gpio(gc);
> +       int pin = d->hwirq;
> +       u32 irq = d->irq;
> +       u32 event_pol, int_pol;
> +

Remove the extra newline.

Bart

> +
> +       switch (type & IRQ_TYPE_SENSE_MASK) {
> +       case IRQ_TYPE_EDGE_RISING:
> +               event_pol = readl(chip->base + GPIO_CCA_INT_EDGE);
> +               event_pol &= ~BIT(pin);
> +               writel(event_pol, chip->base + GPIO_CCA_INT_EDGE);
> +               break;
> +       case IRQ_TYPE_EDGE_FALLING:
> +               event_pol = readl(chip->base + GPIO_CCA_INT_EDGE);
> +               event_pol |= BIT(pin);
> +               writel(event_pol, chip->base + GPIO_CCA_INT_EDGE);
> +               break;
> +       case IRQ_TYPE_LEVEL_HIGH:
> +               int_pol = readl(chip->base + GPIO_CCA_INT_LEVEL);
> +               int_pol &= ~BIT(pin);
> +               writel(int_pol, chip->base + GPIO_CCA_INT_LEVEL);
> +               break;
> +       case IRQ_TYPE_LEVEL_LOW:
> +               int_pol = readl(chip->base + GPIO_CCA_INT_LEVEL);
> +               int_pol |= BIT(pin);
> +               writel(int_pol, chip->base + GPIO_CCA_INT_LEVEL);
> +               break;
> +       default:
> +               /* should not come here */
> +               return -EINVAL;
> +       }
> +
> +       if (type & IRQ_TYPE_LEVEL_MASK)
> +               irq_set_handler_locked(irq_get_irq_data(irq), handle_level_irq);
> +       else if (type & IRQ_TYPE_EDGE_BOTH)
> +               irq_set_handler_locked(irq_get_irq_data(irq), handle_edge_irq);
> +
> +       return 0;
> +}
> +
> +static irqreturn_t iproc_gpio_irq_handler(int irq, void *data)
> +{
> +       struct gpio_chip *gc = (struct gpio_chip *)data;
> +       struct iproc_gpio_chip *chip = to_iproc_gpio(gc);
> +       int bit;
> +       unsigned long int_bits = 0;
> +       u32 int_status;
> +
> +       /* go through the entire GPIOs and handle all interrupts */
> +       int_status = readl(chip->intr + CCA_INT_STS);
> +       if (int_status & CCA_INT_F_GPIOINT) {
> +               u32 event, level;
> +
> +               /* Get level and edge interrupts */
> +               event = readl(chip->base + GPIO_CCA_INT_EVENT_MASK);
> +               event &= readl(chip->base + GPIO_CCA_INT_EVENT);
> +               level = readl(chip->base + GPIO_CCA_DIN);
> +               level ^= readl(chip->base + GPIO_CCA_INT_LEVEL);
> +               level &= readl(chip->base + GPIO_CCA_INT_LEVEL_MASK);
> +               int_bits = level | event;
> +
> +               for_each_set_bit(bit, &int_bits, gc->ngpio)
> +                       generic_handle_irq(
> +                               irq_linear_revmap(gc->irq.domain, bit));
> +       }
> +
> +       return  int_bits ? IRQ_HANDLED : IRQ_NONE;
> +}
> +
> +static int iproc_gpio_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct device_node *dn = pdev->dev.of_node;
> +       struct iproc_gpio_chip *chip;
> +       u32 num_gpios;
> +       int irq, ret;
> +
> +       chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> +       if (!chip)
> +               return -ENOMEM;
> +
> +       chip->dev = dev;
> +       platform_set_drvdata(pdev, chip);
> +
> +       chip->base = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(chip->base))
> +               return PTR_ERR(chip->base);
> +
> +       ret = bgpio_init(&chip->gc, dev, 4,
> +                        chip->base + GPIO_CCA_DIN,
> +                        chip->base + GPIO_CCA_DOUT,
> +                        NULL,
> +                        chip->base + GPIO_CCA_OUT_EN,
> +                        NULL,
> +                        0);
> +       if (ret) {
> +               dev_err(dev, "unable to init GPIO chip\n");
> +               return ret;
> +       }
> +
> +       chip->gc.label = dev_name(dev);
> +       if (of_property_read_u32(dn, "ngpios", &num_gpios))
> +               chip->gc.ngpio = num_gpios;
> +
> +       irq = platform_get_irq(pdev, 0);
> +       if (irq > 0) {
> +               struct gpio_irq_chip *girq;
> +               struct irq_chip *irqc;
> +               u32 val;
> +
> +               irqc = &chip->irqchip;
> +               irqc->name = dev_name(dev);
> +               irqc->irq_ack = iproc_gpio_irq_ack;
> +               irqc->irq_mask = iproc_gpio_irq_mask;
> +               irqc->irq_unmask = iproc_gpio_irq_unmask;
> +               irqc->irq_set_type = iproc_gpio_irq_set_type;
> +
> +               chip->intr = devm_platform_ioremap_resource(pdev, 1);
> +               if (IS_ERR(chip->intr))
> +                       return PTR_ERR(chip->intr);
> +
> +               /* Enable GPIO interrupts for CCA GPIO */
> +               val = readl(chip->intr + CCA_INT_MASK);
> +               val |= CCA_INT_F_GPIOINT;
> +               writel(val, chip->intr + CCA_INT_MASK);
> +
> +               /*
> +                * Directly request the irq here instead of passing
> +                * a flow-handler to gpiochip_set_chained_irqchip,
> +                * because the irq is shared.
> +                */
> +               ret = devm_request_irq(dev, irq, iproc_gpio_irq_handler,
> +                                      IRQF_SHARED, chip->gc.label, &chip->gc);
> +               if (ret) {
> +                       dev_err(dev, "Fail to request IRQ%d: %d\n", irq, ret);
> +                       return ret;
> +               }
> +
> +               girq = &chip->gc.irq;
> +               girq->chip =  irqc;
> +               /* This will let us handle the parent IRQ in the driver */
> +               girq->parent_handler = NULL;
> +               girq->num_parents = 0;
> +               girq->parents = NULL;
> +               girq->default_type = IRQ_TYPE_NONE;
> +               girq->handler = handle_simple_irq;
> +       }
> +
> +       ret = devm_gpiochip_add_data(dev, &chip->gc, chip);
> +       if (ret) {
> +               dev_err(dev, "unable to add GPIO chip\n");
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static int __exit iproc_gpio_remove(struct platform_device *pdev)
> +{
> +       struct iproc_gpio_chip *chip;
> +
> +       chip = platform_get_drvdata(pdev);
> +       if (!chip)
> +               return -ENODEV;
> +
> +       if (chip->intr) {
> +               u32 val;
> +
> +               val = readl(chip->intr + CCA_INT_MASK);
> +               val &= ~CCA_INT_F_GPIOINT;
> +               writel(val, chip->intr + CCA_INT_MASK);
> +       }
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id bcm_iproc_gpio_of_match[] __initconst = {
> +       { .compatible = "brcm,iproc-gpio-cca" },
> +       {}
> +};
> +MODULE_DEVICE_TABLE(of, bcm_iproc_gpio_of_match);
> +
> +static struct platform_driver bcm_iproc_gpio_driver = {
> +       .driver = {
> +               .name = "iproc-xgs-gpio",
> +               .owner = THIS_MODULE,
> +               .of_match_table = bcm_iproc_gpio_of_match,
> +       },
> +       .probe = iproc_gpio_probe,
> +       .remove = iproc_gpio_remove,
> +};
> +
> +module_platform_driver(bcm_iproc_gpio_driver);
> +
> +MODULE_DESCRIPTION("XGS IPROC GPIO driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.23.0
>

Re: [PATCH v2 1/2] dt-bindings: gpio: brcm: Add bindings for xgs-iproc

[Bartosz Golaszewski]

czw., 17 paź 2019 o 05:11 Chris Packham
<chris.packham@alliedtelesis.co.nz> napisał(a):
>
> This GPIO controller is present on a number of Broadcom switch ASICs
> with integrated SoCs. It is similar to the nsp-gpio and iproc-gpio
> blocks but different enough to require a separate driver.
>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
>
> Notes:
>     Changes in v2:
>     - Document as DT schema
>     - Include ngpios, #gpio-cells and gpio-controller properties
>
>  .../bindings/gpio/brcm,xgs-iproc.yaml         | 83 +++++++++++++++++++
>  1 file changed, 83 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml
>
> diff --git a/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml b/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml
> new file mode 100644
> index 000000000000..71998551209e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml
> @@ -0,0 +1,83 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/brcm,xgs-iproc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Broadcom XGS iProc GPIO controller
> +
> +maintainers:
> +  - Chris Packham <chris.packham@alliedtelesis.co.nz>
> +
> +description: |
> +  This controller is the Chip Common A GPIO present on a number of Broadcom
> +  switch ASICs with integrated SoCs.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - brcm,iproc-gpio-cca

I believe this should be:

    const: brcm,iproc-gpio-cca

Bart

> +
> +  reg:
> +    minItems: 2
> +    maxItems: 2
> +    description:
> +      The first region defines the base I/O address containing
> +      the GPIO controller registers. The second region defines
> +      the I/O address containing the Chip Common A interrupt
> +      registers.
> +
> +  gpio-controller: true
> +
> +  '#gpio-cells':
> +      const: 2
> +
> +  ngpios:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 32
> +
> +  interrupt-controller:
> +    type: boolean
> +
> +  '#interrupt-cells':
> +    const: 2
> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#gpio-cells"
> +  - gpio-controller
> +
> +allOf:
> + - if:
> +     properties:
> +       interrupt-controller:
> +         contains:
> +           const: true
> +   then:
> +     required:
> +       - interrupts
> +       - '#interrupt-cells'
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    gpio@18000060 {
> +        compatible = "brcm,iproc-gpio-cca";
> +        #gpio-cells = <2>;
> +        reg = <0x18000060 0x50>,
> +              <0x18000000 0x50>;
> +        ngpios = <12>;
> +        gpio-controller;
> +        interrupt-controller;
> +        #interrupt-cells = <2>;
> +        interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_HIGH>;
> +    };
> +
> +
> +...
> --
> 2.23.0
>

Re: [PATCH v2 1/2] dt-bindings: gpio: brcm: Add bindings for xgs-iproc

[Rob Herring]

On Thu, Oct 17, 2019 at 04:10:50PM +1300, Chris Packham wrote:
> This GPIO controller is present on a number of Broadcom switch ASICs
> with integrated SoCs. It is similar to the nsp-gpio and iproc-gpio
> blocks but different enough to require a separate driver.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> 
> Notes:
>     Changes in v2:
>     - Document as DT schema
>     - Include ngpios, #gpio-cells and gpio-controller properties
> 
>  .../bindings/gpio/brcm,xgs-iproc.yaml         | 83 +++++++++++++++++++
>  1 file changed, 83 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml b/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml
> new file mode 100644
> index 000000000000..71998551209e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml
> @@ -0,0 +1,83 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/brcm,xgs-iproc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Broadcom XGS iProc GPIO controller
> +
> +maintainers:
> +  - Chris Packham <chris.packham@alliedtelesis.co.nz>
> +
> +description: |
> +  This controller is the Chip Common A GPIO present on a number of Broadcom
> +  switch ASICs with integrated SoCs.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - brcm,iproc-gpio-cca

enum vs. const usage depends on whether you think you'll add more 
compatibles.

> +
> +  reg:
> +    minItems: 2
> +    maxItems: 2
> +    description:
> +      The first region defines the base I/O address containing
> +      the GPIO controller registers. The second region defines
> +      the I/O address containing the Chip Common A interrupt
> +      registers.

items:
  - description: the I/O address containing the GPIO controller 
      registers
  - description: the I/O address containing the Chip Common A interrupt 
      registers

And minItems/maxItems can be implicit.

> +
> +  gpio-controller: true
> +
> +  '#gpio-cells':
> +      const: 2
> +
> +  ngpios:
> +    $ref: /schemas/types.yaml#/definitions/uint32

Common property, doesn't need a type definition. Also, it would have to 
be under an 'allOf' to actually work.

> +    minimum: 0
> +    maximum: 32
> +
> +  interrupt-controller:
> +    type: boolean

Just 'interrupt-controller: true'

> +
> +  '#interrupt-cells':
> +    const: 2
> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#gpio-cells"
> +  - gpio-controller
> +
> +allOf:
> + - if:
> +     properties:
> +       interrupt-controller:
> +         contains:
> +           const: true
> +   then:
> +     required:
> +       - interrupts
> +       - '#interrupt-cells'

This is mostly handled in the core schema already and 'dependencies' 
works better for this anyways. All you need here is:

dependencies:
  interrupt-controller: [ interrupts ]

> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    gpio@18000060 {
> +        compatible = "brcm,iproc-gpio-cca";
> +        #gpio-cells = <2>;
> +        reg = <0x18000060 0x50>,
> +              <0x18000000 0x50>;
> +        ngpios = <12>;
> +        gpio-controller;
> +        interrupt-controller;
> +        #interrupt-cells = <2>;
> +        interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_HIGH>;
> +    };
> +
> +
> +...
> -- 
> 2.23.0
> 

Re: [PATCH v2 1/2] dt-bindings: gpio: brcm: Add bindings for xgs-iproc

[Bartosz Golaszewski]

czw., 17 paź 2019 o 21:24 Rob Herring <robh@kernel.org> napisał(a):
>
> On Thu, Oct 17, 2019 at 04:10:50PM +1300, Chris Packham wrote:
> > This GPIO controller is present on a number of Broadcom switch ASICs
> > with integrated SoCs. It is similar to the nsp-gpio and iproc-gpio
> > blocks but different enough to require a separate driver.
> >
> > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> > ---
> >
> > Notes:
> >     Changes in v2:
> >     - Document as DT schema
> >     - Include ngpios, #gpio-cells and gpio-controller properties
> >
> >  .../bindings/gpio/brcm,xgs-iproc.yaml         | 83 +++++++++++++++++++
> >  1 file changed, 83 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml b/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml
> > new file mode 100644
> > index 000000000000..71998551209e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml
> > @@ -0,0 +1,83 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/gpio/brcm,xgs-iproc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Broadcom XGS iProc GPIO controller
> > +
> > +maintainers:
> > +  - Chris Packham <chris.packham@alliedtelesis.co.nz>
> > +
> > +description: |
> > +  This controller is the Chip Common A GPIO present on a number of Broadcom
> > +  switch ASICs with integrated SoCs.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - brcm,iproc-gpio-cca
>
> enum vs. const usage depends on whether you think you'll add more
> compatibles.
>

What if you don't know yet? For instance we use a const compatible and
then a new chip is released for which we can reuse the driver? Is this
something that is expected to remain stable in the binding document?
The question is unrelated to this patch, I'm just unsure about my own
approach to writing yaml bindings.

Bart

> > +
> > +  reg:
> > +    minItems: 2
> > +    maxItems: 2
> > +    description:
> > +      The first region defines the base I/O address containing
> > +      the GPIO controller registers. The second region defines
> > +      the I/O address containing the Chip Common A interrupt
> > +      registers.
>
> items:
>   - description: the I/O address containing the GPIO controller
>       registers
>   - description: the I/O address containing the Chip Common A interrupt
>       registers
>
> And minItems/maxItems can be implicit.
>
> > +
> > +  gpio-controller: true
> > +
> > +  '#gpio-cells':
> > +      const: 2
> > +
> > +  ngpios:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
>
> Common property, doesn't need a type definition. Also, it would have to
> be under an 'allOf' to actually work.
>
> > +    minimum: 0
> > +    maximum: 32
> > +
> > +  interrupt-controller:
> > +    type: boolean
>
> Just 'interrupt-controller: true'
>
> > +
> > +  '#interrupt-cells':
> > +    const: 2
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - "#gpio-cells"
> > +  - gpio-controller
> > +
> > +allOf:
> > + - if:
> > +     properties:
> > +       interrupt-controller:
> > +         contains:
> > +           const: true
> > +   then:
> > +     required:
> > +       - interrupts
> > +       - '#interrupt-cells'
>
> This is mostly handled in the core schema already and 'dependencies'
> works better for this anyways. All you need here is:
>
> dependencies:
>   interrupt-controller: [ interrupts ]
>
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    gpio@18000060 {
> > +        compatible = "brcm,iproc-gpio-cca";
> > +        #gpio-cells = <2>;
> > +        reg = <0x18000060 0x50>,
> > +              <0x18000000 0x50>;
> > +        ngpios = <12>;
> > +        gpio-controller;
> > +        interrupt-controller;
> > +        #interrupt-cells = <2>;
> > +        interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_HIGH>;
> > +    };
> > +
> > +
> > +...
> > --
> > 2.23.0
> >

Re: [PATCH v2 1/2] dt-bindings: gpio: brcm: Add bindings for xgs-iproc

[Rob Herring]

On Fri, Oct 18, 2019 at 1:00 AM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
>
> czw., 17 paź 2019 o 21:24 Rob Herring <robh@kernel.org> napisał(a):
> >
> > On Thu, Oct 17, 2019 at 04:10:50PM +1300, Chris Packham wrote:
> > > This GPIO controller is present on a number of Broadcom switch ASICs
> > > with integrated SoCs. It is similar to the nsp-gpio and iproc-gpio
> > > blocks but different enough to require a separate driver.
> > >
> > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> > > ---
> > >
> > > Notes:
> > >     Changes in v2:
> > >     - Document as DT schema
> > >     - Include ngpios, #gpio-cells and gpio-controller properties
> > >
> > >  .../bindings/gpio/brcm,xgs-iproc.yaml         | 83 +++++++++++++++++++
> > >  1 file changed, 83 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml b/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml
> > > new file mode 100644
> > > index 000000000000..71998551209e
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml
> > > @@ -0,0 +1,83 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/gpio/brcm,xgs-iproc.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Broadcom XGS iProc GPIO controller
> > > +
> > > +maintainers:
> > > +  - Chris Packham <chris.packham@alliedtelesis.co.nz>
> > > +
> > > +description: |
> > > +  This controller is the Chip Common A GPIO present on a number of Broadcom
> > > +  switch ASICs with integrated SoCs.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - brcm,iproc-gpio-cca
> >
> > enum vs. const usage depends on whether you think you'll add more
> > compatibles.
> >
>
> What if you don't know yet? For instance we use a const compatible and
> then a new chip is released for which we can reuse the driver?

Then you just change it to an enum (or oneOf if the new compatible has
a fallback to the old one). Not really a big deal.

> Is this
> something that is expected to remain stable in the binding document?

No, only in the sense we want to minimize changes.

> The question is unrelated to this patch, I'm just unsure about my own
> approach to writing yaml bindings.

We could perhaps just say single entries should always be 'const'
because then we could write a meta-schema enforcing that:

properties:
  enum:
    minItems: 2

I don't think we should be that strict though unless it becomes a
frequent review topic. So either way is fine, it's up to your
judgement, and let's stop talking about it before I change my mind. :)

Rob