[v4,5/5] cpufreq: qcom-nvmem: use helper to get SMEM SoC ID

Now that SMEM exports a helper to get the SMEM SoC ID lets utilize it.
Currently qcom_cpufreq_get_msm_id() is encoding the returned SMEM SoC ID
into an enum, however there is no reason to do so and we can just match
directly on the SMEM SoC ID as returned by qcom_smem_get_soc_id().

Signed-off-by: Robert Marko <robimarko@gmail.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
Changes in v4:
* Adapt to name change to qcom_smem_get_soc_id()

Changes in v3:
* Adapt to helper using argument now

Changes in v2:
* Utilize helper exported by SMEM instead of refactoring
qcom_cpufreq_get_msm_id()
---
 drivers/cpufreq/qcom-cpufreq-nvmem.c | 56 +++++-----------------------
 1 file changed, 10 insertions(+), 46 deletions(-)

On Fri, May 26, 2023 at 01:18:02AM +0200, Konrad Dybcio wrote:
> 
> 
> On 25.05.2023 23:02, Robert Marko wrote:
> > Now that SMEM exports a helper to get the SMEM SoC ID lets utilize it.
> > Currently qcom_cpufreq_get_msm_id() is encoding the returned SMEM SoC ID
> > into an enum, however there is no reason to do so and we can just match
> > directly on the SMEM SoC ID as returned by qcom_smem_get_soc_id().
> > 
> > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> > Changes in v4:
> > * Adapt to name change to qcom_smem_get_soc_id()
> > 
> > Changes in v3:
> > * Adapt to helper using argument now
> > 
> > Changes in v2:
> > * Utilize helper exported by SMEM instead of refactoring
> > qcom_cpufreq_get_msm_id()
> > ---
> >  drivers/cpufreq/qcom-cpufreq-nvmem.c | 56 +++++-----------------------
> >  1 file changed, 10 insertions(+), 46 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> > index 60e99be2d3db..a88b6fe5db50 100644
> > --- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
> > +++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> > @@ -29,16 +29,8 @@
> >  #include <linux/slab.h>
> >  #include <linux/soc/qcom/smem.h>
> >  
> > -#define MSM_ID_SMEM	137
> > -
> >  #include <dt-bindings/arm/qcom,ids.h>
> >  
> > -enum _msm8996_version {
> > -	MSM8996_V3,
> > -	MSM8996_SG,
> > -	NUM_OF_MSM8996_VERSIONS,
> > -};
> > -
> >  struct qcom_cpufreq_drv;
> >  
> >  struct qcom_cpufreq_match_data {
> > @@ -135,60 +127,32 @@ static void get_krait_bin_format_b(struct device *cpu_dev,
> >  	dev_dbg(cpu_dev, "PVS version: %d\n", *pvs_ver);
> >  }
> >  
> > -static enum _msm8996_version qcom_cpufreq_get_msm_id(void)
> > -{
> > -	size_t len;
> > -	u32 *msm_id;
> > -	enum _msm8996_version version;
> > -
> > -	msm_id = qcom_smem_get(QCOM_SMEM_HOST_ANY, MSM_ID_SMEM, &len);
> > -	if (IS_ERR(msm_id))
> > -		return NUM_OF_MSM8996_VERSIONS;
> > -
> > -	/* The first 4 bytes are format, next to them is the actual msm-id */
> > -	msm_id++;
> > -
> > -	switch ((enum _msm_id)*msm_id) {
> > -	case QCOM_ID_MSM8996:
> > -	case QCOM_ID_APQ8096:
> > -		version = MSM8996_V3;
> > -		break;
> > -	case QCOM_ID_MSM8996SG:
> > -	case QCOM_ID_APQ8096SG:
> > -		version = MSM8996_SG;
> > -		break;
> > -	default:
> > -		version = NUM_OF_MSM8996_VERSIONS;
> > -	}
> > -
> > -	return version;
> > -}
> > -
> >  static int qcom_cpufreq_kryo_name_version(struct device *cpu_dev,
> >  					  struct nvmem_cell *speedbin_nvmem,
> >  					  char **pvs_name,
> >  					  struct qcom_cpufreq_drv *drv)
> >  {
> >  	size_t len;
> > +	u32 msm_id;
> __le32
> 
> >  	u8 *speedbin;
> > -	enum _msm8996_version msm8996_version;
> > +	int ret;
> >  	*pvs_name = NULL;
> >  
> > -	msm8996_version = qcom_cpufreq_get_msm_id();
> > -	if (NUM_OF_MSM8996_VERSIONS == msm8996_version) {
> > -		dev_err(cpu_dev, "Not Snapdragon 820/821!");
> > -		return -ENODEV;
> > -	}
> > +	ret = qcom_smem_get_soc_id(&msm_id);
> > +	if (ret)
> > +		return ret;
> Now since it can return a PTR_ERR, you should check for IS_ERR
> and return ERR_PTR if that happens

No, the PTR_ERR() extracted the error value out of the pointer, so it's
just an integer now (or zero on success). So this is looking correct to
me.

> 
> LGTM otherwise!
> 
> Konrad
> >  
> >  	speedbin = nvmem_cell_read(speedbin_nvmem, &len);
> >  	if (IS_ERR(speedbin))
> >  		return PTR_ERR(speedbin);
> >  
> > -	switch (msm8996_version) {
> > -	case MSM8996_V3:
> > +	switch (msm_id) {
> > +	case QCOM_ID_MSM8996:

And here are those cpu-endian constants... If msm_id is a __le32 then
all these constants needs to be cpu_to_le32().

Regards,
Bjorn

> > +	case QCOM_ID_APQ8096:
> >  		drv->versions = 1 << (unsigned int)(*speedbin);
> >  		break;
> > -	case MSM8996_SG:
> > +	case QCOM_ID_MSM8996SG:
> > +	case QCOM_ID_APQ8096SG:
> >  		drv->versions = 1 << ((unsigned int)(*speedbin) + 4);
> >  		break;
> >  	default: