Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1264077 > unrolled thread

Re: [PATCH 08/10] pwm: core: add pulse feature to the PWM framework

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 08/10] pwm: core: add pulse feature to the PWM framework Thierry Reding <thierry.reding@gmail.com> - 2015-11-06 16:20 +0100
    Re: [PATCH 08/10] pwm: core: add pulse feature to the PWM framework Olliver Schinagl <o.schinagl@ultimaker.com> - 2015-11-06 16:50 +0100
      Re: [PATCH 08/10] pwm: core: add pulse feature to the PWM framework Thierry Reding <thierry.reding@gmail.com> - 2015-11-06 17:10 +0100
        Re: [PATCH 08/10] pwm: core: add pulse feature to the PWM framework Olliver Schinagl <o.schinagl@ultimaker.com> - 2015-11-06 17:20 +0100

#1264077 — Re: [PATCH 08/10] pwm: core: add pulse feature to the PWM framework

[Multipart message — attachments visible in raw view] — view raw

On Mon, Oct 26, 2015 at 10:32:39PM +0100, Olliver Schinagl wrote:
> From: Olliver Schinagl <oliver@schinagl.nl>
> 
> Some hardware PWM's have the possibility to only send out one (or more)
> pulses. This can be quite a useful feature in case one wants or needs
> only a single pulse, but at the exact width.
> 
> Additionally, if multiple pulses are possible, outputting a fixed amount
> of pulses can be useful for various timing specific purposes.

I see how theoretically this would be nice to have. But I'm reluctant to
merge this feature if there aren't any users. What drivers in the kernel
would want to use this feature? Are there new drivers being worked on
that will need this?

> A few new functions have been expanded or added for this new behavior.
> 
> * pwm_config()	now takes an additional parameter to setup the number of
> 		pulses to output. The driver may force this to 0 or 1
> 		for if example if this feature is not or only partially
> 		supported

This is problematic because you need to atomically update all drivers
and users (the kbuild robot already told you that you didn't do this).
To make things easier I suggest you wait with this change until the
atomic PWM patches have been merged, at which point it should become a
lot easier to deal with this kind of extension.

> * pwm_[sg]et_pulse_count()	get or set the number of pulses the pwm
> 				framework is configured for
> * pwm_get_pulse_count_max()	get the maximum number of pulses the pwm
> 				driver supports
> * pwm_pulse()		Tell the PWM to emit a pre-configured number of pulses

Isn't this essentially the same as pwm_enable()? I'd think that if the
PWM is configured to output pulses, then pwm_enable() would simply do
what it's been configured to do (emit the pulses). Why the need for an
additional function?

> * pwm_pulse_done()	an internal function for drivers to call when
> 			they have completed their pre-configured number
> 			of pulses
> * pwm_is_pulsing()	tells the callers if the pwm is busy pulsing,
> 			yielding a little more information than just
> 			pwm_is_enabled()

Similarily, I'd think that once the PWM is done executing the series of
pulses that it was configured for it would be automatically disabled. A
consumer could then simply use pwm_is_enabled() and drivers could call
pwm_disable() on their PWM to mark them as disabled when they're done
pulsing.

> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
>  drivers/pwm/core.c      | 30 +++++++++++++++++++----
>  drivers/pwm/pwm-gpio.c  |  3 ++-
>  drivers/pwm/pwm-sun4i.c |  3 ++-
>  drivers/pwm/sysfs.c     | 58 ++++++++++++++++++++++++++++++++++++++++++--
>  include/linux/pwm.h     | 64 ++++++++++++++++++++++++++++++++++++++++++++++---
>  5 files changed, 147 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 3f9df3e..e2c1c0a 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -432,22 +432,29 @@ EXPORT_SYMBOL_GPL(pwm_free);
>   * @pwm: PWM device
>   * @duty_ns: "on" time (in nanoseconds)
>   * @period_ns: duration (in nanoseconds) of one cycle
> + * @pulse_count: number of pulses (periods) to output on pwm_pulse
>   *
>   * Returns: 0 on success or a negative error code on failure.
>   */
> -int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
> +int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns,
> +	       unsigned int pulse_count)

Like I said, this is problematic because every driver and every consumer
now needs to be aware of pulsing. Once the PWM atomic patches are merged
this will become easier to do because the pulse configuration would be a
part of the atomic state, and hence can be conveniently ignored by users
and driver alike.

Thierry

[toc] | [next] | [standalone]

#1264106

Hey Thierry,

