diff mbox

[v5,15/46] pwm: introduce the pwm_state concept

Message ID 1459368249-13241-16-git-send-email-boris.brezillon@free-electrons.com (mailing list archive)
State New, archived
Headers show

Commit Message

Boris BREZILLON March 30, 2016, 8:03 p.m. UTC
The PWM state, represented by its period, duty_cycle and polarity,
is currently directly stored in the PWM device.
Declare a pwm_state structure embedding those field so that we can later
use this struct to atomically update all the PWM parameters at once.

All pwm_get_xxx() helpers are now implemented as wrappers around
pwm_get_state().

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/pwm/core.c  |  8 ++++----
 include/linux/pwm.h | 54 +++++++++++++++++++++++++++++++++++++++++------------
 2 files changed, 46 insertions(+), 16 deletions(-)

Comments

Thierry Reding April 12, 2016, 11:49 a.m. UTC | #1
On Wed, Mar 30, 2016 at 10:03:38PM +0200, Boris Brezillon wrote:
> The PWM state, represented by its period, duty_cycle and polarity,
> is currently directly stored in the PWM device.
> Declare a pwm_state structure embedding those field so that we can later
> use this struct to atomically update all the PWM parameters at once.
> 
> All pwm_get_xxx() helpers are now implemented as wrappers around
> pwm_get_state().
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  drivers/pwm/core.c  |  8 ++++----
>  include/linux/pwm.h | 54 +++++++++++++++++++++++++++++++++++++++++------------
>  2 files changed, 46 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 6433059..f3f91e7 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -268,7 +268,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
>  		pwm->chip = chip;
>  		pwm->pwm = chip->base + i;
>  		pwm->hwpwm = i;
> -		pwm->polarity = polarity;
> +		pwm->state.polarity = polarity;

Would this not more correctly be assigned to pwm->args.polarity? After
all this is setting up the "initial" state, much like DT or the lookup
tables would for duty cycle and period.

Thierry
Boris BREZILLON April 12, 2016, 12:17 p.m. UTC | #2
On Tue, 12 Apr 2016 13:49:04 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Wed, Mar 30, 2016 at 10:03:38PM +0200, Boris Brezillon wrote:
> > The PWM state, represented by its period, duty_cycle and polarity,
> > is currently directly stored in the PWM device.
> > Declare a pwm_state structure embedding those field so that we can later
> > use this struct to atomically update all the PWM parameters at once.
> > 
> > All pwm_get_xxx() helpers are now implemented as wrappers around
> > pwm_get_state().
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> >  drivers/pwm/core.c  |  8 ++++----
> >  include/linux/pwm.h | 54 +++++++++++++++++++++++++++++++++++++++++------------
> >  2 files changed, 46 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index 6433059..f3f91e7 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -268,7 +268,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
> >  		pwm->chip = chip;
> >  		pwm->pwm = chip->base + i;
> >  		pwm->hwpwm = i;
> > -		pwm->polarity = polarity;
> > +		pwm->state.polarity = polarity;
> 
> Would this not more correctly be assigned to pwm->args.polarity? After
> all this is setting up the "initial" state, much like DT or the lookup
> tables would for duty cycle and period.

Yes, I wasn't sure about the pwm_add_with_polarity() meaning. To me,
all the reference info should be extracted from DT, PWM lookup table or
driver specific ->request() implementation, but I can definitely
initialize the args.polarity here too.

