RE: [PATCH 1/7] pwm: Add pwm core driver

RE: [PATCH 1/7] pwm: Add pwm core driver

[Hemanth V]

>> >> On 28 of September 2010 10:40:42 Arun Murthy wrote:
>> >> > The existing pwm based led and backlight driver makes use of the
>> >> > pwm(include/linux/pwm.h). So all the board specific pwm drivers
>> will
>> >> > be exposing the same set of function name as in
>> include/linux/pwm.h.
>> >> > As a result build fails.
>> >>
>> >> Which build fails? One with multi-SoC support? Please be more
>> specific.
>> > Sure will add this in v2.
>> >
>>
>> Could you clarify for the benefit of all, which specific issues you are
>> trying to address with this patch series
> 1. Now since all the pwm driver export same set of function(pwm_enable, pwm_disable,..), if it happens that there are two pwm driver enabled this
> leads to re-declaration and results in build failure. The proper way to handle this would be to have a pwm core function, and let all the pwm
> drivers register to the pwm core driver.
> 2. The above scenario also occurs in place of multi-soc environment. Lets say OMAP has a pwm module and that is being used for primary lcd backlight
> and twl has a backlight that is being used for controlling the charging led brightness. In this case there exists 2 pwm drivers and one pwm driver
> will be used by pwm_bl.c and other by leds-pwm.c

Speaking specifically of OMAP4, twl6030 supports multiple PWMs i.e for display/keypad backlight, charging
led. But there should not be need for multiple drivers since twl6030-pwm should be able to support
all these (currently it doesnot though). So there would single pwm_enable, pwm_disable exported and driver
internally takes care configuring the correct PWM based on id. Would it not be similar
situation for other hardware also.

Thanks
Hemanth

RE: [PATCH 1/7] pwm: Add pwm core driver

[Arun MURTHY]

> >> >> On 28 of September 2010 10:40:42 Arun Murthy wrote:
> >> >> > The existing pwm based led and backlight driver makes use of
> the
> >> >> > pwm(include/linux/pwm.h). So all the board specific pwm drivers
> >> will
> >> >> > be exposing the same set of function name as in
> >> include/linux/pwm.h.
> >> >> > As a result build fails.
> >> >>
> >> >> Which build fails? One with multi-SoC support? Please be more
> >> specific.
> >> > Sure will add this in v2.
> >> >
> >>
> >> Could you clarify for the benefit of all, which specific issues you
> are
> >> trying to address with this patch series
> > 1. Now since all the pwm driver export same set of
> function(pwm_enable, pwm_disable,..), if it happens that there are two
> pwm driver enabled this
> > leads to re-declaration and results in build failure. The proper way
> to handle this would be to have a pwm core function, and let all the
> pwm
> > drivers register to the pwm core driver.
> > 2. The above scenario also occurs in place of multi-soc environment.
> Lets say OMAP has a pwm module and that is being used for primary lcd
> backlight
> > and twl has a backlight that is being used for controlling the
> charging led brightness. In this case there exists 2 pwm drivers and
> one pwm driver
> > will be used by pwm_bl.c and other by leds-pwm.c
> 
> Speaking specifically of OMAP4, twl6030 supports multiple PWMs i.e for
> display/keypad backlight, charging
> led. But there should not be need for multiple drivers since twl6030-
> pwm should be able to support
> all these (currently it doesnot though). So there would single
> pwm_enable, pwm_disable exported and driver
> internally takes care configuring the correct PWM based on id. Would it
> not be similar
> situation for other hardware also.
> 
You are right, there is only one pwm module in twl4030/twl6030 and this module might have any number or pwm's line PWM1, PWM2, PWM3 etc.
My consideration is when you have two separate pwm modules on different soc. Its not in case of OMAP boards. But that was just an example that I gave.

Let me be more specific, consider an environment where there is an APE and Power Management subsystem(separate IC but on same board/platform)
APE has a pwm module and Power Management SubSystem also has pwm module. Both are part of the platform.
Not there exists two drivers in a single platform and both of them are enabled. Building such a kernel results in re-declaration build error.

Hope I am clear, I am not trying to distinguish the number of pwm in a pwm module, but trying to distinguish the number of pwm modules in an environment/platform.

Thanks and Regards,
Arun R Murthy
------------
 

[PATCH 1/7] pwm: Add pwm core driver

[Arun Murthy]

The existing pwm based led and backlight driver makes use of the
pwm(include/linux/pwm.h). So all the board specific pwm drivers will
be exposing the same set of function name as in include/linux/pwm.h.
As a result build fails in case of multi soc environments where each soc
has a pwm device in it.

In order to overcome this issue all the pwm drivers must register to
some core pwm driver with function pointers for pwm operations (i.e
pwm_config, pwm_enable, pwm_disable).

The clients of pwm device will have to call pwm_request, wherein
they will get the pointer to struct pwm_ops. This structure include
function pointers for pwm_config, pwm_enable and pwm_disable.

Signed-off-by: Arun Murthy <arun.murthy@stericsson.com>
Acked-by: Linus Walleij <linus.walleij@stericsson.com>
---
 drivers/Kconfig        |    2 +
 drivers/Makefile       |    1 +
 drivers/pwm/Kconfig    |   16 ++++++
 drivers/pwm/Makefile   |    1 +
 drivers/pwm/pwm-core.c |  129 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pwm.h    |   12 ++++-
 6 files changed, 160 insertions(+), 1 deletions(-)
 create mode 100644 drivers/pwm/Kconfig
 create mode 100644 drivers/pwm/Makefile
 create mode 100644 drivers/pwm/pwm-core.c

diff --git a/drivers/Kconfig b/drivers/Kconfig
index a2b902f..e042f27 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -111,4 +111,6 @@ source "drivers/xen/Kconfig"
 source "drivers/staging/Kconfig"
 
 source "drivers/platform/Kconfig"
