| Message ID | 20240216064112.962582-1-Shyam-sundar.S-k@amd.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Hans de Goede |
| Headers | show |
| Series | [v3,1/2] platform/x86/amd/pmf: remove smart_pc_status enum | expand |
On 2/16/2024 00:41, Shyam Sundar S K wrote: > Improve code readability by removing smart_pc_status enum, as the same > can be done with a simple true/false check; Update the code checks > accordingly. > > Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com> > --- > v2->v3: > - handle missing case for amd_pmf_init_smart_pc() after removing enum > > v1->v2: > - remove enum smart_pc_status and adjust the code handling > > drivers/platform/x86/amd/pmf/core.c | 11 ++++++++--- > drivers/platform/x86/amd/pmf/pmf.h | 5 ----- > drivers/platform/x86/amd/pmf/tee-if.c | 4 ++-- > 3 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c > index feaa09f5b35a..1d6dbd246d65 100644 > --- a/drivers/platform/x86/amd/pmf/core.c > +++ b/drivers/platform/x86/amd/pmf/core.c > @@ -330,9 +330,14 @@ static void amd_pmf_init_features(struct amd_pmf_dev *dev) > dev_dbg(dev->dev, "SPS enabled and Platform Profiles registered\n"); > } > > - if (!amd_pmf_init_smart_pc(dev)) { > + amd_pmf_init_smart_pc(dev); > + if (dev->smart_pc_enabled) { > dev_dbg(dev->dev, "Smart PC Solution Enabled\n"); > - } else if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) { > + /* If Smart PC is enabled, no need to check for other features */ > + return; > + } > + > + if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) { > amd_pmf_init_auto_mode(dev); > dev_dbg(dev->dev, "Auto Mode Init done\n"); > } else if (is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_AC) || > @@ -351,7 +356,7 @@ static void amd_pmf_deinit_features(struct amd_pmf_dev *dev) > amd_pmf_deinit_sps(dev); > } > > - if (!dev->smart_pc_enabled) { > + if (dev->smart_pc_enabled) { > amd_pmf_deinit_smart_pc(dev); > } else if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) { > amd_pmf_deinit_auto_mode(dev); > diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h > index 16999c5b334f..66cae1cca73c 100644 > --- a/drivers/platform/x86/amd/pmf/pmf.h > +++ b/drivers/platform/x86/amd/pmf/pmf.h > @@ -441,11 +441,6 @@ struct apmf_dyn_slider_output { > struct apmf_cnqf_power_set ps[APMF_CNQF_MAX]; > } __packed; > > -enum smart_pc_status { > - PMF_SMART_PC_ENABLED, > - PMF_SMART_PC_DISABLED, > -}; > - > /* Smart PC - TA internals */ > enum system_state { > SYSTEM_STATE_S0i3, > diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c > index f8c0177afb0d..8b7e3f87702e 100644 > --- a/drivers/platform/x86/amd/pmf/tee-if.c > +++ b/drivers/platform/x86/amd/pmf/tee-if.c > @@ -260,7 +260,7 @@ static int amd_pmf_start_policy_engine(struct amd_pmf_dev *dev) > res = amd_pmf_invoke_cmd_init(dev); > if (res == TA_PMF_TYPE_SUCCESS) { > /* Now its safe to announce that smart pc is enabled */ > - dev->smart_pc_enabled = PMF_SMART_PC_ENABLED; > + dev->smart_pc_enabled = true; > /* > * Start collecting the data from TA FW after a small delay > * or else, we might end up getting stale values. > @@ -268,7 +268,7 @@ static int amd_pmf_start_policy_engine(struct amd_pmf_dev *dev) > schedule_delayed_work(&dev->pb_work, msecs_to_jiffies(pb_actions_ms * 3)); > } else { > dev_err(dev->dev, "ta invoke cmd init failed err: %x\n", res); > - dev->smart_pc_enabled = PMF_SMART_PC_DISABLED; > + dev->smart_pc_enabled = false; > return res; > } >
Hi Shyam, On 2/16/24 07:41, Shyam Sundar S K wrote: > Improve code readability by removing smart_pc_status enum, as the same > can be done with a simple true/false check; Update the code checks > accordingly. > > Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> > --- > v2->v3: > - handle missing case for amd_pmf_init_smart_pc() after removing enum > > v1->v2: > - remove enum smart_pc_status and adjust the code handling > > drivers/platform/x86/amd/pmf/core.c | 11 ++++++++--- > drivers/platform/x86/amd/pmf/pmf.h | 5 ----- > drivers/platform/x86/amd/pmf/tee-if.c | 4 ++-- > 3 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c > index feaa09f5b35a..1d6dbd246d65 100644 > --- a/drivers/platform/x86/amd/pmf/core.c > +++ b/drivers/platform/x86/amd/pmf/core.c > @@ -330,9 +330,14 @@ static void amd_pmf_init_features(struct amd_pmf_dev *dev) > dev_dbg(dev->dev, "SPS enabled and Platform Profiles registered\n"); > } > > - if (!amd_pmf_init_smart_pc(dev)) { > + amd_pmf_init_smart_pc(dev); > + if (dev->smart_pc_enabled) { > dev_dbg(dev->dev, "Smart PC Solution Enabled\n"); > - } else if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) { > + /* If Smart PC is enabled, no need to check for other features */ > + return; This return here is new behavior and a minimum should have been mentioned in the commit message. I plan to prepare a set of fixes to send to Linus tomorrow, so I will take this as is. I'll amend the commit message myself. Reviewed-by: Hans de Goede <hdegoede@redhat.com> But next time, even for a one liner like adding this commit split it out in a separate patch. Especially when it is causing significantly different behavior of the driver as is the case here. Regards, Hans > + } > + > + if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) { > amd_pmf_init_auto_mode(dev); > dev_dbg(dev->dev, "Auto Mode Init done\n"); > } else if (is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_AC) || > @@ -351,7 +356,7 @@ static void amd_pmf_deinit_features(struct amd_pmf_dev *dev) > amd_pmf_deinit_sps(dev); > } > > - if (!dev->smart_pc_enabled) { > + if (dev->smart_pc_enabled) { > amd_pmf_deinit_smart_pc(dev); > } else if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) { > amd_pmf_deinit_auto_mode(dev); > diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h > index 16999c5b334f..66cae1cca73c 100644 > --- a/drivers/platform/x86/amd/pmf/pmf.h > +++ b/drivers/platform/x86/amd/pmf/pmf.h > @@ -441,11 +441,6 @@ struct apmf_dyn_slider_output { > struct apmf_cnqf_power_set ps[APMF_CNQF_MAX]; > } __packed; > > -enum smart_pc_status { > - PMF_SMART_PC_ENABLED, > - PMF_SMART_PC_DISABLED, > -}; > - > /* Smart PC - TA internals */ > enum system_state { > SYSTEM_STATE_S0i3, > diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c > index f8c0177afb0d..8b7e3f87702e 100644 > --- a/drivers/platform/x86/amd/pmf/tee-if.c > +++ b/drivers/platform/x86/amd/pmf/tee-if.c > @@ -260,7 +260,7 @@ static int amd_pmf_start_policy_engine(struct amd_pmf_dev *dev) > res = amd_pmf_invoke_cmd_init(dev); > if (res == TA_PMF_TYPE_SUCCESS) { > /* Now its safe to announce that smart pc is enabled */ > - dev->smart_pc_enabled = PMF_SMART_PC_ENABLED; > + dev->smart_pc_enabled = true; > /* > * Start collecting the data from TA FW after a small delay > * or else, we might end up getting stale values. > @@ -268,7 +268,7 @@ static int amd_pmf_start_policy_engine(struct amd_pmf_dev *dev) > schedule_delayed_work(&dev->pb_work, msecs_to_jiffies(pb_actions_ms * 3)); > } else { > dev_err(dev->dev, "ta invoke cmd init failed err: %x\n", res); > - dev->smart_pc_enabled = PMF_SMART_PC_DISABLED; > + dev->smart_pc_enabled = false; > return res; > } >
Hi Hans, On 2/18/2024 15:54, Hans de Goede wrote: > Hi Shyam, > > On 2/16/24 07:41, Shyam Sundar S K wrote: >> Improve code readability by removing smart_pc_status enum, as the same >> can be done with a simple true/false check; Update the code checks >> accordingly. >> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> >> --- >> v2->v3: >> - handle missing case for amd_pmf_init_smart_pc() after removing enum >> >> v1->v2: >> - remove enum smart_pc_status and adjust the code handling >> >> drivers/platform/x86/amd/pmf/core.c | 11 ++++++++--- >> drivers/platform/x86/amd/pmf/pmf.h | 5 ----- >> drivers/platform/x86/amd/pmf/tee-if.c | 4 ++-- >> 3 files changed, 10 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c >> index feaa09f5b35a..1d6dbd246d65 100644 >> --- a/drivers/platform/x86/amd/pmf/core.c >> +++ b/drivers/platform/x86/amd/pmf/core.c >> @@ -330,9 +330,14 @@ static void amd_pmf_init_features(struct amd_pmf_dev *dev) >> dev_dbg(dev->dev, "SPS enabled and Platform Profiles registered\n"); >> } >> >> - if (!amd_pmf_init_smart_pc(dev)) { >> + amd_pmf_init_smart_pc(dev); >> + if (dev->smart_pc_enabled) { >> dev_dbg(dev->dev, "Smart PC Solution Enabled\n"); >> - } else if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) { >> + /* If Smart PC is enabled, no need to check for other features */ >> + return; > > This return here is new behavior and a minimum should have been > mentioned in the commit message. > > I plan to prepare a set of fixes to send to Linus tomorrow, > so I will take this as is. I'll amend the commit message myself. > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > > But next time, even for a one liner like adding this commit split > it out in a separate patch. Especially when it is causing significantly > different behavior of the driver as is the case here. Sorry. I thought I amended the commit message, but seems like its not. I will take care of this in the future. Thanks, Shyam > > Regards, > > Hans > > > > > > >> + } >> + >> + if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) { >> amd_pmf_init_auto_mode(dev); >> dev_dbg(dev->dev, "Auto Mode Init done\n"); >> } else if (is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_AC) || >> @@ -351,7 +356,7 @@ static void amd_pmf_deinit_features(struct amd_pmf_dev *dev) >> amd_pmf_deinit_sps(dev); >> } >> >> - if (!dev->smart_pc_enabled) { >> + if (dev->smart_pc_enabled) { >> amd_pmf_deinit_smart_pc(dev); >> } else if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) { >> amd_pmf_deinit_auto_mode(dev); >> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h >> index 16999c5b334f..66cae1cca73c 100644 >> --- a/drivers/platform/x86/amd/pmf/pmf.h >> +++ b/drivers/platform/x86/amd/pmf/pmf.h >> @@ -441,11 +441,6 @@ struct apmf_dyn_slider_output { >> struct apmf_cnqf_power_set ps[APMF_CNQF_MAX]; >> } __packed; >> >> -enum smart_pc_status { >> - PMF_SMART_PC_ENABLED, >> - PMF_SMART_PC_DISABLED, >> -}; >> - >> /* Smart PC - TA internals */ >> enum system_state { >> SYSTEM_STATE_S0i3, >> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c >> index f8c0177afb0d..8b7e3f87702e 100644 >> --- a/drivers/platform/x86/amd/pmf/tee-if.c >> +++ b/drivers/platform/x86/amd/pmf/tee-if.c >> @@ -260,7 +260,7 @@ static int amd_pmf_start_policy_engine(struct amd_pmf_dev *dev) >> res = amd_pmf_invoke_cmd_init(dev); >> if (res == TA_PMF_TYPE_SUCCESS) { >> /* Now its safe to announce that smart pc is enabled */ >> - dev->smart_pc_enabled = PMF_SMART_PC_ENABLED; >> + dev->smart_pc_enabled = true; >> /* >> * Start collecting the data from TA FW after a small delay >> * or else, we might end up getting stale values. >> @@ -268,7 +268,7 @@ static int amd_pmf_start_policy_engine(struct amd_pmf_dev *dev) >> schedule_delayed_work(&dev->pb_work, msecs_to_jiffies(pb_actions_ms * 3)); >> } else { >> dev_err(dev->dev, "ta invoke cmd init failed err: %x\n", res); >> - dev->smart_pc_enabled = PMF_SMART_PC_DISABLED; >> + dev->smart_pc_enabled = false; >> return res; >> } >> >
Hi, On 2/16/24 07:41, Shyam Sundar S K wrote: > Improve code readability by removing smart_pc_status enum, as the same > can be done with a simple true/false check; Update the code checks > accordingly. > > Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> Thank you for your patch-series, I've applied this series to my review-hans branch: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans Note it will show up in my review-hans branch once I've pushed my local branch there, which might take a while. I will include this patch in my next fixes pull-req to Linus for the current kernel development cycle. Regards, Hans > --- > v2->v3: > - handle missing case for amd_pmf_init_smart_pc() after removing enum > > v1->v2: > - remove enum smart_pc_status and adjust the code handling > > drivers/platform/x86/amd/pmf/core.c | 11 ++++++++--- > drivers/platform/x86/amd/pmf/pmf.h | 5 ----- > drivers/platform/x86/amd/pmf/tee-if.c | 4 ++-- > 3 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c > index feaa09f5b35a..1d6dbd246d65 100644 > --- a/drivers/platform/x86/amd/pmf/core.c > +++ b/drivers/platform/x86/amd/pmf/core.c > @@ -330,9 +330,14 @@ static void amd_pmf_init_features(struct amd_pmf_dev *dev) > dev_dbg(dev->dev, "SPS enabled and Platform Profiles registered\n"); > } > > - if (!amd_pmf_init_smart_pc(dev)) { > + amd_pmf_init_smart_pc(dev); > + if (dev->smart_pc_enabled) { > dev_dbg(dev->dev, "Smart PC Solution Enabled\n"); > - } else if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) { > + /* If Smart PC is enabled, no need to check for other features */ > + return; > + } > + > + if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) { > amd_pmf_init_auto_mode(dev); > dev_dbg(dev->dev, "Auto Mode Init done\n"); > } else if (is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_AC) || > @@ -351,7 +356,7 @@ static void amd_pmf_deinit_features(struct amd_pmf_dev *dev) > amd_pmf_deinit_sps(dev); > } > > - if (!dev->smart_pc_enabled) { > + if (dev->smart_pc_enabled) { > amd_pmf_deinit_smart_pc(dev); > } else if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) { > amd_pmf_deinit_auto_mode(dev); > diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h > index 16999c5b334f..66cae1cca73c 100644 > --- a/drivers/platform/x86/amd/pmf/pmf.h > +++ b/drivers/platform/x86/amd/pmf/pmf.h > @@ -441,11 +441,6 @@ struct apmf_dyn_slider_output { > struct apmf_cnqf_power_set ps[APMF_CNQF_MAX]; > } __packed; > > -enum smart_pc_status { > - PMF_SMART_PC_ENABLED, > - PMF_SMART_PC_DISABLED, > -}; > - > /* Smart PC - TA internals */ > enum system_state { > SYSTEM_STATE_S0i3, > diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c > index f8c0177afb0d..8b7e3f87702e 100644 > --- a/drivers/platform/x86/amd/pmf/tee-if.c > +++ b/drivers/platform/x86/amd/pmf/tee-if.c > @@ -260,7 +260,7 @@ static int amd_pmf_start_policy_engine(struct amd_pmf_dev *dev) > res = amd_pmf_invoke_cmd_init(dev); > if (res == TA_PMF_TYPE_SUCCESS) { > /* Now its safe to announce that smart pc is enabled */ > - dev->smart_pc_enabled = PMF_SMART_PC_ENABLED; > + dev->smart_pc_enabled = true; > /* > * Start collecting the data from TA FW after a small delay > * or else, we might end up getting stale values. > @@ -268,7 +268,7 @@ static int amd_pmf_start_policy_engine(struct amd_pmf_dev *dev) > schedule_delayed_work(&dev->pb_work, msecs_to_jiffies(pb_actions_ms * 3)); > } else { > dev_err(dev->dev, "ta invoke cmd init failed err: %x\n", res); > - dev->smart_pc_enabled = PMF_SMART_PC_DISABLED; > + dev->smart_pc_enabled = false; > return res; > } >
diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c index feaa09f5b35a..1d6dbd246d65 100644 --- a/drivers/platform/x86/amd/pmf/core.c +++ b/drivers/platform/x86/amd/pmf/core.c @@ -330,9 +330,14 @@ static void amd_pmf_init_features(struct amd_pmf_dev *dev) dev_dbg(dev->dev, "SPS enabled and Platform Profiles registered\n"); } - if (!amd_pmf_init_smart_pc(dev)) { + amd_pmf_init_smart_pc(dev); + if (dev->smart_pc_enabled) { dev_dbg(dev->dev, "Smart PC Solution Enabled\n"); - } else if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) { + /* If Smart PC is enabled, no need to check for other features */ + return; + } + + if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) { amd_pmf_init_auto_mode(dev); dev_dbg(dev->dev, "Auto Mode Init done\n"); } else if (is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_AC) || @@ -351,7 +356,7 @@ static void amd_pmf_deinit_features(struct amd_pmf_dev *dev) amd_pmf_deinit_sps(dev); } - if (!dev->smart_pc_enabled) { + if (dev->smart_pc_enabled) { amd_pmf_deinit_smart_pc(dev); } else if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) { amd_pmf_deinit_auto_mode(dev); diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h index 16999c5b334f..66cae1cca73c 100644 --- a/drivers/platform/x86/amd/pmf/pmf.h +++ b/drivers/platform/x86/amd/pmf/pmf.h @@ -441,11 +441,6 @@ struct apmf_dyn_slider_output { struct apmf_cnqf_power_set ps[APMF_CNQF_MAX]; } __packed; -enum smart_pc_status { - PMF_SMART_PC_ENABLED, - PMF_SMART_PC_DISABLED, -}; - /* Smart PC - TA internals */ enum system_state { SYSTEM_STATE_S0i3, diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c index f8c0177afb0d..8b7e3f87702e 100644 --- a/drivers/platform/x86/amd/pmf/tee-if.c +++ b/drivers/platform/x86/amd/pmf/tee-if.c @@ -260,7 +260,7 @@ static int amd_pmf_start_policy_engine(struct amd_pmf_dev *dev) res = amd_pmf_invoke_cmd_init(dev); if (res == TA_PMF_TYPE_SUCCESS) { /* Now its safe to announce that smart pc is enabled */ - dev->smart_pc_enabled = PMF_SMART_PC_ENABLED; + dev->smart_pc_enabled = true; /* * Start collecting the data from TA FW after a small delay * or else, we might end up getting stale values. @@ -268,7 +268,7 @@ static int amd_pmf_start_policy_engine(struct amd_pmf_dev *dev) schedule_delayed_work(&dev->pb_work, msecs_to_jiffies(pb_actions_ms * 3)); } else { dev_err(dev->dev, "ta invoke cmd init failed err: %x\n", res); - dev->smart_pc_enabled = PMF_SMART_PC_DISABLED; + dev->smart_pc_enabled = false; return res; }
Improve code readability by removing smart_pc_status enum, as the same can be done with a simple true/false check; Update the code checks accordingly. Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> --- v2->v3: - handle missing case for amd_pmf_init_smart_pc() after removing enum v1->v2: - remove enum smart_pc_status and adjust the code handling drivers/platform/x86/amd/pmf/core.c | 11 ++++++++--- drivers/platform/x86/amd/pmf/pmf.h | 5 ----- drivers/platform/x86/amd/pmf/tee-if.c | 4 ++-- 3 files changed, 10 insertions(+), 10 deletions(-)