implement a generic PWM framework

I'm completely against this effort.

The objections to my submission weren't ever because I was changing the user-visible API, so I don't think you can claim any advantage to mine in that regard.

I will take some blame for not getting my API finished, but I have been fighting some serious non-work issues that have consumed nearly all of my available time--- including the professional time I would otherwise have to invest in getting my code finished.

Is there the possibility that we could cooperate to get my patches finished, rather than discarding and reinventing them completely?

A lot of people want to see a framework get merged, and I think it's great that Sascha has volunteered to do the work to push that through this time, especially since you have not been able to finish your work.

What I think would be the best plan forward is to merge Sascha's patches as soon as we can, then get all currently existing pwm drivers converted to that and moved to drivers/pwm, and finally do the interface changes that you have proposed earlier.

I would also hope that you can give constructive feedback to the submission and point out potential problems that you see where the code should be changed now in order to make your interface changes more easy later.

I realize that it's annoying to spend a lot of time on a specific implementation and then see competing code get merged.

Unfortunately,
this happens all the time, and the code we merge is often not the one that has had the most effort spent on it, but the one that looks most promising at the time when it gets merged.

Guys:

Quoted 4 lines · Arnd Bergmann

A lot of people want to see a framework get merged, and I think it's great that Sascha has volunteered to do the work to push that through this time, especially since you have not been able to finish your work.

Sascha is wasting his time by reinventing the wheel. He's traveling over exactly the same path I have already covered. In fact, some of his reviewer comments are almost word-for-word the same as those I have received and addressed in the past.

My patches were always kept current in this mailing list and others, and Sascha clearly has the skills necessary to make improvements and corrections should he have chosen to do so.

So please don't complain too much: We are currently only doing the work you didn't want to do.

Bill,

Quoted 13 lines · Bill Gatliff

Guys:

Quoted 4 lines · Arnd Bergmann

A lot of people want to see a framework get merged, and I think it's great that Sascha has volunteered to do the work to push that through this time, especially since you have not been able to finish your work.

Sascha is wasting his time by reinventing the wheel. He's traveling over exactly the same path I have already covered. In fact, some of his reviewer comments are almost word-for-word the same as those I have received and addressed in the past.

Quoted 3 lines

My patches were always kept current in this mailing list and others, and Sascha clearly has the skills necessary to make improvements and corrections should he have chosen to do so.

I think that you made the fundamental mistake to completly ignore the existing pwm API and its users. With a competing api we are basically stuck. We can't convert the existing hardware drivers to the new API because leds-pwm.c, pwm_bl.c and others still depend on the old API and boards using it would break.

We can't convert the function drivers
either because again this would break boards for which only an old style pwm driver exists. So the logical thing to do is to put a step in
between: Consolidate the existing drivers and *then* change the API atomically so that nothing breaks. Your patches don't do this, so I don't think at all that what I did is duplication of work.

Given the current rush to move drivers out of arch/ it probably won't take long until all pwm drivers are moved to drivers/pwm/ and converted to use the framework, and then you have a good base to put your work onto. So please don't complain too much: We are currently only doing the work you didn't want to do.

Quoted 5 lines · Sascha Hauer

I think that you made the fundamental mistake to completly ignore the existing pwm API and its users. With a competing api we are basically stuck. We can't convert the existing hardware drivers to the new API because leds-pwm.c, pwm_bl.c and others still depend on the old API and boards using it would break.

I don't think this is really a blocker to Bill's patches. There are three (that I can see) pwm users currently:
drivers/video/backlight/pwm_bl.c
drivers/leds/leds-pwm.c
drivers/input/misc/pwm-beeper.c

All of those drivers are trivial and good easily be updated to work with Bill's patches. Bill already provided a leds-pwm driver.

There is also the interesting case of the Atmel pwm driver, which does not fit the current pwm api and has its own backlight and leds drivers. Bill's patches addressed this, Sasha's patches do not. If we merge Sasha's patches then we are going to be in the same position as Bill's patches at some point in that we need to change the pwm api (and all its users) to meet the needs of the Atmel (and any similar hardware) pwm device.

The ep93xx pwm driver (drivers/misc/ep93xx-pwm.c) also does not fit the current api (though it could), but instead provides a sysfs interface to user-space. Again, this was addressed by Bill's patches.

Quoted 6 lines

We can't convert the function drivers
either because again this would break boards for which only an old style pwm driver exists. So the logical thing to do is to put a step in
between: Consolidate the existing drivers and *then* change the API atomically so that nothing breaks. Your patches don't do this, so I don't think at all that what I did is duplication of work.

