The get_version handler takes a pvs_name buffer and can override it with
the speed, pvs, and pvs version values. The function does not take as
argument the buffer size and is currently being determined by calling
`sizeof("speedXX-pvsXX-vXX")`. This is not great because it duplicates
the string in several locations which makes it error-prone if we need to
modify the string someday. Also since the buffer and its size are tied
together, it makes sense that they should both be passed together to the
get_version as parameters.
This commit makes sure that the PVS name template string is only
defined once, and that the pvs_name buffer is passed with its size.
Signed-off-by: Fabien Parent <[email protected]>
---
drivers/cpufreq/qcom-cpufreq-nvmem.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c
index 3bd38acde4b9..64ce077a4848 100644
--- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
+++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
@@ -30,6 +30,7 @@
#include <linux/soc/qcom/smem.h>
#define MSM_ID_SMEM 137
+#define PVS_NAME_TEMPLATE "speedXX-pvsXX-vXX"
enum _msm_id {
MSM8996V3 = 0xF6ul,
@@ -50,6 +51,7 @@ struct qcom_cpufreq_match_data {
int (*get_version)(struct device *cpu_dev,
struct nvmem_cell *speedbin_nvmem,
char **pvs_name,
+ size_t pvs_name_size,
struct qcom_cpufreq_drv *drv);
const char **genpd_names;
};
@@ -172,6 +174,7 @@ static enum _msm8996_version qcom_cpufreq_get_msm_id(void)
static int qcom_cpufreq_kryo_name_version(struct device *cpu_dev,
struct nvmem_cell *speedbin_nvmem,
char **pvs_name,
+ size_t pvs_name_size,
struct qcom_cpufreq_drv *drv)
{
size_t len;
@@ -208,6 +211,7 @@ static int qcom_cpufreq_kryo_name_version(struct device *cpu_dev,
static int qcom_cpufreq_krait_name_version(struct device *cpu_dev,
struct nvmem_cell *speedbin_nvmem,
char **pvs_name,
+ size_t pvs_name_size,
struct qcom_cpufreq_drv *drv)
{
int speed = 0, pvs = 0, pvs_ver = 0;
@@ -235,7 +239,7 @@ static int qcom_cpufreq_krait_name_version(struct device *cpu_dev,
goto len_error;
}
- snprintf(*pvs_name, sizeof("speedXX-pvsXX-vXX"), "speed%d-pvs%d-v%d",
+ snprintf(*pvs_name, pvs_name_size, "speed%d-pvs%d-v%d",
speed, pvs, pvs_ver);
drv->versions = (1 << speed);
@@ -265,7 +269,7 @@ static int qcom_cpufreq_probe(struct platform_device *pdev)
struct nvmem_cell *speedbin_nvmem;
struct device_node *np;
struct device *cpu_dev;
- char *pvs_name = "speedXX-pvsXX-vXX";
+ char *pvs_name = PVS_NAME_TEMPLATE;
unsigned cpu;
const struct of_device_id *match;
int ret;
@@ -306,8 +310,8 @@ static int qcom_cpufreq_probe(struct platform_device *pdev)
goto free_drv;
}
- ret = drv->data->get_version(cpu_dev,
- speedbin_nvmem, &pvs_name, drv);
+ ret = drv->data->get_version(cpu_dev, speedbin_nvmem, &pvs_name,
+ sizeof(PVS_NAME_TEMPLATE), drv);
if (ret) {
nvmem_cell_put(speedbin_nvmem);
goto free_drv;
--
2.37.2
On 01-10-22, 19:10, Fabien Parent wrote:
> @@ -265,7 +269,7 @@ static int qcom_cpufreq_probe(struct platform_device *pdev)
> struct nvmem_cell *speedbin_nvmem;
> struct device_node *np;
> struct device *cpu_dev;
> - char *pvs_name = "speedXX-pvsXX-vXX";
> + char *pvs_name = PVS_NAME_TEMPLATE;
> unsigned cpu;
> const struct of_device_id *match;
> int ret;
> @@ -306,8 +310,8 @@ static int qcom_cpufreq_probe(struct platform_device *pdev)
> goto free_drv;
> }
>
> - ret = drv->data->get_version(cpu_dev,
> - speedbin_nvmem, &pvs_name, drv);
> + ret = drv->data->get_version(cpu_dev, speedbin_nvmem, &pvs_name,
> + sizeof(PVS_NAME_TEMPLATE), drv);
Since the pvs name is always PVS_NAME_TEMPLATE, why are we even
passing it and size here ? Why not directly use it in those
get_version() implementations directly ?
> if (ret) {
> nvmem_cell_put(speedbin_nvmem);
> goto free_drv;
> --
> 2.37.2
--
viresh
On 10-10-22, 11:22, Viresh Kumar wrote:
> On 01-10-22, 19:10, Fabien Parent wrote:
> > @@ -265,7 +269,7 @@ static int qcom_cpufreq_probe(struct platform_device *pdev)
> > struct nvmem_cell *speedbin_nvmem;
> > struct device_node *np;
> > struct device *cpu_dev;
> > - char *pvs_name = "speedXX-pvsXX-vXX";
> > + char *pvs_name = PVS_NAME_TEMPLATE;
> > unsigned cpu;
> > const struct of_device_id *match;
> > int ret;
> > @@ -306,8 +310,8 @@ static int qcom_cpufreq_probe(struct platform_device *pdev)
> > goto free_drv;
> > }
> >
> > - ret = drv->data->get_version(cpu_dev,
> > - speedbin_nvmem, &pvs_name, drv);
> > + ret = drv->data->get_version(cpu_dev, speedbin_nvmem, &pvs_name,
> > + sizeof(PVS_NAME_TEMPLATE), drv);
>
> Since the pvs name is always PVS_NAME_TEMPLATE, why are we even
> passing it and size here ? Why not directly use it in those
> get_version() implementations directly ?
I understand how &pvs_name is used here to later set the prop_name. I
think instead you should also implement drv->data->prop_name() here,
which can return valid name or NULL.
--
viresh