On Fri, Feb 28, 2014 at 11:57:45AM -0600, Felipe Balbi wrote:
> On Fri, Feb 28, 2014 at 09:37:02AM -0800, Greg KH wrote:
> > On Fri, Feb 28, 2014 at 02:25:31PM +0900, Jingoo Han wrote:
> > > On Thursday, February 27, 2014 11:41 PM, Roger Quadros wrote:
> > > > On 02/27/2014 01:47 PM, Jingoo Han wrote:
> > > > > Use SIMPLE_DEV_PM_OPS macro in order to make the code simpler.
> > > > >
> > > > > Signed-off-by: Jingoo Han <jg1.han@xxxxxxxxxxx>
> > > > > ---
> > > > > drivers/usb/phy/phy-am335x.c | 4 +---
> > > > > 1 file changed, 1 insertion(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/usb/phy/phy-am335x.c b/drivers/usb/phy/phy-am335x.c
> > > > > index 12fc346..ebf8373 100644
> > > > > --- a/drivers/usb/phy/phy-am335x.c
> > > > > +++ b/drivers/usb/phy/phy-am335x.c
> > > > > @@ -123,9 +123,7 @@ static int am335x_phy_resume(struct device *dev)
> > > > > return 0;
> > > > > }
> > > > >
> > > > > -static const struct dev_pm_ops am335x_pm_ops = {
> > > > > - SET_SYSTEM_SLEEP_PM_OPS(am335x_phy_suspend, am335x_phy_resume)
> > > > > -};
> > > > > +static SIMPLE_DEV_PM_OPS(am335x_pm_ops, am335x_phy_suspend, am335x_phy_resume);
> > > >
> > > > You need to move this outside the #ifdef CONFIG_PM_SLEEP condition and get rid
> > > > of the DEV_PM_OPS macro below as well.
> > >
> > > 1. Your suggestion - Removing '#define DEV_PM_OPS NULL'
> > >
> > > #ifdef CONFIG_PM_SLEEP
> > > static int am335x_phy_suspend(struct device *dev)
> > > static int am335x_phy_resume(struct device *dev)
> > > #endif
> > > static SIMPLE_DEV_PM_OPS(am335x_pm_ops, am335x_phy_suspend, am335x_phy_resume);
> > >
> > > In this case, code can be simpler. However, a dev_pm_ops structure is
> > > created, and it requires additional NULL checking by PM framework,
> > > when "CONFIG_PM_SLEEP=n && CONFIG_PM=y".
> > >
> > > 2. Current code - Keeping '#define DEV_PM_OPS NULL'
> > >
> > > #ifdef CONFIG_PM_SLEEP
> > > static int am335x_phy_suspend(struct device *dev)
> > > static int am335x_phy_resume(struct device *dev)
> > > static SIMPLE_DEV_PM_OPS(am335x_pm_ops, am335x_phy_suspend, am335x_phy_resume);
> > > #define DEV_PM_OPS (&am335x_pm_ops)
> > > #else
> > > #define DEV_PM_OPS NULL
> > > #endif
> > >
> > > In this case, code size is larger. However, a dev_pm_ops structure is
> > > NOT created, and it does NOT require additional NULL checking by PM framework,
> > > when "CONFIG_PM_SLEEP=n && CONFIG_PM=y".
> > >
> > > Personally, I prefer the first one. Additional NULL checking is not
> > > crucial. Is there any other opinions?
> >
> > Yes, none of this at all, just fix the structures to always have the
> > fields no matter what the config option is, and then deal with it in the
>
> the fields in dev_pm_ops *are* always available the problem is with
> SET_SYSTEM_SLEEP_PM_OPS and SET_PM_RUNTIME_OPS which do nothing if
> !PM_{SLEEP,RUNTIME}.
Great, then this is a self-inflicted driver issue that can easily be
fixed in each driver, no need for this crazy PM_OPS mess.
> I would be more than glad if someone got PM fellows to accept removing
> the ifdeffery around both macros and just *always* leave PM hooks
> available and ready to go.
Why are the macros even needed?
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