On 06-11-15 16:18, Thierry Reding wrote:
> On Mon, Oct 26, 2015 at 10:32:39PM +0100, Olliver Schinagl wrote:
>> From: Olliver Schinagl <oliver@schinagl.nl>
>>
>> Some hardware PWM's have the possibility to only send out one (or more)
>> pulses. This can be quite a useful feature in case one wants or needs
>> only a single pulse, but at the exact width.
>>
>> Additionally, if multiple pulses are possible, outputting a fixed amount
>> of pulses can be useful for various timing specific purposes.
> I see how theoretically this would be nice to have. But I'm reluctant to
> merge this feature if there aren't any users. What drivers in the kernel
> would want to use this feature? Are there new drivers being worked on
> that will need this?
I should have brought this up as to why I added this, I'm working on a 
stepper driver framework (inspired by the pwm framework actually) and 
rotating moters by x degree's you do by sending pulses, using controlled 
pulses (timing wise) you can precisely move stepper motors. Yes we can 
do this reasonably accurate in software, but doing it in hardware is so 
much nicer.
>
>> A few new functions have been expanded or added for this new behavior.
>>
>> * pwm_config()	now takes an additional parameter to setup the number of
>> 		pulses to output. The driver may force this to 0 or 1
>> 		for if example if this feature is not or only partially
>> 		supported
> This is problematic because you need to atomically update all drivers
> and users (the kbuild robot already told you that you didn't do this).
> To make things easier I suggest you wait with this change until the
> atomic PWM patches have been merged, at which point it should become a
> lot easier to deal with this kind of extension.
yes, I think i mentioned this in the cover letter, I wanted to get your 
input whilst waiting for Boris's patches. So I deffinatly want to 
combine it then, just getting some head work started :)
>
>> * pwm_[sg]et_pulse_count()	get or set the number of pulses the pwm
>> 				framework is configured for
>> * pwm_get_pulse_count_max()	get the maximum number of pulses the pwm
>> 				driver supports
>> * pwm_pulse()		Tell the PWM to emit a pre-configured number of pulses
> Isn't this essentially the same as pwm_enable()? I'd think that if the
> PWM is configured to output pulses, then pwm_enable() would simply do
> what it's been configured to do (emit the pulses). Why the need for an
> additional function?
pwm_pulse() should be dropped, I think I accidentally left that in the 
documentation, sorry.
>
>> * pwm_pulse_done()	an internal function for drivers to call when
>> 			they have completed their pre-configured number
>> 			of pulses
>> * pwm_is_pulsing()	tells the callers if the pwm is busy pulsing,
>> 			yielding a little more information than just
>> 			pwm_is_enabled()
> Similarily, I'd think that once the PWM is done executing the series of
> pulses that it was configured for it would be automatically disabled. A
> consumer could then simply use pwm_is_enabled() and drivers could call
> pwm_disable() on their PWM to mark them as disabled when they're done
> pulsing.
I agree, pulseating can be dropped too as we know that a) the pulse flag 
is set, b) we are enabled. But I'm not sure now if the flag is exported 
to sysfs, in any case, sysfs should just check the pulseating flag?
>
>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>> ---
>>   drivers/pwm/core.c      | 30 +++++++++++++++++++----
>>   drivers/pwm/pwm-gpio.c  |  3 ++-
>>   drivers/pwm/pwm-sun4i.c |  3 ++-
>>   drivers/pwm/sysfs.c     | 58 ++++++++++++++++++++++++++++++++++++++++++--
>>   include/linux/pwm.h     | 64 ++++++++++++++++++++++++++++++++++++++++++++++---
>>   5 files changed, 147 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
>> index 3f9df3e..e2c1c0a 100644
>> --- a/drivers/pwm/core.c
>> +++ b/drivers/pwm/core.c
>> @@ -432,22 +432,29 @@ EXPORT_SYMBOL_GPL(pwm_free);
>>    * @pwm: PWM device
>>    * @duty_ns: "on" time (in nanoseconds)
>>    * @period_ns: duration (in nanoseconds) of one cycle
>> + * @pulse_count: number of pulses (periods) to output on pwm_pulse
>>    *
>>    * Returns: 0 on success or a negative error code on failure.
>>    */
>> -int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
>> +int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns,
>> +	       unsigned int pulse_count)
> Like I said, this is problematic because every driver and every consumer
> now needs to be aware of pulsing. Once the PWM atomic patches are merged
> this will become easier to do because the pulse configuration would be a
> part of the atomic state, and hence can be conveniently ignored by users
> and driver alike.
I agree :) I'll take your initial comments and work with those so far in 
cleaning stuff up. Feel free to get back to me about the validity of the 
pwm_pulse for steppers generally
> Thierry

-- 
Met vriendelijke groeten, Kind regards, 与亲切的问候

Olliver Schinagl
Software Engineer
Research & Development
Ultimaker B.V.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1264119

[Multipart message — attachments visible in raw view] — view raw

On Fri, Nov 06, 2015 at 04:46:54PM +0100, Olliver Schinagl wrote:
> Hey Thierry,
> 
> On 06-11-15 16:18, Thierry Reding wrote:
> >On Mon, Oct 26, 2015 at 10:32:39PM +0100, Olliver Schinagl wrote:
> >>From: Olliver Schinagl <oliver@schinagl.nl>
> >>
> >>Some hardware PWM's have the possibility to only send out one (or more)
> >>pulses. This can be quite a useful feature in case one wants or needs
> >>only a single pulse, but at the exact width.
> >>
> >>Additionally, if multiple pulses are possible, outputting a fixed amount
> >>of pulses can be useful for various timing specific purposes.
> >I see how theoretically this would be nice to have. But I'm reluctant to
> >merge this feature if there aren't any users. What drivers in the kernel
> >would want to use this feature? Are there new drivers being worked on
> >that will need this?
> I should have brought this up as to why I added this, I'm working on a
> stepper driver framework (inspired by the pwm framework actually) and
> rotating moters by x degree's you do by sending pulses, using controlled
> pulses (timing wise) you can precisely move stepper motors. Yes we can do
> this reasonably accurate in software, but doing it in hardware is so much
> nicer.

