RE: [PATCH 1/3] soc: fsl: add Platform PM driver QorIQ platforms

[Ran Wang]

Hi Dongsheng

> On 2018/9/5 11:05, Dongsheng Wang wrote:
> 
> Please change your comments style.
> 
> On 2018/8/31 11:57, Ran Wang wrote:
> > This driver is to provide a independent framework for PM service
> > provider and consumer to configure system level wake up feature. For
> > example, RCPM driver could register a callback function on this
> > platform first, and Flex timer driver who want to enable timer wake up
> > feature, will call generic API provided by this platform driver, and
> > then it will trigger RCPM driver to do it. The benefit is to isolate
> > the user and service, such as flex timer driver will not have to know
> > the implement details of wakeup function it require. Besides, it is
> > also easy for service side to upgrade its logic when design is changed
> > and remain user side unchanged.
> >
> > Signed-off-by: Ran Wang <ran.wang_1@xxxxxxx>
> > ---
> >  drivers/soc/fsl/Kconfig   |   14 +++++
> >  drivers/soc/fsl/Makefile  |    1 +
> >  drivers/soc/fsl/plat_pm.c |  144
> +++++++++++++++++++++++++++++++++++++++++++++
> >  include/soc/fsl/plat_pm.h |   22 +++++++
> >  4 files changed, 181 insertions(+), 0 deletions(-)  create mode
> > 100644 drivers/soc/fsl/plat_pm.c  create mode 100644
> > include/soc/fsl/plat_pm.h
> >
> > diff --git a/drivers/soc/fsl/Kconfig b/drivers/soc/fsl/Kconfig index
> > 7a9fb9b..6517412 100644
> > --- a/drivers/soc/fsl/Kconfig
> > +++ b/drivers/soc/fsl/Kconfig
> > @@ -16,3 +16,17 @@ config FSL_GUTS
> >  	  Initially only reading SVR and registering soc device are supported.
> >  	  Other guts accesses, such as reading RCW, should eventually be
> moved
> >  	  into this driver as well.
> > +
> > +config FSL_PLAT_PM
> > +	bool "Freescale platform PM framework"
> > +	help
> > +	  This driver is to provide a independent framework for PM service
> > +	  provider and consumer to configure system level wake up feature.
> For
> > +	  example, RCPM driver could register a callback function on this
> > +	  platform first, and Flex timer driver who want to enable timer wake
> > +	  up feature, will call generic API provided by this platform driver,
> > +	  and then it will trigger RCPM driver to do it. The benefit is to
> > +	  isolate the user and service, such as  flex timer driver will not
> > +	  have to know the implement details of wakeup function it require.
> > +	  Besides, it is also easy for service side to upgrade its logic when
> > +	  design changed and remain user side unchanged.
> > diff --git a/drivers/soc/fsl/Makefile b/drivers/soc/fsl/Makefile index
> > 44b3beb..8f9db23 100644
> > --- a/drivers/soc/fsl/Makefile
> > +++ b/drivers/soc/fsl/Makefile
> > @@ -6,3 +6,4 @@ obj-$(CONFIG_FSL_DPAA)                 += qbman/
> >  obj-$(CONFIG_QUICC_ENGINE)		+= qe/
> >  obj-$(CONFIG_CPM)			+= qe/
> >  obj-$(CONFIG_FSL_GUTS)			+= guts.o
> > +obj-$(CONFIG_FSL_PLAT_PM)	+= plat_pm.o
> > diff --git a/drivers/soc/fsl/plat_pm.c b/drivers/soc/fsl/plat_pm.c new
> > file mode 100644 index 0000000..19ea14e
> > --- /dev/null
> > +++ b/drivers/soc/fsl/plat_pm.c
> > @@ -0,0 +1,144 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// plat_pm.c - Freescale platform PM framework // // Copyright 2018
> > +NXP // // Author: Ran Wang <ran.wang_1@xxxxxxx>,
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/device.h>
> > +#include <linux/list.h>
> > +#include <linux/slab.h>
> > +#include <linux/err.h>
> > +#include <soc/fsl/plat_pm.h>
> > +
> > +
> > +struct plat_pm_t {
> > +	struct list_head node;
> > +	fsl_plat_pm_handle handle;
> > +	void *handle_priv;
> > +	spinlock_t	lock;
> > +};
> > +
> > +static struct plat_pm_t plat_pm;
> > +
> > +// register_fsl_platform_wakeup_source - Register callback function
> > +to plat_pm // @handle: Pointer to handle PM feature requirement //
> > +@handle_priv: Handler specific data struct // // Return 0 on success
> > +other negative errno int
> > +register_fsl_platform_wakeup_source(fsl_plat_pm_handle handle,
> > +		void *handle_priv)
> > +{
> > +	struct plat_pm_t *p;
> > +	unsigned long	flags;
> > +
> > +	if (!handle) {
> > +		pr_err("FSL plat_pm: Handler invalid, reject\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	p = kmalloc(sizeof(*p), GFP_KERNEL);
> > +	if (!p)
> > +		return -ENOMEM;
> > +
> > +	p->handle = handle;
> > +	p->handle_priv = handle_priv;
> > +
> > +	spin_lock_irqsave(&plat_pm.lock, flags);
> > +	list_add_tail(&p->node, &plat_pm.node);
> > +	spin_unlock_irqrestore(&plat_pm.lock, flags);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(register_fsl_platform_wakeup_source);
> > +
> > +// Deregister_fsl_platform_wakeup_source - deregister callback
> > +function // @handle_priv: Handler specific data struct // // Return 0
> > +on success other negative errno int
> > +deregister_fsl_platform_wakeup_source(void *handle_priv) {
> > +	struct plat_pm_t *p, *tmp;
> > +	unsigned long	flags;
> > +
> > +	spin_lock_irqsave(&plat_pm.lock, flags);
> > +	list_for_each_entry_safe(p, tmp, &plat_pm.node, node) {
> > +		if (p->handle_priv == handle_priv) {
> > +			list_del(&p->node);
> > +			kfree(p);
> > +		}
> > +	}
> > +	spin_unlock_irqrestore(&plat_pm.lock, flags);
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(deregister_fsl_platform_wakeup_source);
> > +
> > +// fsl_platform_wakeup_config - Configure wakeup source by calling
> > +handlers // @dev: pointer to user's device struct // @flag: to tell
> > +enable or disable wakeup source // // Return 0 on success other
> > +negative errno int fsl_platform_wakeup_config(struct device *dev,
> > +bool flag) {
> > +	struct plat_pm_t *p;
> > +	int ret;
> > +	bool success_handled;
> > +	unsigned long	flags;
> > +
> > +	success_handled = false;
> > +
> > +	// Will consider success if at least one callback return 0.
> > +	// Also, rest handles still get oppertunity to be executed
> > +	spin_lock_irqsave(&plat_pm.lock, flags);
> > +	list_for_each_entry(p, &plat_pm.node, node) {
> > +		if (p->handle) {
> > +			ret = p->handle(dev, flag, p->handle_priv);
> > +			if (!ret)
> > +				success_handled = true;
> Miss a break?

Actually my idea is to allow more than one registered handler to handle this
request, so I define a flag rather than return to indicated if there is at least one handler successfully
do it. This design might give more flexibility to framework when running.

> > +			else if (ret != -ENODEV) {
> > +				pr_err("FSL plat_pm: Failed to config wakeup
> source:%d\n", ret);
> Please unlock before return.

Yes, will fix it in next version, thanks for pointing out!

> > +				return ret;
> > +			}
> > +		} else
> > +			pr_warn("FSL plat_pm: Invalid handler detected,
> skip\n");
> > +	}
> > +	spin_unlock_irqrestore(&plat_pm.lock, flags);
> > +
> > +	if (success_handled == false) {
> > +		pr_err("FSL plat_pm: Cannot find the matchhed handler for
> wakeup source config\n");
> > +		return -ENODEV;
> > +	}
> Add this into the loop.

My design is that if the 1st handler return -ENODEV to indicated this device it doesn't support, 
then the framework will continue try 2nd handler...

So I think it is needed to place this checking out of loop, what do you say?

Regards,
Ran
> > +
> > +	return 0;
> > +}
> > +
> > +// fsl_platform_wakeup_enable - Enable wakeup source // @dev: pointer
> > +to user's device struct // // Return 0 on success other negative
> > +errno int fsl_platform_wakeup_enable(struct device *dev) {
> > +	return fsl_platform_wakeup_config(dev, true); }
> > +EXPORT_SYMBOL_GPL(fsl_platform_wakeup_enable);
> > +
> > +// fsl_platform_wakeup_disable - Disable wakeup source // @dev:
> > +pointer to user's device struct // // Return 0 on success other
> > +negative errno int fsl_platform_wakeup_disable(struct device *dev) {
> > +	return fsl_platform_wakeup_config(dev, false); }
> > +EXPORT_SYMBOL_GPL(fsl_platform_wakeup_disable);
> > +
> > +static int __init fsl_plat_pm_init(void) {
> > +	spin_lock_init(&plat_pm.lock);
> > +	INIT_LIST_HEAD(&plat_pm.node);
> > +	return 0;
> > +}
> > +
> > +core_initcall(fsl_plat_pm_init);
> > diff --git a/include/soc/fsl/plat_pm.h b/include/soc/fsl/plat_pm.h new
> > file mode 100644 index 0000000..bbe151e
> > --- /dev/null
> > +++ b/include/soc/fsl/plat_pm.h
> > @@ -0,0 +1,22 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// plat_pm.h - Freescale platform PM Header // // Copyright 2018 NXP
> > +// // Author: Ran Wang <ran.wang_1@xxxxxxx>,
> > +
> > +#ifndef __FSL_PLAT_PM_H
> > +#define __FSL_PLAT_PM_H
> > +
> > +typedef int (*fsl_plat_pm_handle)(struct device *dev, bool flag,
> > +		void *handle_priv);
> > +
> > +int register_fsl_platform_wakeup_source(fsl_plat_pm_handle handle,
> > +		void *handle_priv);
> > +int deregister_fsl_platform_wakeup_source(void *handle_priv); int
> > +fsl_platform_wakeup_config(struct device *dev, bool flag); int
> > +fsl_platform_wakeup_enable(struct device *dev); int
> > +fsl_platform_wakeup_disable(struct device *dev);
> > +
> > +#endif	// __FSL_PLAT_PM_H
>