> -----Original Message----- > From: Peter Rosin <p...@axentia.se> > Sent: Monday, February 18, 2019 8:28 AM > To: Pankaj Bansal <pankaj.ban...@nxp.com>; Leo Li <leoyang...@nxp.com>; > linux-kernel@vger.kernel.org; Philipp Zabel <p.za...@pengutronix.de> > Subject: Re: [PATCH] drivers: mux: Generic register bitfield-based > multiplexer driver > > On 2019-02-18 11:20, Pankaj Bansal wrote: > > Hi Peter, > > > >> -----Original Message----- > >> From: Peter Rosin [mailto:p...@axentia.se] > >> Sent: Monday, 18 February, 2019 03:17 PM > >> To: Pankaj Bansal <pankaj.ban...@nxp.com>; Leo Li > >> <leoyang...@nxp.com>; linux-kernel@vger.kernel.org; Philipp Zabel > >> <p.za...@pengutronix.de> > >> Subject: Re: [PATCH] drivers: mux: Generic register bitfield-based > >> multiplexer driver > >> > >> Hi! > >> > >> On 2019-02-18 06:40, Pankaj Bansal wrote: > >>> Generic register bitfield-based multiplexer driver that controls the > >>> multiplexer producer defined under a parent node. > >>> The driver corresponding to parent node provides register read/write > >>> capabilities. > >> > >> This driver is just a rename of drivers/mux/mmio.c with a one-liner > >> on top. And there's a license change too. That's obnoxious. Please > >> keep this as GPL v2. Not that I /think/ Philipp nor Pengutronix cares > deeply, but what do I know? > >> Changing the license as you copy the code is simply not all right. > > > > My Apologies. I will fix it as I send V2. > > > >> > >> Anyway, I would prefer if you could extend drivers/mux/mmio.c to > >> support both compatibles, and using the compatible to select if > >> > >> regmap = syscon_node_to_regmap(np->parent); > >> > >> or > >> > >> regmap = dev_get_regmap(dev->parent, NULL); > >> > >> is called to get to the desired regmap. > > > > This can be done. The name mmio.c however suggests that mux is > controlled by a Memory mapped device. > > IMO, if the generic regmap API is to be added to it, the name needs to > changed. Any suggestions ? > > Just keep the driver name as is, there is no shortage of drivers supporting > more than one thing...
You are right that a lot of drivers support multiple functions. But the problem here is that the current name mmio is only a subset of what the updated driver will handle, which can create confusion. Regards, Leo > > Cheers, > Peter > > >> > >> Philipp, you don't object to extending the mmio driver, right? > >> > >> Or are there more differences that I failed to notice? > > > > Nope, it's the only difference. > > > >> > >> Cheers, > >> Peter > >> > >>> > >>> Signed-off-by: Pankaj Bansal <pankaj.ban...@nxp.com> > >>> --- > >>> > >>> Notes: > >>> Dependencies: > >>> - > >>> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fp > >>> at > >>> > >> > chwork.ozlabs.org%2Fpatch%2F1043790%2F&data=02%7C01%7Cpankaj. > b > >> ansa > >>> > >> > l%40nxp.com%7C3b8a1e8520ce4345105108d695861210%7C686ea1d3bc2b4c6 > f > >> a92cd > >>> > >> > 99c5c301635%7C0%7C0%7C636860800396857042&sdata=5e%2BhyasYwf > >> uc%2Fbr > >>> 1u6WKlKybYipz5c4ndBeLZDflHqk%3D&reserved=0 > >>> > >>> drivers/mux/Kconfig | 13 ++++ > >>> drivers/mux/Makefile | 2 + > >>> drivers/mux/regmap.c | 139 > >> +++++++++++++++++++++++++++++++++++++++++ > >>> 3 files changed, 154 insertions(+) > >>> > >>> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig index > >>> 7659d6c5f718..a412d0955258 100644 > >>> --- a/drivers/mux/Kconfig > >>> +++ b/drivers/mux/Kconfig > >>> @@ -58,4 +58,17 @@ config MUX_MMIO > >>> To compile the driver as a module, choose M here: the module will > >>> be called mux-mmio. > >>> > >>> +config MUX_REGMAP > >>> + tristate "Regmap register bitfield-controlled Multiplexer" > >>> + depends on (OF && REGMAP) || COMPILE_TEST > >>> + help > >>> + Regmap register bitfield-controlled Multiplexer controller. > >>> + > >>> + The driver builds multiplexer controllers for bitfields in a regmap > >>> + device register. For N bit wide bitfields, there will be 2^N possible > >>> + multiplexer states. > >>> + > >>> + To compile the driver as a module, choose M here: the module will > >>> + be called regmap-mmio. > >>> + > >>> endmenu > >>> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile index > >>> 6e9fa47daf56..ae1bafac0cbd 100644 > >>> --- a/drivers/mux/Makefile > >>> +++ b/drivers/mux/Makefile > >>> @@ -8,9 +8,11 @@ mux-adg792a-objs := adg792a.o > >>> mux-adgs1408-objs := adgs1408.o > >>> mux-gpio-objs := gpio.o > >>> mux-mmio-objs := mmio.o > >>> +mux-regmap-objs := regmap.o > >>> > >>> obj-$(CONFIG_MULTIPLEXER) += mux-core.o > >>> obj-$(CONFIG_MUX_ADG792A) += mux-adg792a.o > >>> obj-$(CONFIG_MUX_ADGS1408) += mux-adgs1408.o > >>> obj-$(CONFIG_MUX_GPIO) += mux-gpio.o > >>> obj-$(CONFIG_MUX_MMIO) += mux-mmio.o > >>> +obj-$(CONFIG_MUX_REGMAP) += mux-regmap.o > >>> diff --git a/drivers/mux/regmap.c b/drivers/mux/regmap.c new file > >>> mode > >>> 100644 index 000000000000..c2156302929a > >>> --- /dev/null > >>> +++ b/drivers/mux/regmap.c > >>> @@ -0,0 +1,139 @@ > >>> +// SPDX-License-Identifier: GPL-2.0+ > >>> +/* > >>> + * Regmap register bitfield-controlled multiplexer driver > >>> + * > >>> + * Based on drivers/mux/mmio.c > >>> + * > >>> + * Copyright 2019 NXP > >>> + */ > >>> + > >>> +#include <linux/bitops.h> > >>> +#include <linux/err.h> > >>> +#include <linux/module.h> > >>> +#include <linux/mux/driver.h> > >>> +#include <linux/of_platform.h> > >>> +#include <linux/platform_device.h> > >>> +#include <linux/property.h> > >>> +#include <linux/regmap.h> > >>> + > >>> +static int mux_regmap_set(struct mux_control *mux, int state) { > >>> + struct regmap_field **fields = mux_chip_priv(mux->chip); > >>> + > >>> + return regmap_field_write(fields[mux_control_get_index(mux)], > >>> +state); } > >>> + > >>> +static const struct mux_control_ops mux_regmap_ops = { > >>> + .set = mux_regmap_set, > >>> +}; > >>> + > >>> +static const struct of_device_id mux_regmap_dt_ids[] = { > >>> + { .compatible = "reg-mux", }, > >>> + { /* sentinel */ } > >>> +}; > >>> +MODULE_DEVICE_TABLE(of, mux_regmap_dt_ids); > >>> + > >>> +static int mux_regmap_probe(struct platform_device *pdev) { > >>> + struct device *dev = &pdev->dev; > >>> + struct device_node *np = dev->of_node; > >>> + struct regmap_field **fields; > >>> + struct mux_chip *mux_chip; > >>> + struct regmap *regmap; > >>> + int num_fields; > >>> + int ret; > >>> + int i; > >>> + > >>> + regmap = dev_get_regmap(dev->parent, NULL); > >>> + if (IS_ERR(regmap)) { > >>> + ret = PTR_ERR(regmap); > >>> + dev_err(dev, "failed to get regmap: %d\n", ret); > >>> + return ret; > >>> + } > >>> + > >>> + ret = of_property_count_u32_elems(np, "mux-reg-masks"); > >>> + if (ret == 0 || ret % 2) > >>> + ret = -EINVAL; > >>> + if (ret < 0) { > >>> + dev_err(dev, "mux-reg-masks property missing or > invalid: %d\n", > >>> + ret); > >>> + return ret; > >>> + } > >>> + num_fields = ret / 2; > >>> + > >>> + 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); > >>> + > >>> + for (i = 0; i < num_fields; i++) { > >>> + struct mux_control *mux = &mux_chip->mux[i]; > >>> + struct reg_field field; > >>> + s32 idle_state = MUX_IDLE_AS_IS; > >>> + u32 reg, mask; > >>> + int bits; > >>> + > >>> + ret = of_property_read_u32_index(np, "mux-reg-masks", > >>> + 2 * i, ®); > >>> + if (!ret) > >>> + ret = of_property_read_u32_index(np, "mux-reg- > >> masks", > >>> + 2 * i + 1, &mask); > >>> + if (ret < 0) { > >>> + dev_err(dev, "bitfield %d: failed to read mux-reg- > masks > >> property: %d\n", > >>> + i, ret); > >>> + return ret; > >>> + } > >>> + > >>> + field.reg = reg; > >>> + field.msb = fls(mask) - 1; > >>> + field.lsb = ffs(mask) - 1; > >>> + > >>> + if (mask != GENMASK(field.msb, field.lsb)) { > >>> + dev_err(dev, "bitfield %d: invalid mask 0x%x\n", > >>> + i, mask); > >>> + return -EINVAL; > >>> + } > >>> + > >>> + fields[i] = devm_regmap_field_alloc(dev, regmap, field); > >>> + if (IS_ERR(fields[i])) { > >>> + ret = PTR_ERR(fields[i]); > >>> + dev_err(dev, "bitfield %d: failed allocate: %d\n", > >>> + i, ret); > >>> + return ret; > >>> + } > >>> + > >>> + bits = 1 + field.msb - field.lsb; > >>> + mux->states = 1 << bits; > >>> + > >>> + 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, "bitfield: %d: out of range idle > >> state %d\n", > >>> + i, idle_state); > >>> + return -EINVAL; > >>> + } > >>> + > >>> + mux->idle_state = idle_state; > >>> + } > >>> + } > >>> + > >>> + mux_chip->ops = &mux_regmap_ops; > >>> + > >>> + return devm_mux_chip_register(dev, mux_chip); } > >>> + > >>> +static struct platform_driver mux_regmap_driver = { > >>> + .driver = { > >>> + .name = "regmap-mux", > >>> + .of_match_table = > of_match_ptr(mux_regmap_dt_ids), > >>> + }, > >>> + .probe = mux_regmap_probe, > >>> +}; > >>> +module_platform_driver(mux_regmap_driver); > >>> + > >>> +MODULE_DESCRIPTION("Regmap register bitfield-controlled > multiplexer > >>> +driver"); MODULE_AUTHOR("Pankaj Bansal > <pankaj.ban...@nxp.com>"); > >>> +MODULE_LICENSE("GPL"); > >>> > >