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@xxxxxxxxxxx>
> >
> > 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