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

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(-)

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

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)

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

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

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?