On Sat, Jan 14, 2017 at 03:05:30PM +0800, Baoyou Xie wrote: > +static int zx2967_reset_assert(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + struct zx2967_reset *reset = NULL; > + int bank = id / 32; > + int offset = id % 32; > + unsigned int reg;
u32 is probably better for register value. > + unsigned long flags; > + > + reset = container_of(rcdev, struct zx2967_reset, rcdev); > + > + spin_lock_irqsave(&reset->lock, flags); > + > + reg = readl(reset->reg_base + (bank * 4)); > + writel(reg & ~BIT(offset), reset->reg_base + (bank * 4)); > + reg = readl(reset->reg_base + (bank * 4)); Is this read on the register is necessary? If so, we should probably have a comment for that. > + > + spin_unlock_irqrestore(&reset->lock, flags); > + > + return 0; > +} > + > +static int zx2967_reset_deassert(struct reset_controller_dev *rcdev, > + unsigned long id) Please indent the line right after parentheses. > +{ > + struct zx2967_reset *reset = NULL; > + int bank = id / 32; > + int offset = id % 32; > + unsigned int reg; > + unsigned long flags; > + > + reset = container_of(rcdev, struct zx2967_reset, rcdev); > + > + spin_lock_irqsave(&reset->lock, flags); > + > + reg = readl(reset->reg_base + (bank * 4)); > + writel(reg | BIT(offset), reset->reg_base + (bank * 4)); > + reg = readl(reset->reg_base + (bank * 4)); > + > + spin_unlock_irqrestore(&reset->lock, flags); > + > + return 0; > +} Only difference between these two functions is only one line. Should we consolidate them a bit? Shawn