Should I keep the pwm->state.polarity assignment (to set the initial
polarity when the driver does not support hardware readout)?
Thierry Reding April 12, 2016, 12:21 p.m. UTC | #3
On Tue, Apr 12, 2016 at 02:17:18PM +0200, Boris Brezillon wrote:
> On Tue, 12 Apr 2016 13:49:04 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> > On Wed, Mar 30, 2016 at 10:03:38PM +0200, Boris Brezillon wrote:
> > > The PWM state, represented by its period, duty_cycle and polarity,
> > > is currently directly stored in the PWM device.
> > > Declare a pwm_state structure embedding those field so that we can later
> > > use this struct to atomically update all the PWM parameters at once.
> > > 
> > > All pwm_get_xxx() helpers are now implemented as wrappers around
> > > pwm_get_state().
> > > 
> > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > ---
> > >  drivers/pwm/core.c  |  8 ++++----
> > >  include/linux/pwm.h | 54 +++++++++++++++++++++++++++++++++++++++++------------
> > >  2 files changed, 46 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > > index 6433059..f3f91e7 100644
> > > --- a/drivers/pwm/core.c
> > > +++ b/drivers/pwm/core.c
> > > @@ -268,7 +268,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
> > >  		pwm->chip = chip;
> > >  		pwm->pwm = chip->base + i;
> > >  		pwm->hwpwm = i;
> > > -		pwm->polarity = polarity;
> > > +		pwm->state.polarity = polarity;
> > 
> > Would this not more correctly be assigned to pwm->args.polarity? After
> > all this is setting up the "initial" state, much like DT or the lookup
> > tables would for duty cycle and period.
> 
> Yes, I wasn't sure about the pwm_add_with_polarity() meaning. To me,
> all the reference info should be extracted from DT, PWM lookup table or
> driver specific ->request() implementation, but I can definitely
> initialize the args.polarity here too.
> 
> Should I keep the pwm->state.polarity assignment (to set the initial
> polarity when the driver does not support hardware readout)?

Wouldn't this work automatically as part of the pwm_apply_args() helper
if we extended it with this setting?

Thierry
Boris BREZILLON April 12, 2016, 12:45 p.m. UTC | #4
On Tue, 12 Apr 2016 14:21:41 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Tue, Apr 12, 2016 at 02:17:18PM +0200, Boris Brezillon wrote:
> > On Tue, 12 Apr 2016 13:49:04 +0200
> > Thierry Reding <thierry.reding@gmail.com> wrote:
> > 
> > > On Wed, Mar 30, 2016 at 10:03:38PM +0200, Boris Brezillon wrote:
> > > > The PWM state, represented by its period, duty_cycle and polarity,
> > > > is currently directly stored in the PWM device.
> > > > Declare a pwm_state structure embedding those field so that we can later
> > > > use this struct to atomically update all the PWM parameters at once.
> > > > 
> > > > All pwm_get_xxx() helpers are now implemented as wrappers around
> > > > pwm_get_state().
> > > > 
> > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > > ---
> > > >  drivers/pwm/core.c  |  8 ++++----
> > > >  include/linux/pwm.h | 54 +++++++++++++++++++++++++++++++++++++++++------------
> > > >  2 files changed, 46 insertions(+), 16 deletions(-)
> > > > 
> > > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > > > index 6433059..f3f91e7 100644
> > > > --- a/drivers/pwm/core.c
> > > > +++ b/drivers/pwm/core.c
> > > > @@ -268,7 +268,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
> > > >  		pwm->chip = chip;
> > > >  		pwm->pwm = chip->base + i;
> > > >  		pwm->hwpwm = i;
> > > > -		pwm->polarity = polarity;
> > > > +		pwm->state.polarity = polarity;
> > > 
> > > Would this not more correctly be assigned to pwm->args.polarity? After
> > > all this is setting up the "initial" state, much like DT or the lookup
> > > tables would for duty cycle and period.
> > 
> > Yes, I wasn't sure about the pwm_add_with_polarity() meaning. To me,
> > all the reference info should be extracted from DT, PWM lookup table or
> > driver specific ->request() implementation, but I can definitely
> > initialize the args.polarity here too.
> > 
> > Should I keep the pwm->state.polarity assignment (to set the initial
> > polarity when the driver does not support hardware readout)?
> 
> Wouldn't this work automatically as part of the pwm_apply_args() helper
> if we extended it with this setting?

Well, as you explained in you answer to patch 5, pwm_apply_args()
should be called on a per-request basis (each time a PWM device is
requested), while the initial polarity setting should only be applied
when registering the PWM chip (and its devices). After that, the
framework takes care of keeping the PWM state in sync with the hardware
state.

Let's take a real (though a bit unusual) example. Say you have a single
PWM device referenced by two different users. Only one user can be
enabled at a time, but each of them has its own reference config
(different polarity, different period).

