[PATCH v3 0/2] gpio: sch: Interrupt support

[PATCH v3 0/2] gpio: sch: Interrupt support

[Jan Kiszka]

Changes in v3:
 - split-up of the irq enabling patch as requested by Andy

Jan

Jan Kiszka (2):
  gpio: sch: Add edge event support
  gpio: sch: Hook into ACPI SCI handler to catch GPIO edge events

 drivers/gpio/gpio-sch.c | 144 +++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 137 insertions(+), 7 deletions(-)

-- 
2.16.4

[PATCH v3 1/2] gpio: sch: Add edge event support

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Add the required infrastructure consisting of an irq_chip_generic with
its irq_chip_type callbacks to enable and report edge events of the pins
to the gpio core. The actual hook-up of the event interrupt will happen
separately.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/gpio/gpio-sch.c | 114 +++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 107 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
index fb143f28c386..6a9c5500800c 100644
--- a/drivers/gpio/gpio-sch.c
+++ b/drivers/gpio/gpio-sch.c
@@ -18,12 +18,17 @@
 #define GEN	0x00
 #define GIO	0x04
 #define GLV	0x08
+#define GTPE	0x0c
+#define GTNE	0x10
+#define GGPE	0x14
+#define GTS	0x1c
 
 struct sch_gpio {
 	struct gpio_chip chip;
 	spinlock_t lock;
 	unsigned short iobase;
 	unsigned short resume_base;
+	int irq_base;
 };
 
 static unsigned sch_gpio_offset(struct sch_gpio *sch, unsigned gpio,
@@ -79,10 +84,11 @@ static void sch_gpio_reg_set(struct sch_gpio *sch, unsigned gpio, unsigned reg,
 static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned gpio_num)
 {
 	struct sch_gpio *sch = gpiochip_get_data(gc);
+	unsigned long flags;
 
-	spin_lock(&sch->lock);
+	spin_lock_irqsave(&sch->lock, flags);
 	sch_gpio_reg_set(sch, gpio_num, GIO, 1);
-	spin_unlock(&sch->lock);
+	spin_unlock_irqrestore(&sch->lock, flags);
 	return 0;
 }
 
@@ -95,20 +101,22 @@ static int sch_gpio_get(struct gpio_chip *gc, unsigned gpio_num)
 static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val)
 {
 	struct sch_gpio *sch = gpiochip_get_data(gc);
+	unsigned long flags;
 
-	spin_lock(&sch->lock);
+	spin_lock_irqsave(&sch->lock, flags);
 	sch_gpio_reg_set(sch, gpio_num, GLV, val);
-	spin_unlock(&sch->lock);
+	spin_unlock_irqrestore(&sch->lock, flags);
 }
 
 static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num,
 				  int val)
 {
 	struct sch_gpio *sch = gpiochip_get_data(gc);
+	unsigned long flags;
 
-	spin_lock(&sch->lock);
+	spin_lock_irqsave(&sch->lock, flags);
 	sch_gpio_reg_set(sch, gpio_num, GIO, 0);
-	spin_unlock(&sch->lock);
+	spin_unlock_irqrestore(&sch->lock, flags);
 
 	/*
 	 * according to the datasheet, writing to the level register has no
@@ -130,6 +138,12 @@ static int sch_gpio_get_direction(struct gpio_chip *gc, unsigned gpio_num)
 	return sch_gpio_reg_get(sch, gpio_num, GIO);
 }
 
+static int sch_gpio_to_irq(struct gpio_chip *gpio, unsigned int offset)
+{
+	struct sch_gpio *sch = gpiochip_get_data(gpio);
+	return sch->irq_base + offset;
+}
+
 static const struct gpio_chip sch_gpio_chip = {
 	.label			= "sch_gpio",
 	.owner			= THIS_MODULE,
@@ -138,12 +152,70 @@ static const struct gpio_chip sch_gpio_chip = {
 	.direction_output	= sch_gpio_direction_out,
 	.set			= sch_gpio_set,
 	.get_direction		= sch_gpio_get_direction,
+	.to_irq			= sch_gpio_to_irq,
 };
 
+static int sch_irq_type(struct irq_data *d, unsigned int type)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct sch_gpio *sch = gc->private;
+	unsigned int gpio_num = d->irq - sch->irq_base;
+	unsigned long flags;
+	int rising = 0;
+	int falling = 0;
+
+	switch (type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_EDGE_RISING:
+		rising = 1;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		falling = 1;
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		rising = 1;
+		falling = 1;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	spin_lock_irqsave(&sch->lock, flags);
+	sch_gpio_reg_set(sch, gpio_num, GTPE, rising);
+	sch_gpio_reg_set(sch, gpio_num, GTNE, falling);
+	spin_unlock_irqrestore(&sch->lock, flags);
+
+	return 0;
+}
+
+static void sch_irq_set_enable(struct irq_data *d, int val)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct sch_gpio *sch = gc->private;
+	unsigned int gpio_num = d->irq - sch->irq_base;
+	unsigned long flags;
+
+	spin_lock_irqsave(&sch->lock, flags);
+	sch_gpio_reg_set(sch, gpio_num, GGPE, val);
+	spin_unlock_irqrestore(&sch->lock, flags);
+}
+
+static void sch_irq_mask(struct irq_data *d)
+{
+	sch_irq_set_enable(d, 0);
+}
+
+static void sch_irq_unmask(struct irq_data *d)
+{
+	sch_irq_set_enable(d, 1);
+}
+
 static int sch_gpio_probe(struct platform_device *pdev)
 {
+	struct irq_chip_generic *gc;
+	struct irq_chip_type *ct;
 	struct sch_gpio *sch;
 	struct resource *res;
+	int irq_base, ret;
 
 	sch = devm_kzalloc(&pdev->dev, sizeof(*sch), GFP_KERNEL);
 	if (!sch)
@@ -203,7 +275,35 @@ static int sch_gpio_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, sch);
 
-	return devm_gpiochip_add_data(&pdev->dev, &sch->chip, sch);
+	ret = devm_gpiochip_add_data(&pdev->dev, &sch->chip, sch);
+	if (ret)
+		return ret;
+
+	irq_base = devm_irq_alloc_descs(&pdev->dev, -1, 0, sch->chip.ngpio,
+					NUMA_NO_NODE);
+	if (irq_base < 0)
+		return irq_base;
+	sch->irq_base = irq_base;
+
+	gc = devm_irq_alloc_generic_chip(&pdev->dev, "sch_gpio", 1, irq_base,
+					 NULL, handle_simple_irq);
+	if (!gc)
+		return -ENOMEM;
+
+	gc->private = sch;
+	ct = gc->chip_types;
+
+	ct->chip.irq_mask = sch_irq_mask;
+	ct->chip.irq_unmask = sch_irq_unmask;
+	ct->chip.irq_set_type = sch_irq_type;
+
+	ret = devm_irq_setup_generic_chip(&pdev->dev, gc,
+					  IRQ_MSK(sch->chip.ngpio),
+					  0, IRQ_NOREQUEST | IRQ_NOPROBE, 0);
+	if (ret)
+		return ret;
+
+	return 0;
 }
 
 static struct platform_driver sch_gpio_driver = {
-- 
2.16.4

[PATCH v3 2/2] gpio: sch: Hook into ACPI SCI handler to catch GPIO edge events

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

The ACPI description on the Quark platform does not provide the required
information to do establish generic handling. Therefore, we need to hook
from the driver directly into SCI handler of the ACPI subsystem in order
to catch and report GPIO-related events.

Validated on the Quark-based IOT2000 platform.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/gpio/gpio-sch.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
index 6a9c5500800c..75c95da145d8 100644
--- a/drivers/gpio/gpio-sch.c
+++ b/drivers/gpio/gpio-sch.c
@@ -155,6 +155,31 @@ static const struct gpio_chip sch_gpio_chip = {
 	.to_irq			= sch_gpio_to_irq,
 };
 
+static u32 sch_sci_handler(void *context)
+{
+	struct sch_gpio *sch = context;
+	unsigned long core_status, resume_status;
+	unsigned int resume_gpios, offset;
+
+	core_status = inl(sch->iobase + GTS);
+	resume_status = inl(sch->iobase + GTS + 0x20);
+
+	if (core_status == 0 && resume_status == 0)
+		return ACPI_INTERRUPT_NOT_HANDLED;
+
+	for_each_set_bit(offset, &core_status, sch->resume_base)
+		generic_handle_irq(sch->irq_base + offset);
+
+	resume_gpios = sch->chip.ngpio - sch->resume_base;
+	for_each_set_bit(offset, &resume_status, resume_gpios)
+		generic_handle_irq(sch->irq_base + sch->resume_base + offset);
+
+	outl(core_status, sch->iobase + GTS);
+	outl(resume_status, sch->iobase + GTS + 0x20);
+
+	return ACPI_INTERRUPT_HANDLED;
+}
+
 static int sch_irq_type(struct irq_data *d, unsigned int type)
 {
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
@@ -215,6 +240,7 @@ static int sch_gpio_probe(struct platform_device *pdev)
 	struct irq_chip_type *ct;
 	struct sch_gpio *sch;
 	struct resource *res;
+	acpi_status status;
 	int irq_base, ret;
 
 	sch = devm_kzalloc(&pdev->dev, sizeof(*sch), GFP_KERNEL);
@@ -303,6 +329,10 @@ static int sch_gpio_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	status = acpi_install_sci_handler(sch_sci_handler, sch);
+	if (ACPI_FAILURE(status))
+		return -EINVAL;
+
 	return 0;
 }
 
-- 
2.16.4

Re: [PATCH v3 1/2] gpio: sch: Add edge event support

[Mika Westerberg]

On Wed, Nov 20, 2019 at 08:20:13PM +0100, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Add the required infrastructure consisting of an irq_chip_generic with
> its irq_chip_type callbacks to enable and report edge events of the pins
> to the gpio core. The actual hook-up of the event interrupt will happen
> separately.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

One nit below. Regardless of that

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

> ---
>  drivers/gpio/gpio-sch.c | 114 +++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 107 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
> index fb143f28c386..6a9c5500800c 100644
> --- a/drivers/gpio/gpio-sch.c
> +++ b/drivers/gpio/gpio-sch.c
> @@ -18,12 +18,17 @@
>  #define GEN	0x00
>  #define GIO	0x04
>  #define GLV	0x08
> +#define GTPE	0x0c
> +#define GTNE	0x10
> +#define GGPE	0x14
> +#define GTS	0x1c
>  
>  struct sch_gpio {
>  	struct gpio_chip chip;
>  	spinlock_t lock;
>  	unsigned short iobase;
>  	unsigned short resume_base;
> +	int irq_base;
>  };
>  
>  static unsigned sch_gpio_offset(struct sch_gpio *sch, unsigned gpio,
> @@ -79,10 +84,11 @@ static void sch_gpio_reg_set(struct sch_gpio *sch, unsigned gpio, unsigned reg,
>  static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned gpio_num)
>  {
>  	struct sch_gpio *sch = gpiochip_get_data(gc);
> +	unsigned long flags;
>  
> -	spin_lock(&sch->lock);
> +	spin_lock_irqsave(&sch->lock, flags);
>  	sch_gpio_reg_set(sch, gpio_num, GIO, 1);
> -	spin_unlock(&sch->lock);
> +	spin_unlock_irqrestore(&sch->lock, flags);
>  	return 0;
>  }
>  
> @@ -95,20 +101,22 @@ static int sch_gpio_get(struct gpio_chip *gc, unsigned gpio_num)
>  static void sch_gpio_set(struct gpio_chip *gc, unsigned gpio_num, int val)
>  {
>  	struct sch_gpio *sch = gpiochip_get_data(gc);
> +	unsigned long flags;
>  
> -	spin_lock(&sch->lock);
> +	spin_lock_irqsave(&sch->lock, flags);
>  	sch_gpio_reg_set(sch, gpio_num, GLV, val);
> -	spin_unlock(&sch->lock);
> +	spin_unlock_irqrestore(&sch->lock, flags);
>  }
>  
>  static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned gpio_num,
>  				  int val)
>  {
>  	struct sch_gpio *sch = gpiochip_get_data(gc);
> +	unsigned long flags;
>  
> -	spin_lock(&sch->lock);
> +	spin_lock_irqsave(&sch->lock, flags);
>  	sch_gpio_reg_set(sch, gpio_num, GIO, 0);
> -	spin_unlock(&sch->lock);
> +	spin_unlock_irqrestore(&sch->lock, flags);
>  
>  	/*
>  	 * according to the datasheet, writing to the level register has no
> @@ -130,6 +138,12 @@ static int sch_gpio_get_direction(struct gpio_chip *gc, unsigned gpio_num)
>  	return sch_gpio_reg_get(sch, gpio_num, GIO);
>  }
>  
> +static int sch_gpio_to_irq(struct gpio_chip *gpio, unsigned int offset)
> +{
> +	struct sch_gpio *sch = gpiochip_get_data(gpio);
> +	return sch->irq_base + offset;
> +}
> +
>  static const struct gpio_chip sch_gpio_chip = {
>  	.label			= "sch_gpio",
>  	.owner			= THIS_MODULE,
> @@ -138,12 +152,70 @@ static const struct gpio_chip sch_gpio_chip = {
>  	.direction_output	= sch_gpio_direction_out,
>  	.set			= sch_gpio_set,
>  	.get_direction		= sch_gpio_get_direction,
> +	.to_irq			= sch_gpio_to_irq,
>  };
>  
> +static int sch_irq_type(struct irq_data *d, unsigned int type)
> +{
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct sch_gpio *sch = gc->private;
> +	unsigned int gpio_num = d->irq - sch->irq_base;
> +	unsigned long flags;
> +	int rising = 0;
> +	int falling = 0;
> +
> +	switch (type & IRQ_TYPE_SENSE_MASK) {
> +	case IRQ_TYPE_EDGE_RISING:
> +		rising = 1;
> +		break;
> +	case IRQ_TYPE_EDGE_FALLING:
> +		falling = 1;
> +		break;
> +	case IRQ_TYPE_EDGE_BOTH:
> +		rising = 1;
> +		falling = 1;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	spin_lock_irqsave(&sch->lock, flags);
> +	sch_gpio_reg_set(sch, gpio_num, GTPE, rising);
> +	sch_gpio_reg_set(sch, gpio_num, GTNE, falling);
> +	spin_unlock_irqrestore(&sch->lock, flags);
> +
> +	return 0;
> +}
> +
> +static void sch_irq_set_enable(struct irq_data *d, int val)
> +{
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct sch_gpio *sch = gc->private;
> +	unsigned int gpio_num = d->irq - sch->irq_base;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&sch->lock, flags);
> +	sch_gpio_reg_set(sch, gpio_num, GGPE, val);
> +	spin_unlock_irqrestore(&sch->lock, flags);
> +}
> +
> +static void sch_irq_mask(struct irq_data *d)
> +{
> +	sch_irq_set_enable(d, 0);
> +}
> +
> +static void sch_irq_unmask(struct irq_data *d)
> +{
> +	sch_irq_set_enable(d, 1);
> +}
> +
>  static int sch_gpio_probe(struct platform_device *pdev)
>  {
> +	struct irq_chip_generic *gc;
> +	struct irq_chip_type *ct;
>  	struct sch_gpio *sch;
>  	struct resource *res;
> +	int irq_base, ret;
>  
>  	sch = devm_kzalloc(&pdev->dev, sizeof(*sch), GFP_KERNEL);
>  	if (!sch)
> @@ -203,7 +275,35 @@ static int sch_gpio_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, sch);
>  
> -	return devm_gpiochip_add_data(&pdev->dev, &sch->chip, sch);
> +	ret = devm_gpiochip_add_data(&pdev->dev, &sch->chip, sch);
> +	if (ret)
> +		return ret;
> +
> +	irq_base = devm_irq_alloc_descs(&pdev->dev, -1, 0, sch->chip.ngpio,
> +					NUMA_NO_NODE);
> +	if (irq_base < 0)
> +		return irq_base;
> +	sch->irq_base = irq_base;
> +
> +	gc = devm_irq_alloc_generic_chip(&pdev->dev, "sch_gpio", 1, irq_base,
> +					 NULL, handle_simple_irq);
> +	if (!gc)
> +		return -ENOMEM;
> +
> +	gc->private = sch;
> +	ct = gc->chip_types;
> +
> +	ct->chip.irq_mask = sch_irq_mask;
> +	ct->chip.irq_unmask = sch_irq_unmask;
> +	ct->chip.irq_set_type = sch_irq_type;
> +
> +	ret = devm_irq_setup_generic_chip(&pdev->dev, gc,
> +					  IRQ_MSK(sch->chip.ngpio),
> +					  0, IRQ_NOREQUEST | IRQ_NOPROBE, 0);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

Here you can simply do 

	return devm_irq_setup_generic_chip(...);
>  }
>  
>  static struct platform_driver sch_gpio_driver = {
> -- 
> 2.16.4

Re: [PATCH v3 2/2] gpio: sch: Hook into ACPI SCI handler to catch GPIO edge events

[Mika Westerberg]

On Wed, Nov 20, 2019 at 08:20:14PM +0100, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> The ACPI description on the Quark platform does not provide the required
> information to do establish generic handling. Therefore, we need to hook
> from the driver directly into SCI handler of the ACPI subsystem in order
> to catch and report GPIO-related events.
> 
> Validated on the Quark-based IOT2000 platform.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Below one minor stylistic comment.

> ---
>  drivers/gpio/gpio-sch.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
> index 6a9c5500800c..75c95da145d8 100644
> --- a/drivers/gpio/gpio-sch.c
> +++ b/drivers/gpio/gpio-sch.c
> @@ -155,6 +155,31 @@ static const struct gpio_chip sch_gpio_chip = {
>  	.to_irq			= sch_gpio_to_irq,
>  };
>  
> +static u32 sch_sci_handler(void *context)
> +{
> +	struct sch_gpio *sch = context;
> +	unsigned long core_status, resume_status;
> +	unsigned int resume_gpios, offset;
> +
> +	core_status = inl(sch->iobase + GTS);
> +	resume_status = inl(sch->iobase + GTS + 0x20);
> +
> +	if (core_status == 0 && resume_status == 0)

You can also write this like

	if (!core_status &&& !resume_status)

> +		return ACPI_INTERRUPT_NOT_HANDLED;
> +
> +	for_each_set_bit(offset, &core_status, sch->resume_base)
> +		generic_handle_irq(sch->irq_base + offset);
> +
> +	resume_gpios = sch->chip.ngpio - sch->resume_base;
> +	for_each_set_bit(offset, &resume_status, resume_gpios)
> +		generic_handle_irq(sch->irq_base + sch->resume_base + offset);
> +
> +	outl(core_status, sch->iobase + GTS);
> +	outl(resume_status, sch->iobase + GTS + 0x20);
> +
> +	return ACPI_INTERRUPT_HANDLED;
> +}
> +
>  static int sch_irq_type(struct irq_data *d, unsigned int type)
>  {
>  	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> @@ -215,6 +240,7 @@ static int sch_gpio_probe(struct platform_device *pdev)
>  	struct irq_chip_type *ct;
>  	struct sch_gpio *sch;
>  	struct resource *res;
> +	acpi_status status;
>  	int irq_base, ret;
>  
>  	sch = devm_kzalloc(&pdev->dev, sizeof(*sch), GFP_KERNEL);
> @@ -303,6 +329,10 @@ static int sch_gpio_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	status = acpi_install_sci_handler(sch_sci_handler, sch);
> +	if (ACPI_FAILURE(status))
> +		return -EINVAL;
> +
>  	return 0;
>  }
>  
> -- 
> 2.16.4

Re: [PATCH v3 1/2] gpio: sch: Add edge event support

[Andy Shevchenko]

On Wed, Nov 20, 2019 at 08:20:13PM +0100, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Add the required infrastructure consisting of an irq_chip_generic with
> its irq_chip_type callbacks to enable and report edge events of the pins
> to the gpio core. The actual hook-up of the event interrupt will happen
> separately.

> +static int sch_irq_type(struct irq_data *d, unsigned int type)
> +{
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct sch_gpio *sch = gc->private;
> +	unsigned int gpio_num = d->irq - sch->irq_base;
> +	unsigned long flags;
> +	int rising = 0;
> +	int falling = 0;
> +
> +	switch (type & IRQ_TYPE_SENSE_MASK) {
> +	case IRQ_TYPE_EDGE_RISING:
> +		rising = 1;
> +		break;
> +	case IRQ_TYPE_EDGE_FALLING:
> +		falling = 1;
> +		break;
> +	case IRQ_TYPE_EDGE_BOTH:
> +		rising = 1;
> +		falling = 1;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	spin_lock_irqsave(&sch->lock, flags);
> +	sch_gpio_reg_set(sch, gpio_num, GTPE, rising);
> +	sch_gpio_reg_set(sch, gpio_num, GTNE, falling);
> +	spin_unlock_irqrestore(&sch->lock, flags);

Won't we need to set up IRQ handler here and use handle_bad_irq() during
initialization phase?

> +
> +	return 0;
> +}

> +	irq_base = devm_irq_alloc_descs(&pdev->dev, -1, 0, sch->chip.ngpio,
> +					NUMA_NO_NODE);
> +	if (irq_base < 0)
> +		return irq_base;
> +	sch->irq_base = irq_base;
> +
> +	gc = devm_irq_alloc_generic_chip(&pdev->dev, "sch_gpio", 1, irq_base,
> +					 NULL, handle_simple_irq);
> +	if (!gc)
> +		return -ENOMEM;
> +
> +	gc->private = sch;
> +	ct = gc->chip_types;
> +
> +	ct->chip.irq_mask = sch_irq_mask;
> +	ct->chip.irq_unmask = sch_irq_unmask;
> +	ct->chip.irq_set_type = sch_irq_type;
> +
> +	ret = devm_irq_setup_generic_chip(&pdev->dev, gc,
> +					  IRQ_MSK(sch->chip.ngpio),
> +					  0, IRQ_NOREQUEST | IRQ_NOPROBE, 0);
> +	if (ret)
> +		return ret;

Shan't we do this in the (similar) way how it's done in pinctrl-cherryview.c
driver? (Keep in mind later patches which are going to be v5.5)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/2] gpio: sch: Add edge event support

[Jan Kiszka]

On 22.11.19 12:12, Andy Shevchenko wrote:
> On Wed, Nov 20, 2019 at 08:20:13PM +0100, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> Add the required infrastructure consisting of an irq_chip_generic with
>> its irq_chip_type callbacks to enable and report edge events of the pins
>> to the gpio core. The actual hook-up of the event interrupt will happen
>> separately.
> 
>> +static int sch_irq_type(struct irq_data *d, unsigned int type)
>> +{
>> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>> +	struct sch_gpio *sch = gc->private;
>> +	unsigned int gpio_num = d->irq - sch->irq_base;
>> +	unsigned long flags;
>> +	int rising = 0;
>> +	int falling = 0;
>> +
>> +	switch (type & IRQ_TYPE_SENSE_MASK) {
>> +	case IRQ_TYPE_EDGE_RISING:
>> +		rising = 1;
>> +		break;
>> +	case IRQ_TYPE_EDGE_FALLING:
>> +		falling = 1;
>> +		break;
>> +	case IRQ_TYPE_EDGE_BOTH:
>> +		rising = 1;
>> +		falling = 1;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	spin_lock_irqsave(&sch->lock, flags);
>> +	sch_gpio_reg_set(sch, gpio_num, GTPE, rising);
>> +	sch_gpio_reg_set(sch, gpio_num, GTNE, falling);
>> +	spin_unlock_irqrestore(&sch->lock, flags);
> 
> Won't we need to set up IRQ handler here and use handle_bad_irq() during
> initialization phase?

Why? This is just defining the edge type, not whether an interrupt could 
be generated or not. Also, we only have edge events here, so no reason 
to switch types.

> 
>> +
>> +	return 0;
>> +}
> 
>> +	irq_base = devm_irq_alloc_descs(&pdev->dev, -1, 0, sch->chip.ngpio,
>> +					NUMA_NO_NODE);
>> +	if (irq_base < 0)
>> +		return irq_base;
>> +	sch->irq_base = irq_base;
>> +
>> +	gc = devm_irq_alloc_generic_chip(&pdev->dev, "sch_gpio", 1, irq_base,
>> +					 NULL, handle_simple_irq);
>> +	if (!gc)
>> +		return -ENOMEM;
>> +
>> +	gc->private = sch;
>> +	ct = gc->chip_types;
>> +
>> +	ct->chip.irq_mask = sch_irq_mask;
>> +	ct->chip.irq_unmask = sch_irq_unmask;
>> +	ct->chip.irq_set_type = sch_irq_type;
>> +
>> +	ret = devm_irq_setup_generic_chip(&pdev->dev, gc,
>> +					  IRQ_MSK(sch->chip.ngpio),
>> +					  0, IRQ_NOREQUEST | IRQ_NOPROBE, 0);
>> +	if (ret)
>> +		return ret;
> 
> Shan't we do this in the (similar) way how it's done in pinctrl-cherryview.c
> driver? (Keep in mind later patches which are going to be v5.5)
> 

Can you be a bit more specific for me? Do you mean the pattern 
gpiochip_irqchip_add / gpiochip_set_chained_irqchip? What would be the 
difference / benefit? And how would I link sch_sci_handler to that pattern?

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH v3 1/2] gpio: sch: Add edge event support

[Linus Walleij]

Hi Jan,

thanks for your patch!

On Wed, Nov 20, 2019 at 8:20 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:

> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> Add the required infrastructure consisting of an irq_chip_generic with
> its irq_chip_type callbacks to enable and report edge events of the pins
> to the gpio core. The actual hook-up of the event interrupt will happen
> separately.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Please resend after the merge window, some comments:

First I'm pretty sure this driver can select GPIOLIB_IRQCHIP
and use infrastructure from the core to handle interrupts.

The fact that you register your own irq handler does not
stop that. See for example the solution in
drivers/gpio/gpio-mt7621.c
where we set the ->parent_handler to NULL to let
the driver handle the IRQs itself.

I will try to make this more explicit in the API as we work
with this.

>  struct sch_gpio {
>         struct gpio_chip chip;
>         spinlock_t lock;
>         unsigned short iobase;
>         unsigned short resume_base;
> +       int irq_base;

Why are you keeping this around in the state?
Why not just a local variable?

> +static int sch_gpio_to_irq(struct gpio_chip *gpio, unsigned int offset)
> +{
> +       struct sch_gpio *sch = gpiochip_get_data(gpio);
> +       return sch->irq_base + offset;
> +}
(...)
> +       .to_irq                 = sch_gpio_to_irq,
(...)
> +       irq_base = devm_irq_alloc_descs(&pdev->dev, -1, 0, sch->chip.ngpio,
> +                                       NUMA_NO_NODE);
> +       if (irq_base < 0)
> +               return irq_base;
> +       sch->irq_base = irq_base;
> +
> +       gc = devm_irq_alloc_generic_chip(&pdev->dev, "sch_gpio", 1, irq_base,
> +                                        NULL, handle_simple_irq);
> +       if (!gc)
> +               return -ENOMEM;
(...)
> +       ret = devm_irq_setup_generic_chip(&pdev->dev, gc,
> +                                         IRQ_MSK(sch->chip.ngpio),
> +                                         0, IRQ_NOREQUEST | IRQ_NOPROBE, 0);
> +       if (ret)
> +               return ret;

So I think you can avoid this complexity by jus doing what
gpio-mt7621.c is doing,
use devm_request_irq(), populate girq = &gc->irq; before
registering the gpio_chip pass a handle_simple_irq
and reuse core gpio irqchip infrastructure.

But I don't know everything so let's test and see!

Yours,
Linus Walleij

Re: [PATCH v3 1/2] gpio: sch: Add edge event support

[Andy Shevchenko]

On Fri, Nov 22, 2019 at 04:33:05PM +0100, Jan Kiszka wrote:
> On 22.11.19 12:12, Andy Shevchenko wrote:
> > On Wed, Nov 20, 2019 at 08:20:13PM +0100, Jan Kiszka wrote:

> > > +	switch (type & IRQ_TYPE_SENSE_MASK) {
> > > +	case IRQ_TYPE_EDGE_RISING:
> > > +		rising = 1;
> > > +		break;
> > > +	case IRQ_TYPE_EDGE_FALLING:
> > > +		falling = 1;
> > > +		break;
> > > +	case IRQ_TYPE_EDGE_BOTH:
> > > +		rising = 1;
> > > +		falling = 1;
> > > +		break;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}

> > Won't we need to set up IRQ handler here and use handle_bad_irq() during
> > initialization phase?
> 
> Why? This is just defining the edge type, not whether an interrupt could be
> generated or not. Also, we only have edge events here, so no reason to
> switch types.

OK.

> > > +	irq_base = devm_irq_alloc_descs(&pdev->dev, -1, 0, sch->chip.ngpio,
> > > +					NUMA_NO_NODE);
> > > +	if (irq_base < 0)
> > > +		return irq_base;
> > > +	sch->irq_base = irq_base;
> > > +
> > > +	gc = devm_irq_alloc_generic_chip(&pdev->dev, "sch_gpio", 1, irq_base,
> > > +					 NULL, handle_simple_irq);
> > > +	if (!gc)
> > > +		return -ENOMEM;
> > > +
> > > +	gc->private = sch;
> > > +	ct = gc->chip_types;
> > > +
> > > +	ct->chip.irq_mask = sch_irq_mask;
> > > +	ct->chip.irq_unmask = sch_irq_unmask;
> > > +	ct->chip.irq_set_type = sch_irq_type;
> > > +
> > > +	ret = devm_irq_setup_generic_chip(&pdev->dev, gc,
> > > +					  IRQ_MSK(sch->chip.ngpio),
> > > +					  0, IRQ_NOREQUEST | IRQ_NOPROBE, 0);
> > > +	if (ret)
> > > +		return ret;
> > 
> > Shan't we do this in the (similar) way how it's done in pinctrl-cherryview.c
> > driver? (Keep in mind later patches which are going to be v5.5)
> > 
> 
> Can you be a bit more specific for me? Do you mean the pattern
> gpiochip_irqchip_add / gpiochip_set_chained_irqchip? What would be the
> difference / benefit? And how would I link sch_sci_handler to that pattern?

Now we have struct irq_chip is part of GPIO chip, so, we may use it and supply
needed callbacks and settings before calling gpiochip_add_data().

Will it work in this case?

-- 
With Best Regards,
Andy Shevchenko