| Message ID | 20200405192246.3741784-3-oleksandr.suvorov@toradex.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | None | expand |
On Sun, Apr 05, 2020 at 10:22:42PM +0300, Oleksandr Suvorov wrote: > Add the description of PWM_NOFLAGS flag property. > > Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com> As I already wrote in reply to the v1 series I'd prefer a name for 0 that explicitly handles normal polarity. Best regards Uwe
On Tue, Apr 7, 2020 at 9:17 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > On Sun, Apr 05, 2020 at 10:22:42PM +0300, Oleksandr Suvorov wrote: > > Add the description of PWM_NOFLAGS flag property. > > > > Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com> > > As I already wrote in reply to the v1 series I'd prefer a name for 0 > that explicitly handles normal polarity. Uwe, AFAIU, there is no flag that forces normal polarity, the normal polarity is the default state if there is no flag to invert the polarity is set. '0' value in the bit flags cell really means there are no flags set for the PWM instance. > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ | -- Best regards Oleksandr Suvorov Toradex AG Ebenaustrasse 10 | 6048 Horw | Switzerland | T: +41 41 500 48 00
On Tue, Apr 07, 2020 at 01:51:42PM +0300, Oleksandr Suvorov wrote: > On Tue, Apr 7, 2020 at 9:17 AM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > > > On Sun, Apr 05, 2020 at 10:22:42PM +0300, Oleksandr Suvorov wrote: > > > Add the description of PWM_NOFLAGS flag property. > > > > > > Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com> > > > > As I already wrote in reply to the v1 series I'd prefer a name for 0 > > that explicitly handles normal polarity. > > Uwe, AFAIU, there is no flag that forces normal polarity, the normal polarity > is the default state if there is no flag to invert the polarity is set. Yes, that's the status quo. > '0' value in the bit flags cell really means there are no flags set > for the PWM instance. For me the relevance of giving 0 a name is mostly for human consumption. Currently there is only a single flag encoded in the number in question. But as soon as we add another, say PWM_AUTOSTART we have the following possible settings: PWM_NOFLAGS PWM_POLARITY_INVERTED PWM_AUTOSTART PWM_POLARITY_INVERTED | PWM_AUTOSTART Then for the first two a reader doesn't see if autostart is not in use because the dt author doesn't know this feature (e.g. because autostart is too new) or if they don't want autostart at all. If however we had PWM_POLARITY_NORMAL and PWM_NO_AUTOSTART to complement PWM_POLARITY_INVERTED and PWM_AUTOSTART every flag's setting could be explicit and if there is a device tree that only has PWM_POLARITY_NORMAL it would be obvious that nobody thought enough about autostarting to explicitly mention it. Best regards Uwe
On Tue, Apr 7, 2020 at 2:19 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > On Tue, Apr 07, 2020 at 01:51:42PM +0300, Oleksandr Suvorov wrote: > > On Tue, Apr 7, 2020 at 9:17 AM Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: > > > > > > On Sun, Apr 05, 2020 at 10:22:42PM +0300, Oleksandr Suvorov wrote: > > > > Add the description of PWM_NOFLAGS flag property. > > > > > > > > Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com> > > > > > > As I already wrote in reply to the v1 series I'd prefer a name for 0 > > > that explicitly handles normal polarity. > > > > Uwe, AFAIU, there is no flag that forces normal polarity, the normal polarity > > is the default state if there is no flag to invert the polarity is set. > > Yes, that's the status quo. > > > '0' value in the bit flags cell really means there are no flags set > > for the PWM instance. > > For me the relevance of giving 0 a name is mostly for human consumption. > Currently there is only a single flag encoded in the number in question. > But as soon as we add another, say PWM_AUTOSTART we have the following > possible settings: > > PWM_NOFLAGS > PWM_POLARITY_INVERTED > PWM_AUTOSTART > PWM_POLARITY_INVERTED | PWM_AUTOSTART > > Then for the first two a reader doesn't see if autostart is not in use > because the dt author doesn't know this feature (e.g. because autostart > is too new) or if they don't want autostart at all. > > If however we had PWM_POLARITY_NORMAL and PWM_NO_AUTOSTART to complement > PWM_POLARITY_INVERTED and PWM_AUTOSTART So using this approach, in theory, we'll have several flags that all just equals to 0 (0 << 0, 0 << 1, 0 << 2 ...). What if just describe default states for each flag in the DT documentation? > every flag's setting could be explicit and if there is a device tree that only has > > PWM_POLARITY_NORMAL > > it would be obvious that nobody thought enough about autostarting to > explicitly mention it. If you insist on the flag complement model, I have another suggestion. As the normal polarity is the default state, can we use PWM_NO_POLARITY_INVERTED instead of PWM_POLARITY_NORMAL? It gives us 2 benefits: 1. The name will not interfere with enum PWM_POLARITY_NORMAL in <linux/pmw.h> 2. Each flag complement will be made with the same scheme: PWM_flagA -> PWM_NO_flagA... > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ | -- Best regards Oleksandr Suvorov Toradex AG Ebenaustrasse 10 | 6048 Horw | Switzerland | T: +41 41 500 48 00
diff --git a/Documentation/devicetree/bindings/pwm/pwm.txt b/Documentation/devicetree/bindings/pwm/pwm.txt index 084886bd721e..7f0bbda127c4 100644 --- a/Documentation/devicetree/bindings/pwm/pwm.txt +++ b/Documentation/devicetree/bindings/pwm/pwm.txt @@ -45,7 +45,8 @@ period in nanoseconds. Optionally, the pwm-specifier can encode a number of flags (defined in <dt-bindings/pwm/pwm.h>) in a third cell: -- PWM_POLARITY_INVERTED: invert the PWM signal polarity +- PWM_NOFLAGS: no modification flags. +- PWM_POLARITY_INVERTED: invert the PWM signal polarity. Example with optional PWM specifier for inverse polarity @@ -54,6 +55,14 @@ Example with optional PWM specifier for inverse polarity pwm-names = "backlight"; }; + vibrator { + [...] + pwms = <&pwm1 0 10000000 PWM_POLARITY_INVERTED>, + <&pwm2 0 10000000 PWM_NOFLAGS>; + pwm-names = "enable", "direction"; + [...] + }; + 2) PWM controller nodes -----------------------
Add the description of PWM_NOFLAGS flag property. Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com> --- Changes in v2: - add the example of PWM_NOFLAGS usage; - Use PWM_NOFLAGS instead of PWM_POLARITY_NORMAL to avoid possible conflict with enum pwm_polarity in <linux/pwm.h>. Also, this name reflects the sense of the original value more precisely. Documentation/devicetree/bindings/pwm/pwm.txt | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)