Re: [PATCH v2 2/4] watchdog: i6300esb: support multiple devices

[Guenter Roeck <linux@roeck-us.net>]

On 10/26/2017 04:35 AM, Radu Rendec wrote:
> On Wed, 2017-10-25 at 13:55 -0700, Guenter Roeck wrote:
>> On Wed, Oct 25, 2017 at 04:39:12PM +0100, Radu Rendec wrote:
>>> -/* We can only use 1 card due to the /dev/watchdog restriction */
>>> +/* Probed devices counter; used only for printing the initial info message */
>>>   static int cards_found;
>>
>> Now unnecessary.
> 
> [snip]
> 
>>>   static int esb_probe(struct pci_dev *pdev,
>>>   		const struct pci_device_id *ent)
>>>   {
>>> +	struct esb_dev *edev;
>>>   	int ret;
>>>   
>>>   	cards_found++;
>>
>> Now unnecessary.
> 
> This variable is still used for printing the module "greeting" message
> only when the first device is probed. The code snippet is below, but
> unfortunately the diff context doesn't include the condition that tests
> cards_found (the line was originally there, I didn't touch it).
> 
>>> @@ -309,26 +313,33 @@ static int esb_probe(struct pci_dev *pdev,
>>>   		pr_info("Intel 6300ESB WatchDog Timer Driver v%s\n",
>>>   			ESB_VERSION);
>>>   
> 
> So we should either keep the variable or drop the "greeting" message
> altogether. Perhaps it makes more sense if we use it as a bool (rather
> than a counter)? Something like:
> 
> if (!cards_found) {
> 	pr_info(...);
> 	cards_found = 1;
> }
> 

Just drop that pr_info. There is an info message at the end of the probe
function which is sufficient. Make that dev_info if you didn't already,
and it will report additional instance data automatically.

> I fixed the other issues and I will submit a new version of the patch
> series as soon as I get your input on this one.
> 
> Thanks,
> Radu
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>