[v3] doc: document the pwm command

Signed-off-by: Emil Kronborg <emil.kronborg@protonmail.com>
---
Changes in v3:
- The help text for 'pwm config' in the examples section is more
  explicit about 20 us and 14 us for the period and duty cycle,
  respectively.
- Fixed 'pwm enable' and 'pwm disable' in the examples section to only
  expect 2 arguments.
- Rebased on top of master.

 doc/usage/cmd/pwm.rst | 80 +++++++++++++++++++++++++++++++++++++++++++
 doc/usage/index.rst   |  1 +
 2 files changed, 81 insertions(+)
 create mode 100644 doc/usage/cmd/pwm.rst


base-commit: 7010f22eba35b2a6b66ba37ce565e566ca08c68f

Hi Quentin,

> On 8/1/24 12:07 PM, Emil Kronborg wrote:
> > [...]
> > +.. SPDX-License-Identifier: GPL-2.0+:
> 
> This isn't a valid identifier (spurious ':') I believe.
> 
> Also, I would highly suggest to change it to GPL-2.0-or-later which is
> the current naming scheme for the same license, c.f.
> https://spdx.org/licenses/
> 
> Otherwise, looking good so with that changed:
> 
> Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
> 
> Thanks!
> Quentin


> On 8/1/24 12:07 PM, Emil Kronborg wrote:
> > [...]
> > +.. SPDX-License-Identifier: GPL-2.0+:
> 
> This isn't a valid identifier (spurious ':') I believe.

Hm, I agree that it looks weird, but I think it is fine. There seems to
be a mix of colon and no colon though.

    $ git grep "SPDX-License-Identifier: GPL-2.0+:" doc/usage/ | wc -l
    48
    $ git grep "SPDX-License-Identifier: GPL-2.0+" doc/usage/ | wc -l
    114

With that said, GPL-2.0-or-later is most correct to use, and probably
_without_ a trailing colon. In fact, GPL-2.0+ is actually deprecated
according to the link you sent.

Should I send a new patch where the license is fixed?

On 01.08.24 12:07, Emil Kronborg wrote:
> Signed-off-by: Emil Kronborg <emil.kronborg@protonmail.com>
> ---
> Changes in v3:
> - The help text for 'pwm config' in the examples section is more
>    explicit about 20 us and 14 us for the period and duty cycle,
>    respectively.
> - Fixed 'pwm enable' and 'pwm disable' in the examples section to only
>    expect 2 arguments.
> - Rebased on top of master.
> 
>   doc/usage/cmd/pwm.rst | 80 +++++++++++++++++++++++++++++++++++++++++++
>   doc/usage/index.rst   |  1 +
>   2 files changed, 81 insertions(+)
>   create mode 100644 doc/usage/cmd/pwm.rst
> 
> diff --git a/doc/usage/cmd/pwm.rst b/doc/usage/cmd/pwm.rst
> new file mode 100644
> index 000000000000..bd1255d82186
> --- /dev/null
> +++ b/doc/usage/cmd/pwm.rst
> @@ -0,0 +1,80 @@
> +.. SPDX-License-Identifier: GPL-2.0+:
> +
> +.. index::
> +   single: pwm (command)
> +
> +pwm command
> +===========
> +
> +Synopsis
> +--------
> +
> +::
> +
> +    pwm invert <pwm_dev_num> <channel> <polarity> - invert polarity
> +    pwm config <pwm_dev_num> <channel> <period_ns> <duty_ns> - config PWM
> +    pwm enable <pwm_dev_num> <channel> - enable PWM output
> +    pwm disable <pwm_dev_num> <channel> - disable PWM output

Thank you for documenting the command.

To stay consistent with other pages, please, remove the descriptions 
here. They are available below.

> +
> +
> +Description
> +-----------
> +
> +The ``pwm`` command is used to access and configure PWM (Pulse Width Modulation)
> +signals. For all subcommands, the following arguments are common:
> +
> +pwm_dev_num
> +    device number
> +
> +channel
> +    channel on chosen device
> +
> +pwm invert
> +----------
> +
> +The signal's active and inactive states are swapped.

This would imply that all invocations irrespective of the polarity value 
swap the active and inactive state.

I guess we could say here:

The polarity of the signal is set.

If the value of `polarity` is 0, the default polarity is used.
If the value of `polarity` is 1, the polarity is inverted.

> +
> +pwm config
> +----------
> +
> +Configure the period and duty cycle in nanoseconds.

According to https://en.wikipedia.org/wiki/Duty_cycle "duty cycle" is 
the ratio of "pulse active time" and "total period". This implies that 
it is dimensionless.

%s/duty cycle/pulse width/ or "duty period" as in pwm.h.