+
+source "drivers/pwm/Kconfig"
 endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index 4ca727d..0061ec4 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -116,3 +116,4 @@ obj-$(CONFIG_STAGING)		+= staging/
 obj-y				+= platform/
 obj-y				+= ieee802154/
 obj-y				+= vbus/
+obj-$(CONFIG_PWM_DEVICES)	+= pwm/
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
new file mode 100644
index 0000000..5d10106
--- /dev/null
+++ b/drivers/pwm/Kconfig
@@ -0,0 +1,16 @@
+#
+# PWM devices
+#
+
+menuconfig PWM_DEVICES
+	bool "PWM devices"
+	default y
+	---help---
+	  Say Y here to get to see options for device drivers from various
+	  different categories. This option alone does not add any kernel code.
+
+	  If you say N, all options in this submenu will be skipped and disabled.
+
+if PWM_DEVICES
+
+endif # PWM_DEVICES
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
new file mode 100644
index 0000000..552f969
--- /dev/null
+++ b/drivers/pwm/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_PWM_DEVICES)	+= pwm-core.o
diff --git a/drivers/pwm/pwm-core.c b/drivers/pwm/pwm-core.c
new file mode 100644
index 0000000..b84027a
--- /dev/null
+++ b/drivers/pwm/pwm-core.c
@@ -0,0 +1,129 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2010
+ *
+ * License Terms: GNU General Public License v2
+ * Author: Arun R Murthy <arun.murthy@stericsson.com>
+ */
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+#include <linux/rwsem.h>
+#include <linux/err.h>
+#include <linux/pwm.h>
+
+struct pwm_device {
+	struct pwm_ops *pops;
+	int pwm_id;
+};
+
+struct pwm_dev_info {
+	struct pwm_device *pwm_dev;
+	struct list_head list;
+};
+static struct pwm_dev_info *di;
+
+DECLARE_RWSEM(pwm_list_lock);
+
+void __deprecated pwm_free(struct pwm_device *pwm)
+{
+}
+
+int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
+{
+	return pwm->pops->pwm_config(pwm, duty_ns, period_ns);
+}
+EXPORT_SYMBOL(pwm_config);
+
+int pwm_enable(struct pwm_device *pwm)
+{
+	return pwm->pops->pwm_enable(pwm);
+}
+EXPORT_SYMBOL(pwm_enable);
+
+void pwm_disable(struct pwm_device *pwm)
+{
+	pwm->pops->pwm_disable(pwm);
+}
+EXPORT_SYMBOL(pwm_disable);
+
+int pwm_device_register(struct pwm_device *pwm_dev)
+{
+	struct pwm_dev_info *pwm;
+
+	down_write(&pwm_list_lock);
+	pwm = kzalloc(sizeof(struct pwm_dev_info), GFP_KERNEL);
+	if (!pwm) {
+		up_write(&pwm_list_lock);
+		return -ENOMEM;
+	}
+	pwm->pwm_dev = pwm_dev;
+	list_add_tail(&pwm->list, &di->list);
+	up_write(&pwm_list_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL(pwm_device_register);
+
+int pwm_device_unregister(struct pwm_device *pwm_dev)
+{
+	struct pwm_dev_info *tmp;
+	struct list_head *pos, *tmp_lst;
+
+	down_write(&pwm_list_lock);
+	list_for_each_safe(pos, tmp_lst, &di->list) {
+		tmp = list_entry(pos, struct pwm_dev_info, list);
+		if (tmp->pwm_dev == pwm_dev) {
+			list_del(pos);
+			kfree(tmp);
+			up_write(&pwm_list_lock);
+			return 0;
+		}
+	}
+	up_write(&pwm_list_lock);
+	return -ENOENT;
+}
+EXPORT_SYMBOL(pwm_device_unregister);
+
+struct pwm_device *pwm_request(int pwm_id, const char *name)
+{
+	struct pwm_dev_info *pwm;
+	struct list_head *pos;
+
+	down_read(&pwm_list_lock);
+	list_for_each(pos, &di->list) {
+		pwm = list_entry(pos, struct pwm_dev_info, list);
+		if ((!strcmp(pwm->pwm_dev->pops->name, name)) &&
+				(pwm->pwm_dev->pwm_id == pwm_id)) {
+			up_read(&pwm_list_lock);
+			return pwm->pwm_dev;
+		}
+	}
+	up_read(&pwm_list_lock);
+	return ERR_PTR(-ENOENT);
+}
+EXPORT_SYMBOL(pwm_request);
+
+static int __init pwm_init(void)
+{
+	struct pwm_dev_info *pwm;
+
+	pwm = kzalloc(sizeof(struct pwm_dev_info), GFP_KERNEL);
+	if (!pwm)
+		return -ENOMEM;
+	INIT_LIST_HEAD(&pwm->list);
+	di = pwm;
+	return 0;
+}
+
+static void __exit pwm_exit(void)
+{
+	kfree(di);
+}
+
+subsys_initcall(pwm_init);
+module_exit(pwm_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Arun R Murthy");
+MODULE_ALIAS("core:pwm");
+MODULE_DESCRIPTION("Core pwm driver");
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 7c77575..6e7da1f 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -3,6 +3,13 @@
 
 struct pwm_device;
 
+struct pwm_ops {
+	int (*pwm_config)(struct pwm_device *pwm, int duty_ns, int period_ns);
+	int (*pwm_enable)(struct pwm_device *pwm);
+	int (*pwm_disable)(struct pwm_device *pwm);
+	char *name;
+};
+
 /*
  * pwm_request - request a PWM device
  */
@@ -11,7 +18,7 @@ struct pwm_device *pwm_request(int pwm_id, const char *label);
 /*
  * pwm_free - free a PWM device
  */
-void pwm_free(struct pwm_device *pwm);
+void __deprecated pwm_free(struct pwm_device *pwm);
 
 /*
  * pwm_config - change a PWM device configuration
@@ -28,4 +35,7 @@ int pwm_enable(struct pwm_device *pwm);
  */
 void pwm_disable(struct pwm_device *pwm);
 
+int pwm_device_register(struct pwm_device *pwm_dev);
+int pwm_device_unregister(struct pwm_device *pwm_dev);
+
 #endif /* __LINUX_PWM_H */
-- 
1.7.2.dirty

RE: [PATCH 1/7] pwm: Add pwm core driver

[Hemanth V]

>> >> >> On 28 of September 2010 10:40:42 Arun Murthy wrote:
>> >> >> > The existing pwm based led and backlight driver makes use of
>> the
>> >> >> > pwm(include/linux/pwm.h). So all the board specific pwm drivers
>> >> will
>> >> >> > be exposing the same set of function name as in
>> >> include/linux/pwm.h.
>> >> >> > As a result build fails.
>> >> >>
>> >> >> Which build fails? One with multi-SoC support? Please be more
>> >> specific.
>> >> > Sure will add this in v2.
>> >> >
>> >>
>> >> Could you clarify for the benefit of all, which specific issues you
>> are
>> >> trying to address with this patch series
>> > 1. Now since all the pwm driver export same set of
>> function(pwm_enable, pwm_disable,..), if it happens that there are two
>> pwm driver enabled this
>> > leads to re-declaration and results in build failure. The proper way
>> to handle this would be to have a pwm core function, and let all the
>> pwm
>> > drivers register to the pwm core driver.
>> > 2. The above scenario also occurs in place of multi-soc environment.
>> Lets say OMAP has a pwm module and that is being used for primary lcd
>> backlight
>> > and twl has a backlight that is being used for controlling the
>> charging led brightness. In this case there exists 2 pwm drivers and
>> one pwm driver
>> > will be used by pwm_bl.c and other by leds-pwm.c
>>
>> Speaking specifically of OMAP4, twl6030 supports multiple PWMs i.e for
>> display/keypad backlight, charging
>> led. But there should not be need for multiple drivers since twl6030-
>> pwm should be able to support
>> all these (currently it doesnot though). So there would single
>> pwm_enable, pwm_disable exported and driver
>> internally takes care configuring the correct PWM based on id. Would it
>> not be similar
>> situation for other hardware also.
>>
> You are right, there is only one pwm module in twl4030/twl6030 and this module might have any number or pwm's line PWM1, PWM2, PWM3 etc.
> My consideration is when you have two separate pwm modules on different soc. Its not in case of OMAP boards. But that was just an example that I
> gave.
>
> Let me be more specific, consider an environment where there is an APE and Power Management subsystem(separate IC but on same board/platform)
> APE has a pwm module and Power Management SubSystem also has pwm module. Both are part of the platform.
> Not there exists two drivers in a single platform and both of them are enabled. Building such a kernel results in re-declaration build error.
>

Is this something specific to ST chipsets or do you know of any other chips which might use this feature.

Thanks
Hemanth

RE: [PATCH 1/7] pwm: Add pwm core driver

[Arun MURTHY]

> > Let me be more specific, consider an environment where there is an
> APE and Power Management subsystem(separate IC but on same
> board/platform)
> > APE has a pwm module and Power Management SubSystem also has pwm
> module. Both are part of the platform.
> > Not there exists two drivers in a single platform and both of them
> are enabled. Building such a kernel results in re-declaration build
> error.
> >
> 
> Is this something specific to ST chipsets or do you know of any other
> chips which might use this feature.
> 
It's not like specific to ST chipset. There are multi Soc systems.
You can take ST chipset as an example.

And also let continue our discussion on the new thread that I have mailed so that the mailing list also gets notified.
Sorry for inconvenience.

Thanks and Regards,
Arun R Murthy
-------------

Re: [PATCH 1/7] pwm: Add pwm core driver

[Hemanth V]

----- Original Message ----- 
From: "Arun Murthy" <arun.murthy@stericsson.com>


> The existing pwm based led and backlight driver makes use of the
> pwm(include/linux/pwm.h). So all the board specific pwm drivers will
> be exposing the same set of function name as in include/linux/pwm.h.
> As a result build fails in case of multi soc environments where each soc
> has a pwm device in it.

This seems very specific to ST environment,  looking at the driver list from
( [PATCH 4/7] pwm: Align existing pwm drivers with pwm-core ) it seems
most multi SOC environments might support PWM in either one of the SOC.

arch/arm/plat-mxc/pwm.c
arch/arm/plat-pxa/pwm.c
arch/arm/plat-samsung/pwm.c
arch/mips/jz4740/pwm.c
drivers/mfd/twl6030-pwm.c

Unless people have examples of other SOCs which might use this,
the better approach might be to go for a custom driver rather than changing
the framework.

Thanks
Hemanth 

Re: [PATCH 1/7] pwm: Add pwm core driver

[Samuel Ortiz]

On Tue, Sep 28, 2010 at 06:23:24PM +0530, Hemanth V wrote:
> ----- Original Message ----- From: "Arun Murthy"
> <arun.murthy@stericsson.com>
> 
> 
> >The existing pwm based led and backlight driver makes use of the
> >pwm(include/linux/pwm.h). So all the board specific pwm drivers will
> >be exposing the same set of function name as in include/linux/pwm.h.
> >As a result build fails in case of multi soc environments where each soc
> >has a pwm device in it.
> 
> This seems very specific to ST environment,  
No it's not. It's an issue Arun has hit while enabling one of the ST MFD chip,
but he's tackling a generic issue.

> looking at the driver list from
> ( [PATCH 4/7] pwm: Align existing pwm drivers with pwm-core ) it seems
> most multi SOC environments might support PWM in either one of the SOC.
> 
> arch/arm/plat-mxc/pwm.c
> arch/arm/plat-pxa/pwm.c
> arch/arm/plat-samsung/pwm.c
> arch/mips/jz4740/pwm.c
> drivers/mfd/twl6030-pwm.c
> 
> Unless people have examples of other SOCs which might use this,
> the better approach might be to go for a custom driver rather than changing
> the framework.
I wouldn't call the current pwm code a framework. It's a bunch of header
definitions that happens to work in the specific case of 1 pwm per
sub architecture.
What Arun is proposing is an actual framework. And it seems to be clean and
simple enough.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [PATCH 1/7] pwm: Add pwm core driver

[Felipe Balbi]

On Tue, Sep 28, 2010 at 08:06:11AM -0500, Samuel Ortiz wrote:
>On Tue, Sep 28, 2010 at 06:23:24PM +0530, Hemanth V wrote:
>> ----- Original Message ----- From: "Arun Murthy"
>> <arun.murthy@stericsson.com>
>>
>>
>> >The existing pwm based led and backlight driver makes use of the
>> >pwm(include/linux/pwm.h). So all the board specific pwm drivers will
>> >be exposing the same set of function name as in include/linux/pwm.h.
>> >As a result build fails in case of multi soc environments where each soc
>> >has a pwm device in it.
>>
>> This seems very specific to ST environment,
>No it's not. It's an issue Arun has hit while enabling one of the ST MFD chip,
>but he's tackling a generic issue.
>
>> looking at the driver list from
>> ( [PATCH 4/7] pwm: Align existing pwm drivers with pwm-core ) it seems
>> most multi SOC environments might support PWM in either one of the SOC.
>>
>> arch/arm/plat-mxc/pwm.c
>> arch/arm/plat-pxa/pwm.c
>> arch/arm/plat-samsung/pwm.c
>> arch/mips/jz4740/pwm.c
>> drivers/mfd/twl6030-pwm.c
>>
>> Unless people have examples of other SOCs which might use this,
>> the better approach might be to go for a custom driver rather than changing
>> the framework.
>I wouldn't call the current pwm code a framework. It's a bunch of header
>definitions that happens to work in the specific case of 1 pwm per
>sub architecture.
>What Arun is proposing is an actual framework. And it seems to be clean and
>simple enough.

FWIW, I agree with you Sam. Sooner or later, this will hit other SoCs.

-- 
balbi

Re: [PATCH 1/7] pwm: Add pwm core driver

[Lars-Peter Clausen]

Arun MURTHY wrote:
>>>> Shouldn't PWM_DEVICES select HAVE_PWM?
>>>
>>> No not required, the entire concept is to remove HAVE_PWM and use
>> PWM_CORE.
>>
>> Well in patch 4 you say that PWM_CORE is currently limited to ARM.
>> Furthermore you
>> change the pwm-backlight and pwm-led Kconfig entries to depend on
>> HAVE_PWM ||
>> PWM_CORE. Adding a select HAVE_PWM here would make those changes
>> unnecessary.
> HAVE_PWM is retained just because the mips pwm driver is not aligned with the pwm core driver.
> On mips pwm driver aligning to the pwm core driver HAVE_PWM will be replaced by PWM_CORE.
> 
>> HAVE_PWM should be set, when pwm_* functions are available. When your
>> pwm-core driver
>> is selected they are available.
> On applying this patch set pwm_* function will be exported in pwm_core driver and in mips pwm driver.
> Since mips pwm driver is not aligned with the pwm core, HAVE_PWM is retained and removed in places where pwm drivers register to pwm core driver.

pwm_{enable,disable,request,free} are the interface of the pwm api. Your pwm-core is
one implementation of that interface. A somewhat special though, because it tries to
be a generic implementation.
There are still other implementations though. For example right now the mips jz4740 one.
In my opinion HAVE_PWM should be defined if there is a implementation for the pwm
interface is available.
I know that your plan is that in the end pwm-core is the only implementation of the
pwm interface.

But right now it is not and on the other hand some SoC implementors might choose that
they want to provide their own minimal pwm interface implementation.
Furthermore this would allow you to start with pwm-core for one SoC which you have on
your desk and where you can properly test things and keep the patches clean from
clutter changing all the different archs.
Once pwm-core is in a proper shape you or other people can start porting all the
different SoC support code to pwm-core.

Similar behavior is for example true for the gpio api. There is gpiolib which is the
generic implementation which allows having gpio chips outside of the chip. On the
other hand there are still archs which choose to have their own gpio api implementation.

> 
>>> pwm_device will be passed to each and every pwm driver that are
>> registered as client with pwm core.
>>> The list consists of the registered pwm drivers and is to be handled
>> by pwm core.
>>> Why should each and every pwm driver get to know about the entire pwm
>> driver list?
>> Declare the list field to be private, by saying that it should only be
>> touched by the
>> core. Right now you allocate a rather small additional structure for
>> each registered
>> device. This could be easily be avoided by embedding the list field
>> into the
>> pwm_device struct.
> 
> The one that is being allocated in register is the pwm_device and this has to. Because each pwm driver will have their own data related to ops, pwm_id.
> Also note that there exists an element "data" that points to the pwm device specific information. Hence this allocation is required.
> 
>>>>> +	}
>>>>> +	pwm->pwm_dev = pwm_dev;
>>>>> +	list_add_tail(&pwm->list, &di->list);
>>>>> +	up_write(&pwm_list_lock);
>>>>> +
>>>> I guess you only need to lock the list when accessing the list and
>>>> adding the new
>>>> pwm_dev.
>>> Oops, thanks for pointing out, will implement this in the v2 patch.
> Coming back to this, I guess the locking has to be done while traversing the list also, as my present pointer in the list my get over written by the time I add an element to list. Please let me know if I am wrong.
> 
>>>>> +struct pwm_ops {
>>>>> +	int (*pwm_config)(struct pwm_device *pwm, int duty_ns, int
>>>> period_ns);
>>>>> +	int (*pwm_enable)(struct pwm_device *pwm);
>>>>> +	int (*pwm_disable)(struct pwm_device *pwm);
>>>>> +	char *name;
>>>>> +};
>>>>> +
>>>> Shouldn't name be part of the pwm_device? That would allow the ops
>> to
>>>> be shared
>>>> between different devices.
>>> Good catch, the reason being that 2 or more devices can share the
>> same ops and get registered to pwm core.
>>> But the catch lies while identifying the pwm device while the clients
>> are requesting for.
>>> The pwm backlight will request the pwm driver by name. This is
>> parameter that distinguishes among different pwm devices irrespective
>> of same ops or not.
>> Yes. And thats why it should go into the pwm_device struct itself.
>>
>> If an additional ops struct is allocated for each device anyway we
>> would be better of
>> embedding it directly into the device struct instead of just holding a
>> pointer to it.
> Yes ops structure will be allocated. But how can we get access to the ops structure of another driver?
> And moreover two pwm driver sharing same ops ideally means a single pwm module. If not everything atleast the pwm registers of two different modules changes. So this scenario can never occur.
> 
>>>>>  #endif /* __LINUX_PWM_H */
>>>> It might be also a good idea to add a device class for pwm devices.
>>> Sure, but can you please explain with an example the use case.
>>>
>> Well, for one it helps to keep data structured.
>> And there would be functions to traverse all devices of a class, so you
>> could get rid
>> of your "di" list.
> Sorry, I didn't get you can you please elaborate more?
> 
Sure. You would create a "struct class" device class for pwm devices. For each
registered pwm device there would then be a "struct device" representing the pwm
device whithin the linux device tree.
This has serveral advantages:
For one you can use this for keeping track of the all the pwm devices instead of
having your custom list. You can use class_for_each_device and class_find_device
instead of traversing your list. This would make the core much simpler and more readable.
Also you can use the device structure for refcounting of modules and devices. Right
now if a pwm-core driver, like the twl6040, is build as a module you can remove the
module while another driver, for example pwm-backlight, is using a pwm device from
the pwm-core driver. Then upon accessing the pwm device from the pwm-backlight driver
the code would crash, because it would access already freed memory.

- Lars


> Thanks and Regards,
> Arun R Murthy
> -------------

RE: [PATCH 1/7] pwm: Add pwm core driver

[Arun MURTHY]

> Arun MURTHY wrote:
> >>>> Shouldn't PWM_DEVICES select HAVE_PWM?
> >>>
> >>> No not required, the entire concept is to remove HAVE_PWM and use
> >> PWM_CORE.
> >>
> >> Well in patch 4 you say that PWM_CORE is currently limited to ARM.
> >> Furthermore you
> >> change the pwm-backlight and pwm-led Kconfig entries to depend on
> >> HAVE_PWM ||
> >> PWM_CORE. Adding a select HAVE_PWM here would make those changes
> >> unnecessary.
> > HAVE_PWM is retained just because the mips pwm driver is not aligned
> with the pwm core driver.
> > On mips pwm driver aligning to the pwm core driver HAVE_PWM will be
> replaced by PWM_CORE.
> >
> >> HAVE_PWM should be set, when pwm_* functions are available. When
> your
> >> pwm-core driver
> >> is selected they are available.
> > On applying this patch set pwm_* function will be exported in
> pwm_core driver and in mips pwm driver.
> > Since mips pwm driver is not aligned with the pwm core, HAVE_PWM is
> retained and removed in places where pwm drivers register to pwm core
> driver.
> 
> pwm_{enable,disable,request,free} are the interface of the pwm api.
> Your pwm-core is
> one implementation of that interface. A somewhat special though,
> because it tries to
> be a generic implementation.
> There are still other implementations though. For example right now the
> mips jz4740 one.
> In my opinion HAVE_PWM should be defined if there is a implementation
> for the pwm
> interface is available.
> I know that your plan is that in the end pwm-core is the only
> implementation of the
> pwm interface.
Yes for that reason, I still have retained the HAVE_PWM, once that is
also aligned with pwm core, HAVE_PWM will be totally removed.
I am in the process of doing the same in this series of patch, but was
blocked by mips jz4740 pwm driver.
Will do that very soon in near future.

> 
> But right now it is not and on the other hand some SoC implementors
> might choose that
> they want to provide their own minimal pwm interface implementation.
> Furthermore this would allow you to start with pwm-core for one SoC
> which you have on
> your desk and where you can properly test things and keep the patches
> clean from
> clutter changing all the different archs.
> Once pwm-core is in a proper shape you or other people can start
> porting all the
> different SoC support code to pwm-core.
That's the exact reason for me to come up with this core driver, which has
the minimal part and the one that are common in all pwm drivers.
Not only common most if not all pwm drivers just make use of only these
functions.
Moreover I have aligned all existing pwm driver in my patch 4/7 :-)

> 
> Similar behavior is for example true for the gpio api. There is gpiolib
> which is the
> generic implementation which allows having gpio chips outside of the
> chip. On the
> other hand there are still archs which choose to have their own gpio
> api implementation.
> 
> >
> >>> pwm_device will be passed to each and every pwm driver that are
> >> registered as client with pwm core.
> >>> The list consists of the registered pwm drivers and is to be
> handled
> >> by pwm core.
> >>> Why should each and every pwm driver get to know about the entire
> pwm
> >> driver list?
> >> Declare the list field to be private, by saying that it should only
> be
> >> touched by the
> >> core. Right now you allocate a rather small additional structure for
> >> each registered
> >> device. This could be easily be avoided by embedding the list field
> >> into the
> >> pwm_device struct.
> >
> > The one that is being allocated in register is the pwm_device and
> this has to. Because each pwm driver will have their own data related
> to ops, pwm_id.
> > Also note that there exists an element "data" that points to the pwm
> device specific information. Hence this allocation is required.
> >
> >>>>> +	}
> >>>>> +	pwm->pwm_dev = pwm_dev;
> >>>>> +	list_add_tail(&pwm->list, &di->list);
> >>>>> +	up_write(&pwm_list_lock);
> >>>>> +
> >>>> I guess you only need to lock the list when accessing the list and
> >>>> adding the new
> >>>> pwm_dev.
> >>> Oops, thanks for pointing out, will implement this in the v2 patch.
> > Coming back to this, I guess the locking has to be done while
> traversing the list also, as my present pointer in the list my get over
> written by the time I add an element to list. Please let me know if I
> am wrong.
> >
> >>>>> +struct pwm_ops {
> >>>>> +	int (*pwm_config)(struct pwm_device *pwm, int duty_ns, int
> >>>> period_ns);
> >>>>> +	int (*pwm_enable)(struct pwm_device *pwm);
> >>>>> +	int (*pwm_disable)(struct pwm_device *pwm);
> >>>>> +	char *name;
> >>>>> +};
> >>>>> +
> >>>> Shouldn't name be part of the pwm_device? That would allow the ops
> >> to
> >>>> be shared
> >>>> between different devices.
> >>> Good catch, the reason being that 2 or more devices can share the
> >> same ops and get registered to pwm core.
> >>> But the catch lies while identifying the pwm device while the
> clients
> >> are requesting for.
> >>> The pwm backlight will request the pwm driver by name. This is
> >> parameter that distinguishes among different pwm devices
> irrespective
> >> of same ops or not.
> >> Yes. And thats why it should go into the pwm_device struct itself.
> >>
> >> If an additional ops struct is allocated for each device anyway we
> >> would be better of
> >> embedding it directly into the device struct instead of just holding
> a
> >> pointer to it.
> > Yes ops structure will be allocated. But how can we get access to the
> ops structure of another driver?
> > And moreover two pwm driver sharing same ops ideally means a single
> pwm module. If not everything atleast the pwm registers of two
> different modules changes. So this scenario can never occur.
> >
> >>>>>  #endif /* __LINUX_PWM_H */
> >>>> It might be also a good idea to add a device class for pwm
> devices.
> >>> Sure, but can you please explain with an example the use case.
> >>>
> >> Well, for one it helps to keep data structured.
> >> And there would be functions to traverse all devices of a class, so
> you
> >> could get rid
> >> of your "di" list.
> > Sorry, I didn't get you can you please elaborate more?
> >
> Sure. You would create a "struct class" device class for pwm devices.
> For each
> registered pwm device there would then be a "struct device"
> representing the pwm
> device whithin the linux device tree.
> This has serveral advantages:
> For one you can use this for keeping track of the all the pwm devices
> instead of
> having your custom list. You can use class_for_each_device and
> class_find_device
> instead of traversing your list. This would make the core much simpler
> and more readable.
> Also you can use the device structure for refcounting of modules and
> devices. Right
> now if a pwm-core driver, like the twl6040, is build as a module you
> can remove the
> module while another driver, for example pwm-backlight, is using a pwm
> device from
> the pwm-core driver. Then upon accessing the pwm device from the pwm-
> backlight driver
> the code would crash, because it would access already freed memory.
> 
Can this be taken after this patch is merged?

