* [PATCH v3 0/2] pinctrl: bcm: nsp: gpio improvements
@ 2019-11-04 0:18 Chris Packham
2019-11-04 0:18 ` [PATCH v3 1/2] pinctrl: bcm: nsp: use gpiolib infrastructure for interrupts Chris Packham
2019-11-04 0:18 ` [PATCH v3 2/2] pinctrl: bcm: nsp: implement get_direction Chris Packham
0 siblings, 2 replies; 7+ messages in thread
From: Chris Packham @ 2019-11-04 0:18 UTC (permalink / raw)
To: linus.walleij, rjui, sbranden, bcm-kernel-feedback-list
Cc: linux-arm-kernel, linux-gpio, linux-kernel, Chris Packham
I'm working on a platform using the BCM 58525 SoC. I noticed that some of
the peripherals were being deferred (not that that's a problem) and debugfs
was complaining "File ':axi@18000000:gpio@20' in directory 'domains' already
present!" which is more of a sign that things were not right.
The debugfs error was because the manually created irq domain was not
cleaned up on failure (or deferral).
I've dropped the patch from this series that changes the order in the
device tree. I can probably live with the deferrals.
While I was debugging another issue I noticed my gpio-hogs weren't
showing up correctly in /sys/kernel/debug/gpio. At first I thought I was
missing commit d95da993383c ("gpiolib: Preserve desc->flags when setting
state") but as it turns out pinctrl-nsp-gpio.c didn't provide a
get_direction function so the generic code assumed they all started as
inputs. I've added a new patch to address that.
Chris Packham (2):
pinctrl: bcm: nsp: use gpiolib infrastructure for interrupts
pinctrl: bcm: nsp: implement get_direction
drivers/pinctrl/bcm/pinctrl-nsp-gpio.c | 119 ++++++++++++-------------
1 file changed, 56 insertions(+), 63 deletions(-)
--
2.23.0
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH v3 1/2] pinctrl: bcm: nsp: use gpiolib infrastructure for interrupts 2019-11-04 0:18 [PATCH v3 0/2] pinctrl: bcm: nsp: gpio improvements Chris Packham @ 2019-11-04 0:18 ` Chris Packham 2019-11-04 15:20 ` Linus Walleij 2019-11-04 0:18 ` [PATCH v3 2/2] pinctrl: bcm: nsp: implement get_direction Chris Packham 1 sibling, 1 reply; 7+ messages in thread From: Chris Packham @ 2019-11-04 0:18 UTC (permalink / raw) To: linus.walleij, rjui, sbranden, bcm-kernel-feedback-list Cc: linux-arm-kernel, linux-gpio, linux-kernel, Chris Packham Use more of the gpiolib infrastructure for handling interrupts. The root interrupt still needs to be handled manually as it is shared with other peripherals on the SoC. This will allow multiple instances of this driver to be supported and will clean up gracefully on failure thanks to the device managed APIs. Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> --- Notes: Changes in v3: - retain old irqchip name for ABI compatilbilty - add revew from Florian Changes in v2: - none drivers/pinctrl/bcm/pinctrl-nsp-gpio.c | 105 ++++++++++--------------- 1 file changed, 42 insertions(+), 63 deletions(-) diff --git a/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c b/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c index e67ae52023ad..45ae29b22548 100644 --- a/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c +++ b/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c @@ -64,17 +64,16 @@ * @gc: GPIO chip * @pctl: pointer to pinctrl_dev * @pctldesc: pinctrl descriptor - * @irq_domain: pointer to irq domain * @lock: lock to protect access to I/O registers */ struct nsp_gpio { struct device *dev; void __iomem *base; void __iomem *io_ctrl; + struct irq_chip irqchip; struct gpio_chip gc; struct pinctrl_dev *pctl; struct pinctrl_desc pctldesc; - struct irq_domain *irq_domain; raw_spinlock_t lock; }; @@ -136,8 +135,8 @@ static inline bool nsp_get_bit(struct nsp_gpio *chip, enum base_type address, static irqreturn_t nsp_gpio_irq_handler(int irq, void *data) { - struct nsp_gpio *chip = (struct nsp_gpio *)data; - struct gpio_chip gc = chip->gc; + struct gpio_chip *gc = (struct gpio_chip *)data; + struct nsp_gpio *chip = gpiochip_get_data(gc); int bit; unsigned long int_bits = 0; u32 int_status; @@ -155,14 +154,14 @@ static irqreturn_t nsp_gpio_irq_handler(int irq, void *data) level &= readl(chip->base + NSP_GPIO_INT_MASK); int_bits = level | event; - for_each_set_bit(bit, &int_bits, gc.ngpio) { + for_each_set_bit(bit, &int_bits, gc->ngpio) { /* * Clear the interrupt before invoking the * handler, so we do not leave any window */ writel(BIT(bit), chip->base + NSP_GPIO_EVENT); generic_handle_irq( - irq_linear_revmap(chip->irq_domain, bit)); + irq_linear_revmap(gc->irq.domain, bit)); } } @@ -171,7 +170,8 @@ static irqreturn_t nsp_gpio_irq_handler(int irq, void *data) static void nsp_gpio_irq_ack(struct irq_data *d) { - struct nsp_gpio *chip = irq_data_get_irq_chip_data(d); + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct nsp_gpio *chip = gpiochip_get_data(gc); unsigned gpio = d->hwirq; u32 val = BIT(gpio); u32 trigger_type; @@ -189,7 +189,8 @@ static void nsp_gpio_irq_ack(struct irq_data *d) */ static void nsp_gpio_irq_set_mask(struct irq_data *d, bool unmask) { - struct nsp_gpio *chip = irq_data_get_irq_chip_data(d); + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct nsp_gpio *chip = gpiochip_get_data(gc); unsigned gpio = d->hwirq; u32 trigger_type; @@ -202,7 +203,8 @@ static void nsp_gpio_irq_set_mask(struct irq_data *d, bool unmask) static void nsp_gpio_irq_mask(struct irq_data *d) { - struct nsp_gpio *chip = irq_data_get_irq_chip_data(d); + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct nsp_gpio *chip = gpiochip_get_data(gc); unsigned long flags; raw_spin_lock_irqsave(&chip->lock, flags); @@ -212,7 +214,8 @@ static void nsp_gpio_irq_mask(struct irq_data *d) static void nsp_gpio_irq_unmask(struct irq_data *d) { - struct nsp_gpio *chip = irq_data_get_irq_chip_data(d); + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct nsp_gpio *chip = gpiochip_get_data(gc); unsigned long flags; raw_spin_lock_irqsave(&chip->lock, flags); @@ -222,7 +225,8 @@ static void nsp_gpio_irq_unmask(struct irq_data *d) static int nsp_gpio_irq_set_type(struct irq_data *d, unsigned int type) { - struct nsp_gpio *chip = irq_data_get_irq_chip_data(d); + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct nsp_gpio *chip = gpiochip_get_data(gc); unsigned gpio = d->hwirq; bool level_low; bool falling; @@ -265,16 +269,6 @@ static int nsp_gpio_irq_set_type(struct irq_data *d, unsigned int type) return 0; } -static struct irq_chip nsp_gpio_irq_chip = { - .name = "gpio-a", - .irq_enable = nsp_gpio_irq_unmask, - .irq_disable = nsp_gpio_irq_mask, - .irq_ack = nsp_gpio_irq_ack, - .irq_mask = nsp_gpio_irq_mask, - .irq_unmask = nsp_gpio_irq_unmask, - .irq_set_type = nsp_gpio_irq_set_type, -}; - static int nsp_gpio_direction_input(struct gpio_chip *gc, unsigned gpio) { struct nsp_gpio *chip = gpiochip_get_data(gc); @@ -322,13 +316,6 @@ static int nsp_gpio_get(struct gpio_chip *gc, unsigned gpio) return !!(readl(chip->base + NSP_GPIO_DATA_IN) & BIT(gpio)); } -static int nsp_gpio_to_irq(struct gpio_chip *gc, unsigned offset) -{ - struct nsp_gpio *chip = gpiochip_get_data(gc); - - return irq_linear_revmap(chip->irq_domain, offset); -} - static int nsp_get_groups_count(struct pinctrl_dev *pctldev) { return 1; @@ -613,10 +600,9 @@ static const struct of_device_id nsp_gpio_of_match[] = { static int nsp_gpio_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; - struct resource *res; struct nsp_gpio *chip; struct gpio_chip *gc; - u32 val, count; + u32 val; int irq, ret; if (of_property_read_u32(pdev->dev.of_node, "ngpios", &val)) { @@ -631,15 +617,13 @@ static int nsp_gpio_probe(struct platform_device *pdev) chip->dev = dev; platform_set_drvdata(pdev, chip); - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - chip->base = devm_ioremap_resource(dev, res); + chip->base = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(chip->base)) { dev_err(dev, "unable to map I/O memory\n"); return PTR_ERR(chip->base); } - res = platform_get_resource(pdev, IORESOURCE_MEM, 1); - chip->io_ctrl = devm_ioremap_resource(dev, res); + chip->io_ctrl = devm_platform_ioremap_resource(pdev, 1); if (IS_ERR(chip->io_ctrl)) { dev_err(dev, "unable to map I/O memory\n"); return PTR_ERR(chip->io_ctrl); @@ -659,44 +643,44 @@ static int nsp_gpio_probe(struct platform_device *pdev) gc->direction_output = nsp_gpio_direction_output; gc->set = nsp_gpio_set; gc->get = nsp_gpio_get; - gc->to_irq = nsp_gpio_to_irq; /* optional GPIO interrupt support */ irq = platform_get_irq(pdev, 0); if (irq > 0) { - /* Create irq domain so that each pin can be assigned an IRQ.*/ - chip->irq_domain = irq_domain_add_linear(gc->of_node, gc->ngpio, - &irq_domain_simple_ops, - chip); - if (!chip->irq_domain) { - dev_err(&pdev->dev, "Couldn't allocate IRQ domain\n"); - return -ENXIO; - } + struct gpio_irq_chip *girq; + struct irq_chip *irqc; - /* Map each gpio to an IRQ and set the handler for gpiolib. */ - for (count = 0; count < gc->ngpio; count++) { - int irq = irq_create_mapping(chip->irq_domain, count); + irqc = &chip->irqchip; + irqc->name = "gpio-a"; + irqc->irq_ack = nsp_gpio_irq_ack; + irqc->irq_mask = nsp_gpio_irq_mask; + irqc->irq_unmask = nsp_gpio_irq_unmask; + irqc->irq_set_type = nsp_gpio_irq_set_type; - irq_set_chip_and_handler(irq, &nsp_gpio_irq_chip, - handle_simple_irq); - irq_set_chip_data(irq, chip); - } + val = readl(chip->base + NSP_CHIP_A_INT_MASK); + val = val | NSP_CHIP_A_GPIO_INT_BIT; + writel(val, (chip->base + NSP_CHIP_A_INT_MASK)); /* Install ISR for this GPIO controller. */ - ret = devm_request_irq(&pdev->dev, irq, nsp_gpio_irq_handler, - IRQF_SHARED, "gpio-a", chip); + ret = devm_request_irq(dev, irq, nsp_gpio_irq_handler, + IRQF_SHARED, "gpio-a", &chip->gc); if (ret) { dev_err(&pdev->dev, "Unable to request IRQ%d: %d\n", irq, ret); - goto err_rm_gpiochip; + return ret; } - val = readl(chip->base + NSP_CHIP_A_INT_MASK); - val = val | NSP_CHIP_A_GPIO_INT_BIT; - writel(val, (chip->base + NSP_CHIP_A_INT_MASK)); + 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 = gpiochip_add_data(gc, chip); + ret = devm_gpiochip_add_data(dev, gc, chip); if (ret < 0) { dev_err(dev, "unable to add GPIO chip\n"); return ret; @@ -705,15 +689,10 @@ static int nsp_gpio_probe(struct platform_device *pdev) ret = nsp_gpio_register_pinconf(chip); if (ret) { dev_err(dev, "unable to register pinconf\n"); - goto err_rm_gpiochip; + return ret; } return 0; - -err_rm_gpiochip: - gpiochip_remove(gc); - - return ret; } static struct platform_driver nsp_gpio_driver = { -- 2.23.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] pinctrl: bcm: nsp: use gpiolib infrastructure for interrupts 2019-11-04 0:18 ` [PATCH v3 1/2] pinctrl: bcm: nsp: use gpiolib infrastructure for interrupts Chris Packham @ 2019-11-04 15:20 ` Linus Walleij 0 siblings, 0 replies; 7+ messages in thread From: Linus Walleij @ 2019-11-04 15:20 UTC (permalink / raw) To: Chris Packham Cc: Ray Jui, Scott Branden, bcm-kernel-feedback-list, Linux ARM, open list:GPIO SUBSYSTEM, linux-kernel On Mon, Nov 4, 2019 at 1:18 AM Chris Packham <chris.packham@alliedtelesis.co.nz> wrote: > Use more of the gpiolib infrastructure for handling interrupts. The > root interrupt still needs to be handled manually as it is shared with > other peripherals on the SoC. > > This will allow multiple instances of this driver to be supported and > will clean up gracefully on failure thanks to the device managed APIs. > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > --- > > Notes: > Changes in v3: > - retain old irqchip name for ABI compatilbilty > - add revew from Florian > Changes in v2: > - none Patch applied. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3 2/2] pinctrl: bcm: nsp: implement get_direction 2019-11-04 0:18 [PATCH v3 0/2] pinctrl: bcm: nsp: gpio improvements Chris Packham 2019-11-04 0:18 ` [PATCH v3 1/2] pinctrl: bcm: nsp: use gpiolib infrastructure for interrupts Chris Packham @ 2019-11-04 0:18 ` Chris Packham 2019-11-04 15:20 ` Linus Walleij 2019-11-04 15:24 ` Linus Walleij 1 sibling, 2 replies; 7+ messages in thread From: Chris Packham @ 2019-11-04 0:18 UTC (permalink / raw) To: linus.walleij, rjui, sbranden, bcm-kernel-feedback-list Cc: linux-arm-kernel, linux-gpio, linux-kernel, Chris Packham The get_direction api is strongly recommended to be implemented. In fact if it is not implemented gpio-hogs will not get the correct direction. Add an implementation of get_direction for the nsp-gpio driver. Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> --- Notes: Changes in v3: - add revew from Florian Changes in v2: - New drivers/pinctrl/bcm/pinctrl-nsp-gpio.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c b/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c index 45ae29b22548..bed0124388c0 100644 --- a/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c +++ b/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c @@ -297,6 +297,19 @@ static int nsp_gpio_direction_output(struct gpio_chip *gc, unsigned gpio, return 0; } +static int nsp_gpio_get_direction(struct gpio_chip *gc, unsigned gpio) +{ + struct nsp_gpio *chip = gpiochip_get_data(gc); + unsigned long flags; + int val; + + raw_spin_lock_irqsave(&chip->lock, flags); + val = nsp_get_bit(chip, REG, NSP_GPIO_OUT_EN, gpio); + raw_spin_unlock_irqrestore(&chip->lock, flags); + + return !val; +} + static void nsp_gpio_set(struct gpio_chip *gc, unsigned gpio, int val) { struct nsp_gpio *chip = gpiochip_get_data(gc); @@ -641,6 +654,7 @@ static int nsp_gpio_probe(struct platform_device *pdev) gc->free = gpiochip_generic_free; gc->direction_input = nsp_gpio_direction_input; gc->direction_output = nsp_gpio_direction_output; + gc->get_direction = nsp_gpio_get_direction; gc->set = nsp_gpio_set; gc->get = nsp_gpio_get; -- 2.23.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3 2/2] pinctrl: bcm: nsp: implement get_direction 2019-11-04 0:18 ` [PATCH v3 2/2] pinctrl: bcm: nsp: implement get_direction Chris Packham @ 2019-11-04 15:20 ` Linus Walleij 2019-11-04 15:24 ` Linus Walleij 1 sibling, 0 replies; 7+ messages in thread From: Linus Walleij @ 2019-11-04 15:20 UTC (permalink / raw) To: Chris Packham Cc: Ray Jui, Scott Branden, bcm-kernel-feedback-list, Linux ARM, open list:GPIO SUBSYSTEM, linux-kernel On Mon, Nov 4, 2019 at 1:18 AM Chris Packham <chris.packham@alliedtelesis.co.nz> wrote: > The get_direction api is strongly recommended to be implemented. In fact > if it is not implemented gpio-hogs will not get the correct direction. > Add an implementation of get_direction for the nsp-gpio driver. > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > --- > > Notes: > Changes in v3: > - add revew from Florian > Changes in v2: > - New Patch applied. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 2/2] pinctrl: bcm: nsp: implement get_direction 2019-11-04 0:18 ` [PATCH v3 2/2] pinctrl: bcm: nsp: implement get_direction Chris Packham 2019-11-04 15:20 ` Linus Walleij @ 2019-11-04 15:24 ` Linus Walleij 2019-11-04 19:58 ` Chris Packham 1 sibling, 1 reply; 7+ messages in thread From: Linus Walleij @ 2019-11-04 15:24 UTC (permalink / raw) To: Chris Packham Cc: Ray Jui, Scott Branden, bcm-kernel-feedback-list, Linux ARM, open list:GPIO SUBSYSTEM, linux-kernel On Mon, Nov 4, 2019 at 1:18 AM Chris Packham <chris.packham@alliedtelesis.co.nz> wrote: > The get_direction api is strongly recommended to be implemented. In fact > if it is not implemented gpio-hogs will not get the correct direction. > Add an implementation of get_direction for the nsp-gpio driver. > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> BTW I think it looks like this the GPIO part of this driver can be converted to use GPIO_GENERIC. Compare to other drivers such as drivers/gpio/gpio-ftgpio010.c. It's a fun way to cut down lines if you have time to check and test! Yours, Linus Walleij ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 2/2] pinctrl: bcm: nsp: implement get_direction 2019-11-04 15:24 ` Linus Walleij @ 2019-11-04 19:58 ` Chris Packham 0 siblings, 0 replies; 7+ messages in thread From: Chris Packham @ 2019-11-04 19:58 UTC (permalink / raw) To: linus.walleij Cc: linux-arm-kernel, linux-kernel, bcm-kernel-feedback-list, linux-gpio, sbranden, rjui On Mon, 2019-11-04 at 16:24 +0100, Linus Walleij wrote: > On Mon, Nov 4, 2019 at 1:18 AM Chris Packham > <chris.packham@alliedtelesis.co.nz> wrote: > > > The get_direction api is strongly recommended to be implemented. In fact > > if it is not implemented gpio-hogs will not get the correct direction. > > Add an implementation of get_direction for the nsp-gpio driver. > > > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > > BTW I think it looks like this the GPIO part of this driver can be > converted to use GPIO_GENERIC. Compare to other > drivers such as drivers/gpio/gpio-ftgpio010.c. > > It's a fun way to cut down lines if you have time to check > and test! > I'll see if I can fit it in. Got another problem I'm chasing on the same platform. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-11-04 19:58 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-11-04 0:18 [PATCH v3 0/2] pinctrl: bcm: nsp: gpio improvements Chris Packham 2019-11-04 0:18 ` [PATCH v3 1/2] pinctrl: bcm: nsp: use gpiolib infrastructure for interrupts Chris Packham 2019-11-04 15:20 ` Linus Walleij 2019-11-04 0:18 ` [PATCH v3 2/2] pinctrl: bcm: nsp: implement get_direction Chris Packham 2019-11-04 15:20 ` Linus Walleij 2019-11-04 15:24 ` Linus Walleij 2019-11-04 19:58 ` Chris Packham
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).