* [bug report] pinctrl: eswin: Add EIC7700 pinctrl driver
@ 2025-08-19 9:40 Dan Carpenter
2025-09-01 7:34 ` luyulin
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2025-08-19 9:40 UTC (permalink / raw)
To: Yulin Lu; +Cc: linux-gpio
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
619 static int eic7700_pinctrl_probe(struct platform_device *pdev)
620 {
621 struct device *dev = &pdev->dev;
622 struct pinctrl_dev *pctldev;
623 struct eic7700_pinctrl *pc;
624 struct regulator *regulator;
625 u32 rgmii0_mode, rgmii1_mode;
626 int ret, voltage;
627
628 pc = devm_kzalloc(dev, struct_size(pc, functions, EIC7700_FUNCTIONS_COUNT), GFP_KERNEL);
629 if (!pc)
630 return -ENOMEM;
631
632 pc->base = devm_platform_ioremap_resource(pdev, 0);
633 if (IS_ERR(pc->base))
634 return PTR_ERR(pc->base);
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.
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [bug report] pinctrl: eswin: Add EIC7700 pinctrl driver
2025-08-19 9:40 [bug report] pinctrl: eswin: Add EIC7700 pinctrl driver Dan Carpenter
@ 2025-09-01 7:34 ` luyulin
2025-09-01 9:07 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: luyulin @ 2025-09-01 7:34 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-gpio
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [bug report] pinctrl: eswin: Add EIC7700 pinctrl driver
2025-09-01 7:34 ` luyulin
@ 2025-09-01 9:07 ` Dan Carpenter
2025-09-02 7:38 ` luyulin
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2025-09-01 9:07 UTC (permalink / raw)
To: luyulin; +Cc: linux-gpio
On Mon, Sep 01, 2025 at 03:34:34PM +0800, luyulin@eswincomputing.com wrote:
> 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?
>
drivers/pinctrl/Kconfig
209 config PINCTRL_EIC7700
210 tristate "EIC7700 PINCTRL driver"
211 depends on ARCH_ESWIN || COMPILE_TEST
^^^^^^^^^^
ARCH_ESWIN doesn't exist so this driver can't actually be enabled. I'm
not a Kconfig expert but I think that ARCH_ESWIN would normally have the
regulator stuff?
212 select PINMUX
213 select GENERIC_PINCONF
214 help
215 This driver support for the pin controller in ESWIN's EIC7700 SoC,
216 which supports pin multiplexing, pin configuration,and rgmii voltage
217 control.
218 Say Y here to enable the eic7700 pinctrl driver
But, yes, the Kconfig should ensure that the driver has all the required
bits so we don't compile an unprobe-able (improbe-able?) driver.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: Re: [bug report] pinctrl: eswin: Add EIC7700 pinctrl driver
2025-09-01 9:07 ` Dan Carpenter
@ 2025-09-02 7:38 ` luyulin
2025-09-02 8:00 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: luyulin @ 2025-09-02 7:38 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-gpio
Hi, Dan
Thank you very much for your suggestions and reply.
I changed IS_ERR_OR_NULL to IS_ERR and added select REGULATOR
and REGULATOR_FIXED_VOLTAGE in Kconfig. The local tests are normal.
I also added "Suggested-by: Dan Carpenter dan.carpenter@linaro.org" in the commit.
Do you think this is acceptable?
The commit content is as follows:
From 4bf1f20bafc8dcfaebd43e7c8b42820edb36f028 Mon Sep 17 00:00:00 2001
From: Yulin Lu <luyulin@eswincomputing.com>
Date: Tue, 2 Sep 2025 14:41:53 +0800
Subject: [PATCH] pinctrl: eswin: Fix regulator error check and Kconfig
dependency
Smatch reported the following warning in eic7700_pinctrl_probe():
drivers/pinctrl/pinctrl-eic7700.c:638 eic7700_pinctrl_probe()
warn: passing zero to 'PTR_ERR'
The root cause is that devm_regulator_get() may return NULL when
CONFIG_REGULATOR is disabled. In such case, IS_ERR_OR_NULL() triggers
PTR_ERR(NULL) which evaluates to 0, leading to passing a success code
as an error.
However, this driver cannot work without a regulator. To fix this:
- Change the check from IS_ERR_OR_NULL() to IS_ERR()
- Update Kconfig to explicitly select REGULATOR and
REGULATOR_FIXED_VOLTAGE, ensuring that the regulator framework is
always available.
This resolves the Smatch warning and enforces the correct dependency.
Suggested-by: Dan Carpenter <dan.carpenter@linaro.org>
Fixes: 5b797bcc00ef ("pinctrl: eswin: Add EIC7700 pinctrl driver")
Signed-off-by: Yulin Lu <luyulin@eswincomputing.com>
>
> On Mon, Sep 01, 2025 at 03:34:34PM +0800, luyulin@eswincomputing.com wrote:
> > 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'
> > >
> > >
> > > 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?
> >
>
> drivers/pinctrl/Kconfig
> 209 config PINCTRL_EIC7700
> 210 tristate "EIC7700 PINCTRL driver"
> 211 depends on ARCH_ESWIN || COMPILE_TEST
> ^^^^^^^^^^
> ARCH_ESWIN doesn't exist so this driver can't actually be enabled. I'm
> not a Kconfig expert but I think that ARCH_ESWIN would normally have the
> regulator stuff?
>
> 212 select PINMUX
> 213 select GENERIC_PINCONF
> 214 help
> 215 This driver support for the pin controller in ESWIN's EIC7700 SoC,
> 216 which supports pin multiplexing, pin configuration,and rgmii voltage
> 217 control.
> 218 Say Y here to enable the eic7700 pinctrl driver
>
> But, yes, the Kconfig should ensure that the driver has all the required
> bits so we don't compile an unprobe-able (improbe-able?) driver.
>
Thanks,
Yulin
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: Re: [bug report] pinctrl: eswin: Add EIC7700 pinctrl driver
2025-09-02 7:38 ` luyulin
@ 2025-09-02 8:00 ` Dan Carpenter
0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2025-09-02 8:00 UTC (permalink / raw)
To: luyulin; +Cc: linux-gpio
On Tue, Sep 02, 2025 at 03:38:41PM +0800, luyulin@eswincomputing.com wrote:
> Hi, Dan
>
> Thank you very much for your suggestions and reply.
> I changed IS_ERR_OR_NULL to IS_ERR and added select REGULATOR
> and REGULATOR_FIXED_VOLTAGE in Kconfig. The local tests are normal.
> I also added "Suggested-by: Dan Carpenter dan.carpenter@linaro.org" in the commit.
> Do you think this is acceptable?
>
> The commit content is as follows:
>
> From 4bf1f20bafc8dcfaebd43e7c8b42820edb36f028 Mon Sep 17 00:00:00 2001
> From: Yulin Lu <luyulin@eswincomputing.com>
> Date: Tue, 2 Sep 2025 14:41:53 +0800
> Subject: [PATCH] pinctrl: eswin: Fix regulator error check and Kconfig
> dependency
>
> Smatch reported the following warning in eic7700_pinctrl_probe():
>
> drivers/pinctrl/pinctrl-eic7700.c:638 eic7700_pinctrl_probe()
> warn: passing zero to 'PTR_ERR'
>
> The root cause is that devm_regulator_get() may return NULL when
> CONFIG_REGULATOR is disabled. In such case, IS_ERR_OR_NULL() triggers
> PTR_ERR(NULL) which evaluates to 0, leading to passing a success code
> as an error.
>
> However, this driver cannot work without a regulator. To fix this:
>
> - Change the check from IS_ERR_OR_NULL() to IS_ERR()
> - Update Kconfig to explicitly select REGULATOR and
> REGULATOR_FIXED_VOLTAGE, ensuring that the regulator framework is
> always available.
>
> This resolves the Smatch warning and enforces the correct dependency.
>
> Suggested-by: Dan Carpenter <dan.carpenter@linaro.org>
> Fixes: 5b797bcc00ef ("pinctrl: eswin: Add EIC7700 pinctrl driver")
> Signed-off-by: Yulin Lu <luyulin@eswincomputing.com>
>
Sounds good to me. I'm not a Kconfig expert so I never know if a select
or a depend is more appropriate...
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-09-02 8:00 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-19 9:40 [bug report] pinctrl: eswin: Add EIC7700 pinctrl driver Dan Carpenter
2025-09-01 7:34 ` luyulin
2025-09-01 9:07 ` Dan Carpenter
2025-09-02 7:38 ` luyulin
2025-09-02 8:00 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox