On Wednesday 29 April 2015 01:49:53 fu.wei@linaro.org wrote:
From: Fu Wei fu.wei@linaro.org
This needs a changelog. Please explain what this is good for here.
+/*
- Architected system timer support.
- */
+static __always_inline
Just drop the __always_inline here, unless it's broken to do so (in which case you should add a comment). The compiler will inline static functions by itself, and you can use 'inline' to annotate the fact that you expect the function to be inlined.
+void sbsa_gwdt_reg_write(enum sbsa_gwdt_frame frame, unsigned int reg, u32 val,
struct watchdog_device *wdd)
+{
- struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
- struct sbsa_gwdt_device_data *device_data = &gwdt->device_data;
- switch (frame) {
- case SBSA_GWDT_RF:
writel_relaxed(val, device_data->refresh_base + reg);
break;
- case SBSA_GWDT_CF:
writel_relaxed(val, device_data->control_base + reg);
break;
- default:
pr_err("sbsa_gwdt: reg write fail(frame not available) \n");
- }
+}
[coding style]
How about having two separate functions to do this, it seems that would simplify both the wrapper and its callers.
+static long sbsa_gwdt_ioctl(struct watchdog_device *wdd, unsigned int cmd,
unsigned long arg)
+{
- struct sbsa_gwdt *gwdt = watchdog_get_drvdata(wdd);
- void __user *argp = (void __user *)arg;
- int ret = -ENOIOCTLCMD;
- unsigned int __user *p = argp;
- unsigned int new_value;
- int __user *preaction = argp;
- int new_preaction;
- /* FIXME: what else do we need here?? */
Answer: as little as possible.
- switch (cmd) {
- case WDIOC_SETPRETIMEOUT:
if (get_user(new_value, p))
return -EFAULT;
ret = sbsa_gwdt_set_pretimeout(wdd, new_value);
break;
- case WDIOC_GETPRETIMEOUT:
ret = put_user(gwdt->pretimeout, (unsigned int __user *)arg);
break;
- case SBSA_GWDIOC_SETPREACTION:
if (get_user(new_preaction, preaction))
return -EFAULT;
ret = sbsa_gwdt_set_preaction(gwdt, new_preaction);
break;
- case SBSA_GWDIOC_GETPREACTION:
ret = put_user(gwdt->preaction, (int __user *)arg);
break;
- }
- return ret;
+}
Don't introduce driver specific commands here. Instead, make a separate patch to add new generic callbacks so the user API is shared with all drivers.
It would be helpful to add support for the WDIOC_SETPRETIMEOUT and WDIOC_GETPRETIMEOUT calls in the watchdog core as well.
+static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id) +{
- struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;
- struct watchdog_device *wdd = &gwdt->wdd;
- u32 status;
- unsigned int left;
- pr_debug("SBSA Watchdog: in %s\n", __func__);
- status = sbsa_gwdt_reg_read(SBSA_GWDT_CF, SBSA_GWDT_WCS, wdd);
- if (status & SBSA_GWDT_WCS_WS0) {
if (printk_ratelimit()) {
pr_crit("SBSA Watchdog WS0 is triggered\n");
left = sbsa_gwdt_get_timeleft(wdd);
pr_crit("please try to feed the dog in %ds!\n", left);
}
/* FIXME:anything we can do here?
* send_signal(sig, info, p, group); ??
*/
if (gwdt->preaction == SBSA_GWDT_PREACT_FEEDDOG) {
sbsa_gwdt_keepalive(wdd);
pr_warn("kernel keep system running by feeding dog!\n");
What is the point of this? Is there any realistic use case for setting SBSA_GWDT_PREACT_FEEDDOG?
} else if (gwdt->preaction == SBSA_GWDT_PREACT_PANIC) {
panic("SBSA Watchdog pre-timeout");
}
Same question here: what is this good for?
+static int sbsa_gwdt_probe(struct platform_device *pdev) +{
- struct device *dev = &pdev->dev;
- struct sbsa_gwdt *gwdt;
- struct sbsa_gwdt_device_data *device_data;
- struct watchdog_device *wdd;
- struct resource *res;
- void *rf_base = NULL, *cf_base = NULL;
Don't initialize local variables to NULL unless you have to use that value later.
- res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
SBSA_GWDT_RF_RES_NAME);
- rf_base = devm_ioremap_resource(dev, res);
- if (IS_ERR(rf_base))
return PTR_ERR(rf_base);
- res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
SBSA_GWDT_CF_RES_NAME);
- cf_base = devm_ioremap_resource(dev, res);
- if (IS_ERR(rf_base))
return PTR_ERR(rf_base);
- res = platform_get_resource_byname(pdev, IORESOURCE_IRQ,
SBSA_GWDT_IRQ_RES_NAME);
Please open-code the resource names here, the macros make it harder to read what you do here.
Use platform_get_irq() here to allow deferred probing of the irqchip.
+static const struct of_device_id sbsa_gwdt_of_match[] = {
- { .compatible = "arm,sbsa_gwdt", },
- {},
+}; +MODULE_DEVICE_TABLE(of, sbsa_gwdt_of_match);
Please add a binding document for this that describes the mandatory properties and resources.
+#ifdef CONFIG_ACPI +static int __init map_generic_timer_interrupt(u32 interrupt, u32 flags) +{
- int trigger, polarity;
The ACPI parts of this patch should probably be in a separate file, and submitted as a separate patch.
+static int __always_inline acpi_gtdt_set_resource_irq_flags(u32 timer_flags) +{
- int flags;
- switch (timer_flags &
(ACPI_GTDT_INTERRUPT_MODE | ACPI_GTDT_INTERRUPT_POLARITY)) {
- case (ACPI_GTDT_INTERRUPT_MODE | ACPI_GTDT_INTERRUPT_POLARITY):
flags = IORESOURCE_IRQ_LOWEDGE | IORESOURCE_IRQ;
pr_info("sbsa_gwdt: device irq flags is low edge");
break;
- case ACPI_GTDT_INTERRUPT_MODE:
flags = IORESOURCE_IRQ_HIGHEDGE | IORESOURCE_IRQ;
pr_info("sbsa_gwdt: device irq flags is high edge");
break;
- case ACPI_GTDT_INTERRUPT_POLARITY:
flags = IORESOURCE_IRQ_LOWLEVEL | IORESOURCE_IRQ;
pr_info("sbsa_gwdt: device irq flags is low level");
break;
- default:
flags = IORESOURCE_IRQ_HIGHLEVEL | IORESOURCE_IRQ;
pr_info("sbsa_gwdt: device irq flags is high level");
- }
- pr_info("sbsa_gwdt: device irq flags:%d\n", flags);
- return flags;
+}
This all looks like code that should be handled by the ACPI core, and not be driver specific.
+/* Initialize the SBSA Generic Watchdog presented in GTDT */ +static int __init sbsa_gwdt_init(void) +{
- if (acpi_disabled)
return 0;
- return acpi_table_parse(ACPI_SIG_GTDT, sbsa_gwdt_acpi_init);
+}
+arch_initcall(sbsa_gwdt_init); +#endif
You can't have arch_initcall() and module_platform_driver() in the same file, it prevents you from making the driver a loadable module.
diff --git a/include/linux/sbsa_gwdt.h b/include/linux/sbsa_gwdt.h new file mode 100644 index 0000000..58425ef --- /dev/null +++ b/include/linux/sbsa_gwdt.h +/* refresh frame */ +#define SBSA_GWDT_WRR (0x000)
+/* control frame */ +#define SBSA_GWDT_WCS (0x000) +#define SBSA_GWDT_WOR (0x008) +#define SBSA_GWDT_WCV_LO (0x010) +#define SBSA_GWDT_WCV_HI (0x014)
+/* refresh/control frame */ +#define SBSA_GWDT_W_IIDR (0xfcc) +#define SBSA_GWDT_IDR (0xfd0)
+/* Watchdog Control and Status Register */ +#define SBSA_GWDT_WCS_EN (1 << 0) +#define SBSA_GWDT_WCS_WS0 (1 << 1) +#define SBSA_GWDT_WCS_WS1 (1 << 2)
Put all the register definitions into the driver itself. Only put things in a global header that need to be shared between multiple drivers. In case of a watchdog driver, that should be nothing.
+#define SBSA_GWDT_WS1_KERNEL_KEEPER IS_BUILTIN(CONFIG_SBSA_GWDT_WS1_KERNEL_KEEPER)
+#define SBSA_GWD_IOCTL_BASE ('S') +#define SBSA_GWDIOC_GETPREACTION _IOR(SBSA_GWD_IOCTL_BASE, 0, int) +#define SBSA_GWDIOC_SETPREACTION _IOWR(SBSA_GWD_IOCTL_BASE, 1, int)
ioctl definitions have to be in include/uapi/linux/ so they are visible to user space programs. Just put them into include/uapi/linux/watchdog.h and remove the SBSA_ prefix in the command and data structure names to make it easier to reuse these for other drivers.
Arnd