[RFC PATCH] Input: tm2-touchkey - add hardware dependency

[RFC PATCH] Input: tm2-touchkey - add hardware dependency

[Jean Delvare]

The tm2-touchkey driver is only useful on specific platforms. Add the
missing hardware dependency so that the driver is not proposed on
systems where the device does not exist.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Fixes: 72d1f2346ded ("Input: tm2-touchkey - add touchkey driver support for TM2")
Cc: Jaechul Lee <jcsing.lee@samsung.com>
Cc: Beomho Seo <beomho.seo@samsung.com>
Cc: Javier Martinez Canillas <javier@osg.samsung.com>
Cc: Andi Shyti <andi.shyti@samsung.com>
Cc: Chanwoo Choi <cw00.choi@samsung.com>
Cc: Krzysztof Kozlowski <krzk@kernel.org>
Cc: Rob Herring <robh@kernel.org>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
Note: I am not familar with the hardware in question so I'm not sure
if this is the right dependency to add, or if ARM64 would be more
appropriate, or something else, or a combination thereof. Please
advise.

 drivers/input/keyboard/Kconfig |    1 +
 1 file changed, 1 insertion(+)

--- linux-4.11-rc8.orig/drivers/input/keyboard/Kconfig	2017-04-03 02:23:54.000000000 +0200
+++ linux-4.11-rc8/drivers/input/keyboard/Kconfig	2017-04-24 09:26:43.314252700 +0200
@@ -670,6 +670,7 @@ config KEYBOARD_TM2_TOUCHKEY
 	tristate "TM2 touchkey support"
 	depends on I2C
 	depends on LEDS_CLASS
+	depends on ARCH_EXYNOS || COMPILE_TEST
 	help
 	  Say Y here to enable device driver for tm2-touchkey with
 	  LED control for the Exynos5433 TM2 board.


-- 
Jean Delvare
SUSE L3 Support

Re: [RFC PATCH] Input: tm2-touchkey - add hardware dependency

[Krzysztof Kozlowski]

On Mon, Apr 24, 2017 at 9:42 AM, Jean Delvare <jdelvare@suse.de> wrote:
> The tm2-touchkey driver is only useful on specific platforms. Add the
> missing hardware dependency so that the driver is not proposed on
> systems where the device does not exist.

Although the device exists in only two upstreamed Exynos boards but
there is no hardware dependency on Exynos. The hardware does not
depend on Exynos.

> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Fixes: 72d1f2346ded ("Input: tm2-touchkey - add touchkey driver support for TM2")

I do not see a bug to fix. Even if we agree that driver building
should be limited to Exynos then definitely there is no missing
dependency to fix.

Best regards,
Krzysztof

> Cc: Jaechul Lee <jcsing.lee@samsung.com>
> Cc: Beomho Seo <beomho.seo@samsung.com>
> Cc: Javier Martinez Canillas <javier@osg.samsung.com>
> Cc: Andi Shyti <andi.shyti@samsung.com>
> Cc: Chanwoo Choi <cw00.choi@samsung.com>
> Cc: Krzysztof Kozlowski <krzk@kernel.org>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> Note: I am not familar with the hardware in question so I'm not sure
> if this is the right dependency to add, or if ARM64 would be more
> appropriate, or something else, or a combination thereof. Please
> advise.
>
>  drivers/input/keyboard/Kconfig |    1 +
>  1 file changed, 1 insertion(+)
>
> --- linux-4.11-rc8.orig/drivers/input/keyboard/Kconfig  2017-04-03 02:23:54.000000000 +0200
> +++ linux-4.11-rc8/drivers/input/keyboard/Kconfig       2017-04-24 09:26:43.314252700 +0200
> @@ -670,6 +670,7 @@ config KEYBOARD_TM2_TOUCHKEY
>         tristate "TM2 touchkey support"
>         depends on I2C
>         depends on LEDS_CLASS
> +       depends on ARCH_EXYNOS || COMPILE_TEST
>         help
>           Say Y here to enable device driver for tm2-touchkey with
>           LED control for the Exynos5433 TM2 board.
>
>
> --
> Jean Delvare
> SUSE L3 Support

Re: [RFC PATCH] Input: tm2-touchkey - add hardware dependency

[Jean Delvare]

Hi Krzysztof,

Thanks for your quick answer.

On Mon, 24 Apr 2017 10:00:32 +0200, Krzysztof Kozlowski wrote:
> On Mon, Apr 24, 2017 at 9:42 AM, Jean Delvare <jdelvare@suse.de> wrote:
> > The tm2-touchkey driver is only useful on specific platforms. Add the
> > missing hardware dependency so that the driver is not proposed on
> > systems where the device does not exist.
> 
> Although the device exists in only two upstreamed Exynos boards but
> there is no hardware dependency on Exynos. The hardware does not
> depend on Exynos.

I understand that, and this is the reason why there was no dependency
expressed so far. But this is irrelevant to the problem I am trying to
solve, which is that people configuring a kernel for platforms where
this device is known to NOT exist shouldn't be bothered with a question
about its driver. This is what I meant with "hardware dependency" but
you can call it "hardware focus" or "intended hardware target" if you
prefer. This is still expressed in terms of dependency in Kconfig terms
(but a soft one, thus the || COMPILE_TEST.)

> > Signed-off-by: Jean Delvare <jdelvare@suse.de>
> > Fixes: 72d1f2346ded ("Input: tm2-touchkey - add touchkey driver support for TM2")
> 
> I do not see a bug to fix. Even if we agree that driver building
> should be limited to Exynos then definitely there is no missing
> dependency to fix.

I can drop the Fixes tag, no problem.

