Re: [PATCH v1 5/5] irqchip: starfive: Implement irq_set_type and irq_ack hooks

Thomas Gleixner
2026-04-10T14:47:01Z

On Fri, Apr 10 2026 at 02:01, Changhuang Liang wrote:

s/hooks/callbacks and please use function notation irq_set_type() and ....

> +static void starfive_intc_mod(struct starfive_irq_chip *irqc, u32 reg,
> + u32 mask, u32 data)

No line break required. You have 100 characters.

> +static int starfive_intc_set_type(struct irq_data *d, unsigned int type)
> +{
> + struct starfive_irq_chip *irqc = irq_data_get_irq_chip_data(d);
> + u32 i, bitpos, ty_pos, ty_shift, tmp;
> +
> + i = d->hwirq / STARFIVE_INTC_SRC_IRQ_NUM;
> + bitpos = d->hwirq % STARFIVE_INTC_SRC_IRQ_NUM;
> + ty_pos = bitpos / STARFIVE_INTC_TYPE_NUM;
> + ty_shift = (bitpos % STARFIVE_INTC_TYPE_NUM) * 2;
> +
> + switch (type) {
> + case IRQF_TRIGGER_LOW:
> + tmp = STARFIVE_INTC_TRIGGER_LOW << ty_shift;

tmp is not really an intuitive variable name.

> + irq_set_handler_locked(d, handle_level_irq);
> + break;
> + case IRQF_TRIGGER_HIGH:
> + tmp = STARFIVE_INTC_TRIGGER_HIGH << ty_shift;
> + irq_set_handler_locked(d, handle_level_irq);
> + break;
> + case IRQF_TRIGGER_FALLING:
> + tmp = STARFIVE_INTC_TRIGGER_NEGEDGE << ty_shift;
> + irq_set_handler_locked(d, handle_edge_irq);
> + break;
> + case IRQF_TRIGGER_RISING:
> + tmp = STARFIVE_INTC_TRIGGER_POSEDGE << ty_shift;
> + irq_set_handler_locked(d, handle_edge_irq);

This can be simplified so it avoids to have a function in every case
statement:

switch (type) {
case IRQF_TRIGGER_LOW:
trigger = STARFIVE_INTC_TRIGGER_LOW;
handler = handle_level_irq;
break;
case ...
}

irq_set_handler_locked(d, handler);
typeval = trigger << ty_shift;

You get the idea.

> + raw_spin_lock(&irqc->lock);

guard(...)

Thanks,

tglx