On 11/9/23 04:52, Bean Huo wrote:
The objective of this patch is to incorporate Real Time Clock (RTC) support in Universal Flash Storage (UFS) device. This enhancement is crucial for the internal maintenance operations of the UFS device.
That sounds vague. Please explain why a UFS device should know the real time since other storage devices don't need to know the real time.
+ dev_info->rtc_time_baseline = mktime64(2010, 1, 1, 0, 0, 0) - + mktime64(1970, 1, 1, 0, 0, 0);
The formatting of the above code is not compliant with the kernel coding style. Please consider reformatting this patch with e.g. git clang-format HEAD^.
+ schedule_delayed_work(&hba->ufs_rtc_delayed_work, + msecs_to_jiffies(UFS_RTC_UPDATE_EVERY_MS));
The formatting of the above code is not compliant with the style guide either.
@@ -9746,6 +9834,8 @@ static int __ufshcd_wl_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op) ret = ufshcd_vops_suspend(hba, pm_op, POST_CHANGE); if (ret) goto set_link_active; + + cancel_delayed_work(&hba->ufs_rtc_delayed_work);
Why cancel_delayed_work() instead of cancel_delayed_work_sync()?
@@ -9840,6 +9930,8 @@ static int __ufshcd_wl_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op) if (ret) goto set_old_link_state; ufshcd_set_timestamp_attr(hba); + schedule_delayed_work(&hba->ufs_rtc_delayed_work, + msecs_to_jiffies(UFS_RTC_UPDATE_EVERY_MS));
The name of the constant "UFS_RTC_UPDATE_EVERY_MS" suggests that the real-time clock is updated 1000 times per second while the comment above the UFS_RTC_UPDATE_EVERY_MS constant says that it is updated once every ten seconds. Please choose a better name for UFS_RTC_UPDATE_EVERY_MS, e.g. UFS_RTC_UPDATE_INTERVAL_MS.
diff --git a/include/ufs/ufs.h b/include/ufs/ufs.h index e77ab1786856..18b39c6b3a97 100644 --- a/include/ufs/ufs.h +++ b/include/ufs/ufs.h @@ -14,6 +14,7 @@ #include <linux/bitops.h> #include <linux/types.h> #include <uapi/scsi/scsi_bsg_ufs.h> +#include <linux/rtc.h>
The include/ufs/ufs.h header file does not depend on anything declared in <linux/rtc.h> so please move the above include directive into a .c file.
+ struct delayed_work ufs_rtc_delayed_work;
Please change the name of this structure member into ufs_rtc_update_work. Thanks, Bart.