User1 calls pwm_get() and applies its own polarity and period. Then
user1 is unregistered and release the PWM device, leaving the polarity
and period untouched.

User2 is registered and request the same PWM device, but user2 is
smarter and tries to extract the current PWM state before adapting the
config according to pwm_args. If you just reset pwm->state.polarity
each time pwm_apply_args() is called (and you suggested to call it as
part of the request procedure), then this means the PWM state is no
longer in sync with the hardware state.
Thierry Reding April 12, 2016, 1:11 p.m. UTC | #5
On Tue, Apr 12, 2016 at 02:45:08PM +0200, Boris Brezillon wrote:
> On Tue, 12 Apr 2016 14:21:41 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> > On Tue, Apr 12, 2016 at 02:17:18PM +0200, Boris Brezillon wrote:
> > > On Tue, 12 Apr 2016 13:49:04 +0200
> > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > > 
> > > > On Wed, Mar 30, 2016 at 10:03:38PM +0200, Boris Brezillon wrote:
> > > > > The PWM state, represented by its period, duty_cycle and polarity,
> > > > > is currently directly stored in the PWM device.
> > > > > Declare a pwm_state structure embedding those field so that we can later
> > > > > use this struct to atomically update all the PWM parameters at once.
> > > > > 
> > > > > All pwm_get_xxx() helpers are now implemented as wrappers around
> > > > > pwm_get_state().
> > > > > 
> > > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > > > ---
> > > > >  drivers/pwm/core.c  |  8 ++++----
> > > > >  include/linux/pwm.h | 54 +++++++++++++++++++++++++++++++++++++++++------------
> > > > >  2 files changed, 46 insertions(+), 16 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > > > > index 6433059..f3f91e7 100644
> > > > > --- a/drivers/pwm/core.c
> > > > > +++ b/drivers/pwm/core.c
> > > > > @@ -268,7 +268,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
> > > > >  		pwm->chip = chip;
> > > > >  		pwm->pwm = chip->base + i;
> > > > >  		pwm->hwpwm = i;
> > > > > -		pwm->polarity = polarity;
> > > > > +		pwm->state.polarity = polarity;
> > > > 
> > > > Would this not more correctly be assigned to pwm->args.polarity? After
> > > > all this is setting up the "initial" state, much like DT or the lookup
> > > > tables would for duty cycle and period.
> > > 
> > > Yes, I wasn't sure about the pwm_add_with_polarity() meaning. To me,
> > > all the reference info should be extracted from DT, PWM lookup table or
> > > driver specific ->request() implementation, but I can definitely
> > > initialize the args.polarity here too.
> > > 
> > > Should I keep the pwm->state.polarity assignment (to set the initial
> > > polarity when the driver does not support hardware readout)?
> > 
> > Wouldn't this work automatically as part of the pwm_apply_args() helper
> > if we extended it with this setting?
> 
> Well, as you explained in you answer to patch 5, pwm_apply_args()
> should be called on a per-request basis (each time a PWM device is
> requested), while the initial polarity setting should only be applied
> when registering the PWM chip (and its devices). After that, the
> framework takes care of keeping the PWM state in sync with the hardware
> state.
> 
> Let's take a real (though a bit unusual) example. Say you have a single
> PWM device referenced by two different users. Only one user can be
> enabled at a time, but each of them has its own reference config
> (different polarity, different period).
> 
> User1 calls pwm_get() and applies its own polarity and period. Then
> user1 is unregistered and release the PWM device, leaving the polarity
> and period untouched.
> 
> User2 is registered and request the same PWM device, but user2 is
> smarter and tries to extract the current PWM state before adapting the
> config according to pwm_args. If you just reset pwm->state.polarity
> each time pwm_apply_args() is called (and you suggested to call it as
> part of the request procedure), then this means the PWM state is no
> longer in sync with the hardware state.

In that case neither will be the period or duty cycle. Essentially this
gets us back to square one where we need to decide how to handle current
state vs. initial arguments.

But I don't think this is really going to be an issue because this is
all moot until we've moved over to the atomic API, at which point this
is all going to go away anyway.

