Messages in this thread Patch in this message |  | | From | Andrea Tomassetti <> | | Subject | Re: [PATCH RESEND] mux: gpio-mux: add support for 4:1 2-channels mux | | Date | Tue, 5 May 2026 15:20:41 +0000 |
| |
Hi Linus, thank you very much for taking the time to review and reply.
On 5/5/26 10:23, Linus Walleij wrote: > **Warning** This email is from an external address. Please take care to proceed. > > > Hi Andrea, > > thanks for your patch! > > On Thu, Apr 30, 2026 at 4:13 PM Andrea Tomassetti > <andrea.tomassetti@sipearl.com> wrote: > >> Some gpio multiplexers, like TMUX1209, offer differential 4:1 >> or dual 4:1 single-ended channels. Similarly to what already done by >> the adg792a driver, the gpio-mux driver has to take into account >> the #mux-control-cells property and allocate as many controllers >> as advised by it. >> >> So, in the DTS you can now define: >> >> tmux1209: mux-controller { >> compatible = "gpio-mux"; >> #mux-control-cells = <1>; >> >> mux-gpios = <&gpio_expander 01 GPIO_ACTIVE_HIGH>, >> <&gpio_expander 02 GPIO_ACTIVE_HIGH>; >> }; >> >> adcmux30: adcmux30 { >> compatible = "io-channel-mux"; >> io-channels = <&adc1 4>; >> io-channel-names = "parent"; >> #io-channel-cells = <1>; >> mux-controls = <&tmux1209 0>; >> >> channels = "S1A", "S2A", "S3A", "S4A"; >> }; >> >> adcmux31: adcmux31 { >> compatible = "io-channel-mux"; >> io-channels = <&adc1 5>; >> io-channel-names = "parent"; >> #io-channel-cells = <1>; >> mux-controls = <&tmux1209 1>; >> >> channels = "S1B", "S2B", "S3B", "S4B"; >> }; >> >> Signed-off-by: Andrea Tomassetti <andrea.tomassetti@sipearl.com> > > The mux controller binding looks like this: > > properties: > '#mux-control-cells': > enum: [ 0, 1 ] > > So you do not patch the bindings, you actually implement this > for the case when #mux-control-cells is 1. > > Please detail this in the commit. > Exactly, I'm not patching the bindings, there's no need. I tried to detail this in my commit message:
[...] the gpio-mux driver has to take into account the #mux-control-cells property and allocate as many controllers as advised by it.
but it looks like is not as clear as I thought. Maybe I can add the fact that "patching the binding is not needed because the binding already supports #mux-control-cells 0 and 1". What do you think?
>> - mux_chip = devm_mux_chip_alloc(dev, 1, sizeof(*mux_gpio)); >> + ret = device_property_read_u32(dev, "#mux-control-cells", &cells); >> + if (ret < 0) >> + cells = 0; >> + >> + if (cells >= 2) { >> + dev_err(dev, "invalid control-cells %u\n", cells); >> + return -EINVAL; >> + } > > Maybe put in a comment that the bindings only allow 0 or 1 cell. > Noted it!
>> + >> + mux_chip = devm_mux_chip_alloc(dev, cells + 1, sizeof(*mux_gpio)); > > Otherwise looks correct to me. > >> - mux_chip->mux->states = BIT(pins); >> + >> + for (i = 0; i < mux_chip->controllers; ++i) >> + mux_chip->mux[i].states = BIT(pins); > > Is the mux core handling any other specifics? (I guess so...) > I guess so too but if there's something specific I'm missing, please let me know.
Moreover, I just realized that I missed two lines that should be probably adjusted as well:
@@ -78,7 +91,8 @@ static int mux_gpio_probe(struct platform_device *pdev) return -EINVAL; }
- mux_chip->mux->idle_state = idle_state; + for (i = 0; i < mux_chip->controllers; ++i) + mux_chip->mux[i].idle_state = idle_state; }
ret = devm_regulator_get_enable_optional(dev, "mux"); @@ -89,8 +103,8 @@ static int mux_gpio_probe(struct platform_device *pdev) if (ret < 0) return ret;
- dev_info(dev, "%u-way mux-controller registered\n", - mux_chip->mux->states); + dev_info(dev, "%u-way mux-controller registered, %u controllers\n", + mux_chip->mux->states, mux_chip->controllers);
return 0; }
Should I integrate these modifications on the next version of the patch?
Thank you very much, Andrea
> With the above added comments, details: > Reviewed-by: Linus Walleij <linusw@kernel.org> > > Thanks! > Linus Walleij >
|  |