> +
> +pwm enable
> +----------
> +
> +Enable output on the configured device and channel.

What happens if the device-channel combination is not configured yet?

> +
> +pwm disable
> +-----------
> +
> +Disable output on the configured device and channel.



Please, describe the parameters here:

pwm_dev_num
    Device number of the pulse width modulation device

channel
    Output channel of the PWM device

polarity
    * 0 - use normal polarity
    * 1 - use inverted polarity

duty_ns
    pulse width in ns

period_ns
    cycle time in ns


> +
> +Examples
> +--------
> +
> +Configure device 0 channel 0 to 20 us period and 14 us (that is, 70%) duty cycle::

Please, add comma, use 'µs', and don't misuse 'duty cycle':

"Configure device 0, channel 0 to 20 µs period time and 14 µs (that is, 
70%) pulse width::"

Best regards

Heinrich

> +
> +    => pwm config 0 0 20000 14000
> +
> +Enable output on the configured device and channel::
> +
> +    => pwm enable 0 0
> +
> +Disable output on the configured device and channel::
> +
> +    => pwm disable 0 0
> +
> +Invert the signal on the configured device and channel::
> +
> +    => pwm invert 0 0 1
> +
> +Configuration
> +-------------
> +
> +The ``pwm`` command is only available if CONFIG_CMD_PWM=y.
> +
> +Return value
> +------------
> +
> +If the command succeeds, the return value ``$?`` is set to 0. If an error occurs, the
> +return value ``$?`` is set to 1.
> diff --git a/doc/usage/index.rst b/doc/usage/index.rst
> index 49b354e6ffd2..1adab5283950 100644
> --- a/doc/usage/index.rst
> +++ b/doc/usage/index.rst
> @@ -92,6 +92,7 @@ Shell commands
>      cmd/pinmux
>      cmd/printenv
>      cmd/pstore
> +   cmd/pwm
>      cmd/qfw
>      cmd/read
>      cmd/reset
> 
> base-commit: 7010f22eba35b2a6b66ba37ce565e566ca08c68f

On Fri, Aug 02, 2024 at 17:28 GMT, Heinrich Schuchardt wrote:
> On 01.08.24 12:07, Emil Kronborg wrote:
> > [...]
> > +Synopsis
> > +--------
> > +
> > +::
> > +
> > +    pwm invert <pwm_dev_num> <channel> <polarity> - invert polarity
> > +    pwm config <pwm_dev_num> <channel> <period_ns> <duty_ns> - config PWM
> > +    pwm enable <pwm_dev_num> <channel> - enable PWM output
> > +    pwm disable <pwm_dev_num> <channel> - disable PWM output
> 
> To stay consistent with other pages, please, remove the descriptions
> here. They are available below.
> 

Hm, all other documented commands in /doc/usage/cmd have a Synopsis
section except for the write command, which refers to the read command.
Wouldn't this break the consistency?

> > +pwm invert
> > +----------
> > +
> > +The signal's active and inactive states are swapped.
> 
> This would imply that all invocations irrespective of the polarity value
> swap the active and inactive state.
> 
> I guess we could say here:
> 
> The polarity of the signal is set.
> 
> If the value of `polarity` is 0, the default polarity is used.
> If the value of `polarity` is 1, the polarity is inverted.

Your suggestion is more precise. I will include it in the next revision.

> > +pwm config
> > +----------
> > +
> > +Configure the period and duty cycle in nanoseconds.
> 
> According to https://en.wikipedia.org/wiki/Duty_cycle "duty cycle" is
> the ratio of "pulse active time" and "total period". This implies that
> it is dimensionless.
> 
> %s/duty cycle/pulse width/ or "duty period" as in pwm.h.

That's a good point. I will also include this change.

> > +
> > +pwm enable
> > +----------
> > +
> > +Enable output on the configured device and channel.
> 
> What happens if the device-channel combination is not configured yet?

The device is enabled with whatever period and duty period are currently
set. My guess is that the default reset values of the SoC are used.

    => pwm enable 0 0
    => echo $?
    0
    => pwm config 0 0 20000 14000
    => pwm enable 0 0
    => echo $?
    0

> > +
> > +pwm disable
> > +-----------
> > +
> > +Disable output on the configured device and channel.
> 
> 
> 
> Please, describe the parameters here:
> 
> pwm_dev_num
>     Device number of the pulse width modulation device
> 
> channel
>     Output channel of the PWM device
> 
> polarity
>     * 0 - use normal polarity
>     * 1 - use inverted polarity
> 
> duty_ns
>     pulse width in ns
> 
> period_ns
>     cycle time in ns
> 

The common arguments, namely pwm_dev_num and channel, are already
described in the Description section. The parameters polarity, duty_ns,
and period_ns are detailed within the command descriptions. Including
them again here might be redundant.

