Re: [PATCH v1 2/2] reset: mediatek: add syscon-based reset controller driver

[sashiko-bot@kernel.org]

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