> > Cc: Jaechul Lee <jcsing.lee@samsung.com>
> > Cc: Beomho Seo <beomho.seo@samsung.com>
> > Cc: Javier Martinez Canillas <javier@osg.samsung.com>
> > Cc: Andi Shyti <andi.shyti@samsung.com>
> > Cc: Chanwoo Choi <cw00.choi@samsung.com>
> > Cc: Krzysztof Kozlowski <krzk@kernel.org>
> > Cc: Rob Herring <robh@kernel.org>
> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> > Note: I am not familar with the hardware in question so I'm not sure
> > if this is the right dependency to add, or if ARM64 would be more
> > appropriate, or something else, or a combination thereof. Please
> > advise.
> >
> >  drivers/input/keyboard/Kconfig |    1 +
> >  1 file changed, 1 insertion(+)
> >
> > --- linux-4.11-rc8.orig/drivers/input/keyboard/Kconfig  2017-04-03 02:23:54.000000000 +0200
> > +++ linux-4.11-rc8/drivers/input/keyboard/Kconfig       2017-04-24 09:26:43.314252700 +0200
> > @@ -670,6 +670,7 @@ config KEYBOARD_TM2_TOUCHKEY
> >         tristate "TM2 touchkey support"
> >         depends on I2C
> >         depends on LEDS_CLASS
> > +       depends on ARCH_EXYNOS || COMPILE_TEST
> >         help
> >           Say Y here to enable device driver for tm2-touchkey with
> >           LED control for the Exynos5433 TM2 board.
> >
> >
> > --
> > Jean Delvare
> > SUSE L3 Support

Re: [RFC PATCH] Input: tm2-touchkey - add hardware dependency

[Krzysztof Kozlowski]

On Mon, Apr 24, 2017 at 11:48 AM, Jean Delvare <jdelvare@suse.de> wrote:
> On Mon, 24 Apr 2017 10:00:32 +0200, Krzysztof Kozlowski wrote:
> > On Mon, Apr 24, 2017 at 9:42 AM, Jean Delvare <jdelvare@suse.de> wrote:
> > > The tm2-touchkey driver is only useful on specific platforms. Add the
> > > missing hardware dependency so that the driver is not proposed on
> > > systems where the device does not exist.
> >
> > Although the device exists in only two upstreamed Exynos boards but
> > there is no hardware dependency on Exynos. The hardware does not
> > depend on Exynos.
>
> I understand that, and this is the reason why there was no dependency
> expressed so far. But this is irrelevant to the problem I am trying to
> solve, which is that people configuring a kernel for platforms where
> this device is known to NOT exist shouldn't be bothered with a question
> about its driver. This is what I meant with "hardware dependency" but
> you can call it "hardware focus" or "intended hardware target" if you
> prefer.

You need a depends-like version of "imply" keyword. I think it is
worth adding it to solve such problems and help in configuring the
system. However I am not convinced that "depends" should be used in
the meaning of "intended use".

Best regards,
Krzysztof

Re: [RFC PATCH] Input: tm2-touchkey - add hardware dependency

[Jean Delvare]

On Mon, 24 Apr 2017 11:58:09 +0200, Krzysztof Kozlowski wrote:
> On Mon, Apr 24, 2017 at 11:48 AM, Jean Delvare <jdelvare@suse.de> wrote:
> > On Mon, 24 Apr 2017 10:00:32 +0200, Krzysztof Kozlowski wrote:
> > > On Mon, Apr 24, 2017 at 9:42 AM, Jean Delvare <jdelvare@suse.de> wrote:
> > > > The tm2-touchkey driver is only useful on specific platforms. Add the
> > > > missing hardware dependency so that the driver is not proposed on
> > > > systems where the device does not exist.
> > >
> > > Although the device exists in only two upstreamed Exynos boards but
> > > there is no hardware dependency on Exynos. The hardware does not
> > > depend on Exynos.
> >
> > I understand that, and this is the reason why there was no dependency
> > expressed so far. But this is irrelevant to the problem I am trying to
> > solve, which is that people configuring a kernel for platforms where
> > this device is known to NOT exist shouldn't be bothered with a question
> > about its driver. This is what I meant with "hardware dependency" but
> > you can call it "hardware focus" or "intended hardware target" if you
> > prefer.
> 
> You need a depends-like version of "imply" keyword. I think it is
> worth adding it to solve such problems and help in configuring the
> system. However I am not convinced that "depends" should be used in
> the meaning of "intended use".

You are a bit late to the party I am afraid. COMPILE_TEST was
introduced for this very usage 4 years ago and I count 760 occurrences
of it. Not as many as I would like but I think this is going in the
right direction.

To be honest, I have also considered the possibility of a dedicated
keyword to express these "intended hardware target" soft dependencies.
Maybe it would make things clearer. But I never had the time to look
into it. Feel free to propose something if you are interested.

-- 
Jean Delvare
SUSE L3 Support

Re: [RFC PATCH] Input: tm2-touchkey - add hardware dependency

[Krzysztof Kozlowski]

On Mon, Apr 24, 2017 at 1:34 PM, Jean Delvare <jdelvare@suse.de> wrote:
> On Mon, 24 Apr 2017 11:58:09 +0200, Krzysztof Kozlowski wrote:
>> On Mon, Apr 24, 2017 at 11:48 AM, Jean Delvare <jdelvare@suse.de> wrote:
>> > On Mon, 24 Apr 2017 10:00:32 +0200, Krzysztof Kozlowski wrote:
>> > > On Mon, Apr 24, 2017 at 9:42 AM, Jean Delvare <jdelvare@suse.de> wrote:
>> > > > The tm2-touchkey driver is only useful on specific platforms. Add the
>> > > > missing hardware dependency so that the driver is not proposed on
>> > > > systems where the device does not exist.
>> > >
>> > > Although the device exists in only two upstreamed Exynos boards but
>> > > there is no hardware dependency on Exynos. The hardware does not
>> > > depend on Exynos.
>> >
>> > I understand that, and this is the reason why there was no dependency
>> > expressed so far. But this is irrelevant to the problem I am trying to
>> > solve, which is that people configuring a kernel for platforms where
>> > this device is known to NOT exist shouldn't be bothered with a question
>> > about its driver. This is what I meant with "hardware dependency" but
>> > you can call it "hardware focus" or "intended hardware target" if you
>> > prefer.
>>
>> You need a depends-like version of "imply" keyword. I think it is
>> worth adding it to solve such problems and help in configuring the
>> system. However I am not convinced that "depends" should be used in
>> the meaning of "intended use".
>
> You are a bit late to the party I am afraid. COMPILE_TEST was
> introduced for this very usage 4 years ago and I count 760 occurrences
> of it. Not as many as I would like but I think this is going in the
> right direction.

