| Message ID | 20201201181406.2371881-1-nobuhiro1.iwamatsu@toshiba.co.jp |
|---|---|
| Headers | show |
| Series | gpio: visconti: Add Toshiba Visconti GPIO support | expand |
Iwamatsu-san, Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> writes: > Add bindings for the Toshiba Visconti GPIO Controller. > > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> > --- > .../bindings/gpio/toshiba,gpio-visconti.yaml | 85 +++++++++++++++++++ > 1 file changed, 85 insertions(+) > create mode 100644 Documentation/devicetree/bindings/gpio/toshiba,gpio-visconti.yaml > > diff --git a/Documentation/devicetree/bindings/gpio/toshiba,gpio-visconti.yaml b/Documentation/devicetree/bindings/gpio/toshiba,gpio-visconti.yaml > new file mode 100644 > index 000000000000..5168a15b90e1 > --- /dev/null > +++ b/Documentation/devicetree/bindings/gpio/toshiba,gpio-visconti.yaml > @@ -0,0 +1,85 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/gpio/toshiba,gpio-visconti.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Toshiba Visconti ARM SoCs GPIO controller > + > +maintainers: > + - Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> > + > +properties: > + compatible: > + items: > + - const: toshiba,gpio-tmpv7708 > + > + reg: > + maxItems: 1 > + > + "#gpio-cells": > + const: 2 > + > + gpio-ranges: true I am not sure I have a good handle on the yaml schema definitions but "gpio-ranges" feels like it should be a list of ranges not a boolean. Something like - gpio-ranges: maxItems: 1 feels more appropriate. I see both the usages in gpio bindings and for other range properties so maybe it's OK. I hope Rob or somebody more knowledgeable on this can clarify the usage. Otherwise, the patch looks good. Thanks, Punit > + > + gpio-controller: true > + > + interrupt-controller: true > + > + "#interrupt-cells": > + const: 2 > + > + interrupts: > + description: > + interrupt mapping one per GPIO. > + minItems: 16 > + maxItems: 16 > + > +required: > + - compatible > + - reg > + - "#gpio-cells" > + - gpio-ranges > + - gpio-controller > + - interrupt-controller > + - "#interrupt-cells" > + - interrupts > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/irq.h> > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + > + soc { > + #address-cells = <2>; > + #size-cells = <2>; > + > + gpio: gpio@28020000 { > + compatible = "toshiba,gpio-tmpv7708"; > + reg = <0 0x28020000 0 0x1000>; > + #gpio-cells = <0x2>; > + gpio-ranges = <&pmux 0 0 32>; > + gpio-controller; > + interrupt-controller; > + #interrupt-cells = <2>; > + interrupts = <GIC_SPI 24 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 28 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 35 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>; > + }; > + }; > +...
Hi Iwamatsu-san, A couple of very minor comments below - Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> writes: > Add the GPIO driver for Toshiba Visconti ARM SoCs. > > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> > --- > drivers/gpio/Kconfig | 9 + > drivers/gpio/Makefile | 1 + > drivers/gpio/gpio-visconti.c | 232 ++++++++++++++++++++++ > drivers/pinctrl/visconti/pinctrl-common.c | 23 +++ > 4 files changed, 265 insertions(+) > create mode 100644 drivers/gpio/gpio-visconti.c > [...] > diff --git a/drivers/gpio/gpio-visconti.c b/drivers/gpio/gpio-visconti.c > new file mode 100644 > index 000000000000..17e6da161f48 > --- /dev/null > +++ b/drivers/gpio/gpio-visconti.c > @@ -0,0 +1,232 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Toshiba Visconti GPIO Support > + * > + * (C) Copyright 2020 Toshiba Electronic Devices & Storage Corporation > + * (C) Copyright 2020 Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> > + */ > + > +#include <linux/init.h> > +#include <linux/interrupt.h> > +#include <linux/module.h> > +#include <linux/io.h> > +#include <linux/platform_device.h> > +#include <linux/gpio/driver.h> > +#include <linux/of.h> > +#include <linux/bitops.h> > + > +/* register offset */ > +#define GPIO_DIR 0x00 > +#define GPIO_IDATA 0x08 > +#define GPIO_ODATA 0x10 > +#define GPIO_OSET 0x18 > +#define GPIO_OCLR 0x20 > +#define GPIO_INTMODE 0x30 > + > +#define VISCONTI_GPIO_NR 32 The macro doesn't seem to be used. Can we drop this? On the other hand should there be a macro to indicate the maximum number of GPIO lines that support interrupt (16)? > + > +struct visconti_gpio { > + void __iomem *base; > + int *irq; > + spinlock_t lock; /* protect gpio register */ > + struct device *dev; > + struct gpio_chip gpio_chip; > + struct irq_chip irq_chip; > +}; > + > +static void visconti_gpio_irq_mask(struct irq_data *d) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct visconti_gpio *priv = gpiochip_get_data(gc); > + > + disable_irq_nosync(priv->irq[irqd_to_hwirq(d)]); > +} > + > +static void visconti_gpio_irq_unmask(struct irq_data *d) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct visconti_gpio *priv = gpiochip_get_data(gc); > + > + enable_irq(priv->irq[irqd_to_hwirq(d)]); > +} > + > +static int visconti_gpio_irq_set_type(struct irq_data *d, unsigned int type) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct visconti_gpio *priv = gpiochip_get_data(gc); > + u32 offset = irqd_to_hwirq(d); > + u32 bit = BIT(offset); > + u32 intc_type = IRQ_TYPE_EDGE_RISING; > + u32 intmode, odata; > + int ret = 0; > + unsigned long flags; > + > + spin_lock_irqsave(&priv->lock, flags); > + > + odata = readl(priv->base + GPIO_ODATA); > + intmode = readl(priv->base + GPIO_INTMODE); > + > + switch (type) { > + case IRQ_TYPE_EDGE_RISING: > + odata &= ~bit; > + intmode &= ~bit; > + break; > + case IRQ_TYPE_EDGE_FALLING: > + odata |= bit; > + intmode &= ~bit; > + break; > + case IRQ_TYPE_EDGE_BOTH: > + intmode |= bit; > + break; > + case IRQ_TYPE_LEVEL_HIGH: > + intc_type = IRQ_TYPE_LEVEL_HIGH; > + odata &= ~bit; > + intmode &= ~bit; > + break; > + case IRQ_TYPE_LEVEL_LOW: > + intc_type = IRQ_TYPE_LEVEL_HIGH; > + odata |= bit; > + intmode &= ~bit; > + break; > + default: > + ret = -EINVAL; > + goto err; > + } > + > + writel(odata, priv->base + GPIO_ODATA); > + writel(intmode, priv->base + GPIO_INTMODE); > + irq_set_irq_type(priv->irq[offset], intc_type); > +err: > + spin_unlock_irqrestore(&priv->lock, flags); > + > + return ret; > +} > + > +static irqreturn_t visconti_gpio_irq_handler(int irq, void *dev_id) > +{ > + struct visconti_gpio *priv = dev_id; > + u32 offset = irq - priv->irq[0]; > + > + generic_handle_irq(irq_find_mapping(priv->gpio_chip.irq.domain, offset)); > + > + return IRQ_HANDLED; > +} > + > +static void visconti_init_irq_valid_mask(struct gpio_chip *chip, unsigned long *valid_mask, > + unsigned int ngpios) > +{ > + int i; > + > + /* Exclude GPIO pins 16-31 from irq */ > + for (i = 16; i < ngpios; i++) > + clear_bit(i, valid_mask); > +} > + > +static int visconti_gpio_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct visconti_gpio *priv; > + struct irq_chip *irq_chip; > + struct irq_desc *desc; > + struct gpio_irq_chip *girq; > + const char *name = dev_name(dev); > + int i, ret, num_irq; > + > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->dev = dev; > + spin_lock_init(&priv->lock); > + > + priv->base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(priv->base)) > + return PTR_ERR(priv->base); > + > + ret = platform_irq_count(pdev); > + if (!ret) { > + dev_err(dev, "Couldn't determine # GPIO banks\n"); > + return -ENOENT; > + } platform_irq_count() can return -EPROBE_DEFER. Is that something that should be handled? With the above two comments addressed, feel free to add Reviewed-by: Punit Agrawal <punit1.agrawal@toshiba.co.jp> Thanks, Punit > + num_irq = ret; > + > + priv->irq = devm_kcalloc(dev, num_irq, sizeof(priv->irq), GFP_KERNEL); > + if (!priv->irq) > + return -ENOMEM; > + > + for (i = 0; i < num_irq; i++) { > + priv->irq[i] = platform_get_irq(pdev, i); > + if (priv->irq[i] < 0) { > + dev_err(dev, "invalid IRQ[%d]\n", i); > + return priv->irq[i]; > + } > + } > + > + ret = bgpio_init(&priv->gpio_chip, dev, 4, > + priv->base + GPIO_IDATA, > + priv->base + GPIO_OSET, > + priv->base + GPIO_OCLR, > + priv->base + GPIO_DIR, > + NULL, > + 0); > + if (ret) { > + dev_err(dev, "unable to init generic GPIO\n"); > + return ret; > + } > + > + priv->gpio_chip.irq.init_valid_mask = visconti_init_irq_valid_mask; > + > + irq_chip = &priv->irq_chip; > + irq_chip->name = "gpio-visconti"; > + irq_chip->irq_mask = visconti_gpio_irq_mask; > + irq_chip->irq_unmask = visconti_gpio_irq_unmask; > + irq_chip->irq_set_type = visconti_gpio_irq_set_type; > + irq_chip->flags = IRQCHIP_SET_TYPE_MASKED | IRQCHIP_MASK_ON_SUSPEND; > + > + girq = &priv->gpio_chip.irq; > + girq->chip = irq_chip; > + /* 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_level_irq; > + > + ret = devm_gpiochip_add_data(dev, &priv->gpio_chip, priv); > + if (ret) { > + dev_err(dev, "failed to add GPIO chip\n"); > + return ret; > + } > + > + for (i = 0; i < num_irq; i++) { > + desc = irq_to_desc(priv->irq[i]); > + desc->status_use_accessors |= IRQ_NOAUTOEN; > + if (devm_request_irq(dev, priv->irq[i], > + visconti_gpio_irq_handler, 0, name, priv)) { > + dev_err(dev, "failed to request IRQ[%d]\n", i); > + return -ENOENT; > + } > + } > + > + return ret; > +} > + > +static const struct of_device_id visconti_gpio_of_match[] = { > + { .compatible = "toshiba,gpio-tmpv7708", }, > + { /* end of table */ } > +}; > +MODULE_DEVICE_TABLE(of, visconti_gpio_of_match); > + > +static struct platform_driver visconti_gpio_driver = { > + .probe = visconti_gpio_probe, > + .driver = { > + .name = "visconti_gpio", > + .of_match_table = of_match_ptr(visconti_gpio_of_match), > + } > +}; > +module_platform_driver(visconti_gpio_driver); > + > +MODULE_AUTHOR("Toshiba Electronic Devices & Storage Corporation"); > +MODULE_AUTHOR("Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>"); > +MODULE_DESCRIPTION("Toshiba Visconti GPIO Driver"); > +MODULE_LICENSE("GPL v2"); [...]
On Thu, Dec 03, 2020 at 05:16:51PM +0900, Punit Agrawal wrote: > Iwamatsu-san, > > Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> writes: > > > Add bindings for the Toshiba Visconti GPIO Controller. > > > > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> > > --- > > .../bindings/gpio/toshiba,gpio-visconti.yaml | 85 +++++++++++++++++++ > > 1 file changed, 85 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/gpio/toshiba,gpio-visconti.yaml > > > > diff --git a/Documentation/devicetree/bindings/gpio/toshiba,gpio-visconti.yaml b/Documentation/devicetree/bindings/gpio/toshiba,gpio-visconti.yaml > > new file mode 100644 > > index 000000000000..5168a15b90e1 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/gpio/toshiba,gpio-visconti.yaml > > @@ -0,0 +1,85 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/gpio/toshiba,gpio-visconti.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Toshiba Visconti ARM SoCs GPIO controller > > + > > +maintainers: > > + - Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> > > + > > +properties: > > + compatible: > > + items: > > + - const: toshiba,gpio-tmpv7708 > > + > > + reg: > > + maxItems: 1 > > + > > + "#gpio-cells": > > + const: 2 > > + > > + gpio-ranges: true > > I am not sure I have a good handle on the yaml schema definitions but > "gpio-ranges" feels like it should be a list of ranges not a boolean. > > Something like - > > gpio-ranges: > maxItems: 1 > > feels more appropriate. > > I see both the usages in gpio bindings and for other range properties so > maybe it's OK. I hope Rob or somebody more knowledgeable on this can > clarify the usage. If you know how many (or a range) entries there are for gpio-ranges, then maxItems is good. If you don't, then 'gpio-ranges: true' is fine. That doesn't make the property a boolean, but just says the property can be present. Rob
On Wed, 02 Dec 2020 03:14:03 +0900, Nobuhiro Iwamatsu wrote: > Add bindings for the Toshiba Visconti GPIO Controller. > > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> > --- > .../bindings/gpio/toshiba,gpio-visconti.yaml | 85 +++++++++++++++++++ > 1 file changed, 85 insertions(+) > create mode 100644 Documentation/devicetree/bindings/gpio/toshiba,gpio-visconti.yaml > Reviewed-by: Rob Herring <robh@kernel.org>
Rob Herring <robh@kernel.org> writes: [...] >> > + gpio-ranges: true >> >> I am not sure I have a good handle on the yaml schema definitions but >> "gpio-ranges" feels like it should be a list of ranges not a boolean. >> >> Something like - >> >> gpio-ranges: >> maxItems: 1 >> >> feels more appropriate. >> >> I see both the usages in gpio bindings and for other range properties so >> maybe it's OK. I hope Rob or somebody more knowledgeable on this can >> clarify the usage. > > If you know how many (or a range) entries there are for gpio-ranges, > then maxItems is good. If you don't, then 'gpio-ranges: true' is fine. > That doesn't make the property a boolean, but just says the property can > be present. Makes sense. Thanks for the explanation. [...]
Hi, Thanks for your review. On Thu, Dec 03, 2020 at 07:03:27PM +0900, Punit Agrawal wrote: > Hi Iwamatsu-san, > > A couple of very minor comments below - > > Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> writes: > > > Add the GPIO driver for Toshiba Visconti ARM SoCs. > > > > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> > > --- > > drivers/gpio/Kconfig | 9 + > > drivers/gpio/Makefile | 1 + > > drivers/gpio/gpio-visconti.c | 232 ++++++++++++++++++++++ > > drivers/pinctrl/visconti/pinctrl-common.c | 23 +++ > > 4 files changed, 265 insertions(+) > > create mode 100644 drivers/gpio/gpio-visconti.c > > > > [...] > > > diff --git a/drivers/gpio/gpio-visconti.c b/drivers/gpio/gpio-visconti.c > > new file mode 100644 > > index 000000000000..17e6da161f48 > > --- /dev/null > > +++ b/drivers/gpio/gpio-visconti.c > > @@ -0,0 +1,232 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Toshiba Visconti GPIO Support > > + * > > + * (C) Copyright 2020 Toshiba Electronic Devices & Storage Corporation > > + * (C) Copyright 2020 Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> > > + */ > > + > > +#include <linux/init.h> > > +#include <linux/interrupt.h> > > +#include <linux/module.h> > > +#include <linux/io.h> > > +#include <linux/platform_device.h> > > +#include <linux/gpio/driver.h> > > +#include <linux/of.h> > > +#include <linux/bitops.h> > > + > > +/* register offset */ > > +#define GPIO_DIR 0x00 > > +#define GPIO_IDATA 0x08 > > +#define GPIO_ODATA 0x10 > > +#define GPIO_OSET 0x18 > > +#define GPIO_OCLR 0x20 > > +#define GPIO_INTMODE 0x30 > > + > > +#define VISCONTI_GPIO_NR 32 > > The macro doesn't seem to be used. Can we drop this? On the other hand > should there be a macro to indicate the maximum number of GPIO lines > that support interrupt (16)? Thanks, I will drop this macro. And about interrutp, it supports with this patch. > > > + > > +struct visconti_gpio { > > + void __iomem *base; > > + int *irq; > > + spinlock_t lock; /* protect gpio register */ > > + struct device *dev; > > + struct gpio_chip gpio_chip; > > + struct irq_chip irq_chip; > > +}; > > + > > +static void visconti_gpio_irq_mask(struct irq_data *d) > > +{ > > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > > + struct visconti_gpio *priv = gpiochip_get_data(gc); > > + > > + disable_irq_nosync(priv->irq[irqd_to_hwirq(d)]); > > +} > > + > > +static void visconti_gpio_irq_unmask(struct irq_data *d) > > +{ > > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > > + struct visconti_gpio *priv = gpiochip_get_data(gc); > > + > > + enable_irq(priv->irq[irqd_to_hwirq(d)]); > > +} > > + > > +static int visconti_gpio_irq_set_type(struct irq_data *d, unsigned int type) > > +{ > > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > > + struct visconti_gpio *priv = gpiochip_get_data(gc); > > + u32 offset = irqd_to_hwirq(d); > > + u32 bit = BIT(offset); > > + u32 intc_type = IRQ_TYPE_EDGE_RISING; > > + u32 intmode, odata; > > + int ret = 0; > > + unsigned long flags; > > + > > + spin_lock_irqsave(&priv->lock, flags); > > + > > + odata = readl(priv->base + GPIO_ODATA); > > + intmode = readl(priv->base + GPIO_INTMODE); > > + > > + switch (type) { > > + case IRQ_TYPE_EDGE_RISING: > > + odata &= ~bit; > > + intmode &= ~bit; > > + break; > > + case IRQ_TYPE_EDGE_FALLING: > > + odata |= bit; > > + intmode &= ~bit; > > + break; > > + case IRQ_TYPE_EDGE_BOTH: > > + intmode |= bit; > > + break; > > + case IRQ_TYPE_LEVEL_HIGH: > > + intc_type = IRQ_TYPE_LEVEL_HIGH; > > + odata &= ~bit; > > + intmode &= ~bit; > > + break; > > + case IRQ_TYPE_LEVEL_LOW: > > + intc_type = IRQ_TYPE_LEVEL_HIGH; > > + odata |= bit; > > + intmode &= ~bit; > > + break; > > + default: > > + ret = -EINVAL; > > + goto err; > > + } > > + > > + writel(odata, priv->base + GPIO_ODATA); > > + writel(intmode, priv->base + GPIO_INTMODE); > > + irq_set_irq_type(priv->irq[offset], intc_type); > > +err: > > + spin_unlock_irqrestore(&priv->lock, flags); > > + > > + return ret; > > +} > > + > > +static irqreturn_t visconti_gpio_irq_handler(int irq, void *dev_id) > > +{ > > + struct visconti_gpio *priv = dev_id; > > + u32 offset = irq - priv->irq[0]; > > + > > + generic_handle_irq(irq_find_mapping(priv->gpio_chip.irq.domain, offset)); > > + > > + return IRQ_HANDLED; > > +} > > + > > +static void visconti_init_irq_valid_mask(struct gpio_chip *chip, unsigned long *valid_mask, > > + unsigned int ngpios) > > +{ > > + int i; > > + > > + /* Exclude GPIO pins 16-31 from irq */ > > + for (i = 16; i < ngpios; i++) > > + clear_bit(i, valid_mask); > > +} > > + > > +static int visconti_gpio_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct visconti_gpio *priv; > > + struct irq_chip *irq_chip; > > + struct irq_desc *desc; > > + struct gpio_irq_chip *girq; > > + const char *name = dev_name(dev); > > + int i, ret, num_irq; > > + > > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > + > > + priv->dev = dev; > > + spin_lock_init(&priv->lock); > > + > > + priv->base = devm_platform_ioremap_resource(pdev, 0); > > + if (IS_ERR(priv->base)) > > + return PTR_ERR(priv->base); > > + > > + ret = platform_irq_count(pdev); > > + if (!ret) { > > + dev_err(dev, "Couldn't determine # GPIO banks\n"); > > + return -ENOENT; > > + } > > platform_irq_count() can return -EPROBE_DEFER. Is that something that > should be handled? I see. In case of an error, I will change it to return ret directly. > > With the above two comments addressed, feel free to add > > Reviewed-by: Punit Agrawal <punit1.agrawal@toshiba.co.jp> > Thank you. > Thanks, > Punit > Best regards., Nobuhiro > > > + num_irq = ret; > > + > > + priv->irq = devm_kcalloc(dev, num_irq, sizeof(priv->irq), GFP_KERNEL); > > + if (!priv->irq) > > + return -ENOMEM; > > + > > + for (i = 0; i < num_irq; i++) { > > + priv->irq[i] = platform_get_irq(pdev, i); > > + if (priv->irq[i] < 0) { > > + dev_err(dev, "invalid IRQ[%d]\n", i); > > + return priv->irq[i]; > > + } > > + } > > + > > + ret = bgpio_init(&priv->gpio_chip, dev, 4, > > + priv->base + GPIO_IDATA, > > + priv->base + GPIO_OSET, > > + priv->base + GPIO_OCLR, > > + priv->base + GPIO_DIR, > > + NULL, > > + 0); > > + if (ret) { > > + dev_err(dev, "unable to init generic GPIO\n"); > > + return ret; > > + } > > + > > + priv->gpio_chip.irq.init_valid_mask = visconti_init_irq_valid_mask; > > + > > + irq_chip = &priv->irq_chip; > > + irq_chip->name = "gpio-visconti"; > > + irq_chip->irq_mask = visconti_gpio_irq_mask; > > + irq_chip->irq_unmask = visconti_gpio_irq_unmask; > > + irq_chip->irq_set_type = visconti_gpio_irq_set_type; > > + irq_chip->flags = IRQCHIP_SET_TYPE_MASKED | IRQCHIP_MASK_ON_SUSPEND; > > + > > + girq = &priv->gpio_chip.irq; > > + girq->chip = irq_chip; > > + /* 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_level_irq; > > + > > + ret = devm_gpiochip_add_data(dev, &priv->gpio_chip, priv); > > + if (ret) { > > + dev_err(dev, "failed to add GPIO chip\n"); > > + return ret; > > + } > > + > > + for (i = 0; i < num_irq; i++) { > > + desc = irq_to_desc(priv->irq[i]); > > + desc->status_use_accessors |= IRQ_NOAUTOEN; > > + if (devm_request_irq(dev, priv->irq[i], > > + visconti_gpio_irq_handler, 0, name, priv)) { > > + dev_err(dev, "failed to request IRQ[%d]\n", i); > > + return -ENOENT; > > + } > > + } > > + > > + return ret; > > +} > > + > > +static const struct of_device_id visconti_gpio_of_match[] = { > > + { .compatible = "toshiba,gpio-tmpv7708", }, > > + { /* end of table */ } > > +}; > > +MODULE_DEVICE_TABLE(of, visconti_gpio_of_match); > > + > > +static struct platform_driver visconti_gpio_driver = { > > + .probe = visconti_gpio_probe, > > + .driver = { > > + .name = "visconti_gpio", > > + .of_match_table = of_match_ptr(visconti_gpio_of_match), > > + } > > +}; > > +module_platform_driver(visconti_gpio_driver); > > + > > +MODULE_AUTHOR("Toshiba Electronic Devices & Storage Corporation"); > > +MODULE_AUTHOR("Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>"); > > +MODULE_DESCRIPTION("Toshiba Visconti GPIO Driver"); > > +MODULE_LICENSE("GPL v2"); > > [...] > >
On Tue, Dec 1, 2020 at 10:16 AM Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> wrote: > > Hi, > > This series is the GPIO driver for Toshiba's ARM SoC, Visconti[0]. > This provides DT binding documentation, device driver, MAINTAINER files, and updates to DT files. > > Update: > > dt-bindings: gpio: Add bindings for Toshiba Visconti GPIO Controller: > v2 -> v3: Fix dtschema/dtc warnings. > dtschema/dtc warnings/errors: > Documentation/devicetree/bindings/gpio/toshiba,gpio-visconti.example.dt.yaml: gpio@28020000: interrupts: [[0, 24, 4], [0, 25, 4], [0, 26, 4], [0, 27, 4], [0, 28, 4], [0, 29, 4], [0, 30, 4], [0, 31, 4], [0, 32, 4], [0, 33, 4], [0, 34, 4], [0, 35, 4], [0, 36, 4], [0, 37, 4], [0, 38, 4], [0, 39, 4]] is too short > From schema: Documentation/devicetree/bindings/gpio/toshiba,gpio-visconti.yaml > v1 -> v2: Fix typo. > > gpio: visoconti: Add Toshiba Visconti GPIO support: > v2 -> v3: Add select GPIO_GENERIC > Use genric MMIO GPIO library > Use bgpio_init() as initialized the generic helpers. > Use irqchip template instead of gpiochip_irqchip_add(). > v1 -> v2: No update > > MAINTAINERS: Add entries for Toshiba Visconti GPIO controller: > v2 -> v3: No update > v1 -> v2: No update > > arm: dts: visconti: Add DT support for Toshiba Visconti5 GPIO driver: > v2 -> v3: Fix compatible string. > v1 -> v2: No update > > Best regards, > Nobuhiro > Nobuhiro, In the future please use the get_maintainers.pl script - I have never been Cc'ed on this series and I would have ignored it if Linus W hadn't brought it to my attention. Bartosz
Hi, This series is the GPIO driver for Toshiba's ARM SoC, Visconti[0]. This provides DT binding documentation, device driver, MAINTAINER files, and updates to DT files. Update: dt-bindings: gpio: Add bindings for Toshiba Visconti GPIO Controller: v2 -> v3: Fix dtschema/dtc warnings. dtschema/dtc warnings/errors: Documentation/devicetree/bindings/gpio/toshiba,gpio-visconti.example.dt.yaml: gpio@28020000: interrupts: [[0, 24, 4], [0, 25, 4], [0, 26, 4], [0, 27, 4], [0, 28, 4], [0, 29, 4], [0, 30, 4], [0, 31, 4], [0, 32, 4], [0, 33, 4], [0, 34, 4], [0, 35, 4], [0, 36, 4], [0, 37, 4], [0, 38, 4], [0, 39, 4]] is too short From schema: Documentation/devicetree/bindings/gpio/toshiba,gpio-visconti.yaml v1 -> v2: Fix typo. gpio: visoconti: Add Toshiba Visconti GPIO support: v2 -> v3: Add select GPIO_GENERIC Use genric MMIO GPIO library Use bgpio_init() as initialized the generic helpers. Use irqchip template instead of gpiochip_irqchip_add(). v1 -> v2: No update MAINTAINERS: Add entries for Toshiba Visconti GPIO controller: v2 -> v3: No update v1 -> v2: No update arm: dts: visconti: Add DT support for Toshiba Visconti5 GPIO driver: v2 -> v3: Fix compatible string. v1 -> v2: No update Best regards, Nobuhiro [0]: https://toshiba.semicon-storage.com/ap-en/semiconductor/product/image-recognition-processors-visconti.html Nobuhiro Iwamatsu (4): dt-bindings: gpio: Add bindings for Toshiba Visconti GPIO Controller gpio: visconti: Add Toshiba Visconti GPIO support MAINTAINERS: Add entries for Toshiba Visconti GPIO controller arm: dts: visconti: Add DT support for Toshiba Visconti5 GPIO driver .../bindings/gpio/toshiba,gpio-visconti.yaml | 85 +++++++ MAINTAINERS | 2 + .../boot/dts/toshiba/tmpv7708-rm-mbrc.dts | 4 + arch/arm64/boot/dts/toshiba/tmpv7708.dtsi | 27 ++ drivers/gpio/Kconfig | 9 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-visconti.c | 232 ++++++++++++++++++ drivers/pinctrl/visconti/pinctrl-common.c | 23 ++ 8 files changed, 383 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpio/toshiba,gpio-visconti.yaml create mode 100644 drivers/gpio/gpio-visconti.c