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

[Simo Koskinen]

On Fri, Jun 23, 2017 at 5:59 PM, Joe Perches <joe@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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.
>