That is not the purpose of COMPILE_TEST. It serves only to allow
compile testing of everything, not selecting "soft dependencies". It
allows you to build kernel which will not even work in certain cases.

Also it does not bring any information about wanted or unwanted links
- like "imply".

> To be honest, I have also considered the possibility of a dedicated
> keyword to express these "intended hardware target" soft dependencies.
> Maybe it would make things clearer. But I never had the time to look
> into it. Feel free to propose something if you are interested.

Workaround might be using default = N, unless ARCH_EXYNOS. Something like:
default y if ARCH_EXYNOS

Best regards,
Krzysztof

Re: [RFC PATCH] Input: tm2-touchkey - add hardware dependency

[Dmitry Torokhov]

On Mon, Apr 24, 2017 at 01:56:06PM +0200, Krzysztof Kozlowski wrote:
> On Mon, Apr 24, 2017 at 1:34 PM, Jean Delvare <jdelvare@suse.de> wrote:
> > On Mon, 24 Apr 2017 11:58:09 +0200, Krzysztof Kozlowski wrote:
> >> On Mon, Apr 24, 2017 at 11:48 AM, Jean Delvare <jdelvare@suse.de> wrote:
> >> > On Mon, 24 Apr 2017 10:00:32 +0200, Krzysztof Kozlowski wrote:
> >> > > On Mon, Apr 24, 2017 at 9:42 AM, Jean Delvare <jdelvare@suse.de> wrote:
> >> > > > The tm2-touchkey driver is only useful on specific platforms. Add the
> >> > > > missing hardware dependency so that the driver is not proposed on
> >> > > > systems where the device does not exist.
> >> > >
> >> > > Although the device exists in only two upstreamed Exynos boards but
> >> > > there is no hardware dependency on Exynos. The hardware does not
> >> > > depend on Exynos.
> >> >
> >> > I understand that, and this is the reason why there was no dependency
> >> > expressed so far. But this is irrelevant to the problem I am trying to
> >> > solve, which is that people configuring a kernel for platforms where
> >> > this device is known to NOT exist shouldn't be bothered with a question
> >> > about its driver. This is what I meant with "hardware dependency" but
> >> > you can call it "hardware focus" or "intended hardware target" if you
> >> > prefer.
> >>
> >> You need a depends-like version of "imply" keyword. I think it is
> >> worth adding it to solve such problems and help in configuring the
> >> system. However I am not convinced that "depends" should be used in
> >> the meaning of "intended use".
> >
> > You are a bit late to the party I am afraid. COMPILE_TEST was
> > introduced for this very usage 4 years ago and I count 760 occurrences
> > of it. Not as many as I would like but I think this is going in the
> > right direction.
> 
> That is not the purpose of COMPILE_TEST. It serves only to allow
> compile testing of everything, not selecting "soft dependencies". It
> allows you to build kernel which will not even work in certain cases.
> 
> Also it does not bring any information about wanted or unwanted links
> - like "imply".
> 
> > To be honest, I have also considered the possibility of a dedicated
> > keyword to express these "intended hardware target" soft dependencies.
> > Maybe it would make things clearer. But I never had the time to look
> > into it. Feel free to propose something if you are interested.
> 
> Workaround might be using default = N, unless ARCH_EXYNOS. Something like:
> default y if ARCH_EXYNOS

This would strongly imply that the device is available on all Exynos
boards. I am fairly certain that our Chromebooks, for example, do not
have it.

Besides, I do not see what is wrong with driver being offered on a
platform that does not have it. This happens all the time. Consider I2C
stuff:

config I2C_I801
	tristate "Intel 82801 (ICH/PCH)"
	depends on PCI
	select CHECK_SIGNATURE if X86 && DMI
	select I2C_SMBUS

This is an X86 device, but is offered everywhere where we have PCI.

Thanks.

-- 
Dmitry

Re: [RFC PATCH] Input: tm2-touchkey - add hardware dependency

[Krzysztof Kozlowski]

On Mon, Apr 24, 2017 at 10:09:53AM -0700, Dmitry Torokhov wrote:
> On Mon, Apr 24, 2017 at 01:56:06PM +0200, Krzysztof Kozlowski wrote:
> > Workaround might be using default = N, unless ARCH_EXYNOS. Something like:
> > default y if ARCH_EXYNOS
> 
> This would strongly imply that the device is available on all Exynos
> boards. I am fairly certain that our Chromebooks, for example, do not
> have it.

A new Kconfig keyword like "usable on" would imply that as well... I do
not know how you want to limit this to only few specific boards.

> 
> Besides, I do not see what is wrong with driver being offered on a
> platform that does not have it. This happens all the time. Consider I2C
> stuff:
> 
> config I2C_I801
> 	tristate "Intel 82801 (ICH/PCH)"
> 	depends on PCI
> 	select CHECK_SIGNATURE if X86 && DMI
> 	select I2C_SMBUS
> 
> This is an X86 device, but is offered everywhere where we have PCI.

But that I2C_I801 driver has a real dependency on PCI. It just needs the
PCI. This is why you have depends for.

