| 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: <48c110fc-6416-4b3a-911f-c24af3352e3b@ti.com>
Date: Tue, 4 Mar 2025 10:46:16 +0530
From: "Vankar, Chintan" <c-vankar@...com>
To: Andrew Davis <afd@...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
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@...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?
>
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;
>> }
>> }
Powered by blists - more mailing lists