On Tue, Jul 26, 2022 at 4:24 AM Guenter Roeck <linux@xxxxxxxxxxxx> wrote: > [...] > > > > + > > > > + streamlabs_wdt->intf = intf; > > > > + usb_set_intfdata(intf, &streamlabs_wdt->wdt_dev); > > > > + watchdog_set_drvdata(&streamlabs_wdt->wdt_dev, streamlabs_wdt); > > > > + watchdog_set_nowayout(&streamlabs_wdt->wdt_dev, nowayout); > > > > + > > > > + retval = usb_streamlabs_wdt_stop(&streamlabs_wdt->wdt_dev); > > > > + if (retval) > > > > + return -ENODEV; > > > > + > > > > > > A comment explaining why the watchdog is explicitly stopped when running > > > might be useful. > > > > What do you mean by saying "when running"? > > Everytime during my testing the initial state is "stopped" on > > boot/power on/after reset, so not sure what you mean by saying "when > > running". > > Should I have used the term "active" ? "started" ? > > > There is a comment above that explains the stop command but I will > > add/change comments that explain things better. > > The point of executing "stop" command here is to check that device > > being probed behaves like we expect it to. This is a bit paranoid > > check since I am a not 100% sure that all devices with such USB ids > > are watchdogs -- that's why additional checks for usbdev->product and > > usbdev->manufacturer and this stop command that doesn't change the > > initial state. In theory, I can remove this stop command at all. > > > > Normally one does not want a watchdog to stop if it is running (started ? > active ? pick your term) when the device is instantiated to avoid gaps > in watchdog coverage. The watchdog core provides a flag, WDOG_HW_RUNNING, > for this purpose (sorry, there is the 'running' term again). It is used > by many drivers, and ensures that the time from loading the Linux kernel > to opening the watchdog device is protected by the watchdog. > > Calling the stop function during probe suggests that you at least > considered the possibility that the watchdog may be running/started/active. > Per your explanation, you still want it to stop explicitly if/when that > happens. All I am asking you is to add a comment explaining why this is > not needed/wanted/relevant/supported for this driver. One explanation > might, for example, be that the state can not be detected reliably. Thanks for the explanation. I was confused initially by the phrase "explicitly stopped when running" since in reality it is "explicitly stopped when stopped" (or not active, not running). On the second thought, I can issue a start command and indicate to the watchdog core via set_bit(WDOG_HW_RUNNING, &wdd->status) that it is running or return -ENODEV if the start fails instead of stopping the watchdog. I just would like to have a command sent to the device in ->probe() that checks that the device behaves like expected. If there are no objects I'll change it like this. Best regards, Alexey
