| Message ID | 20170424094231.435f82de@endymion (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.)
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 :-(
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.
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,
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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.
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- 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.
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(+)