IDT 79RC3243x SoCs integrated a gpio controller, which handles up
to 32 gpios. All gpios could be used as an interrupt source.
Signed-off-by: Thomas Bogendoerfer <[email protected]>
---
Changes in v4:
- added spinlock to serialize access to irq registers
- reworked checking of irq sense bits
- start with handle_bad_irq and set handle_level_irq in idt_gpio_irq_set_type
- cleaned up #includes
- use platform_get_irq
Changes in v3:
- changed compatible string to idt,32434-gpio
- registers now start with gpio direction register and leaves
out alternate function register for pinmux/pinctrl driver
Changes in v2:
- made driver buildable as module
- use for_each_set_bit() in irq dispatch handler
- use gpiochip_get_data instead of own container_of helper
- use module_platform_driver() instead of arch_initcall
- don't default y for Mikrotik RB532
drivers/gpio/Kconfig | 12 ++
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-idt3243x.c | 209 +++++++++++++++++++++++++++++++++++
3 files changed, 222 insertions(+)
create mode 100644 drivers/gpio/gpio-idt3243x.c
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index e3607ec4c2e8..90543a95dbb8 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -770,6 +770,18 @@ config GPIO_MSC313
Say Y here to support the main GPIO block on MStar/SigmaStar
ARMv7 based SoCs.
+config GPIO_IDT3243X
+ tristate "IDT 79RC3243X GPIO support"
+ depends on MIKROTIK_RB532 || COMPILE_TEST
+ select GPIO_GENERIC
+ select GPIOLIB_IRQCHIP
+ help
+ Select this option to enable GPIO driver for
+ IDT 79RC3243X based devices like Mikrotik RB532.
+
+ To compile this driver as a module, choose M here: the module will
+ be called gpio-idt3243x.
+
endmenu
menu "Port-mapped I/O GPIO drivers"
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index c58a90a3c3b1..75dd9c5665c5 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_GPIO_HISI) += gpio-hisi.o
obj-$(CONFIG_GPIO_HLWD) += gpio-hlwd.o
obj-$(CONFIG_HTC_EGPIO) += gpio-htc-egpio.o
obj-$(CONFIG_GPIO_ICH) += gpio-ich.o
+obj-$(CONFIG_GPIO_IDT3243X) += gpio-idt3243x.o
obj-$(CONFIG_GPIO_IOP) += gpio-iop.o
obj-$(CONFIG_GPIO_IT87) += gpio-it87.o
obj-$(CONFIG_GPIO_IXP4XX) += gpio-ixp4xx.o
diff --git a/drivers/gpio/gpio-idt3243x.c b/drivers/gpio/gpio-idt3243x.c
new file mode 100644
index 000000000000..62e5643a0228
--- /dev/null
+++ b/drivers/gpio/gpio-idt3243x.c
@@ -0,0 +1,209 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Driver for IDT/Renesas 79RC3243x Interrupt Controller */
+
+#include <linux/gpio/driver.h>
+#include <linux/irq.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+
+#define IDT_PIC_IRQ_PEND 0x00
+#define IDT_PIC_IRQ_MASK 0x08
+
+#define IDT_GPIO_DIR 0x00
+#define IDT_GPIO_DATA 0x04
+#define IDT_GPIO_ILEVEL 0x08
+#define IDT_GPIO_ISTAT 0x0C
+
+struct idt_gpio_ctrl {
+ struct gpio_chip gc;
+ void __iomem *pic;
+ void __iomem *gpio;
+ u32 mask_cache;
+ spinlock_t irq_lock; /* serialize access to irq registers */
+};
+
+static void idt_gpio_dispatch(struct irq_desc *desc)
+{
+ struct gpio_chip *gc = irq_desc_get_handler_data(desc);
+ struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc);
+ struct irq_chip *host_chip = irq_desc_get_chip(desc);
+ unsigned int bit, virq;
+ unsigned long pending;
+
+ chained_irq_enter(host_chip, desc);
+
+ pending = readl(ctrl->pic + IDT_PIC_IRQ_PEND);
+ pending &= ~ctrl->mask_cache;
+ for_each_set_bit(bit, &pending, gc->ngpio) {
+ virq = irq_linear_revmap(gc->irq.domain, bit);
+ if (virq)
+ generic_handle_irq(virq);
+ }
+
+ chained_irq_exit(host_chip, desc);
+}
+
+static int idt_gpio_irq_set_type(struct irq_data *d, unsigned int flow_type)
+{
+ struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+ struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc);
+ unsigned int sense = flow_type & IRQ_TYPE_SENSE_MASK;
+ unsigned long flags;
+ u32 ilevel;
+
+ /* hardware only supports level triggered */
+ if (sense & IRQ_TYPE_EDGE_BOTH)
+ return -EINVAL;
+
+ if (sense == 0 || sense == IRQ_TYPE_LEVEL_MASK)
+ return -EINVAL;
+
+ spin_lock_irqsave(&ctrl->irq_lock, flags);
+
+ ilevel = readl(ctrl->gpio + IDT_GPIO_ILEVEL);
+ if (sense & IRQ_TYPE_LEVEL_HIGH)
+ ilevel |= BIT(d->hwirq);
+ else if (sense & IRQ_TYPE_LEVEL_LOW)
+ ilevel &= ~BIT(d->hwirq);
+
+ writel(ilevel, ctrl->gpio + IDT_GPIO_ILEVEL);
+ irq_set_handler_locked(d, handle_level_irq);
+
+ spin_unlock_irqrestore(&ctrl->irq_lock, flags);
+ return 0;
+}
+
+static void idt_gpio_ack(struct irq_data *d)
+{
+ struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+ struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc);
+
+ writel(~BIT(d->hwirq), ctrl->gpio + IDT_GPIO_ISTAT);
+}
+
+static void idt_gpio_mask(struct irq_data *d)
+{
+ struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+ struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc);
+ unsigned long flags;
+
+ spin_lock_irqsave(&ctrl->irq_lock, flags);
+
+ ctrl->mask_cache |= BIT(d->hwirq);
+ writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK);
+
+ spin_unlock_irqrestore(&ctrl->irq_lock, flags);
+}
+
+static void idt_gpio_unmask(struct irq_data *d)
+{
+ struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+ struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc);
+ unsigned long flags;
+
+ spin_lock_irqsave(&ctrl->irq_lock, flags);
+
+ ctrl->mask_cache &= ~BIT(d->hwirq);
+ writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK);
+
+ spin_unlock_irqrestore(&ctrl->irq_lock, flags);
+}
+
+static int idt_gpio_irq_init_hw(struct gpio_chip *gc)
+{
+ struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc);
+
+ /* Mask interrupts. */
+ ctrl->mask_cache = 0xffffffff;
+ writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK);
+
+ return 0;
+}
+
+static struct irq_chip idt_gpio_irqchip = {
+ .name = "IDTGPIO",
+ .irq_mask = idt_gpio_mask,
+ .irq_ack = idt_gpio_ack,
+ .irq_unmask = idt_gpio_unmask,
+ .irq_set_type = idt_gpio_irq_set_type
+};
+
+static int idt_gpio_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct gpio_irq_chip *girq;
+ struct idt_gpio_ctrl *ctrl;
+ unsigned int parent_irq;
+ int ngpios;
+ int ret;
+
+ ret = device_property_read_u32(dev, "ngpios", &ngpios);
+ if (ret) {
+ dev_err(dev, "ngpios property is not valid\n");
+ return ret;
+ }
+
+ ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
+ if (!ctrl)
+ return -ENOMEM;
+
+ ctrl->gpio = devm_platform_ioremap_resource_byname(pdev, "gpio");
+ if (!ctrl->gpio)
+ return -ENOMEM;
+
+ ctrl->gc.parent = dev;
+
+ ret = bgpio_init(&ctrl->gc, &pdev->dev, 4, ctrl->gpio + IDT_GPIO_DATA,
+ NULL, NULL, ctrl->gpio + IDT_GPIO_DIR, NULL, 0);
+ if (ret) {
+ dev_err(dev, "bgpio_init failed\n");
+ return ret;
+ }
+ ctrl->gc.ngpio = ngpios;
+
+ ctrl->pic = devm_platform_ioremap_resource_byname(pdev, "pic");
+ if (!ctrl->pic)
+ return -ENOMEM;
+
+ parent_irq = platform_get_irq(pdev, 0);
+ if (!parent_irq)
+ return -EINVAL;
+
+ girq = &ctrl->gc.irq;
+ girq->chip = &idt_gpio_irqchip;
+ girq->init_hw = idt_gpio_irq_init_hw;
+ girq->parent_handler = idt_gpio_dispatch;
+ girq->num_parents = 1;
+ girq->parents = devm_kcalloc(dev, girq->num_parents,
+ sizeof(*girq->parents), GFP_KERNEL);
+ if (!girq->parents)
+ return -ENOMEM;
+
+ girq->parents[0] = parent_irq;
+ girq->default_type = IRQ_TYPE_NONE;
+ girq->handler = handle_bad_irq;
+
+ spin_lock_init(&ctrl->irq_lock);
+
+ return devm_gpiochip_add_data(&pdev->dev, &ctrl->gc, ctrl);
+}
+
+static const struct of_device_id idt_gpio_of_match[] = {
+ { .compatible = "idt,32434-gpio" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, idt_gpio_of_match);
+
+static struct platform_driver idt_gpio_driver = {
+ .probe = idt_gpio_probe,
+ .driver = {
+ .name = "idt3243x-gpio",
+ .of_match_table = idt_gpio_of_match,
+ },
+};
+module_platform_driver(idt_gpio_driver);
+
+MODULE_DESCRIPTION("IDT 79RC3243x GPIO/PIC Driver");
+MODULE_AUTHOR("Thomas Bogendoerfer <[email protected]>");
+MODULE_LICENSE("GPL");
--
2.29.2
Add YAML devicetree binding for IDT 79RC32434 GPIO controller
Signed-off-by: Thomas Bogendoerfer <[email protected]>
---
Changes in v4:
- renamed to idt,32434-gpio this time for real
Changes in v3:
- renamed to idt,32434-gpio
- drop ngpio description
- use gpio0: gpio@50004 in example
.../bindings/gpio/idt,32434-gpio.yaml | 71 +++++++++++++++++++
1 file changed, 71 insertions(+)
create mode 100644 Documentation/devicetree/bindings/gpio/idt,32434-gpio.yaml
diff --git a/Documentation/devicetree/bindings/gpio/idt,32434-gpio.yaml b/Documentation/devicetree/bindings/gpio/idt,32434-gpio.yaml
new file mode 100644
index 000000000000..517d14b6c2e2
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/idt,32434-gpio.yaml
@@ -0,0 +1,71 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/idt,32434-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: IDT 79RC32434 GPIO controller
+
+maintainers:
+ - Thomas Bogendoerfer <[email protected]>
+
+properties:
+ compatible:
+ const: idt,32434-gpio
+
+ reg:
+ maxItems: 2
+
+ reg-names:
+ items:
+ - const: gpio
+ - const: pic
+
+ gpio-controller: true
+
+ "#gpio-cells":
+ const: 2
+
+ ngpios:
+ minimum: 1
+ maximum: 32
+
+ interrupt-controller: true
+
+ "#interrupt-cells":
+ const: 2
+
+ interrupts:
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+ - reg-names
+ - gpio-controller
+ - "#gpio-cells"
+ - ngpios
+ - interrupt-controller
+ - "#interrupt-cells"
+ - interrupts
+
+additionalProperties: false
+
+examples:
+ - |
+ gpio0: gpio@50004 {
+ compatible = "idt,32434-gpio";
+ reg = <0x50004 0x10>, <0x38030 0x0c>;
+ reg-names = "gpio", "pic";
+
+ interrupt-controller;
+ #interrupt-cells = <2>;
+
+ interrupt-parent = <&cpuintc>;
+ interrupts = <6>;
+
+ gpio-controller;
+ #gpio-cells = <2>;
+
+ ngpios = <14>;
+ };
--
2.29.2
On Mon, Apr 26, 2021 at 12:55 PM Thomas Bogendoerfer
<[email protected]> wrote:
>
> IDT 79RC3243x SoCs integrated a gpio controller, which handles up
> to 32 gpios. All gpios could be used as an interrupt source.
Thank you!
Honestly speaking, I was about to give a tag but realized 1) we missed
v5.13 anyway, and 2) there is gpio-regmap generic code, that may be
worth considering. Otherwise this is in a pretty good shape.
My comments below.
> Signed-off-by: Thomas Bogendoerfer <[email protected]>
> ---
> Changes in v4:
> - added spinlock to serialize access to irq registers
I'm not sure it's enough to have separated locks for these registers
versus direction / value ones.
Can't you reuse bgpio_lock?
Looking into bgpio code, I think it has issues with locking in some
cases (it does two or more operations each of them serialized, but not
together, it means there is a window where another I/O may happen and
potentially screw up the GPIO state.
Dunno if gpio-regmap has this solved (I suggest to look into it as
well, at least regmap API provides locking by default).
> - reworked checking of irq sense bits
> - start with handle_bad_irq and set handle_level_irq in idt_gpio_irq_set_type
> - cleaned up #includes
> - use platform_get_irq
>
> Changes in v3:
> - changed compatible string to idt,32434-gpio
> - registers now start with gpio direction register and leaves
> out alternate function register for pinmux/pinctrl driver
>
> Changes in v2:
> - made driver buildable as module
> - use for_each_set_bit() in irq dispatch handler
> - use gpiochip_get_data instead of own container_of helper
> - use module_platform_driver() instead of arch_initcall
> - don't default y for Mikrotik RB532
>
> drivers/gpio/Kconfig | 12 ++
> drivers/gpio/Makefile | 1 +
> drivers/gpio/gpio-idt3243x.c | 209 +++++++++++++++++++++++++++++++++++
> 3 files changed, 222 insertions(+)
> create mode 100644 drivers/gpio/gpio-idt3243x.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index e3607ec4c2e8..90543a95dbb8 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -770,6 +770,18 @@ config GPIO_MSC313
> Say Y here to support the main GPIO block on MStar/SigmaStar
> ARMv7 based SoCs.
>
> +config GPIO_IDT3243X
> + tristate "IDT 79RC3243X GPIO support"
> + depends on MIKROTIK_RB532 || COMPILE_TEST
> + select GPIO_GENERIC
> + select GPIOLIB_IRQCHIP
> + help
> + Select this option to enable GPIO driver for
> + IDT 79RC3243X based devices like Mikrotik RB532.
> +
> + To compile this driver as a module, choose M here: the module will
> + be called gpio-idt3243x.
> +
> endmenu
>
> menu "Port-mapped I/O GPIO drivers"
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index c58a90a3c3b1..75dd9c5665c5 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -67,6 +67,7 @@ obj-$(CONFIG_GPIO_HISI) += gpio-hisi.o
> obj-$(CONFIG_GPIO_HLWD) += gpio-hlwd.o
> obj-$(CONFIG_HTC_EGPIO) += gpio-htc-egpio.o
> obj-$(CONFIG_GPIO_ICH) += gpio-ich.o
> +obj-$(CONFIG_GPIO_IDT3243X) += gpio-idt3243x.o
> obj-$(CONFIG_GPIO_IOP) += gpio-iop.o
> obj-$(CONFIG_GPIO_IT87) += gpio-it87.o
> obj-$(CONFIG_GPIO_IXP4XX) += gpio-ixp4xx.o
> diff --git a/drivers/gpio/gpio-idt3243x.c b/drivers/gpio/gpio-idt3243x.c
> new file mode 100644
> index 000000000000..62e5643a0228
> --- /dev/null
> +++ b/drivers/gpio/gpio-idt3243x.c
> @@ -0,0 +1,209 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Driver for IDT/Renesas 79RC3243x Interrupt Controller */
> +
+ bitops.h
> +#include <linux/gpio/driver.h>
> +#include <linux/irq.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/platform_device.h>
+ spinlock.h (but see above)
> +
> +#define IDT_PIC_IRQ_PEND 0x00
> +#define IDT_PIC_IRQ_MASK 0x08
> +
> +#define IDT_GPIO_DIR 0x00
> +#define IDT_GPIO_DATA 0x04
> +#define IDT_GPIO_ILEVEL 0x08
> +#define IDT_GPIO_ISTAT 0x0C
> +
> +struct idt_gpio_ctrl {
> + struct gpio_chip gc;
> + void __iomem *pic;
> + void __iomem *gpio;
> + u32 mask_cache;
> + spinlock_t irq_lock; /* serialize access to irq registers */
> +};
> +
> +static void idt_gpio_dispatch(struct irq_desc *desc)
> +{
> + struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> + struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc);
> + struct irq_chip *host_chip = irq_desc_get_chip(desc);
> + unsigned int bit, virq;
> + unsigned long pending;
> +
> + chained_irq_enter(host_chip, desc);
> +
> + pending = readl(ctrl->pic + IDT_PIC_IRQ_PEND);
> + pending &= ~ctrl->mask_cache;
> + for_each_set_bit(bit, &pending, gc->ngpio) {
> + virq = irq_linear_revmap(gc->irq.domain, bit);
> + if (virq)
> + generic_handle_irq(virq);
> + }
> +
> + chained_irq_exit(host_chip, desc);
> +}
> +
> +static int idt_gpio_irq_set_type(struct irq_data *d, unsigned int flow_type)
> +{
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc);
> + unsigned int sense = flow_type & IRQ_TYPE_SENSE_MASK;
> + unsigned long flags;
> + u32 ilevel;
> +
> + /* hardware only supports level triggered */
> + if (sense & IRQ_TYPE_EDGE_BOTH)
> + return -EINVAL;
> +
> + if (sense == 0 || sense == IRQ_TYPE_LEVEL_MASK)
0 => IRQ_TYPE_NONE
Now I have got the below exit. You need to check here for EDGE
So,
if (sense == IRQ_TYPE_NONE || (sense & IRQ_TYPE_EDGE_BOTH)
And setting LEVEL_HIGH + LEVEL_LOW shouldn't stop you here. It's fine,
just declared HIGH as a winner.
> + return -EINVAL;
> +
> + spin_lock_irqsave(&ctrl->irq_lock, flags);
> +
> + ilevel = readl(ctrl->gpio + IDT_GPIO_ILEVEL);
> + if (sense & IRQ_TYPE_LEVEL_HIGH)
> + ilevel |= BIT(d->hwirq);
> + else if (sense & IRQ_TYPE_LEVEL_LOW)
> + ilevel &= ~BIT(d->hwirq);
> +
> + writel(ilevel, ctrl->gpio + IDT_GPIO_ILEVEL);
> + irq_set_handler_locked(d, handle_level_irq);
> +
> + spin_unlock_irqrestore(&ctrl->irq_lock, flags);
> + return 0;
> +}
> +
> +static void idt_gpio_ack(struct irq_data *d)
> +{
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc);
> +
> + writel(~BIT(d->hwirq), ctrl->gpio + IDT_GPIO_ISTAT);
> +}
> +
> +static void idt_gpio_mask(struct irq_data *d)
> +{
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&ctrl->irq_lock, flags);
> +
> + ctrl->mask_cache |= BIT(d->hwirq);
> + writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK);
> +
> + spin_unlock_irqrestore(&ctrl->irq_lock, flags);
> +}
> +
> +static void idt_gpio_unmask(struct irq_data *d)
> +{
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&ctrl->irq_lock, flags);
> +
> + ctrl->mask_cache &= ~BIT(d->hwirq);
> + writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK);
> +
> + spin_unlock_irqrestore(&ctrl->irq_lock, flags);
> +}
> +
> +static int idt_gpio_irq_init_hw(struct gpio_chip *gc)
> +{
> + struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc);
> +
> + /* Mask interrupts. */
> + ctrl->mask_cache = 0xffffffff;
> + writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK);
> +
> + return 0;
> +}
> +
> +static struct irq_chip idt_gpio_irqchip = {
> + .name = "IDTGPIO",
> + .irq_mask = idt_gpio_mask,
> + .irq_ack = idt_gpio_ack,
> + .irq_unmask = idt_gpio_unmask,
> + .irq_set_type = idt_gpio_irq_set_type
> +};
> +
> +static int idt_gpio_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct gpio_irq_chip *girq;
> + struct idt_gpio_ctrl *ctrl;
> + unsigned int parent_irq;
> + int ngpios;
> + int ret;
> +
> + ret = device_property_read_u32(dev, "ngpios", &ngpios);
> + if (ret) {
> + dev_err(dev, "ngpios property is not valid\n");
> + return ret;
> + }
> +
> + ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
> + if (!ctrl)
> + return -ENOMEM;
> +
> + ctrl->gpio = devm_platform_ioremap_resource_byname(pdev, "gpio");
> + if (!ctrl->gpio)
> + return -ENOMEM;
> +
> + ctrl->gc.parent = dev;
> +
> + ret = bgpio_init(&ctrl->gc, &pdev->dev, 4, ctrl->gpio + IDT_GPIO_DATA,
> + NULL, NULL, ctrl->gpio + IDT_GPIO_DIR, NULL, 0);
> + if (ret) {
> + dev_err(dev, "bgpio_init failed\n");
> + return ret;
> + }
> + ctrl->gc.ngpio = ngpios;
> +
> + ctrl->pic = devm_platform_ioremap_resource_byname(pdev, "pic");
> + if (!ctrl->pic)
> + return -ENOMEM;
> +
> + parent_irq = platform_get_irq(pdev, 0);
> + if (!parent_irq)
> + return -EINVAL;
> +
> + girq = &ctrl->gc.irq;
> + girq->chip = &idt_gpio_irqchip;
> + girq->init_hw = idt_gpio_irq_init_hw;
> + girq->parent_handler = idt_gpio_dispatch;
> + girq->num_parents = 1;
> + girq->parents = devm_kcalloc(dev, girq->num_parents,
> + sizeof(*girq->parents), GFP_KERNEL);
> + if (!girq->parents)
> + return -ENOMEM;
> +
> + girq->parents[0] = parent_irq;
> + girq->default_type = IRQ_TYPE_NONE;
> + girq->handler = handle_bad_irq;
> +
> + spin_lock_init(&ctrl->irq_lock);
> +
> + return devm_gpiochip_add_data(&pdev->dev, &ctrl->gc, ctrl);
> +}
> +
> +static const struct of_device_id idt_gpio_of_match[] = {
> + { .compatible = "idt,32434-gpio" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, idt_gpio_of_match);
> +
> +static struct platform_driver idt_gpio_driver = {
> + .probe = idt_gpio_probe,
> + .driver = {
> + .name = "idt3243x-gpio",
> + .of_match_table = idt_gpio_of_match,
> + },
> +};
> +module_platform_driver(idt_gpio_driver);
> +
> +MODULE_DESCRIPTION("IDT 79RC3243x GPIO/PIC Driver");
> +MODULE_AUTHOR("Thomas Bogendoerfer <[email protected]>");
> +MODULE_LICENSE("GPL");
> --
> 2.29.2
>
--
With Best Regards,
Andy Shevchenko
Hi,
Am 2021-04-26 12:29, schrieb Andy Shevchenko:
> On Mon, Apr 26, 2021 at 12:55 PM Thomas Bogendoerfer
> <[email protected]> wrote:
>
> 2) there is gpio-regmap generic code, that may be worth
> considering.
This driver uses memory mapped registers. While that is
also possible with gpio-regmap, there is one drawback:
it assumes gpiochip->can_sleep = true for now, see [1].
Unfortunately, there is no easy way to ask the regmap
if its mmio/fastio.
-michael
[1]
https://elixir.bootlin.com/linux/v5.12/source/drivers/gpio/gpio-regmap.c#L257
On Wed, Apr 28, 2021 at 1:51 AM Michael Walle <[email protected]> wrote:
> Am 2021-04-26 12:29, schrieb Andy Shevchenko:
> > On Mon, Apr 26, 2021 at 12:55 PM Thomas Bogendoerfer
> > <[email protected]> wrote:
> >
> > 2) there is gpio-regmap generic code, that may be worth
> > considering.
>
> This driver uses memory mapped registers. While that is
> also possible with gpio-regmap, there is one drawback:
> it assumes gpiochip->can_sleep = true for now, see [1].
> Unfortunately, there is no easy way to ask the regmap
> if its mmio/fastio.
I don't see how it is an impediment.
Prerequisite patch?
> [1]
> https://elixir.bootlin.com/linux/v5.12/source/drivers/gpio/gpio-regmap.c#L257
--
With Best Regards,
Andy Shevchenko
Am 2021-04-28 13:07, schrieb Andy Shevchenko:
> On Wed, Apr 28, 2021 at 1:51 AM Michael Walle <[email protected]> wrote:
>> Am 2021-04-26 12:29, schrieb Andy Shevchenko:
>> > On Mon, Apr 26, 2021 at 12:55 PM Thomas Bogendoerfer
>> > <[email protected]> wrote:
>> >
>> > 2) there is gpio-regmap generic code, that may be worth
>> > considering.
>>
>> This driver uses memory mapped registers. While that is
>> also possible with gpio-regmap, there is one drawback:
>> it assumes gpiochip->can_sleep = true for now, see [1].
>> Unfortunately, there is no easy way to ask the regmap
>> if its mmio/fastio.
>
> I don't see how it is an impediment.
You'd have to use the *_cansleep() variants with the gpios,
which cannot be used everywhere, no?
-michael
On Wed, Apr 28, 2021 at 2:57 PM Michael Walle <[email protected]> wrote:
>
> Am 2021-04-28 13:07, schrieb Andy Shevchenko:
> > On Wed, Apr 28, 2021 at 1:51 AM Michael Walle <[email protected]> wrote:
> >> Am 2021-04-26 12:29, schrieb Andy Shevchenko:
> >> > On Mon, Apr 26, 2021 at 12:55 PM Thomas Bogendoerfer
> >> > <[email protected]> wrote:
> >> >
> >> > 2) there is gpio-regmap generic code, that may be worth
> >> > considering.
> >>
> >> This driver uses memory mapped registers. While that is
> >> also possible with gpio-regmap, there is one drawback:
> >> it assumes gpiochip->can_sleep = true for now, see [1].
> >> Unfortunately, there is no easy way to ask the regmap
> >> if its mmio/fastio.
> >
> > I don't see how it is an impediment.
>
> You'd have to use the *_cansleep() variants with the gpios,
> which cannot be used everywhere, no?
*can* sleep means that it requires a sleeping context to run, if your
controller is fine with that, there are no worries. OTOH if you want
to run this in an atomic context, then consumers can't do with that
kind of controller. What I meant above (and you stripped it here) is
to add a patch that will fix that and set it based on
gpio_regmap_config.
--
With Best Regards,
Andy Shevchenko
Am 2021-04-28 15:44, schrieb Andy Shevchenko:
> On Wed, Apr 28, 2021 at 2:57 PM Michael Walle <[email protected]> wrote:
>>
>> Am 2021-04-28 13:07, schrieb Andy Shevchenko:
>> > On Wed, Apr 28, 2021 at 1:51 AM Michael Walle <[email protected]> wrote:
>> >> Am 2021-04-26 12:29, schrieb Andy Shevchenko:
>> >> > On Mon, Apr 26, 2021 at 12:55 PM Thomas Bogendoerfer
>> >> > <[email protected]> wrote:
>> >> >
>> >> > 2) there is gpio-regmap generic code, that may be worth
>> >> > considering.
>> >>
>> >> This driver uses memory mapped registers. While that is
>> >> also possible with gpio-regmap, there is one drawback:
>> >> it assumes gpiochip->can_sleep = true for now, see [1].
>> >> Unfortunately, there is no easy way to ask the regmap
>> >> if its mmio/fastio.
>> >
>> > I don't see how it is an impediment.
>>
>> You'd have to use the *_cansleep() variants with the gpios,
>> which cannot be used everywhere, no?
>
> *can* sleep means that it requires a sleeping context to run, if your
> controller is fine with that, there are no worries. OTOH if you want
> to run this in an atomic context, then consumers can't do with that
> kind of controller.
Ok, then we are on the same track.
> What I meant above (and you stripped it here) is
> to add a patch that will fix that and set it based on
> gpio_regmap_config.
Yes, but ideally, it would ask the regmap. Otherwise that
information is redundant and might mismatch, i.e. gpio_regmap_config
tell can_sleep=false but the regmap is an I2C type for example. Also
if a driver wants to support both regmap types, we are no step
further.
-michael
[Adding Mark here, too]
Am 2021-04-28 16:32, schrieb Andy Shevchenko:
> On Wed, Apr 28, 2021 at 5:04 PM Michael Walle <[email protected]> wrote:
>> Am 2021-04-28 15:44, schrieb Andy Shevchenko:
>> > On Wed, Apr 28, 2021 at 2:57 PM Michael Walle <[email protected]> wrote:
>> >>
>> >> Am 2021-04-28 13:07, schrieb Andy Shevchenko:
>> >> > On Wed, Apr 28, 2021 at 1:51 AM Michael Walle <[email protected]> wrote:
>> >> >> Am 2021-04-26 12:29, schrieb Andy Shevchenko:
>> >> >> > On Mon, Apr 26, 2021 at 12:55 PM Thomas Bogendoerfer
>> >> >> > <[email protected]> wrote:
>> >> >> >
>> >> >> > 2) there is gpio-regmap generic code, that may be worth
>> >> >> > considering.
>> >> >>
>> >> >> This driver uses memory mapped registers. While that is
>> >> >> also possible with gpio-regmap, there is one drawback:
>> >> >> it assumes gpiochip->can_sleep = true for now, see [1].
>> >> >> Unfortunately, there is no easy way to ask the regmap
>> >> >> if its mmio/fastio.
>> >> >
>> >> > I don't see how it is an impediment.
>> >>
>> >> You'd have to use the *_cansleep() variants with the gpios,
>> >> which cannot be used everywhere, no?
>> >
>> > *can* sleep means that it requires a sleeping context to run, if your
>> > controller is fine with that, there are no worries. OTOH if you want
>> > to run this in an atomic context, then consumers can't do with that
>> > kind of controller.
>>
>> Ok, then we are on the same track.
>>
>> > What I meant above (and you stripped it here) is
>> > to add a patch that will fix that and set it based on
>> > gpio_regmap_config.
>>
>> Yes, but ideally, it would ask the regmap. Otherwise that
>> information is redundant and might mismatch, i.e. gpio_regmap_config
>> tell can_sleep=false but the regmap is an I2C type for example. Also
>> if a driver wants to support both regmap types, we are no step
>> further.
>
> Yeah, I agree that is a band aid, but you are free to fix it actually
> on regmap level.
> I don't think it will require an enormous amount of work there.
I'd love to fix that, but Mark was against exposing that property
outside of regmap. So it it what it is for now ;) Maybe he'll change
his mind or someone has another idea.
-michael
On Wed, Apr 28, 2021 at 5:48 PM Michael Walle <[email protected]> wrote:
>
> [Adding Mark here, too]
>
> Am 2021-04-28 16:32, schrieb Andy Shevchenko:
> > On Wed, Apr 28, 2021 at 5:04 PM Michael Walle <[email protected]> wrote:
> >> Am 2021-04-28 15:44, schrieb Andy Shevchenko:
> >> > On Wed, Apr 28, 2021 at 2:57 PM Michael Walle <[email protected]> wrote:
> >> >>
> >> >> Am 2021-04-28 13:07, schrieb Andy Shevchenko:
> >> >> > On Wed, Apr 28, 2021 at 1:51 AM Michael Walle <[email protected]> wrote:
> >> >> >> Am 2021-04-26 12:29, schrieb Andy Shevchenko:
> >> >> >> > On Mon, Apr 26, 2021 at 12:55 PM Thomas Bogendoerfer
> >> >> >> > <[email protected]> wrote:
> >> >> >> >
> >> >> >> > 2) there is gpio-regmap generic code, that may be worth
> >> >> >> > considering.
> >> >> >>
> >> >> >> This driver uses memory mapped registers. While that is
> >> >> >> also possible with gpio-regmap, there is one drawback:
> >> >> >> it assumes gpiochip->can_sleep = true for now, see [1].
> >> >> >> Unfortunately, there is no easy way to ask the regmap
> >> >> >> if its mmio/fastio.
> >> >> >
> >> >> > I don't see how it is an impediment.
> >> >>
> >> >> You'd have to use the *_cansleep() variants with the gpios,
> >> >> which cannot be used everywhere, no?
> >> >
> >> > *can* sleep means that it requires a sleeping context to run, if your
> >> > controller is fine with that, there are no worries. OTOH if you want
> >> > to run this in an atomic context, then consumers can't do with that
> >> > kind of controller.
> >>
> >> Ok, then we are on the same track.
> >>
> >> > What I meant above (and you stripped it here) is
> >> > to add a patch that will fix that and set it based on
> >> > gpio_regmap_config.
> >>
> >> Yes, but ideally, it would ask the regmap. Otherwise that
> >> information is redundant and might mismatch, i.e. gpio_regmap_config
> >> tell can_sleep=false but the regmap is an I2C type for example. Also
> >> if a driver wants to support both regmap types, we are no step
> >> further.
> >
> > Yeah, I agree that is a band aid, but you are free to fix it actually
> > on regmap level.
> > I don't think it will require an enormous amount of work there.
>
> I'd love to fix that, but Mark was against exposing that property
> outside of regmap. So it it what it is for now ;) Maybe he'll change
> his mind or someone has another idea.
Then let's go to ugly variant with duplicating it in gpio-regmap
config. with a FIXME note or so. I don't think we should allow new
drivers be based on bgpio_init().
--
With Best Regards,
Andy Shevchenko
Am 2021-04-28 17:02, schrieb Andy Shevchenko:
> On Wed, Apr 28, 2021 at 5:48 PM Michael Walle <[email protected]> wrote:
>>
>> [Adding Mark here, too]
>>
>> Am 2021-04-28 16:32, schrieb Andy Shevchenko:
>> > On Wed, Apr 28, 2021 at 5:04 PM Michael Walle <[email protected]> wrote:
>> >> Am 2021-04-28 15:44, schrieb Andy Shevchenko:
>> >> > On Wed, Apr 28, 2021 at 2:57 PM Michael Walle <[email protected]> wrote:
>> >> >>
>> >> >> Am 2021-04-28 13:07, schrieb Andy Shevchenko:
>> >> >> > On Wed, Apr 28, 2021 at 1:51 AM Michael Walle <[email protected]> wrote:
>> >> >> >> Am 2021-04-26 12:29, schrieb Andy Shevchenko:
>> >> >> >> > On Mon, Apr 26, 2021 at 12:55 PM Thomas Bogendoerfer
>> >> >> >> > <[email protected]> wrote:
>> >> >> >> >
>> >> >> >> > 2) there is gpio-regmap generic code, that may be worth
>> >> >> >> > considering.
>> >> >> >>
>> >> >> >> This driver uses memory mapped registers. While that is
>> >> >> >> also possible with gpio-regmap, there is one drawback:
>> >> >> >> it assumes gpiochip->can_sleep = true for now, see [1].
>> >> >> >> Unfortunately, there is no easy way to ask the regmap
>> >> >> >> if its mmio/fastio.
>> >> >> >
>> >> >> > I don't see how it is an impediment.
>> >> >>
>> >> >> You'd have to use the *_cansleep() variants with the gpios,
>> >> >> which cannot be used everywhere, no?
>> >> >
>> >> > *can* sleep means that it requires a sleeping context to run, if your
>> >> > controller is fine with that, there are no worries. OTOH if you want
>> >> > to run this in an atomic context, then consumers can't do with that
>> >> > kind of controller.
>> >>
>> >> Ok, then we are on the same track.
>> >>
>> >> > What I meant above (and you stripped it here) is
>> >> > to add a patch that will fix that and set it based on
>> >> > gpio_regmap_config.
>> >>
>> >> Yes, but ideally, it would ask the regmap. Otherwise that
>> >> information is redundant and might mismatch, i.e. gpio_regmap_config
>> >> tell can_sleep=false but the regmap is an I2C type for example. Also
>> >> if a driver wants to support both regmap types, we are no step
>> >> further.
>> >
>> > Yeah, I agree that is a band aid, but you are free to fix it actually
>> > on regmap level.
>> > I don't think it will require an enormous amount of work there.
>>
>> I'd love to fix that, but Mark was against exposing that property
>> outside of regmap. So it it what it is for now ;) Maybe he'll change
>> his mind or someone has another idea.
>
> Then let's go to ugly variant with duplicating it in gpio-regmap
> config. with a FIXME note or so. I don't think we should allow new
> drivers be based on bgpio_init().
Agreed, given that a possible fix should be easy enough later.
-michael
On Wed, Apr 28, 2021 at 5:04 PM Michael Walle <[email protected]> wrote:
> Am 2021-04-28 15:44, schrieb Andy Shevchenko:
> > On Wed, Apr 28, 2021 at 2:57 PM Michael Walle <[email protected]> wrote:
> >>
> >> Am 2021-04-28 13:07, schrieb Andy Shevchenko:
> >> > On Wed, Apr 28, 2021 at 1:51 AM Michael Walle <[email protected]> wrote:
> >> >> Am 2021-04-26 12:29, schrieb Andy Shevchenko:
> >> >> > On Mon, Apr 26, 2021 at 12:55 PM Thomas Bogendoerfer
> >> >> > <[email protected]> wrote:
> >> >> >
> >> >> > 2) there is gpio-regmap generic code, that may be worth
> >> >> > considering.
> >> >>
> >> >> This driver uses memory mapped registers. While that is
> >> >> also possible with gpio-regmap, there is one drawback:
> >> >> it assumes gpiochip->can_sleep = true for now, see [1].
> >> >> Unfortunately, there is no easy way to ask the regmap
> >> >> if its mmio/fastio.
> >> >
> >> > I don't see how it is an impediment.
> >>
> >> You'd have to use the *_cansleep() variants with the gpios,
> >> which cannot be used everywhere, no?
> >
> > *can* sleep means that it requires a sleeping context to run, if your
> > controller is fine with that, there are no worries. OTOH if you want
> > to run this in an atomic context, then consumers can't do with that
> > kind of controller.
>
> Ok, then we are on the same track.
>
> > What I meant above (and you stripped it here) is
> > to add a patch that will fix that and set it based on
> > gpio_regmap_config.
>
> Yes, but ideally, it would ask the regmap. Otherwise that
> information is redundant and might mismatch, i.e. gpio_regmap_config
> tell can_sleep=false but the regmap is an I2C type for example. Also
> if a driver wants to support both regmap types, we are no step
> further.
Yeah, I agree that is a band aid, but you are free to fix it actually
on regmap level.
I don't think it will require an enormous amount of work there.
We have time :-)
--
With Best Regards,
Andy Shevchenko
On Mon, 26 Apr 2021 11:54:26 +0200, Thomas Bogendoerfer wrote:
> Add YAML devicetree binding for IDT 79RC32434 GPIO controller
>
> Signed-off-by: Thomas Bogendoerfer <[email protected]>
> ---
> Changes in v4:
> - renamed to idt,32434-gpio this time for real
>
> Changes in v3:
> - renamed to idt,32434-gpio
> - drop ngpio description
> - use gpio0: gpio@50004 in example
>
> .../bindings/gpio/idt,32434-gpio.yaml | 71 +++++++++++++++++++
> 1 file changed, 71 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/gpio/idt,32434-gpio.yaml
>
Reviewed-by: Rob Herring <[email protected]>
On Mon, Apr 26, 2021 at 11:54 AM Thomas Bogendoerfer
<[email protected]> wrote:
> Add YAML devicetree binding for IDT 79RC32434 GPIO controller
>
> Signed-off-by: Thomas Bogendoerfer <[email protected]>
> ---
> Changes in v4:
> - renamed to idt,32434-gpio this time for real
Overall looks good to me.
> +required:
(...)
> + - ngpios
Is there a *technical* reason why this is required?
Can't the driver just default to 32 gpios when not specified?
> + - interrupt-controller
> + - "#interrupt-cells"
> + - interrupts
Why can't interrupt support be made optional?
It is fine if the driver errors out if not provided, but
for the bindings this feels optional.
Or does the thing break unless you handle the IRQs?
Yours,
Linus Walleij
On Sat, May 1, 2021 at 7:13 AM Linus Walleij <[email protected]> wrote:
>
> On Mon, Apr 26, 2021 at 11:54 AM Thomas Bogendoerfer
> <[email protected]> wrote:
>
> > Add YAML devicetree binding for IDT 79RC32434 GPIO controller
> >
> > Signed-off-by: Thomas Bogendoerfer <[email protected]>
> > ---
> > Changes in v4:
> > - renamed to idt,32434-gpio this time for real
>
> Overall looks good to me.
>
> > +required:
> (...)
> > + - ngpios
>
> Is there a *technical* reason why this is required?
>
> Can't the driver just default to 32 gpios when not specified?
>
> > + - interrupt-controller
> > + - "#interrupt-cells"
> > + - interrupts
>
> Why can't interrupt support be made optional?
>
> It is fine if the driver errors out if not provided, but
> for the bindings this feels optional.
>
> Or does the thing break unless you handle the IRQs?
If the hardware has interrupts, then we should describe that. It's the
OS driver that may or may not support interrupts.
Rob
On Tue, May 4, 2021 at 3:44 PM Rob Herring <[email protected]> wrote:
> On Sat, May 1, 2021 at 7:13 AM Linus Walleij <[email protected]> wrote:
> > Why can't interrupt support be made optional?
(...)
>
> If the hardware has interrupts, then we should describe that. It's the
> OS driver that may or may not support interrupts.
You're right of course. What was I thinking.
Yours,
Linus Walleij
On Sat, May 01, 2021 at 02:13:35PM +0200, Linus Walleij wrote:
> On Mon, Apr 26, 2021 at 11:54 AM Thomas Bogendoerfer
> <[email protected]> wrote:
>
> > Add YAML devicetree binding for IDT 79RC32434 GPIO controller
> >
> > Signed-off-by: Thomas Bogendoerfer <[email protected]>
> > ---
> > Changes in v4:
> > - renamed to idt,32434-gpio this time for real
>
> Overall looks good to me.
>
> > +required:
> (...)
> > + - ngpios
>
> Is there a *technical* reason why this is required?
>
> Can't the driver just default to 32 gpios when not specified?
sure, I make it optional.
> > + - interrupt-controller
> > + - "#interrupt-cells"
> > + - interrupts
>
> Why can't interrupt support be made optional?
>
> It is fine if the driver errors out if not provided, but
> for the bindings this feels optional.
I'll make them optional.
> Or does the thing break unless you handle the IRQs?
no, they could be used just as GPIOs.
Thomas.
--
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea. [ RFC1925, 2.3 ]