On Fri, Oct 24, 2014 at 1:59 PM, Thomas Petazzoni
<thomas.petazzoni@xxxxxxxxxxxxxxxxxx> wrote:
> This commit adds the implementation of ->suspend() and ->resume()
> platform_driver hooks in order to save and restore the state of the
> GPIO configuration. In order to achieve that, additional fields are
> added to the mvebu_gpio_chip structure.
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@xxxxxxxxxxxxxxxxxx>
> Cc: Linus Walleij <linus.walleij@xxxxxxxxxx>
> Cc: Alexandre Courbot <gnurou@xxxxxxxxx>
> Cc: linux-gpio@xxxxxxxxxxxxxxx
(...)
> + mvchip->out_reg = readl(mvebu_gpioreg_out(mvchip));
> + mvchip->io_conf_reg = readl(mvebu_gpioreg_io_conf(mvchip));
> + mvchip->blink_en_reg = readl(mvebu_gpioreg_blink(mvchip));
> + mvchip->in_pol_reg = readl(mvebu_gpioreg_in_pol(mvchip));
OK...
> + switch (mvchip->soc_variant) {
> + case MVEBU_GPIO_SOC_VARIANT_ORION:
> + mvchip->edge_mask_regs[0] =
> + readl(mvchip->membase + GPIO_EDGE_MASK_OFF);
> + mvchip->level_mask_regs[0] =
> + readl(mvchip->membase + GPIO_LEVEL_MASK_OFF);
> + break;
You are assigning index [0] twice, why? There must be some
reason, and it should be stated in a comment. If the first read
is necessary for hardware reasons, don't assign it but
discard the result.
(void) readl(...);
(This pattern repeats for each save call below.)
> + switch (mvchip->soc_variant) {
> + case MVEBU_GPIO_SOC_VARIANT_ORION:
> + writel(mvchip->edge_mask_regs[0],
> + mvchip->membase + GPIO_EDGE_MASK_OFF);
> + writel(mvchip->level_mask_regs[0],
> + mvchip->membase + GPIO_LEVEL_MASK_OFF);
And on the way up same thing. Now you write
each register twice. Why?
Yours,
Linus Walleij
--
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