You have to modify the drivers anyway match the new pwm core. The Atmel and ep93xx drivers are going to be difficult to merge into the new api, and seeing as there are only about seven pwm drivers total in the kernel I think its a significant portion. Any pwm api patchset could easily convert all of the existing pwm drivers without becoming overly massive.

If we merge Sasha's api, then we can move most of the existing drivers and maybe add some new ones, but we will still have the unconsolidated outliers. When (if?) we try to fix those we will probably need to change the pwm api and therefore all of the drivers to. So its basically a case of do the effort now (Bill's patches) or do it later. Doing it later will probably require more effort.

Quoted 6 lines

Given the current rush to move drivers out of arch/ it probably won't take long until all pwm drivers are moved to drivers/pwm/ and converted to use the framework, and then you have a good base to put your work onto. So please don't complain too much: We are currently only doing the work you didn't want to do.

You can move all of the drivers out of arch now if you want. You just need to make sure you are only compiling one of them in. The real job in consolidating means making sure that the api meets the needs of all of the drivers. The in kernel Atmel pwm driver at least is not going to convert easily to this api.

Quoted 13 lines · Ryan Mallon

Quoted 5 lines · Sascha Hauer

I think that you made the fundamental mistake to completly ignore the existing pwm API and its users. With a competing api we are basically stuck. We can't convert the existing hardware drivers to the new API because leds-pwm.c, pwm_bl.c and others still depend on the old API and boards using it would break.

I don't think this is really a blocker to Bill's patches. There are three (that I can see) pwm users currently:
drivers/video/backlight/pwm_bl.c
drivers/leds/leds-pwm.c
drivers/input/misc/pwm-beeper.c

Quoted 2 lines

All of those drivers are trivial and good easily be updated to work with Bill's patches. Bill already provided a leds-pwm driver.

I agree with Ryan. ?The three drivers listed above appear to be the only pwm "user" drivers. ?They are all trivial and could be updated easily.

The "core" drivers using the existing API appear to be:
arch/blackfin/kernel/pwm.c

Quoted 16 lines · Sascha Hauer

Bill,

Quoted 13 lines · Bill Gatliff

Guys:

Quoted 4 lines · Arnd Bergmann

A lot of people want to see a framework get merged, and I think it's great that Sascha has volunteered to do the work to push that through this time, especially since you have not been able to finish your work.

Sascha is wasting his time by reinventing the wheel. He's traveling over exactly the same path I have already covered. In fact, some of his reviewer comments are almost word-for-word the same as those I have received and addressed in the past.

Quoted 8 lines

Quoted 3 lines

My patches were always kept current in this mailing list and others, and Sascha clearly has the skills necessary to make improvements and corrections should he have chosen to do so.

I think that you made the fundamental mistake to completly ignore the existing pwm API and its users. With a competing api we are basically stuck. We can't convert the existing hardware drivers to the new API because leds-pwm.c, pwm_bl.c and others still depend on the old API and boards using it would break.

I don't think this is really a blocker to Bill's patches. There are three (that I can see) pwm users currently:
drivers/video/backlight/pwm_bl.c
drivers/leds/leds-pwm.c
drivers/input/misc/pwm-beeper.c

All of those drivers are trivial and good easily be updated to work with Bill's patches. Bill already provided a leds-pwm driver.

There is also the interesting case of the Atmel pwm driver, which does not fit the current pwm api and has its own backlight and leds drivers. Bill's patches addressed this, Sasha's patches do not. If we merge Sasha's patches then we are going to be in the same position as Bill's patches at some point in that we need to change the pwm api (and all its users) to meet the needs of the Atmel (and any similar hardware) pwm device.

The ep93xx pwm driver (drivers/misc/ep93xx-pwm.c) also does not fit the current api (though it could), but instead provides a sysfs interface to user-space. Again, this was addressed by Bill's patches.

Quoted 6 lines

We can't convert the function drivers
either because again this would break boards for which only an old style pwm driver exists. So the logical thing to do is to put a step in
between: Consolidate the existing drivers and *then* change the API atomically so that nothing breaks. Your patches don't do this, so I don't think at all that what I did is duplication of work.

You have to modify the drivers anyway match the new pwm core.

The
Atmel and ep93xx drivers are going to be difficult to merge into the new api, and seeing as there are only about seven pwm drivers total in the kernel I think its a significant portion. Any pwm api patchset could easily convert all of the existing pwm drivers without becoming overly massive.

