| Message ID | 20240419151800.2168903-8-elder@linaro.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 319b6d4ef08714ed593a5eb842deb249544c19e1 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ipa: eight simple cleanups | expand |
On Fri, 2024-04-19 at 10:17 -0500, Alex Elder wrote: > In "ipa_cmd.h", ipa_cmd_data_valid() is declared, but that function > does not exist. So delete that declaration. > > Also, for some reason ipa_cmd_init() never gets called. It isn't > really critical--it just validates that some memory offsets and a > size can be represented in some register fields, and they won't fail > with current data. Regardless, call the function in ipa_probe(). That name sounds confusing to me: I expect *init to allocate/set something that will need some reverse operation at shutdown/removal. What about a possible follow-up renaming the function to ipa_cmd_validate() or the like? Not blocking the series, I'm applying it. Thanks, Paolo
On 4/23/24 6:21 AM, Paolo Abeni wrote: > On Fri, 2024-04-19 at 10:17 -0500, Alex Elder wrote: >> In "ipa_cmd.h", ipa_cmd_data_valid() is declared, but that function >> does not exist. So delete that declaration. >> >> Also, for some reason ipa_cmd_init() never gets called. It isn't >> really critical--it just validates that some memory offsets and a >> size can be represented in some register fields, and they won't fail >> with current data. Regardless, call the function in ipa_probe(). > > That name sounds confusing to me: I expect *init to allocate/set > something that will need some reverse operation at shutdown/removal. > What about a possible follow-up renaming the function to > ipa_cmd_validate() or the like? In the IPA driver I have several phases of initialization that occur: - *_init() is done to initialize anything (like allocating memory and looking up DT information) that does not require any access to hardware. Its inverse is *_exit(). - *_config() is done once "primitive" (register-based) access to the hardware is needed, where the hardware must be clocked. Its inverse is *_deconfig(). - *_setup() is done after the above, at a point where a higher-level command-based (submit/await completion) interface is available. That is used for the last steps of setting up the hardware. Its inverse is *_teardown(). You're right, that in this case all this init function does is validate things. But at an abstract level, this is the place in the "IPA command" module where *any* early-stage initialization takes place. The caller doesn't "know" that at the moment this happens to only be validation. (I don't recall, but this might previously have done some other things.) So that's the reasoning behind the name. Changing it to ipa_cmd_validate() makes sense too, but wouldn't fit the pattern used elsewhere. I'm open to it though; it's just a design choice. But unless you're convinced such a change would really improve the code, I plan to leave it as-is. > Not blocking the series, I'm applying it. Thank you very much. -Alex > Thanks, > > Paolo >
diff --git a/drivers/net/ipa/ipa_cmd.h b/drivers/net/ipa/ipa_cmd.h index 5824bb131ebab..2077fdbade99c 100644 --- a/drivers/net/ipa/ipa_cmd.h +++ b/drivers/net/ipa/ipa_cmd.h @@ -53,14 +53,6 @@ enum ipa_cmd_opcode { bool ipa_cmd_table_init_valid(struct ipa *ipa, const struct ipa_mem *mem, bool route); -/** - * ipa_cmd_data_valid() - Validate command-realted configuration is valid - * @ipa: - IPA pointer - * - * Return: true if assumptions required for command are valid - */ -bool ipa_cmd_data_valid(struct ipa *ipa); - /** * ipa_cmd_pool_init() - initialize command channel pools * @channel: AP->IPA command TX GSI channel pointer diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c index b13a59f27106d..6a0fec873cddf 100644 --- a/drivers/net/ipa/ipa_main.c +++ b/drivers/net/ipa/ipa_main.c @@ -865,6 +865,10 @@ static int ipa_probe(struct platform_device *pdev) if (ret) goto err_reg_exit; + ret = ipa_cmd_init(ipa); + if (ret) + goto err_mem_exit; + ret = gsi_init(&ipa->gsi, pdev, ipa->version, data->endpoint_count, data->endpoint_data); if (ret)
In "ipa_cmd.h", ipa_cmd_data_valid() is declared, but that function does not exist. So delete that declaration. Also, for some reason ipa_cmd_init() never gets called. It isn't really critical--it just validates that some memory offsets and a size can be represented in some register fields, and they won't fail with current data. Regardless, call the function in ipa_probe(). Signed-off-by: Alex Elder <elder@linaro.org> --- drivers/net/ipa/ipa_cmd.h | 8 -------- drivers/net/ipa/ipa_main.c | 4 ++++ 2 files changed, 4 insertions(+), 8 deletions(-)