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

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

[Peter Robinson]

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.

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)
 
 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
-- 
2.13.0

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

[Heikki Krogerus]

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.
> 
> 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)
>  
>  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,

-- 
heikki

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

[Andy Shevchenko]

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.

> > 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?

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

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?

> >  
> >  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

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

[Peter Robinson]

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

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

[Linus Walleij]

On Tue, Jul 4, 2017 at 8:49 AM, Peter Robinson <pbrobinson@gmail.com> 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.
>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Signed-off-by: Peter Robinson <pbrobinson@gmail.com>

I'm waiting for Mika to say what he thinks about this. He's the overall
Intel pin control maintainer.

Yours,
Linus Walleij

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

[Mika Westerberg]

On Mon, Jul 31, 2017 at 03:41:16PM +0200, Linus Walleij wrote:
> On Tue, Jul 4, 2017 at 8:49 AM, Peter Robinson <pbrobinson@gmail.com> 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.
> >
> > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Signed-off-by: Peter Robinson <pbrobinson@gmail.com>
> 
> I'm waiting for Mika to say what he thinks about this. He's the overall
> Intel pin control maintainer.

Looks fine to me,

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

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

[Linus Walleij]

On Tue, Jul 4, 2017 at 8:49 AM, Peter Robinson <pbrobinson@gmail.com> 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.
>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Signed-off-by: Peter Robinson <pbrobinson@gmail.com>

Patch applied with the ACKs.

Yours,
Linus Walleij

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

[Andy Shevchenko]

On Wed, 2017-07-05 at 09:01 +0100, Peter Robinson wrote:
> 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:

> > > > --- 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.

ARM world is too fragmented. I can't take it as a good example.
Most of distros would like to maintain less kernels (ideally one per
architecture). In the above example it's a sub-arch.

Yes, I know that x86 has 3 let's say "sub-arches" which require
different settings to kernel. (None of them makes difference to pin
control case though)

> > 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.

Okay, so, what's wrong with defining big blocks on per ARCH basis as I
pointed above?

ARM, ARM64, X86, etc.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy