Re: [PATCH net v2 2/2] ptp: idt82p33: implement double dco time correction

On Fri, 17 Sep 2021 20:19:08 +0000 Min Li wrote:

quoted

quoted

@@ -29,6 +29,14 @@ module_param(phase_snap_threshold, uint, 0);

MODULE_PARM_DESC(phase_snap_threshold,
 "threshold (1000ns by default) below which adjtime would ignore");

+static bool delayed_accurate_adjtime = false;
+module_param(delayed_accurate_adjtime, bool, false);
+MODULE_PARM_DESC(delayed_accurate_adjtime,
+"set to true to use more accurate adjtime that is delayed to next
+1PPS signal");  

Module parameters are discouraged. If you have multiple devices on the
system module parameters don't allow setting different options depending
on device. Unless Richard or someone else suggests a better API for this
please use something like devlink params instead (and remember to
document them).
  

quoted

+static char *firmware;
+module_param(firmware, charp, 0);  

Yes, this was suggested by Richard back then

  > On Fri, Jun 25, 2021 at 02:24:24PM +0000, Min Li wrote:
  > > How would you suggest to implement the change that make the new driver behavior optional?
  > I would say, module parameter or debugfs knob.

Aright, in which case devlink or debugfs, please.

quoted

What's the point of this? Just rename the file in the filesystem.  

We use this parameter to specify firmware so that module can be autoloaded
/etc/modprobe.d/modname.conf

Sorry, I don't understand. The firmware is in /lib/firmware.
Previously you used a card coded name (whatever FW_FILENAME
is, "idt82p33xxx.bin"?). This patch adds the ability to change 
the firmware file name by a module param.

Now let me repeat the question - what's the point of user changing
the requested firmware name if they can simply rename the file?