If we merge Sasha's api, then we can move most of the existing drivers and maybe add some new ones, but we will still have the unconsolidated outliers. When (if?) we try to fix those we will probably need to change the pwm api and therefore all of the drivers to. So its basically a case of do the effort now (Bill's patches) or do it later. Doing it later will probably require more effort.

Quoted 6 lines

Given the current rush to move drivers out of arch/ it probably won't take long until all pwm drivers are moved to drivers/pwm/ and converted to use the framework, and then you have a good base to put your work onto. So please don't complain too much: We are currently only doing the work you didn't want to do.

You can move all of the drivers out of arch now if you want. You just need to make sure you are only compiling one of them in.

The
real job in consolidating means making sure that the api meets the needs of all of the drivers. The in kernel Atmel pwm driver at least is not going to convert easily to this api.

Also, please not my change of email address for future emails.

Quoted 25 lines · Ryan Mallon

Quoted 16 lines · Sascha Hauer

Bill,

Quoted 13 lines · Bill Gatliff

Guys:

Quoted 4 lines · Arnd Bergmann

A lot of people want to see a framework get merged, and I think it's great that Sascha has volunteered to do the work to push that through this time, especially since you have not been able to finish your work.

Sascha is wasting his time by reinventing the wheel. He's traveling over exactly the same path I have already covered. In fact, some of his reviewer comments are almost word-for-word the same as those I have received and addressed in the past.

Quoted 8 lines

Quoted 3 lines

My patches were always kept current in this mailing list and others, and Sascha clearly has the skills necessary to make improvements and corrections should he have chosen to do so.

I think that you made the fundamental mistake to completly ignore the existing pwm API and its users. With a competing api we are basically stuck. We can't convert the existing hardware drivers to the new API because leds-pwm.c, pwm_bl.c and others still depend on the old API and boards using it would break.

Quoted 8 lines

I don't think this is really a blocker to Bill's patches. There are three (that I can see) pwm users currently:
drivers/video/backlight/pwm_bl.c
drivers/leds/leds-pwm.c
drivers/input/misc/pwm-beeper.c

All of those drivers are trivial and good easily be updated to work with Bill's patches. Bill already provided a leds-pwm driver.

Yes, it is easy but that's not my point. The point is that you can't convert the drivers without converting *all* hardware drivers in a *single* step. If you choose to have two competing APIs in the tree for one purpose you can't convert the drivers but instead you have to copy them, either with cp or ifdefs. I have just looked at the leds-pwm driver Bill provided. Applying it immediately breaks all users.

Quoted 7 lines

There is also the interesting case of the Atmel pwm driver, which does not fit the current pwm api and has its own backlight and leds drivers. Bill's patches addressed this, Sasha's patches do not. If we merge Sasha's patches then we are going to be in the same position as Bill's patches at some point in that we need to change the pwm api (and all its users) to meet the needs of the Atmel (and any similar hardware) pwm device.

My patches are compatible to the current (user-) API on purpose. It offers the possibility to convert each hardware driver independently of the others. Once we have a framework we can change the driver API independent of the user API.

Quoted 11 lines

The ep93xx pwm driver (drivers/misc/ep93xx-pwm.c) also does not fit the current api (though it could), but instead provides a sysfs interface to user-space. Again, this was addressed by Bill's patches.

Quoted 6 lines

We can't convert the function drivers
either because again this would break boards for which only an old style pwm driver exists. So the logical thing to do is to put a step in
between: Consolidate the existing drivers and *then* change the API atomically so that nothing breaks. Your patches don't do this, so I don't think at all that what I did is duplication of work.

Quoted 1 line

You have to modify the drivers anyway match the new pwm core.

Yes. But changing the user API *and* the driver API in a single patch really is a bad idea.

I don't say at all that the end result Bill is aiming at is bad because it isn't. We are not talking about the end result, but only the way to get there. And getting from a to b in bisectable small steps is a well established development model in the Kernel, or have I missed something?

Quoted 20 lines

The
Atmel and ep93xx drivers are going to be difficult to merge into the new api, and seeing as there are only about seven pwm drivers total in the kernel I think its a significant portion. Any pwm api patchset could easily convert all of the existing pwm drivers without becoming overly massive.

If we merge Sasha's api, then we can move most of the existing drivers and maybe add some new ones, but we will still have the unconsolidated outliers. When (if?) we try to fix those we will probably need to change the pwm api and therefore all of the drivers to. So its basically a case of do the effort now (Bill's patches) or do it later. Doing it later will probably require more effort.

