* Re: [PATCH v1 2/2] reset: mediatek: add syscon-based reset controller driver
[not found] ` <20260626074820.2537772-3-peter.wang@mediatek.com>
@ 2026-06-26 8:00 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-06-26 8:00 UTC (permalink / raw)
To: peter.wang; +Cc: devicetree, robh, conor+dt
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The driver incorrectly uses read-modify-write operations (`regmap_write_bits`) on dedicated set/clear registers, causing spurious resets or clear operations on unrelated hardware lines.
- [Medium] The temporary array `arr` is allocated with `devm_kcalloc`, permanently wasting memory for the lifetime of the device.
--
commit 51d3326953762868e035e2bcec526fcc8e9e6083
Author: Peter Wang <peter.wang@mediatek.com>
reset: mediatek: add syscon-based reset controller driver
Add a new reset controller driver for MediaTek SoCs that expose reset
lines through memory-mapped registers managed by a syscon node. The
driver reads reset configurations from the mediatek,reset-bits property.
> diff --git a/drivers/reset/reset-mediatek-syscon.c b/drivers/reset/reset-mediatek-syscon.c
> new file mode 100644
> index 0000000000000..da8aa646cee76
> --- /dev/null
> +++ b/drivers/reset/reset-mediatek-syscon.c
[ ... ]
> +static int mediatek_syscon_reset_assert(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + struct mediatek_syscon_reset_data *data =
> + container_of(rcdev, struct mediatek_syscon_reset_data, rcdev);
> + struct mediatek_syscon_reset_control *rstc = &data->mrstc[id];
> + unsigned int val = BIT(rstc->assert_bit);
> +
> + return regmap_write_bits(data->regmap, rstc->assert_offset, val, val);
[Severity: High]
Since the hardware uses separate set and clear registers, could using
regmap_write_bits() here cause an unintended read-modify-write cycle?
If the register is Write-1-to-Set, reading it might return the current
status, and writing that back will unintentionally write '1' to other
active lines in the same register. Should this use a direct regmap_write()
instead?
> +}
> +
> +/**
> + * mediatek_syscon_reset_deassert() - Deassert a specific reset line.
[ ... ]
> + */
> +static int mediatek_syscon_reset_deassert(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + struct mediatek_syscon_reset_data *data =
> + container_of(rcdev, struct mediatek_syscon_reset_data, rcdev);
> + struct mediatek_syscon_reset_control *rstc = &data->mrstc[id];
> + unsigned int val = BIT(rstc->deassert_bit);
> +
> + return regmap_write_bits(data->regmap, rstc->deassert_offset, val, val);
[Severity: High]
Similarly, for the deassert (Write-1-to-Clear) register, does using a
read-modify-write operation here risk unintentionally clearing other
unrelated reset lines that happen to read back as '1'?
> +}
[ ... ]
> +static int mediatek_syscon_reset_probe(struct platform_device *pdev)
> +{
[ ... ]
> + if (count % 5 != 0) {
> + dev_err(dev, "mediatek,reset-bits count (%d) must be a multiple of 5\n", count);
> + return -EINVAL;
> + }
> +
> + nr_resets = count / 5;
> +
> + arr = devm_kcalloc(dev, count, sizeof(u32), GFP_KERNEL);
[Severity: Medium]
Since arr is only used temporarily during probe to parse the device tree
properties and is not needed afterwards, does using devm_kcalloc()
unnecessarily keep this memory allocated for the lifetime of the device?
Would a standard kcalloc() followed by a kfree() at the end of the function
be more appropriate?
> + if (!arr)
> + return -ENOMEM;
> +
> + ret = of_property_read_u32_array(np, "mediatek,reset-bits", arr, count);
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260626074820.2537772-1-peter.wang@mediatek.com?part=2
^ permalink raw reply [flat|nested] 2+ messages in thread