Re: [PATCH v2 2/2] iio: adc: ad7173: add AD7173 driver

[Arnd Bergmann]

On Tue, Oct 3, 2023, at 13:02, Andy Shevchenko wrote:
> On Tue, Oct 3, 2023 at 1:57 PM Ceclan Dumitru-Ioan
> <mitrutzceclan@xxxxxxxxx> wrote:
>> On 10/3/23 13:45, Andy Shevchenko wrote:
>> > On Tue, Oct 03, 2023 at 01:33:36PM +0300, Ceclan Dumitru-Ioan wrote:
>> >> On 9/30/23 17:05, Jonathan Cameron wrote:
>> >>>>
>> >>>> +  select GPIO_REGMAP
>> >>> If you are selecting it, why does it have if guards in the driver.
>> >>> I prefer the select here, so drop this if guards.
>> >> From what i checked, selecting GPIO_REGMAP does not select GPIOLIB but only REGMAP.

I think the correct solution for this is to use

      select GPIO_REGMAP if GPIOLIB

which matches what the driver does. 

>> >> Also, in the thread from V1 Arnd Bergmann suggested:
>> >>      " I think the best way to handle these is to remove both
>> >>       the 'select' and the #ifdef in the driver and instead use
>> >>       'if (IS_ENABLED(CONFIG_GPIOLIB))' to handle optional gpio
>> >>       providers in the code. "
>> > Why not simply to be dependent on GPIOLIB like other drivers do in this folder?

Some (possibly all) of the other drivers are gpiolib users that do not
function without gpiolib. This one is only a provider and not a consumer,
and the gpiolib functionality in it is optional, so I think there is
technically no dependency, even if in practice gpiolib is always there.

>> I followed the suggestion given by Arnd. The full argument:
>>
>> "From a Kconfig perspective, any user-visible symbol ideally only uses
>> 'depends on', while hidden symbols usually use 'select'.
>>
>> For the GPIOLIB symbol specifically, we have a mix of both, but the
>> overall usage is that gpio consumers only use 'depends on',
>> while some of the providers use 'select'. This risks causing build
>> breakage from a dependency loop when combined with other symbols
>> that have the same problem (e.g. I2C), but it tends to work out
>> as long as a strong hierarchy is kept. In particular, using 'select'
>> from an arch/*/Kconfig platform option is generally harmless as
>> long as those don't depend on anything else.
>>
>> The new driver is a gpio provider and at least ad4130 and
>> ad5592r uses 'select' here, but then again ad74115 and
>> ad74113 use 'depends on' and ads7950 uses neither.
>>
>> I think the best way to handle these is to remove both
>> the 'select' and the #ifdef in the driver and instead use
>> 'if (IS_ENABLED(CONFIG_GPIOLIB))' to handle optional gpio
>> providers in the code."
>>
>> I do not have a lot of experience with this subject.
>> As such, if you consider the argument invalid, mention it and i will
>> change to 'depends on'.
>
> I see, I would ask GPIOLIB maintainers about this.
> I don't know if there is any plan to fix this through the entire
> kernel and which way has been chosen for that.

I think the way we have handled it in the past is to not touch
existing drivers unless there is a problem with circular dependencies,
and then we change a bunch of 'select' to 'depends on' to make
it work again.

Changing all the wrong ones at once would be nice, but likely
cause introduce other dependency problems, but circular dependencies
and incorrect defaults in defconfig builds.

In the long run, we might decide to make gpiolib unconditional
all architectures, but for the moment that causes bloat on
small memory configurations like most m68k or armv7-m.

     Arnd