Quoted 6 lines

Given the current rush to move drivers out of arch/ it probably won't take long until all pwm drivers are moved to drivers/pwm/ and converted to use the framework, and then you have a good base to put your work onto. So please don't complain too much: We are currently only doing the work you didn't want to do.

Quoted 2 lines

You can move all of the drivers out of arch now if you want. You just need to make sure you are only compiling one of them in.

Yes, we can. But this does not solve the migration problem I try to describe in this and the previous mail.

Let's face it: Bill is working on his PWM patches for three years now, still the merge of these patches is not in sight. Let's just split them into smaller parts which are easier to swallow.

Quoted 34 lines · Sascha Hauer

Quoted 25 lines · Ryan Mallon

Quoted 16 lines · Sascha Hauer

Bill,

Quoted 13 lines · Bill Gatliff

Guys:

Quoted 4 lines · Arnd Bergmann

A lot of people want to see a framework get merged, and I think it's great that Sascha has volunteered to do the work to push that through this time, especially since you have not been able to finish your work.

Sascha is wasting his time by reinventing the wheel. He's traveling over exactly the same path I have already covered. In fact, some of his reviewer comments are almost word-for-word the same as those I have received and addressed in the past.

Quoted 8 lines

Quoted 3 lines

My patches were always kept current in this mailing list and others, and Sascha clearly has the skills necessary to make improvements and corrections should he have chosen to do so.

I think that you made the fundamental mistake to completly ignore the existing pwm API and its users. With a competing api we are basically stuck. We can't convert the existing hardware drivers to the new API because leds-pwm.c, pwm_bl.c and others still depend on the old API and boards using it would break.

Quoted 8 lines

I don't think this is really a blocker to Bill's patches. There are three (that I can see) pwm users currently:
drivers/video/backlight/pwm_bl.c
drivers/leds/leds-pwm.c
drivers/input/misc/pwm-beeper.c

All of those drivers are trivial and good easily be updated to work with Bill's patches. Bill already provided a leds-pwm driver.

Quoted 8 lines

Yes, it is easy but that's not my point. The point is that you can't convert the drivers without converting *all* hardware drivers in a *single* step. If you choose to have two competing APIs in the tree for one purpose you can't convert the drivers but instead you have to copy them, either with cp or ifdefs. I have just looked at the leds-pwm driver Bill provided. Applying it immediately breaks all users.

At _any_ point that you change the pwm api you will need to change all of the drivers. How do you plan to adapt the api in the future to support the oddball pwm drivers without having to change all of the drivers in one go?

The number of pwm drivers and users is so small that it would be nice to see a patch series that introduces the new framework/api and converts all of the drivers over. That way we don't get left in an intermediate state with some drivers using the new framework and some using the old one.

Quoted 8 lines

Quoted 8 lines

There is also the interesting case of the Atmel pwm driver, which does not fit the current pwm api and has its own backlight and leds drivers. Bill's patches addressed this, Sasha's patches do not. If we merge Sasha's patches then we are going to be in the same position as Bill's patches at some point in that we need to change the pwm api (and all its users) to meet the needs of the Atmel (and any similar hardware) pwm device.

Quoted 4 lines

My patches are compatible to the current (user-) API on purpose. It offers the possibility to convert each hardware driver independently of the others. Once we have a framework we can change the driver API independent of the user API.

We should just convert all of the drivers now rather than waiting for the various maintainers to do it. Because your proposed api is backwards compatible and most SoC platforms don't have additional external pwms there is little motivation for them to do this work.

Quoted 12 lines

Quoted 12 lines

The ep93xx pwm driver (drivers/misc/ep93xx-pwm.c) also does not fit the current api (though it could), but instead provides a sysfs interface to user-space. Again, this was addressed by Bill's patches.

Quoted 6 lines

We can't convert the function drivers
either because again this would break boards for which only an old style pwm driver exists. So the logical thing to do is to put a step in
between: Consolidate the existing drivers and *then* change the API atomically so that nothing breaks. Your patches don't do this, so I don't think at all that what I did is duplication of work.

Quoted 3 lines

Quoted 1 line

You have to modify the drivers anyway match the new pwm core.

Yes. But changing the user API *and* the driver API in a single patch really is a bad idea.

It doesn't have to be in a single patch, but it should be in a single patch series.

Quoted 4 lines

