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

[luyulin@eswincomputing.com]

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