Re: [PATCH v2 2/2] mux: gpio: Add optional enable gpio

[Peter Rosin <peda@lysator.liu.se>]

Hi,

Thanks for the patches! This all looks nice, but I do have a
few cosmetic nits...

Den Mon, Aug 31, 2026 at 10:28:25AM +0000, skrev Tapio Reijonen:
> Analog multiplexers have an enable input that disconnects all channels

Some analog multiplexers ...
or perhaps just
Some multiplexers ...

> when deasserted, independent of the address inputs; on a 74HC4051 it is
> the E input. Add it as an optional gpio.
> 
> The mux gpios are not updated atomically. gpiod_multi_set_value_cansleep()

The mux gpios are not guaranteed to be updated atomically.

> groups them per gpio controller, and only controllers implementing
> set_multiple() update theirs in a single write, so a mux with its address

... set_multiple() can possibly update theirs ...

> inputs spread over two controllers passes through the intermediate
> addresses on every change. Deassert the enable gpio for the duration of
> the update and assert it once the address is settled.
> 
> The enable gpio is also what makes an idle state of MUX_IDLE_DISCONNECT
> possible, which the mux core applies when the chip is registered and after
> every deselect: it leaves the enable gpio deasserted and the address
> inputs alone. Refuse that idle state without an enable gpio, because the
> address inputs cannot disconnect anything on their own and mux_gpio_set()
> would instead drive them to the bit pattern of MUX_IDLE_DISCONNECT.
> 
> Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
> ---
>  drivers/mux/gpio.c | 34 ++++++++++++++++++++++++++++------
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mux/gpio.c b/drivers/mux/gpio.c
> index f9c7863e51b82fb76ed03cf17308fae74716cf6d..df5d24214bd5fa8c3c1d733b25d32ca88c06c8c0 100644
> --- a/drivers/mux/gpio.c
> +++ b/drivers/mux/gpio.c
> @@ -18,6 +18,7 @@
>  
>  struct mux_gpio {
>  	struct gpio_descs *gpios;
> +	struct gpio_desc *enable_gpio;

Please drop the _gpio suffix from the name.

>  };
>  
>  static int mux_gpio_set(struct mux_control *mux, int state)
> @@ -26,10 +27,18 @@ static int mux_gpio_set(struct mux_control *mux, int state)
>  	DECLARE_BITMAP(values, BITS_PER_TYPE(state));
>  	u32 value = state;
>  
> +	/* The gpios are not updated atomically, disable the mux meanwhile. */

	/*
	 * The gpios might not be updated atomically, disable the mux
	 * meanwhile.
	 */

> +	gpiod_set_value_cansleep(mux_gpio->enable_gpio, 0);
> +
> +	if (state == MUX_IDLE_DISCONNECT)
> +		return 0;
> +
>  	bitmap_from_arr32(values, &value, BITS_PER_TYPE(value));
>  
>  	gpiod_multi_set_value_cansleep(mux_gpio->gpios, values);
>  
> +	gpiod_set_value_cansleep(mux_gpio->enable_gpio, 1);
> +
>  	return 0;
>  }
>  
> @@ -70,14 +79,27 @@ static int mux_gpio_probe(struct platform_device *pdev)
>  	WARN_ON(pins != mux_gpio->gpios->ndescs);
>  	mux_chip->mux->states = BIT(pins);
>  
> +	mux_gpio->enable_gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_LOW);
> +	if (IS_ERR(mux_gpio->enable_gpio))
> +		return dev_err_probe(dev, PTR_ERR(mux_gpio->enable_gpio),
> +				     "failed to get optional enable gpio\n");
> +
>  	ret = device_property_read_u32(dev, "idle-state", (u32 *)&idle_state);
> -	if (ret >= 0 && idle_state != MUX_IDLE_AS_IS) {
> -		if (idle_state < 0 || idle_state >= mux_chip->mux->states) {
> -			dev_err(dev, "invalid idle-state %u\n", idle_state);
> -			return -EINVAL;
> +	if (ret >= 0) {
> +		if (idle_state == MUX_IDLE_DISCONNECT) {
> +			if (!mux_gpio->enable_gpio)
> +				return dev_err_probe(dev, -EINVAL,
> +						     "idle-state disconnect requires enable-gpios\n");
> +
> +			mux_chip->mux->idle_state = MUX_IDLE_DISCONNECT;
> +		} else if (idle_state != MUX_IDLE_AS_IS) {
> +			if (idle_state < 0 || idle_state >= mux_chip->mux->states) {
> +				return dev_err_probe(dev, -EINVAL,
> +						     "invalid idle-state %d\n",
> +						     idle_state);
> +			}
> +			mux_chip->mux->idle_state = idle_state;

The new code is a bit convoluted and too deeply indented, methinks. I'd
prefer:

	ret = device_property_read_u32(dev, "idle-state", (u32 *)&idle_state);
	if (ret < 0)
		idle_state = mux_chip->mux->idle_state;

	if (idle_state == MUX_IDLE_AS_IS) {
	} else if (idle_state == MUX_IDLE_DISCONNECT && !mux_gpio->enable) {
		dev_err(dev, "idle-state disconnect requires enable-gpios\n");
		return -EINVAL;
	} else if (idle_state == MUX_IDLE_DISCONNECT) {
	} else if (idle_state < 0 || idle_state >= mux_chip->mux->states)
		dev_err(dev, "invalid idle-state %d\n", idle_state);
		return -EINVAL;
	}

	mux_chip->mux->idle_state = idle_state;

Using the "return dev_err_probe(dev, -EINVAL, ...)" pattern only buys
you an "error EINVAL" prefix but costs over-long lines. So, not worth
it to me.

Beware, the above snippet was written directly in the MUA and has never
seen a compiler. It probably has some silly bug, but something like
that...

Cheers,
Peter

>  		}
> -
> -		mux_chip->mux->idle_state = idle_state;
>  	}
>  
>  	ret = devm_regulator_get_enable_optional(dev, "mux");
> 
> -- 
> 2.47.3
>