I don't say at all that the end result Bill is aiming at is bad because it isn't. We are not talking about the end result, but only the way to get there. And getting from a to b in bisectable small steps is a well established development model in the Kernel, or have I missed something?

The bi-sectable steps is fine. But what we have is a patch series which introduces a new framework with a vague promise that the existing drivers will get converted and the api will be improved later. Again, the number of in-kernel users is so small that we may as well do it all in one series.

Quoted 26 lines

Quoted 15 lines

The
Atmel and ep93xx drivers are going to be difficult to merge into the new api, and seeing as there are only about seven pwm drivers total in the kernel I think its a significant portion. Any pwm api patchset could easily convert all of the existing pwm drivers without becoming overly massive.

If we merge Sasha's api, then we can move most of the existing drivers and maybe add some new ones, but we will still have the unconsolidated outliers. When (if?) we try to fix those we will probably need to change the pwm api and therefore all of the drivers to. So its basically a case of do the effort now (Bill's patches) or do it later. Doing it later will probably require more effort.

Quoted 8 lines

Quoted 6 lines

Given the current rush to move drivers out of arch/ it probably won't take long until all pwm drivers are moved to drivers/pwm/ and converted to use the framework, and then you have a good base to put your work onto. So please don't complain too much: We are currently only doing the work you didn't want to do.

You can move all of the drivers out of arch now if you want. You just need to make sure you are only compiling one of them in.

Yes, we can. But this does not solve the migration problem I try to describe in this and the previous mail.

Quoted 4 lines

Let's face it: Bill is working on his PWM patches for three years now, still the merge of these patches is not in sight. Let's just split them into smaller parts which are easier to swallow.

Sure, but your series still leaves a lot of work to be done. How many years will the ep93xx and atmel pwm drivers exist in isolation from the rest of the pwm drivers?

Quoted 43 lines · Ryan Mallon

Quoted 42 lines · Sascha Hauer

Quoted 25 lines · Ryan Mallon

Quoted 16 lines · Sascha Hauer

Bill,

Quoted 13 lines · Bill Gatliff

Guys:

Quoted 4 lines · Arnd Bergmann

A lot of people want to see a framework get merged, and I think it's great that Sascha has volunteered to do the work to push that through this time, especially since you have not been able to finish your work.

Sascha is wasting his time by reinventing the wheel. He's traveling over exactly the same path I have already covered. In fact, some of his reviewer comments are almost word-for-word the same as those I have received and addressed in the past.

Quoted 8 lines

Quoted 3 lines

My patches were always kept current in this mailing list and others, and Sascha clearly has the skills necessary to make improvements and corrections should he have chosen to do so.

I think that you made the fundamental mistake to completly ignore the existing pwm API and its users. With a competing api we are basically stuck. We can't convert the existing hardware drivers to the new API because leds-pwm.c, pwm_bl.c and others still depend on the old API and boards using it would break.

Quoted 8 lines

I don't think this is really a blocker to Bill's patches. There are three (that I can see) pwm users currently:
drivers/video/backlight/pwm_bl.c
drivers/leds/leds-pwm.c
drivers/input/misc/pwm-beeper.c

All of those drivers are trivial and good easily be updated to work with Bill's patches. Bill already provided a leds-pwm driver.

Yes, it is easy but that's not my point. The point is that you can't convert the drivers without converting *all* hardware drivers in a *single* step. If you choose to have two competing APIs in the tree for one purpose you can't convert the drivers but instead you have to copy them, either with cp or ifdefs. I have just looked at the leds-pwm driver Bill provided. Applying it immediately breaks all users.

Quoted 4 lines

At _any_ point that you change the pwm api you will need to change all of the drivers. How do you plan to adapt the api in the future to support the oddball pwm drivers without having to change all of the drivers in one go?

It is a framework between hardware drivers and user drivers and as such it is able to abstract between the API to the hardware drivers and the API to the user drivers. You can change one of those without touching the other.

Quoted 9 lines

Quoted 4 lines

I don't say at all that the end result Bill is aiming at is bad because it isn't. We are not talking about the end result, but only the way to get there. And getting from a to b in bisectable small steps is a well established development model in the Kernel, or have I missed something?

The bi-sectable steps is fine. But what we have is a patch series which introduces a new framework with a vague promise that the existing drivers will get converted and the api will be improved later. Again, the number of in-kernel users is so small that we may as well do it all in one series.