It seems like you want some specific layout. Is there some described
command I can use as reference? Or is it documented somewhere how to
describe a command properly? I used some of the commonly used commands
as reference, but even those are documented differently.

> > +
> > +Examples
> > +--------
> > +
> > +Configure device 0 channel 0 to 20 us period and 14 us (that is, 70%) duty cycle::
> 
> Please, add comma, use 'µs', and don't misuse 'duty cycle':
> 
> "Configure device 0, channel 0 to 20 µs period time and 14 µs (that is,
> 70%) pulse width::"
> 

I wasn't sure if it was allowed to use characters like 'µ', but it's
good to know that it is allowed; I think it looks better.

Thanks for reviewing.

On 05.08.24 14:48, Emil Kronborg wrote:
> On Fri, Aug 02, 2024 at 17:28 GMT, Heinrich Schuchardt wrote:
>> On 01.08.24 12:07, Emil Kronborg wrote:
>>> [...]
>>> +Synopsis
>>> +--------
>>> +
>>> +::
>>> +
>>> +    pwm invert <pwm_dev_num> <channel> <polarity> - invert polarity
>>> +    pwm config <pwm_dev_num> <channel> <period_ns> <duty_ns> - config PWM
>>> +    pwm enable <pwm_dev_num> <channel> - enable PWM output
>>> +    pwm disable <pwm_dev_num> <channel> - disable PWM output
>>
>> To stay consistent with other pages, please, remove the descriptions
>> here. They are available below.
>>
> 
> Hm, all other documented commands in /doc/usage/cmd have a Synopsis
> section except for the write command, which refers to the read command.
> Wouldn't this break the consistency?

I meant, please, remove " - invert polarity", etc.

Best regards

Heinrich

> 
>>> +pwm invert
>>> +----------
>>> +
>>> +The signal's active and inactive states are swapped.
>>
>> This would imply that all invocations irrespective of the polarity value
>> swap the active and inactive state.
>>
>> I guess we could say here:
>>
>> The polarity of the signal is set.
>>
>> If the value of `polarity` is 0, the default polarity is used.
>> If the value of `polarity` is 1, the polarity is inverted.
> 
> Your suggestion is more precise. I will include it in the next revision.
> 
>>> +pwm config
>>> +----------
>>> +
>>> +Configure the period and duty cycle in nanoseconds.
>>
>> According to https://en.wikipedia.org/wiki/Duty_cycle "duty cycle" is
>> the ratio of "pulse active time" and "total period". This implies that
>> it is dimensionless.
>>
>> %s/duty cycle/pulse width/ or "duty period" as in pwm.h.
> 
> That's a good point. I will also include this change.
> 
>>> +
>>> +pwm enable
>>> +----------
>>> +
>>> +Enable output on the configured device and channel.
>>
>> What happens if the device-channel combination is not configured yet?
> 
> The device is enabled with whatever period and duty period are currently
> set. My guess is that the default reset values of the SoC are used.
> 
>      => pwm enable 0 0
>      => echo $?
>      0
>      => pwm config 0 0 20000 14000
>      => pwm enable 0 0
>      => echo $?
>      0
> 
>>> +
>>> +pwm disable
>>> +-----------
>>> +
>>> +Disable output on the configured device and channel.
>>
>>
>>
>> Please, describe the parameters here:
>>
>> pwm_dev_num
>>      Device number of the pulse width modulation device
>>
>> channel
>>      Output channel of the PWM device
>>
>> polarity
>>      * 0 - use normal polarity
>>      * 1 - use inverted polarity
>>
>> duty_ns
>>      pulse width in ns
>>
>> period_ns
>>      cycle time in ns
>>
> 
> The common arguments, namely pwm_dev_num and channel, are already
> described in the Description section. The parameters polarity, duty_ns,
> and period_ns are detailed within the command descriptions. Including
> them again here might be redundant.
> 
> It seems like you want some specific layout. Is there some described
> command I can use as reference? Or is it documented somewhere how to
> describe a command properly? I used some of the commonly used commands
> as reference, but even those are documented differently.
> 
>>> +
>>> +Examples
>>> +--------
>>> +
>>> +Configure device 0 channel 0 to 20 us period and 14 us (that is, 70%) duty cycle::
>>
>> Please, add comma, use 'µs', and don't misuse 'duty cycle':
>>
>> "Configure device 0, channel 0 to 20 µs period time and 14 µs (that is,
>> 70%) pulse width::"
>>
> 
> I wasn't sure if it was allowed to use characters like 'µ', but it's
> good to know that it is allowed; I think it looks better.
> 
> Thanks for reviewing.
>