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:
