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

[Dan Carpenter <dan.carpenter@linaro.org>]

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