lkml.org 
[lkml]   [2015]   [Jan]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 6/9] watchdog: st_wdt: Add new driver for ST's LPC Watchdog
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.

> >+ 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


\
 
 \ /
  Last update: 2015-01-22 18:41    [W:0.041 / U:0.600 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site