Re: [PATCH] Staging : rts5208 : checkpatch.pl fixes

Simo Koskinen
2017-06-26T09:13:01Z

On Fri, Jun 23, 2017 at 5:59 PM, Joe Perches <joe@perches.com> wrote:
> On Fri, 2017-06-23 at 15:55 +0200, Simo Koskinen wrote:
>> Fixed some issues reported by checkpatch.pl script.
> []
>> diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c
>> index b8177f5..ceef5fc 100644
>> --- a/drivers/staging/rts5208/rtsx.c
>> +++ b/drivers/staging/rts5208/rtsx.c
>> @@ -1009,7 +1009,7 @@ static void rtsx_remove(struct pci_dev *pci)
>> {
>> struct rtsx_dev *dev = pci_get_drvdata(pci);
>>
>> - dev_info(&pci->dev, "rtsx_remove() called\n");
>> + dev_info(&pci->dev, "%s called\n", "rtsx_remove()");
>
> This would be better as dev_dbg
True, I can change that...

>>
>> quiesce_and_remove_host(dev);
>> release_everything(dev);
>> diff --git a/drivers/staging/rts5208/rtsx_chip.c b/drivers/staging/rts5208/rtsx_chip.c
>> index 7f4107b..892b97a 100644
>> --- a/drivers/staging/rts5208/rtsx_chip.c
>> +++ b/drivers/staging/rts5208/rtsx_chip.c
>> @@ -616,8 +616,10 @@ int rtsx_reset_chip(struct rtsx_chip *chip)
>> else
>> retval = rtsx_pre_handle_sdio_new(chip);
>>
>> - dev_dbg(rtsx_dev(chip), "chip->need_reset = 0x%x (rtsx_reset_chip)\n",
>> - (unsigned int)(chip->need_reset));
>> + dev_dbg(rtsx_dev(chip), "%s = 0x%x (%s)\n",
>> + "chip->need_reset",
>> + (unsigned int)(chip->need_reset),
>> + "rtsx_reset_chip");
>
> This and other changes that take part of the format
> and convert them to '"%s", substrings' are not good.
> checkpatch doesn't emit a warning for long formats.
The reason for changes were the following warning when run the
checkpatch.pl script:

WARNING: Prefer using '"%s...", __func__' to using 'rtsx_reset_chip',
this function's name, in a string
#619: FILE: rtsx_chip.c:619:
+ dev_dbg(rtsx_dev(chip), "chip->need_reset = 0x%x (rtsx_reset_chip)\n",

So it's not a good idea to fix these warnings?

On Fri, Jun 23, 2017 at 5:59 PM, Joe Perches <joe@perches.com> wrote:
> On Fri, 2017-06-23 at 15:55 +0200, Simo Koskinen wrote:
>> Fixed some issues reported by checkpatch.pl script.
> []
>> diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c
>> index b8177f5..ceef5fc 100644
>> --- a/drivers/staging/rts5208/rtsx.c
>> +++ b/drivers/staging/rts5208/rtsx.c
>> @@ -1009,7 +1009,7 @@ static void rtsx_remove(struct pci_dev *pci)
>> {
>> struct rtsx_dev *dev = pci_get_drvdata(pci);
>>
>> - dev_info(&pci->dev, "rtsx_remove() called\n");
>> + dev_info(&pci->dev, "%s called\n", "rtsx_remove()");
>
> This would be better as dev_dbg
>>
>> quiesce_and_remove_host(dev);
>> release_everything(dev);
>> diff --git a/drivers/staging/rts5208/rtsx_chip.c b/drivers/staging/rts5208/rtsx_chip.c
>> index 7f4107b..892b97a 100644
>> --- a/drivers/staging/rts5208/rtsx_chip.c
>> +++ b/drivers/staging/rts5208/rtsx_chip.c
>> @@ -616,8 +616,10 @@ int rtsx_reset_chip(struct rtsx_chip *chip)
>> else
>> retval = rtsx_pre_handle_sdio_new(chip);
>>
>> - dev_dbg(rtsx_dev(chip), "chip->need_reset = 0x%x (rtsx_reset_chip)\n",
>> - (unsigned int)(chip->need_reset));
>> + dev_dbg(rtsx_dev(chip), "%s = 0x%x (%s)\n",
>> + "chip->need_reset",
>> + (unsigned int)(chip->need_reset),
>> + "rtsx_reset_chip");
>
> This and other changes that take part of the format
> and convert them to '"%s", substrings' are not good.
> checkpatch doesn't emit a warning for long formats.
>
>> diff --git a/drivers/staging/rts5208/sd.c b/drivers/staging/rts5208/sd.c
> []
>> @@ -910,8 +910,12 @@ static int sd_change_phase(struct rtsx_chip *chip, u8 sample_point, u8 tune_dir)
>> int retval;
>> bool ddr_rx = false;
>>
>> - dev_dbg(rtsx_dev(chip), "sd_change_phase (sample_point = %d, tune_dir = %d)\n",
>> - sample_point, tune_dir);
>> + dev_dbg(rtsx_dev(chip), "%s (%s = %d, %s = %d)\n",
>> + "sd_change_phase",
>> + "sample_point",
>> + sample_point,
>> + "tune_dir",
>> + tune_dir);
>
> etc.
>