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

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

[Dan Carpenter]

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

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

[luyulin]

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

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

[Dan Carpenter]

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

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

[luyulin]

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

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

[Dan Carpenter]

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