[PATCH] platform/chrome: Make CROS_EC_PROTO a user selectable option

[PATCH] platform/chrome: Make CROS_EC_PROTO a user selectable option

[Javier Martinez Canillas]

The boolean CROS_EC_PROTO symbol is selected by MFD_CROS_EC but that can
cause Kconfig circular dependencies so is better to change the select to
a depends on. But in order to be able to change that, the CROS_EC_PROTO
symbol has to be one that can be selected by the user.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---
 drivers/platform/chrome/Kconfig | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
index 3271cd1abe7c..a73b823d4967 100644
--- a/drivers/platform/chrome/Kconfig
+++ b/drivers/platform/chrome/Kconfig
@@ -59,8 +59,9 @@ config CROS_EC_LPC
           module will be called cros_ec_lpc.
 
 config CROS_EC_PROTO
-        bool
+        bool "ChromeOS EC communication protocol helpers"
         help
-          ChromeOS EC communication protocol helpers.
+          Say Y here to enable support for the different communication
+          protocols used to talk with the ChromeOS embedded controller.
 
 endif # CHROMEOS_PLATFORMS
-- 
2.4.3

Re: [PATCH] platform/chrome: Make CROS_EC_PROTO a user selectable option

[Bartlomiej Zolnierkiewicz]

Hi,

On Tuesday, August 18, 2015 09:15:06 AM Javier Martinez Canillas wrote:
> The boolean CROS_EC_PROTO symbol is selected by MFD_CROS_EC but that can
> cause Kconfig circular dependencies so is better to change the select to

Could you please give a reference to the problem or error message
that you're getting (I was not following the previous discussion).

> a depends on. But in order to be able to change that, the CROS_EC_PROTO
> symbol has to be one that can be selected by the user.

Looking at the code behind the config option it seems that it is just
a helper library and should not be made user-visible.  Why can't
the issue be fixed the other way (make other config options select
CROS_EC_PROTO consistently)?

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
>  drivers/platform/chrome/Kconfig | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
> index 3271cd1abe7c..a73b823d4967 100644
> --- a/drivers/platform/chrome/Kconfig
> +++ b/drivers/platform/chrome/Kconfig
> @@ -59,8 +59,9 @@ config CROS_EC_LPC
>            module will be called cros_ec_lpc.
>  
>  config CROS_EC_PROTO
> -        bool
> +        bool "ChromeOS EC communication protocol helpers"
>          help
> -          ChromeOS EC communication protocol helpers.
> +          Say Y here to enable support for the different communication
> +          protocols used to talk with the ChromeOS embedded controller.
>  
>  endif # CHROMEOS_PLATFORMS

Re: [PATCH] platform/chrome: Make CROS_EC_PROTO a user selectable option

[Javier Martinez Canillas]

Hello Bartlomiej,

Thanks a lot for your feedback.

On 08/18/2015 02:40 PM, Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Tuesday, August 18, 2015 09:15:06 AM Javier Martinez Canillas wrote:
>> The boolean CROS_EC_PROTO symbol is selected by MFD_CROS_EC but that can
>> cause Kconfig circular dependencies so is better to change the select to
> 
> Could you please give a reference to the problem or error message
> that you're getting (I was not following the previous discussion).
>

Yes, the original Kconfig warning message was:

warning: (MFD_CROS_EC) selects CHROME_PLATFORMS which has unmet direct dependencies (X86 || ARM)

Paul fixed on [0] by making MFD_CROS_EC depends on X86 || ARM but that
is not really true since the driver could be used in other platforms so
I tried to instead fix it by removing unneeded dependencies in [1] but
one of the patches was nacked by Geert [2] who mentioned the issue about
mixing select and "depends on".

>> a depends on. But in order to be able to change that, the CROS_EC_PROTO
>> symbol has to be one that can be selected by the user.
> 
> Looking at the code behind the config option it seems that it is just
> a helper library and should not be made user-visible.  Why can't

Yeah, I in fact used as a reference the DRM/KMS helpers (DRM_KMS_HELPER)
that is selected by drivers instead of depending on it.

> the issue be fixed the other way (make other config options select
> CROS_EC_PROTO consistently)?
>

That is another option indeed. I thought that the use of select was
discouraged and that it was preferable to use "depends on" even when
the Kconfig symbol is a boolean to enable support for some helpers.

But I don't really have a strong opinion on either approach tbh.
 
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
> 

[0]: https://lkml.org/lkml/2015/6/20/219
[1]: https://lkml.org/lkml/2015/6/24/689
[2]: https://lkml.org/lkml/2015/8/17/103

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH] platform/chrome: Make CROS_EC_PROTO a user selectable option

[Bartlomiej Zolnierkiewicz]

