From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lee Jones Subject: Re: [PATCH v2 6/9] watchdog: st_wdt: Add new driver for ST's LPC Watchdog Date: Thu, 22 Jan 2015 17:21:00 +0000 Message-ID: <20150122172100.GF9129@x1> References: <1421927767-28889-1-git-send-email-lee.jones@linaro.org> <1421927767-28889-7-git-send-email-lee.jones@linaro.org> <54C12F10.4090901@roeck-us.net> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Content-Disposition: inline In-Reply-To: <54C12F10.4090901-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Guenter Roeck Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, kernel-F5mvAk5X5gdBDgjK7y7TUQ@public.gmane.org, rtc-linux-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org, wim-IQzOog9fTRqzQB+pC5nmwQ@public.gmane.org, linux-watchdog-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, david.paris-qxv4g6HH51o@public.gmane.org List-Id: devicetree@vger.kernel.org On Thu, 22 Jan 2015, Guenter Roeck wrote: > On 01/22/2015 03:56 AM, Lee Jones wrote: > >Signed-off-by: David Paris > >Signed-off-by: Lee Jones > >--- > > 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; >=20 > Hi David & Lee, >=20 > Why did you add the regmap pointer to this structure and not to st_wd= og ? > It is not a configuration value, after all, and it is always derefere= nced > through wt_wdog->syscfg. So all it does is to make the code more comp= licated. >=20 > 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. > >+ unsigned int reset_type_reg; > >+ unsigned int reset_type_mask; > >+ unsigned int enable_reg; > >+ unsigned int enable_mask; > >+}; > >+ > >+struct st_wdog { > >+ void __iomem *base; > >+ struct device *dev; > >+ struct st_wdog_syscfg *syscfg; > >+ struct clk *clk; > >+ unsigned long clkrate; > >+ bool warm_reset; > >+}; [...] > >+static int st_wdog_probe(struct platform_device *pdev) > >+{ > >+ const struct of_device_id *match; > >+ struct device_node *np =3D pdev->dev.of_node; > >+ struct st_wdog *st_wdog; > >+ struct regmap *regmap; > >+ struct resource *res; > >+ struct clk *clk; > >+ void __iomem *base; > >+ uint32_t mode; > >+ int ret; > >+ > >+ ret =3D of_property_read_u32(np, "st,lpc-mode", &mode); > >+ if (ret) { > >+ dev_err(&pdev->dev, "An LPC mode must be provided\n"); > >+ return -EINVAL; > >+ } > >+ > >+ /* LPC can either run in RTC or WDT mode */ > >+ if (mode !=3D ST_LPC_MODE_WDT) > >+ return -ENODEV; > >+ > >+ st_wdog =3D devm_kzalloc(&pdev->dev, sizeof(*st_wdog), GFP_KERNEL)= ; > >+ if (!st_wdog) > >+ return -ENOMEM; > >+ > >+ match =3D of_match_device(st_wdog_match, &pdev->dev); > >+ if (!match) { > >+ dev_err(&pdev->dev, "Couldn't match device\n"); > >+ return -ENODEV; > >+ } > >+ st_wdog->syscfg =3D (struct st_wdog_syscfg *)match->data; > >+ > >+ res =3D platform_get_resource(pdev, IORESOURCE_MEM, 0); > >+ base =3D devm_ioremap_resource(&pdev->dev, res); > >+ if (IS_ERR(base)) { > >+ dev_err(&pdev->dev, "Failed to ioremap base\n"); >=20 > devm_ioremap_resource already displays an error message, do this one = is unnecessary. You're right. Will fix. [...] --=20 Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org =E2=94=82 Open source software for ARM SoCs =46ollow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe devicetree" i= n the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html