diff mbox

[v3,11/18] pwm: Add new pwm-samsung driver

Message ID 2925617.TjWXJcIKRC@flatron (mailing list archive)
State New, archived
Headers show

Commit Message

Tomasz Figa June 22, 2013, 1:06 p.m. UTC
This patch introduces new Samsung PWM driver, which is heavily cleaned,
multiplatform aware and supports DeviceTree based instantiation.

Since on historical hardware PWM block can be shared with clocksource
driver, a shared spinlock is used to protect access to shared registers,
already exported from the clocksource driver.

Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com>
---
 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm-samsung.c | 601 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 602 insertions(+)
 create mode 100644 drivers/pwm/pwm-samsung.c

Changes since v2:
 - Replaced __raw_{readl,writel} with {readl,writel}.
 - Corrected commit message.

Comments

Kim Kukjin June 24, 2013, 3:22 p.m. UTC | #1
On 06/22/13 22:06, Tomasz Figa wrote:
> This patch introduces new Samsung PWM driver, which is heavily cleaned,
> multiplatform aware and supports DeviceTree based instantiation.
>
> Since on historical hardware PWM block can be shared with clocksource
> driver, a shared spinlock is used to protect access to shared registers,
> already exported from the clocksource driver.
>
> Signed-off-by: Tomasz Figa<tomasz.figa@gmail.com>
> ---
>   drivers/pwm/Makefile      |   1 +
>   drivers/pwm/pwm-samsung.c | 601 ++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 602 insertions(+)
>   create mode 100644 drivers/pwm/pwm-samsung.c
>
> Changes since v2:
>   - Replaced __raw_{readl,writel} with {readl,writel}.
>   - Corrected commit message.
>

[...]

