> > > *mvebu_gpioreg_blink(struct mvebu_gpio_chip *mvchip) return > > > mvchip->membase + GPIO_BLINK_EN_OFF; } > > > > > > +static void __iomem *mvebu_gpioreg_blink_select(struct > > > mvebu_gpio_chip *mvchip) +{ > > > + return mvchip->membase + GPIO_BLINK_CNT_SELECT_OFF; > > > +} > > > > That's a really weird thing to do. Why not just use this expression in > > your calls to readl() and writel() directly? Seems a lot of additional > > code for no gain. This driver is made more complex by the Armada XP SMP handling. Some GPIO registers are per-cpu, others are global. Registers for interrupts in particular are per CPU. So there is a general trend in this driver to have a function which returns the address of a register, for a given SOC variant. In this case, it is fixed, so could be collapsed into the actual read/write. But then it would be different to all others in this driver... > > > + pwm->chip.dev = dev; > > > + pwm->chip.ops = &mvebu_pwm_ops; > > > + pwm->chip.base = mvchip->chip.base; > > > + pwm->chip.npwm = mvchip->chip.ngpio; > > > > Isn't that a lie? The code above suggests you can only ever have a > > single GPIO turn into a PWM, so I'd expect ".npwm = 1" here. Well, any of the 32 GPIOs can be a PWM. But only one can be enabled at a time. What exactly does pwm->chip.npwm mean? If we say 1 here, how at run time do we say which of the 32 GPIOs is used as a PWM output? By saying 32, it is simpler, which ever is enabled first is the one to use. Andrew -- 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