[PATCH] iio: adc: ad7173: fix compiling without gpiolib

[PATCH] iio: adc: ad7173: fix compiling without gpiolib

[David Lechner]

Fix compiling the ad7173 driver when CONFIG_GPIOLIB is not set by
selecting GPIOLIB to be always enabled and remove the #if.

Commit 031bdc8aee01 ("iio: adc: ad7173: add calibration support") placed
unrelated code in the middle of the #if IS_ENABLED(CONFIG_GPIOLIB) block
which caused the reported compile error.

However, later commit 7530ed2aaa3f ("iio: adc: ad7173: add openwire
detection support for single conversions") makes use of the gpio regmap
even when we aren't providing gpio controller support. So it makes more
sense to always enable GPIOLIB rather than trying to make it optional.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202504220824.HVrTVov1-lkp@intel.com/
Fixes: 031bdc8aee01 ("iio: adc: ad7173: add calibration support")
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
Not related to the fix, but I also question the use of the regmap here.
This is one of the ad_sigma_delta drivers that does funny things with
the SPI bus, like keeping it locked during the entire time a buffer is
enabled. So, if someone tried to use a GPIO during a buffered read, the
GPIO call could block (waiting for the SPI bus mutex) until the buffer
is disabled, which could be an indefinitely long time. And to make it
even worse, this is not an interruptible wait, so the GPIO consumer
would effectively be deadlocked.
---
 drivers/iio/adc/Kconfig  |  5 +++--
 drivers/iio/adc/ad7173.c | 15 +--------------
 2 files changed, 4 insertions(+), 16 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index ad06cf5567851ee71f1211ec69d59fd5c1857ee5..4591c886ffea5519b374a28c7a0698eb19169c9f 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -137,8 +137,9 @@ config AD7173
 	tristate "Analog Devices AD7173 driver"
 	depends on SPI_MASTER
 	select AD_SIGMA_DELTA
-	select GPIO_REGMAP if GPIOLIB
-	select REGMAP_SPI if GPIOLIB
+	select GPIOLIB
+	select GPIO_REGMAP
+	select REGMAP_SPI
 	help
 	  Say yes here to build support for Analog Devices AD7173 and similar ADC
 	  Currently supported models:
diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index 69de5886474ce2f700bf277ce707b15637113564..b3e6bd2a55d717d5384616d9a8a160c57a8f1948 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -230,10 +230,8 @@ struct ad7173_state {
 	unsigned long long *config_cnts;
 	struct clk *ext_clk;
 	struct clk_hw int_clk_hw;
-#if IS_ENABLED(CONFIG_GPIOLIB)
 	struct regmap *reg_gpiocon_regmap;
 	struct gpio_regmap *gpio_regmap;
-#endif
 };
 
 static unsigned int ad4115_sinc5_data_rates[] = {
@@ -288,8 +286,6 @@ static const char *const ad7173_clk_sel[] = {
 	"ext-clk", "xtal"
 };
 
-#if IS_ENABLED(CONFIG_GPIOLIB)
-
 static const struct regmap_range ad7173_range_gpio[] = {
 	regmap_reg_range(AD7173_REG_GPIO, AD7173_REG_GPIO),
 };
@@ -543,12 +539,6 @@ static int ad7173_gpio_init(struct ad7173_state *st)
 
 	return 0;
 }
-#else
-static int ad7173_gpio_init(struct ad7173_state *st)
-{
-	return 0;
-}
-#endif /* CONFIG_GPIOLIB */
 
 static struct ad7173_state *ad_sigma_delta_to_ad7173(struct ad_sigma_delta *sd)
 {
@@ -1797,10 +1787,7 @@ static int ad7173_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
-	if (IS_ENABLED(CONFIG_GPIOLIB))
-		return ad7173_gpio_init(st);
-
-	return 0;
+	return ad7173_gpio_init(st);
 }
 
 static const struct of_device_id ad7173_of_match[] = {

---
base-commit: b475195fecc79a1a6e7fb0846aaaab0a1a4cb2e6
change-id: 20250422-iio-adc-ad7173-fix-compile-without-gpiolib-7dd72e254994

Best regards,
-- 
David Lechner <dlechner@baylibre.com>

Re: [PATCH] iio: adc: ad7173: fix compiling without gpiolib

[Andy Shevchenko]

On Tue, Apr 22, 2025 at 11:12 PM David Lechner <dlechner@baylibre.com> wrote:
>
> Fix compiling the ad7173 driver when CONFIG_GPIOLIB is not set by
> selecting GPIOLIB to be always enabled and remove the #if.

I'm not sure we need to select GPIOLIB. If you want it, depend on it.
GPIOLIB is not a hidden symbol, so why "select"?

> Commit 031bdc8aee01 ("iio: adc: ad7173: add calibration support") placed
> unrelated code in the middle of the #if IS_ENABLED(CONFIG_GPIOLIB) block
> which caused the reported compile error.
>
> However, later commit 7530ed2aaa3f ("iio: adc: ad7173: add openwire
> detection support for single conversions") makes use of the gpio regmap
> even when we aren't providing gpio controller support. So it makes more
> sense to always enable GPIOLIB rather than trying to make it optional.

...

> Not related to the fix, but I also question the use of the regmap here.
> This is one of the ad_sigma_delta drivers that does funny things with
> the SPI bus, like keeping it locked during the entire time a buffer is
> enabled. So, if someone tried to use a GPIO during a buffered read, the
> GPIO call could block (waiting for the SPI bus mutex) until the buffer
> is disabled, which could be an indefinitely long time. And to make it
> even worse, this is not an interruptible wait, so the GPIO consumer
> would effectively be deadlocked.

I would say either the entire buffer mode is broken (in software), or
hardware is broken and GPIO shouldn't be supported at all if the
buffer mode is enabled. I think the best solution here is to remove
the GPIO chip before enabling buffered mode. If GPIO is in use, fail
the buffer mode.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: adc: ad7173: fix compiling without gpiolib

[David Lechner]

On 4/22/25 4:03 PM, Andy Shevchenko wrote:
> On Tue, Apr 22, 2025 at 11:12 PM David Lechner <dlechner@baylibre.com> wrote:
>>
>> Fix compiling the ad7173 driver when CONFIG_GPIOLIB is not set by
>> selecting GPIOLIB to be always enabled and remove the #if.
> 
> I'm not sure we need to select GPIOLIB. If you want it, depend on it.
> GPIOLIB is not a hidden symbol, so why "select"?
> 
Since this parts of the driver unrelated to GPIO provider/consumer rely on this
being enabled to function, select seems more appropriate.

Re: [PATCH] iio: adc: ad7173: fix compiling without gpiolib

[Andy Shevchenko]

On Fri, Apr 25, 2025 at 7:55 PM David Lechner <dlechner@baylibre.com> wrote:
> On 4/22/25 4:03 PM, Andy Shevchenko wrote:
> > On Tue, Apr 22, 2025 at 11:12 PM David Lechner <dlechner@baylibre.com> wrote:

> >> Fix compiling the ad7173 driver when CONFIG_GPIOLIB is not set by
> >> selecting GPIOLIB to be always enabled and remove the #if.
> >
> > I'm not sure we need to select GPIOLIB. If you want it, depend on it.
> > GPIOLIB is not a hidden symbol, so why "select"?
> >
> Since this parts of the driver unrelated to GPIO provider/consumer rely on this
> being enabled to function, select seems more appropriate.

Hmm... The current state of affairs is 177 for select vs. 231 for
depends on. I dunno how many of them are historical, for now it seems
like 40%/60%. So if you think so, go for it!


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: adc: ad7173: fix compiling without gpiolib

[Jonathan Cameron]

On Tue, 22 Apr 2025 15:12:27 -0500
David Lechner <dlechner@baylibre.com> wrote:

> Fix compiling the ad7173 driver when CONFIG_GPIOLIB is not set by
> selecting GPIOLIB to be always enabled and remove the #if.
> 
> Commit 031bdc8aee01 ("iio: adc: ad7173: add calibration support") placed
> unrelated code in the middle of the #if IS_ENABLED(CONFIG_GPIOLIB) block
> which caused the reported compile error.
> 
> However, later commit 7530ed2aaa3f ("iio: adc: ad7173: add openwire
> detection support for single conversions") makes use of the gpio regmap
> even when we aren't providing gpio controller support. So it makes more
> sense to always enable GPIOLIB rather than trying to make it optional.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202504220824.HVrTVov1-lkp@intel.com/
> Fixes: 031bdc8aee01 ("iio: adc: ad7173: add calibration support")
> Signed-off-by: David Lechner <dlechner@baylibre.com>
Applied and marked for stable.

Thanks,

Jonathan

Re: [PATCH] iio: adc: ad7173: fix compiling without gpiolib

[Jonathan Cameron]

On Wed, 23 Apr 2025 00:03:38 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Apr 22, 2025 at 11:12 PM David Lechner <dlechner@baylibre.com> wrote:
> >
> > Fix compiling the ad7173 driver when CONFIG_GPIOLIB is not set by
> > selecting GPIOLIB to be always enabled and remove the #if.  
> 
> I'm not sure we need to select GPIOLIB. If you want it, depend on it.
> GPIOLIB is not a hidden symbol, so why "select"?
> 
> > Commit 031bdc8aee01 ("iio: adc: ad7173: add calibration support") placed
> > unrelated code in the middle of the #if IS_ENABLED(CONFIG_GPIOLIB) block
> > which caused the reported compile error.
> >
> > However, later commit 7530ed2aaa3f ("iio: adc: ad7173: add openwire
> > detection support for single conversions") makes use of the gpio regmap
> > even when we aren't providing gpio controller support. So it makes more
> > sense to always enable GPIOLIB rather than trying to make it optional.  
> 
> ...
> 
> > Not related to the fix, but I also question the use of the regmap here.
> > This is one of the ad_sigma_delta drivers that does funny things with
> > the SPI bus, like keeping it locked during the entire time a buffer is
> > enabled. So, if someone tried to use a GPIO during a buffered read, the
> > GPIO call could block (waiting for the SPI bus mutex) until the buffer
> > is disabled, which could be an indefinitely long time. And to make it
> > even worse, this is not an interruptible wait, so the GPIO consumer
> > would effectively be deadlocked.  
> 
> I would say either the entire buffer mode is broken (in software), or
> hardware is broken and GPIO shouldn't be supported at all if the
> buffer mode is enabled. I think the best solution here is to remove
> the GPIO chip before enabling buffered mode. If GPIO is in use, fail
> the buffer mode.
I'd kind of assume that anyone using these GPIOs is doing it in a fashion
related closely to the ADC itself.

Can we make any other use fail more cleanly? 

J
> 

Re: [PATCH] iio: adc: ad7173: fix compiling without gpiolib

[David Lechner]

On 4/26/25 10:18 AM, Jonathan Cameron wrote:
> On Wed, 23 Apr 2025 00:03:38 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
>> On Tue, Apr 22, 2025 at 11:12 PM David Lechner <dlechner@baylibre.com> wrote:
>>>

...

>>> Not related to the fix, but I also question the use of the regmap here.
>>> This is one of the ad_sigma_delta drivers that does funny things with
>>> the SPI bus, like keeping it locked during the entire time a buffer is
>>> enabled. So, if someone tried to use a GPIO during a buffered read, the
>>> GPIO call could block (waiting for the SPI bus mutex) until the buffer
>>> is disabled, which could be an indefinitely long time. And to make it
>>> even worse, this is not an interruptible wait, so the GPIO consumer
>>> would effectively be deadlocked.  
>>
>> I would say either the entire buffer mode is broken (in software), or
>> hardware is broken and GPIO shouldn't be supported at all if the
>> buffer mode is enabled. I think the best solution here is to remove
>> the GPIO chip before enabling buffered mode. If GPIO is in use, fail
>> the buffer mode.
> I'd kind of assume that anyone using these GPIOs is doing it in a fashion
> related closely to the ADC itself.
> 
> Can we make any other use fail more cleanly? 
> 
> J
>>
> 

My inclination would be to implement it like [1] where we use iio_claim_direct()
to return -EBUSY during buffered reads to avoid the deadlock-like possibility
instead of using the gpio regmap.

[1]: https://lore.kernel.org/linux-iio/2a789531fda5031c135fc207a547f2c3f00a13ea.1744325346.git.Jonathan.Santos@analog.com/

Re: [PATCH] iio: adc: ad7173: fix compiling without gpiolib

[Jonathan Cameron]

On Sat, 26 Apr 2025 17:45:02 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 4/26/25 10:18 AM, Jonathan Cameron wrote:
> > On Wed, 23 Apr 2025 00:03:38 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >   
> >> On Tue, Apr 22, 2025 at 11:12 PM David Lechner <dlechner@baylibre.com> wrote:  
> >>>  
> 
> ...
> 
> >>> Not related to the fix, but I also question the use of the regmap here.
> >>> This is one of the ad_sigma_delta drivers that does funny things with
> >>> the SPI bus, like keeping it locked during the entire time a buffer is
> >>> enabled. So, if someone tried to use a GPIO during a buffered read, the
> >>> GPIO call could block (waiting for the SPI bus mutex) until the buffer
> >>> is disabled, which could be an indefinitely long time. And to make it
> >>> even worse, this is not an interruptible wait, so the GPIO consumer
> >>> would effectively be deadlocked.    
> >>
> >> I would say either the entire buffer mode is broken (in software), or
> >> hardware is broken and GPIO shouldn't be supported at all if the
> >> buffer mode is enabled. I think the best solution here is to remove
> >> the GPIO chip before enabling buffered mode. If GPIO is in use, fail
> >> the buffer mode.  
> > I'd kind of assume that anyone using these GPIOs is doing it in a fashion
> > related closely to the ADC itself.
> > 
> > Can we make any other use fail more cleanly? 
> > 
> > J  
> >>  
> >   
> 
> My inclination would be to implement it like [1] where we use iio_claim_direct()
> to return -EBUSY during buffered reads to avoid the deadlock-like possibility
> instead of using the gpio regmap.
> 
> [1]: https://lore.kernel.org/linux-iio/2a789531fda5031c135fc207a547f2c3f00a13ea.1744325346.git.Jonathan.Santos@analog.com/

That sounds better.

J