| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3b232103-3c02-4d7a-864c-45e6a3de3095@ti.com>
Date: Thu, 27 Feb 2025 15:39:26 -0600
From: Andrew Davis <afd@...com>
To: Chintan Vankar <c-vankar@...com>, Conor Dooley <conor+dt@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Rob Herring <robh@...nel.org>, Peter Rosin <peda@...ntia.se>,
<tglx@...utronix.de>, <gregkh@...uxfoundation.org>, <vigneshr@...com>,
<nm@...com>, <s-vadapalli@...com>, <danishanwar@...com>
CC: <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>
Subject: Re: [RFC PATCH 2/2] mux: mmio: Extend mmio-mux driver to configure
mux with new DT property
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@...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@...gutronix.de>
> + * Copyright (C) 2017-2025 Pengutronix, Philipp Zabel <kernel@...gutronix.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;
> }
> }
>
Powered by blists - more mailing lists