So is this going to be a kernel framework for stepper motors? If you say
you rotate the motors by sending pulses, doesn't that mean that the PWM
framework with pulse support would be enough? Or are there dedicated
stepper chips that you plan to support, with PWM + pulses being the
fallback for when you don't have one of those chips?

> >>* pwm_pulse_done()	an internal function for drivers to call when
> >>			they have completed their pre-configured number
> >>			of pulses
> >>* pwm_is_pulsing()	tells the callers if the pwm is busy pulsing,
> >>			yielding a little more information than just
> >>			pwm_is_enabled()
> >Similarily, I'd think that once the PWM is done executing the series of
> >pulses that it was configured for it would be automatically disabled. A
> >consumer could then simply use pwm_is_enabled() and drivers could call
> >pwm_disable() on their PWM to mark them as disabled when they're done
> >pulsing.
> I agree, pulseating can be dropped too as we know that a) the pulse flag is
> set, b) we are enabled. But I'm not sure now if the flag is exported to
> sysfs, in any case, sysfs should just check the pulseating flag?

Can't you derive that information simply by looking at the enable and
pulses attributes? If enable == 1 and pulses > 0 you know the PWM is
pulsing. If enable == 1 and pulses == 0 you know it's in regular mode.

Thierry

[toc] | [prev] | [next] | [standalone]

#1264131

Hey Thierry,

On 06-11-15 17:05, Thierry Reding wrote:
> On Fri, Nov 06, 2015 at 04:46:54PM +0100, Olliver Schinagl wrote:
>> Hey Thierry,
>>
>> On 06-11-15 16:18, Thierry Reding wrote:
>>> On Mon, Oct 26, 2015 at 10:32:39PM +0100, Olliver Schinagl wrote:
>>>> From: Olliver Schinagl <oliver@schinagl.nl>
>>>>
>>>> Some hardware PWM's have the possibility to only send out one (or more)
>>>> pulses. This can be quite a useful feature in case one wants or needs
>>>> only a single pulse, but at the exact width.
>>>>
>>>> Additionally, if multiple pulses are possible, outputting a fixed amount
>>>> of pulses can be useful for various timing specific purposes.
>>> I see how theoretically this would be nice to have. But I'm reluctant to
>>> merge this feature if there aren't any users. What drivers in the kernel
>>> would want to use this feature? Are there new drivers being worked on
>>> that will need this?
>> I should have brought this up as to why I added this, I'm working on a
>> stepper driver framework (inspired by the pwm framework actually) and
>> rotating moters by x degree's you do by sending pulses, using controlled
>> pulses (timing wise) you can precisely move stepper motors. Yes we can do
>> this reasonably accurate in software, but doing it in hardware is so much
>> nicer.
> So is this going to be a kernel framework for stepper motors? If you say
> you rotate the motors by sending pulses, doesn't that mean that the PWM
> framework with pulse support would be enough? Or are there dedicated
> stepper chips that you plan to support, with PWM + pulses being the
> fallback for when you don't have one of those chips?
Well I'll have to investigate more into what other chips do, but 
generally speaking from what I know so far, that often you supply a 
stepper driver chip a variable voltage (via a regular pwm) to setup the 
current control, you have gpio's for direction, enable etc, and you 
'pulse' for each step you want the motor to take. There are of course 
some chips that have more logic, that work via i2c and spi interfaces.
>
>>>> * pwm_pulse_done()	an internal function for drivers to call when
>>>> 			they have completed their pre-configured number
>>>> 			of pulses
>>>> * pwm_is_pulsing()	tells the callers if the pwm is busy pulsing,
>>>> 			yielding a little more information than just
>>>> 			pwm_is_enabled()
>>> Similarily, I'd think that once the PWM is done executing the series of
>>> pulses that it was configured for it would be automatically disabled. A
>>> consumer could then simply use pwm_is_enabled() and drivers could call
>>> pwm_disable() on their PWM to mark them as disabled when they're done
>>> pulsing.
>> I agree, pulseating can be dropped too as we know that a) the pulse flag is
>> set, b) we are enabled. But I'm not sure now if the flag is exported to
>> sysfs, in any case, sysfs should just check the pulseating flag?
> Can't you derive that information simply by looking at the enable and
> pulses attributes? If enable == 1 and pulses > 0 you know the PWM is
> pulsing. If enable == 1 and pulses == 0 you know it's in regular mode.
oh right, yes you can :)

olliver
>
> Thierry

-- 
Met vriendelijke groeten, Kind regards, 与亲切的问候

Olliver Schinagl
Software Engineer
Research & Development
Ultimaker B.V.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web