Re: [bug report] pinctrl: eswin: Add EIC7700 pinctrl driver

[luyulin@eswincomputing.com]

Hi, Dan

Thank you very much for your findings and suggestions.
I would also like to confirm my understanding with you.

> 
> Hello Yulin Lu,
> 
> Commit 5b797bcc00ef ("pinctrl: eswin: Add EIC7700 pinctrl driver")
> from Jun 12, 2025 (linux-next), leads to the following Smatch static
> checker warning:
> 
> 	drivers/pinctrl/pinctrl-eic7700.c:638 eic7700_pinctrl_probe()
> 	warn: passing zero to 'PTR_ERR'
> 
> drivers/pinctrl/pinctrl-eic7700.c
>     635 
>     636         regulator = devm_regulator_get(dev, "vrgmii");
>     637         if (IS_ERR_OR_NULL(regulator)) {
> --> 638                 return dev_err_probe(dev, PTR_ERR(regulator),
> 
> devm_regulator_get() will return NULL if CONFIG_REGULATOR is disabled.
> PTR_ERR(NULL) is success.
> 
>     639                                          "failed to get vrgmii regulator\n");
>     640         }
>     641 
>     642         voltage = regulator_get_voltage(regulator);
>     643         if (voltage < 0) {
>     644                 return dev_err_probe(&pdev->dev, voltage,
>     645                          "Failed to get voltage from regulator\n");
> 
> If CONFIG_REGULATOR is disabled then this will return negative.  So this
> driver can't work without a regulator.  Ideally the KConfig would enforce
> that so we don't build drivers which can't work.

Thank you! You are right. IS_ERR_OR_NULL should be changed to IS_ERR.
Based on your professional advice, should I add "depends on REGULATOR" and
"depends on REGULATOR_FIXED_VOLTAGE" in the Kconfig to enforce compilation dependencies?

> 
> In that case the if (IS_ERR_OR_NULL()) check should be changed to
> if (IS_ERR()) {.  See my blog for more details:
> https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/
> 
>     646         }
>     647 
>     648         rgmii0_mode = readl_relaxed(pc->base + EIC7700_RGMII0_SEL_MODE);
> 
> regards,
> dan carpenter

Best Regards,
Yulin