* [RFC PATCH 0/2] Extend mmio-mux driver to configure mux with
@ 2025-02-27 20:22 Chintan Vankar
2025-02-27 20:22 ` [RFC PATCH 1/2] devicetree: bindings: mux: reg-mux: Update bindings for reg-mux for new property Chintan Vankar
2025-02-27 20:22 ` [RFC PATCH 2/2] mux: mmio: Extend mmio-mux driver to configure mux with new DT property Chintan Vankar
0 siblings, 2 replies; 10+ messages in thread
From: Chintan Vankar @ 2025-02-27 20:22 UTC (permalink / raw)
To: Conor Dooley, Krzysztof Kozlowski, Rob Herring, Peter Rosin, tglx,
gregkh, vigneshr, nm, s-vadapalli, danishanwar
Cc: linux-kernel, devicetree, c-vankar
This series extends mmio-mux driver's capability to configure driver in
with extended property.
In current driver implementation, driver is parsing register's offset,
mask and value from two different device tree property which makes it
complex to specify a specific register or set of registers. Introducing
mux-reg-masks-states will make it easier to specify the same values for
particular register or set of registers.
This series is based on linux next tagged next-20250227.
Chintan Vankar (2):
devicetree: bindings: mux: reg-mux: Update bindings for reg-mux for
new property
mux: mmio: Extend mmio-mux driver to configure mux with new DT
property
.../devicetree/bindings/mux/reg-mux.yaml | 29 +++-
drivers/mux/mmio.c | 148 +++++++++++++++---
2 files changed, 149 insertions(+), 28 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [RFC PATCH 1/2] devicetree: bindings: mux: reg-mux: Update bindings for reg-mux for new property
2025-02-27 20:22 [RFC PATCH 0/2] Extend mmio-mux driver to configure mux with Chintan Vankar
@ 2025-02-27 20:22 ` Chintan Vankar
2025-02-27 21:26 ` Andrew Davis
2025-02-27 20:22 ` [RFC PATCH 2/2] mux: mmio: Extend mmio-mux driver to configure mux with new DT property Chintan Vankar
1 sibling, 1 reply; 10+ messages in thread
From: Chintan Vankar @ 2025-02-27 20:22 UTC (permalink / raw)
To: Conor Dooley, Krzysztof Kozlowski, Rob Herring, Peter Rosin, tglx,
gregkh, vigneshr, nm, s-vadapalli, danishanwar
Cc: linux-kernel, devicetree, c-vankar
DT-binding of reg-mux is defined in such a way that one need to provide
register offset and mask in a "mux-reg-masks" property and corresponding
register value in "idle-states" property. This constraint forces to define
these values in such a way that "mux-reg-masks" and "idle-states" must be
in sync with each other. This implementation would be more complex if
specific register or set of registers need to be configured which has
large memory space. Introduce a new property "mux-reg-masks-state" which
allow to specify offset, mask and value as a tuple in a single property.
Signed-off-by: Chintan Vankar <c-vankar@ti.com>
---
.../devicetree/bindings/mux/reg-mux.yaml | 29 +++++++++++++++++--
1 file changed, 27 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/mux/reg-mux.yaml b/Documentation/devicetree/bindings/mux/reg-mux.yaml
index dc4be092fc2f..a73c5efcf860 100644
--- a/Documentation/devicetree/bindings/mux/reg-mux.yaml
+++ b/Documentation/devicetree/bindings/mux/reg-mux.yaml
@@ -32,11 +32,36 @@ properties:
- description: pre-shifted bitfield mask
description: Each entry pair describes a single mux control.
- idle-states: true
+ idle-states:
+ description: Each entry describes mux register state.
+
+ mux-reg-masks-state:
+ $ref: /schemas/types.yaml#/definitions/uint32-matrix
+ items:
+ items:
+ - description: register offset
+ - description: pre-shifted bitfield mask
+ - description: register value to be set
+ description: This property is an extension of mux-reg-masks which
+ allows specifying register offset, mask and register
+ value to be set in a single property.
+
+allOf:
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - reg-mux
+ - mmio-mux
+ then:
+ properties:
+ mux-reg-masks: true
+ mux-reg-masks-state: true
+ maxItems: 1
required:
- compatible
- - mux-reg-masks
- '#mux-control-cells'
additionalProperties: false
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [RFC PATCH 2/2] mux: mmio: Extend mmio-mux driver to configure mux with new DT property
2025-02-27 20:22 [RFC PATCH 0/2] Extend mmio-mux driver to configure mux with Chintan Vankar
2025-02-27 20:22 ` [RFC PATCH 1/2] devicetree: bindings: mux: reg-mux: Update bindings for reg-mux for new property Chintan Vankar
@ 2025-02-27 20:22 ` Chintan Vankar
2025-02-27 21:39 ` Andrew Davis
1 sibling, 1 reply; 10+ messages in thread
From: Chintan Vankar @ 2025-02-27 20:22 UTC (permalink / raw)
To: Conor Dooley, Krzysztof Kozlowski, Rob Herring, Peter Rosin, tglx,
gregkh, vigneshr, nm, s-vadapalli, danishanwar
Cc: linux-kernel, devicetree, c-vankar
MMIO mux driver is designed to parse "mux-reg-masks" and "idle-states"
property independently to configure mux registers. Drawback of this
approach is, while configuring mux-controller one need to specify every
register of memory space with offset and mask in "mux-reg-masks" and
register state to "idle-states", that would be more complex for devices
with large memory space.
Add support to extend the mmio mux driver to configure a specific register
or set of register in memory space.
Signed-off-by: Chintan Vankar <c-vankar@ti.com>
---
drivers/mux/mmio.c | 148 +++++++++++++++++++++++++++++++++++++--------
1 file changed, 122 insertions(+), 26 deletions(-)
diff --git a/drivers/mux/mmio.c b/drivers/mux/mmio.c
index 30a952c34365..8937d0ea2b11 100644
--- a/drivers/mux/mmio.c
+++ b/drivers/mux/mmio.c
@@ -2,7 +2,7 @@
/*
* MMIO register bitfield-controlled multiplexer driver
*
- * Copyright (C) 2017 Pengutronix, Philipp Zabel <kernel@pengutronix.de>
+ * Copyright (C) 2017-2025 Pengutronix, Philipp Zabel <kernel@pengutronix.de>
*/
#include <linux/bitops.h>
@@ -33,10 +33,84 @@ static const struct of_device_id mux_mmio_dt_ids[] = {
};
MODULE_DEVICE_TABLE(of, mux_mmio_dt_ids);
+static int reg_mux_get_controllers(const struct device_node *np, char *prop_name)
+{
+ int ret;
+
+ ret = of_property_count_u32_elems(np, prop_name);
+ if (ret == 0 || ret % 2)
+ ret = -EINVAL;
+
+ return ret;
+}
+
+static int reg_mux_get_controllers_extended(const struct device_node *np, char *prop_name)
+{
+ int ret;
+
+ ret = of_property_count_u32_elems(np, prop_name);
+ if (ret == 0 || ret % 3)
+ ret = -EINVAL;
+
+ return ret;
+}
+
+static int reg_mux_parse_dt(const struct device_node *np, bool *mux_reg_masks_state,
+ int *num_fields)
+{
+ int ret;
+
+ if (*mux_reg_masks_state) {
+ ret = reg_mux_get_controllers_extended(np, "mux-reg-masks-state");
+ if (ret < 0)
+ return ret;
+ *num_fields = ret / 3;
+ } else {
+ ret = reg_mux_get_controllers(np, "mux-reg-masks");
+ if (ret < 0)
+ return ret;
+ *num_fields = ret / 2;
+ }
+ return ret;
+}
+
+static int mux_reg_set_parameters(const struct device_node *np, char *prop_name, u32 *reg,
+ u32 *mask, int index)
+{
+ int ret;
+
+ ret = of_property_read_u32_index(np, prop_name,
+ 2 * index, reg);
+ if (!ret)
+ ret = of_property_read_u32_index(np, prop_name,
+ 2 * index + 1, mask);
+
+ return ret;
+}
+
+static int mux_reg_set_parameters_extended(const struct device_node *np, char *prop_name, u32 *reg,
+ u32 *mask, u32 *state, int index)
+{
+ int ret;
+
+ ret = of_property_read_u32_index(np, prop_name,
+ 3 * index, reg);
+ if (!ret) {
+ ret = of_property_read_u32_index(np, prop_name,
+ 3 * index + 1, mask);
+ if (!ret)
+ ret = of_property_read_u32_index(np, prop_name,
+ 3 * index + 2, state);
+ }
+
+ return ret;
+}
+
static int mux_mmio_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct device_node *np = dev->of_node;
+ bool mux_reg_masks_state = false;
struct regmap_field **fields;
struct mux_chip *mux_chip;
struct regmap *regmap;
@@ -59,15 +133,19 @@ static int mux_mmio_probe(struct platform_device *pdev)
return dev_err_probe(dev, PTR_ERR(regmap),
"failed to get regmap\n");
- ret = of_property_count_u32_elems(np, "mux-reg-masks");
- if (ret == 0 || ret % 2)
- ret = -EINVAL;
+ if (of_property_present(np, "mux-reg-masks-state"))
+ mux_reg_masks_state = true;
+
+ ret = reg_mux_parse_dt(np, &mux_reg_masks_state, &num_fields);
if (ret < 0) {
- dev_err(dev, "mux-reg-masks property missing or invalid: %d\n",
- ret);
+ if (mux_reg_masks_state)
+ dev_err(dev, "mux-reg-masks-state property missing or invalid: %d\n",
+ ret);
+ else
+ 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));
@@ -79,19 +157,25 @@ static int mux_mmio_probe(struct platform_device *pdev)
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;
+ s32 state, 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;
+ if (!mux_reg_masks_state) {
+ ret = mux_reg_set_parameters(np, "mux-reg-masks", ®, &mask, i);
+ if (ret < 0) {
+ dev_err(dev, "bitfield %d: failed to read mux-reg-masks property: %d\n",
+ i, ret);
+ return ret;
+ }
+ } else {
+ ret = mux_reg_set_parameters_extended(np, "mux-reg-masks-state", ®,
+ &mask, &state, i);
+ if (ret < 0) {
+ dev_err(dev, "bitfield %d: failed to read custom-states property: %d\n",
+ i, ret);
+ return ret;
+ }
}
field.reg = reg;
@@ -115,16 +199,28 @@ static int mux_mmio_probe(struct platform_device *pdev)
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;
+ if (!mux_reg_masks_state) {
+ 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;
+ }
+ } else {
+ if (state != MUX_IDLE_AS_IS) {
+ if (state < 0 || state >= mux->states) {
+ dev_err(dev, "bitfield: %d: out of range idle state %d\n",
+ i, state);
+ return -EINVAL;
+ }
+
+ mux->idle_state = state;
}
-
- mux->idle_state = idle_state;
}
}
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFC PATCH 1/2] devicetree: bindings: mux: reg-mux: Update bindings for reg-mux for new property
2025-02-27 20:22 ` [RFC PATCH 1/2] devicetree: bindings: mux: reg-mux: Update bindings for reg-mux for new property Chintan Vankar
@ 2025-02-27 21:26 ` Andrew Davis
2025-02-28 18:52 ` Conor Dooley
0 siblings, 1 reply; 10+ messages in thread
From: Andrew Davis @ 2025-02-27 21:26 UTC (permalink / raw)
To: Chintan Vankar, Conor Dooley, Krzysztof Kozlowski, Rob Herring,
Peter Rosin, tglx, gregkh, vigneshr, nm, s-vadapalli, danishanwar
Cc: linux-kernel, devicetree
On 2/27/25 2:22 PM, Chintan Vankar wrote:
> DT-binding of reg-mux is defined in such a way that one need to provide
> register offset and mask in a "mux-reg-masks" property and corresponding
> register value in "idle-states" property. This constraint forces to define
> these values in such a way that "mux-reg-masks" and "idle-states" must be
> in sync with each other. This implementation would be more complex if
> specific register or set of registers need to be configured which has
> large memory space. Introduce a new property "mux-reg-masks-state" which
> allow to specify offset, mask and value as a tuple in a single property.
>
> Signed-off-by: Chintan Vankar <c-vankar@ti.com>
> ---
> .../devicetree/bindings/mux/reg-mux.yaml | 29 +++++++++++++++++--
> 1 file changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mux/reg-mux.yaml b/Documentation/devicetree/bindings/mux/reg-mux.yaml
> index dc4be092fc2f..a73c5efcf860 100644
> --- a/Documentation/devicetree/bindings/mux/reg-mux.yaml
> +++ b/Documentation/devicetree/bindings/mux/reg-mux.yaml
> @@ -32,11 +32,36 @@ properties:
> - description: pre-shifted bitfield mask
> description: Each entry pair describes a single mux control.
>
> - idle-states: true
> + idle-states:
> + description: Each entry describes mux register state.
> +
> + mux-reg-masks-state:
> + $ref: /schemas/types.yaml#/definitions/uint32-matrix
> + items:
> + items:
> + - description: register offset
> + - description: pre-shifted bitfield mask
> + - description: register value to be set
> + description: This property is an extension of mux-reg-masks which
> + allows specifying register offset, mask and register
> + value to be set in a single property.
> +
> +allOf:
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - reg-mux
> + - mmio-mux
These are the only two possible compatibles, is this "if" check needed?
> + then:
> + properties:
> + mux-reg-masks: true
> + mux-reg-masks-state: true
You need one, but cannot have both, right? There should be some
way to describe that.
Also an example added below would be good.
Andrew
> + maxItems: 1
>
> required:
> - compatible
> - - mux-reg-masks
> - '#mux-control-cells'
>
> additionalProperties: false
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH 2/2] mux: mmio: Extend mmio-mux driver to configure mux with new DT property
2025-02-27 20:22 ` [RFC PATCH 2/2] mux: mmio: Extend mmio-mux driver to configure mux with new DT property Chintan Vankar
@ 2025-02-27 21:39 ` Andrew Davis
2025-03-04 5:16 ` Vankar, Chintan
0 siblings, 1 reply; 10+ messages in thread
From: Andrew Davis @ 2025-02-27 21:39 UTC (permalink / raw)
To: Chintan Vankar, Conor Dooley, Krzysztof Kozlowski, Rob Herring,
Peter Rosin, tglx, gregkh, vigneshr, nm, s-vadapalli, danishanwar
Cc: linux-kernel, devicetree
On 2/27/25 2:22 PM, Chintan Vankar wrote:
> MMIO mux driver is designed to parse "mux-reg-masks" and "idle-states"
> property independently to configure mux registers. Drawback of this
> approach is, while configuring mux-controller one need to specify every
> register of memory space with offset and mask in "mux-reg-masks" and
> register state to "idle-states", that would be more complex for devices
> with large memory space.
>
> Add support to extend the mmio mux driver to configure a specific register
> or set of register in memory space.
>
> Signed-off-by: Chintan Vankar <c-vankar@ti.com>
> ---
> drivers/mux/mmio.c | 148 +++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 122 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/mux/mmio.c b/drivers/mux/mmio.c
> index 30a952c34365..8937d0ea2b11 100644
> --- a/drivers/mux/mmio.c
> +++ b/drivers/mux/mmio.c
> @@ -2,7 +2,7 @@
> /*
> * MMIO register bitfield-controlled multiplexer driver
> *
> - * Copyright (C) 2017 Pengutronix, Philipp Zabel <kernel@pengutronix.de>
> + * Copyright (C) 2017-2025 Pengutronix, Philipp Zabel <kernel@pengutronix.de>
> */
>
> #include <linux/bitops.h>
> @@ -33,10 +33,84 @@ static const struct of_device_id mux_mmio_dt_ids[] = {
> };
> MODULE_DEVICE_TABLE(of, mux_mmio_dt_ids);
>
> +static int reg_mux_get_controllers(const struct device_node *np, char *prop_name)
> +{
> + int ret;
> +
> + ret = of_property_count_u32_elems(np, prop_name);
> + if (ret == 0 || ret % 2)
> + ret = -EINVAL;
> +
> + return ret;
> +}
> +
> +static int reg_mux_get_controllers_extended(const struct device_node *np, char *prop_name)
> +{
> + int ret;
> +
> + ret = of_property_count_u32_elems(np, prop_name);
> + if (ret == 0 || ret % 3)
> + ret = -EINVAL;
> +
> + return ret;
> +}
> +
> +static int reg_mux_parse_dt(const struct device_node *np, bool *mux_reg_masks_state,
> + int *num_fields)
> +{
> + int ret;
> +
> + if (*mux_reg_masks_state) {
> + ret = reg_mux_get_controllers_extended(np, "mux-reg-masks-state");
> + if (ret < 0)
> + return ret;
> + *num_fields = ret / 3;
> + } else {
> + ret = reg_mux_get_controllers(np, "mux-reg-masks");
> + if (ret < 0)
> + return ret;
> + *num_fields = ret / 2;
> + }
> + return ret;
> +}
> +
> +static int mux_reg_set_parameters(const struct device_node *np, char *prop_name, u32 *reg,
> + u32 *mask, int index)
> +{
> + int ret;
> +
> + ret = of_property_read_u32_index(np, prop_name,
> + 2 * index, reg);
> + if (!ret)
> + ret = of_property_read_u32_index(np, prop_name,
> + 2 * index + 1, mask);
> +
> + return ret;
> +}
> +
> +static int mux_reg_set_parameters_extended(const struct device_node *np, char *prop_name, u32 *reg,
> + u32 *mask, u32 *state, int index)
> +{
> + int ret;
> +
> + ret = of_property_read_u32_index(np, prop_name,
> + 3 * index, reg);
This is some odd line wrapping, why newline at 55 chars here?
You can go to 80 or 100 if it is readable.
> + if (!ret) {
Just return early, no need for this MISRA-like "single return" junk.
> + ret = of_property_read_u32_index(np, prop_name,
> + 3 * index + 1, mask);
> + if (!ret)
> + ret = of_property_read_u32_index(np, prop_name,
> + 3 * index + 2, state);
> + }
> +
> + return ret;
> +}
> +
> static int mux_mmio_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> struct device_node *np = dev->of_node;
> + bool mux_reg_masks_state = false;
> struct regmap_field **fields;
> struct mux_chip *mux_chip;
> struct regmap *regmap;
> @@ -59,15 +133,19 @@ static int mux_mmio_probe(struct platform_device *pdev)
> return dev_err_probe(dev, PTR_ERR(regmap),
> "failed to get regmap\n");
>
> - ret = of_property_count_u32_elems(np, "mux-reg-masks");
> - if (ret == 0 || ret % 2)
> - ret = -EINVAL;
> + if (of_property_present(np, "mux-reg-masks-state"))
> + mux_reg_masks_state = true;
> +
> + ret = reg_mux_parse_dt(np, &mux_reg_masks_state, &num_fields);
Why are you passing this bool by pointer? You don't modify it in the function..
> if (ret < 0) {
> - dev_err(dev, "mux-reg-masks property missing or invalid: %d\n",
> - ret);
> + if (mux_reg_masks_state)
> + dev_err(dev, "mux-reg-masks-state property missing or invalid: %d\n",
> + ret);
> + else
> + 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));
> @@ -79,19 +157,25 @@ static int mux_mmio_probe(struct platform_device *pdev)
> 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;
> + s32 state, 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;
> + if (!mux_reg_masks_state) {
> + ret = mux_reg_set_parameters(np, "mux-reg-masks", ®, &mask, i);
> + if (ret < 0) {
> + dev_err(dev, "bitfield %d: failed to read mux-reg-masks property: %d\n",
> + i, ret);
> + return ret;
> + }
> + } else {
> + ret = mux_reg_set_parameters_extended(np, "mux-reg-masks-state", ®,
> + &mask, &state, i);
> + if (ret < 0) {
> + dev_err(dev, "bitfield %d: failed to read custom-states property: %d\n",
> + i, ret);
> + return ret;
> + }
> }
>
> field.reg = reg;
> @@ -115,16 +199,28 @@ static int mux_mmio_probe(struct platform_device *pdev)
> 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;
> + if (!mux_reg_masks_state) {
> + of_property_read_u32_index(np, "idle-states", i,
> + (u32 *)&idle_state);
From here down, both branches of this are almost identical, idle_state and
your new "state" var do the same thing, why do you need both?
Andrew
> + 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;
> + }
> + } else {
> + if (state != MUX_IDLE_AS_IS) {
> + if (state < 0 || state >= mux->states) {
> + dev_err(dev, "bitfield: %d: out of range idle state %d\n",
> + i, state);
> + return -EINVAL;
> + }
> +
> + mux->idle_state = state;
> }
> -
> - mux->idle_state = idle_state;
> }
> }
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH 1/2] devicetree: bindings: mux: reg-mux: Update bindings for reg-mux for new property
2025-02-27 21:26 ` Andrew Davis
@ 2025-02-28 18:52 ` Conor Dooley
2025-02-28 21:38 ` Vankar, Chintan
0 siblings, 1 reply; 10+ messages in thread
From: Conor Dooley @ 2025-02-28 18:52 UTC (permalink / raw)
To: Andrew Davis
Cc: Chintan Vankar, Conor Dooley, Krzysztof Kozlowski, Rob Herring,
Peter Rosin, tglx, gregkh, vigneshr, nm, s-vadapalli, danishanwar,
linux-kernel, devicetree
[-- Attachment #1: Type: text/plain, Size: 3552 bytes --]
On Thu, Feb 27, 2025 at 03:26:31PM -0600, Andrew Davis wrote:
> On 2/27/25 2:22 PM, Chintan Vankar wrote:
> > DT-binding of reg-mux is defined in such a way that one need to provide
> > register offset and mask in a "mux-reg-masks" property and corresponding
> > register value in "idle-states" property. This constraint forces to define
> > these values in such a way that "mux-reg-masks" and "idle-states" must be
> > in sync with each other. This implementation would be more complex if
> > specific register or set of registers need to be configured which has
> > large memory space. Introduce a new property "mux-reg-masks-state" which
> > allow to specify offset, mask and value as a tuple in a single property.
> >
> > Signed-off-by: Chintan Vankar <c-vankar@ti.com>
> > ---
> > .../devicetree/bindings/mux/reg-mux.yaml | 29 +++++++++++++++++--
> > 1 file changed, 27 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/mux/reg-mux.yaml b/Documentation/devicetree/bindings/mux/reg-mux.yaml
> > index dc4be092fc2f..a73c5efcf860 100644
> > --- a/Documentation/devicetree/bindings/mux/reg-mux.yaml
> > +++ b/Documentation/devicetree/bindings/mux/reg-mux.yaml
> > @@ -32,11 +32,36 @@ properties:
> > - description: pre-shifted bitfield mask
> > description: Each entry pair describes a single mux control.
> > - idle-states: true
> > + idle-states:
> > + description: Each entry describes mux register state.
> > +
> > + mux-reg-masks-state:
> > + $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > + items:
> > + items:
> > + - description: register offset
> > + - description: pre-shifted bitfield mask
> > + - description: register value to be set
> > + description: This property is an extension of mux-reg-masks which
> > + allows specifying register offset, mask and register
> > + value to be set in a single property.
> > +
> > +allOf:
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - reg-mux
> > + - mmio-mux
>
> These are the only two possible compatibles, is this "if" check needed?
Aye.
> > + then:
> > + properties:
> > + mux-reg-masks: true
> > + mux-reg-masks-state: true
>
> You need one, but cannot have both, right? There should be some
> way to describe that.
>
> Also an example added below would be good.
From the example schema:
# if/then schema can be used to handle conditions on a property affecting
# another property. A typical case is a specific 'compatible' value changes the
# constraints on other properties.
#
# For multiple 'if' schema, group them under an 'allOf'.
#
# If the conditionals become too unweldy, then it may be better to just split
# the binding into separate schema documents.
allOf:
- if:
properties:
compatible:
contains:
const: vendor,soc2-ip
then:
required:
- foo-supply
else:
# If otherwise the property is not allowed:
properties:
foo-supply: false
What's missing from here is making one of the properties required,
so
oneOf:
- required:
- masks
- required:
- masks-state
>
> Andrew
>
> > + maxItems: 1
> > required:
> > - compatible
> > - - mux-reg-masks
> > - '#mux-control-cells'
> > additionalProperties: false
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH 1/2] devicetree: bindings: mux: reg-mux: Update bindings for reg-mux for new property
2025-02-28 18:52 ` Conor Dooley
@ 2025-02-28 21:38 ` Vankar, Chintan
2025-03-03 16:58 ` Conor Dooley
0 siblings, 1 reply; 10+ messages in thread
From: Vankar, Chintan @ 2025-02-28 21:38 UTC (permalink / raw)
To: Conor Dooley, Andrew Davis
Cc: Conor Dooley, Krzysztof Kozlowski, Rob Herring, Peter Rosin, tglx,
gregkh, vigneshr, nm, s-vadapalli, danishanwar, linux-kernel,
devicetree
Hello Conor, Andrew,
On 3/1/2025 12:22 AM, Conor Dooley wrote:
> On Thu, Feb 27, 2025 at 03:26:31PM -0600, Andrew Davis wrote:
>> On 2/27/25 2:22 PM, Chintan Vankar wrote:
>>> DT-binding of reg-mux is defined in such a way that one need to provide
>>> register offset and mask in a "mux-reg-masks" property and corresponding
>>> register value in "idle-states" property. This constraint forces to define
>>> these values in such a way that "mux-reg-masks" and "idle-states" must be
>>> in sync with each other. This implementation would be more complex if
>>> specific register or set of registers need to be configured which has
>>> large memory space. Introduce a new property "mux-reg-masks-state" which
>>> allow to specify offset, mask and value as a tuple in a single property.
>>>
>>> Signed-off-by: Chintan Vankar <c-vankar@ti.com>
>>> ---
>>> .../devicetree/bindings/mux/reg-mux.yaml | 29 +++++++++++++++++--
>>> 1 file changed, 27 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/mux/reg-mux.yaml b/Documentation/devicetree/bindings/mux/reg-mux.yaml
>>> index dc4be092fc2f..a73c5efcf860 100644
>>> --- a/Documentation/devicetree/bindings/mux/reg-mux.yaml
>>> +++ b/Documentation/devicetree/bindings/mux/reg-mux.yaml
>>> @@ -32,11 +32,36 @@ properties:
>>> - description: pre-shifted bitfield mask
>>> description: Each entry pair describes a single mux control.
>>> - idle-states: true
>>> + idle-states:
>>> + description: Each entry describes mux register state.
>>> +
>>> + mux-reg-masks-state:
>>> + $ref: /schemas/types.yaml#/definitions/uint32-matrix
>>> + items:
>>> + items:
>>> + - description: register offset
>>> + - description: pre-shifted bitfield mask
>>> + - description: register value to be set
>>> + description: This property is an extension of mux-reg-masks which
>>> + allows specifying register offset, mask and register
>>> + value to be set in a single property.
>>> +
>>> +allOf:
>>> + - if:
>>> + properties:
>>> + compatible:
>>> + contains:
>>> + enum:
>>> + - reg-mux
>>> + - mmio-mux
>>
>> These are the only two possible compatibles, is this "if" check needed?
>
> Aye.
>
>>> + then:
>>> + properties:
>>> + mux-reg-masks: true
>>> + mux-reg-masks-state: true
>>
>> You need one, but cannot have both, right? There should be some
>> way to describe that.
>>
>> Also an example added below would be good.
>
> From the example schema:
> # if/then schema can be used to handle conditions on a property affecting
> # another property. A typical case is a specific 'compatible' value changes the
> # constraints on other properties.
> #
> # For multiple 'if' schema, group them under an 'allOf'.
> #
> # If the conditionals become too unweldy, then it may be better to just split
> # the binding into separate schema documents.
> allOf:
> - if:
> properties:
> compatible:
> contains:
> const: vendor,soc2-ip
> then:
> required:
> - foo-supply
> else:
> # If otherwise the property is not allowed:
> properties:
> foo-supply: false
>
> What's missing from here is making one of the properties required,
> so
> oneOf:
> - required:
> - masks
> - required:
> - masks-state
>
>>
>> Andrew
Thanks for reviewing this patch.
For the use-case we have following three rules to be followed:
1. "mux-reg-masks" and "mux-reg-masks-state" should be mutually
exclusive.
2. "mux-reg-masks-state" and "idle-states" should also be mutually
exclusive.
3. If "mux-reg-masks" is present then "idle-states" might or might not
be there.
For the above conditions I have tried to write a binding as:
allOf:
- not:
required: [mux-reg-masks, mux-reg-masks-state]
- if:
required: [mux-reg-masks-state]
then:
not:
required: [idle-states]
- if:
required: [mux-reg-masks]
then:
properties:
idle-states:
description: It can be present with mux-reg-masks, but not
required
It is passing dt_binding_check and dtbs_check against correct and
incorrect properties provided in device tree node.
Let me know if you find this correct.
Regards,
Chintan.
>>
>>> + maxItems: 1
>>> required:
>>> - compatible
>>> - - mux-reg-masks
>>> - '#mux-control-cells'
>>> additionalProperties: false
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH 1/2] devicetree: bindings: mux: reg-mux: Update bindings for reg-mux for new property
2025-02-28 21:38 ` Vankar, Chintan
@ 2025-03-03 16:58 ` Conor Dooley
2025-03-03 18:45 ` Vankar, Chintan
0 siblings, 1 reply; 10+ messages in thread
From: Conor Dooley @ 2025-03-03 16:58 UTC (permalink / raw)
To: Vankar, Chintan
Cc: Andrew Davis, Conor Dooley, Krzysztof Kozlowski, Rob Herring,
Peter Rosin, tglx, gregkh, vigneshr, nm, s-vadapalli, danishanwar,
linux-kernel, devicetree
[-- Attachment #1: Type: text/plain, Size: 4996 bytes --]
On Sat, Mar 01, 2025 at 03:08:40AM +0530, Vankar, Chintan wrote:
> Hello Conor, Andrew,
>
> On 3/1/2025 12:22 AM, Conor Dooley wrote:
> > On Thu, Feb 27, 2025 at 03:26:31PM -0600, Andrew Davis wrote:
> > > On 2/27/25 2:22 PM, Chintan Vankar wrote:
> > > > DT-binding of reg-mux is defined in such a way that one need to provide
> > > > register offset and mask in a "mux-reg-masks" property and corresponding
> > > > register value in "idle-states" property. This constraint forces to define
> > > > these values in such a way that "mux-reg-masks" and "idle-states" must be
> > > > in sync with each other. This implementation would be more complex if
> > > > specific register or set of registers need to be configured which has
> > > > large memory space. Introduce a new property "mux-reg-masks-state" which
> > > > allow to specify offset, mask and value as a tuple in a single property.
> > > >
> > > > Signed-off-by: Chintan Vankar <c-vankar@ti.com>
> > > > ---
> > > > .../devicetree/bindings/mux/reg-mux.yaml | 29 +++++++++++++++++--
> > > > 1 file changed, 27 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/mux/reg-mux.yaml b/Documentation/devicetree/bindings/mux/reg-mux.yaml
> > > > index dc4be092fc2f..a73c5efcf860 100644
> > > > --- a/Documentation/devicetree/bindings/mux/reg-mux.yaml
> > > > +++ b/Documentation/devicetree/bindings/mux/reg-mux.yaml
> > > > @@ -32,11 +32,36 @@ properties:
> > > > - description: pre-shifted bitfield mask
> > > > description: Each entry pair describes a single mux control.
> > > > - idle-states: true
> > > > + idle-states:
> > > > + description: Each entry describes mux register state.
> > > > +
> > > > + mux-reg-masks-state:
> > > > + $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > > > + items:
> > > > + items:
> > > > + - description: register offset
> > > > + - description: pre-shifted bitfield mask
> > > > + - description: register value to be set
> > > > + description: This property is an extension of mux-reg-masks which
> > > > + allows specifying register offset, mask and register
> > > > + value to be set in a single property.
> > > > +
> > > > +allOf:
> > > > + - if:
> > > > + properties:
> > > > + compatible:
> > > > + contains:
> > > > + enum:
> > > > + - reg-mux
> > > > + - mmio-mux
> > >
> > > These are the only two possible compatibles, is this "if" check needed?
> >
> > Aye.
> >
> > > > + then:
> > > > + properties:
> > > > + mux-reg-masks: true
> > > > + mux-reg-masks-state: true
> > >
> > > You need one, but cannot have both, right? There should be some
> > > way to describe that.
> > >
> > > Also an example added below would be good.
> >
> > From the example schema:
> > # if/then schema can be used to handle conditions on a property affecting
> > # another property. A typical case is a specific 'compatible' value changes the
> > # constraints on other properties.
> > #
> > # For multiple 'if' schema, group them under an 'allOf'.
> > #
> > # If the conditionals become too unweldy, then it may be better to just split
> > # the binding into separate schema documents.
> > allOf:
> > - if:
> > properties:
> > compatible:
> > contains:
> > const: vendor,soc2-ip
> > then:
> > required:
> > - foo-supply
> > else:
> > # If otherwise the property is not allowed:
> > properties:
> > foo-supply: false
> >
> > What's missing from here is making one of the properties required,
> > so
> > oneOf:
> > - required:
> > - masks
> > - required:
> > - masks-state
> >
> > >
> > > Andrew
>
> Thanks for reviewing this patch.
>
> For the use-case we have following three rules to be followed:
> 1. "mux-reg-masks" and "mux-reg-masks-state" should be mutually
> exclusive.
> 2. "mux-reg-masks-state" and "idle-states" should also be mutually
> exclusive.
> 3. If "mux-reg-masks" is present then "idle-states" might or might not
> be there.
>
> For the above conditions I have tried to write a binding as:
>
> allOf:
> - not:
> required: [mux-reg-masks, mux-reg-masks-state]
>
> - if:
> required: [mux-reg-masks-state]
> then:
> not:
> required: [idle-states]
Why'd you pick two different syntax here?
The normal syntax for mutual exclusion is:
if:
required:
- foo
then:
properties:
foobar: false
>
> - if:
> required: [mux-reg-masks]
> then:
> properties:
> idle-states:
> description: It can be present with mux-reg-masks, but not
> required
This one here is the default, I don't think it needs an if/else.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH 1/2] devicetree: bindings: mux: reg-mux: Update bindings for reg-mux for new property
2025-03-03 16:58 ` Conor Dooley
@ 2025-03-03 18:45 ` Vankar, Chintan
0 siblings, 0 replies; 10+ messages in thread
From: Vankar, Chintan @ 2025-03-03 18:45 UTC (permalink / raw)
To: Conor Dooley
Cc: Andrew Davis, Conor Dooley, Krzysztof Kozlowski, Rob Herring,
Peter Rosin, tglx, gregkh, vigneshr, nm, s-vadapalli, danishanwar,
linux-kernel, devicetree
Hello Conor,
On 3/3/2025 10:28 PM, Conor Dooley wrote:
> On Sat, Mar 01, 2025 at 03:08:40AM +0530, Vankar, Chintan wrote:
>> Hello Conor, Andrew,
>>
>> On 3/1/2025 12:22 AM, Conor Dooley wrote:
>>> On Thu, Feb 27, 2025 at 03:26:31PM -0600, Andrew Davis wrote:
>>>> On 2/27/25 2:22 PM, Chintan Vankar wrote:
>>>>> DT-binding of reg-mux is defined in such a way that one need to provide
>>>>> register offset and mask in a "mux-reg-masks" property and corresponding
>>>>> register value in "idle-states" property. This constraint forces to define
>>>>> these values in such a way that "mux-reg-masks" and "idle-states" must be
>>>>> in sync with each other. This implementation would be more complex if
>>>>> specific register or set of registers need to be configured which has
>>>>> large memory space. Introduce a new property "mux-reg-masks-state" which
>>>>> allow to specify offset, mask and value as a tuple in a single property.
>>>>>
>>>>> Signed-off-by: Chintan Vankar <c-vankar@ti.com>
>>>>> ---
>>>>> .../devicetree/bindings/mux/reg-mux.yaml | 29 +++++++++++++++++--
>>>>> 1 file changed, 27 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/mux/reg-mux.yaml b/Documentation/devicetree/bindings/mux/reg-mux.yaml
>>>>> index dc4be092fc2f..a73c5efcf860 100644
>>>>> --- a/Documentation/devicetree/bindings/mux/reg-mux.yaml
>>>>> +++ b/Documentation/devicetree/bindings/mux/reg-mux.yaml
>>>>> @@ -32,11 +32,36 @@ properties:
>>>>> - description: pre-shifted bitfield mask
>>>>> description: Each entry pair describes a single mux control.
>>>>> - idle-states: true
>>>>> + idle-states:
>>>>> + description: Each entry describes mux register state.
>>>>> +
>>>>> + mux-reg-masks-state:
>>>>> + $ref: /schemas/types.yaml#/definitions/uint32-matrix
>>>>> + items:
>>>>> + items:
>>>>> + - description: register offset
>>>>> + - description: pre-shifted bitfield mask
>>>>> + - description: register value to be set
>>>>> + description: This property is an extension of mux-reg-masks which
>>>>> + allows specifying register offset, mask and register
>>>>> + value to be set in a single property.
>>>>> +
>>>>> +allOf:
>>>>> + - if:
>>>>> + properties:
>>>>> + compatible:
>>>>> + contains:
>>>>> + enum:
>>>>> + - reg-mux
>>>>> + - mmio-mux
>>>>
>>>> These are the only two possible compatibles, is this "if" check needed?
>>>
>>> Aye.
>>>
>>>>> + then:
>>>>> + properties:
>>>>> + mux-reg-masks: true
>>>>> + mux-reg-masks-state: true
>>>>
>>>> You need one, but cannot have both, right? There should be some
>>>> way to describe that.
>>>>
>>>> Also an example added below would be good.
>>>
>>> From the example schema:
>>> # if/then schema can be used to handle conditions on a property affecting
>>> # another property. A typical case is a specific 'compatible' value changes the
>>> # constraints on other properties.
>>> #
>>> # For multiple 'if' schema, group them under an 'allOf'.
>>> #
>>> # If the conditionals become too unweldy, then it may be better to just split
>>> # the binding into separate schema documents.
>>> allOf:
>>> - if:
>>> properties:
>>> compatible:
>>> contains:
>>> const: vendor,soc2-ip
>>> then:
>>> required:
>>> - foo-supply
>>> else:
>>> # If otherwise the property is not allowed:
>>> properties:
>>> foo-supply: false
>>>
>>> What's missing from here is making one of the properties required,
>>> so
>>> oneOf:
>>> - required:
>>> - masks
>>> - required:
>>> - masks-state
>>>
>>>>
>>>> Andrew
>>
>> Thanks for reviewing this patch.
>>
>> For the use-case we have following three rules to be followed:
>> 1. "mux-reg-masks" and "mux-reg-masks-state" should be mutually
>> exclusive.
>> 2. "mux-reg-masks-state" and "idle-states" should also be mutually
>> exclusive.
>> 3. If "mux-reg-masks" is present then "idle-states" might or might not
>> be there.
>>
>> For the above conditions I have tried to write a binding as:
>>
>> allOf:
>> - not:
>> required: [mux-reg-masks, mux-reg-masks-state]
>>
>> - if:
>> required: [mux-reg-masks-state]
>> then:
>> not:
>> required: [idle-states]
>
> Why'd you pick two different syntax here?
> The normal syntax for mutual exclusion is:
> if:
> required:
> - foo
> then:
> properties:
> foobar: false
>
>
>>
>
>> - if:
>> required: [mux-reg-masks]
>> then:
>> properties:
>> idle-states:
>> description: It can be present with mux-reg-masks, but not
>> required
>
> This one here is the default, I don't think it needs an if/else.
>
Thank you for reviewing the patch. I modified binding according to your
suggestions and that worked for all conditions mentioned above. I will
update it and post v2.
Regards,
Chintan.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH 2/2] mux: mmio: Extend mmio-mux driver to configure mux with new DT property
2025-02-27 21:39 ` Andrew Davis
@ 2025-03-04 5:16 ` Vankar, Chintan
0 siblings, 0 replies; 10+ messages in thread
From: Vankar, Chintan @ 2025-03-04 5:16 UTC (permalink / raw)
To: Andrew Davis, Conor Dooley, Krzysztof Kozlowski, Rob Herring,
Peter Rosin, tglx, gregkh, vigneshr, nm, s-vadapalli, danishanwar
Cc: linux-kernel, devicetree
Hello Andrew,
On 2/28/2025 3:09 AM, Andrew Davis wrote:
> On 2/27/25 2:22 PM, Chintan Vankar wrote:
>> MMIO mux driver is designed to parse "mux-reg-masks" and "idle-states"
>> property independently to configure mux registers. Drawback of this
>> approach is, while configuring mux-controller one need to specify every
>> register of memory space with offset and mask in "mux-reg-masks" and
>> register state to "idle-states", that would be more complex for devices
>> with large memory space.
>>
>> Add support to extend the mmio mux driver to configure a specific
>> register
>> or set of register in memory space.
>>
>> Signed-off-by: Chintan Vankar <c-vankar@ti.com>
>> ---
>> drivers/mux/mmio.c | 148 +++++++++++++++++++++++++++++++++++++--------
>> 1 file changed, 122 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/mux/mmio.c b/drivers/mux/mmio.c
>> index 30a952c34365..8937d0ea2b11 100644
>> --- a/drivers/mux/mmio.c
>> +++ b/drivers/mux/mmio.c
>> @@ -2,7 +2,7 @@
>> /*
>> * MMIO register bitfield-controlled multiplexer driver
>> *
>> - * Copyright (C) 2017 Pengutronix, Philipp Zabel <kernel@pengutronix.de>
>> + * Copyright (C) 2017-2025 Pengutronix, Philipp Zabel
>> <kernel@pengutronix.de>
>> */
>> #include <linux/bitops.h>
>> @@ -33,10 +33,84 @@ static const struct of_device_id mux_mmio_dt_ids[]
>> = {
>> };
>> MODULE_DEVICE_TABLE(of, mux_mmio_dt_ids);
>> +static int reg_mux_get_controllers(const struct device_node *np, char
>> *prop_name)
>> +{
>> + int ret;
>> +
>> + ret = of_property_count_u32_elems(np, prop_name);
>> + if (ret == 0 || ret % 2)
>> + ret = -EINVAL;
>> +
>> + return ret;
>> +}
>> +
>> +static int reg_mux_get_controllers_extended(const struct device_node
>> *np, char *prop_name)
>> +{
>> + int ret;
>> +
>> + ret = of_property_count_u32_elems(np, prop_name);
>> + if (ret == 0 || ret % 3)
>> + ret = -EINVAL;
>> +
>> + return ret;
>> +}
>> +
>> +static int reg_mux_parse_dt(const struct device_node *np, bool
>> *mux_reg_masks_state,
>> + int *num_fields)
>> +{
>> + int ret;
>> +
>> + if (*mux_reg_masks_state) {
>> + ret = reg_mux_get_controllers_extended(np,
>> "mux-reg-masks-state");
>> + if (ret < 0)
>> + return ret;
>> + *num_fields = ret / 3;
>> + } else {
>> + ret = reg_mux_get_controllers(np, "mux-reg-masks");
>> + if (ret < 0)
>> + return ret;
>> + *num_fields = ret / 2;
>> + }
>> + return ret;
>> +}
>> +
>> +static int mux_reg_set_parameters(const struct device_node *np, char
>> *prop_name, u32 *reg,
>> + u32 *mask, int index)
>> +{
>> + int ret;
>> +
>> + ret = of_property_read_u32_index(np, prop_name,
>> + 2 * index, reg);
>> + if (!ret)
>> + ret = of_property_read_u32_index(np, prop_name,
>> + 2 * index + 1, mask);
>> +
>> + return ret;
>> +}
>> +
>> +static int mux_reg_set_parameters_extended(const struct device_node
>> *np, char *prop_name, u32 *reg,
>> + u32 *mask, u32 *state, int index)
>> +{
>> + int ret;
>> +
>> + ret = of_property_read_u32_index(np, prop_name,
>> + 3 * index, reg);
>
> This is some odd line wrapping, why newline at 55 chars here?
> You can go to 80 or 100 if it is readable.
>
>> + if (!ret) {
>
> Just return early, no need for this MISRA-like "single return" junk.
>
>> + ret = of_property_read_u32_index(np, prop_name,
>> + 3 * index + 1, mask);
>> + if (!ret)
>> + ret = of_property_read_u32_index(np, prop_name,
>> + 3 * index + 2, state);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> static int mux_mmio_probe(struct platform_device *pdev)
>> {
>> struct device *dev = &pdev->dev;
>> struct device_node *np = dev->of_node;
>> + bool mux_reg_masks_state = false;
>> struct regmap_field **fields;
>> struct mux_chip *mux_chip;
>> struct regmap *regmap;
>> @@ -59,15 +133,19 @@ static int mux_mmio_probe(struct platform_device
>> *pdev)
>> return dev_err_probe(dev, PTR_ERR(regmap),
>> "failed to get regmap\n");
>> - ret = of_property_count_u32_elems(np, "mux-reg-masks");
>> - if (ret == 0 || ret % 2)
>> - ret = -EINVAL;
>> + if (of_property_present(np, "mux-reg-masks-state"))
>> + mux_reg_masks_state = true;
>> +
>> + ret = reg_mux_parse_dt(np, &mux_reg_masks_state, &num_fields);
>
> Why are you passing this bool by pointer? You don't modify it in the
> function..
>
>> if (ret < 0) {
>> - dev_err(dev, "mux-reg-masks property missing or invalid: %d\n",
>> - ret);
>> + if (mux_reg_masks_state)
>> + dev_err(dev, "mux-reg-masks-state property missing or
>> invalid: %d\n",
>> + ret);
>> + else
>> + 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));
>> @@ -79,19 +157,25 @@ static int mux_mmio_probe(struct platform_device
>> *pdev)
>> 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;
>> + s32 state, 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;
>> + if (!mux_reg_masks_state) {
>> + ret = mux_reg_set_parameters(np, "mux-reg-masks", ®,
>> &mask, i);
>> + if (ret < 0) {
>> + dev_err(dev, "bitfield %d: failed to read
>> mux-reg-masks property: %d\n",
>> + i, ret);
>> + return ret;
>> + }
>> + } else {
>> + ret = mux_reg_set_parameters_extended(np,
>> "mux-reg-masks-state", ®,
>> + &mask, &state, i);
>> + if (ret < 0) {
>> + dev_err(dev, "bitfield %d: failed to read
>> custom-states property: %d\n",
>> + i, ret);
>> + return ret;
>> + }
>> }
>> field.reg = reg;
>> @@ -115,16 +199,28 @@ static int mux_mmio_probe(struct platform_device
>> *pdev)
>> 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;
>> + if (!mux_reg_masks_state) {
>> + of_property_read_u32_index(np, "idle-states", i,
>> + (u32 *)&idle_state);
>
> From here down, both branches of this are almost identical, idle_state and
> your new "state" var do the same thing, why do you need both?
>
I will address your above comments.
For the idle-states I keep following older DT-binding terminology, hence
when idle states are getting parsed I am storing that in idle_state
variable. For new DT-Binding I have introduce a new property for
register offset, mask and state, storing it in new variable "state".
Regards,
Chintan.
> Andrew
>
>> + 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;
>> + }
>> + } else {
>> + if (state != MUX_IDLE_AS_IS) {
>> + if (state < 0 || state >= mux->states) {
>> + dev_err(dev, "bitfield: %d: out of range idle
>> state %d\n",
>> + i, state);
>> + return -EINVAL;
>> + }
>> +
>> + mux->idle_state = state;
>> }
>> -
>> - mux->idle_state = idle_state;
>> }
>> }
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2025-03-04 5:16 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-27 20:22 [RFC PATCH 0/2] Extend mmio-mux driver to configure mux with Chintan Vankar
2025-02-27 20:22 ` [RFC PATCH 1/2] devicetree: bindings: mux: reg-mux: Update bindings for reg-mux for new property Chintan Vankar
2025-02-27 21:26 ` Andrew Davis
2025-02-28 18:52 ` Conor Dooley
2025-02-28 21:38 ` Vankar, Chintan
2025-03-03 16:58 ` Conor Dooley
2025-03-03 18:45 ` Vankar, Chintan
2025-02-27 20:22 ` [RFC PATCH 2/2] mux: mmio: Extend mmio-mux driver to configure mux with new DT property Chintan Vankar
2025-02-27 21:39 ` Andrew Davis
2025-03-04 5:16 ` Vankar, Chintan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox