On 06.06.22 08:42, Marek Vasut wrote:
On 5/29/22 17:37, Jan Kiszka wrote:
On 29.05.22 03:46, Marek Vasut wrote:
On 5/28/22 15:02, Jan Kiszka wrote:
From: Jan Kiszka jan.kiszka@siemens.com
This completes what 890feecaab72 started by selecting ENV_APPEND and ENV_IS_NOWHERE and by moving this driver to top if the list. This
s@if the list@of the list@
ensures that load operations pick up both the default env and the permitted parts of the next-prio location. When writing though, we must not use NOWHERE and rather need to forward directly to the first external location.
Isn't the env load order a board-specific setting ?
There can always be special cases, but I don't see why it should be _always_ board-specific.
Because this is a special case, right ?
It's no longer special when you request the feature by turning on the related config options.
With this change, boards only need to define the list of writable variables but no longer have to provide a custom env_get_location implementation.
This also brings in a lot of ifdeffery and extra complexity. If you implement this as a board-specific env_get_location() override, you can avoid that. Try:
enum env_location env_get_location(enum env_operation op, int prio) { if (op == ENVOP_SAVE) { if (prio == 0) return ENVL_SPI_FLASH; } else { if (prio == 0) return ENVL_NOWHERE; if (prio == 1) return ENVL_SPI_FLASH; }
return ENVL_UNKNOWN; }
This is exactly what I would like to avoid, having to carry such a generic function (minus that hard-coded the choice of storage - this can be controlled via .config) in every board.
Just for a quick statistics -- how many boards would make use of this generic function ?
I don't have stats for how many boards are turning on ENV_WRITEABLE_LIST except for our boards. Here, I have about 3-5 boards (depending on how you count variants) that would all be fine with this switch and could then drop their custom handler (or never introduce one - IOT2050).
If you want to make this into a generic patch, can you somehow reduce the ever-growing ifdeffery, so that the patch won't add to it so much ? I suspect the code above can help with that, maybe it can be used to remove at least the env_locations[] reordering ifdeffery ?
Your code is not generic enough as it ignores the config-selected storage device. It would only happen to cover all our boards, but we are not the world. That's why I had to add some ifdeffery.
I could try write an alternative arch_env_get_location that does the required logic with a single ifdef. The prize might be some code duplication, though.
Jan