On Thursday 04 December 2014 04:23:44 vishnupatekar wrote: > + > +#define DRIVER_NAME "sunxi-ps2" > + > +#define RESSIZE(res) (((res)->end - (res)->start)+1) Remove this and use the existing resource_size() function > + > +struct sunxips2data { > + int irq; > + spinlock_t ps2_lock; > + void __iomem *base_address; /* virt address of control registers*/ > + struct serio *serio; /* serio*/ > + struct device *dev; > + struct clk *pclk; > +}; As this is dynamically allocated, better embed the serio member directly to avoid allocating both separately. > +static int sunxips2_open(struct serio *pserio) > +{ > + struct sunxips2data *drvdata = pserio->port_data; > + u32 src_clk = 0; > + u32 clk_scdf; > + u32 clk_pcdf; > + u32 rval; > + > + /*Set Line Control And Enable Interrupt*/ > + rval = SWPS2_LCTL_TXDTOEN|SWPS2_LCTL_STOPERREN|SWPS2_LCTL_ACKERREN|SWPS2_LCTL_PARERREN|SWPS2_LCTL_RXDTOEN; > + writel(rval, drvdata->base_address + SW_PS2_LCTRL); > + > + /*Reset FIFO*/ > + writel(0x3<<16 | 0x607, drvdata->base_address + SW_PS2_FCTRL); > + > + src_clk = clk_get_rate(drvdata->pclk); > + > + if (!src_clk) { > + dev_info(drvdata->dev, "w_ps2c_set_sclk error, source clock is 0."); > + return -1; > + } > + > + /*Set Clock Divider Register*/ > + clk_scdf = ((src_clk + (SW_PS2_SAMPLE_CLK>>1)) / SW_PS2_SAMPLE_CLK - 1); > + clk_pcdf = ((SW_PS2_SAMPLE_CLK + (SW_PS2_SCLK>>1)) / SW_PS2_SCLK - 1); > + rval = (clk_scdf<<8) | clk_pcdf;/* | (PS2_DEBUG_SEL<<16);*/ > + writel(rval, drvdata->base_address + SW_PS2_CLKDR); > + > + /*Set Global Control Register*/ > + rval = SWPS2_RESET|SWPS2_INTEN|SWPS2_MASTER|SWPS2_BUSEN; > + writel(rval, drvdata->base_address + SW_PS2_GCTRL); > + > + udelay(100); 100 microseconds is a rather long time to block the CPU for, so this needs a comment explaining why the particular delay is needed and why you can't use usleep_range() instead. > +static void sunxips2_close(struct serio *pserio) > +{ > + struct sunxips2data *drvdata = pserio->port_data; > + > + spin_lock(&drvdata->ps2_lock); > + /* Disable the PS2 interrupts */ > + writel(0, drvdata->base_address + SW_PS2_GCTRL); > + spin_unlock(&drvdata->ps2_lock); > +} The locking is wrong here: you take the lock without disabling the interrupts first, so if the interrupt happens between the spin_lock() call and the writel(), the kernel will deadlock. You will either have to use spin_lock_irq() here, or find a justification for dropping the lock entirely. > +static int sunxips2_write(struct serio *pserio, unsigned char val) > +{ > + struct sunxips2data *drvdata = (struct sunxips2data *)pserio->port_data; > + u32 timeout = 10000; > + > + do { > + if (readl(drvdata->base_address + SW_PS2_FSTAT) & SWPS2_FSTA_TXRDY) { > + writel(val, drvdata->base_address + SW_PS2_DATA); > + return 0; > + } > + } while (timeout--); > + > + return -1; > +} We never return '-1' from in-kernel functions. Either make this a bool argument, or return a proper errno.h value. This should probably return -EIO. > + drvdata->irq = irq; > + drvdata->serio = serio; > + drvdata->dev = dev; > + error = devm_request_any_context_irq(drvdata->dev, drvdata->irq, &sunxips2_interrupt, 0, > + DRIVER_NAME, drvdata); > + if (error) { > + dev_err(drvdata->dev, > + "Couldn't allocate interrupt %d : error: %d\n", drvdata->irq, error); > + return error; > + } > + return 0; /* success */ > +} Why any_context? Arnd -- 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