Thanks and Regards,
Arun R Murthy
------------

Re: [PATCH 1/7] pwm: Add pwm core driver

[Trilok Soni]

Hi Arun,

Adding Bill Gatliff (anyway, CC list already crowded)

On Wed, Sep 29, 2010 at 10:19 AM, Arun MURTHY
<arun.murthy@stericsson.com> wrote:
>> Arun MURTHY wrote:
>> >>>> Shouldn't PWM_DEVICES select HAVE_PWM?
>> >>>
>> >>> No not required, the entire concept is to remove HAVE_PWM and use
>> >> PWM_CORE.

There is already nice and clean framework written by Bill for PWM, if
you grep the LKML and linux-embedded mailing list archive then you
will get his patches, and it seems that he had promised to send the
updated version few week back, but not heard from him (may be because
he was travelling as per FB status).

Please evaluate that framework too.

-- 
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni

RE: [PATCH 1/7] pwm: Add pwm core driver

[Arun MURTHY]

Hi Trilok,

> Hi Arun,
> 
> Adding Bill Gatliff (anyway, CC list already crowded)
> 
> On Wed, Sep 29, 2010 at 10:19 AM, Arun MURTHY
> <arun.murthy@stericsson.com> wrote:
> >> Arun MURTHY wrote:
> >> >>>> Shouldn't PWM_DEVICES select HAVE_PWM?
> >> >>>
> >> >>> No not required, the entire concept is to remove HAVE_PWM and
> use
> >> >> PWM_CORE.
> 
> There is already nice and clean framework written by Bill for PWM, if
> you grep the LKML and linux-embedded mailing list archive then you
> will get his patches, and it seems that he had promised to send the
> updated version few week back, but not heard from him (may be because
> he was travelling as per FB status).
> 
> Please evaluate that framework too.
> 
Thanks for this information, I did search in linux-embedded mailing list
archive. Below are my views on that patch set.
Many of the functions that has been defined in pwm core driver
written by Bill Gatliff is not being used by the most of the pwm drivers
except Atmel PWM driver. I rather felt the pwm core driver was an attempt
made to generalize the Atmel pwm driver.
And moreover this was posted long back somewhere in the beginning of this
year i.e Feb and the thread is dead thereafter.

