Re: [PATCH] pinctrl: intel: wrap Intel pin control drivers in an architecture check

[Peter Robinson <pbrobinson@gmail.com>]

On Tue, Jul 4, 2017 at 11:21 AM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Tue, 2017-07-04 at 12:54 +0300, Heikki Krogerus wrote:
>> On Tue, Jul 04, 2017 at 07:49:47AM +0100, Peter Robinson wrote:
>> > The Intel pin control drivers are architecture specific so add an if
>> > arch
>> > to check for X86 or compile test to ensure continued test coverage.
>> >
>
> Sorry, have not seen the original mail.

https://marc.info/?l=linux-gpio&m=149915099506284&w=3

>> > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
>> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>> > Signed-off-by: Peter Robinson <pbrobinson@gmail.com>
>> > ---
>> >  drivers/pinctrl/intel/Kconfig | 3 +++
>> >  1 file changed, 3 insertions(+)
>> >
>> > diff --git a/drivers/pinctrl/intel/Kconfig
>> > b/drivers/pinctrl/intel/Kconfig
>> > index 396830a41127..0c7edc321415 100644
>> > --- a/drivers/pinctrl/intel/Kconfig
>> > +++ b/drivers/pinctrl/intel/Kconfig
>> > @@ -1,6 +1,7 @@
>> >  #
>> >  # Intel pin control drivers
>> >  #
>> > +if (X86 || COMPILE_TEST)
>
> And what about ARM et al. architectures?

If you look in the various sub directories you'll see that the various
arch sub directories have similar for their SoC eg ARCH_SUNXI or
explicit depends on the SoC achieving the same outcome.

> Instead I would propose to reorganize parent Kconfig to have something
> like

Well they're done in alphabetical and there's appropriate depends
ARCH_ or SOC_ etc so that those architectures don't randomly pop up in
configs for other unrelated things, the intel/ one is one of the only
ones that doesn't do this (hence this patch)

> if (ARM || COMPILE_TEST)
> ...ARM stuff...
> endif
>
> if (X86 || COMPILE_TEST)
> ...X86 stuff...
> endif
>
> But personally I don't like any of the above. So, what's the issue this
> patch is targeting against?

So that every time (in my case a distro) they don't have to explicitly
have unrelated "# CONFIG_PINCTRL_CHERRYVIEW is not set" style bits
through all their configs because it's completely unrelated to the
platform.

>> >
>> >  config PINCTRL_BAYTRAIL
>> >     bool "Intel Baytrail GPIO pin control"
>> > @@ -72,3 +73,5 @@ config PINCTRL_SUNRISEPOINT
>> >       Sunrisepoint is the PCH of Intel Skylake. This pinctrl
>> > driver
>> >       provides an interface that allows configuring of PCH pins
>> > and
>> >       using them as GPIOs.
>> > +
>> > +endif
>>
>> OK by me:
>>
>> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>
>>
>> Thanks,
>>
>
> --
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy