Messages in this thread | | | Date | Thu, 22 Jan 2015 11:31:45 -0800 | From | Guenter Roeck <> | Subject | Re: [PATCH v2 6/9] watchdog: st_wdt: Add new driver for ST's LPC Watchdog |
| |
On 01/22/2015 09:21 AM, Lee Jones wrote: > On Thu, 22 Jan 2015, Guenter Roeck wrote: > >> On 01/22/2015 03:56 AM, Lee Jones wrote: >>> Signed-off-by: David Paris <david.paris@st.com> >>> Signed-off-by: Lee Jones <lee.jones@linaro.org> >>> --- >>> drivers/watchdog/Kconfig | 13 ++ >>> drivers/watchdog/Makefile | 1 + >>> drivers/watchdog/st_lpc_wdt.c | 335 ++++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 349 insertions(+) >>> create mode 100644 drivers/watchdog/st_lpc_wdt.c > > [...] > >>> +struct st_wdog_syscfg { >>> + struct regmap *regmap; >> >> Hi David & Lee, >> >> Why did you add the regmap pointer to this structure and not to st_wdog ? >> It is not a configuration value, after all, and it is always dereferenced >> through wt_wdog->syscfg. So all it does is to make the code more complicated. >> >> Am I missing something ? > > I guess it was just to group it all together, as it will all be used > at the same time: > > regmap_update_bits(st_wdog->syscfg->regmap, > st_wdog->syscfg->reset_type_reg, > st_wdog->syscfg->reset_type_mask, > st_wdog->warm_reset); > > It really doesn't matter to me either way. > Having it in syscfg means it is stored in the static configuration data instead of the allocated data, which is at least conceptually wrong. So I think it should be in st_wdog.
Thanks, Guenter
| |