Re: [EXTERNAL] Re: [PATCH 2/2] mux-controller: ti: add driver for event mux router

["Sharma, Rahul" <r-sharma3@ti.com>]

Hi Peter,

Thanks for detailed review.

On 8/29/2026 8:44 PM, Peter Rosin wrote:
> Hi Rahul, I believe you posted something like this as an RFC about half
> a year ago? Please include a pointer to the previous posting along with
> some notes about what changed since the last posting when you update a
> patch series. Thanks! And
> 
> 
> Hi Rahul,
> 
> I believe you posted something like this as an RFC about half a year
> ago? Please include a pointer to the previous posting along with some
> notes about what changed since the last posting when you update a
> patch series. Thanks!

I will follow this with v2. For now I am adding the RFC link below,
https://lore.kernel.org/lkml/20260313060437.3704592-1-r-sharma3@ti.com

Not much has changed since RFC was posted first. In the binding doc
there was a term "syscon" which got added mistakenly, is now removed.
Event mux router is a complete IP block and not a syscon node, rest of
the content is same.

Due to lack of comments in RFC, this time I posted as fresh patch
series. That was anyhow the original plan.

> 
> And sorry for being so very slow with the review. I intend to be more
> responsive going forward...
> 
> Den Fri, Aug 28, 2026 at 03:36:15PM +0530, skrev Rahul Sharma:
>> The driver supports event muxing routers like gpio mux router and timesync
>> router. This driver is adaptation of original reg-mux driver, along with
>> changes specific to support TI's mux router.
>> 
>> The idle states this driver supports are only 2 which active(represented
>> by 1 in dt-node) and in-active(represented by 0 in dt-node).
> 
> I don't see the point of using 1 as idle state. Why would anyone
> do that?
> 
>> 
>> Signed-off-by: Rahul Sharma <r-sharma3@ti.com>
>> ---
>>  drivers/mux/Kconfig           |  15 +++
>>  drivers/mux/Makefile          |   2 +
>>  drivers/mux/ti-k3-event-mux.c | 235 ++++++++++++++++++++++++++++++++++
>>  3 files changed, 252 insertions(+)
>>  create mode 100644 drivers/mux/ti-k3-event-mux.c
>> 
>> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
>> index eb34457beaab..58739a01f0d5 100644
>> --- a/drivers/mux/Kconfig
>> +++ b/drivers/mux/Kconfig
>> @@ -83,6 +83,21 @@ config MUX_RZV2H_VBENCTL
>>  	  To compile the driver as a module, choose M here: the module will
>>  	  be called mux-rzv2h-vbenctl.
>>  
>> +config MUX_TI_K3_EVENT_ROUTER
>> +	tristate "TI Event Mux Router using MMIO registers"
>> +	depends on OF && (REGMAP_MMIO || COMPILE_TEST)
>> +	help
>> +	  This is extension of MMIO mux for  timesync router and gpiomux
>> +	  routers on TI K3 SoCs. This driver supports the 3-field format for
>> +	  mux control: <register-offset mask value>.
> 
> The first sentence has some grammar issues, and the second talks
> about "the 3-field format" as if that is some well established format.

I just mentioned because it is different from standard Key-Value pairs :) .