I don't understand you. On one hand you argue that it's trivial enough to port all drivers over the a new API in a single series and on the other hand you argue that we have to wait for several subsystem maintainers to port the drivers over to an existing-but-now-in-a-framework API.

Quoted 8 lines · Sascha Hauer

Quoted 4 lines

At _any_ point that you change the pwm api you will need to change all of the drivers. How do you plan to adapt the api in the future to support the oddball pwm drivers without having to change all of the drivers in one go?

It is a framework between hardware drivers and user drivers and as such it is able to abstract between the API to the hardware drivers and the API to the user drivers. You can change one of those without touching the other.

You are missing the point. For example, the current in-kernel api to configure a pwm is this:

int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns);

Bill's proposed change to the api, which is more flexible, looks like this:

int pwm_config(struct pwm_channel *p,
struct pwm_channel_config *c)

If you want to change the api in this way at any time, you will need to change all of the pwm drivers and all of the users (pwm-bl, pwm-leds, etc) at the same time, or introduce some sort of wrapper layer to ease the conversion. Either way, if you do it now or do it after your patch set it is still the same amount of work.

We do want a better api for pwm drivers, if only to support oddball hardware like the atmel. The process I see is this if we go with Bill's patches:
- Introduce new api with common framework

- Move drivers to new api

With your patches we get this:
- Introduce common framework

- Move drivers to common framework

- Modify api
- Rewrite drivers for new api

The problem we have seen with these processes in the past is they get left half way with some drivers moving to the new framework and some trailing behind and some having their own hand-rolled approach because the common api doesn't meet their needs.

I am tired of discussing this. It seems we can't agree and unless someone else jumps in here we will probably have to wait for another year until something moves in the PWM area.

I am tired of discussing this. It seems we can't agree and unless someone else jumps in here we will probably have to wait for another year until something moves in the PWM area.

On 04/07/11 17:55, Sascha Hauer wrote:

If we are going to introduce a new framework for pwms then we should create one which meets the needs of at least all of the in kernel drivers. This patch series provides no solution for either the atmel or ep93xx drivers, so it is not a complete solution. At some point the api/framework _must_ be changed. If we can introduce transition layers then we should do that now so they we can provide a common framework along with some forward thinking about how the other drivers are going

The pwm framework needs to incorporate at least the following:
- sysfs access (ep93xx driver)
- Multiple channels per device (atmel driver)

The other nice things to have for the pwm framework are:
- More fine grained control of pwms: pwm_period_ns, period_duty_ns, etc
- Polarity control
- Synchronisation support for multi-channel devices
- Interrupt handler support
- Sleeping vs non-sleeping configuration api

The fine-grained control api could be added now. pwm_config could be left as is for the time being (the new api could be a wrapper around it to start with). Polarity control and interrupt handling apis could also be defined without affecting the drivers which don't need to implement

them. Multiple channels and the sleeping/non-sleeping api are the more difficult ones, but at least having some sort of indication about how these plan to be solved would be useful.

~Ryan

Quoted 4 lines

The pwm framework needs to incorporate at least the following:
- sysfs access (ep93xx driver)
- Multiple channels per device (atmel driver)

These are 2 very hardware dependant additions. Is this really the job for a framework to incorporate this?
IMHO, the job of a framework is to allow such things. Creating a framework that does all special things of all vendors makes such thing complicated.

With socketCAN, we encountered a similar problem. Every chip maker tries to create added value by means of special options. You can't support them all in the framework. Therefore, sysfs can be added to configure special things.

Quoted 4 lines · Kurt Van Dijck

With socketCAN, we encountered a similar problem. Every chip maker tries to create added value by means of special options. You can't support them all in the framework. Therefore, sysfs can be added to configure special things.

I would expect that pwm is much simpler than CAN,

so the amount
of creativity there is also limited.

Arnd

Quoted 3 lines · Sascha Hauer

I am tired of discussing this. It seems we can't agree and unless someone else jumps in here we will probably have to wait for another year until something moves in the PWM area.

If we are going to introduce a new framework for pwms then we should create one which meets the needs of at least all of the in kernel drivers. This patch series provides no solution for either the atmel or ep93xx drivers, so it is not a complete solution. At some point the api/framework _must_ be changed. If we can introduce transition layers then we should do that now so they we can provide a common framework along with some forward thinking about how the other drivers are going to be migrated to the new framework. This patch series doesn't even migrate _any_ of the existing drivers.

