[PATCH v3 03/10] arm: mvebu: clearfog: initial ClearFog Base variant

[Stefan Roese]

On 22.01.20 20:28, Joel Johnson wrote:

<snip>

>> It probably makes more sense to reverse the order of ORed conditions:
>>
>> 	if (IS_ENABLED(TARGET_CLEARFOG_BASE) ||
>>         		sr_product_is(&cf_tlv_data, "Clearfog Base"))
>>
>> IS_ENABLED() is evaluated at build time. When it is true, the compiler
>> drops all other 'if' branches, thus saving space. That also means that
>> the build time configuration overrides the EEPROM set value, which is a
>> Good Thing I believe.
> 
> I'll take a look and do testing and size comparison in the next day or
> two.
> 
> Note that I intended (and wrote the Base target help docs accordingly)
> that runtime detection, if both enabled and supported in hardware,
> should be preferred to the static configuration, with static config
> being only a fallback.

This sounds reasonable.

> This seems to be different from what your
> thought about it being good for build configuration to override
> EEPROM detected values, and I'm curious as to your reasoning.
> 
> There are a few gaps here related to what you point out (e.g.
> booting on a Pro with EEPROM and Base static config won't give the
> expected results). Relocating the static adjustment may be fine and
> help with that case as well.
> 
> I'll want to add support in such handling for the EEPROM Pro
> devices but don't have one available. You seem to have them
> available, can you confirm that "Clearfog Pro" would match those
> devices using sr_product_is?

I currently don't have any of these boards available, so I also would
like to see some reviewed-by and even better tested-by comments from
Baruch (or someone else at SolidRun).

Thanks,
Stefan