> -----Original Message----- > From: Fabio Estevam [mailto:festevam@xxxxxxxxx] > Sent: 2015年7月29日 16:21 > To: Wang Shenwei-B38339 > Cc: Greg Kroah-Hartman; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; > linux-serial@xxxxxxxxxxxxxxx > Subject: Re: [PATCH v3 1/1] Serial: imx: add dev_pm_ops to support suspend to > ram/disk > > On Wed, Jul 29, 2015 at 5:58 PM, Shenwei Wang <shenwei.wang@xxxxxxxxxxxxx> > wrote: > > > +static int imx_serial_port_suspend_noirq(struct device *dev) { > > + struct platform_device *pdev = to_platform_device(dev); > > + struct imx_port *sport = platform_get_drvdata(pdev); > > + > > + /* Save necessary regs */ > > + clk_enable(sport->clk_ipg); > > clk_enable() may fail, so you should check its return value. The check seems a little superfluous because the failure condition for clk_enable is (clk == NULL || IS_ERR(clk)) Once this function is called which means the initialization of the driver is successful. > > + sport->saved_reg[0] = readl(sport->port.membase + UCR1); > > + sport->saved_reg[1] = readl(sport->port.membase + UCR2); > > + sport->saved_reg[2] = readl(sport->port.membase + UCR3); > > + sport->saved_reg[3] = readl(sport->port.membase + UCR4); > > + sport->saved_reg[4] = readl(sport->port.membase + UFCR); > > + sport->saved_reg[5] = readl(sport->port.membase + UESC); > > + sport->saved_reg[6] = readl(sport->port.membase + UTIM); > > + sport->saved_reg[7] = readl(sport->port.membase + UBIR); > > + sport->saved_reg[8] = readl(sport->port.membase + UBMR); > > + sport->saved_reg[9] = readl(sport->port.membase + IMX21_UTS); > > + clk_disable(sport->clk_ipg); > > + > > + dev_dbg(dev, "0x%p (%d)\r\n", sport->port.membase, > > + sport->port.line); > > + > > + return 0; > > +} > > + > > +static int imx_serial_port_resume_noirq(struct device *dev) { > > + struct platform_device *pdev = to_platform_device(dev); > > + struct imx_port *sport = platform_get_drvdata(pdev); > > + > > + clk_enable(sport->clk_ipg); > > Same here. > > > + writel(sport->saved_reg[4], sport->port.membase + UFCR); > > + writel(sport->saved_reg[5], sport->port.membase + UESC); > > + writel(sport->saved_reg[6], sport->port.membase + UTIM); > > + writel(sport->saved_reg[7], sport->port.membase + UBIR); > > + writel(sport->saved_reg[8], sport->port.membase + UBMR); > > + writel(sport->saved_reg[9], sport->port.membase + IMX21_UTS); > > + writel(sport->saved_reg[0], sport->port.membase + UCR1); > > + writel(sport->saved_reg[1] | 0x1, sport->port.membase + UCR2); > > Why this magic '| 0x1'? Can't you use a define instead? Yes, should be a macro. Thanks, Shenwei ��.n��������+%������w��{.n�����{��ǫ����{ay� ʇڙ���f���h������_�(�階�ݢj"��������G����?���&��
