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.
> >+ 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 = 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 = 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 != ST_LPC_MODE_WDT)
> >+ return -ENODEV;
> >+
> >+ st_wdog = devm_kzalloc(&pdev->dev, sizeof(*st_wdog), GFP_KERNEL);
> >+ if (!st_wdog)
> >+ return -ENOMEM;
> >+
> >+ match = of_match_device(st_wdog_match, &pdev->dev);
> >+ if (!match) {
> >+ dev_err(&pdev->dev, "Couldn't match device\n");
> >+ return -ENODEV;
> >+ }
> >+ st_wdog->syscfg = (struct st_wdog_syscfg *)match->data;
> >+
> >+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >+ base = devm_ioremap_resource(&pdev->dev, res);
> >+ if (IS_ERR(base)) {
> >+ dev_err(&pdev->dev, "Failed to ioremap base\n");
>
> devm_ioremap_resource already displays an error message, do this one is unnecessary.
You're right. Will fix.
[...]
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
