Re: [PATCH v2 6/9] watchdog: st_wdt: Add new driver for ST's LPC Watchdog

[Guenter Roeck]

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@xxxxxx>
> > > Signed-off-by: Lee Jones <lee.jones@xxxxxxxxxx>
> > > ---
> > >   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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/