| Message ID | 20240220094211.20678-1-peter.wang@mediatek.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] ufs: core: adjust config_scsi_dev usage | expand |
On 2/20/24 01:42, peter.wang@mediatek.com wrote: > Adjust the usage of config_scis_dev to mach the existing usage of > other vops in ufs driver. I'm not sure the above is sufficient as motivation to make this change. Why to introduce the helper function ufshcd_vops_config_scsi_dev() since it only has one caller? Is this patch really an improvement of the UFS driver code base? Thanks, Bart.
On Tue, 2024-02-20 at 09:17 -0800, Bart Van Assche wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On 2/20/24 01:42, peter.wang@mediatek.com wrote: > > Adjust the usage of config_scis_dev to mach the existing usage of > > other vops in ufs driver. > > I'm not sure the above is sufficient as motivation to make this > change. > Why to introduce the helper function ufshcd_vops_config_scsi_dev() > since it > only has one caller? Is this patch really an improvement of the UFS > driver > code base? > > Thanks, > > Bart. Hi Bart, This help function is designed to assist users eliminating the check. Formalize the usage can make the code more concise and easier to read. Such as ufshcd_vops_init/ufshcd_vops_exit is also only one caller. Besides, it also need a comment of config_scsi_dev in ufshcd.h This patch dose not alter the logic, it simply makes the code easier to use and read. Pkease consider merging it. Thanks. Peter
On 2/20/24 18:44, Peter Wang (王信友) wrote: > This help function is designed to assist users eliminating the check. > Formalize the usage can make the code more concise and easier to read. > Such as ufshcd_vops_init/ufshcd_vops_exit is also only one caller. > Besides, it also need a comment of config_scsi_dev in ufshcd.h My personal opinion is that the helper function makes code harder to read because the definition of a helper function needs to be looked up to understand the code. There are many examples in the changelog of the kernel tree that show that there is a preference in the Linux kernel code base to inline short functions rather than keeping or introducing short helper functions. Regarding the config_scsi_dev comment, shouldn't that be a separate patch since that change is unrelated to the introduction of a helper function? Thanks, Bart.
On Wed, 2024-02-21 at 10:01 -0800, Bart Van Assche wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On 2/20/24 18:44, Peter Wang (王信友) wrote: > > This help function is designed to assist users eliminating the > check. > > Formalize the usage can make the code more concise and easier to > read. > > Such as ufshcd_vops_init/ufshcd_vops_exit is also only one caller. > > Besides, it also need a comment of config_scsi_dev in ufshcd.h > > My personal opinion is that the helper function makes code harder to > read because the definition of a helper function needs to be looked > up > to understand the code. There are many examples in the changelog of > the > kernel tree that show that there is a preference in the Linux kernel > code base to inline short functions rather than keeping or > introducing > short helper functions. > > Regarding the config_scsi_dev comment, shouldn't that be a separate > patch since that change is unrelated to the introduction of a helper > function? > > Thanks, > > Bart. Hi Bart, Got it, I will update patch and add config_scsi_dev comment only. Thanks. Peter
diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h index f42d99ce5bf1..72d5287c15ee 100644 --- a/drivers/ufs/core/ufshcd-priv.h +++ b/drivers/ufs/core/ufshcd-priv.h @@ -288,6 +288,13 @@ static inline int ufshcd_mcq_vops_config_esi(struct ufs_hba *hba) return -EOPNOTSUPP; } +static inline void ufshcd_vops_config_scsi_dev(struct ufs_hba *hba, + struct scsi_device *sdev) +{ + if (hba->vops && hba->vops->config_scsi_dev) + hba->vops->config_scsi_dev(sdev); +} + extern const struct ufs_pm_lvl_states ufs_pm_lvl_states[]; /** diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 16d76325039a..92f9de1d3152 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -5211,8 +5211,7 @@ static int ufshcd_slave_configure(struct scsi_device *sdev) */ sdev->silence_suspend = 1; - if (hba->vops && hba->vops->config_scsi_dev) - hba->vops->config_scsi_dev(sdev); + ufshcd_vops_config_scsi_dev(hba, sdev); ufshcd_crypto_register(hba, q); diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h index 7f0b2c5599cd..a19d87e7980f 100644 --- a/include/ufs/ufshcd.h +++ b/include/ufs/ufshcd.h @@ -327,6 +327,7 @@ struct ufs_pwr_mode_info { * @op_runtime_config: called to config Operation and runtime regs Pointers * @get_outstanding_cqs: called to get outstanding completion queues * @config_esi: called to config Event Specific Interrupt + * @config_scsi_dev: called to configure scsi device parameters */ struct ufs_hba_variant_ops { const char *name;