Other drivers who exist only on Exynos boards do not have such
dependencies. They have only real hardware or software "depends", e.g.:
config MFD_MAX77686                                                                                                                                                                      
         tristate "Maxim Semiconductor MAX77686/802 PMIC Support"
         depends on I2C
         depends on OF || COMPILE_TEST
         select MFD_CORE
         select REGMAP_I2C
         select REGMAP_IRQ
         select IRQ_DOMAIN

This driver needs I2C and OF. It does not need Exynos for anything,
although it is used only for Exynos specific boards so far.

Modeling drivers that "exist only on two Exynos based boards" through
depending on ARCH_EXYNOS is different. Not mentioning that the TM2 touch
key is in fact Cypress touchkey and could be used on other boards.

Best regards,
Krzysztof

Re: [RFC PATCH] Input: tm2-touchkey - add hardware dependency

[Jean Delvare]

Hi Dmitry,

On Mon, 24 Apr 2017 10:09:53 -0700, Dmitry Torokhov wrote:
> On Mon, Apr 24, 2017 at 01:56:06PM +0200, Krzysztof Kozlowski wrote:
> > >> > On Mon, 24 Apr 2017 10:00:32 +0200, Krzysztof Kozlowski wrote:
> > >> > > On Mon, Apr 24, 2017 at 9:42 AM, Jean Delvare <jdelvare@suse.de> wrote:
> > >> > > > The tm2-touchkey driver is only useful on specific platforms. Add the
> > >> > > > missing hardware dependency so that the driver is not proposed on
> > >> > > > systems where the device does not exist.
> > >> > >
> > >> > > Although the device exists in only two upstreamed Exynos boards but
> > >> > > there is no hardware dependency on Exynos. The hardware does not
> > >> > > depend on Exynos.
> > >> > (...)
> > Workaround might be using default = N, unless ARCH_EXYNOS. Something like:
> > default y if ARCH_EXYNOS
> 
> This would strongly imply that the device is available on all Exynos
> boards. I am fairly certain that our Chromebooks, for example, do not
> have it.
> 
> Besides, I do not see what is wrong with driver being offered on a
> platform that does not have it.

It makes the life of distribution kernel maintainers a lot harder, by
asking them questions that are irrelevant. They may eventually figure
it out and say "no", but it costs them time to investigate each option.
Worst case they will accidentally say yes and bloat their kernel.

> This happens all the time.

That doesn't make it right. The trend is changing, and you should
embrace that change. What was acceptable 10 days ago when we had 7000
kernel options no longer works today with 17000.

> Consider I2C stuff:

Trying to make it personal? ;-)

> config I2C_I801
> 	tristate "Intel 82801 (ICH/PCH)"
> 	depends on PCI
> 	select CHECK_SIGNATURE if X86 && DMI
> 	select I2C_SMBUS
> 
> This is an X86 device, but is offered everywhere where we have PCI.

In fact the 82801 family of south bridges is not limited to X86, it can
be found on IA64 hardware as well. It was discussed here:

https://patchwork.ozlabs.org/patch/635865/

and as you can see I would have liked the hardware dependency to be
added, it's Andy Shevchenko who declined :-(

-- 
Jean Delvare
SUSE L3 Support

Re: [RFC PATCH] Input: tm2-touchkey - add hardware dependency

[Jean Delvare]

On Mon, 24 Apr 2017 13:56:06 +0200, Krzysztof Kozlowski wrote:
> On Mon, Apr 24, 2017 at 1:34 PM, Jean Delvare <jdelvare@suse.de> wrote:
> > On Mon, 24 Apr 2017 11:58:09 +0200, Krzysztof Kozlowski wrote:
> >> On Mon, Apr 24, 2017 at 11:48 AM, Jean Delvare <jdelvare@suse.de> wrote:
> >> > On Mon, 24 Apr 2017 10:00:32 +0200, Krzysztof Kozlowski wrote:
> >> > > On Mon, Apr 24, 2017 at 9:42 AM, Jean Delvare <jdelvare@suse.de> wrote:
> >> > > > The tm2-touchkey driver is only useful on specific platforms. Add the
> >> > > > missing hardware dependency so that the driver is not proposed on
> >> > > > systems where the device does not exist.
> >> > >
> >> > > Although the device exists in only two upstreamed Exynos boards but
> >> > > there is no hardware dependency on Exynos. The hardware does not
> >> > > depend on Exynos.
> >> >
> >> > I understand that, and this is the reason why there was no dependency
> >> > expressed so far. But this is irrelevant to the problem I am trying to
> >> > solve, which is that people configuring a kernel for platforms where
> >> > this device is known to NOT exist shouldn't be bothered with a question
> >> > about its driver. This is what I meant with "hardware dependency" but
> >> > you can call it "hardware focus" or "intended hardware target" if you
> >> > prefer.
> >>
> >> You need a depends-like version of "imply" keyword. I think it is
> >> worth adding it to solve such problems and help in configuring the
> >> system. However I am not convinced that "depends" should be used in
> >> the meaning of "intended use".
> >
> > You are a bit late to the party I am afraid. COMPILE_TEST was
> > introduced for this very usage 4 years ago and I count 760 occurrences
> > of it. Not as many as I would like but I think this is going in the
> > right direction.
> 
> That is not the purpose of COMPILE_TEST. It serves only to allow
> compile testing of everything, not selecting "soft dependencies".

Think again and you'll realize this is exactly the same.

> It
> allows you to build kernel which will not even work in certain cases.

This is an extreme theoretical case, which has nothing to do with the
problem at hand.

> Also it does not bring any information about wanted or unwanted links
> - like "imply".

I have no idea what you mean here, sorry.

> > To be honest, I have also considered the possibility of a dedicated
> > keyword to express these "intended hardware target" soft dependencies.
> > Maybe it would make things clearer. But I never had the time to look
> > into it. Feel free to propose something if you are interested.
> 
> Workaround might be using default = N, unless ARCH_EXYNOS. Something like:
> default y if ARCH_EXYNOS

