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

[Thomas Gleixner]

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