Re: [PATCH v2 2/8] platform/x86/amd/pmc: Update function names to align with new STB file

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Tue, 29 Oct 2024, Shyam Sundar S K wrote:

> With STB now in a separate file, update the function names to match the
> correct naming schema by removing the _pmc_ prefix where needed.
> 
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>  drivers/platform/x86/amd/pmc/mp1_stb.c | 61 +++++++++++++-------------
>  drivers/platform/x86/amd/pmc/pmc.c     |  8 ++--
>  drivers/platform/x86/amd/pmc/pmc.h     |  4 +-
>  3 files changed, 36 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmc/mp1_stb.c b/drivers/platform/x86/amd/pmc/mp1_stb.c
> index 9a34dd94982c..5efec020ecac 100644
> --- a/drivers/platform/x86/amd/pmc/mp1_stb.c
> +++ b/drivers/platform/x86/amd/pmc/mp1_stb.c
> @@ -47,12 +47,12 @@ enum s2d_arg {
>  	S2D_DRAM_SIZE,
>  };
>  
> -struct amd_pmc_stb_v2_data {
> +struct amd_stb_v2_data {
>  	size_t size;
>  	u8 data[] __counted_by(size);
>  };
>  
> -int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data)
> +int amd_write_stb(struct amd_pmc_dev *dev, u32 data)
>  {
>  	int err;
>  
> @@ -65,7 +65,7 @@ int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data)
>  	return 0;
>  }
>  
> -static int amd_pmc_read_stb(struct amd_pmc_dev *dev, u32 *buf)
> +static int amd_read_stb(struct amd_pmc_dev *dev, u32 *buf)

Why aren't these two consistent with the rest that start with amd_stb_?

And thanks for doing this in a patch separate from the move, it's just so 
much simpler to review them independently. :-)

