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.
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.
Philipp, you don't object to extending the mmio driver, right?
Or are there more differences that I failed to notice?
Cheers,
Peter
>
> Signed-off-by: Pankaj Bansal <[email protected]>
> ---
>
> Notes:
> Dependencies:
> - https://patchwork.ozlabs.org/patch/1043790/
>
> 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 <[email protected]>");
> +MODULE_LICENSE("GPL");
>
Hi Peter,
> -----Original Message-----
> From: Peter Rosin [mailto:[email protected]]
> Sent: Monday, 18 February, 2019 03:17 PM
> To: Pankaj Bansal <[email protected]>; Leo Li <[email protected]>;
> [email protected]; Philipp Zabel <[email protected]>
> 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 ?
>
> 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 <[email protected]>
> > ---
> >
> > Notes:
> > Dependencies:
> > -
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpat
> >
> chwork.ozlabs.org%2Fpatch%2F1043790%2F&data=02%7C01%7Cpankaj.b
> ansa
> >
> l%40nxp.com%7C3b8a1e8520ce4345105108d695861210%7C686ea1d3bc2b4c6f
> 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 <[email protected]>");
> > +MODULE_LICENSE("GPL");
> >
On 2019-02-18 11:20, Pankaj Bansal wrote:
> Hi Peter,
>
>> -----Original Message-----
>> From: Peter Rosin [mailto:[email protected]]
>> Sent: Monday, 18 February, 2019 03:17 PM
>> To: Pankaj Bansal <[email protected]>; Leo Li <[email protected]>;
>> [email protected]; Philipp Zabel <[email protected]>
>> 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...
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 <[email protected]>
>>> ---
>>>
>>> Notes:
>>> Dependencies:
>>> -
>>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpat
>>>
>> chwork.ozlabs.org%2Fpatch%2F1043790%2F&data=02%7C01%7Cpankaj.b
>> ansa
>>>
>> l%40nxp.com%7C3b8a1e8520ce4345105108d695861210%7C686ea1d3bc2b4c6f
>> 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 <[email protected]>");
>>> +MODULE_LICENSE("GPL");
>>>
>
> -----Original Message-----
> From: Peter Rosin <[email protected]>
> Sent: Monday, February 18, 2019 8:28 AM
> To: Pankaj Bansal <[email protected]>; Leo Li <[email protected]>;
> [email protected]; Philipp Zabel <[email protected]>
> 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:[email protected]]
> >> Sent: Monday, 18 February, 2019 03:17 PM
> >> To: Pankaj Bansal <[email protected]>; Leo Li
> >> <[email protected]>; [email protected]; Philipp Zabel
> >> <[email protected]>
> >> 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 <[email protected]>
> >>> ---
> >>>
> >>> 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
> <[email protected]>");
> >>> +MODULE_LICENSE("GPL");
> >>>
> >
On 2019-02-18 22:07, Leo Li wrote:
> From: Peter Rosin <[email protected]>
>> On 2019-02-18 11:20, Pankaj Bansal wrote:
>>> From: Peter Rosin [mailto:[email protected]]
>>>> 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.
I refuse the duplication. This new driver is doing the exact same
thing (-ish) as the old one. Having the same code in two places is just
a recipe for future divergence when everyone have forgotten that
there are two nearly identical drivers that both need patching. Stating
this in a comment somewhere in the drivers will not help all that much
in my experience. The comment will be missing from the context in some
seemingly trivial patch, and there you go. There *will* be weed down
the line, if duplication is allowed.
I can agree that mux-regmap.c and CONFIG_MUX_REGMAP would have been
better names, but mux-mmio is already there. So it is what it is. But
if you can convince me that changing the name will not cause any
trouble anywhere for any existing mux-mmio users, I suppose we can
do a rename. But I bet there will be some nasty corner cases and
odd use cases, so you will have to present strong arguments.
Just update the Kconfig to document the dual nature and remove the
MFD_SYSCON dependency. I suppose you also need to handle the possibly
missing syscon in the .c file, details, details. But something like
this perhaps:
config MUX_MMIO
tristate "MMIO/Regmap register bitfield-controlled Multiplexer"
depends on OF || COMPILE_TEST
help
MMIO/Regmap register bitfield-controlled Multiplexer controller.
The driver builds multiplexer controllers for bitfields in either
a syscon register or a driver regmap 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 mux-mmio.
Cheers,
Peter
> -----Original Message-----
> From: Peter Rosin <[email protected]>
> Sent: Monday, February 18, 2019 5:39 PM
> To: Leo Li <[email protected]>; Pankaj Bansal <[email protected]>;
> [email protected]; Philipp Zabel <[email protected]>
> Subject: Re: [PATCH] drivers: mux: Generic register bitfield-based
> multiplexer driver
>
> On 2019-02-18 22:07, Leo Li wrote:
> > From: Peter Rosin <[email protected]>
> >> On 2019-02-18 11:20, Pankaj Bansal wrote:
> >>> From: Peter Rosin [mailto:[email protected]]
> >>>> 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.
>
> I refuse the duplication. This new driver is doing the exact same thing (-ish)
> as the old one. Having the same code in two places is just a recipe for future
> divergence when everyone have forgotten that there are two nearly
> identical drivers that both need patching. Stating this in a comment
> somewhere in the drivers will not help all that much in my experience. The
> comment will be missing from the context in some seemingly trivial patch,
> and there you go. There *will* be weed down the line, if duplication is
> allowed.
I agree that we should avoid the duplication.
>
> I can agree that mux-regmap.c and CONFIG_MUX_REGMAP would have
> been better names, but mux-mmio is already there. So it is what it is. But if
> you can convince me that changing the name will not cause any trouble
> anywhere for any existing mux-mmio users, I suppose we can do a rename.
> But I bet there will be some nasty corner cases and odd use cases, so you will
> have to present strong arguments.
I don't think that it is hard to maintain the backward compatibility with the rename. The updated driver can keep handling the "mmio-mux" device tree compatible string. And we can also have MUX_MMIO selects the new MUX_REGMAP if we want to keep the compatibility with old kernel config file.
>
> Just update the Kconfig to document the dual nature and remove the
> MFD_SYSCON dependency. I suppose you also need to handle the possibly
> missing syscon in the .c file, details, details. But something like this perhaps:
>
> config MUX_MMIO
> tristate "MMIO/Regmap register bitfield-controlled Multiplexer"
> depends on OF || COMPILE_TEST
> help
> MMIO/Regmap register bitfield-controlled Multiplexer controller.
>
> The driver builds multiplexer controllers for bitfields in either
> a syscon register or a driver regmap 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 mux-mmio.
>
> Cheers,
> Peter