Hi,

On Tuesday, August 18, 2015 03:02:26 PM Javier Martinez Canillas wrote:
> Hello Bartlomiej,
> 
> Thanks a lot for your feedback.
> 
> On 08/18/2015 02:40 PM, Bartlomiej Zolnierkiewicz wrote:
> > 
> > Hi,
> > 
> > On Tuesday, August 18, 2015 09:15:06 AM Javier Martinez Canillas wrote:
> >> The boolean CROS_EC_PROTO symbol is selected by MFD_CROS_EC but that can
> >> cause Kconfig circular dependencies so is better to change the select to
> > 
> > Could you please give a reference to the problem or error message
> > that you're getting (I was not following the previous discussion).
> >
> 
> Yes, the original Kconfig warning message was:
> 
> warning: (MFD_CROS_EC) selects CHROME_PLATFORMS which has unmet direct dependencies (X86 || ARM)
> 
> Paul fixed on [0] by making MFD_CROS_EC depends on X86 || ARM but that
> is not really true since the driver could be used in other platforms so
> I tried to instead fix it by removing unneeded dependencies in [1] but
> one of the patches was nacked by Geert [2] who mentioned the issue about
> mixing select and "depends on".

I think that Geert's comments are valid.  Dependencies should limit
the config option to the archs for which the driver is really needed
(the rest can be handled with COMPILE_TEST dependency).

> >> a depends on. But in order to be able to change that, the CROS_EC_PROTO
> >> symbol has to be one that can be selected by the user.
> > 
> > Looking at the code behind the config option it seems that it is just
> > a helper library and should not be made user-visible.  Why can't
> 
> Yeah, I in fact used as a reference the DRM/KMS helpers (DRM_KMS_HELPER)
> that is selected by drivers instead of depending on it.
> 
> > the issue be fixed the other way (make other config options select
> > CROS_EC_PROTO consistently)?
> >
> 
> That is another option indeed. I thought that the use of select was
> discouraged and that it was preferable to use "depends on" even when
> the Kconfig symbol is a boolean to enable support for some helpers.

No, this is not true.  It is just that "select" has to be used with
care. ;)

> But I don't really have a strong opinion on either approach tbh.

Please rework your patches to use "select".

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
 
> > Best regards,
> > --
> > Bartlomiej Zolnierkiewicz
> > Samsung R&D Institute Poland
> > Samsung Electronics
> > 
> 
> [0]: https://lkml.org/lkml/2015/6/20/219
> [1]: https://lkml.org/lkml/2015/6/24/689
> [2]: https://lkml.org/lkml/2015/8/17/103
> 
> Best regards,

Re: [PATCH] platform/chrome: Make CROS_EC_PROTO a user selectable option

[Javier Martinez Canillas]

Hello Bartlomiej,

On 08/18/2015 03:36 PM, Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Tuesday, August 18, 2015 03:02:26 PM Javier Martinez Canillas wrote:
>> Hello Bartlomiej,
>>
>> Thanks a lot for your feedback.
>>
>> On 08/18/2015 02:40 PM, Bartlomiej Zolnierkiewicz wrote:
>>>
>>> Hi,
>>>
>>> On Tuesday, August 18, 2015 09:15:06 AM Javier Martinez Canillas wrote:
>>>> The boolean CROS_EC_PROTO symbol is selected by MFD_CROS_EC but that can
>>>> cause Kconfig circular dependencies so is better to change the select to
>>>
>>> Could you please give a reference to the problem or error message
>>> that you're getting (I was not following the previous discussion).
>>>
>>
>> Yes, the original Kconfig warning message was:
>>
>> warning: (MFD_CROS_EC) selects CHROME_PLATFORMS which has unmet direct dependencies (X86 || ARM)
>>
>> Paul fixed on [0] by making MFD_CROS_EC depends on X86 || ARM but that
>> is not really true since the driver could be used in other platforms so
>> I tried to instead fix it by removing unneeded dependencies in [1] but
>> one of the patches was nacked by Geert [2] who mentioned the issue about
>> mixing select and "depends on".
> 
> I think that Geert's comments are valid.  Dependencies should limit
> the config option to the archs for which the driver is really needed
> (the rest can be handled with COMPILE_TEST dependency).
> 

Yes, which is true for the drivers under CHROME_PLATFORMS (i.e: CROS_EC_LPC
has X86 || COMPILE_TEST) but my point was that CHROME_PLATFORMS is not a
driver but a submenu that has (for now) drivers for ARM and X86. But that
doesn't mean that a Chromebook for another arch could be produced and then
you will need to add a X86 || ARM || FOO || BAR || COMPILE_TEST for
CHROME_PLATFORMS which seems to me kind of silly.

