Messages in this thread | | | Date | Tue, 28 Mar 2023 13:48:22 +0530 | Subject | Re: [PATCH v4 4/5] firmware: qcom_scm: Refactor code to support multiple download mode | From | Mukesh Ojha <> |
| |
On 3/27/2023 11:53 PM, Bjorn Andersson wrote: > On Mon, Mar 27, 2023 at 10:11:20PM +0530, Mukesh Ojha wrote: > [..] >> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c >> index 3c6c5e7..0c94429 100644 >> --- a/drivers/firmware/qcom_scm.c >> +++ b/drivers/firmware/qcom_scm.c >> @@ -20,11 +20,11 @@ >> #include <linux/clk.h> >> #include <linux/reset-controller.h> >> #include <linux/arm-smccc.h> >> +#include <linux/kstrtox.h> >> >> #include "qcom_scm.h" >> >> -static bool download_mode = IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT); >> -module_param(download_mode, bool, 0); >> +static u32 download_mode; >> >> #define SCM_HAS_CORE_CLK BIT(0) >> #define SCM_HAS_IFACE_CLK BIT(1) >> @@ -32,6 +32,7 @@ module_param(download_mode, bool, 0); >> >> #define QCOM_DOWNLOAD_MODE_MASK 0x30 >> #define QCOM_DOWNLOAD_FULLDUMP 0x1 >> +#define QCOM_DOWNLOAD_NODUMP 0x0 >> >> struct qcom_scm { >> struct device *dev; >> @@ -440,8 +441,9 @@ static int __qcom_scm_set_dload_mode(struct device *dev, bool enable) >> return qcom_scm_call_atomic(__scm->dev, &desc, NULL); >> } >> >> -static void qcom_scm_set_download_mode(bool enable) >> +static void qcom_scm_set_download_mode(u32 download_mode) >> { >> + bool enable = !!download_mode; >> bool avail; >> int ret = 0; >> >> @@ -453,7 +455,7 @@ static void qcom_scm_set_download_mode(bool enable) >> } else if (__scm->dload_mode_addr) { >> ret = qcom_scm_io_update_field(__scm->dload_mode_addr, >> QCOM_DOWNLOAD_MODE_MASK, >> - enable ? QCOM_DOWNLOAD_FULLDUMP : 0); >> + enable ? download_mode : 0); > > Afaict, with QCOM_DOWNLOAD_NODUMP as 0, this says: > > when download_mode is non-zero, write that value, otherwise write 0 > > That should be the same as "write download_mode", so you should be able > to drop the enable part. > >> } else { >> dev_err(__scm->dev, >> "No available mechanism for setting download mode\n"); >> @@ -1419,6 +1421,49 @@ static irqreturn_t qcom_scm_irq_handler(int irq, void *data) >> return IRQ_HANDLED; >> } >> >> + >> +static int get_download_mode(char *buffer, const struct kernel_param *kp) >> +{ >> + int len = 0; >> + >> + if (download_mode == QCOM_DOWNLOAD_FULLDUMP) >> + len = sysfs_emit(buffer, "full\n"); >> + else if (download_mode == QCOM_DOWNLOAD_NODUMP) >> + len = sysfs_emit(buffer, "off\n"); >> + >> + return len; >> +} >> + >> +static int set_download_mode(const char *val, const struct kernel_param *kp) >> +{ >> + u32 old = download_mode; >> + >> + if (!strncmp(val, "full", strlen("full"))) { > > strcmp loops over the two string until they differ and/or both are > '\0'. > > As such, the only thing you achieve by using strncmp(.., T, strlen(T)) > is that the code has to iterate over T twice - and you make the code > harder to read.
If we use strcmp, i need to use "full\n" which we would not want to do. I think, we need to take this hit.
-- Mukesh > >> + download_mode = QCOM_DOWNLOAD_FULLDUMP; >> + } else if (!strncmp(val, "off", strlen("off"))) { >> + download_mode = QCOM_DOWNLOAD_NODUMP; >> + } else if (kstrtouint(val, 0, &download_mode) || >> + !(download_mode == 0 || download_mode == 1)) { >> + download_mode = old; >> + pr_err("unknown download mode\n"); > > This will result in a lone "unknown download mode" line somewhere in the > kernel log, without association to any driver or any indication what the > unknown value was. > > pr_err("qcom_scm: unknown download mode: %s\n", val); > > Would give both context and let the reader know right there what value > the code wasn't able to match. > > Regards, > Bjorn
| |