Thierry
Boris BREZILLON April 12, 2016, 1:26 p.m. UTC | #6
On Tue, 12 Apr 2016 15:11:18 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Tue, Apr 12, 2016 at 02:45:08PM +0200, Boris Brezillon wrote:
> > On Tue, 12 Apr 2016 14:21:41 +0200
> > Thierry Reding <thierry.reding@gmail.com> wrote:
> > 
> > > On Tue, Apr 12, 2016 at 02:17:18PM +0200, Boris Brezillon wrote:
> > > > On Tue, 12 Apr 2016 13:49:04 +0200
> > > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > 
> > > > > On Wed, Mar 30, 2016 at 10:03:38PM +0200, Boris Brezillon wrote:
> > > > > > The PWM state, represented by its period, duty_cycle and polarity,
> > > > > > is currently directly stored in the PWM device.
> > > > > > Declare a pwm_state structure embedding those field so that we can later
> > > > > > use this struct to atomically update all the PWM parameters at once.
> > > > > > 
> > > > > > All pwm_get_xxx() helpers are now implemented as wrappers around
> > > > > > pwm_get_state().
> > > > > > 
> > > > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > > > > ---
> > > > > >  drivers/pwm/core.c  |  8 ++++----
> > > > > >  include/linux/pwm.h | 54 +++++++++++++++++++++++++++++++++++++++++------------
> > > > > >  2 files changed, 46 insertions(+), 16 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > > > > > index 6433059..f3f91e7 100644
> > > > > > --- a/drivers/pwm/core.c
> > > > > > +++ b/drivers/pwm/core.c
> > > > > > @@ -268,7 +268,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
> > > > > >  		pwm->chip = chip;
> > > > > >  		pwm->pwm = chip->base + i;
> > > > > >  		pwm->hwpwm = i;
> > > > > > -		pwm->polarity = polarity;
> > > > > > +		pwm->state.polarity = polarity;
> > > > > 
> > > > > Would this not more correctly be assigned to pwm->args.polarity? After
> > > > > all this is setting up the "initial" state, much like DT or the lookup
> > > > > tables would for duty cycle and period.
> > > > 
> > > > Yes, I wasn't sure about the pwm_add_with_polarity() meaning. To me,
> > > > all the reference info should be extracted from DT, PWM lookup table or
> > > > driver specific ->request() implementation, but I can definitely
> > > > initialize the args.polarity here too.
> > > > 
> > > > Should I keep the pwm->state.polarity assignment (to set the initial
> > > > polarity when the driver does not support hardware readout)?
> > > 
> > > Wouldn't this work automatically as part of the pwm_apply_args() helper
> > > if we extended it with this setting?
> > 
> > Well, as you explained in you answer to patch 5, pwm_apply_args()
> > should be called on a per-request basis (each time a PWM device is
> > requested), while the initial polarity setting should only be applied
> > when registering the PWM chip (and its devices). After that, the
> > framework takes care of keeping the PWM state in sync with the hardware
> > state.
> > 
> > Let's take a real (though a bit unusual) example. Say you have a single
> > PWM device referenced by two different users. Only one user can be
> > enabled at a time, but each of them has its own reference config
> > (different polarity, different period).
> > 
> > User1 calls pwm_get() and applies its own polarity and period. Then
> > user1 is unregistered and release the PWM device, leaving the polarity
> > and period untouched.
> > 
> > User2 is registered and request the same PWM device, but user2 is
> > smarter and tries to extract the current PWM state before adapting the
> > config according to pwm_args. If you just reset pwm->state.polarity
> > each time pwm_apply_args() is called (and you suggested to call it as
> > part of the request procedure), then this means the PWM state is no
> > longer in sync with the hardware state.
> 
> In that case neither will be the period or duty cycle. Essentially this
> gets us back to square one where we need to decide how to handle current
> state vs. initial arguments.

That's not true. Now we clearly differentiate the reference config
(content of pwm_args which is only a subset of what you'll find in
pwm_state) and the PWM state (represented by pwm_state).

We should be safe as long as we keep those 2 elements as 2 orthogonal
concepts:
- pwm_args is supposed to give some hint to the PWM user to help him
  configure it's PWM appropriately
- pwm_state is here to reflect the real PWM state, and apply new
  configs