This patch has been submitted focusing all the existing pwm drivers and
only these are the functions that are being used by pwm drivers.
This patch set also included patch to align all the existing pwm driver
with the pwm core driver.
So it is an attempt to generalize most of the pwm drivers and
conclude with a pwm core driver.

Thanks and Regards,
Arun R Murthy
-------------

Re: [PATCH 1/7] pwm: Add pwm core driver

[Trilok Soni]

Hi Arun,

On Fri, Oct 1, 2010 at 8:55 AM, Arun MURTHY <arun.murthy@stericsson.com> wrote:
> Hi Trilok,
>
>> Hi Arun,
>>
>> Adding Bill Gatliff (anyway, CC list already crowded)
>>
>> On Wed, Sep 29, 2010 at 10:19 AM, Arun MURTHY
>> <arun.murthy@stericsson.com> wrote:
>> >> Arun MURTHY wrote:
>> >> >>>> Shouldn't PWM_DEVICES select HAVE_PWM?
>> >> >>>
>> >> >>> No not required, the entire concept is to remove HAVE_PWM and
>> use
>> >> >> PWM_CORE.
>>
>> There is already nice and clean framework written by Bill for PWM, if
>> you grep the LKML and linux-embedded mailing list archive then you
>> will get his patches, and it seems that he had promised to send the
>> updated version few week back, but not heard from him (may be because
>> he was travelling as per FB status).
>>
>> Please evaluate that framework too.
>>
> Thanks for this information, I did search in linux-embedded mailing list
> archive. Below are my views on that patch set.
> Many of the functions that has been defined in pwm core driver
> written by Bill Gatliff is not being used by the most of the pwm drivers
> except Atmel PWM driver. I rather felt the pwm core driver was an attempt
> made to generalize the Atmel pwm driver.
> And moreover this was posted long back somewhere in the beginning of this
> year i.e Feb and the thread is dead thereafter.
>
> This patch has been submitted focusing all the existing pwm drivers and
> only these are the functions that are being used by pwm drivers.
> This patch set also included patch to align all the existing pwm driver
> with the pwm core driver.
> So it is an attempt to generalize most of the pwm drivers and
> conclude with a pwm core driver.

