Re: [PATCH v2 4/6] pinctrl: add NXP MC33978/MC34978 pinctrl driver

[Linus Walleij]

Hi Oleksij,

this is starting to look good!

One thing bothers me:

On Tue, Mar 3, 2026 at 2:40 PM Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> wrote:

> - Add a comment to mc33978_gpio_to_irq() explaining why it must act as a
>   proxy to the parent MFD's irq_domain (shared physical INT_B line with hwmon).
(...)

> +static int mc33978_set(struct gpio_chip *chip, unsigned int offset, int value)
> +{
> +       struct mc33978_pinctrl *mpc = gpiochip_get_data(chip);
> +       int pull;
> +       int ret;
> +
> +       /*
> +        * We emulate open-drain/-source outputs by routing or isolating the
> +        * active wetting current sources.
> +        * To drive the line, we apply the current source.
> +        * To turn the line OFF (achieve High-Impedance), we MUST use the
> +        * hardware TRI_SP / TRI_SG tri-state registers to physically isolate
> +        * it.
> +        */
> +       if (mc33978_is_sp(offset)) {
> +               pull = value ? MC33978_PU : MC33978_PD;
> +               value = 1;
> +       } else {
> +               pull = MC33978_PU;
> +       }
> +
> +       mutex_lock(&mpc->lock);

Have you considered using guards for this to avoid the goto:s?

> +static int mc33978_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct mc33978_pinctrl *mpc = gpiochip_get_data(chip);
> +       int virq;
> +
> +       if (!mpc->domain)
> +               return -ENXIO;
> +
> +       /*
> +        * The hardware shares a single physical INT_B line for both GPIO pin
> +        * changes and internal hardware faults (hwmon). Therefore, the IRQ
> +        * domain and threaded handler are centrally managed by the MFD core.
> +        */
> +       virq = irq_create_mapping(mpc->domain, offset);
> +       if (!virq) {
> +               dev_err(mpc->dev, "Failed to map hwirq %u to virq\n", offset);
> +               return -ENXIO;
> +       }
> +
> +       return virq;
> +}

I don't know about this.

If it is clear from internal registers which bits represents the hwmon
IRQs and which bits are GPIO IRQs, the modern way to handle this
is hierarchical irqchip. Have you looked into that?

For a simple example c.f. drivers/gpio/gpio-ixp4xx.c how this
is registered with a parent irqchip using
girq->parent_domain = parent;
girq->child_to_parent_hwirq = ixp4xx_gpio_child_to_parent_hwirq;
and
ixp4xx_gpio_child_to_parent_hwirq().

This way a unique irqchip for GPIO can be created as a child
of the MFD irqchip.

Yours,
Linus Walleij