[PATCH v2 00/16] extend PWM framework to support PWM modes

Discussion:

Claudiu Beznea

2018-01-12 14:22:47 UTC

Hi all,

Please give feedback on these patches which extends the PWM framework in order
to support multiple PWM modes of operations. This series is a rework of [1].

The current patch series add the following PWM modes:
- PWM mode normal
- PWM mode complementary
- PWM mode push-pull

Normal mode - for PWM chips with one output per PWM channel; output
waveforms looks like this:
__ __ __ __
PWM __| |__| |__| |__| |__
<--T-->

Complementary mode - for PWM chips with more than one output per PWM
channel; output waveforms for a PWM controller with 2 outputs per PWM
channel may looks line this:
__ __ __ __
PWMH1 __| |__| |__| |__| |__
__ __ __ __ __
PWML1 |__| |__| |__| |__|
<--T-->

Where T is the signal period.

Push-pull mode - for PWM chips with mode than one output per PWM channel;
output waveform for a PWM controller with 2 outputs per PWM channel, in
push-pull mode, with normal polarity looks like this:
__ __
PWMH __| |________| |________
__ __
PWML ________| |________| |__
<--T-->

If polarity is inversed:
__ ________ ________
PWMH |__| |__|
________ ________ __
PWML |__| |__|
<--T-->

Where T is the signal period.

Every PWM device from a PWM chip could be configured in the modes registered
by PWM chip. For this, the PWM chip structure contains a field called caps which
keeps the PWM modes. At probe time the PWM chip registers the supported modes
and PWM devices could switch to the modes registered by PWM chips. For the moment,
in my opinion, it is not neccessary to add a new hook in pwm_ops to go through
the driver to check the capabilities of a single PWM device (as was proposed
in [1]).

I choosed to consider that a PWM controller with 2 outputs to also be capable to
work in normal mode, since the outputs could be connected independently to other
devices.

If the drivers doesn't register any PWM capabilities the default capabilities
will be used (currently, these includes PWM mode normal).

These PWM mode could be configured via sysfs, or pwm_set_mode() +
pwm_apply_state() (for driver clients), or via DT.

In sysfs, user could check the PWM controller capabilities by reading modes file
of PWM chip:
***@sama5d2-xplained:/sys/class/pwm/pwmchip0# ls -l
total 0
lrwxrwxrwx 1 root root 0 Oct 7 03:18 device -> ../../../f802c000.pwm
--w------- 1 root root 4096 Oct 7 03:18 export
-r--r--r-- 1 root root 4096 Oct 7 03:18 modes
-r--r--r-- 1 root root 4096 Oct 7 03:18 npwm
drwxr-xr-x 2 root root 0 Oct 7 03:18 power
lrwxrwxrwx 1 root root 0 Oct 7 03:18 subsystem -> ../../../../../../../class/pwm
-rw-r--r-- 1 root root 4096 Oct 7 01:20 uevent
--w------- 1 root root 4096 Oct 7 03:18 unexport
***@sama5d2-xplained:/sys/class/pwm/pwmchip0# cat modes
normal complementary push-pull

And the current capability of the PWM device could be checked as follows:
***@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm1# ls -l
-r--r--r-- 1 root root 4096 Feb 9 10:54 capture
-rw-r--r-- 1 root root 4096 Feb 9 10:54 duty_cycle
-rw-r--r-- 1 root root 4096 Feb 9 10:54 enable
-rw-r--r-- 1 root root 4096 Feb 9 10:54 mode
-rw-r--r-- 1 root root 4096 Feb 9 10:54 period
-rw-r--r-- 1 root root 4096 Feb 9 10:55 polarity
drwxr-xr-x 2 root root 0 Feb 9 10:54 power
-rw-r--r-- 1 root root 4096 Feb 9 10:54 uevent

***@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm1# cat mode
normal

The PWM push-pull mode could be usefull in applications like
half bridge converters.

This series also add support for PWM modes on atmel SAMA5D2 SoC.

Thank you,
Claudiu Beznea

[1] https://www.spinics.net/lists/arm-kernel/msg580275.html

Changes in v2:
- remove of_xlate and of_pwm_n_cells and use generic functions to pharse DT
inputs; this is done in patches 1, 2, 3, 4, 5, 6, 7 of this series; this will
make easy the addition of PWM mode support from DT
- add PWM mode normal
- register PWM modes as capabilities of PWM chips at driver probe and, in case
driver doesn't provide these capabilities use default ones
- change the way PWM mode is pharsed via DT by using a new input for pwms
binding property

Claudiu Beznea (16):
drivers: pwm: core: use a single of xlate function
pwm: pxa: update documentation regarding pwm-cells
pwm: cros-ec: update documentation regarding pwm-cells
pwm: clps711x: update documentation regarding pwm-cells
ARM: dts: clps711x: update pwm-cells
ARM: dts: pxa: update pwm-cells
arm64: dts: rockchip: update pwm-cells
drivers: pwm: core: extend PWM framework with PWM modes
drivers: pwm: core: add PWM mode to pwm_config()
pwm: Add PWM modes
pwm: add documentation for PWM modes
pwm: atmel: add pwm capabilities
drivers: pwm: core: add push-pull mode support
pwm: add push-pull mode
pwm: add documentation for pwm push-pull mode
pwm: atmel: add push-pull mode support

.../bindings/pwm/cirrus,clps711x-pwm.txt | 4 +-
.../devicetree/bindings/pwm/google,cros-ec-pwm.txt | 4 +-
Documentation/devicetree/bindings/pwm/pwm.txt | 23 ++++-
Documentation/devicetree/bindings/pwm/pxa-pwm.txt | 6 +-
Documentation/pwm.txt | 47 ++++++++-
arch/arm/boot/dts/ep7209.dtsi | 2 +-
arch/arm/boot/dts/pxa25x.dtsi | 4 +-
arch/arm/boot/dts/pxa27x.dtsi | 8 +-
arch/arm/boot/dts/pxa3xx.dtsi | 8 +-
arch/arm/mach-s3c24xx/mach-rx1950.c | 5 +-
arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts | 2 +-
arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi | 2 +-
drivers/bus/ts-nbus.c | 2 +-
drivers/clk/clk-pwm.c | 3 +-
drivers/gpu/drm/i915/intel_panel.c | 8 +-
drivers/hwmon/pwm-fan.c | 2 +-
drivers/input/misc/max77693-haptic.c | 2 +-
drivers/input/misc/max8997_haptic.c | 3 +-
drivers/leds/leds-pwm.c | 2 +-
drivers/media/rc/ir-rx51.c | 2 +-
drivers/media/rc/pwm-ir-tx.c | 2 +-
drivers/pwm/core.c | 112 ++++++++++++++-------
drivers/pwm/pwm-atmel-hlcdc.c | 2 -
drivers/pwm/pwm-atmel-tcb.c | 2 -
drivers/pwm/pwm-atmel.c | 100 ++++++++++++------
drivers/pwm/pwm-bcm-iproc.c | 2 -
drivers/pwm/pwm-bcm-kona.c | 2 -
drivers/pwm/pwm-bcm2835.c | 2 -
drivers/pwm/pwm-berlin.c | 2 -
drivers/pwm/pwm-clps711x.c | 11 --
drivers/pwm/pwm-cros-ec.c | 20 ----
drivers/pwm/pwm-fsl-ftm.c | 2 -
drivers/pwm/pwm-hibvt.c | 2 -
drivers/pwm/pwm-imx.c | 8 --
drivers/pwm/pwm-lpc18xx-sct.c | 2 -
drivers/pwm/pwm-meson.c | 2 -
drivers/pwm/pwm-omap-dmtimer.c | 2 -
drivers/pwm/pwm-pxa.c | 19 ----
drivers/pwm/pwm-renesas-tpu.c | 2 -
drivers/pwm/pwm-rockchip.c | 5 -
drivers/pwm/pwm-samsung.c | 3 -
drivers/pwm/pwm-sun4i.c | 2 -
drivers/pwm/pwm-tiecap.c | 2 -
drivers/pwm/pwm-tiehrpwm.c | 2 -
drivers/pwm/pwm-vt8500.c | 2 -
drivers/pwm/pwm-zx.c | 2 -
drivers/pwm/sysfs.c | 64 ++++++++++++
drivers/video/backlight/lm3630a_bl.c | 2 +-
drivers/video/backlight/lp855x_bl.c | 2 +-
drivers/video/backlight/lp8788_bl.c | 2 +-
drivers/video/backlight/pwm_bl.c | 4 +-
drivers/video/fbdev/ssd1307fb.c | 3 +-
include/dt-bindings/pwm/pwm.h | 4 +
include/linux/pwm.h | 90 +++++++++++++++--
54 files changed, 402 insertions(+), 222 deletions(-)

--
2.7.4

Neil Armstrong

2018-01-16 09:07:05 UTC

Permalink

Remove of_pwm_simple_xlate() and of_pwm_xlate_with_flags() functions
and add of_pwm_xlate() which is used in all cases no mather if the OF
bindings are with PWM flags or not. This should not affect the old
behavior since the xlate will be based on #pwm-cells property of the
PWM controller. Based on #pwm-cells property the xlate will consider
the flags or not. This will permit the addition of other inputs to OF
xlate by just adding proper code at the end of of_pwm_xlate() and a new
input to enum pwm_args_xlate_options. With this changes there will be
no need to fill of_xlate and of_pwm_n_cells of struct pwm_chip from
the drivers probe methods. References in drives to references to of_xlate
and of_pwm_n_cells were removed. Drivers which used private of_xlate
functions switched to the generic of_pwm_xlate() function which fits
for it but with little changes in device trees (these drivers translated
differently the "pwms" bindings; the "pwms" bindings now are generic to
all drivers and all drivers should provide them in the format described
in pwm documentation).
---
This patch (and the next 7) could be applied independetly by this series, if
any, but I choosed to have it here since it makes easy the PWM modes parsing.
If you feel it could be independently of this series I could send a new version.
Also, Thierry, Mike, Brian, Shiyan, please take an extra look over pwm-pxa.c,
pwm-cros-ec.c and pwm-clps711x.c since these were moved to use the generic
pwms (minimum 2 pwm-cells).
drivers/pwm/core.c | 56 +++++++++++-------------------------------
drivers/pwm/pwm-atmel-hlcdc.c | 2 --
drivers/pwm/pwm-atmel-tcb.c | 2 --
drivers/pwm/pwm-atmel.c | 6 -----
drivers/pwm/pwm-bcm-iproc.c | 2 --
drivers/pwm/pwm-bcm-kona.c | 2 --
drivers/pwm/pwm-bcm2835.c | 2 --
drivers/pwm/pwm-berlin.c | 2 --
drivers/pwm/pwm-clps711x.c | 11 ---------
drivers/pwm/pwm-cros-ec.c | 20 ---------------
drivers/pwm/pwm-fsl-ftm.c | 2 --
drivers/pwm/pwm-hibvt.c | 2 --
drivers/pwm/pwm-imx.c | 8 ------
drivers/pwm/pwm-lpc18xx-sct.c | 2 --
drivers/pwm/pwm-meson.c | 2 --
drivers/pwm/pwm-omap-dmtimer.c | 2 --
drivers/pwm/pwm-pxa.c | 19 --------------
drivers/pwm/pwm-renesas-tpu.c | 2 --
drivers/pwm/pwm-rockchip.c | 5 ----
drivers/pwm/pwm-samsung.c | 3 ---
drivers/pwm/pwm-sun4i.c | 2 --
drivers/pwm/pwm-tiecap.c | 2 --
drivers/pwm/pwm-tiehrpwm.c | 2 --
drivers/pwm/pwm-vt8500.c | 2 --
drivers/pwm/pwm-zx.c | 2 --
include/linux/pwm.h | 23 ++++++++++-------
26 files changed, 29 insertions(+), 156 deletions(-)

[...]

diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index 0767deba8e62..6702fcc203ce 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -535,8 +535,6 @@ static int meson_pwm_probe(struct platform_device *pdev)
meson->chip.ops = &meson_pwm_ops;
meson->chip.base = -1;
meson->chip.npwm = 2;
- meson->chip.of_xlate = of_pwm_xlate_with_flags;
- meson->chip.of_pwm_n_cells = 3;
meson->data = of_device_get_match_data(&pdev->dev);
meson->inverter_mask = BIT(meson->chip.npwm) - 1;
diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
index 5ad42f33e70c..6bd32ae6dd3e 100644
--- a/drivers/pwm/pwm-omap-dmtimer.c
+++ b/drivers/pwm/pwm-omap-dmtimer.c
@@ -317,8 +317,6 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
omap->chip.ops = &pwm_omap_dmtimer_ops;
omap->chip.base = -1;
omap->chip.npwm = 1;
- omap->chip.of_xlate = of_pwm_xlate_with_flags;
- omap->chip.of_pwm_n_cells = 3;
mutex_init(&omap->mutex);

[...]

Hi Claudiu,

Please avoid changing the device tree plumbing, this of_xlate fields are needed since
some platforms will need to have their own translate functions.

Moving a to single mandatory xlate function is going backward, a possible move
would be to default to of_pwm_xlate_with_flags and 3 cells if of_xlate is NULL,
or use the platforms xlate if provided.

Moving to a single xlate function is unrelated to your needs and can break some platforms.

Neil

Claudiu Beznea

2018-01-16 09:33:08 UTC

Permalink

Post by Neil Armstrong

[...]

[...]
Hi Claudiu,
Please avoid changing the device tree plumbing, this of_xlate fields are needed since
some platforms will need to have their own translate functions.
Moving a to single mandatory xlate function is going backward, a possible move
would be to default to of_pwm_xlate_with_flags and 3 cells if of_xlate is NULL,
or use the platforms xlate if provided.
Moving to a single xlate function is unrelated to your needs and can break some platforms.

Hi Neil,

Thank you for your feedback. Looking over the drivers I saw that only 3 drivers
are using private of_xlate functions, the rest of them uses of_pwm_xlate_with_flags
and of_pwm_simple_xlate function combined with pwm-cells binding and of_pwm_n_cells
(which I think is redundant with pwm-cells). I though that switching to a common
of_xlate to xlate everything based on pwm-cells binding value would be better
and remove redundant code.

But if this is a downward direction I agree to drop the changes related to of_xlate.

Thanks,
Claudiu

Post by Neil Armstrong
Neil

Claudiu Beznea

2018-01-12 14:22:49 UTC

Permalink

pwm-cells should be at least 2 to provide channel number and period value.

Cc: Mike Dunn <***@newsguy.com>
Signed-off-by: Claudiu Beznea <***@microchip.com>
---
Documentation/devicetree/bindings/pwm/pxa-pwm.txt | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
index 5ae9f1e3c338..a0e20edeeebc 100644
--- a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
+++ b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
@@ -10,7 +10,7 @@ Required properties:
Note that one device instance must be created for each PWM that is used, so the
length covers only the register window for one PWM output, not that of the
entire PWM controller. Currently length is 0x10 for all supported devices.
-- #pwm-cells: Should be 1. This cell is used to specify the period in
+- #pwm-cells: Should be 2. This cell is used to specify the period in
nanoseconds.

Example PWM device node:
@@ -18,13 +18,13 @@ Example PWM device node:
pwm0: ***@40b00000 {
compatible = "marvell,pxa250-pwm";
reg = <0x40b00000 0x10>;
- #pwm-cells = <1>;
+ #pwm-cells = <2>;
};

Example PWM client node:

backlight {
compatible = "pwm-backlight";
- pwms = <&pwm0 5000000>;
+ pwms = <&pwm0 0 5000000>;
...
}

--
2.7.4

Claudiu Beznea

2018-01-22 08:47:13 UTC

Permalink

Post by Claudiu Beznea
pwm-cells should be at least 2 to provide channel number and period value.
---
Documentation/devicetree/bindings/pwm/pxa-pwm.txt | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
index 5ae9f1e3c338..a0e20edeeebc 100644
--- a/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
+++ b/Documentation/devicetree/bindings/pwm/pxa-pwm.txt
Note that one device instance must be created for each PWM that is used, so the
length covers only the register window for one PWM output, not that of the
entire PWM controller. Currently length is 0x10 for all supported devices.
-- #pwm-cells: Should be 1. This cell is used to specify the period in
+- #pwm-cells: Should be 2. This cell is used to specify the period in
nanoseconds.

What's the new cell? channel? Does the PXA PWM have more than one
channel? If not, then you shouldn't add a cell.

The new cell had to be period, to have a generic OF function in the kernel,
to parse the pwms bindings for all PWMs, something like
pwms=<pwm channel-number pwm-period>. After several discussions on this
series, I found that old DT binaries must be compatible with latest kernel
version and this series doesn't guarantee this. So I will drop this change
in next version.

Thank you,
Claudiu Beznea

Rob

Claudiu Beznea

2018-01-12 14:22:52 UTC

Permalink

Update pwm-cells to 2 to allow initialization of channel number an period.

Cc: Alexander Shiyan <***@mail.ru>
Signed-off-by: Claudiu Beznea <***@microchip.com>
---
arch/arm/boot/dts/ep7209.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/ep7209.dtsi b/arch/arm/boot/dts/ep7209.dtsi
index aaf1261d2ee4..fdfe6ab31569 100644
--- a/arch/arm/boot/dts/ep7209.dtsi
+++ b/arch/arm/boot/dts/ep7209.dtsi
@@ -133,7 +133,7 @@
compatible = "cirrus,ep7209-pwm";
reg = <0x80000400 0x4>;
clocks = <&clks CLPS711X_CLK_PWM>;
- #pwm-cells = <1>;
+ #pwm-cells = <2>;
};

uart1: ***@80000480 {

--
2.7.4

Claudiu Beznea

2018-01-12 14:22:51 UTC

Permalink

pwm-cells should be at least 2 to provide channel number and period value.

Cc: Alexander Shiyan <***@mail.ru>
Signed-off-by: Claudiu Beznea <***@microchip.com>
---
Documentation/devicetree/bindings/pwm/cirrus,clps711x-pwm.txt | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/cirrus,clps711x-pwm.txt b/Documentation/devicetree/bindings/pwm/cirrus,clps711x-pwm.txt
index c0b2028238d6..57f480a872e3 100644
--- a/Documentation/devicetree/bindings/pwm/cirrus,clps711x-pwm.txt
+++ b/Documentation/devicetree/bindings/pwm/cirrus,clps711x-pwm.txt
@@ -4,12 +4,12 @@ Required properties:
- compatible: Shall contain "cirrus,ep7209-pwm".
- reg: Physical base address and length of the controller's registers.
- clocks: phandle + clock specifier pair of the PWM reference clock.
-- #pwm-cells: Should be 1. The cell specifies the index of the channel.
+- #pwm-cells: Should be 2. The cell specifies the index of the channel.

Example:
pwm: ***@80000400 {
compatible = "cirrus,ep7312-pwm", "cirrus,ep7209-pwm";
reg = <0x80000400 0x4>;
clocks = <&clks 8>;
- #pwm-cells = <1>;
+ #pwm-cells = <2>;
};

--
2.7.4

Claudiu Beznea

2018-01-12 14:22:50 UTC

Permalink

pwm-cells should be at least 2 to provide channel number and period value.

Cc: Brian Norris <***@chromium.org>
Signed-off-by: Claudiu Beznea <***@microchip.com>
---
Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt b/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt
index 472bd46ab5a4..03347fd302b5 100644
--- a/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt
+++ b/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt
@@ -8,7 +8,7 @@ Documentation/devicetree/bindings/mfd/cros-ec.txt).

Required properties:
- compatible: Must contain "google,cros-ec-pwm"
-- #pwm-cells: Should be 1. The cell specifies the PWM index.
+- #pwm-cells: Should be 2. The cell specifies the PWM index.

Example:
cros-***@0 {
@@ -18,6 +18,6 @@ Example:

cros_ec_pwm: ec-pwm {
compatible = "google,cros-ec-pwm";
- #pwm-cells = <1>;
+ #pwm-cells = <2>;
};
};

--
2.7.4

Brian Norris

2018-01-12 18:31:23 UTC

Permalink

Post by Claudiu Beznea
pwm-cells should be at least 2 to provide channel number and period value.

Nacked-by: Brian Norris <***@chromium.org>

We don't control the period from the kernel; only the duty cycle. (Now,
that's perhaps not a wise firmware interface, and we may fix that
someday, but you can't just declare a breaking change to a documented,
reviewed binding.)

Post by Claudiu Beznea
---
Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt b/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt
index 472bd46ab5a4..03347fd302b5 100644
--- a/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt
+++ b/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt
@@ -8,7 +8,7 @@ Documentation/devicetree/bindings/mfd/cros-ec.txt).
- compatible: Must contain "google,cros-ec-pwm"
-- #pwm-cells: Should be 1. The cell specifies the PWM index.
+- #pwm-cells: Should be 2. The cell specifies the PWM index.

Umm, "2 cells", but you use the singular "cell", and don't document what
the second one is? That's nonsense.

Brian

Post by Claudiu Beznea
cros_ec_pwm: ec-pwm {
compatible = "google,cros-ec-pwm";
- #pwm-cells = <1>;
+ #pwm-cells = <2>;
};
};
--
2.7.4

Claudiu Beznea

2018-01-15 09:01:34 UTC

Permalink

Post by Brian Norris

Post by Claudiu Beznea
pwm-cells should be at least 2 to provide channel number and period value.

We don't control the period from the kernel; only the duty cycle.

I agree, I saw this in the driver. This is the way I put the 0xffff
period in the patch 7 of this series. I though that since all the drivers
which uses PWM framework uses the generic PWM bindings (except pwm-pxa.c,
pwm-cros-ec.c and pwm-clps711x.c) I though it would be simpler (from the
driver's perspective and also from core's perspective) to have generic
bindings for all as follows:
pwms = <&controller PWM-channel PWM-period PWM-flags>;

To allow pwm-cross-ec.c to use this generic binding, since it is uses a
fix period and of_pwm_xlate() xlate DT arguments without taking care of
the cross-ec particularity, using 0xffff period in the pwms binding will
not harm this driver (correct me if I'm wrong). For this, the pwm-cells
argument need to be increased at 2. In patch 7 of this series I used
pwms = <&cros_ec_pwm 1 65535>;
which initialize the PWM 1 with 0xffff period.

Thanks,
Claudiu

(Now,

Post by Brian Norris
that's perhaps not a wise firmware interface, and we may fix that
someday, but you can't just declare a breaking change to a documented,
reviewed binding.

Umm, "2 cells", but you use the singular "cell", and don't document what
the second one is? That's nonsense.
Brian

Post by Claudiu Beznea
cros_ec_pwm: ec-pwm {
compatible = "google,cros-ec-pwm";
- #pwm-cells = <1>;
+ #pwm-cells = <2>;
};
};
--
2.7.4

Claudiu Beznea

2018-01-18 09:18:02 UTC

Permalink

With these changes, if pwm-cells=1 then only PWM-channel will be parsed,

I'm not sure if I'm understanding you correctly but...no. If cells is 1,
then your driver change just causes us not to parse correctly, and
everything fails.

My bad, agree with you, will fail with pwm-cells=1. I forgot about:
+ if (args->args_count < PWM_ARGS_CNT_XLATE_PERIOD ||
+ args->args_count > PWM_ARGS_CNT_XLATE_MAX)
return ERR_PTR(-EINVAL);
restriction.

if it is 2 PWM-channel and PWM-period will be parsed, if pwm-cells=3
then PWM-channel, PWM-period and PWM-flags will be parsed.
In your driver you used to have only one cell because you wanted to allow
user to give as argument only PWM channel, and you did not want a change
of PWM period (and in of_xlate function you initialize pwm period with 0xffff
value: this is why I changed the binding in patch 7 of this series, file

It's not a matter of "allow", it's a matter of description. The period
isn't actually even 0xffff, that's just a pseudo-period, to reflect that
you have a choice of duty cycles of 0 to 0xffff. I (justifiably, I
think) didn't think putting this false value in the device tree was
accurate.

Ok, I didn't investigate the driver to see what is truly set in HW.

rk3399-gru-kevin.dts). But e.g. sysfs could try to change the PWM period,
there is no restriction to change the PWM period from sysfs, in the sysfs
interface but the restriction is in PWM apply of the drive. The same things
happens with these changes too. The user could introduce any PWM period via
DT but the pwm apply function of the driver will return error.

sysfs has no bearing on a device tree binding. Just because we have a
broken interface here doesn't mean we should change how we describe the
hardware.

Based on [1] and the comments I will drop the first 7 patches of this series.

Thanks,
Claudiu

[1] https://www.kernel.org/doc/Documentation/devicetree/bindings/ABI.txt

Brian

Claudiu Beznea

2018-01-12 14:22:56 UTC

Permalink

Add PWM mode to pwm_config() function. The drivers which uses pwm_config()
were adapted to this change.

Signed-off-by: Claudiu Beznea <***@microchip.com>
---
arch/arm/mach-s3c24xx/mach-rx1950.c | 5 +++--
drivers/bus/ts-nbus.c | 2 +-
drivers/clk/clk-pwm.c | 3 ++-
drivers/gpu/drm/i915/intel_panel.c | 8 +++++---
drivers/hwmon/pwm-fan.c | 2 +-
drivers/input/misc/max77693-haptic.c | 2 +-
drivers/input/misc/max8997_haptic.c | 3 ++-
drivers/leds/leds-pwm.c | 2 +-
drivers/media/rc/ir-rx51.c | 2 +-
drivers/media/rc/pwm-ir-tx.c | 2 +-
drivers/video/backlight/lm3630a_bl.c | 2 +-
drivers/video/backlight/lp855x_bl.c | 2 +-
drivers/video/backlight/lp8788_bl.c | 2 +-
drivers/video/backlight/pwm_bl.c | 4 ++--
drivers/video/fbdev/ssd1307fb.c | 3 ++-
include/linux/pwm.h | 4 +++-
16 files changed, 28 insertions(+), 20 deletions(-)

diff --git a/arch/arm/mach-s3c24xx/mach-rx1950.c b/arch/arm/mach-s3c24xx/mach-rx1950.c
index e86ad6a68a0b..9ee0ed9ff37e 100644
--- a/arch/arm/mach-s3c24xx/mach-rx1950.c
+++ b/arch/arm/mach-s3c24xx/mach-rx1950.c
@@ -433,14 +433,15 @@ static void rx1950_lcd_power(int enable)

/* GPB1->OUTPUT, GPB1->0 */
gpio_direction_output(S3C2410_GPB(1), 0);
- pwm_config(lcd_pwm, 0, LCD_PWM_PERIOD);
+ pwm_config(lcd_pwm, 0, LCD_PWM_PERIOD, PWM_MODE_NORMAL);
pwm_disable(lcd_pwm);

/* GPC0->0, GPC10->0 */
gpio_direction_output(S3C2410_GPC(0), 0);
gpio_direction_output(S3C2410_GPC(10), 0);
} else {
- pwm_config(lcd_pwm, LCD_PWM_DUTY, LCD_PWM_PERIOD);
+ pwm_config(lcd_pwm, LCD_PWM_DUTY, LCD_PWM_PERIOD,
+ PWM_MODE_NORMAL);
pwm_enable(lcd_pwm);

gpio_direction_output(S3C2410_GPC(0), 1);
diff --git a/drivers/bus/ts-nbus.c b/drivers/bus/ts-nbus.c
index 073fd9011154..dcd2ca3bcd99 100644
--- a/drivers/bus/ts-nbus.c
+++ b/drivers/bus/ts-nbus.c
@@ -316,7 +316,7 @@ static int ts_nbus_probe(struct platform_device *pdev)
* the atomic PWM API.
*/
pwm_apply_args(pwm);
- ret = pwm_config(pwm, pargs.period, pargs.period);
+ ret = pwm_config(pwm, pargs.period, pargs.period, pargs.mode);
if (ret < 0)
return ret;

diff --git a/drivers/clk/clk-pwm.c b/drivers/clk/clk-pwm.c
index 8cb9d117fdbf..605a6bffe893 100644
--- a/drivers/clk/clk-pwm.c
+++ b/drivers/clk/clk-pwm.c
@@ -92,7 +92,8 @@ static int clk_pwm_probe(struct platform_device *pdev)
* atomic PWM API.
*/
pwm_apply_args(pwm);
- ret = pwm_config(pwm, (pargs.period + 1) >> 1, pargs.period);
+ ret = pwm_config(pwm, (pargs.period + 1) >> 1, pargs.period,
+ pargs.mode);
if (ret < 0)
return ret;

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index adc51e452e3e..1ea93ebd3e56 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -634,7 +634,8 @@ static void pwm_set_backlight(const struct drm_connector_state *conn_state, u32
struct intel_panel *panel = &to_intel_connector(conn_state->connector)->panel;
int duty_ns = DIV_ROUND_UP(level * CRC_PMIC_PWM_PERIOD_NS, 100);

- pwm_config(panel->backlight.pwm, duty_ns, CRC_PMIC_PWM_PERIOD_NS);
+ pwm_config(panel->backlight.pwm, duty_ns, CRC_PMIC_PWM_PERIOD_NS,
+ PWM_MODE_NORMAL);
}

static void
@@ -823,7 +824,8 @@ static void pwm_disable_backlight(const struct drm_connector_state *old_conn_sta
struct intel_panel *panel = &connector->panel;

/* Disable the backlight */
- pwm_config(panel->backlight.pwm, 0, CRC_PMIC_PWM_PERIOD_NS);
+ pwm_config(panel->backlight.pwm, 0, CRC_PMIC_PWM_PERIOD_NS,
+ PWM_MODE_NORMAL);
usleep_range(2000, 3000);
pwm_disable(panel->backlight.pwm);
}
@@ -1771,7 +1773,7 @@ static int pwm_setup_backlight(struct intel_connector *connector,
pwm_apply_args(panel->backlight.pwm);

retval = pwm_config(panel->backlight.pwm, CRC_PMIC_PWM_PERIOD_NS,
- CRC_PMIC_PWM_PERIOD_NS);
+ CRC_PMIC_PWM_PERIOD_NS, PWM_MODE_NORMAL);
if (retval < 0) {
DRM_ERROR("Failed to configure the pwm chip\n");
pwm_put(panel->backlight.pwm);
diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index 70cc0d134f3c..bd05cd81d3d5 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -308,7 +308,7 @@ static int pwm_fan_resume(struct device *dev)

pwm_get_args(ctx->pwm, &pargs);
duty = DIV_ROUND_UP(ctx->pwm_value * (pargs.period - 1), MAX_PWM);
- ret = pwm_config(ctx->pwm, duty, pargs.period);
+ ret = pwm_config(ctx->pwm, duty, pargs.period, pargs.mode);
if (ret)
return ret;
return pwm_enable(ctx->pwm);
diff --git a/drivers/input/misc/max77693-haptic.c b/drivers/input/misc/max77693-haptic.c
index 46b0f48fbf49..5fe2ff2b408b 100644
--- a/drivers/input/misc/max77693-haptic.c
+++ b/drivers/input/misc/max77693-haptic.c
@@ -76,7 +76,7 @@ static int max77693_haptic_set_duty_cycle(struct max77693_haptic *haptic)

pwm_get_args(haptic->pwm_dev, &pargs);
delta = (pargs.period + haptic->pwm_duty) / 2;
- error = pwm_config(haptic->pwm_dev, delta, pargs.period);
+ error = pwm_config(haptic->pwm_dev, delta, pargs.period, pargs.mode);
if (error) {
dev_err(haptic->dev, "failed to configure pwm: %d\n", error);
return error;
diff --git a/drivers/input/misc/max8997_haptic.c b/drivers/input/misc/max8997_haptic.c
index 99bc762881d5..c16be1e410c6 100644
--- a/drivers/input/misc/max8997_haptic.c
+++ b/drivers/input/misc/max8997_haptic.c
@@ -73,7 +73,8 @@ static int max8997_haptic_set_duty_cycle(struct max8997_haptic *chip)

if (chip->mode == MAX8997_EXTERNAL_MODE) {
unsigned int duty = chip->pwm_period * chip->level / 100;
- ret = pwm_config(chip->pwm, duty, chip->pwm_period);
+ ret = pwm_config(chip->pwm, duty, chip->pwm_period,
+ PWM_MODE_NORMAL);
} else {
int i;
u8 duty_index = 0;
diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index 8d456dc6c5bf..4d7a55db046a 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -40,7 +40,7 @@ static void __led_pwm_set(struct led_pwm_data *led_dat)
{
int new_duty = led_dat->duty;

- pwm_config(led_dat->pwm, new_duty, led_dat->period);
+ pwm_config(led_dat->pwm, new_duty, led_dat->period, PWM_MODE_NORMAL);

if (new_duty == 0)
pwm_disable(led_dat->pwm);
diff --git a/drivers/media/rc/ir-rx51.c b/drivers/media/rc/ir-rx51.c
index 49265f02e772..0667aa9f1566 100644
--- a/drivers/media/rc/ir-rx51.c
+++ b/drivers/media/rc/ir-rx51.c
@@ -58,7 +58,7 @@ static int init_timing_params(struct ir_rx51 *ir_rx51)

duty = DIV_ROUND_CLOSEST(ir_rx51->duty_cycle * period, 100);

- pwm_config(pwm, duty, period);
+ pwm_config(pwm, duty, period, PWM_MODE_NORMAL);

return 0;
}
diff --git a/drivers/media/rc/pwm-ir-tx.c b/drivers/media/rc/pwm-ir-tx.c
index 27d0f5837a76..4829b09f3a0b 100644
--- a/drivers/media/rc/pwm-ir-tx.c
+++ b/drivers/media/rc/pwm-ir-tx.c
@@ -68,7 +68,7 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier);
duty = DIV_ROUND_CLOSEST(pwm_ir->duty_cycle * period, 100);

- pwm_config(pwm, duty, period);
+ pwm_config(pwm, duty, period, PWM_MODE_NORMAL);

edge = ktime_get();

diff --git a/drivers/video/backlight/lm3630a_bl.c b/drivers/video/backlight/lm3630a_bl.c
index 2030a6b77a09..9992aa9c4cf5 100644
--- a/drivers/video/backlight/lm3630a_bl.c
+++ b/drivers/video/backlight/lm3630a_bl.c
@@ -166,7 +166,7 @@ static void lm3630a_pwm_ctrl(struct lm3630a_chip *pchip, int br, int br_max)
unsigned int period = pchip->pdata->pwm_period;
unsigned int duty = br * period / br_max;

- pwm_config(pchip->pwmd, duty, period);
+ pwm_config(pchip->pwmd, duty, period, PWM_MODE_NORMAL);
if (duty)
pwm_enable(pchip->pwmd);
else
diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c
index 939f057836e1..018be55d762c 100644
--- a/drivers/video/backlight/lp855x_bl.c
+++ b/drivers/video/backlight/lp855x_bl.c
@@ -256,7 +256,7 @@ static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
pwm_apply_args(pwm);
}

- pwm_config(lp->pwm, duty, period);
+ pwm_config(lp->pwm, duty, period, PWM_MODE_NORMAL);
if (duty)
pwm_enable(lp->pwm);
else
diff --git a/drivers/video/backlight/lp8788_bl.c b/drivers/video/backlight/lp8788_bl.c
index cf869ec90cce..cb49f34a7f2e 100644
--- a/drivers/video/backlight/lp8788_bl.c
+++ b/drivers/video/backlight/lp8788_bl.c
@@ -153,7 +153,7 @@ static void lp8788_pwm_ctrl(struct lp8788_bl *bl, int br, int max_br)
pwm_apply_args(pwm);
}

- pwm_config(bl->pwm, duty, period);
+ pwm_config(bl->pwm, duty, period, PWM_MODE_NORMAL);
if (duty)
pwm_enable(bl->pwm);
else
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 1c2289ddd555..ec5215c4c937 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -66,7 +66,7 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb)
if (!pb->enabled)
return;

- pwm_config(pb->pwm, 0, pb->period);
+ pwm_config(pb->pwm, 0, pb->period, PWM_MODE_NORMAL);
pwm_disable(pb->pwm);

if (pb->enable_gpio)
@@ -108,7 +108,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)

if (brightness > 0) {
duty_cycle = compute_duty_cycle(pb, brightness);
- pwm_config(pb->pwm, duty_cycle, pb->period);
+ pwm_config(pb->pwm, duty_cycle, pb->period, PWM_MODE_NORMAL);
pwm_backlight_power_on(pb, brightness);
} else
pwm_backlight_power_off(pb);
diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
index f599520374dd..9cf17721e6e2 100644
--- a/drivers/video/fbdev/ssd1307fb.c
+++ b/drivers/video/fbdev/ssd1307fb.c
@@ -308,7 +308,8 @@ static int ssd1307fb_init(struct ssd1307fb_par *par)

par->pwm_period = pargs.period;
/* Enable the PWM */
- pwm_config(par->pwm, par->pwm_period / 2, par->pwm_period);
+ pwm_config(par->pwm, par->pwm_period / 2, par->pwm_period,
+ PWM_MODE_NORMAL);
pwm_enable(par->pwm);

dev_dbg(&par->client->dev, "Using PWM%d with a %dns period.\n",
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 0fdc680651aa..2e8dfc3ea516 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -375,11 +375,12 @@ int pwm_adjust_config(struct pwm_device *pwm);
* @pwm: PWM device
* @duty_ns: "on" time (in nanoseconds)
* @period_ns: duration (in nanoseconds) of one cycle
+ * @mode: PWM mode
*
* Returns: 0 on success or a negative error code on failure.
*/
static inline int pwm_config(struct pwm_device *pwm, int duty_ns,
- int period_ns)
+ int period_ns, enum pwm_mode mode)
{
struct pwm_state state;

@@ -395,6 +396,7 @@ static inline int pwm_config(struct pwm_device *pwm, int duty_ns,

state.duty_cycle = duty_ns;
state.period = period_ns;
+ state.mode = mode;
return pwm_apply_state(pwm, &state);
}

--
2.7.4

Claudiu Beznea

2018-01-12 14:22:57 UTC

Permalink

Define a macros for PWM modes to be used by device tree sources.

Signed-off-by: Claudiu Beznea <***@microchip.com>
---
include/dt-bindings/pwm/pwm.h | 3 +++
1 file changed, 3 insertions(+)

diff --git a/include/dt-bindings/pwm/pwm.h b/include/dt-bindings/pwm/pwm.h
index ab9a077e3c7d..b8617431f8ec 100644
--- a/include/dt-bindings/pwm/pwm.h
+++ b/include/dt-bindings/pwm/pwm.h
@@ -12,4 +12,7 @@

#define PWM_POLARITY_INVERTED (1 << 0)

+#define PWM_DTMODE_NORMAL (1 << 0)
+#define PWM_DTMODE_COMPLEMENTARY (1 << 1)
+
#endif

--
2.7.4

Claudiu Beznea

2018-01-22 08:54:07 UTC

Permalink

Post by Claudiu Beznea
Define a macros for PWM modes to be used by device tree sources.
---
include/dt-bindings/pwm/pwm.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/include/dt-bindings/pwm/pwm.h b/include/dt-bindings/pwm/pwm.h
index ab9a077e3c7d..b8617431f8ec 100644
--- a/include/dt-bindings/pwm/pwm.h
+++ b/include/dt-bindings/pwm/pwm.h
@@ -12,4 +12,7 @@
#define PWM_POLARITY_INVERTED (1 << 0)
+#define PWM_DTMODE_NORMAL (1 << 0)

Bit 0 is already taken. I think you mean (0 << 1)?

I wanted to have the PWM modes in a new cell, so that the pwms binding to be
something like:
pwms=<pwm-controller pwm-channel pwm-period pwm-flags pwm-mode>

If you think it is mode feasible to also include PWM mode in the cell for
PWM flags, please let me know.

Personally, I'd just drop this define. A define for a 0 value makes more
sense when each state is equally used (like active high or low), but if
0 is the more common case, then I don't the need for a define.

I want it to have these defines like bit defines:
PWM_DTMODE_NORMAL=0x1
PWM_DTMODE_COMPLEMENTARY=0x2
PWM_DTMODE_PUSH_PULL=0x4

Thank you,
Claudiu Beznea

Post by Claudiu Beznea
+#define PWM_DTMODE_COMPLEMENTARY (1 << 1)
+
#endif
--
2.7.4

Claudiu Beznea

2018-01-23 16:55:30 UTC

Permalink

On Tue, Jan 23, 2018 at 4:40 AM, Claudiu Beznea

On Mon, Jan 22, 2018 at 2:54 AM, Claudiu Beznea

Post by Claudiu Beznea

Bit 0 is already taken. I think you mean (0 << 1)?

I wanted to have the PWM modes in a new cell, so that the pwms binding to be
pwms=<pwm-controller pwm-channel pwm-period pwm-flags pwm-mode>
If you think it is mode feasible to also include PWM mode in the cell for
PWM flags, please let me know.

Yes, but you have to make "normal" be no bit set to be compatible with
everything already out there.

I'm thinking having it separately is more clear and modular.

Having a standard number of cells (and fields in cells) is easier to
maintain. We've set this at 3 for PWMs and you have already found what
happens when you have a different number of cells. Adding a 4th cell
(and possibly a different form of 3 cells in the case of no channel #
cell) just creates more combinations to parse.

Agree with you!

And we don't want to go

update all the existing users using 3 cells to use 4 cells just to
align.

Is this necessary? I mean, the drivers that will use PWM modes could be
updated to use 4 cells or not. For these drivers I created a generic OF
parse function which parse everything. This is currently on my local branch
only (previous approach to have a common parse function for all driver wasn't
well accepted):

struct pwm_device *
of_pwm_xlate_all(struct pwm_chip *pc, const struct of_phandle_args *args)
{
struct pwm_device *pwm;
enum pwm_mode mode;

/* check, whether the driver supports all cells */
if (pc->of_pwm_n_cells < 4)
return ERR_PTR(-EINVAL);

/* flags in the third and fourth cells are optional */
if (args->args_count < 2)
return ERR_PTR(-EINVAL);

if (args->args[0] >= pc->npwm)
return ERR_PTR(-EINVAL);

pwm = pwm_request_from_chip(pc, args->args[0], NULL);
if (IS_ERR(pwm))
return pwm;

pwm->args.period = args->args[1];
pwm->args.polarity = PWM_POLARITY_NORMAL;
pwm->args.mode = BIT(ffs(pwm->chip->caps->modes) - 1);

switch (args->args_count) {
case 3:
mode = BIT(ffs(args->args[3]) - 1);
if (pwm_mode_valid(pwm->chip, mode))
pwm->args.mode = mode;

case 2:
if (args->args[2] & PWM_POLARITY_INVERTED)
pwm->args.polarity = PWM_POLARITY_INVERSED;
break;

default:
break;
}

return pwm;
}

Drivers which uses PWM mode could use this generic parse function.
In case of using old DT binaries, with no PWM mode bindings, and with
- a driver that support PWM modes, the pwm->args will be initialized
with PWM mode normal (the default for every PWM controller).

In case the driver haven't specific support for PWM modes (so it support
only PWM normal mode) there is no need to update to 4 cells. The
of_pwm_xlate_with_flags() or
of_pwm_simple_xlate()
could be used to parse DT bindings (with some little changes to also update
the PWM mode for pwm->args). Currently I have this line to be added, in these
two functions, in next version for this series:

pwm->args.mode = BIT(ffs(pwm->chip->caps->modes) - 1);

I did some tests on my side and it looks ok.

Please correct me if I'm saying something wrong.

If the mode needed to be set in the common case, then I might feel
differently about having a separate cell.

No there is no need to set the mode explicitly. The implicit mode is normal
mode. If other mode needs to be set then the mode cells (or the mode inserted
in the flag cells as you wish) should take care of this.

But these modes seem like a

special case. How many PWM controllers actually support modes like
these?I did a little research on this back in time and seems that is a characteristic

of PWM controller. I'm saying about PWM push-pull mode. Also about PWM controllers
with 2 outputs per PWM channel.

Regarding the other modes I had a discussion with Thierry, back in time, regarding
this (see [1]) where he asked me to add these normal and complementary modes
before adding push-pull mode in order to differentiate b/w PWM controllers with
one output and PWM controllers with 2 outputs per PWM channel so that the PWM
user to be aware of how its PWM channel works.

[1] https://www.spinics.net/lists/arm-kernel/msg580275.html

Post by Claudiu Beznea

PWM_DTMODE_NORMAL=0x1
PWM_DTMODE_COMPLEMENTARY=0x2
PWM_DTMODE_PUSH_PULL=0x4

Thinking about this some more, shouldn't the new modes just be
implied? A client is going to require one of these modes or it won't
work right.

The clients could or could not request the mode via DT. Every PWM chip registers
supported modes at driver probe (default, if no PWM mode support is added
to the PWM chip driver the PWM chip will supports only normal mode). If a
PWM channel is requested by a PWM client via DT, and there is no PWM mode setting
pwms=<pwm-controller pwm-channel pwm-period> or
pwms=<pwm-controller pwm-channel pwm-period pwm-flags>
the first available mode of PWM chip will be used to instantiate the mode.
PWM_DTMODE_NORMAL=0x1
PWM_DTMODE_COMPLEMENTARY=0x2
PWM_DTMODE_PUSH_PULL=0x4
and the PWM chip driver registers itself, at probe, with (PWM_DTMODE_COMPLEMENTARY | PWM_DTMODE_PUSH_PULL)
then the first available mode will be PWM_DTMODE_COMPLEMENTARY (first LSB bit
of the variable that keeps the available modes).

Every driver already supports "normal", so that's implied. It would be
pointless to make every driver register that explicitly.

Agree! The chip registered with COMPLEMENTARY and PUSH-PULL modes was just
an example.

It would be

pretty hard to not support normal as that's just ignore the 2nd
signal.

Also, agree!

Also complementary mode could be accomplished with a single pwm output
and a board level inverter, right?

Yes, I think this could be accomplished. Here I took into account only PWM controller
capabilities. Having this, I think will involve having extra PWM bindings describing
extra capabilities per-channel. And to change a little bit the implementation in order
to have these capabilities per channel nor per PWM controller. What do you think?
I think push-pull mode could also be accomplished having board inverter and delay
modules. So, in these cases make sense to have per channel capabilities, as per my
understanding.

Yes, it certainly is per channel. You may or may not have the 2nd pin
on any given channel.The changes in this series are for PWM controllers. Having for instance push-pull mode

registered for the channel of a PWM controller which has only one output
will also involve some control of the external h/w that is doing push-pull.
For instance, I'm talking about a schema like this:

+---------------+
| | ch0 +-----------+ __ __
| |------>| push-pull |---> _| |________| |____
|PWM controller | | external | __ __
| | | module |---> _______| |_________| |__
| | +-----------+
+---------------+

From my point of view, for this kind of schematic having the PWM channel controller
supporting all the modes (normal, complementary, push-pull) will be useless without
a way to control the push-pull external module (e.g. a GPIO).

Thank you,
Claudiu Beznea

But again, if the client needs one of these

modes, then the h/w must be hooked up correctly to a channel with 2
signals.
Rob

Claudiu Beznea

2018-01-12 14:22:58 UTC

Permalink

Add documentation for PWM normal and complementary modes.

Signed-off-by: Claudiu Beznea <***@microchip.com>
---
Documentation/devicetree/bindings/pwm/pwm.txt | 17 ++++++++++++++--
Documentation/pwm.txt | 29 +++++++++++++++++++++++++--
2 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/pwm.txt b/Documentation/devicetree/bindings/pwm/pwm.txt
index 8556263b8502..fdff25bad1db 100644
--- a/Documentation/devicetree/bindings/pwm/pwm.txt
+++ b/Documentation/devicetree/bindings/pwm/pwm.txt
@@ -43,8 +43,8 @@ because the name "backlight" would be used as fallback anyway.
pwm-specifier typically encodes the chip-relative PWM number and the PWM
period in nanoseconds.

-Optionally, the pwm-specifier can encode a number of flags (defined in
-<dt-bindings/pwm/pwm.h>) in a third cell:
+Optionally, the pwm-specifier can encode:
+1. a number of flags (defined in <dt-bindings/pwm/pwm.h>) in a third cell:
- PWM_POLARITY_INVERTED: invert the PWM signal polarity

Example with optional PWM specifier for inverse polarity
@@ -54,6 +54,19 @@ Example with optional PWM specifier for inverse polarity
pwm-names = "backlight";
};

+2. PWM working modes (defined in <dt-bindings/pwm/pwm.h>) in the 4th cell:
+- PWM_MODE_NORMAL: for all PWM controllers
+- PWM_MODE_COMPLEMENTARY: for PWM controllers with more than one output per
+PWM channel
+
+Example with PWM modes:
+
+ bl: blacklight {
+ pwms = <&pwm 0 5000000 PWM_POLARITY_INVERTED
+ PWM_DTMODE_NORMAL | PWM_DTMODE_COMPLEMENTARY>;
+ pwm-names = "backlight";
+ };
+
2) PWM controller nodes
-----------------------

diff --git a/Documentation/pwm.txt b/Documentation/pwm.txt
index 8fbf0aa3ba2d..58c9bd55f021 100644
--- a/Documentation/pwm.txt
+++ b/Documentation/pwm.txt
@@ -61,8 +61,8 @@ In addition to the PWM state, the PWM API also exposes PWM arguments, which
are the reference PWM config one should use on this PWM.
PWM arguments are usually platform-specific and allows the PWM user to only
care about dutycycle relatively to the full period (like, duty = 50% of the
-period). struct pwm_args contains 2 fields (period and polarity) and should
-be used to set the initial PWM config (usually done in the probe function
+period). struct pwm_args contains 3 fields (period, polarity and mode) and
+should be used to set the initial PWM config (usually done in the probe function
of the PWM user). PWM arguments are retrieved with pwm_get_args().

Using PWMs with the sysfs interface
@@ -83,6 +83,9 @@ will find:
unexport
Unexports a PWM channel from sysfs (write-only).

+ mode
+ PWM chip supported modes.
+
The PWM channels are numbered using a per-chip index from 0 to npwm-1.

When a PWM channel is exported a pwmX directory will be created in the
@@ -110,6 +113,28 @@ channel that was exported. The following properties will then be available:
- 0 - disabled
- 1 - enabled

+ mode
+ Set PWM channel working mode (normal and complementary). PWM chip with
+ complementary mode could also work in normal mode by using only one physical
+ output.
+
+ Normal mode - for PWM chips with one output per PWM channel; output
+ waveforms looks like this:
+ __ __ __ __
+ PWM __| |__| |__| |__| |__
+ <--T-->
+
+ Complementary mode - for PWM chips with more than one output per PWM
+ channel; output waveforms for a PWM controller with 2 outputs per PWM
+ channel looks line this:
+ __ __ __ __
+ PWMH1 __| |__| |__| |__| |__
+ __ __ __ __ __
+ PWML1 |__| |__| |__| |__|
+ <--T-->
+
+ Where T is the signal period.
+
Implementing a PWM driver
-------------------------

--
2.7.4

Claudiu Beznea

2018-01-22 08:55:43 UTC

Permalink

Post by Claudiu Beznea
Add documentation for PWM normal and complementary modes.

This and the previous patch can be combined.

Post by Claudiu Beznea
---
Documentation/devicetree/bindings/pwm/pwm.txt | 17 ++++++++++++++--
Documentation/pwm.txt | 29 +++++++++++++++++++++++++--
2 files changed, 42 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/pwm/pwm.txt b/Documentation/devicetree/bindings/pwm/pwm.txt
index 8556263b8502..fdff25bad1db 100644
--- a/Documentation/devicetree/bindings/pwm/pwm.txt
+++ b/Documentation/devicetree/bindings/pwm/pwm.txt
@@ -43,8 +43,8 @@ because the name "backlight" would be used as fallback anyway.
pwm-specifier typically encodes the chip-relative PWM number and the PWM
period in nanoseconds.
-Optionally, the pwm-specifier can encode a number of flags (defined in

Based on the prior patches, "third cell" is probably to restrictive
here and it should just say "in a flags cell".

OK

Thank you,
Claudiu Beznea

Post by Claudiu Beznea
- PWM_POLARITY_INVERTED: invert the PWM signal polarity
Example with optional PWM specifier for inverse polarity
@@ -54,6 +54,19 @@ Example with optional PWM specifier for inverse polarity
pwm-names = "backlight";
};
+- PWM_MODE_NORMAL: for all PWM controllers
+- PWM_MODE_COMPLEMENTARY: for PWM controllers with more than one output per
+PWM channel
+
+
+ bl: blacklight {
+ pwms = <&pwm 0 5000000 PWM_POLARITY_INVERTED
+ PWM_DTMODE_NORMAL | PWM_DTMODE_COMPLEMENTARY>;
+ pwm-names = "backlight";
+ };
+
2) PWM controller nodes
-----------------------
diff --git a/Documentation/pwm.txt b/Documentation/pwm.txt
index 8fbf0aa3ba2d..58c9bd55f021 100644
--- a/Documentation/pwm.txt
+++ b/Documentation/pwm.txt
@@ -61,8 +61,8 @@ In addition to the PWM state, the PWM API also exposes PWM arguments, which
are the reference PWM config one should use on this PWM.
PWM arguments are usually platform-specific and allows the PWM user to only
care about dutycycle relatively to the full period (like, duty = 50% of the
-period). struct pwm_args contains 2 fields (period and polarity) and should
-be used to set the initial PWM config (usually done in the probe function
+period). struct pwm_args contains 3 fields (period, polarity and mode) and
+should be used to set the initial PWM config (usually done in the probe function
of the PWM user). PWM arguments are retrieved with pwm_get_args().
Using PWMs with the sysfs interface
unexport
Unexports a PWM channel from sysfs (write-only).
+ mode
+ PWM chip supported modes.
+
The PWM channels are numbered using a per-chip index from 0 to npwm-1.
When a PWM channel is exported a pwmX directory will be created in the
- 0 - disabled
- 1 - enabled
+ mode
+ Set PWM channel working mode (normal and complementary). PWM chip with
+ complementary mode could also work in normal mode by using only one physical
+ output.
+
+ Normal mode - for PWM chips with one output per PWM channel; output
+ __ __ __ __
+ PWM __| |__| |__| |__| |__
+ <--T-->
+
+ Complementary mode - for PWM chips with more than one output per PWM
+ channel; output waveforms for a PWM controller with 2 outputs per PWM
+ __ __ __ __
+ PWMH1 __| |__| |__| |__| |__
+ __ __ __ __ __
+ PWML1 |__| |__| |__| |__|
+ <--T-->
+
+ Where T is the signal period.
+
Implementing a PWM driver
-------------------------
--
2.7.4

Claudiu Beznea

2018-01-12 14:22:59 UTC

Permalink

Add pwm capabilities to Atmel PWM controllers.

Signed-off-by: Claudiu Beznea <***@microchip.com>
---
drivers/pwm/pwm-atmel.c | 58 +++++++++++++++++++++++++++++++------------------
1 file changed, 37 insertions(+), 21 deletions(-)

diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index 32427d2b7877..e879d5459b55 100644
--- a/drivers/pwm/pwm-atmel.c
+++ b/drivers/pwm/pwm-atmel.c
@@ -65,6 +65,11 @@ struct atmel_pwm_registers {
u8 duty_upd;
};