> +	of_property_for_each_u32(np, "samsung,pwm-outputs", prop, cur, val) {
> +		if (val>= SAMSUNG_PWM_NUM) {
> +			pr_warning("%s: invalid channel index in samsung,pwm-outputs property\n",

Just note, checkpatch complains following, so fixed to use pr_warn() 
when I applied.

WARNING: Prefer pr_warn(... to pr_warning(...
#471: FILE: drivers/pwm/pwm-samsung.c:432:
+			pr_warning("%s: invalid channel index in samsung,pwm-outputs 
property\n",

[...]

> +
> +static struct dev_pm_ops pwm_samsung_pm_ops = {

WARNING: struct dev_pm_ops should normally be const
#622: FILE: drivers/pwm/pwm-samsung.c:583:
+static struct dev_pm_ops pwm_samsung_pm_ops = {

fixed to add const when I applied.

> +	SET_SYSTEM_SLEEP_PM_OPS(pwm_samsung_suspend, pwm_samsung_resume)
> +};
> +

Thanks,
- Kukjin
Tomasz Figa June 24, 2013, 3:37 p.m. UTC | #2
On Tuesday 25 of June 2013 00:22:42 Kukjin Kim wrote:
> On 06/22/13 22:06, Tomasz Figa wrote:
> > This patch introduces new Samsung PWM driver, which is heavily
> > cleaned,
> > multiplatform aware and supports DeviceTree based instantiation.
> > 
> > Since on historical hardware PWM block can be shared with clocksource
> > driver, a shared spinlock is used to protect access to shared
> > registers, already exported from the clocksource driver.
> > 
> > Signed-off-by: Tomasz Figa<tomasz.figa@gmail.com>
> > ---
> > 
> >   drivers/pwm/Makefile      |   1 +
> >   drivers/pwm/pwm-samsung.c | 601
> >   ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 602
> >   insertions(+)
> >   create mode 100644 drivers/pwm/pwm-samsung.c
> > 
> > Changes since v2:
> >   - Replaced __raw_{readl,writel} with {readl,writel}.
> >   - Corrected commit message.
> 
> [...]
> 
> > +	of_property_for_each_u32(np, "samsung,pwm-outputs", prop, cur, 
val)
> > {
> > +		if (val>= SAMSUNG_PWM_NUM) {
> > +			pr_warning("%s: invalid channel index in 
samsung,pwm-outputs
> > property\n",
> Just note, checkpatch complains following, so fixed to use pr_warn()
> when I applied.
> 
> WARNING: Prefer pr_warn(... to pr_warning(...
> #471: FILE: drivers/pwm/pwm-samsung.c:432:
> +			pr_warning("%s: invalid channel index in 
samsung,pwm-outputs
> property\n",
> 
> [...]
> 
> > +
> > +static struct dev_pm_ops pwm_samsung_pm_ops = {
> 
> WARNING: struct dev_pm_ops should normally be const
> #622: FILE: drivers/pwm/pwm-samsung.c:583:
> +static struct dev_pm_ops pwm_samsung_pm_ops = {
> 
> fixed to add const when I applied.

Oops, I forgot to fix them, sorry. Thank you for fixing and applying.

Best regards,
Tomasz
Thierry Reding June 24, 2013, 5:49 p.m. UTC | #3
On Tue, Jun 25, 2013 at 12:22:42AM +0900, Kukjin Kim wrote:
> On 06/22/13 22:06, Tomasz Figa wrote:
> >This patch introduces new Samsung PWM driver, which is heavily cleaned,
> >multiplatform aware and supports DeviceTree based instantiation.
> >
> >Since on historical hardware PWM block can be shared with clocksource
> >driver, a shared spinlock is used to protect access to shared registers,
> >already exported from the clocksource driver.
> >
> >Signed-off-by: Tomasz Figa<tomasz.figa@gmail.com>
> >---
> >  drivers/pwm/Makefile      |   1 +
> >  drivers/pwm/pwm-samsung.c | 601 ++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 602 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-samsung.c
> >
> >Changes since v2:
> >  - Replaced __raw_{readl,writel} with {readl,writel}.
> >  - Corrected commit message.
> >
> 
> [...]
> 
> >+	of_property_for_each_u32(np, "samsung,pwm-outputs", prop, cur, val) {
> >+		if (val>= SAMSUNG_PWM_NUM) {
> >+			pr_warning("%s: invalid channel index in samsung,pwm-outputs property\n",
> 
> Just note, checkpatch complains following, so fixed to use pr_warn()
> when I applied.

Note that you can't apply patches that touch the PWM tree without my Ack
and I already mentioned that the current way this driver is written
isn't acceptable.

So either you fix it properly, or if everybody except me thinks we don't
need a proper design for drivers anymore, then the only way I'll accept
this driver into the PWM tree is if you put a really big comment at the
top of the file saying that the driver is badly designed on purpose and
that people shouldn't be using it as a reference.

Thierry
Tomasz Figa June 24, 2013, 6:31 p.m. UTC | #4
On Monday 24 of June 2013 19:49:04 Thierry Reding wrote:
> On Tue, Jun 25, 2013 at 12:22:42AM +0900, Kukjin Kim wrote:
> > On 06/22/13 22:06, Tomasz Figa wrote:
> > >This patch introduces new Samsung PWM driver, which is heavily
> > >cleaned,
> > >multiplatform aware and supports DeviceTree based instantiation.
> > >
> > >Since on historical hardware PWM block can be shared with clocksource
> > >driver, a shared spinlock is used to protect access to shared
> > >registers, already exported from the clocksource driver.
> > >
> > >Signed-off-by: Tomasz Figa<tomasz.figa@gmail.com>
> > >---
> > >
> > >  drivers/pwm/Makefile      |   1 +
> > >  drivers/pwm/pwm-samsung.c | 601
> > >  ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed,
> > >  602 insertions(+)
> > >  create mode 100644 drivers/pwm/pwm-samsung.c
> > >
> > >Changes since v2:
> > >  - Replaced __raw_{readl,writel} with {readl,writel}.
> > >  - Corrected commit message.
> > 
> > [...]
> > 
> > >+	of_property_for_each_u32(np, "samsung,pwm-outputs", prop, cur, 
val)
> > >{
> > >+		if (val>= SAMSUNG_PWM_NUM) {
> > >+			pr_warning("%s: invalid channel index in 
samsung,pwm-outputs
> > >property\n",> 
> > Just note, checkpatch complains following, so fixed to use pr_warn()
> > when I applied.
> 
> Note that you can't apply patches that touch the PWM tree without my Ack
> and I already mentioned that the current way this driver is written
> isn't acceptable.
> 
> So either you fix it properly, or if everybody except me thinks we don't
> need a proper design for drivers anymore, then the only way I'll accept
> this driver into the PWM tree is if you put a really big comment at the
> top of the file saying that the driver is badly designed on purpose and
> that people shouldn't be using it as a reference.

Sorry, I don't understand what problem you have with this design. It 
completely meets all the requirements applicable on hardware platforms it 
is (and going to be) used on.

The only thing it would do in a suboptimal way would be synchronization of 
register accesses for multiple instances of the driver - one spinlock 
would be used for all of them. This is insignificant because there is no 
time critical code in this driver and it is really unlikely that a SoC 
with multiple instances of this IP block shows up.

Channel reservation between clocksource and PWM drivers is completely 
correct, relying on the fact that the former can only use channels 
_without_ outputs and the latter can only use channels _with_ outputs. Do 
I have to add that there can't be a channel both with and without output?

So the only place for improvement here, without starting overengineering 
things, is a comment about the purpose of the spinlock and why it can be 
used in our case.

Best regards,
Tomasz
Thierry Reding June 24, 2013, 8:13 p.m. UTC | #5
On Mon, Jun 24, 2013 at 08:31:43PM +0200, Tomasz Figa wrote:
> On Monday 24 of June 2013 19:49:04 Thierry Reding wrote:
> > On Tue, Jun 25, 2013 at 12:22:42AM +0900, Kukjin Kim wrote:
> > > On 06/22/13 22:06, Tomasz Figa wrote:
> > > >This patch introduces new Samsung PWM driver, which is heavily
> > > >cleaned,
> > > >multiplatform aware and supports DeviceTree based instantiation.
> > > >
> > > >Since on historical hardware PWM block can be shared with clocksource
> > > >driver, a shared spinlock is used to protect access to shared
> > > >registers, already exported from the clocksource driver.
> > > >
> > > >Signed-off-by: Tomasz Figa<tomasz.figa@gmail.com>
> > > >---
> > > >
> > > >  drivers/pwm/Makefile      |   1 +
> > > >  drivers/pwm/pwm-samsung.c | 601
> > > >  ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed,
> > > >  602 insertions(+)
> > > >  create mode 100644 drivers/pwm/pwm-samsung.c
> > > >
> > > >Changes since v2:
> > > >  - Replaced __raw_{readl,writel} with {readl,writel}.
> > > >  - Corrected commit message.
> > > 
> > > [...]
> > > 
> > > >+	of_property_for_each_u32(np, "samsung,pwm-outputs", prop, cur, 
> val)
> > > >{
> > > >+		if (val>= SAMSUNG_PWM_NUM) {
> > > >+			pr_warning("%s: invalid channel index in 
> samsung,pwm-outputs
> > > >property\n",> 
> > > Just note, checkpatch complains following, so fixed to use pr_warn()
> > > when I applied.
> > 
> > Note that you can't apply patches that touch the PWM tree without my Ack
> > and I already mentioned that the current way this driver is written
> > isn't acceptable.
> > 
> > So either you fix it properly, or if everybody except me thinks we don't
> > need a proper design for drivers anymore, then the only way I'll accept
> > this driver into the PWM tree is if you put a really big comment at the
> > top of the file saying that the driver is badly designed on purpose and
> > that people shouldn't be using it as a reference.
> 
> Sorry, I don't understand what problem you have with this design. It 
> completely meets all the requirements applicable on hardware platforms it 
> is (and going to be) used on.

My main problem with it is that there is no design. And as such it sets
a bad example. If I accept this into the PWM tree as is then what am I
supposed to tell the next person that comes up with a similarly broken
driver?

> The only thing it would do in a suboptimal way would be synchronization of 
> register accesses for multiple instances of the driver - one spinlock 
> would be used for all of them. This is insignificant because there is no 
> time critical code in this driver and it is really unlikely that a SoC 
> with multiple instances of this IP block shows up.

I've had people give me guarantees that this and that would *never*
happen only to change their minds 6 months down the road. And again,
even if it was actually true in this case, it isn't a valid excuse for
setting a bad example.

> Channel reservation between clocksource and PWM drivers is completely 
> correct, relying on the fact that the former can only use channels 
> _without_ outputs and the latter can only use channels _with_ outputs. Do 
> I have to add that there can't be a channel both with and without output?

The issue is that you can't really ensure that both the clocksource and
PWM drivers use the same variant and therefore might be using different
masks.

> So the only place for improvement here, without starting overengineering 
> things, is a comment about the purpose of the spinlock and why it can be 
> used in our case.

I disagree. You actually even had a version with a halfway decent design
at some point and it was discarded. I don't think I'm asking for all
that much here. I even gave you the option of admitting that the driver
was suboptimal. All I request in return is that you mention that in the
code so that either somebody else might go and clean things up or at
least that nobody will copy from a bad example.

Thierry
Tomasz Figa June 24, 2013, 8:32 p.m. UTC | #6
On Monday 24 of June 2013 22:13:27 Thierry Reding wrote:
> On Mon, Jun 24, 2013 at 08:31:43PM +0200, Tomasz Figa wrote:
> > On Monday 24 of June 2013 19:49:04 Thierry Reding wrote:
> > > On Tue, Jun 25, 2013 at 12:22:42AM +0900, Kukjin Kim wrote:
[snip]
> > > > 
> > > > Just note, checkpatch complains following, so fixed to use
> > > > pr_warn()
> > > > when I applied.
> > > 
> > > Note that you can't apply patches that touch the PWM tree without my
> > > Ack and I already mentioned that the current way this driver is
> > > written isn't acceptable.
> > > 
> > > So either you fix it properly, or if everybody except me thinks we
> > > don't need a proper design for drivers anymore, then the only way
> > > I'll accept this driver into the PWM tree is if you put a really
> > > big comment at the top of the file saying that the driver is badly
> > > designed on purpose and that people shouldn't be using it as a
> > > reference.
> > 
> > Sorry, I don't understand what problem you have with this design. It
> > completely meets all the requirements applicable on hardware platforms
> > it is (and going to be) used on.
> 
> My main problem with it is that there is no design. And as such it sets
> a bad example. If I accept this into the PWM tree as is then what am I
> supposed to tell the next person that comes up with a similarly broken
> driver?

I wouldn't call it no design. It's a simple (trivial) design.

Simple design is often better than a complex one. In this case it indeed 
is, because it doesn't add code that serves no purpose, without any 
functional drawbacks.

> > The only thing it would do in a suboptimal way would be
> > synchronization of register accesses for multiple instances of the
> > driver - one spinlock would be used for all of them. This is
> > insignificant because there is no time critical code in this driver
> > and it is really unlikely that a SoC with multiple instances of this
> > IP block shows up.
> 
> I've had people give me guarantees that this and that would *never*
> happen only to change their minds 6 months down the road. And again,
> even if it was actually true in this case, it isn't a valid excuse for
> setting a bad example.

Well, even if that happens, there is nothing on the way stopping from 
extending this design with something that will work optimally for multiple 
instances. For all currently supported platforms solution used in this 
series is enough.

> > Channel reservation between clocksource and PWM drivers is completely
> > correct, relying on the fact that the former can only use channels
> > _without_ outputs and the latter can only use channels _with_ outputs.
> > Do I have to add that there can't be a channel both with and without
> > output?
> The issue is that you can't really ensure that both the clocksource and
> PWM drivers use the same variant and therefore might be using different
> masks.

Output mask is _not_ a part of the constant variant data. It is stored in 
the same struct, which is to simplify passing SoC-specific parameters from 
platform code to both drivers, but it is not defined as a constant 
anywhere in drivers.

As I already explained in one of my previous replies, it is either parsed 
from DT (samsung,pwm-outputs property) or passed through the variant 
struct as platform_data from board files. It isn't possible for both 
drivers to get different mask values.

> > So the only place for improvement here, without starting
> > overengineering things, is a comment about the purpose of the
> > spinlock and why it can be used in our case.
> 
> I disagree. You actually even had a version with a halfway decent design
> at some point and it was discarded. I don't think I'm asking for all
> that much here. I even gave you the option of admitting that the driver
> was suboptimal. All I request in return is that you mention that in the
> code so that either somebody else might go and clean things up or at
> least that nobody will copy from a bad example.

What about:

/*
 * PWM block is shared between pwm-samsung and samsung_pwm_timer drivers
 * and some registers need access synchronization. If both drivers are
 * compiled in, the spinlock is defined in the clocksource driver,
 * otherwise following definition is used.
 *
 * Currently we do not need any more complex synchronization method 
 * because all the supported SoCs contain only one instance of the PWM 
 * IP. Should this change, both drivers will need to be modified to
 * properly synchronize accesses to particular instances.
 */

Best regards,
Tomasz
Thierry Reding June 24, 2013, 8:53 p.m. UTC | #7
On Mon, Jun 24, 2013 at 10:32:55PM +0200, Tomasz Figa wrote:
> On Monday 24 of June 2013 22:13:27 Thierry Reding wrote:
> > On Mon, Jun 24, 2013 at 08:31:43PM +0200, Tomasz Figa wrote:
[...]
> > > Channel reservation between clocksource and PWM drivers is completely
> > > correct, relying on the fact that the former can only use channels
> > > _without_ outputs and the latter can only use channels _with_ outputs.
> > > Do I have to add that there can't be a channel both with and without
> > > output?
> > The issue is that you can't really ensure that both the clocksource and
> > PWM drivers use the same variant and therefore might be using different
> > masks.
> 
> Output mask is _not_ a part of the constant variant data. It is stored in 
> the same struct, which is to simplify passing SoC-specific parameters from 
> platform code to both drivers, but it is not defined as a constant 
> anywhere in drivers.
> 
> As I already explained in one of my previous replies, it is either parsed 
> from DT (samsung,pwm-outputs property) or passed through the variant 
> struct as platform_data from board files. It isn't possible for both 
> drivers to get different mask values.

But samsung_pwm_set_platdata() and samsung_pwm_clocksource_init() can
still be called with different variants, can't they? Furthermore the
variants aren't read-only and therefore can be modified at any point in
time. So it is indeed possible for them to get different mask values.

Granted, it may be unlikely that they will but it wouldn't be all that
difficult to write the driver in a way to make it really impossible and
at the same time make sure that both driver share the same variant
information.

> > > So the only place for improvement here, without starting
> > > overengineering things, is a comment about the purpose of the
> > > spinlock and why it can be used in our case.
> > 
> > I disagree. You actually even had a version with a halfway decent design
> > at some point and it was discarded. I don't think I'm asking for all
> > that much here. I even gave you the option of admitting that the driver
> > was suboptimal. All I request in return is that you mention that in the
> > code so that either somebody else might go and clean things up or at
> > least that nobody will copy from a bad example.
> 
> What about:
> 
> /*
>  * PWM block is shared between pwm-samsung and samsung_pwm_timer drivers
>  * and some registers need access synchronization. If both drivers are
>  * compiled in, the spinlock is defined in the clocksource driver,
>  * otherwise following definition is used.
>  *
>  * Currently we do not need any more complex synchronization method 
>  * because all the supported SoCs contain only one instance of the PWM 
>  * IP. Should this change, both drivers will need to be modified to
>  * properly synchronize accesses to particular instances.
>  */

I see that you can't be persuaded. And everybody else seems to be okay
with it so... have it your way. I'm probably going to regret this.

Thierry
Tomasz Figa June 24, 2013, 9:17 p.m. UTC | #8
On Monday 24 of June 2013 22:53:42 Thierry Reding wrote:
> On Mon, Jun 24, 2013 at 10:32:55PM +0200, Tomasz Figa wrote:
> > On Monday 24 of June 2013 22:13:27 Thierry Reding wrote:
> > > On Mon, Jun 24, 2013 at 08:31:43PM +0200, Tomasz Figa wrote:
> [...]
> 
> > > > Channel reservation between clocksource and PWM drivers is
> > > > completely
> > > > correct, relying on the fact that the former can only use channels
> > > > _without_ outputs and the latter can only use channels _with_
> > > > outputs.
> > > > Do I have to add that there can't be a channel both with and
> > > > without
> > > > output?
> > > 
> > > The issue is that you can't really ensure that both the clocksource
> > > and
> > > PWM drivers use the same variant and therefore might be using
> > > different
> > > masks.
> > 
> > Output mask is _not_ a part of the constant variant data. It is stored
> > in the same struct, which is to simplify passing SoC-specific
> > parameters from platform code to both drivers, but it is not defined
> > as a constant anywhere in drivers.
> > 
> > As I already explained in one of my previous replies, it is either
> > parsed from DT (samsung,pwm-outputs property) or passed through the
> > variant struct as platform_data from board files. It isn't possible
> > for both drivers to get different mask values.
> 
> But samsung_pwm_set_platdata() and samsung_pwm_clocksource_init() can
> still be called with different variants, can't they?

Sure, they can be, but the question is who would even want to call them 
this way? We are on kernel level, we have assumptions here and we don't 
break those assumptions.

> Furthermore the
> variants aren't read-only and therefore can be modified at any point in
> time. So it is indeed possible for them to get different mask values.

Again, who would be willing to do something like this? We are not in 
userspace.

> Granted, it may be unlikely that they will but it wouldn't be all that
> difficult to write the driver in a way to make it really impossible and
> at the same time make sure that both driver share the same variant
> information.

Sure, it wouldn't. If I'm not mistaken, I already acomplished this in one 
or two of the versions I posted long time ago, but was it really worth all 
the added drawbacks?

Keep in mind that originally, when I started my work on this, I thought 
exactly the same as you - let's design something scalable and sane. 
However, sometimes it's necessary to ask the question if that's really 
what is needed.

> > > > So the only place for improvement here, without starting
> > > > overengineering things, is a comment about the purpose of the
> > > > spinlock and why it can be used in our case.
> > > 
> > > I disagree. You actually even had a version with a halfway decent
> > > design at some point and it was discarded. I don't think I'm asking
> > > for all that much here. I even gave you the option of admitting
> > > that the driver was suboptimal. All I request in return is that you
> > > mention that in the code so that either somebody else might go and
> > > clean things up or at least that nobody will copy from a bad
> > > example.
> > 
> > What about:
> > 
> > /*
> > 
> >  * PWM block is shared between pwm-samsung and samsung_pwm_timer
> >  drivers
> >  * and some registers need access synchronization. If both drivers are
> >  * compiled in, the spinlock is defined in the clocksource driver,
> >  * otherwise following definition is used.
> >  *
> >  * Currently we do not need any more complex synchronization method
> >  * because all the supported SoCs contain only one instance of the PWM
> >  * IP. Should this change, both drivers will need to be modified to
> >  * properly synchronize accesses to particular instances.
> >  */
> 
> I see that you can't be persuaded. And everybody else seems to be okay
> with it so... have it your way. I'm probably going to regret this.

This was just a proposal. Do you want anything else to be added to the 
comment or anything to be changed?

Sure, an alternative would be getting rid of the hacky shared spinlock, 
but do we really need that? Do we really want to have something in the 
kernel that we don't need? I'm just trying to avoid this.

Best regards,
Tomasz
Thierry Reding June 25, 2013, 10:26 a.m. UTC | #9
On Mon, Jun 24, 2013 at 11:17:03PM +0200, Tomasz Figa wrote:
> On Monday 24 of June 2013 22:53:42 Thierry Reding wrote:
> > On Mon, Jun 24, 2013 at 10:32:55PM +0200, Tomasz Figa wrote:
[...]
> > > What about:
> > > 
> > > /*
> > > 
> > >  * PWM block is shared between pwm-samsung and samsung_pwm_timer
> > >  drivers
> > >  * and some registers need access synchronization. If both drivers are
> > >  * compiled in, the spinlock is defined in the clocksource driver,
> > >  * otherwise following definition is used.
> > >  *
> > >  * Currently we do not need any more complex synchronization method
> > >  * because all the supported SoCs contain only one instance of the PWM
> > >  * IP. Should this change, both drivers will need to be modified to
> > >  * properly synchronize accesses to particular instances.
> > >  */
> > 
> > I see that you can't be persuaded. And everybody else seems to be okay
> > with it so... have it your way. I'm probably going to regret this.
> 
> This was just a proposal. Do you want anything else to be added to the 
> comment or anything to be changed?

For reference, I still don't like this but since I'm the only one
complaining, go ahead. With that comment added to the driver:

Acked-by: Thierry Reding <thierry.reding@gmail.com>
Tomasz Figa June 25, 2013, 11:19 a.m. UTC | #10
On Tuesday 25 of June 2013 12:26:47 Thierry Reding wrote:
> On Mon, Jun 24, 2013 at 11:17:03PM +0200, Tomasz Figa wrote:
> > On Monday 24 of June 2013 22:53:42 Thierry Reding wrote:
> > > On Mon, Jun 24, 2013 at 10:32:55PM +0200, Tomasz Figa wrote:
> [...]
> 
> > > > What about:
> > > > 
> > > > /*
> > > > 
> > > >  * PWM block is shared between pwm-samsung and samsung_pwm_timer
> > > >  drivers
> > > >  * and some registers need access synchronization. If both drivers
> > > >  are
> > > >  * compiled in, the spinlock is defined in the clocksource driver,
> > > >  * otherwise following definition is used.
> > > >  *
> > > >  * Currently we do not need any more complex synchronization method
> > > >  * because all the supported SoCs contain only one instance of the
> > > >  PWM
> > > >  * IP. Should this change, both drivers will need to be modified to
> > > >  * properly synchronize accesses to particular instances.
> > > >  */
> > > 
> > > I see that you can't be persuaded. And everybody else seems to be
> > > okay
> > > with it so... have it your way. I'm probably going to regret this.
> > 
> > This was just a proposal. Do you want anything else to be added to the
> > comment or anything to be changed?
> 
> For reference, I still don't like this but since I'm the only one
> complaining, go ahead. With that comment added to the driver:
> 
> Acked-by: Thierry Reding <thierry.reding@gmail.com>

OK. Thank you.

Kukjin, can you amend the comment I mentioned to this patch above the line 
with DEFINE_SPINLOCK or I should send fixed version?

Best regards,
Tomasz
Mark Brown June 25, 2013, 3:18 p.m. UTC | #11
On Tue, Jun 25, 2013 at 01:19:54PM +0200, Tomasz Figa wrote:

> Kukjin, can you amend the comment I mentioned to this patch above the line 
> with DEFINE_SPINLOCK or I should send fixed version?

If you're amending stuff please feel free to also add

Tested-by: Mark Brown <broonie@linaro.org>

(for the whole series)
Kim Kukjin June 25, 2013, 4:30 p.m. UTC | #12
On 06/25/13 02:49, Thierry Reding wrote:

[...]

> Note that you can't apply patches that touch the PWM tree without my Ack
> and I already mentioned that the current way this driver is written
> isn't acceptable.
>
> So either you fix it properly, or if everybody except me thinks we don't
> need a proper design for drivers anymore, then the only way I'll accept
> this driver into the PWM tree is if you put a really big comment at the
> top of the file saying that the driver is badly designed on purpose and
> that people shouldn't be using it as a reference.
>
Thierry, hmm...I thought Tomasz addressed comments from you and probably 
there are some comments I I missed...OK, I see what your concern is.

Let me drop this series for upcoming merge window.

- Kukjin
Kim Kukjin June 25, 2013, 4:41 p.m. UTC | #13
On 06/25/13 20:19, Tomasz Figa wrote:

[...]

>>>>> /*
>>>>>
>>>>>   * PWM block is shared between pwm-samsung and samsung_pwm_timer
>>>>>   drivers
>>>>>   * and some registers need access synchronization. If both drivers
>>>>>   are
>>>>>   * compiled in, the spinlock is defined in the clocksource driver,
>>>>>   * otherwise following definition is used.
>>>>>   *
>>>>>   * Currently we do not need any more complex synchronization method
>>>>>   * because all the supported SoCs contain only one instance of the
>>>>>   PWM
>>>>>   * IP. Should this change, both drivers will need to be modified to
>>>>>   * properly synchronize accesses to particular instances.
>>>>>   */
>>>>
>>>> I see that you can't be persuaded. And everybody else seems to be
>>>> okay
>>>> with it so... have it your way. I'm probably going to regret this.
>>>
>>> This was just a proposal. Do you want anything else to be added to the
>>> comment or anything to be changed?
>>
>> For reference, I still don't like this but since I'm the only one
>> complaining, go ahead. With that comment added to the driver:
>>
>> Acked-by: Thierry Reding<thierry.reding@gmail.com>
>
> OK. Thank you.
>
> Kukjin, can you amend the comment I mentioned to this patch above the line
> with DEFINE_SPINLOCK or I should send fixed version?
>
Tomasz, please re-send this one with fixing it but I'm not sure this can 
be sent for upcoming merge window because it's a little bit late. 
Anyway, let me try it.

Thanks,
- Kukjin
diff mbox

Patch

diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 229a599..833c3ac 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -9,6 +9,7 @@  obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
 obj-$(CONFIG_PWM_PUV3)		+= pwm-puv3.o
 obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
 obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung-legacy.o
+obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
 obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
 obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
 obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c
new file mode 100644
index 0000000..04fa29c
--- /dev/null
+++ b/drivers/pwm/pwm-samsung.c
@@ -0,0 +1,601 @@ 
+/*
+ * Copyright (c) 2007 Ben Dooks
+ * Copyright (c) 2008 Simtec Electronics
+ *     Ben Dooks <ben@simtec.co.uk>, <ben-linux@fluff.org>
+ * Copyright (c) 2013 Tomasz Figa <tomasz.figa@gmail.com>
+ *
+ * PWM driver for Samsung SoCs
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License.
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/export.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/time.h>
+
+/* For struct samsung_timer_variant and samsung_pwm_lock. */
+#include <clocksource/samsung_pwm.h>
+
+#define REG_TCFG0			0x00
+#define REG_TCFG1			0x04
+#define REG_TCON			0x08
+
+#define REG_TCNTB(tmr)			(0x0c + ((tmr) * 0xc))
+#define REG_TCMPB(tmr)			(0x10 + ((tmr) * 0xc))
+
+#define TCFG0_PRESCALER_MASK		0xff
+#define TCFG0_PRESCALER1_SHIFT		8
+
+#define TCFG1_MUX_MASK			0xf
+#define TCFG1_SHIFT(x)			((x) * 4)
+
+#define TCON_START(chan)		(1 << (4 * (chan) + 0))
+#define TCON_MANUALUPDATE(chan)		(1 << (4 * (chan) + 1))
+#define TCON_INVERT(chan)		(1 << (4 * (chan) + 2))
+#define TCON_AUTORELOAD(chan)		(1 << (4 * (chan) \
+						+ (((chan) < 5) ? 3 : 2)))
+
+/**
+ * struct samsung_pwm_channel - private data of PWM channel
+ * @period_ns:	current period in nanoseconds programmed to the hardware
+ * @duty_ns:	current duty time in nanoseconds programmed to the hardware
+ * @tin_ns:	time of one timer tick in nanoseconds with current timer rate
+ */
+struct samsung_pwm_channel {
+	u32 period_ns;
+	u32 duty_ns;
+	u32 tin_ns;
+};
+
+/**
+ * struct samsung_pwm_chip - private data of PWM chip
+ * @chip:		generic PWM chip
+ * @variant:		local copy of hardware variant data
+ * @inverter_mask:	inverter status for all channels - one bit per channel
+ * @base:		base address of mapped PWM registers
+ * @base_clk:		base clock used to drive the timers
+ * @tclk0:		external clock 0 (can be ERR_PTR if not present)
+ * @tclk1:		external clock 1 (can be ERR_PTR if not present)
+ */
+struct samsung_pwm_chip {
+	struct pwm_chip chip;
+	struct samsung_pwm_variant variant;
+	u8 inverter_mask;
+
+	void __iomem *base;
+	struct clk *base_clk;
+	struct clk *tclk0;
+	struct clk *tclk1;
+};
+
+#ifndef CONFIG_CLKSRC_SAMSUNG_PWM
+/*
+ * PWM block is shared between pwm-samsung and samsung_pwm_timer drivers
+ * and some registers need access synchronization. If both drivers are
+ * compiled in, the spinlock is defined in the clocksource driver,
+ * otherwise following definition is used.
+ */
+static DEFINE_SPINLOCK(samsung_pwm_lock);
+#endif
+
+static inline
+struct samsung_pwm_chip *to_samsung_pwm_chip(struct pwm_chip *chip)
+{
+	return container_of(chip, struct samsung_pwm_chip, chip);
+}
+
+static inline unsigned int to_tcon_channel(unsigned int channel)
+{
+	/* TCON register has a gap of 4 bits (1 channel) */
+	return (channel == 0) ? 0 : (channel + 1);
+}
+
+static void pwm_samsung_set_divisor(struct samsung_pwm_chip *pwm,
+				    unsigned int channel, u8 divisor)
+{
+	u8 shift = TCFG1_SHIFT(channel);
+	unsigned long flags;
+	u32 reg;
+	u8 bits;
+
+	bits = (fls(divisor) - 1) - pwm->variant.div_base;
+
+	spin_lock_irqsave(&samsung_pwm_lock, flags);
+
+	reg = readl(pwm->base + REG_TCFG1);
+	reg &= ~(TCFG1_MUX_MASK << shift);
+	reg |= bits << shift;
+	writel(reg, pwm->base + REG_TCFG1);
+
+	spin_unlock_irqrestore(&samsung_pwm_lock, flags);
+}
+
+static int pwm_samsung_is_tdiv(struct samsung_pwm_chip *chip, unsigned int chan)
+{
+	struct samsung_pwm_variant *variant = &chip->variant;
+	u32 reg;
+
+	reg = readl(chip->base + REG_TCFG1);
+	reg >>= TCFG1_SHIFT(chan);
+	reg &= TCFG1_MUX_MASK;
+
+	return (BIT(reg) & variant->tclk_mask) == 0;
+}
+
+static unsigned long pwm_samsung_get_tin_rate(struct samsung_pwm_chip *chip,
+					      unsigned int chan)
+{
+	unsigned long rate;
+	u32 reg;
+
+	rate = clk_get_rate(chip->base_clk);
+
+	reg = readl(chip->base + REG_TCFG0);
+	if (chan >= 2)
+		reg >>= TCFG0_PRESCALER1_SHIFT;
+	reg &= TCFG0_PRESCALER_MASK;
+
+	return rate / (reg + 1);
+}
+
+static unsigned long pwm_samsung_calc_tin(struct samsung_pwm_chip *chip,
+					  unsigned int chan, unsigned long freq)
+{
+	struct samsung_pwm_variant *variant = &chip->variant;
+	unsigned long rate;
+	struct clk *clk;
+	u8 div;
+
+	if (!pwm_samsung_is_tdiv(chip, chan)) {
+		clk = (chan < 2) ? chip->tclk0 : chip->tclk1;
+		if (!IS_ERR(clk)) {
+			rate = clk_get_rate(clk);
+			if (rate)
+				return rate;
+		}
+
+		dev_warn(chip->chip.dev,
+			"tclk of PWM %d is inoperational, using tdiv\n", chan);
+	}
+
+	rate = pwm_samsung_get_tin_rate(chip, chan);
+	dev_dbg(chip->chip.dev, "tin parent at %lu\n", rate);
+
+	/*
+	 * Compare minimum PWM frequency that can be achieved with possible
+	 * divider settings and choose the lowest divisor that can generate
+	 * frequencies lower than requested.
+	 */
+	for (div = variant->div_base; div < 4; ++div)
+		if ((rate >> (variant->bits + div)) < freq)
+			break;
+
+	pwm_samsung_set_divisor(chip, chan, BIT(div));
+
+	return rate >> div;
+}
+
+static int pwm_samsung_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct samsung_pwm_chip *our_chip = to_samsung_pwm_chip(chip);
+	struct samsung_pwm_channel *our_chan;
+
+	if (!(our_chip->variant.output_mask & (BIT(pwm->hwpwm)))) {
+		dev_warn(chip->dev,
+			"tried to request PWM channel %d without output\n",
+			pwm->hwpwm);
+		return -EINVAL;
+	}
+
+	our_chan = devm_kzalloc(chip->dev, sizeof(*our_chan), GFP_KERNEL);
+	if (!our_chan)
+		return -ENOMEM;
+
+	pwm_set_chip_data(pwm, our_chan);
+
+	return 0;
+}
+
+static void pwm_samsung_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	pwm_set_chip_data(pwm, NULL);
+	devm_kfree(chip->dev, pwm_get_chip_data(pwm));
+}
+
+static int pwm_samsung_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct samsung_pwm_chip *our_chip = to_samsung_pwm_chip(chip);
+	unsigned int tcon_chan = to_tcon_channel(pwm->hwpwm);
+	unsigned long flags;
+	u32 tcon;
+
+	spin_lock_irqsave(&samsung_pwm_lock, flags);
+
+	tcon = readl(our_chip->base + REG_TCON);
+
+	tcon &= ~TCON_START(tcon_chan);
+	tcon |= TCON_MANUALUPDATE(tcon_chan);
+	writel(tcon, our_chip->base + REG_TCON);
+
+	tcon &= ~TCON_MANUALUPDATE(tcon_chan);
+	tcon |= TCON_START(tcon_chan) | TCON_AUTORELOAD(tcon_chan);
+	writel(tcon, our_chip->base + REG_TCON);
+
+	spin_unlock_irqrestore(&samsung_pwm_lock, flags);
+
+	return 0;
+}
+
+static void pwm_samsung_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct samsung_pwm_chip *our_chip = to_samsung_pwm_chip(chip);
+	unsigned int tcon_chan = to_tcon_channel(pwm->hwpwm);
+	unsigned long flags;
+	u32 tcon;
+
+	spin_lock_irqsave(&samsung_pwm_lock, flags);
+
+	tcon = readl(our_chip->base + REG_TCON);
+	tcon &= ~TCON_AUTORELOAD(tcon_chan);
+	writel(tcon, our_chip->base + REG_TCON);
+
+	spin_unlock_irqrestore(&samsung_pwm_lock, flags);
+}
+
+static int pwm_samsung_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			      int duty_ns, int period_ns)
+{
+	struct samsung_pwm_chip *our_chip = to_samsung_pwm_chip(chip);
+	struct samsung_pwm_channel *chan = pwm_get_chip_data(pwm);
+	u32 tin_ns = chan->tin_ns, tcnt, tcmp;
+
+	/*
+	 * We currently avoid using 64bit arithmetic by using the
+	 * fact that anything faster than 1Hz is easily representable
+	 * by 32bits.
+	 */
+	if (period_ns > NSEC_PER_SEC)
+		return -ERANGE;
+
+	if (period_ns == chan->period_ns && duty_ns == chan->duty_ns)
+		return 0;
+
+	tcnt = readl(our_chip->base + REG_TCNTB(pwm->hwpwm));
+
+	/* We need tick count for calculation, not last tick. */
+	++tcnt;
+
+	/* Check to see if we are changing the clock rate of the PWM. */
+	if (chan->period_ns != period_ns) {
+		unsigned long tin_rate;
+		u32 period;
+
+		period = NSEC_PER_SEC / period_ns;
+
+		dev_dbg(our_chip->chip.dev, "duty_ns=%d, period_ns=%d (%u)\n",
+						duty_ns, period_ns, period);
+
+		tin_rate = pwm_samsung_calc_tin(our_chip, pwm->hwpwm, period);
+
+		dev_dbg(our_chip->chip.dev, "tin_rate=%lu\n", tin_rate);
+
+		tin_ns = NSEC_PER_SEC / tin_rate;
+		tcnt = period_ns / tin_ns;
+	}
+
+	/* Period is too short. */
+	if (tcnt <= 1)
+		return -ERANGE;
+
+	/* Note that counters count down. */
+	tcmp = duty_ns / tin_ns;
+
+	/* 0% duty is not available */
+	if (!tcmp)
+		++tcmp;
+
+	tcmp = tcnt - tcmp;
+
+	/* Decrement to get tick numbers, instead of tick counts. */
+	--tcnt;
+	/* -1UL will give 100% duty. */
+	--tcmp;
+
+	dev_dbg(our_chip->chip.dev,
+				"tin_ns=%u, tcmp=%u/%u\n", tin_ns, tcmp, tcnt);
+
+	/* Update PWM registers. */
+	writel(tcnt, our_chip->base + REG_TCNTB(pwm->hwpwm));
+	writel(tcmp, our_chip->base + REG_TCMPB(pwm->hwpwm));
+
+	if (test_bit(PWMF_ENABLED, &pwm->flags))
+		pwm_samsung_enable(chip, pwm);
+
+	chan->period_ns = period_ns;
+	chan->tin_ns = tin_ns;
+	chan->duty_ns = duty_ns;
+
+	return 0;
+}
+
+static void pwm_samsung_set_invert(struct samsung_pwm_chip *chip,
+				   unsigned int channel, bool invert)
+{
+	unsigned int tcon_chan = to_tcon_channel(channel);
+	unsigned long flags;
+	u32 tcon;
+
+	spin_lock_irqsave(&samsung_pwm_lock, flags);
+
+	tcon = readl(chip->base + REG_TCON);
+
+	if (invert) {
+		chip->inverter_mask |= BIT(channel);
+		tcon |= TCON_INVERT(tcon_chan);
+	} else {
+		chip->inverter_mask &= ~BIT(channel);
+		tcon &= ~TCON_INVERT(tcon_chan);
+	}
+
+	writel(tcon, chip->base + REG_TCON);
+
+	spin_unlock_irqrestore(&samsung_pwm_lock, flags);
+}
+
+static int pwm_samsung_set_polarity(struct pwm_chip *chip,
+				    struct pwm_device *pwm,
+				    enum pwm_polarity polarity)
+{
+	struct samsung_pwm_chip *our_chip = to_samsung_pwm_chip(chip);
+	bool invert = (polarity == PWM_POLARITY_NORMAL);
+
+	/* Inverted means normal in the hardware. */
+	pwm_samsung_set_invert(our_chip, pwm->hwpwm, invert);
+
+	return 0;
+}
+
+static const struct pwm_ops pwm_samsung_ops = {
+	.request	= pwm_samsung_request,
+	.free		= pwm_samsung_free,
+	.enable		= pwm_samsung_enable,
+	.disable	= pwm_samsung_disable,
+	.config		= pwm_samsung_config,
+	.set_polarity	= pwm_samsung_set_polarity,
+	.owner		= THIS_MODULE,
+};
+
+#ifdef CONFIG_OF
+static const struct samsung_pwm_variant s3c24xx_variant = {
+	.bits		= 16,
+	.div_base	= 1,
+	.has_tint_cstat	= false,
+	.tclk_mask	= BIT(4),
+};
+
+static const struct samsung_pwm_variant s3c64xx_variant = {
+	.bits		= 32,
+	.div_base	= 0,
+	.has_tint_cstat	= true,
+	.tclk_mask	= BIT(7) | BIT(6) | BIT(5),
+};
+
+static const struct samsung_pwm_variant s5p64x0_variant = {
+	.bits		= 32,
+	.div_base	= 0,
+	.has_tint_cstat	= true,
+};
+
+static const struct samsung_pwm_variant s5p_variant = {
+	.bits		= 32,
+	.div_base	= 0,
+	.has_tint_cstat	= true,
+	.tclk_mask	= BIT(5),
+};
+
+static const struct of_device_id samsung_pwm_matches[] = {
+	{ .compatible = "samsung,s3c2410-pwm", .data = &s3c24xx_variant },
+	{ .compatible = "samsung,s3c6400-pwm", .data = &s3c64xx_variant },
+	{ .compatible = "samsung,s5p6440-pwm", .data = &s5p64x0_variant },
+	{ .compatible = "samsung,s5pc100-pwm", .data = &s5p_variant },
+	{ .compatible = "samsung,exynos4210-pwm", .data = &s5p64x0_variant },
+	{},
+};
+
+static int pwm_samsung_parse_dt(struct samsung_pwm_chip *chip)
+{
+	struct device_node *np = chip->chip.dev->of_node;
+	const struct of_device_id *match;
+	struct property *prop;
+	const __be32 *cur;
+	u32 val;
+
+	match = of_match_node(samsung_pwm_matches, np);
+	if (!match)
+		return -ENODEV;
+
+	memcpy(&chip->variant, match->data, sizeof(chip->variant));
+
+	of_property_for_each_u32(np, "samsung,pwm-outputs", prop, cur, val) {
+		if (val >= SAMSUNG_PWM_NUM) {
+			pr_warning("%s: invalid channel index in samsung,pwm-outputs property\n",
+								__func__);
+			continue;
+		}
+		chip->variant.output_mask |= BIT(val);
+	}
+
+	return 0;
+}
+#else
+static int pwm_samsung_parse_dt(struct samsung_pwm_chip *chip)
+{
+	return -ENODEV;
+}
+#endif
+
+static int pwm_samsung_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct samsung_pwm_chip *chip;
+	struct resource *res;
+	unsigned int chan;
+	int ret;
+
+	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
+	if (chip == NULL)
+		return -ENOMEM;
+
+	chip->chip.dev = &pdev->dev;
+	chip->chip.ops = &pwm_samsung_ops;
+	chip->chip.base = -1;
+	chip->chip.npwm = SAMSUNG_PWM_NUM;
+	chip->inverter_mask = BIT(SAMSUNG_PWM_NUM) - 1;
+
+	if (IS_ENABLED(CONFIG_OF) && pdev->dev.of_node) {
+		ret = pwm_samsung_parse_dt(chip);
+		if (ret)
+			return ret;
+
+		chip->chip.of_xlate = of_pwm_xlate_with_flags;
+		chip->chip.of_pwm_n_cells = 3;
+	} else {
+		if (!pdev->dev.platform_data) {
+			dev_err(&pdev->dev, "no platform data specified\n");
+			return -EINVAL;
+		}
+
+		memcpy(&chip->variant, pdev->dev.platform_data,
+							sizeof(chip->variant));
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	chip->base = devm_ioremap_resource(&pdev->dev, res);
+	if (!chip->base)
+		return -ENOMEM;
+
+	chip->base_clk = devm_clk_get(&pdev->dev, "timers");
+	if (IS_ERR(chip->base_clk)) {
+		dev_err(dev, "failed to get timer base clk\n");
+		return PTR_ERR(chip->base_clk);
+	}
+
+	ret = clk_prepare_enable(chip->base_clk);
+	if (ret < 0) {
+		dev_err(dev, "failed to enable base clock\n");
+		return ret;
+	}
+
+	for (chan = 0; chan < SAMSUNG_PWM_NUM; ++chan)
+		if (chip->variant.output_mask & BIT(chan))
+			pwm_samsung_set_invert(chip, chan, true);
+
+	/* Following clocks are optional. */
+	chip->tclk0 = devm_clk_get(&pdev->dev, "pwm-tclk0");
+	chip->tclk1 = devm_clk_get(&pdev->dev, "pwm-tclk1");
+
+	platform_set_drvdata(pdev, chip);
+
+	ret = pwmchip_add(&chip->chip);
+	if (ret < 0) {
+		dev_err(dev, "failed to register PWM chip\n");
+		clk_disable_unprepare(chip->base_clk);
+		return ret;
+	}
+
+	dev_dbg(dev, "base_clk at %lu, tclk0 at %lu, tclk1 at %lu\n",
+		clk_get_rate(chip->base_clk),
+		!IS_ERR(chip->tclk0) ? clk_get_rate(chip->tclk0) : 0,
+		!IS_ERR(chip->tclk1) ? clk_get_rate(chip->tclk1) : 0);
+
+	return 0;
+}
+
+static int pwm_samsung_remove(struct platform_device *pdev)
+{
+	struct samsung_pwm_chip *chip = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = pwmchip_remove(&chip->chip);
+	if (ret < 0)
+		return ret;
+
+	clk_disable_unprepare(chip->base_clk);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int pwm_samsung_suspend(struct device *dev)
+{
+	struct samsung_pwm_chip *chip = dev_get_drvdata(dev);
+	unsigned int i;
+
+	/*
+	 * No one preserves these values during suspend so reset them.
+	 * Otherwise driver leaves PWM unconfigured if same values are
+	 * passed to pwm_config() next time.
+	 */
+	for (i = 0; i < SAMSUNG_PWM_NUM; ++i) {
+		struct pwm_device *pwm = &chip->chip.pwms[i];
+		struct samsung_pwm_channel *chan = pwm_get_chip_data(pwm);
+
+		if (!chan)
+			continue;
+
+		chan->period_ns = 0;
+		chan->duty_ns = 0;
+	}
+
+	return 0;
+}
+
+static int pwm_samsung_resume(struct device *dev)
+{
+	struct samsung_pwm_chip *chip = dev_get_drvdata(dev);
+	unsigned int chan;
+
+	/*
+	 * Inverter setting must be preserved across suspend/resume
+	 * as nobody really seems to configure it more than once.
+	 */
+	for (chan = 0; chan < SAMSUNG_PWM_NUM; ++chan) {
+		if (chip->variant.output_mask & BIT(chan))
+			pwm_samsung_set_invert(chip, chan,
+					chip->inverter_mask & BIT(chan));
+	}
+
+	return 0;
+}
+#endif
+
+static struct dev_pm_ops pwm_samsung_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(pwm_samsung_suspend, pwm_samsung_resume)
+};
+
+static struct platform_driver pwm_samsung_driver = {
+	.driver		= {
+		.name	= "samsung-pwm",
+		.owner	= THIS_MODULE,
+		.pm	= &pwm_samsung_pm_ops,
+		.of_match_table = of_match_ptr(samsung_pwm_matches),
+	},
+	.probe		= pwm_samsung_probe,
+	.remove		= pwm_samsung_remove,
+};
+module_platform_driver(pwm_samsung_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Tomasz Figa <tomasz.figa@gmail.com>");
+MODULE_ALIAS("platform:samsung-pwm");