It doesn't have to be an all or nothing approach. Possibly Bill's series is perhaps too involved by changing the api, introducing sysfs support and reworking the pwm users. But your series is at the opposite end of the spectrum. It does too little. It will take a few release cycles to get all of the existing drivers migrated and since we can't change the api until that happens the atmel and ep93xx drivers will take longer still. At the very least your series should migrate some of the drivers.

The timeline argument is a bit poor. Yes, there has been discussion for a lengthy time about how the pwm api should be developed, but I think that is because it is non-trivial to come up with a framework which is good enough to support all of the pwm hardware (some of which is already in the mainline). Getting something merged now just because it can be done quickly is not a good idea if it all has to get reworked in the future so that it can support all the hardware.

The pwm framework needs to incorporate at least the following:
- sysfs access (ep93xx driver)

- Multiple channels per device (atmel driver)

The mxs driver you introduce looks like it could be implemented as a single device (continuous mmio space) with multiple channels rather than the pwm core/driver approach you have. I also can't see anything in this patch set which hooks up the mxs pwms to an actual board (i.e. nothing calls mxs_add_mxs_pwm)?

The other nice things to have for the pwm framework are:
- More fine grained control of pwms: pwm_period_ns, period_duty_ns, etc
- Polarity control
- Synchronisation support for multi-channel devices
- Interrupt handler support
- Sleeping vs non-sleeping configuration api

The fine-grained control api could be added now. pwm_config could be left as is for the time being (the new api could be a wrapper around it to start with). Polarity control and interrupt handling apis could also be defined without affecting the drivers which don't need to implement them. Multiple channels and the sleeping/non-sleeping api are the more difficult ones, but at least having some sort of indication about how these plan to be solved would be useful.

Quoted 7 lines · Ryan Mallon

The fine-grained control api could be added now. pwm_config could be left as is for the time being (the new api could be a wrapper around it to start with). Polarity control and interrupt handling apis could also be defined without affecting the drivers which don't need to implement them. Multiple channels and the sleeping/non-sleeping api are the more difficult ones, but at least having some sort of indication about how these plan to be solved would be useful.

Again, why should we add these *now*? It only raises the chance that there's more discussion.

Quoted 10 lines · Sascha Hauer

Quoted 7 lines · Ryan Mallon

The fine-grained control api could be added now. pwm_config could be left as is for the time being (the new api could be a wrapper around it to start with). Polarity control and interrupt handling apis could also be defined without affecting the drivers which don't need to implement them. Multiple channels and the sleeping/non-sleeping api are the more difficult ones, but at least having some sort of indication about how these plan to be solved would be useful.

Quoted 2 lines

Again, why should we add these *now*? It only raises the chance that there's more discussion.

My impression is that there are a lot of things that could easily be done to improve the state of PWM drivers, but I don't care about the order in which they are done. My main issue is the lack of a subsystem core driver, which both you and Bill have patches for. It's clear that other people have other issues and want to see their problems addressed first.

I also think that the pwm code is simple enough that we don't have to worry too much about the order that things are done in. Any patch that is making the code better should just get in and not have to wait for something else to be completed first.

What we do need now is a maintainer that can coordinate the patches and merge the ones that have been agreed on. Or multiple maintainers.

Quoted 21 lines · Arnd Bergmann

Quoted 10 lines · Sascha Hauer

Quoted 7 lines · Ryan Mallon

The fine-grained control api could be added now. pwm_config could be left as is for the time being (the new api could be a wrapper around it to start with). Polarity control and interrupt handling apis could also be defined without affecting the drivers which don't need to implement them. Multiple channels and the sleeping/non-sleeping api are the more difficult ones, but at least having some sort of indication about how these plan to be solved would be useful.

Quoted 2 lines

Again, why should we add these *now*? It only raises the chance that there's more discussion.

My impression is that there are a lot of things that could easily be done to improve the state of PWM drivers, but I don't care about the order in which they are done. My main issue is the lack of a subsystem core driver, which both you and Bill have patches for. It's clear that other people have other issues and want to see their problems addressed first.

Quoted 7 lines

I also think that the pwm code is simple enough that we don't have to worry too much about the order that things are done in. Any patch that is making the code better should just get in and not have to wait for something else to be completed first.

What we do need now is a maintainer that can coordinate the patches and merge the ones that have been agreed on. Or multiple maintainers.

Hi,

I'm looking at adding DT support for pwm-backlight, and I think a central PWM API will be required first. Looking through the archives this seems to be the last activity in that direction. Perhaps we can get the efforts restarted?

I pretty much agree with Arnd and Sascha here in that we should try to get a basic framework added. Everything else can be added on top later.

