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

[Javier Martinez Canillas <javier@osg.samsung.com>]

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