> 
> But I don't think this is really going to be an issue because this is
> all moot until we've moved over to the atomic API, at which point this
> is all going to go away anyway.

As stated in my answer to patch 5, I think I misunderstood your
suggestion. pwm_apply_args() is supposed to adjust the PWM config to
match the period and polarity specified in pwm_args, right?

If that's the case, my question is, should we really call this function
each time a new user requests a PWM instead of letting those users call
the function on-demand (not all users want to adapt the current PWM
config to the pwm_args, some may just want to apply a completely new
config).
Thierry Reding April 12, 2016, 2:05 p.m. UTC | #7
On Tue, Apr 12, 2016 at 03:26:44PM +0200, Boris Brezillon wrote:
> On Tue, 12 Apr 2016 15:11:18 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> > On Tue, Apr 12, 2016 at 02:45:08PM +0200, Boris Brezillon wrote:
> > > On Tue, 12 Apr 2016 14:21:41 +0200
> > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > > 
> > > > On Tue, Apr 12, 2016 at 02:17:18PM +0200, Boris Brezillon wrote:
> > > > > On Tue, 12 Apr 2016 13:49:04 +0200
> > > > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > > 
> > > > > > On Wed, Mar 30, 2016 at 10:03:38PM +0200, Boris Brezillon wrote:
> > > > > > > The PWM state, represented by its period, duty_cycle and polarity,
> > > > > > > is currently directly stored in the PWM device.
> > > > > > > Declare a pwm_state structure embedding those field so that we can later
> > > > > > > use this struct to atomically update all the PWM parameters at once.
> > > > > > > 
> > > > > > > All pwm_get_xxx() helpers are now implemented as wrappers around
> > > > > > > pwm_get_state().
> > > > > > > 
> > > > > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > > > > > ---
> > > > > > >  drivers/pwm/core.c  |  8 ++++----
> > > > > > >  include/linux/pwm.h | 54 +++++++++++++++++++++++++++++++++++++++++------------
> > > > > > >  2 files changed, 46 insertions(+), 16 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > > > > > > index 6433059..f3f91e7 100644
> > > > > > > --- a/drivers/pwm/core.c
> > > > > > > +++ b/drivers/pwm/core.c
> > > > > > > @@ -268,7 +268,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
> > > > > > >  		pwm->chip = chip;
> > > > > > >  		pwm->pwm = chip->base + i;
> > > > > > >  		pwm->hwpwm = i;
> > > > > > > -		pwm->polarity = polarity;
> > > > > > > +		pwm->state.polarity = polarity;
> > > > > > 
> > > > > > Would this not more correctly be assigned to pwm->args.polarity? After
> > > > > > all this is setting up the "initial" state, much like DT or the lookup
> > > > > > tables would for duty cycle and period.
> > > > > 
> > > > > Yes, I wasn't sure about the pwm_add_with_polarity() meaning. To me,
> > > > > all the reference info should be extracted from DT, PWM lookup table or
> > > > > driver specific ->request() implementation, but I can definitely
> > > > > initialize the args.polarity here too.
> > > > > 
> > > > > Should I keep the pwm->state.polarity assignment (to set the initial
> > > > > polarity when the driver does not support hardware readout)?
> > > > 
> > > > Wouldn't this work automatically as part of the pwm_apply_args() helper
> > > > if we extended it with this setting?
> > > 
> > > Well, as you explained in you answer to patch 5, pwm_apply_args()
> > > should be called on a per-request basis (each time a PWM device is
> > > requested), while the initial polarity setting should only be applied
> > > when registering the PWM chip (and its devices). After that, the
> > > framework takes care of keeping the PWM state in sync with the hardware
> > > state.
> > > 
> > > Let's take a real (though a bit unusual) example. Say you have a single
> > > PWM device referenced by two different users. Only one user can be
> > > enabled at a time, but each of them has its own reference config
> > > (different polarity, different period).
> > > 
> > > User1 calls pwm_get() and applies its own polarity and period. Then
> > > user1 is unregistered and release the PWM device, leaving the polarity
> > > and period untouched.
> > > 
> > > User2 is registered and request the same PWM device, but user2 is
> > > smarter and tries to extract the current PWM state before adapting the
> > > config according to pwm_args. If you just reset pwm->state.polarity
> > > each time pwm_apply_args() is called (and you suggested to call it as
> > > part of the request procedure), then this means the PWM state is no
> > > longer in sync with the hardware state.
> > 
> > In that case neither will be the period or duty cycle. Essentially this
> > gets us back to square one where we need to decide how to handle current
> > state vs. initial arguments.
> 
> That's not true. Now we clearly differentiate the reference config
> (content of pwm_args which is only a subset of what you'll find in
> pwm_state) and the PWM state (represented by pwm_state).
> 
> We should be safe as long as we keep those 2 elements as 2 orthogonal
> concepts:
> - pwm_args is supposed to give some hint to the PWM user to help him
>   configure it's PWM appropriately
> - pwm_state is here to reflect the real PWM state, and apply new
>   configs
> 
> > 
> > But I don't think this is really going to be an issue because this is
> > all moot until we've moved over to the atomic API, at which point this
> > is all going to go away anyway.
> 
> As stated in my answer to patch 5, I think I misunderstood your
> suggestion. pwm_apply_args() is supposed to adjust the PWM config to
> match the period and polarity specified in pwm_args, right?
> 
> If that's the case, my question is, should we really call this function
> each time a new user requests a PWM instead of letting those users call
> the function on-demand (not all users want to adapt the current PWM
> config to the pwm_args, some may just want to apply a completely new
> config).