That's why patch removed the dependency for CHROME_PLATFORMS but no of course
for the drivers for each arch.

>>>> a depends on. But in order to be able to change that, the CROS_EC_PROTO
>>>> symbol has to be one that can be selected by the user.
>>>
>>> Looking at the code behind the config option it seems that it is just
>>> a helper library and should not be made user-visible.  Why can't
>>
>> Yeah, I in fact used as a reference the DRM/KMS helpers (DRM_KMS_HELPER)
>> that is selected by drivers instead of depending on it.
>>
>>> the issue be fixed the other way (make other config options select
>>> CROS_EC_PROTO consistently)?
>>>
>>
>> That is another option indeed. I thought that the use of select was
>> discouraged and that it was preferable to use "depends on" even when
>> the Kconfig symbol is a boolean to enable support for some helpers.
> 
> No, this is not true.  It is just that "select" has to be used with
> care. ;)
>

Got it :)
 
>> But I don't really have a strong opinion on either approach tbh.
> 
> Please rework your patches to use "select".
>

Ok, will do.
 
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
>  

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH] platform/chrome: Make CROS_EC_PROTO a user selectable option

[Geert Uytterhoeven]

Hi Javier,

On Tue, Aug 18, 2015 at 3:44 PM, Javier Martinez Canillas
<javier@osg.samsung.com> wrote:
>>> Yes, the original Kconfig warning message was:
>>>
>>> warning: (MFD_CROS_EC) selects CHROME_PLATFORMS which has unmet direct dependencies (X86 || ARM)
>>>
>>> Paul fixed on [0] by making MFD_CROS_EC depends on X86 || ARM but that
>>> is not really true since the driver could be used in other platforms so
>>> I tried to instead fix it by removing unneeded dependencies in [1] but
>>> one of the patches was nacked by Geert [2] who mentioned the issue about
>>> mixing select and "depends on".
>>
>> I think that Geert's comments are valid.  Dependencies should limit
>> the config option to the archs for which the driver is really needed
>> (the rest can be handled with COMPILE_TEST dependency).
>>
>
> Yes, which is true for the drivers under CHROME_PLATFORMS (i.e: CROS_EC_LPC
> has X86 || COMPILE_TEST) but my point was that CHROME_PLATFORMS is not a
> driver but a submenu that has (for now) drivers for ARM and X86. But that
> doesn't mean that a Chromebook for another arch could be produced and then
> you will need to add a X86 || ARM || FOO || BAR || COMPILE_TEST for
> CHROME_PLATFORMS which seems to me kind of silly.

However, that's usually the way it is done: such a dependency is added to
prevent the question from showing up on platforms where it doesn't make
sense.

If the drivers become needed on other platforms, the dependency can be
relaxed.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] platform/chrome: Make CROS_EC_PROTO a user selectable option

[Javier Martinez Canillas]

Hello Geert,

On 08/18/2015 03:54 PM, Geert Uytterhoeven wrote:
> Hi Javier,
> 
> On Tue, Aug 18, 2015 at 3:44 PM, Javier Martinez Canillas
> <javier@osg.samsung.com> wrote:
>>>> Yes, the original Kconfig warning message was:
>>>>
>>>> warning: (MFD_CROS_EC) selects CHROME_PLATFORMS which has unmet direct dependencies (X86 || ARM)
>>>>
>>>> Paul fixed on [0] by making MFD_CROS_EC depends on X86 || ARM but that
>>>> is not really true since the driver could be used in other platforms so
>>>> I tried to instead fix it by removing unneeded dependencies in [1] but
>>>> one of the patches was nacked by Geert [2] who mentioned the issue about
>>>> mixing select and "depends on".
>>>
>>> I think that Geert's comments are valid.  Dependencies should limit
>>> the config option to the archs for which the driver is really needed
>>> (the rest can be handled with COMPILE_TEST dependency).
>>>
>>
>> Yes, which is true for the drivers under CHROME_PLATFORMS (i.e: CROS_EC_LPC
>> has X86 || COMPILE_TEST) but my point was that CHROME_PLATFORMS is not a
>> driver but a submenu that has (for now) drivers for ARM and X86. But that
>> doesn't mean that a Chromebook for another arch could be produced and then
>> you will need to add a X86 || ARM || FOO || BAR || COMPILE_TEST for
>> CHROME_PLATFORMS which seems to me kind of silly.
> 
> However, that's usually the way it is done: such a dependency is added to
> prevent the question from showing up on platforms where it doesn't make
> sense.
> 
> If the drivers become needed on other platforms, the dependency can be
> relaxed.
>

Ok, I'll reintroduce the arch dependency for CHROME_PLATFORMS then
and add a || COMPILE_TEST.

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America