>  {
>  	int i, err;
>  
> @@ -81,7 +81,7 @@ static int amd_pmc_read_stb(struct amd_pmc_dev *dev, u32 *buf)
>  	return 0;
>  }
>  
> -static int amd_pmc_stb_debugfs_open(struct inode *inode, struct file *filp)
> +static int amd_stb_debugfs_open(struct inode *inode, struct file *filp)
>  {
>  	struct amd_pmc_dev *dev = filp->f_inode->i_private;
>  	u32 size = FIFO_SIZE * sizeof(u32);
> @@ -92,7 +92,7 @@ static int amd_pmc_stb_debugfs_open(struct inode *inode, struct file *filp)
>  	if (!buf)
>  		return -ENOMEM;
>  
> -	rc = amd_pmc_read_stb(dev, buf);
> +	rc = amd_read_stb(dev, buf);
>  	if (rc) {
>  		kfree(buf);
>  		return rc;
> @@ -102,8 +102,7 @@ static int amd_pmc_stb_debugfs_open(struct inode *inode, struct file *filp)
>  	return rc;
>  }
>  
> -static ssize_t amd_pmc_stb_debugfs_read(struct file *filp, char __user *buf, size_t size,
> -					loff_t *pos)
> +static ssize_t amd_stb_debugfs_read(struct file *filp, char __user *buf, size_t size, loff_t *pos)
>  {
>  	if (!filp->private_data)
>  		return -EINVAL;
> @@ -112,24 +111,24 @@ static ssize_t amd_pmc_stb_debugfs_read(struct file *filp, char __user *buf, siz
>  				       FIFO_SIZE * sizeof(u32));
>  }
>  
> -static int amd_pmc_stb_debugfs_release(struct inode *inode, struct file *filp)
> +static int amd_stb_debugfs_release(struct inode *inode, struct file *filp)
>  {
>  	kfree(filp->private_data);
>  	return 0;
>  }
>  
> -static const struct file_operations amd_pmc_stb_debugfs_fops = {
> +static const struct file_operations amd_stb_debugfs_fops = {
>  	.owner = THIS_MODULE,
> -	.open = amd_pmc_stb_debugfs_open,
> -	.read = amd_pmc_stb_debugfs_read,
> -	.release = amd_pmc_stb_debugfs_release,
> +	.open = amd_stb_debugfs_open,
> +	.read = amd_stb_debugfs_read,
> +	.release = amd_stb_debugfs_release,
>  };
>  
>  /* Enhanced STB Firmware Reporting Mechanism */
> -static int amd_pmc_stb_handle_efr(struct file *filp)
> +static int amd_stb_handle_efr(struct file *filp)
>  {
>  	struct amd_pmc_dev *dev = filp->f_inode->i_private;
> -	struct amd_pmc_stb_v2_data *stb_data_arr;
> +	struct amd_stb_v2_data *stb_data_arr;
>  	u32 fsize;
>  
>  	fsize = dev->dram_size - S2D_RSVD_RAM_SPACE;
> @@ -144,15 +143,15 @@ static int amd_pmc_stb_handle_efr(struct file *filp)
>  	return 0;
>  }
>  
> -static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
> +static int amd_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
>  {
>  	struct amd_pmc_dev *dev = filp->f_inode->i_private;
>  	u32 fsize, num_samples, val, stb_rdptr_offset = 0;
> -	struct amd_pmc_stb_v2_data *stb_data_arr;
> +	struct amd_stb_v2_data *stb_data_arr;
>  	int ret;
>  
>  	/* Write dummy postcode while reading the STB buffer */
> -	ret = amd_pmc_write_stb(dev, AMD_PMC_STB_DUMMY_PC);
> +	ret = amd_write_stb(dev, AMD_PMC_STB_DUMMY_PC);
>  	if (ret)
>  		dev_err(dev->dev, "error writing to STB: %d\n", ret);
>  
> @@ -169,7 +168,7 @@ static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
>  	 * platforms that support enhanced dram size reporting.
>  	 */
>  	if (dump_custom_stb)
> -		return amd_pmc_stb_handle_efr(filp);
> +		return amd_stb_handle_efr(filp);
>  
>  	/* Get the num_samples to calculate the last push location */
>  	ret = amd_pmc_send_cmd(dev, S2D_NUM_SAMPLES, &num_samples, dev->s2d_msg_id, true);
> @@ -209,28 +208,28 @@ static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
>  	return 0;
>  }
>  
> -static ssize_t amd_pmc_stb_debugfs_read_v2(struct file *filp, char __user *buf, size_t size,
> -					   loff_t *pos)
> +static ssize_t amd_stb_debugfs_read_v2(struct file *filp, char __user *buf, size_t size,
> +				       loff_t *pos)
>  {
> -	struct amd_pmc_stb_v2_data *data = filp->private_data;
> +	struct amd_stb_v2_data *data = filp->private_data;
>  
>  	return simple_read_from_buffer(buf, size, pos, data->data, data->size);
>  }
>  
> -static int amd_pmc_stb_debugfs_release_v2(struct inode *inode, struct file *filp)
> +static int amd_stb_debugfs_release_v2(struct inode *inode, struct file *filp)
>  {
>  	kfree(filp->private_data);
>  	return 0;
>  }
>  
> -static const struct file_operations amd_pmc_stb_debugfs_fops_v2 = {
> +static const struct file_operations amd_stb_debugfs_fops_v2 = {
>  	.owner = THIS_MODULE,
> -	.open = amd_pmc_stb_debugfs_open_v2,
> -	.read = amd_pmc_stb_debugfs_read_v2,
> -	.release = amd_pmc_stb_debugfs_release_v2,
> +	.open = amd_stb_debugfs_open_v2,
> +	.read = amd_stb_debugfs_read_v2,
> +	.release = amd_stb_debugfs_release_v2,
>  };
>  
> -static bool amd_pmc_is_stb_supported(struct amd_pmc_dev *dev)
> +static bool amd_is_stb_supported(struct amd_pmc_dev *dev)
>  {
>  	switch (dev->cpu_id) {
>  	case AMD_CPU_ID_YC:
> @@ -249,7 +248,7 @@ static bool amd_pmc_is_stb_supported(struct amd_pmc_dev *dev)
>  	}
>  }
>  
> -int amd_pmc_s2d_init(struct amd_pmc_dev *dev)
> +int amd_s2d_init(struct amd_pmc_dev *dev)

For consistency, amd_stb_s2d_init() ?

>  {
>  	u32 phys_addr_low, phys_addr_hi;
>  	u64 stb_phys_addr;
> @@ -259,12 +258,12 @@ int amd_pmc_s2d_init(struct amd_pmc_dev *dev)
>  	if (!enable_stb)
>  		return 0;
>  
> -	if (amd_pmc_is_stb_supported(dev))
> +	if (amd_is_stb_supported(dev))
>  		debugfs_create_file("stb_read", 0644, dev->dbgfs_dir, dev,
> -				    &amd_pmc_stb_debugfs_fops_v2);
> +				    &amd_stb_debugfs_fops_v2);
>  	else
>  		debugfs_create_file("stb_read", 0644, dev->dbgfs_dir, dev,
> -				    &amd_pmc_stb_debugfs_fops);
> +				    &amd_stb_debugfs_fops);

You might want to consider adding a patch to convert this to use ?: 
operator for the only arg that is changing so the entire call doesn't
need to be written twice nor is if/else needed.

-- 
 i.

>  
>  	/* Spill to DRAM feature uses separate SMU message port */
>  	dev->msg_port = 1;
> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
> index a977ff1e52b5..8e7c87505327 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.c
> +++ b/drivers/platform/x86/amd/pmc/pmc.c
> @@ -667,7 +667,7 @@ static void amd_pmc_s2idle_prepare(void)
>  		return;
>  	}
>  
> -	rc = amd_pmc_write_stb(pdev, AMD_PMC_STB_S2IDLE_PREPARE);
> +	rc = amd_write_stb(pdev, AMD_PMC_STB_S2IDLE_PREPARE);
>  	if (rc)
>  		dev_err(pdev->dev, "error writing to STB: %d\n", rc);
>  }
> @@ -686,7 +686,7 @@ static void amd_pmc_s2idle_check(void)
>  	/* Dump the IdleMask before we add to the STB */
>  	amd_pmc_idlemask_read(pdev, pdev->dev, NULL);
>  
> -	rc = amd_pmc_write_stb(pdev, AMD_PMC_STB_S2IDLE_CHECK);
> +	rc = amd_write_stb(pdev, AMD_PMC_STB_S2IDLE_CHECK);
>  	if (rc)
>  		dev_err(pdev->dev, "error writing to STB: %d\n", rc);
>  }
> @@ -713,7 +713,7 @@ static void amd_pmc_s2idle_restore(void)
>  	/* Let SMU know that we are looking for stats */
>  	amd_pmc_dump_data(pdev);
>  
> -	rc = amd_pmc_write_stb(pdev, AMD_PMC_STB_S2IDLE_RESTORE);
> +	rc = amd_write_stb(pdev, AMD_PMC_STB_S2IDLE_RESTORE);
>  	if (rc)
>  		dev_err(pdev->dev, "error writing to STB: %d\n", rc);
>  
> @@ -828,7 +828,7 @@ static int amd_pmc_probe(struct platform_device *pdev)
>  	}
>  
>  	amd_pmc_dbgfs_register(dev);
> -	err = amd_pmc_s2d_init(dev);
> +	err = amd_s2d_init(dev);
>  	if (err)
>  		goto err_pci_dev_put;
>  
> diff --git a/drivers/platform/x86/amd/pmc/pmc.h b/drivers/platform/x86/amd/pmc/pmc.h
> index 07fcb13a4136..7e7f9170124c 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.h
> +++ b/drivers/platform/x86/amd/pmc/pmc.h
> @@ -75,8 +75,8 @@ void amd_mp2_stb_deinit(struct amd_pmc_dev *dev);
>  #define AMD_S2D_REGISTER_RESPONSE	0xA80
>  #define AMD_S2D_REGISTER_ARGUMENT	0xA88
>  
> -int amd_pmc_s2d_init(struct amd_pmc_dev *dev);
> -int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data);
> +int amd_s2d_init(struct amd_pmc_dev *dev);
> +int amd_write_stb(struct amd_pmc_dev *dev, u32 data);
>  int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32 *data, u8 msg, bool ret);
>  
>  #endif /* PMC_H */
>