Re: [PATCH v1 4/8] pinctrl: add NXP MC33978/MC34978 pinctrl driver

[Oleksij Rempel]

Hi Linus,

On Fri, Feb 27, 2026 at 12:40:40AM +0100, Linus Walleij wrote:
..
> > +static int mc33978_get(struct gpio_chip *chip, unsigned int offset)
> > +{
> > +       struct mc33978_pinctrl *mpc = gpiochip_get_data(chip);
> > +       int status, ret;
> > +       bool is_switch_closed;
> > +       bool is_switch_to_ground = true; /* Default for all SG pins */
> > +
> > +       mutex_lock(&mpc->lock);
> > +
> > +       /* Read hardware switch status (open or closed) */
> > +       ret = mc33978_read(mpc, MC33978_REG_READ_IN, &status);
> > +       if (ret < 0) {
> > +               mutex_unlock(&mpc->lock);
> > +               return 0;
> > +       }
> > +       is_switch_closed = !!(status & BIT(offset));
> > +
> > +       /* Determine current topology for SP pins */
> > +       if (MC33978_IS_SP(offset)) {
> > +               int config_reg;
> > +
> > +               ret = mc33978_read(mpc, MC33978_REG_CONFIG, &config_reg);
> > +               if (ret == 0) {
> > +                       /* CONFIG: 0 = Switch-to-Ground (PU), 1 = Switch-to-Battery (PD) */
> > +                       if (config_reg & MC33978_PINMASK(offset))
> > +                               is_switch_to_ground = false;
> > +               }
> > +       }
> > +
> > +       mutex_unlock(&mpc->lock);
> > +
> > +       /* Translate hardware switch semantics to logical GPIO levels */
> > +       if (is_switch_to_ground) {
> > +               /* SG: Switch open -> High (1), Switch to GND -> Low (0) */
> > +               status = !is_switch_closed;
> > +       } else {
> > +               /* SB: Switch open -> Low (0), Switch to Vbat -> High (1) */
> > +               status = is_switch_closed;
> > +       }
> 
> I don't think this is right.
> 
> The driver needs to report the *physical* level on the line. Then the
> lines need to be flagged with GPIO_ACTIVE_LOW or GPIO_ACTIVE_HIGH
> on the consumers in the device tree.

Returning the physical level is actually exactly what this code is
trying to do. I need to rewrite the comment :) 

The issue is that the MC33978 hardware does not report the physical
voltage level on the pin. As per section 9.10.27 (Read switch status) of
the datasheet: "A Logic [1] means the switch is closed while a Logic [0]
is an open switch."

Because it only reports this abstract "contact status", I have to
translate it back to the actual physical voltage level (1 = High, 0 =
Low) based on the pin's current configuration:

In Switch-to-Ground (SG) mode: the status bit stays 0 when the physical
voltage on the line is High (open), and reports 1 when the physical
voltage is Low (shorted to ground).
In Switch-to-Battery (SB) mode: the exact opposite happens.

Best Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |