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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 22 Nov 2019 16:33:05 +0100
From:   Jan Kiszka <jan.kiszka@...mens.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-gpio@...r.kernel.org,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
        "Rafael J . Wysocki" <rafael.j.wysocki@...el.com>
Subject: Re: [PATCH v3 1/2] gpio: sch: Add edge event support

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@...mens.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