Re: [PATCH RFC 3/4] gpio: loongson-64bit: Add shared interrupt support

[Miao Wang <shankerwangmiao@gmail.com>]

Hi Linus,

Thanks for your suggestions.

> 2026年10月01日 16:33，Linus Walleij <linusw@kernel.org> 写道：
> 
> Hi Miao,
> 
> thanks for your patch!
> 
> On Fri, Sep 25, 2026 at 6:28 PM Miao Wang via B4 Relay
> <devnull+shankerwangmiao.gmail.com@kernel.org> wrote:
> 
>> +struct loongson_gpio_shared_per_parent_irq_data {
>> +       unsigned int parent_index;
>> +       unsigned int parent_irq;
>> +       /*
>> +        * The lock of the irq_desc of the parent irq also protects
>> +        * exclusive/usedby below. It must be taken before
>> +        * lgpio->shared_irq_data->lock.
>> +        */
>> +       struct irq_data *parent_irq_data;
>> +       struct loongson_gpio_chip *lgpio;
>> +
>> +       /*
>> +        * exclusive: exclusive usage, only one gpio pin can use this parent irq.
>> +        *   It will happen in two cases:
>> +        *     - The parent irq is used by a gpio pin which is edge triggered, and
>> +        *       thus cannot be used by other gpio pins which are mapped to the same
>> +        *       parent irq.
>> +        *     - The parent irq is used by a gpio pin which is level triggered, and
>> +        *       the gpio chip lacks the polarity control register, and thus the
>> +        *       interrupt polarity is controlled by the parent irq controller, and
>> +        *       thus cannot be used by other gpio pins which are mapped to the
>> +        *       same parent irq.
>> +        *   When set, the request for other gpio pins which are mapped to this
>> +        *   parent irq will be rejected.
>> +        */
>> +       unsigned int exclusive : 1;
> 
> So use bool exclusive;
> 
>> +       /*
>> +        * usedby: When exclusive is set, these bits indicate the gpio pin which
>> +        *   is using this parent irq. When exclusive is cleared, these bits
>> +        *   indicate the number of gpio pins which are using this parent irq.
>> +        * When both exclusive and usedby are 0, it means that the parent irq is not
>> +        * used by any gpio pin.
>> +        */
>> +       unsigned int usedby : 31;
> 
> Is this a bitmap? Then use a bitmap abstraction.

When exclusive is 1, it is a pure number while when exclusive is 0, 
it is a bitmap.

> 
>> +static void loongson_gpio_shared_irq_unmask(struct irq_data *data)
>> +{
>> +       struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
>> +       struct loongson_gpio_chip *lgpio = to_loongson_gpio_chip(chip);
>> +       irq_hw_number_t hwirq = irqd_to_hwirq(data);
>> +       unsigned int parent_irq_index = lgpio->chip_data->irq_mapping(lgpio, hwirq);
>> +       struct loongson_gpio_shared_per_parent_irq_data *ppid =
>> +               &lgpio->shared_irq_data->per_parent_irq_data[parent_irq_index];
>> +       struct irq_data *parent_irq_data = ppid->parent_irq_data;
> 
> Just look at this code.
> 
> Clearly this needs refactoring, we can't have function heads looking
> like this.
> 
>> +
>> +       if (!lgpio->shared_irq_data->irq_in_use[hwirq])
>> +               return;
>> +
>> +       guard(raw_spinlock_irqsave)(&irq_data_to_desc(ppid->parent_irq_data)->lock);
>> +
>> +       if (ppid->exclusive)
>> +               parent_irq_data->chip->irq_mask(parent_irq_data);
>> +
>> +       scoped_guard(raw_spinlock_irqsave, &lgpio->shared_irq_data->lock)
>> +               loongson_gpio_write_register(lgpio, lgpio->chip_data->inten_offset,
>> +                                            hwirq, 1);
> 
> This locking is ... really hard to follow, what is going on?
> It looks like code written by an LLM.

No, it is written by my hand. lgpio->shared_irq_data->lock is used to protect
the irq related registers of the GPIO controller. It is necessary because writing
to a bit of a register may involve RMW, which is not atomic. As a result, every
call to loongson_gpio_{read,write}_register is protected by this lock.

> 
>> +static int loongson_gpio_shared_irq_set_type(struct irq_data *data, unsigned int type)
>> +{
>> +       struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
>> +       struct loongson_gpio_chip *lgpio = to_loongson_gpio_chip(chip);
>> +       irq_hw_number_t hwirq = irqd_to_hwirq(data);
>> +       unsigned int parent_irq_index = lgpio->chip_data->irq_mapping(lgpio, hwirq);
>> +       struct loongson_gpio_shared_per_parent_irq_data *ppid =
>> +               &lgpio->shared_irq_data->per_parent_irq_data[parent_irq_index];
>> +       unsigned int intpol_offset = lgpio->chip_data->intpol_offset;
>> +       struct irq_data *parent_irq_data = ppid->parent_irq_data;
>> +       struct irq_chip *parent_irq_chip = parent_irq_data->chip;
>> +       int ret = 0;
> 
> Again, what is this? I can't read functions that look like this,
> even less maintain them.
> 
>> +       /*
>> +        * When the irq is not exclusively used and is currently shared by
>> +        * more than one gpio pin; or is currently used by single gpio pin and
>> +        * the gpio pin is not the one requesting to set the irq type, then the
>> +        * irq type can only be set to level triggered.
>> +        */
>> +       if (!ppid->exclusive &&
>> +           (ppid->usedby > 1 ||
>> +            (ppid->usedby == 1 && !lgpio->shared_irq_data->irq_in_use[hwirq]))) {
>> +               if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_LEVEL_LOW)
>> +                       return -EBUSY;
>> +
>> +               /*
>> +                * When exclusive is not set and usedby at least one, the gpio chip must
>> +                * have intpol register to control the interrupt polarity. We will never
>> +                * reach here if the gpio chip lacks intpol register, because in that
>> +                * case, exclusive will be set no matter what trigger type is requested.
>> +                */
>> +               BUG_ON(!intpol_offset);
>> +               /*
>> +                * Since only level triggered irqs are allowed to be shared, we can
>> +                * safely set the intpol register without first disabling the pin in
>> +                * inten register.
>> +                */
>> +               scoped_guard(raw_spinlock_irqsave, &lgpio->shared_irq_data->lock)
>> +                       loongson_gpio_write_register(lgpio, intpol_offset, hwirq,
>> +                                                    type == IRQ_TYPE_LEVEL_HIGH);
>> +
>> +               if (!lgpio->shared_irq_data->irq_in_use[hwirq])
>> +                       ppid->usedby++;
>> +
>> +               irq_set_handler_locked(data, handle_level_irq);
> 
> I like the attention to detail here though!
> 
>> +static void loongson_gpio_shared_irq_shutdown(struct irq_data *data)
>> +{
>> +       struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
>> +       struct loongson_gpio_chip *lgpio = to_loongson_gpio_chip(chip);
>> +       irq_hw_number_t hwirq = irqd_to_hwirq(data);
>> +       unsigned int parent_irq_index = lgpio->chip_data->irq_mapping(lgpio, hwirq);
>> +       struct loongson_gpio_shared_per_parent_irq_data *ppid =
>> +               &lgpio->shared_irq_data->per_parent_irq_data[parent_irq_index];
> 
> Here again some crazy function header.
> 
> I think some helpers need to be broken out so the code becomes more
> readable, see what you can do with it to make it easier to follow.

I did notice that the header of these functions are duplicated and are a bit long.
However, multiple of the variables defined here will be used later. It is not
obtaining B from A, C from B, and D from C and only using D later, so that we can
introduce a helper to obtain D from A. The problem here is that C and D will both
be used later. What's your idea about the styling here?

Cheers,
Miao Wang