I think we're still talking past each other. I didn't mean for this to
be a proper part of the API. Like you said the struct pwm_args doesn't
contain enough data to construct a complete state and apply it.

What I was suggesting is to factor out the individual calls to the
various pwm_set_*() functions into a single call. So we wouldn't be
changing semantics, just refactoring to make it easier to get rid of
again in one of the subsequent patches.

That is, pwm_apply_args() would go away again within this very series,
at the same point that you're currently removing the pwm_set_*() calls.

Thierry
Boris BREZILLON April 12, 2016, 2:13 p.m. UTC | #8
On Tue, 12 Apr 2016 16:05:46 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Tue, Apr 12, 2016 at 03:26:44PM +0200, Boris Brezillon wrote:
> > On Tue, 12 Apr 2016 15:11:18 +0200
> > Thierry Reding <thierry.reding@gmail.com> wrote:
> > 
> > > On Tue, Apr 12, 2016 at 02:45:08PM +0200, Boris Brezillon wrote:
> > > > On Tue, 12 Apr 2016 14:21:41 +0200
> > > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > 
> > > > > On Tue, Apr 12, 2016 at 02:17:18PM +0200, Boris Brezillon wrote:
> > > > > > On Tue, 12 Apr 2016 13:49:04 +0200
> > > > > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > > > 
> > > > > > > On Wed, Mar 30, 2016 at 10:03:38PM +0200, Boris Brezillon wrote:
> > > > > > > > The PWM state, represented by its period, duty_cycle and polarity,
> > > > > > > > is currently directly stored in the PWM device.
> > > > > > > > Declare a pwm_state structure embedding those field so that we can later
> > > > > > > > use this struct to atomically update all the PWM parameters at once.
> > > > > > > > 
> > > > > > > > All pwm_get_xxx() helpers are now implemented as wrappers around
> > > > > > > > pwm_get_state().
> > > > > > > > 
> > > > > > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > > > > > > ---
> > > > > > > >  drivers/pwm/core.c  |  8 ++++----
> > > > > > > >  include/linux/pwm.h | 54 +++++++++++++++++++++++++++++++++++++++++------------
> > > > > > > >  2 files changed, 46 insertions(+), 16 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > > > > > > > index 6433059..f3f91e7 100644
> > > > > > > > --- a/drivers/pwm/core.c
> > > > > > > > +++ b/drivers/pwm/core.c
> > > > > > > > @@ -268,7 +268,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
> > > > > > > >  		pwm->chip = chip;
> > > > > > > >  		pwm->pwm = chip->base + i;
> > > > > > > >  		pwm->hwpwm = i;
> > > > > > > > -		pwm->polarity = polarity;
> > > > > > > > +		pwm->state.polarity = polarity;
> > > > > > > 
> > > > > > > Would this not more correctly be assigned to pwm->args.polarity? After
> > > > > > > all this is setting up the "initial" state, much like DT or the lookup
> > > > > > > tables would for duty cycle and period.
> > > > > > 
> > > > > > Yes, I wasn't sure about the pwm_add_with_polarity() meaning. To me,
> > > > > > all the reference info should be extracted from DT, PWM lookup table or
> > > > > > driver specific ->request() implementation, but I can definitely
> > > > > > initialize the args.polarity here too.
> > > > > > 
> > > > > > Should I keep the pwm->state.polarity assignment (to set the initial
> > > > > > polarity when the driver does not support hardware readout)?
> > > > > 
> > > > > Wouldn't this work automatically as part of the pwm_apply_args() helper
> > > > > if we extended it with this setting?
> > > > 
> > > > Well, as you explained in you answer to patch 5, pwm_apply_args()
> > > > should be called on a per-request basis (each time a PWM device is
> > > > requested), while the initial polarity setting should only be applied
> > > > when registering the PWM chip (and its devices). After that, the
> > > > framework takes care of keeping the PWM state in sync with the hardware
> > > > state.
> > > > 
> > > > Let's take a real (though a bit unusual) example. Say you have a single
> > > > PWM device referenced by two different users. Only one user can be
> > > > enabled at a time, but each of them has its own reference config
> > > > (different polarity, different period).
> > > > 
> > > > User1 calls pwm_get() and applies its own polarity and period. Then
> > > > user1 is unregistered and release the PWM device, leaving the polarity
> > > > and period untouched.
> > > > 
> > > > User2 is registered and request the same PWM device, but user2 is
> > > > smarter and tries to extract the current PWM state before adapting the
> > > > config according to pwm_args. If you just reset pwm->state.polarity
> > > > each time pwm_apply_args() is called (and you suggested to call it as
> > > > part of the request procedure), then this means the PWM state is no
> > > > longer in sync with the hardware state.
> > > 
> > > In that case neither will be the period or duty cycle. Essentially this
> > > gets us back to square one where we need to decide how to handle current
> > > state vs. initial arguments.
> > 
> > That's not true. Now we clearly differentiate the reference config
> > (content of pwm_args which is only a subset of what you'll find in
> > pwm_state) and the PWM state (represented by pwm_state).
> > 
> > We should be safe as long as we keep those 2 elements as 2 orthogonal
> > concepts:
> > - pwm_args is supposed to give some hint to the PWM user to help him
> >   configure it's PWM appropriately
> > - pwm_state is here to reflect the real PWM state, and apply new
> >   configs
> > 
> > > 
> > > But I don't think this is really going to be an issue because this is
> > > all moot until we've moved over to the atomic API, at which point this
> > > is all going to go away anyway.
> > 
> > As stated in my answer to patch 5, I think I misunderstood your
> > suggestion. pwm_apply_args() is supposed to adjust the PWM config to
> > match the period and polarity specified in pwm_args, right?
> > 
> > If that's the case, my question is, should we really call this function
> > each time a new user requests a PWM instead of letting those users call
> > the function on-demand (not all users want to adapt the current PWM
> > config to the pwm_args, some may just want to apply a completely new
> > config).
> 
> I think we're still talking past each other. I didn't mean for this to
> be a proper part of the API. Like you said the struct pwm_args doesn't
> contain enough data to construct a complete state and apply it.
> 
> What I was suggesting is to factor out the individual calls to the
> various pwm_set_*() functions into a single call. So we wouldn't be
> changing semantics, just refactoring to make it easier to get rid of
> again in one of the subsequent patches.
> 
> That is, pwm_apply_args() would go away again within this very series,
> at the same point that you're currently removing the pwm_set_*() calls.