I have a branch which adds a basic PWM framework. It doesn't change the pwm API at all, but only adds support for registering multiple PWM drivers. All in Kernel drivers are converted to this new framework, but except the i.MX driver all of them are untested. I'm quite confident that the drivers itself work, but there will probably be some Kconfig related issues which I wasn't able to sort out. The corresponding Maintainers should probably have a look over it. Feel free to post the patches if you are prepared to work on the comments you receive. Otherwise I'll see if I find some time maybe next week.

Quoted 9 lines · Sascha Hauer

I have a branch which adds a basic PWM framework. It doesn't change the pwm API at all, but only adds support for registering multiple PWM drivers. All in Kernel drivers are converted to this new framework, but except the i.MX driver all of them are untested. I'm quite confident that the drivers itself work, but there will probably be some Kconfig related issues which I wasn't able to sort out. The corresponding Maintainers should probably have a look over it. Feel free to post the patches if you are prepared to work on the comments you receive. Otherwise I'll see if I find some time maybe next week.

[...]

I've looked at this in more detail and one thing that irritates me is that the current driver API requires a driver to register a pwm_chip structure for each PWM it can control. Wouldn't it be easier in such cases to just specify how many PWMs a chip wants to register, much like gpiolib does it?

Quoted 26 lines · Ryan Mallon

Quoted 16 lines · Sascha Hauer

Bill,

Quoted 13 lines · Bill Gatliff

Guys:

Quoted 4 lines · Arnd Bergmann

A lot of people want to see a framework get merged, and I think it's great that Sascha has volunteered to do the work to push that through this time, especially since you have not been able to finish your work.

Sascha is wasting his time by reinventing the wheel. He's traveling over exactly the same path I have already covered. In fact, some of his reviewer comments are almost word-for-word the same as those I have received and addressed in the past.

Quoted 8 lines

Quoted 3 lines

My patches were always kept current in this mailing list and others, and Sascha clearly has the skills necessary to make improvements and corrections should he have chosen to do so.

I think that you made the fundamental mistake to completly ignore the existing pwm API and its users. With a competing api we are basically stuck. We can't convert the existing hardware drivers to the new API because leds-pwm.c, pwm_bl.c and others still depend on the old API and boards using it would break.

Quoted 8 lines

I don't think this is really a blocker to Bill's patches. There are three (that I can see) pwm users currently:
drivers/video/backlight/pwm_bl.c
drivers/leds/leds-pwm.c
drivers/input/misc/pwm-beeper.c

All of those drivers are trivial and good easily be updated to work with Bill's patches. Bill already provided a leds-pwm driver.

Yes, it is easy but that's not my point. The point is that you can't convert the drivers without converting *all* hardware drivers in a *single* step. If you choose to have two competing APIs in the tree for one purpose you can't convert the drivers but instead you have to copy them, either with cp or ifdefs. I have just looked at the leds-pwm driver Bill provided. Applying it immediately breaks all users.

The PWMs on i.MX23/28 have almost seperated register spaces but share a common enable register. To reflect this register a parent device to the PWMs which handles the enable register.

Signed-off-by Sascha Hauer <[email protected]>

Signed-off-by Sascha Hauer <[email protected]>

drivers/pwm/Kconfig +0 -0

index 93c1052..5694574 100644

Quoted 1 line · Sascha Hauer

Signed-off-by Sascha Hauer <[email protected]>

Hi Sascha,

The probing looks good to me now. I would have added extra code to avoid ioremapping the same page multiple times, but I see this as a matter of different preferences, so I'm fine with that.

You are still missing a description of the driver, both in the changelog above and in the Kconfig patch. A small sentence to spell out PWM in the changelog and to list the devices that will use this driver would be helpful to the causal reader.

Provided that you add that:

Reviewed-by Arnd Bergmann <[email protected]>

Quoted 12 lines

drivers/pwm/Kconfig +1 -1

index 93c1052..5694574 100644
@@ -2,11 +2,15 @@ menuconfig PWM
 	bool "PWM Support"
 	help
 	  This enables PWM support through the generic PWM framework.
-	  You only need to enable this, if you also want to enable
+	  You only need to enable this if you also want to enable
 	  one or more of the PWM drivers below.
 

drivers/pwm/Kconfig menuconfig PWM · line 9

Quoted 2 lines

drivers/pwm/Kconfig +0 -0

 	  If unsure, say N.
 

I think this hunk should have been in the other first patch.