On Thu, 25 May 2023, Shyam Sundar S K wrote:
> On 5/23/2023 1:56 PM, Ilpo Järvinen wrote:
> > On Tue, 16 May 2023, Shyam Sundar S K wrote:
> >
> >> Add a helper routine to check the underlying cpu id, that can be used
> >> across the PMC driver to remove the duplicate code.
> >>
> >> Co-developed-by: Sanket Goswami <Sanket.Goswami@xxxxxxx>
> >> Signed-off-by: Sanket Goswami <Sanket.Goswami@xxxxxxx>
> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx>
> >> ---
> >> drivers/platform/x86/amd/pmc.c | 17 ++++++++++++++---
> >> 1 file changed, 14 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
> >> index e2439fda5c02..7e5e6afb3410 100644
> >> --- a/drivers/platform/x86/amd/pmc.c
> >> +++ b/drivers/platform/x86/amd/pmc.c
> >> @@ -564,6 +564,18 @@ static void amd_pmc_dbgfs_unregister(struct amd_pmc_dev *dev)
> >> debugfs_remove_recursive(dev->dbgfs_dir);
> >> }
> >>
> >> +static bool amd_pmc_check_sup_cpuid(struct amd_pmc_dev *dev)
> >
> > Does sup refer to "supported" or some other acronym? If the latter,
>
> Yes, please read that as "supported"
>
> > you should mention/open it in the changelog and/or in a comment. If the
> > former, the function naming seems too generic (an observation entirely
> > based on how/where the function is used, you're not exactly verbose on
> > what this actually checks for other than what looks like a set of CPU
> > IDs but clearly there's more behind it).
>
> OK. renaming the function as amd_pmc_is_cpu_supported() would be fine?
This makes things odder, it gets used in two places:
if (enable_stb) {
if (amd_pmc_check_sup_cpuid(dev))
debugfs_create_file(..., &amd_pmc_stb_debugfs_fops_v2);
else
debugfs_create_file(..., &amd_pmc_stb_debugfs_fops);
}
What about that else branch (PMC is not supported so who does that make
sense when the file is called pmc.c)? And here:
static int amd_pmc_probe(...)
{
...
if (enable_stb && amd_pmc_check_sup_cpuid(dev)) {
err = amd_pmc_s2d_init(dev);
if (err)
...goto + returns error
}
If enable_stb is not set, pmc not being supported is not going to return
error?
--
i.
