drivers/staging/rts5208/sd.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
Replace udelay() with usleep_range() for more precise delay handling.
Reported by checkpatch:
CHECK: usleep_range is preferred over udelay
Signed-off-by: kenechukwu maduechesi <maduechesik@gmail.com>
---
drivers/staging/rts5208/sd.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/rts5208/sd.c b/drivers/staging/rts5208/sd.c
index 74c4f476b3a4..059f99b0a727 100644
--- a/drivers/staging/rts5208/sd.c
+++ b/drivers/staging/rts5208/sd.c
@@ -865,7 +865,7 @@ static int sd_change_phase(struct rtsx_chip *chip, u8 sample_point, u8 tune_dir)
PHASE_CHANGE);
if (retval)
return retval;
- udelay(50);
+ usleep_range(50);
retval = rtsx_write_register(chip, SD_VP_CTL, 0xFF,
PHASE_CHANGE |
PHASE_NOT_RESET |
@@ -877,14 +877,14 @@ static int sd_change_phase(struct rtsx_chip *chip, u8 sample_point, u8 tune_dir)
CHANGE_CLK, CHANGE_CLK);
if (retval)
return retval;
- udelay(50);
+ usleep_range(50);
retval = rtsx_write_register(chip, SD_VP_CTL, 0xFF,
PHASE_NOT_RESET |
sample_point);
if (retval)
return retval;
}
- udelay(100);
+ usleep_range(100);
rtsx_init_cmd(chip);
rtsx_add_cmd(chip, WRITE_REG_CMD, SD_DCMPS_CTL, DCMPS_CHANGE,
@@ -918,7 +918,7 @@ static int sd_change_phase(struct rtsx_chip *chip, u8 sample_point, u8 tune_dir)
return retval;
}
- udelay(50);
+ usleep_range(50);
}
retval = rtsx_write_register(chip, SD_CFG1, SD_ASYNC_FIFO_NOT_RST, 0);
@@ -1416,7 +1416,7 @@ static int sd_wait_data_idle(struct rtsx_chip *chip)
retval = STATUS_SUCCESS;
break;
}
- udelay(100);
+ usleep_range(100);
}
dev_dbg(rtsx_dev(chip), "SD_DATA_STATE: 0x%02x\n", val);
--
2.25.1Hi kenechukwu,
kernel test robot noticed the following build errors:
[auto build test ERROR on staging/staging-testing]
url: https://github.com/intel-lab-lkp/linux/commits/kenechukwu-maduechesi/staging-rts5208-Replace-delay-function/20231018-090457
base: staging/staging-testing
patch link: https://lore.kernel.org/r/20231018004300.GA3189%40ubuntu
patch subject: [PATCH] staging: rts5208: Replace delay function.
config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20231106/202311060455.MQsyG6kq-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231106/202311060455.MQsyG6kq-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311060455.MQsyG6kq-lkp@intel.com/
All errors (new ones prefixed by >>):
>> drivers/staging/rts5208/sd.c:868:19: error: too few arguments to function call, expected 2, have 1
usleep_range(50);
~~~~~~~~~~~~ ^
include/linux/delay.h:66:20: note: 'usleep_range' declared here
static inline void usleep_range(unsigned long min, unsigned long max)
^
drivers/staging/rts5208/sd.c:880:19: error: too few arguments to function call, expected 2, have 1
usleep_range(50);
~~~~~~~~~~~~ ^
include/linux/delay.h:66:20: note: 'usleep_range' declared here
static inline void usleep_range(unsigned long min, unsigned long max)
^
drivers/staging/rts5208/sd.c:887:19: error: too few arguments to function call, expected 2, have 1
usleep_range(100);
~~~~~~~~~~~~ ^
include/linux/delay.h:66:20: note: 'usleep_range' declared here
static inline void usleep_range(unsigned long min, unsigned long max)
^
drivers/staging/rts5208/sd.c:921:18: error: too few arguments to function call, expected 2, have 1
usleep_range(50);
~~~~~~~~~~~~ ^
include/linux/delay.h:66:20: note: 'usleep_range' declared here
static inline void usleep_range(unsigned long min, unsigned long max)
^
drivers/staging/rts5208/sd.c:1419:19: error: too few arguments to function call, expected 2, have 1
usleep_range(100);
~~~~~~~~~~~~ ^
include/linux/delay.h:66:20: note: 'usleep_range' declared here
static inline void usleep_range(unsigned long min, unsigned long max)
^
5 errors generated.
vim +868 drivers/staging/rts5208/sd.c
814
815 static int sd_change_phase(struct rtsx_chip *chip, u8 sample_point, u8 tune_dir)
816 {
817 struct sd_info *sd_card = &chip->sd_card;
818 u16 SD_VP_CTL, SD_DCMPS_CTL;
819 u8 val;
820 int retval;
821 bool ddr_rx = false;
822
823 dev_dbg(rtsx_dev(chip), "%s (sample_point = %d, tune_dir = %d)\n",
824 __func__, sample_point, tune_dir);
825
826 if (tune_dir == TUNE_RX) {
827 SD_VP_CTL = SD_VPRX_CTL;
828 SD_DCMPS_CTL = SD_DCMPS_RX_CTL;
829 if (CHK_SD_DDR50(sd_card))
830 ddr_rx = true;
831 } else {
832 SD_VP_CTL = SD_VPTX_CTL;
833 SD_DCMPS_CTL = SD_DCMPS_TX_CTL;
834 }
835
836 if (chip->asic_code) {
837 retval = rtsx_write_register(chip, CLK_CTL, CHANGE_CLK,
838 CHANGE_CLK);
839 if (retval)
840 return retval;
841 retval = rtsx_write_register(chip, SD_VP_CTL, 0x1F,
842 sample_point);
843 if (retval)
844 return retval;
845 retval = rtsx_write_register(chip, SD_VPCLK0_CTL,
846 PHASE_NOT_RESET, 0);
847 if (retval)
848 return retval;
849 retval = rtsx_write_register(chip, SD_VPCLK0_CTL,
850 PHASE_NOT_RESET, PHASE_NOT_RESET);
851 if (retval)
852 return retval;
853 retval = rtsx_write_register(chip, CLK_CTL, CHANGE_CLK, 0);
854 if (retval)
855 return retval;
856 } else {
857 rtsx_read_register(chip, SD_VP_CTL, &val);
858 dev_dbg(rtsx_dev(chip), "SD_VP_CTL: 0x%x\n", val);
859 rtsx_read_register(chip, SD_DCMPS_CTL, &val);
860 dev_dbg(rtsx_dev(chip), "SD_DCMPS_CTL: 0x%x\n", val);
861
862 if (ddr_rx) {
863 retval = rtsx_write_register(chip, SD_VP_CTL,
864 PHASE_CHANGE,
865 PHASE_CHANGE);
866 if (retval)
867 return retval;
> 868 usleep_range(50);
869 retval = rtsx_write_register(chip, SD_VP_CTL, 0xFF,
870 PHASE_CHANGE |
871 PHASE_NOT_RESET |
872 sample_point);
873 if (retval)
874 return retval;
875 } else {
876 retval = rtsx_write_register(chip, CLK_CTL,
877 CHANGE_CLK, CHANGE_CLK);
878 if (retval)
879 return retval;
880 usleep_range(50);
881 retval = rtsx_write_register(chip, SD_VP_CTL, 0xFF,
882 PHASE_NOT_RESET |
883 sample_point);
884 if (retval)
885 return retval;
886 }
887 usleep_range(100);
888
889 rtsx_init_cmd(chip);
890 rtsx_add_cmd(chip, WRITE_REG_CMD, SD_DCMPS_CTL, DCMPS_CHANGE,
891 DCMPS_CHANGE);
892 rtsx_add_cmd(chip, CHECK_REG_CMD, SD_DCMPS_CTL,
893 DCMPS_CHANGE_DONE, DCMPS_CHANGE_DONE);
894 retval = rtsx_send_cmd(chip, SD_CARD, 100);
895 if (retval != STATUS_SUCCESS)
896 goto fail;
897
898 val = *rtsx_get_cmd_data(chip);
899 if (val & DCMPS_ERROR)
900 goto fail;
901
902 if ((val & DCMPS_CURRENT_PHASE) != sample_point)
903 goto fail;
904
905 retval = rtsx_write_register(chip, SD_DCMPS_CTL,
906 DCMPS_CHANGE, 0);
907 if (retval)
908 return retval;
909 if (ddr_rx) {
910 retval = rtsx_write_register(chip, SD_VP_CTL,
911 PHASE_CHANGE, 0);
912 if (retval)
913 return retval;
914 } else {
915 retval = rtsx_write_register(chip, CLK_CTL,
916 CHANGE_CLK, 0);
917 if (retval)
918 return retval;
919 }
920
921 usleep_range(50);
922 }
923
924 retval = rtsx_write_register(chip, SD_CFG1, SD_ASYNC_FIFO_NOT_RST, 0);
925 if (retval)
926 return retval;
927
928 return STATUS_SUCCESS;
929
930 fail:
931 rtsx_read_register(chip, SD_VP_CTL, &val);
932 dev_dbg(rtsx_dev(chip), "SD_VP_CTL: 0x%x\n", val);
933 rtsx_read_register(chip, SD_DCMPS_CTL, &val);
934 dev_dbg(rtsx_dev(chip), "SD_DCMPS_CTL: 0x%x\n", val);
935
936 rtsx_write_register(chip, SD_DCMPS_CTL, DCMPS_CHANGE, 0);
937 rtsx_write_register(chip, SD_VP_CTL, PHASE_CHANGE, 0);
938 mdelay(10);
939 sd_reset_dcm(chip, tune_dir);
940 return STATUS_FAIL;
941 }
942
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi kenechukwu,
kernel test robot noticed the following build errors:
[auto build test ERROR on staging/staging-testing]
url: https://github.com/intel-lab-lkp/linux/commits/kenechukwu-maduechesi/staging-rts5208-Replace-delay-function/20231018-090457
base: staging/staging-testing
patch link: https://lore.kernel.org/r/20231018004300.GA3189%40ubuntu
patch subject: [PATCH] staging: rts5208: Replace delay function.
config: microblaze-allyesconfig (https://download.01.org/0day-ci/archive/20231024/202310240604.OZnfTcMT-lkp@intel.com/config)
compiler: microblaze-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231024/202310240604.OZnfTcMT-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310240604.OZnfTcMT-lkp@intel.com/
All errors (new ones prefixed by >>):
drivers/staging/rts5208/sd.c: In function 'sd_change_phase':
>> drivers/staging/rts5208/sd.c:868:25: error: too few arguments to function 'usleep_range'
868 | usleep_range(50);
| ^~~~~~~~~~~~
In file included from drivers/staging/rts5208/rtsx.h:17,
from drivers/staging/rts5208/sd.c:16:
include/linux/delay.h:66:20: note: declared here
66 | static inline void usleep_range(unsigned long min, unsigned long max)
| ^~~~~~~~~~~~
drivers/staging/rts5208/sd.c:880:25: error: too few arguments to function 'usleep_range'
880 | usleep_range(50);
| ^~~~~~~~~~~~
include/linux/delay.h:66:20: note: declared here
66 | static inline void usleep_range(unsigned long min, unsigned long max)
| ^~~~~~~~~~~~
drivers/staging/rts5208/sd.c:887:17: error: too few arguments to function 'usleep_range'
887 | usleep_range(100);
| ^~~~~~~~~~~~
include/linux/delay.h:66:20: note: declared here
66 | static inline void usleep_range(unsigned long min, unsigned long max)
| ^~~~~~~~~~~~
drivers/staging/rts5208/sd.c:921:17: error: too few arguments to function 'usleep_range'
921 | usleep_range(50);
| ^~~~~~~~~~~~
include/linux/delay.h:66:20: note: declared here
66 | static inline void usleep_range(unsigned long min, unsigned long max)
| ^~~~~~~~~~~~
drivers/staging/rts5208/sd.c: In function 'sd_wait_data_idle':
drivers/staging/rts5208/sd.c:1419:17: error: too few arguments to function 'usleep_range'
1419 | usleep_range(100);
| ^~~~~~~~~~~~
include/linux/delay.h:66:20: note: declared here
66 | static inline void usleep_range(unsigned long min, unsigned long max)
| ^~~~~~~~~~~~
vim +/usleep_range +868 drivers/staging/rts5208/sd.c
814
815 static int sd_change_phase(struct rtsx_chip *chip, u8 sample_point, u8 tune_dir)
816 {
817 struct sd_info *sd_card = &chip->sd_card;
818 u16 SD_VP_CTL, SD_DCMPS_CTL;
819 u8 val;
820 int retval;
821 bool ddr_rx = false;
822
823 dev_dbg(rtsx_dev(chip), "%s (sample_point = %d, tune_dir = %d)\n",
824 __func__, sample_point, tune_dir);
825
826 if (tune_dir == TUNE_RX) {
827 SD_VP_CTL = SD_VPRX_CTL;
828 SD_DCMPS_CTL = SD_DCMPS_RX_CTL;
829 if (CHK_SD_DDR50(sd_card))
830 ddr_rx = true;
831 } else {
832 SD_VP_CTL = SD_VPTX_CTL;
833 SD_DCMPS_CTL = SD_DCMPS_TX_CTL;
834 }
835
836 if (chip->asic_code) {
837 retval = rtsx_write_register(chip, CLK_CTL, CHANGE_CLK,
838 CHANGE_CLK);
839 if (retval)
840 return retval;
841 retval = rtsx_write_register(chip, SD_VP_CTL, 0x1F,
842 sample_point);
843 if (retval)
844 return retval;
845 retval = rtsx_write_register(chip, SD_VPCLK0_CTL,
846 PHASE_NOT_RESET, 0);
847 if (retval)
848 return retval;
849 retval = rtsx_write_register(chip, SD_VPCLK0_CTL,
850 PHASE_NOT_RESET, PHASE_NOT_RESET);
851 if (retval)
852 return retval;
853 retval = rtsx_write_register(chip, CLK_CTL, CHANGE_CLK, 0);
854 if (retval)
855 return retval;
856 } else {
857 rtsx_read_register(chip, SD_VP_CTL, &val);
858 dev_dbg(rtsx_dev(chip), "SD_VP_CTL: 0x%x\n", val);
859 rtsx_read_register(chip, SD_DCMPS_CTL, &val);
860 dev_dbg(rtsx_dev(chip), "SD_DCMPS_CTL: 0x%x\n", val);
861
862 if (ddr_rx) {
863 retval = rtsx_write_register(chip, SD_VP_CTL,
864 PHASE_CHANGE,
865 PHASE_CHANGE);
866 if (retval)
867 return retval;
> 868 usleep_range(50);
869 retval = rtsx_write_register(chip, SD_VP_CTL, 0xFF,
870 PHASE_CHANGE |
871 PHASE_NOT_RESET |
872 sample_point);
873 if (retval)
874 return retval;
875 } else {
876 retval = rtsx_write_register(chip, CLK_CTL,
877 CHANGE_CLK, CHANGE_CLK);
878 if (retval)
879 return retval;
880 usleep_range(50);
881 retval = rtsx_write_register(chip, SD_VP_CTL, 0xFF,
882 PHASE_NOT_RESET |
883 sample_point);
884 if (retval)
885 return retval;
886 }
887 usleep_range(100);
888
889 rtsx_init_cmd(chip);
890 rtsx_add_cmd(chip, WRITE_REG_CMD, SD_DCMPS_CTL, DCMPS_CHANGE,
891 DCMPS_CHANGE);
892 rtsx_add_cmd(chip, CHECK_REG_CMD, SD_DCMPS_CTL,
893 DCMPS_CHANGE_DONE, DCMPS_CHANGE_DONE);
894 retval = rtsx_send_cmd(chip, SD_CARD, 100);
895 if (retval != STATUS_SUCCESS)
896 goto fail;
897
898 val = *rtsx_get_cmd_data(chip);
899 if (val & DCMPS_ERROR)
900 goto fail;
901
902 if ((val & DCMPS_CURRENT_PHASE) != sample_point)
903 goto fail;
904
905 retval = rtsx_write_register(chip, SD_DCMPS_CTL,
906 DCMPS_CHANGE, 0);
907 if (retval)
908 return retval;
909 if (ddr_rx) {
910 retval = rtsx_write_register(chip, SD_VP_CTL,
911 PHASE_CHANGE, 0);
912 if (retval)
913 return retval;
914 } else {
915 retval = rtsx_write_register(chip, CLK_CTL,
916 CHANGE_CLK, 0);
917 if (retval)
918 return retval;
919 }
920
921 usleep_range(50);
922 }
923
924 retval = rtsx_write_register(chip, SD_CFG1, SD_ASYNC_FIFO_NOT_RST, 0);
925 if (retval)
926 return retval;
927
928 return STATUS_SUCCESS;
929
930 fail:
931 rtsx_read_register(chip, SD_VP_CTL, &val);
932 dev_dbg(rtsx_dev(chip), "SD_VP_CTL: 0x%x\n", val);
933 rtsx_read_register(chip, SD_DCMPS_CTL, &val);
934 dev_dbg(rtsx_dev(chip), "SD_DCMPS_CTL: 0x%x\n", val);
935
936 rtsx_write_register(chip, SD_DCMPS_CTL, DCMPS_CHANGE, 0);
937 rtsx_write_register(chip, SD_VP_CTL, PHASE_CHANGE, 0);
938 mdelay(10);
939 sd_reset_dcm(chip, tune_dir);
940 return STATUS_FAIL;
941 }
942
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On 18.10.2023 03:03, kenechukwu maduechesi wrote: > Replace udelay() with usleep_range() for more precise delay handling. > > Reported by checkpatch: > > CHECK: usleep_range is preferred over udelay Have you tried to compile your patch? This is something you should do before sending it to the mailing list. All the best, Karolina > > Signed-off-by: kenechukwu maduechesi <maduechesik@gmail.com> > --- > drivers/staging/rts5208/sd.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/staging/rts5208/sd.c b/drivers/staging/rts5208/sd.c > index 74c4f476b3a4..059f99b0a727 100644 > --- a/drivers/staging/rts5208/sd.c > +++ b/drivers/staging/rts5208/sd.c > @@ -865,7 +865,7 @@ static int sd_change_phase(struct rtsx_chip *chip, u8 sample_point, u8 tune_dir) > PHASE_CHANGE); > if (retval) > return retval; > - udelay(50); > + usleep_range(50); > retval = rtsx_write_register(chip, SD_VP_CTL, 0xFF, > PHASE_CHANGE | > PHASE_NOT_RESET | > @@ -877,14 +877,14 @@ static int sd_change_phase(struct rtsx_chip *chip, u8 sample_point, u8 tune_dir) > CHANGE_CLK, CHANGE_CLK); > if (retval) > return retval; > - udelay(50); > + usleep_range(50); > retval = rtsx_write_register(chip, SD_VP_CTL, 0xFF, > PHASE_NOT_RESET | > sample_point); > if (retval) > return retval; > } > - udelay(100); > + usleep_range(100); > > rtsx_init_cmd(chip); > rtsx_add_cmd(chip, WRITE_REG_CMD, SD_DCMPS_CTL, DCMPS_CHANGE, > @@ -918,7 +918,7 @@ static int sd_change_phase(struct rtsx_chip *chip, u8 sample_point, u8 tune_dir) > return retval; > } > > - udelay(50); > + usleep_range(50); > } > > retval = rtsx_write_register(chip, SD_CFG1, SD_ASYNC_FIFO_NOT_RST, 0); > @@ -1416,7 +1416,7 @@ static int sd_wait_data_idle(struct rtsx_chip *chip) > retval = STATUS_SUCCESS; > break; > } > - udelay(100); > + usleep_range(100); > } > dev_dbg(rtsx_dev(chip), "SD_DATA_STATE: 0x%02x\n", val); >
On Tue, 17 Oct 2023, kenechukwu maduechesi wrote: > Replace udelay() with usleep_range() for more precise delay handling. > > Reported by checkpatch: > > CHECK: usleep_range is preferred over udelay This message is typically not a good candidate for outreachy patches, because you need access to the device to be sure that any change is correct. julia > > Signed-off-by: kenechukwu maduechesi <maduechesik@gmail.com> > --- > drivers/staging/rts5208/sd.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/staging/rts5208/sd.c b/drivers/staging/rts5208/sd.c > index 74c4f476b3a4..059f99b0a727 100644 > --- a/drivers/staging/rts5208/sd.c > +++ b/drivers/staging/rts5208/sd.c > @@ -865,7 +865,7 @@ static int sd_change_phase(struct rtsx_chip *chip, u8 sample_point, u8 tune_dir) > PHASE_CHANGE); > if (retval) > return retval; > - udelay(50); > + usleep_range(50); > retval = rtsx_write_register(chip, SD_VP_CTL, 0xFF, > PHASE_CHANGE | > PHASE_NOT_RESET | > @@ -877,14 +877,14 @@ static int sd_change_phase(struct rtsx_chip *chip, u8 sample_point, u8 tune_dir) > CHANGE_CLK, CHANGE_CLK); > if (retval) > return retval; > - udelay(50); > + usleep_range(50); > retval = rtsx_write_register(chip, SD_VP_CTL, 0xFF, > PHASE_NOT_RESET | > sample_point); > if (retval) > return retval; > } > - udelay(100); > + usleep_range(100); > > rtsx_init_cmd(chip); > rtsx_add_cmd(chip, WRITE_REG_CMD, SD_DCMPS_CTL, DCMPS_CHANGE, > @@ -918,7 +918,7 @@ static int sd_change_phase(struct rtsx_chip *chip, u8 sample_point, u8 tune_dir) > return retval; > } > > - udelay(50); > + usleep_range(50); > } > > retval = rtsx_write_register(chip, SD_CFG1, SD_ASYNC_FIFO_NOT_RST, 0); > @@ -1416,7 +1416,7 @@ static int sd_wait_data_idle(struct rtsx_chip *chip) > retval = STATUS_SUCCESS; > break; > } > - udelay(100); > + usleep_range(100); > } > dev_dbg(rtsx_dev(chip), "SD_DATA_STATE: 0x%02x\n", val); > > -- > 2.25.1 > > >
Hi Julia, > > Replace udelay() with usleep_range() for more precise delay handling. > > > > Reported by checkpatch: > > > > CHECK: usleep_range is preferred over udelay > > This message is typically not a good candidate for outreachy patches, > because you need access to the device to be sure that any change is > correct. I actually don't really mind this patch... I would exchange these udelay() with almost anything, they look to me placed a bit random anyway (without going too much in detail). But in general, for this project, I think you are right and it's a good idea not to blindly change delay and sleeping functions without really knowing what you are doing. Andi
On 18.10.2023 09:03, Julia Lawall wrote: > > > On Tue, 17 Oct 2023, kenechukwu maduechesi wrote: > >> Replace udelay() with usleep_range() for more precise delay handling. >> >> Reported by checkpatch: >> >> CHECK: usleep_range is preferred over udelay > > This message is typically not a good candidate for outreachy patches, > because you need access to the device to be sure that any change is > correct. Could we add a paragraph on how to pick good checkpatch.pl error to fix to the Outreachyfirstpatch docs? This could go to "Find a driver to clean up" section, for example. All the best, Karolina > > julia > >> >> Signed-off-by: kenechukwu maduechesi <maduechesik@gmail.com> >> --- >> drivers/staging/rts5208/sd.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/staging/rts5208/sd.c b/drivers/staging/rts5208/sd.c >> index 74c4f476b3a4..059f99b0a727 100644 >> --- a/drivers/staging/rts5208/sd.c >> +++ b/drivers/staging/rts5208/sd.c >> @@ -865,7 +865,7 @@ static int sd_change_phase(struct rtsx_chip *chip, u8 sample_point, u8 tune_dir) >> PHASE_CHANGE); >> if (retval) >> return retval; >> - udelay(50); >> + usleep_range(50); >> retval = rtsx_write_register(chip, SD_VP_CTL, 0xFF, >> PHASE_CHANGE | >> PHASE_NOT_RESET | >> @@ -877,14 +877,14 @@ static int sd_change_phase(struct rtsx_chip *chip, u8 sample_point, u8 tune_dir) >> CHANGE_CLK, CHANGE_CLK); >> if (retval) >> return retval; >> - udelay(50); >> + usleep_range(50); >> retval = rtsx_write_register(chip, SD_VP_CTL, 0xFF, >> PHASE_NOT_RESET | >> sample_point); >> if (retval) >> return retval; >> } >> - udelay(100); >> + usleep_range(100); >> >> rtsx_init_cmd(chip); >> rtsx_add_cmd(chip, WRITE_REG_CMD, SD_DCMPS_CTL, DCMPS_CHANGE, >> @@ -918,7 +918,7 @@ static int sd_change_phase(struct rtsx_chip *chip, u8 sample_point, u8 tune_dir) >> return retval; >> } >> >> - udelay(50); >> + usleep_range(50); >> } >> >> retval = rtsx_write_register(chip, SD_CFG1, SD_ASYNC_FIFO_NOT_RST, 0); >> @@ -1416,7 +1416,7 @@ static int sd_wait_data_idle(struct rtsx_chip *chip) >> retval = STATUS_SUCCESS; >> break; >> } >> - udelay(100); >> + usleep_range(100); >> } >> dev_dbg(rtsx_dev(chip), "SD_DATA_STATE: 0x%02x\n", val); >> >> -- >> 2.25.1 >> >> >> >
On Wed, Oct 18, 2023 at 09:32:46AM +0200, Karolina Stolarek wrote: > On 18.10.2023 09:03, Julia Lawall wrote: > > > > > > On Tue, 17 Oct 2023, kenechukwu maduechesi wrote: > > > > > Replace udelay() with usleep_range() for more precise delay handling. > > > > > > Reported by checkpatch: > > > > > > CHECK: usleep_range is preferred over udelay > > > > This message is typically not a good candidate for outreachy patches, > > because you need access to the device to be sure that any change is > > correct. > > Could we add a paragraph on how to pick good checkpatch.pl error to fix to > the Outreachyfirstpatch docs? This could go to "Find a driver to clean up" > section, for example. I think you should put a note about usleep_range() and the extra parentheses one. Also say something about looking up stuff on lore. https://lore.kernel.org/all/?q=sd_change_phase%20usleep_range In this case someone tried to update this before. The patch wasn't merged but it wasn't clearly explained to the developer why the patch wasn't merged. Ah well. Generally fresh warnings are better than old warnings because we fix the simple stuff where checkpatch is obviously correct. The other thing is that people really need to look at the wider context. Look at the surrounding code. Look at when the checkpatch warning was introduced and try to put yourself in the developer's head to figure out what they were thinking. Are there other opportunities for cleanups nearby. regards, dan carpenter
On Wed, Oct 18, 2023 at 09:32:46AM +0200, Karolina Stolarek wrote: > On 18.10.2023 09:03, Julia Lawall wrote: > > > > > > On Tue, 17 Oct 2023, kenechukwu maduechesi wrote: > > > > > Replace udelay() with usleep_range() for more precise delay handling. > > > > > > Reported by checkpatch: > > > > > > CHECK: usleep_range is preferred over udelay > > > > This message is typically not a good candidate for outreachy patches, > > because you need access to the device to be sure that any change is > > correct. > > Could we add a paragraph on how to pick good checkpatch.pl error to fix to > the Outreachyfirstpatch docs? This could go to "Find a driver to clean up" > section, for example. The ability to find a "good" error changes over time, so this might be hard to do. good luck! greg k-h
On 18.10.2023 09:45, Greg Kroah-Hartman wrote: > On Wed, Oct 18, 2023 at 09:32:46AM +0200, Karolina Stolarek wrote: >> On 18.10.2023 09:03, Julia Lawall wrote: >>> >>> >>> On Tue, 17 Oct 2023, kenechukwu maduechesi wrote: >>> >>>> Replace udelay() with usleep_range() for more precise delay handling. >>>> >>>> Reported by checkpatch: >>>> >>>> CHECK: usleep_range is preferred over udelay >>> >>> This message is typically not a good candidate for outreachy patches, >>> because you need access to the device to be sure that any change is >>> correct. >> >> Could we add a paragraph on how to pick good checkpatch.pl error to fix to >> the Outreachyfirstpatch docs? This could go to "Find a driver to clean up" >> section, for example. > > The ability to find a "good" error changes over time, so this might be > hard to do. I agree, but we can all agree that experimenting with udelay during Outreachy is not a good idea, and people should know about it All the best, Karolina > > good luck! > > greg k-h
On Wed, 18 Oct 2023, Karolina Stolarek wrote: > On 18.10.2023 09:45, Greg Kroah-Hartman wrote: > > On Wed, Oct 18, 2023 at 09:32:46AM +0200, Karolina Stolarek wrote: > > > On 18.10.2023 09:03, Julia Lawall wrote: > > > > > > > > > > > > On Tue, 17 Oct 2023, kenechukwu maduechesi wrote: > > > > > > > > > Replace udelay() with usleep_range() for more precise delay handling. > > > > > > > > > > Reported by checkpatch: > > > > > > > > > > CHECK: usleep_range is preferred over udelay > > > > > > > > This message is typically not a good candidate for outreachy patches, > > > > because you need access to the device to be sure that any change is > > > > correct. > > > > > > Could we add a paragraph on how to pick good checkpatch.pl error to fix to > > > the Outreachyfirstpatch docs? This could go to "Find a driver to clean up" > > > section, for example. > > > > The ability to find a "good" error changes over time, so this might be > > hard to do. > > I agree, but we can all agree that experimenting with udelay during Outreachy > is not a good idea, and people should know about it In general, I think that it is better in the contribution period to do the wrong thing and then learn about why it is wrong, but this case comes up over and over, and it is always not the right thing to do, so I added an appropriate explanation. Thanks for the suggestion. julia > > All the best, > Karolina > > > > > good luck! > > > > greg k-h >
On 18.10.2023 12:28, Julia Lawall wrote: > > > On Wed, 18 Oct 2023, Karolina Stolarek wrote: > >> On 18.10.2023 09:45, Greg Kroah-Hartman wrote: >>> On Wed, Oct 18, 2023 at 09:32:46AM +0200, Karolina Stolarek wrote: >>>> On 18.10.2023 09:03, Julia Lawall wrote: >>>>> >>>>> >>>>> On Tue, 17 Oct 2023, kenechukwu maduechesi wrote: >>>>> >>>>>> Replace udelay() with usleep_range() for more precise delay handling. >>>>>> >>>>>> Reported by checkpatch: >>>>>> >>>>>> CHECK: usleep_range is preferred over udelay >>>>> >>>>> This message is typically not a good candidate for outreachy patches, >>>>> because you need access to the device to be sure that any change is >>>>> correct. >>>> >>>> Could we add a paragraph on how to pick good checkpatch.pl error to fix to >>>> the Outreachyfirstpatch docs? This could go to "Find a driver to clean up" >>>> section, for example. >>> >>> The ability to find a "good" error changes over time, so this might be >>> hard to do. >> >> I agree, but we can all agree that experimenting with udelay during Outreachy >> is not a good idea, and people should know about it > > In general, I think that it is better in the contribution period to do the > wrong thing and then learn about why it is wrong, but this case comes up > over and over, and it is always not the right thing to do, so I added an > appropriate explanation. Thanks for the suggestion. Absolutely. Thanks for the docs update. Still, one thing -- is empty section after "Some drivers that are on their way out of the kernel are:" intentional? All the best, Karolina > > julia > >> >> All the best, >> Karolina >> >>> >>> good luck! >>> >>> greg k-h >>
On Wed, Oct 18, 2023 at 12:40:33PM +0200, Karolina Stolarek wrote: > On 18.10.2023 12:28, Julia Lawall wrote: > > > > > > On Wed, 18 Oct 2023, Karolina Stolarek wrote: > > > > > On 18.10.2023 09:45, Greg Kroah-Hartman wrote: > > > > On Wed, Oct 18, 2023 at 09:32:46AM +0200, Karolina Stolarek wrote: > > > > > On 18.10.2023 09:03, Julia Lawall wrote: > > > > > > > > > > > > > > > > > > On Tue, 17 Oct 2023, kenechukwu maduechesi wrote: > > > > > > > > > > > > > Replace udelay() with usleep_range() for more precise delay handling. > > > > > > > > > > > > > > Reported by checkpatch: > > > > > > > > > > > > > > CHECK: usleep_range is preferred over udelay > > > > > > > > > > > > This message is typically not a good candidate for outreachy patches, > > > > > > because you need access to the device to be sure that any change is > > > > > > correct. > > > > > > > > > > Could we add a paragraph on how to pick good checkpatch.pl error to fix to > > > > > the Outreachyfirstpatch docs? This could go to "Find a driver to clean up" > > > > > section, for example. > > > > > > > > The ability to find a "good" error changes over time, so this might be > > > > hard to do. > > > > > > I agree, but we can all agree that experimenting with udelay during Outreachy > > > is not a good idea, and people should know about it > > > > In general, I think that it is better in the contribution period to do the > > wrong thing and then learn about why it is wrong, but this case comes up > > over and over, and it is always not the right thing to do, so I added an > > appropriate explanation. Thanks for the suggestion. > > Absolutely. Thanks for the docs update. Still, one thing -- is empty section > after "Some drivers that are on their way out of the kernel are:" > intentional? > > All the best, > Karolina Perhaps an applicant can expand on that "Find a driver to clean up..." section with a overview of how to use the Outreachy Mailing list, or any Mailing list to search for patches that cleaned up up the checkpatch error/warning/check that they are examining. Here's the query for the udelay one: https://lore.kernel.org/outreachy/?q=%22CHECK%3A+usleep_range+is+preferred+over+udelay%22 When submitters include the actual checkpatch string in their commit log it very easy to find and reference. For the first patch tutorial, I'll suggest something as simple as: 1) goto https://lore.kernel.org/outreachy/ 2) enter the checkpatch string in the 'search' box 3) boom - go see what others have done with this checkpatch error Or, if someone wants to get fancy they can add screenshots :) Note, that we really like to keep the language similar on these cleanups. Find a few references, and do as they did. > > > > > julia > > > > > > > > All the best, > > > Karolina > > > > > > > > > > > good luck! > > > > > > > > greg k-h > > > >
Hi Kenechukwu, On Tue, Oct 17, 2023 at 06:03:05PM -0700, kenechukwu maduechesi wrote: > Replace udelay() with usleep_range() for more precise delay handling. The reason is not the precise handling, quite the opposite. > Reported by checkpatch: > > CHECK: usleep_range is preferred over udelay Can you tell why?[*] > Signed-off-by: kenechukwu maduechesi <maduechesik@gmail.com> > --- > drivers/staging/rts5208/sd.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/staging/rts5208/sd.c b/drivers/staging/rts5208/sd.c > index 74c4f476b3a4..059f99b0a727 100644 > --- a/drivers/staging/rts5208/sd.c > +++ b/drivers/staging/rts5208/sd.c > @@ -865,7 +865,7 @@ static int sd_change_phase(struct rtsx_chip *chip, u8 sample_point, u8 tune_dir) > PHASE_CHANGE); > if (retval) > return retval; > - udelay(50); > + usleep_range(50); What is the range you will be sleeping, now? Andi [*] Documentation/timers/timers-howto.rst
© 2016 - 2026 Red Hat, Inc.