Re: [PATCH 2/3] soc: realtek: Add driver for DHC I/O level detector

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Ignored return values from regmap operations lead to uninitialized stack variable usage and potential hardware misconfiguration.
- [Low] Unnecessary dynamic memory allocation for probe-only data structure.
--

commit c604ce13442f04b606b3ba51b12003236c8a3ba6
Author: Tzuyi Chang <tychang@realtek.com>

soc: realtek: Add driver for DHC I/O level detector

Add driver support for the Realtek DHC I/O level detector. The driver reads
hardware registers to determine the current I/O voltage levels (e.g., 1.8V
or 3.3V) for specific IP blocks and configures pinctrl states.

> diff --git a/drivers/soc/realtek/rtd-io-detect.c b/drivers/soc/realtek/rtd-io-detect.c
> new file mode 100644
> index 0000000000000..84ef8ea23cb5c
> --- /dev/null
> +++ b/drivers/soc/realtek/rtd-io-detect.c

[ ... ]

> +static void detect_io_set(struct pinctrl *pinctrl,
> +			  const struct rtd_io_detect_desc_info *desc,
> +			  struct rtd_io_detect_data *data)
> +{
> +	struct pinctrl_state *state_1v8;
> +	struct pinctrl_state *state_3v3;
> +	unsigned int val;
> +	int ret;

[ ... ]

> +	regmap_update_bits(data->base, desc->reg_offset,
> +			   BIT(desc->en_offset), BIT(desc->en_offset));
> +
> +	regmap_read(data->base, desc->reg_offset, &val);
> +
> +	ret = pinctrl_select_state(pinctrl,
> +				   (val & BIT(desc->status_offset)) ? state_3v3 : state_1v8);

[Severity: High]
If regmap_update_bits() or regmap_read() fail here, won't val remain
uninitialized?

Since val is used immediately after to select the voltage state, could this
cause the hardware to be misconfigured by applying 3.3V to a 1.8V pad if
it evaluates a garbage stack value?

> +	if (ret)
> +		dev_err(data->dev, "Failed to select pinctrl state\n");
> +}
> +
> +static int rtd_io_detect_probe(struct platform_device *pdev)
> +{
> +	struct rtd_io_detect_data *data;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *pinctrl_np;
> +	struct pinctrl *pinctrl;
> +	int i;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;

[Severity: Low]
Does this structure need to be dynamically allocated and tied to the
device lifetime with devm_kzalloc()?

It looks like struct rtd_io_detect_data is only used locally within
rtd_io_detect_probe() to pass arguments into detect_io_set(). Since it is
never saved via platform_set_drvdata() or used in other callbacks, could it
just be allocated on the stack to save memory?

> +
> +	pinctrl_np = of_parse_phandle(dev->of_node, "realtek,iso-pinctrl", 0);
> +	if (!pinctrl_np) {
> +		dev_err(dev, "Failed to find ISO pinctrl node\n");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604111821.975624-1-eleanor.lin@realtek.com?part=2