I don't agree that Bill had only atmel drivers view. The PWM framework
was discussed in-depth and at that time reviewers also requested once
to provide more example drivers using these drivers, someone said "we
atleast need three drivers as rule of thumb". Let's wait until Bill
reviews your framework, I am sure we don't need to end up the same
problems faced by Bill while designing that framework in your code
too.

-- 
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni

RE: [PATCH 1/7] pwm: Add pwm core driver

[Arun MURTHY]

Hi Trilok,

> Hi Arun,
> 
> On Fri, Oct 1, 2010 at 8:55 AM, Arun MURTHY
> <arun.murthy@stericsson.com> wrote:
> > Hi Trilok,
> >
> >> Hi Arun,
> >>
> >> Adding Bill Gatliff (anyway, CC list already crowded)
> >>
> >> On Wed, Sep 29, 2010 at 10:19 AM, Arun MURTHY
> >> <arun.murthy@stericsson.com> wrote:
> >> >> Arun MURTHY wrote:
> >> >> >>>> Shouldn't PWM_DEVICES select HAVE_PWM?
> >> >> >>>
> >> >> >>> No not required, the entire concept is to remove HAVE_PWM and
> >> use
> >> >> >> PWM_CORE.
> >>
> >> There is already nice and clean framework written by Bill for PWM,
> if
> >> you grep the LKML and linux-embedded mailing list archive then you
> >> will get his patches, and it seems that he had promised to send the
> >> updated version few week back, but not heard from him (may be
> because
> >> he was travelling as per FB status).
> >>
> >> Please evaluate that framework too.
> >>
> > Thanks for this information, I did search in linux-embedded mailing
> list
> > archive. Below are my views on that patch set.
> > Many of the functions that has been defined in pwm core driver
> > written by Bill Gatliff is not being used by the most of the pwm
> drivers
> > except Atmel PWM driver. I rather felt the pwm core driver was an
> attempt
> > made to generalize the Atmel pwm driver.
> > And moreover this was posted long back somewhere in the beginning of
> this
> > year i.e Feb and the thread is dead thereafter.
> >
> > This patch has been submitted focusing all the existing pwm drivers
> and
> > only these are the functions that are being used by pwm drivers.
> > This patch set also included patch to align all the existing pwm
> driver
> > with the pwm core driver.
> > So it is an attempt to generalize most of the pwm drivers and
> > conclude with a pwm core driver.
> 
> I don't agree that Bill had only atmel drivers view. The PWM framework
> was discussed in-depth and at that time reviewers also requested once
> to provide more example drivers using these drivers, someone said "we
> atleast need three drivers as rule of thumb". Let's wait until Bill
> reviews your framework, I am sure we don't need to end up the same
> problems faced by Bill while designing that framework in your code
> too.
> 
You can have a look at the pwm_config_nosleep(),pwm_set_polarity(),
pwm_synchronize(),pwm_unsynchronize(), pwm_set_handler() etc.
These are not being used by the exsting pwm drivers except Atmel pwm.
I mean not the functions but the functionality.
PWM is a simple device and most of its clients are controlling intensity
of backlight, leds, vibrator etc.
I don't think these complex functionality are required.
And moreover it also refers to GPIO pins, in that case it comes under
a different classification. The one that I have suggested is a generic
pwm core driver.
You can have a look at the existing pwm drivers in drivers/mfd/twl6030-pwm.c,
arch/arm/plat-samsung/pwm.c, arch/arm/plat-mxc/pwm.c, arch/arm/plat-pxa/pwm.c,
arch/mips/jz4740/pwm.c.
None of these include the function provided the patch " [PWM PATCH 1/5] API
to consolidate PWM devices behind a common user and kernel interface "
except pwm_enable, pwm_config, pwm_disable.
I have focused on all these and come up with this design.
And moreover Bill's patch set for pwm core driver, becomes incompatible with
pwm based backlight and led driver(drivers/leds/leds-pwm.c,
drivers/video/backlight/pwm_bl.c) and drivers/input/misc/pwm-beeper.c.