Maybe this is a good idea, maybe not, I don't know as I am not familiar
with this platform nor the ARM world in general. What I am sure of is
that this has nothing to do with the problem I was originally
discussing. I want to change the visibility on non-Exynos build, you
propose to change the default on Exynos builds. That's not helping (not
me, at least.)

-- 
Jean Delvare
SUSE L3 Support

Re: [RFC PATCH] Input: tm2-touchkey - add hardware dependency

[Krzysztof Kozlowski]

On Mon, Apr 24, 2017 at 08:49:32PM +0200, Jean Delvare wrote:
> On Mon, 24 Apr 2017 13:56:06 +0200, Krzysztof Kozlowski wrote:
> > On Mon, Apr 24, 2017 at 1:34 PM, Jean Delvare <jdelvare@suse.de> wrote:
> > > On Mon, 24 Apr 2017 11:58:09 +0200, Krzysztof Kozlowski wrote:
> > >> On Mon, Apr 24, 2017 at 11:48 AM, Jean Delvare <jdelvare@suse.de> wrote:
> > >> > On Mon, 24 Apr 2017 10:00:32 +0200, Krzysztof Kozlowski wrote:
> > > You are a bit late to the party I am afraid. COMPILE_TEST was
> > > introduced for this very usage 4 years ago and I count 760 occurrences
> > > of it. Not as many as I would like but I think this is going in the
> > > right direction.
> > 
> > That is not the purpose of COMPILE_TEST. It serves only to allow
> > compile testing of everything, not selecting "soft dependencies".
> 
> Think again and you'll realize this is exactly the same.

No, the purpose of COMPILE_TEST is to build everything whenever it is
possible. Regardless if it is reasonable or not. For example without OF
some drivers will not be able to bind but we can build them for build
coverage (and static checkers coverage). That is the goal.

However you want to limit the driver because of specific board existing
in mainline (have you considered out of tree boards?). That is totally
different meaning and use case.

> > It
> > allows you to build kernel which will not even work in certain cases.
> 
> This is an extreme theoretical case, which has nothing to do with the
> problem at hand.

That is the effect of COMPILE_TEST for which it was designed... but you
want to use it in different way.

> 
> > Also it does not bring any information about wanted or unwanted links
> > - like "imply".
> 
> I have no idea what you mean here, sorry.

The depends/select/imply bring an information to the user about
relationship between the Kconfig symbols.

COMPILE_TEST does not bring such relationship.

> 
> > > To be honest, I have also considered the possibility of a dedicated
> > > keyword to express these "intended hardware target" soft dependencies.
> > > Maybe it would make things clearer. But I never had the time to look
> > > into it. Feel free to propose something if you are interested.
> > 
> > Workaround might be using default = N, unless ARCH_EXYNOS. Something like:
> > default y if ARCH_EXYNOS
> 
> Maybe this is a good idea, maybe not, I don't know as I am not familiar
> with this platform nor the ARM world in general. What I am sure of is
> that this has nothing to do with the problem I was originally
> discussing. I want to change the visibility on non-Exynos build, you
> propose to change the default on Exynos builds. That's not helping (not
> me, at least.)

First of all, I am not changing the defaults. Second, you asked for a
hint of a better solution and now you are complaining that you do not
like it... okay, as you wish.

The other way is to introduce a new soft dependency called whatever you
like to (e.g. "usable on").

But depends and COMPILE_TEST are not for that purpose.

Best regards,
Krzysztof

Re: [RFC PATCH] Input: tm2-touchkey - add hardware dependency

[Jean Delvare]

Hi Krzysztof,

I'll try to stay calm but no promise :-D

On Mon, 24 Apr 2017 20:57:42 +0200, Krzysztof Kozlowski wrote:
> On Mon, Apr 24, 2017 at 08:49:32PM +0200, Jean Delvare wrote:
> > On Mon, 24 Apr 2017 13:56:06 +0200, Krzysztof Kozlowski wrote:
> > > On Mon, Apr 24, 2017 at 1:34 PM, Jean Delvare <jdelvare@suse.de> wrote:
> > > > On Mon, 24 Apr 2017 11:58:09 +0200, Krzysztof Kozlowski wrote:
> > > >> On Mon, Apr 24, 2017 at 11:48 AM, Jean Delvare <jdelvare@suse.de> wrote:
> > > > You are a bit late to the party I am afraid. COMPILE_TEST was
> > > > introduced for this very usage 4 years ago and I count 760 occurrences
> > > > of it. Not as many as I would like but I think this is going in the
> > > > right direction.
> > > 
> > > That is not the purpose of COMPILE_TEST. It serves only to allow
> > > compile testing of everything, not selecting "soft dependencies".
> > 
> > Think again and you'll realize this is exactly the same.
> 
> No, the purpose of COMPILE_TEST is to build everything whenever it is
> possible. Regardless if it is reasonable or not. For example without OF
> some drivers will not be able to bind but we can build them for build
> coverage (and static checkers coverage). That is the goal.