> How about:
> 
> 	  This is a mux for timesync and gpiomux routers on TI K3 SoCs.
> 	  The driver supports a 3-field format for mux control:
> 	  <register-offset mask value>.
> 
>> +
>> +	  The driver allows configuration of hardware mux routers using
>> +	  memory-mapped registers. It's based on the mmio-mux driver but
>> +	  supports the extended 3-field format for more precise control.
> 
> The person reading this would not care about the code ancestry, that
> info belongs elsewhere, and the 3-field format has already been
> mentioned. Thus, the second sentence can be dropped.
> 
>> +
>> +	  To compile the driver as a module, choose M here: the module will
>> +	  be called mux-ti-k3-event.
>> +
>>  endmenu
>>  
>>  endif # MULTIPLEXER
>> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
>> index 0854c04613b9..114abf88b75e 100644
>> --- a/drivers/mux/Makefile
>> +++ b/drivers/mux/Makefile
>> @@ -9,6 +9,7 @@ mux-adgs1408-objs		:= adgs1408.o
>>  mux-gpio-objs			:= gpio.o
>>  mux-mmio-objs			:= mmio.o
>>  mux-rzv2h-vbenctl-objs		:= rzv2h-vbenctl.o
>> +mux-ti-k3-event-objs		:= ti-k3-event-mux.o
>>  
>>  obj-$(CONFIG_MULTIPLEXER)	+= mux-core.o
>>  obj-$(CONFIG_MUX_ADG792A)	+= mux-adg792a.o
>> @@ -16,3 +17,4 @@ obj-$(CONFIG_MUX_ADGS1408)	+= mux-adgs1408.o
>>  obj-$(CONFIG_MUX_GPIO)		+= mux-gpio.o
>>  obj-$(CONFIG_MUX_MMIO)		+= mux-mmio.o
>>  obj-$(CONFIG_MUX_RZV2H_VBENCTL)	+= mux-rzv2h-vbenctl.o
>> +obj-$(CONFIG_MUX_TI_K3_EVENT_ROUTER)	+= mux-ti-k3-event.o
>> diff --git a/drivers/mux/ti-k3-event-mux.c b/drivers/mux/ti-k3-event-mux.c
>> new file mode 100644
>> index 000000000000..2469500d1b48
>> --- /dev/null
>> +++ b/drivers/mux/ti-k3-event-mux.c
>> @@ -0,0 +1,235 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * MMIO register bit-field controlled multiplexer driver
> 
> This is some left-over I presume?
> 
>> + *
>> + * Copyright (C) 2026 Texas Instruments Incorporated - https://www.ti.com 
>> + *
>> + * Based on drivers/mux/mmio.c by Philipp Zabel <kernel@pengutronix.de>
> 
> So, why did you drop the Pengutronix copyright?
> 
>> + * Modified to support 3-field format: reg-offset, mask & value
>> + *
>> + * Author: Rahul Sharma <r-sharma3@ti.com>
>> + */
>> +
>> +#include <linux/bitops.h>
>> +#include <linux/err.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/module.h>
>> +#include <linux/mux/driver.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/property.h>
>> +#include <linux/regmap.h>
>> +
>> +#define MUX_ENABLE_INTR BIT(16)
>> +
>> +struct mux_ti_k3_event {
>> +	struct regmap *regmap;
> 
> I think this can be a single pointer in the below chip struct
> instead of "wasting" one pointer for each field, no?
> 
>> +	u32 reg;
>> +	u32 mask;
>> +	u32 value;
>> +};
>> +
>> +struct mux_ti_k3_event_chip {
>> +	struct mux_chip *mux_chip;
> 
> I do not see the need for this back-pointer to the mux chip.
> 
>> +	struct mux_ti_k3_event *fields;
>> +	int num_fields;
> 
> This is redundant. The mux-control count for a chip is available
> as mux_chip->controllers.
> 
>> +	u32 *saved_states;
>> +};
>> +
>> +static int mux_ti_k3_event_suspend(struct device *dev)
>> +{
>> +	struct mux_ti_k3_event_chip *chip = dev_get_drvdata(dev);
>> +	int i, ret;
>> +
>> +	if (!chip->saved_states) {
>> +		chip->saved_states = devm_kcalloc(dev, chip->num_fields,
>> +						  sizeof(u32), GFP_KERNEL);
> 
> Why not allocate this up-front during probe? Or, on second thought, why
> not just add a saved_state (non-array) member to struct mux_ti_k3_event?
> 
>> +		if (!chip->saved_states)
>> +			return -ENOMEM;
>> +	}
>> +
>> +	for (i = 0; i < chip->num_fields; i++) {
>> +		struct mux_ti_k3_event *field = &chip->fields[i];
>> +
>> +		ret = regmap_read(field->regmap, field->reg,
>> +				  &chip->saved_states[i]);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int mux_ti_k3_event_resume(struct device *dev)
>> +{
>> +	struct mux_ti_k3_event_chip *chip = dev_get_drvdata(dev);
>> +	int i, ret;
>> +
>> +	if (!chip->saved_states)
>> +		return 0;
>> +
>> +	for (i = 0; i < chip->num_fields; i++) {
>> +		struct mux_ti_k3_event *field = &chip->fields[i];
>> +
>> +		ret = regmap_write(field->regmap, field->reg,
>> +				   chip->saved_states[i]);
> 
> Should you not apply the mask here?
> 
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static DEFINE_SIMPLE_DEV_PM_OPS(mux_ti_k3_event_pm_ops,
>> +				mux_ti_k3_event_suspend,
>> +				mux_ti_k3_event_resume);
>> +
>> +/*
>> + * State behavior:
>> + * - state 0: Clears the mask bits in the target register (inactive state)
>> + * - state 1: Sets both the value bits and enable bit (bit 16) in the register
>> + */
>> +static int mux_ti_k3_event_set(struct mux_control *mux, int state)
>> +{
>> +	struct mux_ti_k3_event *fields = mux_chip_priv(mux->chip);
>> +	struct mux_ti_k3_event *field = &fields[mux_control_get_index(mux)];
>> +
>> +	if (!state)
>> +		return regmap_update_bits(field->regmap, field->reg, field->mask, 0);
> 
> This is confusing to me. Why do you elect to leave the enable bit as-is
> and write only the zero value? Would it not be saner to clear out the
> enable bit as well?
> 
> You should handle state == MUX_IDLE_DISCONNECT here in the .set function,
> see below for rationale.
> 
>> +
>> +	return regmap_update_bits(field->regmap, field->reg, field->mask | MUX_ENABLE_INTR,
>> +		field->value | MUX_ENABLE_INTR);
>> +}
>> +
>> +static const struct mux_control_ops mux_ti_k3_event_ops = {
>> +	.set = mux_ti_k3_event_set,
>> +};
>> +
>> +static const struct regmap_config mux_ti_k3_event_regmap_cfg = {
>> +	.reg_bits = 32,
>> +	.val_bits = 32,
>> +	.reg_stride = 4,
>> +};
>> +
>> +static int mux_ti_k3_event_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *np = dev->of_node;
>> +	struct mux_ti_k3_event_chip *chip;
>> +	struct mux_ti_k3_event *fields;
>> +	struct mux_chip *mux_chip;
>> +	struct regmap *regmap;
>> +	void __iomem *base;
>> +	int num_fields;
>> +	int ret;
>> +	int i;
>> +
>> +	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
>> +	if (!chip)
>> +		return -ENOMEM;
>> +
>> +	base = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(base)) {
>> +		return dev_err_probe(dev, -ENODEV,
>> +				     "failed to get base address\n");
>> +	} else {
> 
> Drop the else + indentation when the if-block always returns.
> 
>> +		regmap = devm_regmap_init_mmio(dev, base, &mux_ti_k3_event_regmap_cfg);
>> +	}
>> +	if (IS_ERR(regmap)) {
>> +		iounmap(base);
> 
> Why do you need to explicitely unmap base?
> 
>> +		return dev_err_probe(dev, PTR_ERR(regmap),
>> +				     "failed to get regmap\n");
>> +	}
>> +
>> +	ret = of_property_count_u32_elems(np, "ti,reg-mask-val");
>> +	if (!ret || ret % 3) {
>> +		ret = -EINVAL;
>> +		dev_err(dev, "ti,reg-mask-val property missing or invalid: %d\n",
>> +			ret);
>> +		return ret;
>> +	}
>> +
>> +	num_fields = ret / 3;
>> +	mux_chip = devm_mux_chip_alloc(dev, num_fields, num_fields *
>> +				       sizeof(*fields));
>> +	if (IS_ERR(mux_chip))
>> +		return PTR_ERR(mux_chip);
>> +
>> +	fields = mux_chip_priv(mux_chip);
>> +	chip->mux_chip = mux_chip;
> 
> This feels backwards to me. Should not "chip" be what is returned
> from mux_chip_priv()? I.e. something like this:
> 
> 	mux_chip = devm_mux_chip_alloc(dev, num_fields, sizeof(*chip));
> 	chip = mux_chip_priv(mux_chip);
> 	chip->fields = devm_kcalloc(dev, num_fields, sizeof...
> 
> (error checking omitted)
> 
>> +	chip->fields = fields;
>> +	chip->num_fields = num_fields;
>> +
>> +	platform_set_drvdata(pdev, chip);
> 
> I think you should use mux_chip as drvdata. I assume this is why you
> needed the back pointer to the mux_chip at some point.
> 
>> +
>> +	for (i = 0; i < num_fields; i++) {
>> +		struct mux_control *mux = &mux_chip->mux[i];
>> +		s32 idle_state = MUX_IDLE_AS_IS;
>> +		u32 reg, mask, value;
>> +
>> +		ret = of_property_read_u32_index(np, "ti,reg-mask-val",
>> +						 3 * i, &reg);
>> +		if (!ret)
>> +			ret = of_property_read_u32_index(np, "ti,reg-mask-val",
>> +							 3 * i + 1, &mask);
>> +		if (!ret)
>> +			ret = of_property_read_u32_index(np, "ti,reg-mask-val",
>> +							 3 * i + 2, &value);
>> +		if (ret < 0) {
>> +			dev_err(dev, "field %d: failed to read ti,reg-mask-val property: %d\n",
>> +				i, ret);
>> +			return ret;
>> +		}
>> +
>> +		/* Validate that value bits are within mask */
>> +		if (value & ~mask) {
> 
> This is broken and only works as expected if "mask" is a bitfield
> based at the lsb. You should keep the limitation from the mmio
> driver that "mask" has to be a proper field (without holes)
> and you should shift things such that "value" is what will be
> written to that field, and not what will be written to the whole
> register.
> 
>> +			dev_err(dev, "field %d: value 0x%x has bits outside mask 0x%x\n",
>> +				i, value, mask);
>> +			return -EINVAL;
>> +		}
> 
> I think you should also check that the mask does not clobber
> the enable bit.
> 
>> +
>> +		fields[i].regmap = regmap;
>> +		fields[i].reg = reg;
>> +		fields[i].mask = mask;
>> +		fields[i].value = value;
>> +
>> +		/* This driver supports binary mux (2 states: 0 and active) */
>> +		mux->states = 2;
> 
> This is weird. You apparently only have one leg on these muxes,
> and need to turn them on/off with a MUX_IDLE_DISCONNECT idle
> state instead of abusing an extra state that can never be used
> as an actual valid state.
> 
> I.e. these things are not really muxes at all, they are more
> like gates, methinks.
> 
> And all this indicate that the idle state handling below is
> completely bogus. The only sane idle-state with the current
> patch is zero. So, why require the user to fill that in?
> Why not force it instead? But see above, the idle state
> should not be forced to zero but to MUX_IDLE_DISCONNECT and
> state zero should be the only state and the state that "opens
> the gate" when selected.
>

I will work on the idle state improvements and masks usage, thanks again.


> With all that said, I worry about what happens if you write
> other values in the reg-field? Since you have added this as
> a mux driver when you really have implemented gates, I have
> this feeling that the hw spec calls these registers muxes
> and that they can be used to wire vastly different things
> together. If so, what if you need some of these other values
> in the register? I don't know where to look and have not gone
> trawling the TI site for details, do you perhaps have some
> reference for how these registers work?
> 

The TRM for this IP block is having better block diagrams. I can't paste
them here but below is the github link that shows ASCII equivalent block
diagram for event-mux-router IP block, this is the best I could get.

https://github.com/lucifer-9852/linux/commit/aed1746e411a9f0ad7824f4e4a7121e6f56a4ef2

For detailed view of IP you can refer Section 10.2 and 10.2.1 of TRM
https://www.ti.com/lit/pdf/sprujb4

So, from the above diagrams you can see that it is indeed a mux block
instead of a gate, why ? Because there are mux registers corresponding
to each output lines, and they do the job of selection lines in the mux.
Here, the mux type is not many-to-one but instead many-to-few. The mux
register holds the index value of input lines, which are fixed in count.

BR,
Rahul

> Cheers,
> Peter
> 
>> +
>> +		of_property_read_u32_index(np, "idle-states", i,
>> +					   (u32 *)&idle_state);
>> +		if (idle_state != MUX_IDLE_AS_IS) {
>> +			if (idle_state < 0 || idle_state >= mux->states) {
>> +				dev_err(dev, "field: %d: out of range idle state %d\n",
>> +					i, idle_state);
>> +				return -EINVAL;
>> +			}
>> +
>> +			mux->idle_state = idle_state;
>> +		}
>> +	}
>> +
>> +	mux_chip->ops = &mux_ti_k3_event_ops;
>> +
>> +	return devm_mux_chip_register(dev, mux_chip);
>> +}
>> +
>> +static const struct of_device_id mux_ti_k3_event_dt_ids[] = {
>> +	{ .compatible = "ti,am62l-event-mux-router", },
>> +	{ /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, mux_ti_k3_event_dt_ids);
>> +
>> +static struct platform_driver mux_ti_k3_event_driver = {
>> +	.driver = {
>> +		.name = "ti-k3-event-mux",
>> +		.of_match_table	= mux_ti_k3_event_dt_ids,
>> +		.pm = &mux_ti_k3_event_pm_ops,
>> +	},
>> +	.probe = mux_ti_k3_event_probe,
>> +};
>> +module_platform_driver(mux_ti_k3_event_driver);
>> +
>> +MODULE_DESCRIPTION("TI K3 Bit-field Controlled Event Multiplexer driver");
>> +MODULE_AUTHOR("Rahul Sharma <r-sharma3@ti.com>");
>> +MODULE_LICENSE("GPL");
>> -- 
>> 2.34.1
>> 
>