I don't mind waiting for Bill's review on my patch, but he is not active
since Feb 2010.

Thanks and Regards,
Arun R Murthy
-------------

Re: [PATCH 1/7] pwm: Add pwm core driver

[Jassi Brar]

On Fri, Oct 1, 2010 at 4:25 PM, Arun MURTHY <arun.murthy@stericsson.com> wrote:
> You can have a look at the pwm_config_nosleep(),pwm_set_polarity(),
> pwm_synchronize(),pwm_unsynchronize(), pwm_set_handler() etc.
> These are not being used by the exsting pwm drivers except Atmel pwm.
How would your 'simple' driver handle Atmel then ?
What if future's SoCs start providing those 'advance' features like Atmel's ?

> I mean not the functions but the functionality.
> PWM is a simple device and most of its clients are controlling intensity
> of backlight, leds, vibrator etc.
> I don't think these complex functionality are required.
oh dear !

RE: [PATCH 1/7] pwm: Add pwm core driver

[Arun MURTHY]

> On Fri, Oct 1, 2010 at 4:25 PM, Arun MURTHY
> <arun.murthy@stericsson.com> wrote:
> > You can have a look at the pwm_config_nosleep(),pwm_set_polarity(),
> > pwm_synchronize(),pwm_unsynchronize(), pwm_set_handler() etc.
> > These are not being used by the exsting pwm drivers except Atmel pwm.
> How would your 'simple' driver handle Atmel then ?
> What if future's SoCs start providing those 'advance' features like
> Atmel's ?
> 
The pwm core driver is the intersection of all pwm drivers and not union
of all pwm driver. I refer this as simple pwm core driver / framework.
Atmel pwm is of a separate classification.
It includes GPIO also. Though, Atmel can use the pwm core driver framework
for functionalities like pwm_enable, pwm_disable, pwm_config, etc and remaining
functionalities specific to Atmel will be handled in Atlmel pwm driver and
will not be exposed to the entire kernel.
Its that the present day pwm device that has been made easy though, by providing
the same functionality.

