Hi Richard, Am Montag, den 13.10.2014, 02:30 +0000 schrieb Hong-Xing.Zhu@xxxxxxxxxxxxx: > Hi Lucas: > > > -----Original Message----- > > From: linux-pci-owner@xxxxxxxxxxxxxxx [mailto:linux-pci-owner@xxxxxxxxxxxxxxx] > > On Behalf Of Lucas Stach > > Sent: Sunday, October 12, 2014 10:28 PM > > To: Richard Zhu > > Cc: linux-pci@xxxxxxxxxxxxxxx; Guo Shawn-R65073; festevam@xxxxxxxxx; > > tharvey@xxxxxxxxxxxxx; Zhu Richard-R65037 > > Subject: Re: [PATCH v5 4/9] PCI: imx6: add imx6sx pcie support > > > > Am Freitag, den 10.10.2014, 13:41 +0800 schrieb Richard Zhu: > > > From: Richard Zhu <r65037@xxxxxxxxxxxxx> > > > > > > - imx6sx pcie has its own standalone pcie power supply. > > > In order to turn on the imx6sx pcie power during initialization. Add > > > the pcie regulator and the gpc regmap into the imx6sx pcie structure. > > > - imx6sx pcie has the new added reset mechanism, add the reset > > > operations into the initialization. > > > - Register one PM call-back, enter/exit L2 state of the ASPM during > > > system suspend/resume. > > > - disp_axi clock is required by pcie inbound axi port actually. > > > Add one more clock named pcie_inbound_axi for imx6sx pcie. > > > > > Signed-off-by: Richard Zhu <richard.zhu@xxxxxxxxxxxxx> > > > > In addition to Fabios comment I have one additional nitpick below and also you > > didn't include all the feedback from the last round. > > > > > --- > > > drivers/pci/host/pci-imx6.c | 161 > > > +++++++++++++++++++++++++++++++++++++++----- > > > 1 file changed, 143 insertions(+), 18 deletions(-) > > > > > > diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c > > > index eac96fb..5ece4e1 100644 > > > --- a/drivers/pci/host/pci-imx6.c > > > +++ b/drivers/pci/host/pci-imx6.c > > > @@ -22,8 +22,10 @@ > > > #include <linux/pci.h> > > > #include <linux/platform_device.h> > > > #include <linux/regmap.h> > > > +#include <linux/regulator/consumer.h> > > > #include <linux/resource.h> > > > #include <linux/signal.h> > > > +#include <linux/syscore_ops.h> > > > #include <linux/types.h> > > > #include <linux/interrupt.h> > > > > > > @@ -35,11 +37,15 @@ struct imx6_pcie { > > > int reset_gpio; > > > struct clk *pcie_bus; > > > struct clk *pcie_phy; > > > + struct clk *pcie_inbound_axi; > > > struct clk *pcie; > > > struct pcie_port pp; > > > struct regmap *iomuxc_gpr; > > > + struct regmap *gpc_ips_reg; > > > + struct regulator *pcie_phy_regulator; > > > void __iomem *mem_base; > > > }; > > > +static struct imx6_pcie *imx6_pcie; > > > > Remove this static struct. I've already said on the last version that this is > > completely backwards. > > > [Richard] I added my comments in the last round. > Can't use imx6_pcie struct syscore suspend/resume, if the imx6_pcie is not a static type structure. > " > Hi Lucas: > Regarding to the definitions(pasted below) of the struct syscore_ops, both suspend and resume of the syscore_ops is void type functions. > If there is no the static global struct imx6_pcie, I don't know how it can be used in suspend/resume of pci_imx_syscore_ops. > struct syscore_ops { > struct list_head node; > int (*suspend)(void); > void (*resume)(void); > void (*shutdown)(void); > }; > " This only means syscore ops is not the right interface to use here. Introducing global static vars to hold driver instance state is a receipt to create trouble later on. Don't ever do this. I think it should be possible to move those calls to normal dev_pm_ops, maybe use suspend/resume_noirq or even poweroff/restore_noirq. Regards, Lucas -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