+struct atmel_pwm_data {
+ struct atmel_pwm_registers regs;
+ struct pwm_caps caps;
+};
+
struct atmel_pwm_chip {
struct pwm_chip chip;
struct clk *clk;
@@ -277,27 +282,37 @@ static const struct pwm_ops atmel_pwm_ops = {
.owner = THIS_MODULE,
};

-static const struct atmel_pwm_registers atmel_pwm_regs_v1 = {
- .period = PWMV1_CPRD,
- .period_upd = PWMV1_CUPD,
- .duty = PWMV1_CDTY,
- .duty_upd = PWMV1_CUPD,
+static const struct atmel_pwm_data atmel_pwm_data_v1 = {
+ .regs = {
+ .period = PWMV1_CPRD,
+ .period_upd = PWMV1_CUPD,
+ .duty = PWMV1_CDTY,
+ .duty_upd = PWMV1_CUPD,
+ },
+ .caps = {
+ .modes = PWM_MODE_NORMAL,
+ },
};

-static const struct atmel_pwm_registers atmel_pwm_regs_v2 = {
- .period = PWMV2_CPRD,
- .period_upd = PWMV2_CPRDUPD,
- .duty = PWMV2_CDTY,
- .duty_upd = PWMV2_CDTYUPD,
+static const struct atmel_pwm_data atmel_pwm_data_v2 = {
+ .regs = {
+ .period = PWMV2_CPRD,
+ .period_upd = PWMV2_CPRDUPD,
+ .duty = PWMV2_CDTY,
+ .duty_upd = PWMV2_CDTYUPD,
+ },
+ .caps = {
+ .modes = PWM_MODE_NORMAL | PWM_MODE_COMPLEMENTARY,
+ },
};

static const struct platform_device_id atmel_pwm_devtypes[] = {
{
.name = "at91sam9rl-pwm",
- .driver_data = (kernel_ulong_t)&atmel_pwm_regs_v1,
+ .driver_data = (kernel_ulong_t)&atmel_pwm_data_v1,
}, {
.name = "sama5d3-pwm",
- .driver_data = (kernel_ulong_t)&atmel_pwm_regs_v2,
+ .driver_data = (kernel_ulong_t)&atmel_pwm_data_v2,
}, {
/* sentinel */
},
@@ -307,20 +322,20 @@ MODULE_DEVICE_TABLE(platform, atmel_pwm_devtypes);
static const struct of_device_id atmel_pwm_dt_ids[] = {
{
.compatible = "atmel,at91sam9rl-pwm",
- .data = &atmel_pwm_regs_v1,
+ .data = &atmel_pwm_data_v1,
}, {
.compatible = "atmel,sama5d3-pwm",
- .data = &atmel_pwm_regs_v2,
+ .data = &atmel_pwm_data_v2,
}, {
.compatible = "atmel,sama5d2-pwm",
- .data = &atmel_pwm_regs_v2,
+ .data = &atmel_pwm_data_v2,
}, {
/* sentinel */
},
};
MODULE_DEVICE_TABLE(of, atmel_pwm_dt_ids);

-static inline const struct atmel_pwm_registers *
+static inline const struct atmel_pwm_data *
atmel_pwm_get_driver_data(struct platform_device *pdev)
{
const struct platform_device_id *id;
@@ -330,18 +345,18 @@ atmel_pwm_get_driver_data(struct platform_device *pdev)

id = platform_get_device_id(pdev);

- return (struct atmel_pwm_registers *)id->driver_data;
+ return (struct atmel_pwm_data *)id->driver_data;
}

static int atmel_pwm_probe(struct platform_device *pdev)
{
- const struct atmel_pwm_registers *regs;
+ const struct atmel_pwm_data *data;
struct atmel_pwm_chip *atmel_pwm;
struct resource *res;
int ret;

- regs = atmel_pwm_get_driver_data(pdev);
- if (!regs)
+ data = atmel_pwm_get_driver_data(pdev);
+ if (!data)
return -ENODEV;

atmel_pwm = devm_kzalloc(&pdev->dev, sizeof(*atmel_pwm), GFP_KERNEL);
@@ -365,9 +380,10 @@ static int atmel_pwm_probe(struct platform_device *pdev)

atmel_pwm->chip.dev = &pdev->dev;
atmel_pwm->chip.ops = &atmel_pwm_ops;
+ atmel_pwm->chip.caps = &data->caps;
atmel_pwm->chip.base = -1;
atmel_pwm->chip.npwm = 4;
- atmel_pwm->regs = regs;
+ atmel_pwm->regs = &data->regs;
atmel_pwm->updated_pwms = 0;
mutex_init(&atmel_pwm->isr_lock);

--
2.7.4

Claudiu Beznea

2018-01-12 14:23:01 UTC

Permalink

Add macro for push-pull mode to be used in DT.

Signed-off-by: Claudiu Beznea <***@microchip.com>
---
include/dt-bindings/pwm/pwm.h | 1 +
1 file changed, 1 insertion(+)

diff --git a/include/dt-bindings/pwm/pwm.h b/include/dt-bindings/pwm/pwm.h
index b8617431f8ec..c6365807c30d 100644
--- a/include/dt-bindings/pwm/pwm.h
+++ b/include/dt-bindings/pwm/pwm.h
@@ -14,5 +14,6 @@

#define PWM_DTMODE_NORMAL (1 << 0)
#define PWM_DTMODE_COMPLEMENTARY (1 << 1)
+#define PWM_DTMODE_PUSH_PULL (1 << 2)

#endif

--
2.7.4

Claudiu Beznea

2018-01-12 14:23:00 UTC

Permalink

Add push-pull mode support in PWM code. In push-pull mode the channels
outputs has same polarity and the edges are complementary delayed for one
period.

Signed-off-by: Claudiu Beznea <***@microchip.com>
---
drivers/pwm/sysfs.c | 2 ++
include/linux/pwm.h | 9 +++++++--
2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index 7d111ab17e43..5052bdec7ad6 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -249,6 +249,8 @@ static ssize_t mode_store(struct device *child,
mode = PWM_MODE_NORMAL;
else if (sysfs_streq(buf, pwm_get_mode_desc(PWM_MODE_COMPLEMENTARY)))
mode = PWM_MODE_COMPLEMENTARY;
+ else if (sysfs_streq(buf, pwm_get_mode_desc(PWM_MODE_PUSH_PULL)))
+ mode = PWM_MODE_PUSH_PULL;
else
return -EINVAL;

diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 2e8dfc3ea516..a4ad3b7a5317 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -29,13 +29,16 @@ enum pwm_polarity {
* enum pwm_mode - PWM working modes
* PWM_MODE_NORMAL: PWM has one output per channel
* PWM_MODE_COMPLEMENTARY: PWM has 2 outputs per channel with opposite polarity
+ * PWM_MODE_PUSH_PULL: PWM has 2 outputs per channel with same polarity and
+ * the edges are complementary delayed for one period
* PWM_MODE_MAX: Used to get the defined PWM modes mask (PWM_MODE_MAX - 1)
* phase-shifted
*/
enum pwm_mode {
PWM_MODE_NORMAL = BIT(0),
PWM_MODE_COMPLEMENTARY = BIT(1),
- PWM_MODE_MAX = BIT(2),
+ PWM_MODE_PUSH_PULL = BIT(2),
+ PWM_MODE_MAX = BIT(3),
};

/**
@@ -478,7 +481,9 @@ static inline void pwm_disable(struct pwm_device *pwm)

static inline const char * const pwm_get_mode_desc(enum pwm_mode mode)
{
- static const char * const modes[] = { "normal", "complementary" };
+ static const char * const modes[] = {
+ "normal", "complementary", "push-pull"
+ };

return mode ? modes[ffs(mode) - 1] : "invalid";
}

--
2.7.4

Claudiu Beznea

2018-01-12 14:23:02 UTC

Permalink

Add documentation for PWM push-pull mode.

Signed-off-by: Claudiu Beznea <***@microchip.com>
---
Documentation/devicetree/bindings/pwm/pwm.txt | 8 +++++++-
Documentation/pwm.txt | 18 ++++++++++++++++++
2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pwm/pwm.txt b/Documentation/devicetree/bindings/pwm/pwm.txt
index fdff25bad1db..a4562af3577c 100644
--- a/Documentation/devicetree/bindings/pwm/pwm.txt
+++ b/Documentation/devicetree/bindings/pwm/pwm.txt
@@ -58,15 +58,21 @@ Example with optional PWM specifier for inverse polarity
- PWM_MODE_NORMAL: for all PWM controllers
- PWM_MODE_COMPLEMENTARY: for PWM controllers with more than one output per
PWM channel
+- PWM_MODE_PUSH_PULL: for PWM controllers with more than one output per channel,
+in push-pull mode

Example with PWM modes:

bl: blacklight {
pwms = <&pwm 0 5000000 PWM_POLARITY_INVERTED
- PWM_DTMODE_NORMAL | PWM_DTMODE_COMPLEMENTARY>;
+ PWM_DTMODE_NORMAL | PWM_DTMODE_COMPLEMENTARY |
+ PWM_DTMODE_PUSH_PULL>;
pwm-names = "backlight";
};

+If all the available modes are given as argument of pwms binding only the first
+valid one will be considered (first valid LSB bit of mode field).
+
2) PWM controller nodes
-----------------------

diff --git a/Documentation/pwm.txt b/Documentation/pwm.txt
index 58c9bd55f021..71b538239519 100644
--- a/Documentation/pwm.txt
+++ b/Documentation/pwm.txt
@@ -135,6 +135,24 @@ channel that was exported. The following properties will then be available:

Where T is the signal period.

+ Push-pull mode - for PWM chips with mode than one output per PWM channel;
+ output waveform for a PWM controller with 2 outputs per PWM channel, in
+ push-pull mode, with normal polarity looks like this:
+ __ __
+ PWMH __| |________| |________
+ __ __
+ PWML ________| |________| |__
+ <--T-->
+
+ If polarity is inversed:
+ __ ________ ________
+ PWMH |__| |__|
+ ________ ________ __
+ PWML |__| |__|
+ <--T-->
+
+ Where T is the signal period.
+
Implementing a PWM driver
-------------------------

--
2.7.4

Rob Herring

2018-01-19 22:41:14 UTC

Permalink

Post by Claudiu Beznea
Add documentation for PWM push-pull mode.
---
Documentation/devicetree/bindings/pwm/pwm.txt | 8 +++++++-
Documentation/pwm.txt | 18 ++++++++++++++++++
2 files changed, 25 insertions(+), 1 deletion(-)

Other than squashing the previous patch into this one,

Reviewed-by: Rob Herring <***@kernel.org>

Claudiu Beznea

2018-01-12 14:23:03 UTC

Permalink

Add support for PWM push-pull mode. This is only supported by SAMA5D2 SoCs.

Signed-off-by: Claudiu Beznea <***@microchip.com>
---
drivers/pwm/pwm-atmel.c | 38 ++++++++++++++++++++++++++++++++++----
1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index e879d5459b55..0122f51723f8 100644
--- a/drivers/pwm/pwm-atmel.c
+++ b/drivers/pwm/pwm-atmel.c
@@ -33,8 +33,11 @@

#define PWM_CMR 0x0
/* Bit field in CMR */
-#define PWM_CMR_CPOL (1 << 9)
-#define PWM_CMR_UPD_CDTY (1 << 10)
+#define PWM_CMR_CPOL BIT(9)
+#define PWM_CMR_UPD_CDTY BIT(10)
+#define PWM_CMR_DTHI BIT(17)
+#define PWM_CMR_DTLI BIT(18)
+#define PWM_CMR_PPM BIT(19)
#define PWM_CMR_CPRE_MSK 0xF

/* The following registers for PWM v1 */
@@ -228,7 +231,8 @@ static int atmel_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
if (state->enabled) {
if (cstate.enabled &&
cstate.polarity == state->polarity &&
- cstate.period == state->period) {
+ cstate.period == state->period &&
+ cstate.mode == state->mode) {
cprd = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm,
atmel_pwm->regs->period);
atmel_pwm_calculate_cdty(state, cprd, &cdty);
@@ -263,6 +267,18 @@ static int atmel_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
val &= ~PWM_CMR_CPOL;
else
val |= PWM_CMR_CPOL;
+
+ /* PWM mode. */
+ if (chip->caps->modes & PWM_MODE_PUSH_PULL) {
+ if (state->mode == PWM_MODE_PUSH_PULL) {
+ val |= PWM_CMR_PPM | PWM_CMR_DTHI;
+ val &= ~PWM_CMR_DTLI;
+ } else {
+ val &= ~(PWM_CMR_PPM | PWM_CMR_DTLI |
+ PWM_CMR_DTHI);
+ }
+ }
+
atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
atmel_pwm_set_cprd_cdty(chip, pwm, cprd, cdty);
mutex_lock(&atmel_pwm->isr_lock);
@@ -306,6 +322,20 @@ static const struct atmel_pwm_data atmel_pwm_data_v2 = {
},
};

+static const struct atmel_pwm_data atmel_pwm_data_v3 = {
+ .regs = {
+ .period = PWMV2_CPRD,
+ .period_upd = PWMV2_CPRDUPD,
+ .duty = PWMV2_CDTY,
+ .duty_upd = PWMV2_CDTYUPD,
+ },
+ .caps = {
+ .modes = PWM_MODE_NORMAL |
+ PWM_MODE_COMPLEMENTARY |
+ PWM_MODE_PUSH_PULL,
+ },
+};
+
static const struct platform_device_id atmel_pwm_devtypes[] = {
{
.name = "at91sam9rl-pwm",
@@ -328,7 +358,7 @@ static const struct of_device_id atmel_pwm_dt_ids[] = {
.data = &atmel_pwm_data_v2,
}, {
.compatible = "atmel,sama5d2-pwm",
- .data = &atmel_pwm_data_v2,
+ .data = &atmel_pwm_data_v3,
}, {
/* sentinel */
},

--
2.7.4

Brian Norris

2018-01-12 18:35:13 UTC

Permalink

Hi,

For pwm-cros-ec.c:

Nacked-by: Brian Norris <***@chromium.org>

This is a fiat change of the documented binding, which breaks the RK3399
Kevin board. That's not how we do device tree.

You can extend the binding if you want, so you can represent the period
in the device tree if you'd like (though the value won't mean anything;
it can't be changed by the kernel), but don't break existing device
trees.

...

diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c
index 9c13694eaa24..692298693768 100644
--- a/drivers/pwm/pwm-cros-ec.c
+++ b/drivers/pwm/pwm-cros-ec.c
@@ -133,24 +133,6 @@ static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
state->duty_cycle = ret;
}
-static struct pwm_device *
-cros_ec_pwm_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
-{
- struct pwm_device *pwm;
-
- if (args->args[0] >= pc->npwm)
- return ERR_PTR(-EINVAL);
-
- pwm = pwm_request_from_chip(pc, args->args[0], NULL);
- if (IS_ERR(pwm))
- return pwm;
-
- /* The EC won't let us change the period */
- pwm->args.period = EC_PWM_MAX_DUTY;
-
- return pwm;
-}
-
static const struct pwm_ops cros_ec_pwm_ops = {
.get_state = cros_ec_pwm_get_state,
.apply = cros_ec_pwm_apply,
@@ -207,8 +189,6 @@ static int cros_ec_pwm_probe(struct platform_device *pdev)
/* PWM chip */
chip->dev = dev;
chip->ops = &cros_ec_pwm_ops;
- chip->of_xlate = cros_ec_pwm_xlate;
- chip->of_pwm_n_cells = 1;
chip->base = -1;
ret = cros_ec_num_pwms(ec);
if (ret < 0) {

...

Brian

Claudiu Beznea

2018-01-15 12:43:58 UTC

Permalink

Hi Boris,

s/Boris/Brian

Thanks for your review. See below my answers.

Post by Brian Norris
Hi,

This is a fiat change of the documented binding, which breaks the RK3399
Kevin board. That's not how we do device tree.
You can extend the binding if you want, so you can represent the period
in the device tree if you'd like (though the value won't mean anything;
it can't be changed by the kernel), but don't break existing device
trees.

That wasn't the idea, I wasn't intended to break something. The idea was
to have a generic device tree parsing function since all the drivers,
except pwm-pxa.c, pwm-cros-ec.c and pwm-clps711x.c, uses the same function
to parse DT bindings. And I think, these 3 drivers could use this way of
parsing, which is not something new, is what all the current PWM drivers
uses (except pwm-pxa.c, pwm-cros-ec.c and pwm-clps711x.c). It is true
I have no RK3399 board to run any tests.
pwm-cross-ec.c it is true that it's period cannot be changed. It is fixed, as
I saw in the driver, at EC_PWM_MAX_DUTY=0xffff. The driver itself won't apply
any PWM state if the period is different from EC_PWM_MAX_DUTY.
For this driver, the PWM bindings were changed (I did a grep by "google,cros-ec-pwm"
arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
index 0384e3121f18..0c790ec387eb 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
@@ -77,7 +77,7 @@
backlight: backlight {
compatible = "pwm-backlight";
- pwms = <&cros_ec_pwm 1>;
+ pwms = <&cros_ec_pwm 1 65535>;
brightness-levels = <0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16
17 18 19 20 21 22 23 24 25 26 27 28 29 30
31 32 33 34 35 36 37 38 39 40 41 42 43 44
diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
index 5772c52fbfd3..aa377f9ae6ad 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
@@ -853,7 +853,7 @@ ap_i2c_audio: &i2c8 {
cros_ec_pwm: ec-pwm {
compatible = "google,cros-ec-pwm";
- #pwm-cells = <1>;
+ #pwm-cells = <2>;
};
};
};
The code that was removed requests a PWM, the one that was set in the bindings, and
-static struct pwm_device *
-cros_ec_pwm_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
-{
- struct pwm_device *pwm;
-
- if (args->args[0] >= pc->npwm)
- return ERR_PTR(-EINVAL);
-
- pwm = pwm_request_from_chip(pc, args->args[0], NULL);
- if (IS_ERR(pwm))
- return pwm;
-
- /* The EC won't let us change the period */
- pwm->args.period = EC_PWM_MAX_DUTY;
-
- return pwm;
-}
of_pwm_get() -> cros_ec_pwm_xlate() { request chip and set constant period }
of_pwm_get() -> of_pwm_xlate() -> { parse PWM args: channel number, period, flags +
request PWM chip + set pwm->args; }
This path is only used at DT parsing.
In case of PWM channel requested by PWM backlight driver it looks good to me
with the changes in rk3399-gru-kevin.dts (please correct me if I'm wrong).
Since this driver accepts only EC_PWM_MAX_DUTY period maybe the documentation should
be updated regarding this value?
Please, let me know what you think!
Thanks,
Claudiu

Post by Brian Norris

...

...
Brian

_______________________________________________
linux-arm-kernel mailing list
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Andy Shevchenko

2018-01-15 20:27:31 UTC

Permalink

On Mon, Jan 15, 2018 at 10:41 AM, Claudiu Beznea

For this driver, the PWM bindings were changed (I did a grep by "google,cros-ec-pwm"
arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
index 0384e3121f18..0c790ec387eb 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
@@ -77,7 +77,7 @@
backlight: backlight {
compatible = "pwm-backlight";
- pwms = <&cros_ec_pwm 1>;
+ pwms = <&cros_ec_pwm 1 65535>;

This shows an breakage for user. The old PWM device tree sources or
binaries should work independently on what changes you did to kernel.

--
With Best Regards,
Andy Shevchenko

Claudiu Beznea

2018-01-16 08:24:59 UTC

Permalink

Post by Andy Shevchenko
On Mon, Jan 15, 2018 at 10:41 AM, Claudiu Beznea

This shows an breakage for user.

As long as pwm-cells=2 the OF hooks will read PWM channel and PWM period
(e.g. in this case, PWM channel=1, PWM period=65532)
I don't see a breakage here. Please explain me further.

The old PWM device tree sources or

Post by Andy Shevchenko
binaries should work independently on what changes you did to kernel.

Please explain me further. From this I understand, as a general rule,
that the device tree binaries from, e.g. 3 years ago, should be
compatible with, e.g. the current version of kernel?

Thanks,
Claudiu

Claudiu Beznea

2018-01-18 09:11:16 UTC

Permalink

Post by Claudiu Beznea
Please explain me further. From this I understand, as a general rule,
that the device tree binaries from, e.g. 3 years ago, should be
compatible with, e.g. the current version of kernel?

Yes.
https://www.kernel.org/doc/Documentation/devicetree/bindings/ABI.txt

I agree, I found that after some research.

Thanks,
Claudiu

Claudiu Beznea

2018-01-12 14:22:53 UTC

Permalink

Update pwm-cells to 2 to allow initialization of channel number an period.

Signed-off-by: Claudiu Beznea <***@microchip.com>
---
arch/arm/boot/dts/pxa25x.dtsi | 4 ++--
arch/arm/boot/dts/pxa27x.dtsi | 8 ++++----
arch/arm/boot/dts/pxa3xx.dtsi | 8 ++++----
3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/arch/arm/boot/dts/pxa25x.dtsi b/arch/arm/boot/dts/pxa25x.dtsi
index 95d59be97213..8c55b3552c42 100644
--- a/arch/arm/boot/dts/pxa25x.dtsi
+++ b/arch/arm/boot/dts/pxa25x.dtsi
@@ -70,14 +70,14 @@
pwm0: ***@40b00000 {
compatible = "marvell,pxa250-pwm";
reg = <0x40b00000 0x10>;
- #pwm-cells = <1>;
+ #pwm-cells = <2>;
clocks = <&clks CLK_PWM0>;
};

pwm1: ***@40b00010 {
compatible = "marvell,pxa250-pwm";
reg = <0x40b00010 0x10>;
- #pwm-cells = <1>;
+ #pwm-cells = <2>;
clocks = <&clks CLK_PWM1>;
};
};
diff --git a/arch/arm/boot/dts/pxa27x.dtsi b/arch/arm/boot/dts/pxa27x.dtsi
index 747f750f675d..e3db171cfeb1 100644
--- a/arch/arm/boot/dts/pxa27x.dtsi
+++ b/arch/arm/boot/dts/pxa27x.dtsi
@@ -46,28 +46,28 @@
pwm0: ***@40b00000 {
compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm";
reg = <0x40b00000 0x10>;
- #pwm-cells = <1>;
+ #pwm-cells = <2>;
clocks = <&clks CLK_PWM0>;
};

pwm1: ***@40b00010 {
compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm";
reg = <0x40b00010 0x10>;
- #pwm-cells = <1>;
+ #pwm-cells = <2>;
clocks = <&clks CLK_PWM1>;
};

pwm2: ***@40c00000 {
compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm";
reg = <0x40c00000 0x10>;
- #pwm-cells = <1>;
+ #pwm-cells = <2>;
clocks = <&clks CLK_PWM0>;
};

pwm3: ***@40c00010 {
compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm";
reg = <0x40c00010 0x10>;
- #pwm-cells = <1>;
+ #pwm-cells = <2>;
clocks = <&clks CLK_PWM1>;
};

diff --git a/arch/arm/boot/dts/pxa3xx.dtsi b/arch/arm/boot/dts/pxa3xx.dtsi
index 55c75b67351c..1f37295b05a3 100644
--- a/arch/arm/boot/dts/pxa3xx.dtsi
+++ b/arch/arm/boot/dts/pxa3xx.dtsi
@@ -200,7 +200,7 @@
pwm0: ***@40b00000 {
compatible = "marvell,pxa270-pwm";
reg = <0x40b00000 0x10>;
- #pwm-cells = <1>;
+ #pwm-cells = <2>;
clocks = <&clks CLK_PWM0>;
status = "disabled";
};
@@ -208,7 +208,7 @@
pwm1: ***@40b00010 {
compatible = "marvell,pxa270-pwm";
reg = <0x40b00010 0x10>;
- #pwm-cells = <1>;
+ #pwm-cells = <2>;
clocks = <&clks CLK_PWM1>;
status = "disabled";
};
@@ -216,7 +216,7 @@
pwm2: ***@40c00000 {
compatible = "marvell,pxa270-pwm";
reg = <0x40c00000 0x10>;
- #pwm-cells = <1>;
+ #pwm-cells = <2>;
clocks = <&clks CLK_PWM0>;
status = "disabled";
};
@@ -224,7 +224,7 @@
pwm3: ***@40c00010 {
compatible = "marvell,pxa270-pwm";
reg = <0x40c00010 0x10>;
- #pwm-cells = <1>;
+ #pwm-cells = <2>;
clocks = <&clks CLK_PWM1>;
status = "disabled";
};

--
2.7.4

Claudiu Beznea

2018-01-12 14:22:54 UTC

Permalink

Update pwm-cells to 2 to allow initialization of channel number an period.

Cc: Brian Norris <***@chromium.org>
Signed-off-by: Claudiu Beznea <***@microchip.com>
---
arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts | 2 +-
arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
index 0384e3121f18..0c790ec387eb 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
@@ -77,7 +77,7 @@

backlight: backlight {
compatible = "pwm-backlight";
- pwms = <&cros_ec_pwm 1>;
+ pwms = <&cros_ec_pwm 1 65535>;
brightness-levels = <0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16
17 18 19 20 21 22 23 24 25 26 27 28 29 30
31 32 33 34 35 36 37 38 39 40 41 42 43 44
diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
index 5772c52fbfd3..aa377f9ae6ad 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
@@ -853,7 +853,7 @@ ap_i2c_audio: &i2c8 {

cros_ec_pwm: ec-pwm {
compatible = "google,cros-ec-pwm";
- #pwm-cells = <1>;
+ #pwm-cells = <2>;
};
};
};

--
2.7.4

Claudiu Beznea

2018-01-12 14:22:55 UTC

Permalink

Add basic PWM modes: normal and complementary. These modes should
differentiate the single output per channel PWM controllers from multiple
outputs per channel PWM controllers. These modes whould be set as follow:
1. PWM controllers with only one output per channel:
- normal mode
2. PWM controllers with more than one output per channel:
- normal mode
- complementary mode
Since users could use the PWM channel of a multiple output per channel PWM
controller, he could set the channel in normal mode and use only one
physical output.
The PWM modes were implemented as capabilities of PWM chip. In the probe
function of PWM driver the PWM capabilities (which currently contains only
PWM modes) should be provided in the structure of PWM chip. If no
capabilities are provided by the probe function, the default capabilities
will be used (the default capabilities involves PWM normal mode).
Every PWM channel will have associated a mode in the PWM state. Proper
helper functions were added to get/set PWM mode. The mode could also be set
from DT. Only modes registered for PWM chip could be set for a PWM channel.

Signed-off-by: Claudiu Beznea <***@microchip.com>
---
drivers/pwm/core.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
drivers/pwm/sysfs.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/pwm.h | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++-
3 files changed, 182 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 7208b95e8d2f..99126127a467 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -41,6 +41,10 @@ static LIST_HEAD(pwm_chips);
static DECLARE_BITMAP(allocated_pwms, MAX_PWMS);
static RADIX_TREE(pwm_tree, GFP_KERNEL);

+static const struct pwm_caps pwm_chip_default_caps = {
+ .modes = PWM_MODE_NORMAL,
+};
+
static struct pwm_device *pwm_to_device(unsigned int pwm)
{
return radix_tree_lookup(&pwm_tree, pwm);
@@ -142,6 +146,31 @@ static inline void of_pwm_xlate_flags(struct pwm_device *pwm,
pwm->args.polarity = PWM_POLARITY_NORMAL;
}

+static inline bool pwm_mode_valid(const struct pwm_chip *chip,
+ const enum pwm_mode mode)
+{
+ return !!(chip->caps->modes & mode);
+}
+
+static inline void of_pwm_xlate_mode(struct pwm_device *pwm,
+ const struct of_phandle_args *args)
+{
+ unsigned int first = 0, last = 0;
+
+ if (args->args_count >= PWM_ARGS_CNT_XLATE_MODE) {
+ first = ffs(args->args[PWM_ARGS_CNT_XLATE_MODE - 1]);
+ last = fls(args->args[PWM_ARGS_CNT_XLATE_MODE - 1]);
+ }
+
+ /* At least one valid mode provided from DT: use one valid mode */
+ if (first && pwm_mode_valid(pwm->chip, BIT(first - 1)))
+ pwm->args.mode = BIT(first - 1);
+ else if (last && pwm_mode_valid(pwm->chip, BIT(last - 1)))
+ pwm->args.mode = BIT(last - 1);
+ else /* Invalid modes provided from DT: use first available chip mode */
+ pwm->args.mode = BIT(ffs(pwm->chip->caps->modes) - 1);
+}
+
struct pwm_device *of_pwm_xlate(struct pwm_chip *pc,
const struct of_phandle_args *args)
{
@@ -161,6 +190,7 @@ struct pwm_device *of_pwm_xlate(struct pwm_chip *pc,
pwm->args.period = args->args[PWM_ARGS_CNT_XLATE_PERIOD - 1];

of_pwm_xlate_flags(pwm, args);
+ of_pwm_xlate_mode(pwm, args);

return pwm;
}
@@ -223,6 +253,18 @@ static bool pwm_ops_check(const struct pwm_ops *ops)
return false;
}

+static inline bool pwm_caps_valid(const struct pwm_caps *caps)
+{
+ unsigned long modes = PWM_MODE_MAX - 1;
+
+ return !!((modes & caps->modes) == caps->modes);
+}
+
+static inline bool pwm_caps_zero(const struct pwm_caps *caps)
+{
+ return !!(caps->modes == 0);
+}
+
/**
* pwmchip_add_with_polarity() - register a new PWM chip
* @chip: the PWM chip to add
@@ -247,8 +289,14 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
if (!pwm_ops_check(chip->ops))
return -EINVAL;

+ if (chip->caps && !pwm_caps_valid(chip->caps))
+ return -EINVAL;
+
mutex_lock(&pwm_lock);

+ if (!chip->caps || (chip->caps && pwm_caps_zero(chip->caps)))
+ chip->caps = &pwm_chip_default_caps;
+
ret = alloc_pwms(chip->base, chip->npwm);
if (ret < 0)
goto out;
@@ -268,6 +316,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
pwm->pwm = chip->base + i;
pwm->hwpwm = i;
pwm->state.polarity = polarity;
+ pwm->state.mode = BIT(ffs(chip->caps->modes) - 1);

if (chip->ops->get_state)
chip->ops->get_state(chip, pwm, &pwm->state);
@@ -443,7 +492,11 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
int err;

if (!pwm || !state || !state->period ||
- state->duty_cycle > state->period)
+ state->duty_cycle > state->period ||
+ !pwm->chip || !pwm->chip->caps ||
+ !pwm_mode_valid(pwm->chip, state->mode) ||
+ /* Only one active mode at a time. */
+ fls(state->mode) != ffs(state->mode))
return -EINVAL;

if (!memcmp(state, &pwm->state, sizeof(*state)))
@@ -504,6 +557,9 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)

pwm->state.enabled = state->enabled;
}
+
+ /* No mode support for non-atomic PWM. */
+ pwm->state.mode = state->mode;
}

return 0;
@@ -553,6 +609,8 @@ int pwm_adjust_config(struct pwm_device *pwm)
pwm_get_args(pwm, &pargs);
pwm_get_state(pwm, &state);

+ state.mode = pargs.mode;
+
/*
* If the current period is zero it means that either the PWM driver
* does not support initial state retrieval or the PWM has not yet
@@ -824,6 +882,7 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id)

pwm->args.period = chosen->period;
pwm->args.polarity = chosen->polarity;
+ pwm->args.mode = BIT(ffs(chip->caps->modes) - 1);

return pwm;
}
@@ -973,6 +1032,7 @@ static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
seq_printf(s, " duty: %u ns", state.duty_cycle);
seq_printf(s, " polarity: %s",
state.polarity ? "inverse" : "normal");
+ seq_printf(s, " mode: %s", pwm_get_mode_desc(state.mode));

seq_puts(s, "\n");
}
diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index 83f2b0b15712..7d111ab17e43 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -223,11 +223,50 @@ static ssize_t capture_show(struct device *child,
return sprintf(buf, "%u %u\n", result.period, result.duty_cycle);
}

+static ssize_t mode_show(struct device *child,
+ struct device_attribute *attr,
+ char *buf)
+{
+ const struct pwm_device *pwm = child_to_pwm_device(child);
+ struct pwm_state state;
+
+ pwm_get_state(pwm, &state);
+
+ return sprintf(buf, "%s\n", pwm_get_mode_desc(state.mode));
+}
+
+static ssize_t mode_store(struct device *child,
+ struct device_attribute *attr,
+ const char *buf, size_t size)
+{
+ struct pwm_export *export = child_to_pwm_export(child);
+ struct pwm_device *pwm = export->pwm;
+ struct pwm_state state;
+ enum pwm_mode mode;
+ int ret;
+
+ if (sysfs_streq(buf, pwm_get_mode_desc(PWM_MODE_NORMAL)))
+ mode = PWM_MODE_NORMAL;
+ else if (sysfs_streq(buf, pwm_get_mode_desc(PWM_MODE_COMPLEMENTARY)))
+ mode = PWM_MODE_COMPLEMENTARY;
+ else
+ return -EINVAL;
+
+ mutex_lock(&export->lock);
+ pwm_get_state(pwm, &state);
+ state.mode = mode;
+ ret = pwm_apply_state(pwm, &state);
+ mutex_unlock(&export->lock);
+
+ return ret ? : size;
+}
+
static DEVICE_ATTR_RW(period);
static DEVICE_ATTR_RW(duty_cycle);
static DEVICE_ATTR_RW(enable);
static DEVICE_ATTR_RW(polarity);
static DEVICE_ATTR_RO(capture);
+static DEVICE_ATTR_RW(mode);

static struct attribute *pwm_attrs[] = {
&dev_attr_period.attr,
@@ -235,6 +274,7 @@ static struct attribute *pwm_attrs[] = {
&dev_attr_enable.attr,
&dev_attr_polarity.attr,
&dev_attr_capture.attr,
+ &dev_attr_mode.attr,
NULL
};
ATTRIBUTE_GROUPS(pwm);
@@ -362,10 +402,32 @@ static ssize_t npwm_show(struct device *parent, struct device_attribute *attr,
}
static DEVICE_ATTR_RO(npwm);

+static ssize_t modes_show(struct device *parent,
+ struct device_attribute *attr,
+ char *buf)
+{
+ const struct pwm_chip *chip = dev_get_drvdata(parent);
+ enum pwm_mode mode;
+ int i, len = 0;
+
+ for (i = 0; i < PWM_MODE_MAX - 1; i++) {
+ mode = BIT(i);
+ if (chip->caps->modes & mode)
+ len += scnprintf(buf + len, PAGE_SIZE - len, "%s ",
+ pwm_get_mode_desc(mode));
+ }
+
+ len += scnprintf(buf + len, PAGE_SIZE - len, "\n");
+
+ return len;
+}
+static DEVICE_ATTR_RO(modes);
+
static struct attribute *pwm_chip_attrs[] = {
&dev_attr_export.attr,
&dev_attr_unexport.attr,
&dev_attr_npwm.attr,
+ &dev_attr_modes.attr,
NULL,
};
ATTRIBUTE_GROUPS(pwm_chip);
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 4bb628b94d88..0fdc680651aa 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -26,9 +26,23 @@ enum pwm_polarity {
};

/**
+ * enum pwm_mode - PWM working modes
+ * PWM_MODE_NORMAL: PWM has one output per channel
+ * PWM_MODE_COMPLEMENTARY: PWM has 2 outputs per channel with opposite polarity
+ * PWM_MODE_MAX: Used to get the defined PWM modes mask (PWM_MODE_MAX - 1)
+ * phase-shifted
+ */
+enum pwm_mode {
+ PWM_MODE_NORMAL = BIT(0),
+ PWM_MODE_COMPLEMENTARY = BIT(1),
+ PWM_MODE_MAX = BIT(2),
+};
+
+/**
* struct pwm_args - board-dependent PWM arguments
* @period: reference period
* @polarity: reference polarity
+ * @mode: reference mode
*
* This structure describes board-dependent arguments attached to a PWM
* device. These arguments are usually retrieved from the PWM lookup table or
@@ -41,6 +55,7 @@ enum pwm_polarity {
struct pwm_args {
unsigned int period;
enum pwm_polarity polarity;
+ enum pwm_mode mode;
};

enum {
@@ -52,12 +67,22 @@ enum {
* enum pwm_args_xlate_options - options for translating PWM options
* @PWM_ARGS_CNT_XLATE_PERIOD: translate period
* @PWM_ARGS_CNT_XLATE_FLAGS: translate flags (polarity flags)
+ * @PWM_ARGS_CNT_XLATE_MODE: translate with flags and mode
* @PWM_ARGS_CNT_XLATE_MAX: maximum number of translate options
*/
enum pwm_args_xlate_options {
PWM_ARGS_CNT_XLATE_PERIOD = 2,
PWM_ARGS_CNT_XLATE_FLAGS,
- PWM_ARGS_CNT_XLATE_MAX = PWM_ARGS_CNT_XLATE_FLAGS,
+ PWM_ARGS_CNT_XLATE_MODE,
+ PWM_ARGS_CNT_XLATE_MAX = PWM_ARGS_CNT_XLATE_MODE,
+};
+
+/**
+ * struct pwm_caps - PWM capabilities
+ * @modes: PWM chip supported modes
+ */
+struct pwm_caps {
+ unsigned long modes;
};

/*
@@ -65,12 +90,14 @@ enum pwm_args_xlate_options {
* @period: PWM period (in nanoseconds)
* @duty_cycle: PWM duty cycle (in nanoseconds)
* @polarity: PWM polarity
+ * @mode: PWM mode
* @enabled: PWM enabled status
*/
struct pwm_state {
unsigned int period;
unsigned int duty_cycle;
enum pwm_polarity polarity;
+ enum pwm_mode mode;
bool enabled;
};

@@ -156,6 +183,21 @@ static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm)
return state.polarity;
}

+static inline enum pwm_mode pwm_get_mode(const struct pwm_device *pwm)
+{
+ struct pwm_state state;
+
+ pwm_get_state(pwm, &state);
+
+ return state.mode;
+}
+
+static inline void pwm_set_mode(struct pwm_device *pwm, enum pwm_mode mode)
+{
+ if (pwm)
+ pwm->state.mode = mode;
+}
+
static inline void pwm_get_args(const struct pwm_device *pwm,
struct pwm_args *args)
{
@@ -193,6 +235,7 @@ static inline void pwm_init_state(const struct pwm_device *pwm,
state->period = args.period;
state->polarity = args.polarity;
state->duty_cycle = 0;
+ state->mode = args.mode;
}

/**
@@ -295,6 +338,7 @@ struct pwm_ops {
* @dev: device providing the PWMs
* @list: list node for internal use
* @ops: callbacks for this PWM controller
+ * @caps: capabilities for this PWM controller
* @base: number of first PWM controlled by this chip
* @npwm: number of PWMs controlled by this chip
* @pwms: array of PWM devices allocated by the framework
@@ -303,6 +347,7 @@ struct pwm_chip {
struct device *dev;
struct list_head list;
const struct pwm_ops *ops;
+ const struct pwm_caps *caps;
int base;
unsigned int npwm;
struct pwm_device *pwms;
@@ -429,6 +474,13 @@ static inline void pwm_disable(struct pwm_device *pwm)
pwm_apply_state(pwm, &state);
}

+static inline const char * const pwm_get_mode_desc(enum pwm_mode mode)
+{
+ static const char * const modes[] = { "normal", "complementary" };
+
+ return mode ? modes[ffs(mode) - 1] : "invalid";
+}
+
/* PWM provider APIs */
int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result,
unsigned long timeout);
@@ -503,6 +555,11 @@ static inline void pwm_disable(struct pwm_device *pwm)
{
}

+static inline const char * const pwm_get_mode_desc(enum pwm_mode mode)
+{
+ return NULL;
+}
+
static inline int pwm_set_chip_data(struct pwm_device *pwm, void *data)
{
return -EINVAL;
@@ -597,6 +654,7 @@ static inline void pwm_apply_args(struct pwm_device *pwm)
state.enabled = false;
state.polarity = pwm->args.polarity;
state.period = pwm->args.period;
+ state.mode = pwm->args.mode;

pwm_apply_state(pwm, &state);
}

--
2.7.4