I will concede that the short description of COMPILE_TEST ("Compile
also drivers which will not load") can be misleading. It's not only
about "not loading." But if you take a look at the help text, it is
more verbose and accurate:

"Some drivers can be compiled on a different platform than they are
intended to be run on. Despite they cannot be loaded there (or even
when they load they cannot be used due to missing HW support),
developers still, opposing to distributors, might want to build such
drivers to compile-test them.

If you are a developer and want to build everything available, say Y
here. If you are a user/distributor, say N here to exclude useless
drivers to be distributed."

I give you a few minutes to read the whole thing.

Please pay attention to "due to missing HW support", "opposing to
distributors" and "say N here to exclude useless drivers". So, to
summarize, the purpose of COMPILE_TEST is to help distribution kernel
maintainers exclude useless drivers while still allowing a maximum
build testing coverage of such drivers.

What I am trying to do here is to help distribution kernel maintainers
exclude the tm2 touchkey driver from kernel flavors where it would be
useless (this is "depends on ARCH_EXYNOS" in my patch) while not
hurting build test coverage (this is "|| COMPILE_TEST" in my patch.)
Therefore I am clearly using COMPILE_TEST for its intended purpose. QED.

> However you want to limit the driver because of specific board existing
> in mainline (have you considered out of tree boards?). That is totally
> different meaning and use case.

I never mentioned specific boards. I proposed ARCH_EXYNOS, which to the
best of my knowledge is a sub-architecture or platform if you will.
Additionally, if you read my original post again, you will notice that
I explicitly said that I was not certain if it was the most appropriate
"level" of visibility for this driver. What seems clear to me is that
proposing this driver when I build an X86 kernel does not make sense.

> > > (...) It
> > > allows you to build kernel which will not even work in certain cases.
> > 
> > This is an extreme theoretical case, which has nothing to do with the
> > problem at hand.
> 
> That is the effect of COMPILE_TEST for which it was designed...
> but you want to use it in different way.

No. I invite you to read the help text of COMPILE_TEST again. It was
not designed to build kernels which will not work. Nobody needed that.

> (...)
> The depends/select/imply bring an information to the user about
> relationship between the Kconfig symbols.
> 
> COMPILE_TEST does not bring such relationship.

I thank you for trying to clarify what you meant, but even after
reading it 3 times, I still have no idea what you mean. Honestly.

> > > (...)
> > > Workaround might be using default = N, unless ARCH_EXYNOS. Something like:
> > > default y if ARCH_EXYNOS
>
> First of all, I am not changing the defaults.

Oh well, what can I say. I think I'll let you keep arguing with
yourself. This will save me a lot of time to devote to other tasks with
higher chances of success.

> Second, you asked for a
> hint of a better solution and now you are complaining that you do not
> like it... okay, as you wish.

I asked for suggestions on a specific point. Let me quote myself:

"I'm not sure if this is the right dependency to add, or if ARM64
would be more appropriate, or something else (...) Please advise."

I did not ask for a lesson about what COMPILE_TEST is meant for, nor
for suggestions of what the default value should be. I asked for advice
on what the visibility should be.

> The other way is to introduce a new soft dependency called whatever you
> like to (e.g. "usable on").

Again, this may be a good idea that can be discussed, but that keyword
does not exist today, and regular depends + COMPILE_TEST have been used
for the purpose since 2013. Feel free to start a separate discussion
thread about introducing "usable on" as a replacement for COMPILE_TEST,
maybe with a proof of concept, and see what people think about your
idea.

> But depends and COMPILE_TEST are not for that purpose.

... *sigh*.

This is my last reply to you on this topic, as we are clearly running
in circles.

-- 
Jean Delvare
SUSE L3 Support

Re: [RFC PATCH] Input: tm2-touchkey - add hardware dependency

[Andi Shyti]

Hi Jean,

On Mon, Apr 24, 2017 at 09:42:31AM +0200, Jean Delvare wrote:
> The tm2-touchkey driver is only useful on specific platforms. Add the
> missing hardware dependency so that the driver is not proposed on
> systems where the device does not exist.

I'm sorry, I don't see any connection between Exynos and the
TM2-touchkey (which is manufactured by cypress, btw).

It surely does have connections with the TM2 board which happens
to have an Exynos SoC, but that cannot be fixed from the Kconfig.

Thanks,
Andi

Re: [RFC PATCH] Input: tm2-touchkey - add hardware dependency

[Jean Delvare]

Hi Andi,

On Tue, 25 Apr 2017 11:28:17 +0900, Andi Shyti wrote:
> On Mon, Apr 24, 2017 at 09:42:31AM +0200, Jean Delvare wrote:
> > The tm2-touchkey driver is only useful on specific platforms. Add the
> > missing hardware dependency so that the driver is not proposed on
> > systems where the device does not exist.
> 
> I'm sorry, I don't see any connection between Exynos and the
> TM2-touchkey (which is manufactured by cypress, btw).
> 
> It surely does have connections with the TM2 board which happens
> to have an Exynos SoC, but that cannot be fixed from the Kconfig.

Thanks for your constructive contribution to this discussion.

The KEYBOARD_TM2_TOUCHKEY option name, description and help text as
well as the driver name all make it sound highly hardware specific. If
this driver is for a "generic" part from Cypress then the driver should
be renamed and the Kconfig option name and description should be
updated to reflect that. The help text should also first mention the
chip vendor and name, and only then mention that it is used on the
Exynos5433 TM2 board.

That being said, I took a look at the driver, and while "cypress" is
indeed being mentioned as the vendor name, the chip name itself is
referred to as "tm2-touchkey", which isn't generic at all. You can't
identify the hardware under the name "tm2 touchkey" everywhere in the
driver and expect people to understand it is not board specific.

So it really boils down to this question: is that chip a generic part
from Cypress, and if so, what is the real part number? Or was is
designed privately by Cypress specifically for Samsung for this one
board (and possibly others to come)?

Thanks again,
-- 
Jean Delvare
SUSE L3 Support

Re: [RFC PATCH] Input: tm2-touchkey - add hardware dependency

[Andi Shyti]

Hi Jean,

> > On Mon, Apr 24, 2017 at 09:42:31AM +0200, Jean Delvare wrote:
> > > The tm2-touchkey driver is only useful on specific platforms. Add the
> > > missing hardware dependency so that the driver is not proposed on
> > > systems where the device does not exist.
> > 
> > I'm sorry, I don't see any connection between Exynos and the
> > TM2-touchkey (which is manufactured by cypress, btw).
> > 
> > It surely does have connections with the TM2 board which happens
> > to have an Exynos SoC, but that cannot be fixed from the Kconfig.
> 
> Thanks for your constructive contribution to this discussion.
> 
> The KEYBOARD_TM2_TOUCHKEY option name, description and help text as
> well as the driver name all make it sound highly hardware specific. If
> this driver is for a "generic" part from Cypress then the driver should
> be renamed and the Kconfig option name and description should be
> updated to reflect that. The help text should also first mention the
> chip vendor and name, and only then mention that it is used on the
> Exynos5433 TM2 board.
> 
> That being said, I took a look at the driver, and while "cypress" is
> indeed being mentioned as the vendor name, the chip name itself is
> referred to as "tm2-touchkey", which isn't generic at all. You can't
> identify the hardware under the name "tm2 touchkey" everywhere in the
> driver and expect people to understand it is not board specific.
> 
> So it really boils down to this question: is that chip a generic part
> from Cypress, and if so, what is the real part number? Or was is
> designed privately by Cypress specifically for Samsung for this one
> board (and possibly others to come)?

I knew that the naming was bringing confusion and we had a
previous discussion about it with Chanwoo [1].

This is indeed a generic device from Cypress. The driver has been
ported from Android's Kernel [2]; it says that the device
part is cy8cmbr3xxx, but the datasheet [3] doesn't have any
connection with what the TM2 board has (i.e. the registers don't
match). That's why we suspected that (as you said) this might be
a touch key sensor specifically designed for the TM2 board.

Cypress was not that helpful.

The alternative was to not provide support, but it didn't look
right.

Thanks,
Andi

[1] https://marc.info/?l=linux-kernel&m=148374097826303&w=2
[2] http://opensource.samsung.com/reception/receptionSub.do?method=sub&sub=F&searchValue=sm-n910K
[3] http://www.cypress.com/documentation/datasheets/cy8cmbr3002-cy8cmbr3102-cy8cmbr3106s-cy8cmbr3108-cy8cmbr3110-cy8cmbr3116

Re: [RFC PATCH] Input: tm2-touchkey - add hardware dependency

[Dmitry Torokhov]

On Tue, Apr 25, 2017 at 08:00:56PM +0900, Andi Shyti wrote:
> Hi Jean,
> 
> > > On Mon, Apr 24, 2017 at 09:42:31AM +0200, Jean Delvare wrote:
> > > > The tm2-touchkey driver is only useful on specific platforms. Add the
> > > > missing hardware dependency so that the driver is not proposed on
> > > > systems where the device does not exist.
> > > 
> > > I'm sorry, I don't see any connection between Exynos and the
> > > TM2-touchkey (which is manufactured by cypress, btw).
> > > 
> > > It surely does have connections with the TM2 board which happens
> > > to have an Exynos SoC, but that cannot be fixed from the Kconfig.
> > 
> > Thanks for your constructive contribution to this discussion.
> > 
> > The KEYBOARD_TM2_TOUCHKEY option name, description and help text as
> > well as the driver name all make it sound highly hardware specific. If
> > this driver is for a "generic" part from Cypress then the driver should
> > be renamed and the Kconfig option name and description should be
> > updated to reflect that. The help text should also first mention the
> > chip vendor and name, and only then mention that it is used on the
> > Exynos5433 TM2 board.
> > 
> > That being said, I took a look at the driver, and while "cypress" is
> > indeed being mentioned as the vendor name, the chip name itself is
> > referred to as "tm2-touchkey", which isn't generic at all. You can't
> > identify the hardware under the name "tm2 touchkey" everywhere in the
> > driver and expect people to understand it is not board specific.
> > 
> > So it really boils down to this question: is that chip a generic part
> > from Cypress, and if so, what is the real part number? Or was is
> > designed privately by Cypress specifically for Samsung for this one
> > board (and possibly others to come)?
> 
> I knew that the naming was bringing confusion and we had a
> previous discussion about it with Chanwoo [1].
> 
> This is indeed a generic device from Cypress. The driver has been
> ported from Android's Kernel [2]; it says that the device
> part is cy8cmbr3xxx, but the datasheet [3] doesn't have any
> connection with what the TM2 board has (i.e. the registers don't
> match). That's why we suspected that (as you said) this might be
> a touch key sensor specifically designed for the TM2 board.
> 
> Cypress was not that helpful.
> 
> The alternative was to not provide support, but it didn't look
> right.

So this looks like we are dealing with a device designed for a specific
board, but not architecture or board-specific. Similar to
atmel_captouch, ims_pcu, or all drivers living in platform/x86 (I
understand that they do not bother Jean simply because he cares mostly
about x86 and, with SUSE being generic distro, he needs to enable all
the drivers there anyway, but a person configuring "their" kernel still
has to go and consider all options).

I am all for adding dependencies for drivers that are parts of
particular SoCs (you probably not going to have Broardcom IPROC
touchscreen on your x86 device), but external to SoC peripherals is a
different story.

Thanks.

-- 
Dmitry

Re: [RFC PATCH] Input: tm2-touchkey - add hardware dependency

[Jean Delvare]

Hi Dmitry,

On Tue, 25 Apr 2017 10:28:09 -0700, Dmitry Torokhov wrote:
> So this looks like we are dealing with a device designed for a specific
> board, but not architecture or board-specific. Similar to
> atmel_captouch, ims_pcu, or all drivers living in platform/x86 (I
> understand that they do not bother Jean simply because he cares mostly
> about x86 and, with SUSE being generic distro, he needs to enable all
> the drivers there anyway, but a person configuring "their" kernel still
> has to go and consider all options).

Actually they do bother me at times (some of them can't be built as
modules.) Also while I indeed care mostly about x86, openSUSE supports
various other architectures, so I would be equally worried about
x86-specific drivers being proposed on these architectures, for
example. The only difference is that I typically do not catch these
myself.

> I am all for adding dependencies for drivers that are parts of
> particular SoCs (you probably not going to have Broardcom IPROC
> touchscreen on your x86 device), but external to SoC peripherals is a
> different story.

I understand that there is no direct relation between this TM2 touchkey
driver and Exynos as a platform.

My reasoning is as follows: given that at this point this driver is
only useful on the Samsung TM2 board, it should only be presented to
users configuring a kernel for that board. Then the question becomes:
what other kernel option will definitely be enabled on any kernel
running on that board? As the TM2 is based on an Exynos5433 SoC,
ARCH_EXYNOS will have to be enabled, so it seems a convenient way to
limit the visibility of KEYBOARD_TM2_TOUCHKEY.

There are certainly other options that will also be always enabled for
a TM2 board kernel, but ARCH_EXYNOS seems to be a good choice because
it is both generic enough that it doesn't need to be changed if a
variation of the TM2 board is released using a slightly different SoC,
and specific enough that we will skip the question for most users.

The alternative would be to add another option SAMSUNG_TM2 ("Support for
the Samsung TM2 board"), just to let KEYBOARD_TM2_TOUCHKEY depend on it,
but that's really only moving the problem, as there is no point in
asking people about SAMSUNG_TM2 if they are building a kernel that can't
support it anyway. And while I agree it would be somewhat cleaner to
have KEYBOARD_TM2_TOUCHKEY which depends on SAMSUNG_TM2 and SAMSUNG_TM2
which depends on ARCH_EXYNOS, it also seems overly complex to me for no
benefit.

Therefore I still believe that making KEYBOARD_TM2_TOUCHKEY depend on
ARCH_EXYNOS || COMPILE_TEST makes sense from a practical perspective.

-- 
Jean Delvare
SUSE L3 Support

Re: [RFC PATCH] Input: tm2-touchkey - add hardware dependency

[Krzysztof Kozlowski]

On Wed, May 3, 2017 at 11:42 AM, Jean Delvare <jdelvare@suse.de> wrote:
>> I am all for adding dependencies for drivers that are parts of
>> particular SoCs (you probably not going to have Broardcom IPROC
>> touchscreen on your x86 device), but external to SoC peripherals is a
>> different story.
>
> I understand that there is no direct relation between this TM2 touchkey
> driver and Exynos as a platform.
>
> My reasoning is as follows: given that at this point this driver is
> only useful on the Samsung TM2 board, it should only be presented to
> users configuring a kernel for that board. Then the question becomes:
> what other kernel option will definitely be enabled on any kernel
> running on that board? As the TM2 is based on an Exynos5433 SoC,
> ARCH_EXYNOS will have to be enabled, so it seems a convenient way to
> limit the visibility of KEYBOARD_TM2_TOUCHKEY.
>
> There are certainly other options that will also be always enabled for
> a TM2 board kernel, but ARCH_EXYNOS seems to be a good choice because
> it is both generic enough that it doesn't need to be changed if a
> variation of the TM2 board is released using a slightly different SoC,
> and specific enough that we will skip the question for most users.
>
> The alternative would be to add another option SAMSUNG_TM2 ("Support for
> the Samsung TM2 board"), just to let KEYBOARD_TM2_TOUCHKEY depend on it,
> but that's really only moving the problem, as there is no point in
> asking people about SAMSUNG_TM2 if they are building a kernel that can't
> support it anyway. And while I agree it would be somewhat cleaner to
> have KEYBOARD_TM2_TOUCHKEY which depends on SAMSUNG_TM2 and SAMSUNG_TM2
> which depends on ARCH_EXYNOS, it also seems overly complex to me for no
> benefit.
>
> Therefore I still believe that making KEYBOARD_TM2_TOUCHKEY depend on
> ARCH_EXYNOS || COMPILE_TEST makes sense from a practical perspective.

As I mentioned at beginning, I limiting unusable options looks worth
adding to the kernel as a new keyword. However dependency is not for
that purpose because this is not a hardware/software dependency. You
need to add something similar to "imply" keyword, but working not for
select statements but for dependencies. I think such new keyword would
be useful not only for Suse kernels but for many others as well.

But if you do not wish to do it right, then please do not go with this
workaround making a hard (static) dependency between some driver and
some ARM architecture.

On the other hand, maybe this particular driver problem for you can be
solved by removing i2c device binding and adding dependency on OF.
Assuming of course, you do not choose OF for your kernels.

Best regards,
Krzysztof

Re: [RFC PATCH] Input: tm2-touchkey - add hardware dependency

[Jean Delvare]

Hi Andi,

On Tue, 25 Apr 2017 20:00:56 +0900, Andi Shyti wrote:
> > > On Mon, Apr 24, 2017 at 09:42:31AM +0200, Jean Delvare wrote:
> > So it really boils down to this question: is that chip a generic part
> > from Cypress, and if so, what is the real part number? Or was is
> > designed privately by Cypress specifically for Samsung for this one
> > board (and possibly others to come)?
> 
> I knew that the naming was bringing confusion and we had a
> previous discussion about it with Chanwoo [1].
>
> This is indeed a generic device from Cypress. The driver has been
> ported from Android's Kernel [2]; it says that the device
> part is cy8cmbr3xxx, but the datasheet [3] doesn't have any
> connection with what the TM2 board has (i.e. the registers don't
> match). That's why we suspected that (as you said) this might be
> a touch key sensor specifically designed for the TM2 board.

Thanks for the pointers, it helps.

> Cypress was not that helpful.

I've been there before with other manufacturers. Chipsets designed
specifically for one hardware vendor are the hardest to support for this
reason.

> The alternative was to not provide support, but it didn't look
> right.

I agree, you did the right thing by getting the driver upstream. But it
doesn't mean this driver must be enabled in all distribution kernels.

-- 
Jean Delvare
SUSE L3 Support