> > I mean not the functions but the functionality.
> > PWM is a simple device and most of its clients are controlling
> intensity
> > of backlight, leds, vibrator etc.
> > I don't think these complex functionality are required.
> oh dear !
Here I mean why should all those function be exposed to the entire kernel,
as most of the pwm devices do not use them.

Thanks and Regards,
Arun R Murthy
------------

Re: [PATCH 1/7] pwm: Add pwm core driver

[Jassi Brar]

On Fri, Oct 1, 2010 at 5:46 PM, Arun MURTHY <arun.murthy@stericsson.com> wrote:
>> On Fri, Oct 1, 2010 at 4:25 PM, Arun MURTHY
>> <arun.murthy@stericsson.com> wrote:
>> > You can have a look at the pwm_config_nosleep(),pwm_set_polarity(),
>> > pwm_synchronize(),pwm_unsynchronize(), pwm_set_handler() etc.
>> > These are not being used by the exsting pwm drivers except Atmel pwm.
>> How would your 'simple' driver handle Atmel then ?
>> What if future's SoCs start providing those 'advance' features like
>> Atmel's ?
>>
> The pwm core driver is the intersection of all pwm drivers and not union
> of all pwm driver. I refer this as simple pwm core driver / framework.
> Atmel pwm is of a separate classification.
> It includes GPIO also. Though, Atmel can use the pwm core driver framework
> for functionalities like pwm_enable, pwm_disable, pwm_config, etc and remaining
> functionalities specific to Atmel will be handled in Atlmel pwm driver and
> will not be exposed to the entire kernel.
> Its that the present day pwm device that has been made easy though, by providing
> the same functionality.
It's sad that Bill Gatliff didn't/couldn't take his work to conclusive end.
The work was apparently better
http://www.mail-archive.com/linux-embedded@vger.kernel.org/msg02599.html

