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

[Konrad Dybcio]

On 26.05.2023 04:43, Bjorn Andersson wrote:
> 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@xxxxxxxxx>
>>> Acked-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
>>> ---
>>> 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.
Riiight the function that returns a pointer to an error is *within*
the one we're calling.. So much so for me reviewing late at night..

Konrad
> 
>>
>> 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: