RE: [PATCH v1 0/2] Watchdog Core Global Parameters

[Flavio Suligoi <f.suligoi@asem.it>]

Hi Guenter,

> > Instead of adding this kind of module parameters independently to each
> > driver, the best solution is declaring each feature only once, in the
> > watchdog core.
> >
> 
> I agree to and like the idea, but I don't see the point of letting drivers opt in
> or opt out. This adds a lot of complexity for little if any gain.

Do you mean that all the support for this "global parameters" should be done
in the watchdog-core only, without write any code in each single
"hardware" driver?
> 
> Guenter

Regards,

Flavio

> 
> > Additionally, I added a implementation example of this "global"
> > parameters using the module "wdat_wdt"
> >
> > In details:
> >
> > ===============================
> > Watchdog Core Global Parameters
> > ===============================
> >
> > Information for watchdog kernel modules developers.
> >
> > Introduction
> > ============
> >
> > Different watchdog modules frequently require the same type of
> > parameters (for example: *timeout*, *nowayout* feature,
> > *start_enabled* to start the watchdog on module insertion, etc.).
> > Instead of adding this kind of module parameters independently to each
> > driver, the best solution is declaring each feature only once, in the
> > watchdog core.
> >
> > In this way, each driver can read these "global" parameters and then,
> > if needed, can implement them, according to the particular hw watchdog
> > characteristic.
> >
> > Using this approach, it is possible reduce some duplicate code in the
> > *new* watchdog drivers and simplify the code maintenance.  Moreover,
> > the code will be clearer, since the same kind of parameters are often
> > called with different names (see Documentation/watchdog/watchdog-
> parameters.rst).
> > Obviously, for compatibility reasons, we cannot remove the already
> > existing parameters from the code of the various watchdog modules, but
> > we can use this "global" approach for the new watchdog drivers.
> >
> >
> > Global parameters declaration
> > ==============================
> >
> > The global parameters data structure is declared in
> > include/linux/watchdog.h, as::
> >
> > 	struct watchdog_global_parameters_struct {
> > 		int timeout;
> > 		int ioport;
> > 		int irq;
> > 		unsigned long features;
> > 		/* Bit numbers for features flags */
> > 		#define WDOG_GLOBAL_PARAM_VERBOSE	0
> > 		#define WDOG_GLOBAL_PARAM_TEST_MODE	1
> > 		#define WDOG_GLOBAL_PARAM_START_ENABLED	2
> > 		#define WDOG_GLOBAL_PARAM_NOWAYOUT	3
> > 	};
> >
> > The variable "feature" is a bitwise flags container, to store boolean
> > features, such as:
> >
> > * nowayout
> > * start_enable
> > * etc...
> >
> > Other variables can be added, to store some numerical values and other
> > data required.
> >
> > The global parameters are declared (as usual for the module
> > parameters) in the first part of drivers/watchdog/watchdog_core.c file.
> > The above global data structure is then managed by the function *void
> > global_parameters_init()*, in the same file.
> >
> > Global parameters use
> > =====================
> >
> > Each watchdog driver, to check if one of the global parameters is
> > enabled, can use the corresponding in-line function declared in
> > include/linux/watchdog.h.
> > At the moment the following functions are ready to use:
> >
> > * watchdog_global_param_verbose_enabled()
> > * watchdog_global_param_test_mode_enabled()
> > * watchdog_global_param_start_enabled()
> > * watchdog_global_param_nowayout_enabled()
> >
> >
> >
> > Flavio Suligoi (2):
> >   watchdog: add global watchdog kernel module parameters structure
> >   watchdog: wdat: add start_enable global parameter
> >
> >  Documentation/watchdog/index.rst              |  1 +
> >  .../watchdog-core-global-parameters.rst       | 74 +++++++++++++++++++
> >  drivers/watchdog/watchdog_core.c              | 74 +++++++++++++++++++
> >  drivers/watchdog/wdat_wdt.c                   |  2 +
> >  include/linux/watchdog.h                      | 42 +++++++++++
> >  5 files changed, 193 insertions(+)
> >  create mode 100644
> > Documentation/watchdog/watchdog-core-global-parameters.rst
> >