Best of luck.

Re: [PATCH 1/7] pwm: Add pwm core driver

[Mark Brown]

On Fri, Oct 01, 2010 at 10:46:15AM +0200, Arun MURTHY wrote:
> > On Fri, Oct 1, 2010 at 4:25 PM, Arun MURTHY

> > > I mean not the functions but the functionality.
> > > PWM is a simple device and most of its clients are controlling
> > intensity
> > > of backlight, leds, vibrator etc.
> > > I don't think these complex functionality are required.
> > oh dear !
> Here I mean why should all those function be exposed to the entire kernel,
> as most of the pwm devices do not use them.

While many PWM uses are very simple that doesn't mean that we'll never
need to support more advanced uses.  Normally we try to design APIs so
that they both scale down to the simplest use cases and also up to more
complex ones.

RE: [PATCH 1/7] pwm: Add pwm core driver

[Arun MURTHY]

> On Fri, Oct 01, 2010 at 10:46:15AM +0200, Arun MURTHY wrote:
> > > On Fri, Oct 1, 2010 at 4:25 PM, Arun MURTHY
> 
> > > > I mean not the functions but the functionality.
> > > > PWM is a simple device and most of its clients are controlling
> > > intensity
> > > > of backlight, leds, vibrator etc.
> > > > I don't think these complex functionality are required.
> > > oh dear !
> > Here I mean why should all those function be exposed to the entire
> kernel,
> > as most of the pwm devices do not use them.
> 
> While many PWM uses are very simple that doesn't mean that we'll never
> need to support more advanced uses.  Normally we try to design APIs so
> that they both scale down to the simplest use cases and also up to more
> complex ones.
My intention is just to enable two pwm drivers and build successfully.
This patch can be considered for this reason and taken up until Bill's
patches are merged.

Thanks and Regards,
Arun R Murthy
-------------