Okay, eventually got it :).
diff mbox

Patch

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 6433059..f3f91e7 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -268,7 +268,7 @@  int pwmchip_add_with_polarity(struct pwm_chip *chip,
 		pwm->chip = chip;
 		pwm->pwm = chip->base + i;
 		pwm->hwpwm = i;
-		pwm->polarity = polarity;
+		pwm->state.polarity = polarity;
 
 		radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
 	}
@@ -446,8 +446,8 @@  int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
 	if (err)
 		return err;
 
-	pwm->duty_cycle = duty_ns;
-	pwm->period = period_ns;
+	pwm->state.duty_cycle = duty_ns;
+	pwm->state.period = period_ns;
 
 	return 0;
 }
@@ -480,7 +480,7 @@  int pwm_set_polarity(struct pwm_device *pwm, enum pwm_polarity polarity)
 	if (err)
 		return err;
 
-	pwm->polarity = polarity;
+	pwm->state.polarity = polarity;
 
 	return 0;
 }
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index ed65354..f0f0f37 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -97,6 +97,18 @@  enum {
 	PWMF_EXPORTED = 1 << 2,
 };
 
+/*
+ * struct pwm_state - state of a PWM channel
+ * @period: PWM period (in nanoseconds)
+ * @duty_cycle: PWM duty cycle (in nanoseconds)
+ * @polarity: PWM polarity
+ */
+struct pwm_state {
+	unsigned int period;
+	unsigned int duty_cycle;
+	enum pwm_polarity polarity;
+};
+
 /**
  * struct pwm_device - PWM channel object
  * @label: name of the PWM device
@@ -105,10 +117,8 @@  enum {
  * @pwm: global index of the PWM device
  * @chip: PWM chip providing this PWM device
  * @chip_data: chip-private data associated with the PWM device
- * @period: period of the PWM signal (in nanoseconds)
- * @duty_cycle: duty cycle of the PWM signal (in nanoseconds)
- * @polarity: polarity of the PWM signal
  * @args: PWM arguments
+ * @state: curent PWM channel state
  */
 struct pwm_device {
 	const char *label;
@@ -118,13 +128,21 @@  struct pwm_device {
 	struct pwm_chip *chip;
 	void *chip_data;
 
-	unsigned int period;
-	unsigned int duty_cycle;
-	enum pwm_polarity polarity;
-
 	struct pwm_args args;
+	struct pwm_state state;
 };
 
+/**
+ * pwm_get_state() - retrieve the current PWM state
+ * @pwm: PWM device
+ * @state: state to fill with the current PWM state
+ */
+static inline void pwm_get_state(const struct pwm_device *pwm,
+				 struct pwm_state *state)
+{
+	*state = pwm->state;
+}
+
 static inline bool pwm_is_enabled(const struct pwm_device *pwm)
 {
 	return test_bit(PWMF_ENABLED, &pwm->flags);
@@ -133,23 +151,31 @@  static inline bool pwm_is_enabled(const struct pwm_device *pwm)
 static inline void pwm_set_period(struct pwm_device *pwm, unsigned int period)
 {
 	if (pwm)
-		pwm->period = period;
+		pwm->state.period = period;
 }
 
 static inline unsigned int pwm_get_period(const struct pwm_device *pwm)
 {
-	return pwm ? pwm->period : 0;
+	struct pwm_state pstate;
+
+	pwm_get_state(pwm, &pstate);
+
+	return pstate.period;
 }
 
 static inline void pwm_set_duty_cycle(struct pwm_device *pwm, unsigned int duty)
 {
 	if (pwm)
-		pwm->duty_cycle = duty;
+		pwm->state.duty_cycle = duty;
 }
 
 static inline unsigned int pwm_get_duty_cycle(const struct pwm_device *pwm)
 {
-	return pwm ? pwm->duty_cycle : 0;
+	struct pwm_state pstate;
+
+	pwm_get_state(pwm, &pstate);
+
+	return pstate.duty_cycle;
 }
 
 /*
@@ -159,7 +185,11 @@  int pwm_set_polarity(struct pwm_device *pwm, enum pwm_polarity polarity);
 
 static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm)
 {
-	return pwm ? pwm->polarity : PWM_POLARITY_NORMAL;
+	struct pwm_state pstate;
+
+	pwm_get_state(pwm, &pstate);
+
+	return pstate.polarity;
 }
 
 static inline void pwm_get_args(const struct pwm_device *pwm,