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

[Frans Klaver]

On Mon, Jun 26, 2017 at 11:12 AM, Simo Koskinen <koskisoft@xxxxxxxxx> wrote:
> 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?

The warning suggests you change this to

dev_dbg(rtsx_dev(chip), "chip->need_reset = 0x%x (%s)\n", (unsigned
int)(chip->need_reset), __func__);

It doesn't mention the chip->need_reset part. The reason is simply
that when the function name changes, this doesn't have to be updated.
That same reasoning doesn't hold up for when something changes in
"chip->need_reset".

Frans