devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 00/16] extend PWM framework to support PWM modes
@ 2018-01-12 14:22 Claudiu Beznea
  2018-01-12 14:22 ` [PATCH v2 01/16] drivers: pwm: core: use a single of xlate function Claudiu Beznea
                   ` (11 more replies)
  0 siblings, 12 replies; 42+ messages in thread
From: Claudiu Beznea @ 2018-01-12 14:22 UTC (permalink / raw)
  To: thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
	linux-I+IVW8TIWO2tmTQ+vhA3Yw, daniel-cYrQPVfZoowdnm+yROfE0A,
	haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w,
	robert.jarzmik-GANU6spQydw, corbet-T1hC0tSOHrs,
	nicolas.ferre-UWL1GkI3JZL3oGB3hsPCZA,
	alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
  Cc: linux-pwm-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Claudiu Beznea

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:
root@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
root@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:
root@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

root@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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [PATCH v2 01/16] drivers: pwm: core: use a single of xlate function
  2018-01-12 14:22 [PATCH v2 00/16] extend PWM framework to support PWM modes Claudiu Beznea
@ 2018-01-12 14:22 ` Claudiu Beznea
       [not found]   ` <1515766983-15151-2-git-send-email-claudiu.beznea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
  2018-01-16  9:07   ` Neil Armstrong
  2018-01-12 14:22 ` [PATCH v2 02/16] pwm: pxa: update documentation regarding pwm-cells Claudiu Beznea
                   ` (10 subsequent siblings)
  11 siblings, 2 replies; 42+ messages in thread
From: Claudiu Beznea @ 2018-01-12 14:22 UTC (permalink / raw)
  To: thierry.reding, robh+dt, mark.rutland, linux, daniel,
	haojian.zhuang, robert.jarzmik, corbet, nicolas.ferre,
	alexandre.belloni
  Cc: linux-pwm, linux-kernel, linux-arm-kernel, linux-amlogic,
	linux-rockchip, linux-rpi-kernel, devicetree, Claudiu Beznea,
	Mike Dunn, Brian Norris, Alexander Shiyan

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).

Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Mike Dunn <mikedunn@newsguy.com>
Cc: Brian Norris <briannorris@chromium.org>
Cc: Alexander Shiyan <shc_work@mail.ru>
Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---

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/core.c b/drivers/pwm/core.c
index 1581f6ab1b1f..7208b95e8d2f 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -132,47 +132,23 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
 	return 0;
 }
 
-struct pwm_device *
-of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)
+static inline void of_pwm_xlate_flags(struct pwm_device *pwm,
+				      const struct of_phandle_args *args)
 {
-	struct pwm_device *pwm;
-
-	/* check, whether the driver supports a third cell for flags */
-	if (pc->of_pwm_n_cells < 3)
-		return ERR_PTR(-EINVAL);
-
-	/* flags in the third cell 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;
-
-	if (args->args_count > 2 && args->args[2] & PWM_POLARITY_INVERTED)
+	if (args->args_count >= PWM_ARGS_CNT_XLATE_FLAGS &&
+	    args->args[PWM_ARGS_CNT_XLATE_FLAGS - 1] & PWM_POLARITY_INVERTED)
 		pwm->args.polarity = PWM_POLARITY_INVERSED;
-
-	return pwm;
+	else
+		pwm->args.polarity = PWM_POLARITY_NORMAL;
 }
-EXPORT_SYMBOL_GPL(of_pwm_xlate_with_flags);
 
-static struct pwm_device *
-of_pwm_simple_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
+struct pwm_device *of_pwm_xlate(struct pwm_chip *pc,
+				const struct of_phandle_args *args)
 {
 	struct pwm_device *pwm;
 
-	/* sanity check driver support */
-	if (pc->of_pwm_n_cells < 2)
-		return ERR_PTR(-EINVAL);
-
-	/* all cells are required */
-	if (args->args_count != pc->of_pwm_n_cells)
+	if (args->args_count < PWM_ARGS_CNT_XLATE_PERIOD ||
+	    args->args_count > PWM_ARGS_CNT_XLATE_MAX)
 		return ERR_PTR(-EINVAL);
 
 	if (args->args[0] >= pc->npwm)
@@ -182,21 +158,19 @@ of_pwm_simple_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
 	if (IS_ERR(pwm))
 		return pwm;
 
-	pwm->args.period = args->args[1];
+	pwm->args.period = args->args[PWM_ARGS_CNT_XLATE_PERIOD - 1];
+
+	of_pwm_xlate_flags(pwm, args);
 
 	return pwm;
 }
+EXPORT_SYMBOL_GPL(of_pwm_xlate);
 
 static void of_pwmchip_add(struct pwm_chip *chip)
 {
 	if (!chip->dev || !chip->dev->of_node)
 		return;
 
-	if (!chip->of_xlate) {
-		chip->of_xlate = of_pwm_simple_xlate;
-		chip->of_pwm_n_cells = 2;
-	}
-
 	of_node_get(chip->dev->of_node);
 }
 
@@ -685,7 +659,7 @@ struct pwm_device *of_pwm_get(struct device_node *np, const char *con_id)
 		goto put;
 	}
 
-	pwm = pc->of_xlate(pc, &args);
+	pwm = of_pwm_xlate(pc, &args);
 	if (IS_ERR(pwm))
 		goto put;
 
diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c
index 54c6633d9b5d..b574e9e639f1 100644
--- a/drivers/pwm/pwm-atmel-hlcdc.c
+++ b/drivers/pwm/pwm-atmel-hlcdc.c
@@ -277,8 +277,6 @@ static int atmel_hlcdc_pwm_probe(struct platform_device *pdev)
 	chip->chip.dev = dev;
 	chip->chip.base = -1;
 	chip->chip.npwm = 1;
-	chip->chip.of_xlate = of_pwm_xlate_with_flags;
-	chip->chip.of_pwm_n_cells = 3;
 
 	ret = pwmchip_add_with_polarity(&chip->chip, PWM_POLARITY_INVERSED);
 	if (ret) {
diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c
index acd3ce8ecf3f..e75bef50b831 100644
--- a/drivers/pwm/pwm-atmel-tcb.c
+++ b/drivers/pwm/pwm-atmel-tcb.c
@@ -407,8 +407,6 @@ static int atmel_tcb_pwm_probe(struct platform_device *pdev)
 
 	tcbpwm->chip.dev = &pdev->dev;
 	tcbpwm->chip.ops = &atmel_tcb_pwm_ops;
-	tcbpwm->chip.of_xlate = of_pwm_xlate_with_flags;
-	tcbpwm->chip.of_pwm_n_cells = 3;
 	tcbpwm->chip.base = -1;
 	tcbpwm->chip.npwm = NPWM;
 	tcbpwm->tc = tc;
diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index 530d7dc5f1b5..32427d2b7877 100644
--- a/drivers/pwm/pwm-atmel.c
+++ b/drivers/pwm/pwm-atmel.c
@@ -365,12 +365,6 @@ static int atmel_pwm_probe(struct platform_device *pdev)
 
 	atmel_pwm->chip.dev = &pdev->dev;
 	atmel_pwm->chip.ops = &atmel_pwm_ops;
-
-	if (pdev->dev.of_node) {
-		atmel_pwm->chip.of_xlate = of_pwm_xlate_with_flags;
-		atmel_pwm->chip.of_pwm_n_cells = 3;
-	}
-
 	atmel_pwm->chip.base = -1;
 	atmel_pwm->chip.npwm = 4;
 	atmel_pwm->regs = regs;
diff --git a/drivers/pwm/pwm-bcm-iproc.c b/drivers/pwm/pwm-bcm-iproc.c
index d961a8207b1c..773c53b6c13d 100644
--- a/drivers/pwm/pwm-bcm-iproc.c
+++ b/drivers/pwm/pwm-bcm-iproc.c
@@ -207,8 +207,6 @@ static int iproc_pwmc_probe(struct platform_device *pdev)
 	ip->chip.ops = &iproc_pwm_ops;
 	ip->chip.base = -1;
 	ip->chip.npwm = 4;
-	ip->chip.of_xlate = of_pwm_xlate_with_flags;
-	ip->chip.of_pwm_n_cells = 3;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	ip->base = devm_ioremap_resource(&pdev->dev, res);
diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 09a95aeb3a70..e3418e9b7828 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -274,8 +274,6 @@ static int kona_pwmc_probe(struct platform_device *pdev)
 	kp->chip.ops = &kona_pwm_ops;
 	kp->chip.base = -1;
 	kp->chip.npwm = 6;
-	kp->chip.of_xlate = of_pwm_xlate_with_flags;
-	kp->chip.of_pwm_n_cells = 3;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	kp->base = devm_ioremap_resource(&pdev->dev, res);
diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c
index db001cba937f..c5dbf16d810b 100644
--- a/drivers/pwm/pwm-bcm2835.c
+++ b/drivers/pwm/pwm-bcm2835.c
@@ -167,8 +167,6 @@ static int bcm2835_pwm_probe(struct platform_device *pdev)
 	pc->chip.dev = &pdev->dev;
 	pc->chip.ops = &bcm2835_pwm_ops;
 	pc->chip.npwm = 2;
-	pc->chip.of_xlate = of_pwm_xlate_with_flags;
-	pc->chip.of_pwm_n_cells = 3;
 
 	platform_set_drvdata(pdev, pc);
 
diff --git a/drivers/pwm/pwm-berlin.c b/drivers/pwm/pwm-berlin.c
index 771859aca4be..ffe75a891d35 100644
--- a/drivers/pwm/pwm-berlin.c
+++ b/drivers/pwm/pwm-berlin.c
@@ -206,8 +206,6 @@ static int berlin_pwm_probe(struct platform_device *pdev)
 	pwm->chip.ops = &berlin_pwm_ops;
 	pwm->chip.base = -1;
 	pwm->chip.npwm = 4;
-	pwm->chip.of_xlate = of_pwm_xlate_with_flags;
-	pwm->chip.of_pwm_n_cells = 3;
 
 	ret = pwmchip_add(&pwm->chip);
 	if (ret < 0) {
diff --git a/drivers/pwm/pwm-clps711x.c b/drivers/pwm/pwm-clps711x.c
index 26ec24e457b1..432b6ff174e9 100644
--- a/drivers/pwm/pwm-clps711x.c
+++ b/drivers/pwm/pwm-clps711x.c
@@ -106,15 +106,6 @@ static const struct pwm_ops clps711x_pwm_ops = {
 	.owner = THIS_MODULE,
 };
 
-static struct pwm_device *clps711x_pwm_xlate(struct pwm_chip *chip,
-					     const struct of_phandle_args *args)
-{
-	if (args->args[0] >= chip->npwm)
-		return ERR_PTR(-EINVAL);
-
-	return pwm_request_from_chip(chip, args->args[0], NULL);
-}
-
 static int clps711x_pwm_probe(struct platform_device *pdev)
 {
 	struct clps711x_chip *priv;
@@ -137,8 +128,6 @@ static int clps711x_pwm_probe(struct platform_device *pdev)
 	priv->chip.dev = &pdev->dev;
 	priv->chip.base = -1;
 	priv->chip.npwm = 2;
-	priv->chip.of_xlate = clps711x_pwm_xlate;
-	priv->chip.of_pwm_n_cells = 1;
 
 	spin_lock_init(&priv->lock);
 
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) {
diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
index 557b4ea16796..7b9d59796ebd 100644
--- a/drivers/pwm/pwm-fsl-ftm.c
+++ b/drivers/pwm/pwm-fsl-ftm.c
@@ -442,8 +442,6 @@ static int fsl_pwm_probe(struct platform_device *pdev)
 		return PTR_ERR(fpc->clk[FSL_PWM_CLK_CNTEN]);
 
 	fpc->chip.ops = &fsl_pwm_ops;
-	fpc->chip.of_xlate = of_pwm_xlate_with_flags;
-	fpc->chip.of_pwm_n_cells = 3;
 	fpc->chip.base = -1;
 	fpc->chip.npwm = 8;
 
diff --git a/drivers/pwm/pwm-hibvt.c b/drivers/pwm/pwm-hibvt.c
index 27c107e78d59..afc74ef76551 100644
--- a/drivers/pwm/pwm-hibvt.c
+++ b/drivers/pwm/pwm-hibvt.c
@@ -196,8 +196,6 @@ static int hibvt_pwm_probe(struct platform_device *pdev)
 	pwm_chip->chip.dev = &pdev->dev;
 	pwm_chip->chip.base = -1;
 	pwm_chip->chip.npwm = soc->num_pwms;
-	pwm_chip->chip.of_xlate = of_pwm_xlate_with_flags;
-	pwm_chip->chip.of_pwm_n_cells = 3;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	pwm_chip->base = devm_ioremap_resource(&pdev->dev, res);
diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index 2ba5c3a398ff..cdfb04f40583 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -240,7 +240,6 @@ static const struct pwm_ops imx_pwm_ops_v2 = {
 };
 
 struct imx_pwm_data {
-	bool polarity_supported;
 	const struct pwm_ops *ops;
 };
 
@@ -249,7 +248,6 @@ static struct imx_pwm_data imx_pwm_data_v1 = {
 };
 
 static struct imx_pwm_data imx_pwm_data_v2 = {
-	.polarity_supported = true,
 	.ops = &imx_pwm_ops_v2,
 };
 
@@ -290,12 +288,6 @@ static int imx_pwm_probe(struct platform_device *pdev)
 	imx->chip.base = -1;
 	imx->chip.npwm = 1;
 
-	if (data->polarity_supported) {
-		dev_dbg(&pdev->dev, "PWM supports output inversion\n");
-		imx->chip.of_xlate = of_pwm_xlate_with_flags;
-		imx->chip.of_pwm_n_cells = 3;
-	}
-
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	imx->mmio_base = devm_ioremap_resource(&pdev->dev, r);
 	if (IS_ERR(imx->mmio_base))
diff --git a/drivers/pwm/pwm-lpc18xx-sct.c b/drivers/pwm/pwm-lpc18xx-sct.c
index d7f5f7de030d..ea825ec28da9 100644
--- a/drivers/pwm/pwm-lpc18xx-sct.c
+++ b/drivers/pwm/pwm-lpc18xx-sct.c
@@ -380,8 +380,6 @@ static int lpc18xx_pwm_probe(struct platform_device *pdev)
 	lpc18xx_pwm->chip.ops = &lpc18xx_pwm_ops;
 	lpc18xx_pwm->chip.base = -1;
 	lpc18xx_pwm->chip.npwm = 16;
-	lpc18xx_pwm->chip.of_xlate = of_pwm_xlate_with_flags;
-	lpc18xx_pwm->chip.of_pwm_n_cells = 3;
 
 	/* SCT counter must be in unify (32 bit) mode */
 	lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_CONFIG,
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);
 
diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c
index 4143a46684d2..d66816037e87 100644
--- a/drivers/pwm/pwm-pxa.c
+++ b/drivers/pwm/pwm-pxa.c
@@ -151,20 +151,6 @@ static const struct platform_device_id *pxa_pwm_get_id_dt(struct device *dev)
 	return id ? id->data : NULL;
 }
 
-static struct pwm_device *
-pxa_pwm_of_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
-{
-	struct pwm_device *pwm;
-
-	pwm = pwm_request_from_chip(pc, 0, NULL);
-	if (IS_ERR(pwm))
-		return pwm;
-
-	pwm->args.period = args->args[0];
-
-	return pwm;
-}
-
 static int pwm_probe(struct platform_device *pdev)
 {
 	const struct platform_device_id *id = platform_get_device_id(pdev);
@@ -191,11 +177,6 @@ static int pwm_probe(struct platform_device *pdev)
 	pwm->chip.base = -1;
 	pwm->chip.npwm = (id->driver_data & HAS_SECONDARY_PWM) ? 2 : 1;
 
-	if (IS_ENABLED(CONFIG_OF)) {
-		pwm->chip.of_xlate = pxa_pwm_of_xlate;
-		pwm->chip.of_pwm_n_cells = 1;
-	}
-
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	pwm->mmio_base = devm_ioremap_resource(&pdev->dev, r);
 	if (IS_ERR(pwm->mmio_base))
diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c
index 29267d12fb4c..a4606957e7e5 100644
--- a/drivers/pwm/pwm-renesas-tpu.c
+++ b/drivers/pwm/pwm-renesas-tpu.c
@@ -418,8 +418,6 @@ static int tpu_probe(struct platform_device *pdev)
 
 	tpu->chip.dev = &pdev->dev;
 	tpu->chip.ops = &tpu_pwm_ops;
-	tpu->chip.of_xlate = of_pwm_xlate_with_flags;
-	tpu->chip.of_pwm_n_cells = 3;
 	tpu->chip.base = -1;
 	tpu->chip.npwm = TPU_CHANNEL_MAX;
 
diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index 4d99d468df09..c754c57118be 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -363,11 +363,6 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 	pc->chip.base = -1;
 	pc->chip.npwm = 1;
 
-	if (pc->data->supports_polarity) {
-		pc->chip.of_xlate = of_pwm_xlate_with_flags;
-		pc->chip.of_pwm_n_cells = 3;
-	}
-
 	ret = pwmchip_add(&pc->chip);
 	if (ret < 0) {
 		clk_unprepare(pc->clk);
diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c
index 062f2cfc45ec..de171bfa4903 100644
--- a/drivers/pwm/pwm-samsung.c
+++ b/drivers/pwm/pwm-samsung.c
@@ -532,9 +532,6 @@ static int pwm_samsung_probe(struct platform_device *pdev)
 		ret = pwm_samsung_parse_dt(chip);
 		if (ret)
 			return ret;
-
-		chip->chip.of_xlate = of_pwm_xlate_with_flags;
-		chip->chip.of_pwm_n_cells = 3;
 	} else {
 		if (!pdev->dev.platform_data) {
 			dev_err(&pdev->dev, "no platform data specified\n");
diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index 334199c58f1d..b8476ce713a2 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -390,8 +390,6 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
 	pwm->chip.ops = &sun4i_pwm_ops;
 	pwm->chip.base = -1;
 	pwm->chip.npwm = pwm->data->npwm;
-	pwm->chip.of_xlate = of_pwm_xlate_with_flags;
-	pwm->chip.of_pwm_n_cells = 3;
 
 	spin_lock_init(&pwm->ctrl_lock);
 
diff --git a/drivers/pwm/pwm-tiecap.c b/drivers/pwm/pwm-tiecap.c
index 34b228626bd5..4210f3ad87e1 100644
--- a/drivers/pwm/pwm-tiecap.c
+++ b/drivers/pwm/pwm-tiecap.c
@@ -238,8 +238,6 @@ static int ecap_pwm_probe(struct platform_device *pdev)
 
 	pc->chip.dev = &pdev->dev;
 	pc->chip.ops = &ecap_pwm_ops;
-	pc->chip.of_xlate = of_pwm_xlate_with_flags;
-	pc->chip.of_pwm_n_cells = 3;
 	pc->chip.base = -1;
 	pc->chip.npwm = 1;
 
diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
index 4c22cb395040..7b0715624282 100644
--- a/drivers/pwm/pwm-tiehrpwm.c
+++ b/drivers/pwm/pwm-tiehrpwm.c
@@ -469,8 +469,6 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev)
 
 	pc->chip.dev = &pdev->dev;
 	pc->chip.ops = &ehrpwm_pwm_ops;
-	pc->chip.of_xlate = of_pwm_xlate_with_flags;
-	pc->chip.of_pwm_n_cells = 3;
 	pc->chip.base = -1;
 	pc->chip.npwm = NUM_PWM_CHANNEL;
 
diff --git a/drivers/pwm/pwm-vt8500.c b/drivers/pwm/pwm-vt8500.c
index 3a78dd09ac81..3a923c1d30d3 100644
--- a/drivers/pwm/pwm-vt8500.c
+++ b/drivers/pwm/pwm-vt8500.c
@@ -216,8 +216,6 @@ static int vt8500_pwm_probe(struct platform_device *pdev)
 
 	chip->chip.dev = &pdev->dev;
 	chip->chip.ops = &vt8500_pwm_ops;
-	chip->chip.of_xlate = of_pwm_xlate_with_flags;
-	chip->chip.of_pwm_n_cells = 3;
 	chip->chip.base = -1;
 	chip->chip.npwm = VT8500_NR_PWMS;
 
diff --git a/drivers/pwm/pwm-zx.c b/drivers/pwm/pwm-zx.c
index 5d27c16edfb1..201b6f540ad6 100644
--- a/drivers/pwm/pwm-zx.c
+++ b/drivers/pwm/pwm-zx.c
@@ -228,8 +228,6 @@ static int zx_pwm_probe(struct platform_device *pdev)
 	zpc->chip.ops = &zx_pwm_ops;
 	zpc->chip.base = -1;
 	zpc->chip.npwm = 4;
-	zpc->chip.of_xlate = of_pwm_xlate_with_flags;
-	zpc->chip.of_pwm_n_cells = 3;
 
 	/*
 	 * PWM devices may be enabled by firmware, and let's disable all of
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 56518adc31dd..4bb628b94d88 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -48,6 +48,18 @@ enum {
 	PWMF_EXPORTED = 1 << 1,
 };
 
+/**
+ * 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_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,
+};
+
 /*
  * struct pwm_state - state of a PWM channel
  * @period: PWM period (in nanoseconds)
@@ -286,8 +298,6 @@ struct pwm_ops {
  * @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
- * @of_xlate: request a PWM device given a device tree PWM specifier
- * @of_pwm_n_cells: number of cells expected in the device tree PWM specifier
  */
 struct pwm_chip {
 	struct device *dev;
@@ -295,12 +305,7 @@ struct pwm_chip {
 	const struct pwm_ops *ops;
 	int base;
 	unsigned int npwm;
-
 	struct pwm_device *pwms;
-
-	struct pwm_device * (*of_xlate)(struct pwm_chip *pc,
-					const struct of_phandle_args *args);
-	unsigned int of_pwm_n_cells;
 };
 
 /**
@@ -438,8 +443,8 @@ struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
 					 unsigned int index,
 					 const char *label);
 
-struct pwm_device *of_pwm_xlate_with_flags(struct pwm_chip *pc,
-		const struct of_phandle_args *args);
+struct pwm_device *of_pwm_xlate(struct pwm_chip *pc,
+				const struct of_phandle_args *args);
 
 struct pwm_device *pwm_get(struct device *dev, const char *con_id);
 struct pwm_device *of_pwm_get(struct device_node *np, const char *con_id);
-- 
2.7.4

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [PATCH v2 02/16] pwm: pxa: update documentation regarding pwm-cells
  2018-01-12 14:22 [PATCH v2 00/16] extend PWM framework to support PWM modes Claudiu Beznea
  2018-01-12 14:22 ` [PATCH v2 01/16] drivers: pwm: core: use a single of xlate function Claudiu Beznea
@ 2018-01-12 14:22 ` Claudiu Beznea
  2018-01-19 22:30   ` Rob Herring
  2018-01-12 14:22 ` [PATCH v2 04/16] pwm: clps711x: " Claudiu Beznea
                   ` (9 subsequent siblings)
  11 siblings, 1 reply; 42+ messages in thread
From: Claudiu Beznea @ 2018-01-12 14:22 UTC (permalink / raw)
  To: thierry.reding, robh+dt, mark.rutland, linux, daniel,
	haojian.zhuang, robert.jarzmik, corbet, nicolas.ferre,
	alexandre.belloni
  Cc: linux-pwm, Mike Dunn, devicetree, linux-kernel, linux-rockchip,
	linux-rpi-kernel, linux-amlogic, Claudiu Beznea,
	linux-arm-kernel

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

Cc: Mike Dunn <mikedunn@newsguy.com>
Signed-off-by: Claudiu Beznea <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: pwm@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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [PATCH v2 03/16] pwm: cros-ec: update documentation regarding pwm-cells
       [not found] ` <1515766983-15151-1-git-send-email-claudiu.beznea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
@ 2018-01-12 14:22   ` Claudiu Beznea
  2018-01-12 18:31     ` Brian Norris
  2018-01-12 14:22   ` [PATCH v2 06/16] ARM: dts: pxa: update pwm-cells Claudiu Beznea
                     ` (3 subsequent siblings)
  4 siblings, 1 reply; 42+ messages in thread
From: Claudiu Beznea @ 2018-01-12 14:22 UTC (permalink / raw)
  To: thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
	linux-I+IVW8TIWO2tmTQ+vhA3Yw, daniel-cYrQPVfZoowdnm+yROfE0A,
	haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w,
	robert.jarzmik-GANU6spQydw, corbet-T1hC0tSOHrs,
	nicolas.ferre-UWL1GkI3JZL3oGB3hsPCZA,
	alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
  Cc: linux-pwm-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Brian Norris,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Claudiu Beznea,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r

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

Cc: Brian Norris <briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Claudiu Beznea <claudiu.beznea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
---
 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-ec@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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [PATCH v2 04/16] pwm: clps711x: update documentation regarding pwm-cells
  2018-01-12 14:22 [PATCH v2 00/16] extend PWM framework to support PWM modes Claudiu Beznea
  2018-01-12 14:22 ` [PATCH v2 01/16] drivers: pwm: core: use a single of xlate function Claudiu Beznea
  2018-01-12 14:22 ` [PATCH v2 02/16] pwm: pxa: update documentation regarding pwm-cells Claudiu Beznea
@ 2018-01-12 14:22 ` Claudiu Beznea
  2018-01-12 14:22 ` [PATCH v2 05/16] ARM: dts: clps711x: update pwm-cells Claudiu Beznea
                   ` (8 subsequent siblings)
  11 siblings, 0 replies; 42+ messages in thread
From: Claudiu Beznea @ 2018-01-12 14:22 UTC (permalink / raw)
  To: thierry.reding, robh+dt, mark.rutland, linux, daniel,
	haojian.zhuang, robert.jarzmik, corbet, nicolas.ferre,
	alexandre.belloni
  Cc: linux-pwm, linux-kernel, linux-arm-kernel, linux-amlogic,
	linux-rockchip, linux-rpi-kernel, devicetree, Claudiu Beznea,
	Alexander Shiyan

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

Cc: Alexander Shiyan <shc_work@mail.ru>
Signed-off-by: Claudiu Beznea <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: pwm@80000400 {
 		compatible = "cirrus,ep7312-pwm", "cirrus,ep7209-pwm";
 		reg = <0x80000400 0x4>;
 		clocks = <&clks 8>;
-		#pwm-cells = <1>;
+		#pwm-cells = <2>;
 	};
-- 
2.7.4

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [PATCH v2 05/16] ARM: dts: clps711x: update pwm-cells
  2018-01-12 14:22 [PATCH v2 00/16] extend PWM framework to support PWM modes Claudiu Beznea
                   ` (2 preceding siblings ...)
  2018-01-12 14:22 ` [PATCH v2 04/16] pwm: clps711x: " Claudiu Beznea
@ 2018-01-12 14:22 ` Claudiu Beznea
  2018-01-12 14:22 ` [PATCH v2 07/16] arm64: dts: rockchip: " Claudiu Beznea
                   ` (7 subsequent siblings)
  11 siblings, 0 replies; 42+ messages in thread
From: Claudiu Beznea @ 2018-01-12 14:22 UTC (permalink / raw)
  To: thierry.reding, robh+dt, mark.rutland, linux, daniel,
	haojian.zhuang, robert.jarzmik, corbet, nicolas.ferre,
	alexandre.belloni
  Cc: linux-pwm, linux-kernel, linux-arm-kernel, linux-amlogic,
	linux-rockchip, linux-rpi-kernel, devicetree, Claudiu Beznea,
	Alexander Shiyan

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

Cc: Alexander Shiyan <shc_work@mail.ru>
Signed-off-by: Claudiu Beznea <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: uart@80000480 {
-- 
2.7.4

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [PATCH v2 06/16] ARM: dts: pxa: update pwm-cells
       [not found] ` <1515766983-15151-1-git-send-email-claudiu.beznea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
  2018-01-12 14:22   ` [PATCH v2 03/16] pwm: cros-ec: update documentation regarding pwm-cells Claudiu Beznea
@ 2018-01-12 14:22   ` Claudiu Beznea
  2018-01-12 14:22   ` [PATCH v2 09/16] drivers: pwm: core: add PWM mode to pwm_config() Claudiu Beznea
                     ` (2 subsequent siblings)
  4 siblings, 0 replies; 42+ messages in thread
From: Claudiu Beznea @ 2018-01-12 14:22 UTC (permalink / raw)
  To: thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
	linux-I+IVW8TIWO2tmTQ+vhA3Yw, daniel-cYrQPVfZoowdnm+yROfE0A,
	haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w,
	robert.jarzmik-GANU6spQydw, corbet-T1hC0tSOHrs,
	nicolas.ferre-UWL1GkI3JZL3oGB3hsPCZA,
	alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
  Cc: linux-pwm-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Claudiu Beznea,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r

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

Signed-off-by: Claudiu Beznea <claudiu.beznea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
---
 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: pwm@40b00000 {
 			compatible = "marvell,pxa250-pwm";
 			reg = <0x40b00000 0x10>;
-			#pwm-cells = <1>;
+			#pwm-cells = <2>;
 			clocks = <&clks CLK_PWM0>;
 		};
 
 		pwm1: pwm@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: pwm@40b00000 {
 			compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm";
 			reg = <0x40b00000 0x10>;
-			#pwm-cells = <1>;
+			#pwm-cells = <2>;
 			clocks = <&clks CLK_PWM0>;
 		};
 
 		pwm1: pwm@40b00010 {
 			compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm";
 			reg = <0x40b00010 0x10>;
-			#pwm-cells = <1>;
+			#pwm-cells = <2>;
 			clocks = <&clks CLK_PWM1>;
 		};
 
 		pwm2: pwm@40c00000 {
 			compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm";
 			reg = <0x40c00000 0x10>;
-			#pwm-cells = <1>;
+			#pwm-cells = <2>;
 			clocks = <&clks CLK_PWM0>;
 		};
 
 		pwm3: pwm@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: pwm@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: pwm@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: pwm@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: pwm@40c00010 {
 			compatible = "marvell,pxa270-pwm";
 			reg = <0x40c00010 0x10>;
-			#pwm-cells = <1>;
+			#pwm-cells = <2>;
 			clocks = <&clks CLK_PWM1>;
 			status = "disabled";
 		};
-- 
2.7.4

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [PATCH v2 07/16] arm64: dts: rockchip: update pwm-cells
  2018-01-12 14:22 [PATCH v2 00/16] extend PWM framework to support PWM modes Claudiu Beznea
                   ` (3 preceding siblings ...)
  2018-01-12 14:22 ` [PATCH v2 05/16] ARM: dts: clps711x: update pwm-cells Claudiu Beznea
@ 2018-01-12 14:22 ` Claudiu Beznea
  2018-01-12 14:22 ` [PATCH v2 08/16] drivers: pwm: core: extend PWM framework with PWM modes Claudiu Beznea
                   ` (6 subsequent siblings)
  11 siblings, 0 replies; 42+ messages in thread
From: Claudiu Beznea @ 2018-01-12 14:22 UTC (permalink / raw)
  To: thierry.reding, robh+dt, mark.rutland, linux, daniel,
	haojian.zhuang, robert.jarzmik, corbet, nicolas.ferre,
	alexandre.belloni
  Cc: linux-pwm, linux-kernel, linux-arm-kernel, linux-amlogic,
	linux-rockchip, linux-rpi-kernel, devicetree, Claudiu Beznea,
	Brian Norris

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

Cc: Brian Norris <briannorris@chromium.org>
Signed-off-by: Claudiu Beznea <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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [PATCH v2 08/16] drivers: pwm: core: extend PWM framework with PWM modes
  2018-01-12 14:22 [PATCH v2 00/16] extend PWM framework to support PWM modes Claudiu Beznea
                   ` (4 preceding siblings ...)
  2018-01-12 14:22 ` [PATCH v2 07/16] arm64: dts: rockchip: " Claudiu Beznea
@ 2018-01-12 14:22 ` Claudiu Beznea
  2018-01-12 14:22 ` [PATCH v2 10/16] pwm: Add " Claudiu Beznea
                   ` (5 subsequent siblings)
  11 siblings, 0 replies; 42+ messages in thread
From: Claudiu Beznea @ 2018-01-12 14:22 UTC (permalink / raw)
  To: thierry.reding, robh+dt, mark.rutland, linux, daniel,
	haojian.zhuang, robert.jarzmik, corbet, nicolas.ferre,
	alexandre.belloni
  Cc: linux-pwm, linux-kernel, linux-arm-kernel, linux-amlogic,
	linux-rockchip, linux-rpi-kernel, devicetree, Claudiu Beznea

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 <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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [PATCH v2 09/16] drivers: pwm: core: add PWM mode to pwm_config()
       [not found] ` <1515766983-15151-1-git-send-email-claudiu.beznea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
  2018-01-12 14:22   ` [PATCH v2 03/16] pwm: cros-ec: update documentation regarding pwm-cells Claudiu Beznea
  2018-01-12 14:22   ` [PATCH v2 06/16] ARM: dts: pxa: update pwm-cells Claudiu Beznea
@ 2018-01-12 14:22   ` Claudiu Beznea
  2018-01-12 14:22   ` [PATCH v2 11/16] pwm: add documentation for PWM modes Claudiu Beznea
  2018-01-12 14:23   ` [PATCH v2 13/16] drivers: pwm: core: add push-pull mode support Claudiu Beznea
  4 siblings, 0 replies; 42+ messages in thread
From: Claudiu Beznea @ 2018-01-12 14:22 UTC (permalink / raw)
  To: thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
	linux-I+IVW8TIWO2tmTQ+vhA3Yw, daniel-cYrQPVfZoowdnm+yROfE0A,
	haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w,
	robert.jarzmik-GANU6spQydw, corbet-T1hC0tSOHrs,
	nicolas.ferre-UWL1GkI3JZL3oGB3hsPCZA,
	alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
  Cc: linux-pwm-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Claudiu Beznea,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r

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

Signed-off-by: Claudiu Beznea <claudiu.beznea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
---
 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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [PATCH v2 10/16] pwm: Add PWM modes
  2018-01-12 14:22 [PATCH v2 00/16] extend PWM framework to support PWM modes Claudiu Beznea
                   ` (5 preceding siblings ...)
  2018-01-12 14:22 ` [PATCH v2 08/16] drivers: pwm: core: extend PWM framework with PWM modes Claudiu Beznea
@ 2018-01-12 14:22 ` Claudiu Beznea
  2018-01-19 22:34   ` Rob Herring
       [not found] ` <1515766983-15151-1-git-send-email-claudiu.beznea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
                   ` (4 subsequent siblings)
  11 siblings, 1 reply; 42+ messages in thread
From: Claudiu Beznea @ 2018-01-12 14:22 UTC (permalink / raw)
  To: thierry.reding, robh+dt, mark.rutland, linux, daniel,
	haojian.zhuang, robert.jarzmik, corbet, nicolas.ferre,
	alexandre.belloni
  Cc: linux-pwm, linux-kernel, linux-arm-kernel, linux-amlogic,
	linux-rockchip, linux-rpi-kernel, devicetree, Claudiu Beznea

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

Signed-off-by: Claudiu Beznea <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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [PATCH v2 11/16] pwm: add documentation for PWM modes
       [not found] ` <1515766983-15151-1-git-send-email-claudiu.beznea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
                     ` (2 preceding siblings ...)
  2018-01-12 14:22   ` [PATCH v2 09/16] drivers: pwm: core: add PWM mode to pwm_config() Claudiu Beznea
@ 2018-01-12 14:22   ` Claudiu Beznea
  2018-01-19 22:39     ` Rob Herring
  2018-01-12 14:23   ` [PATCH v2 13/16] drivers: pwm: core: add push-pull mode support Claudiu Beznea
  4 siblings, 1 reply; 42+ messages in thread
From: Claudiu Beznea @ 2018-01-12 14:22 UTC (permalink / raw)
  To: thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
	linux-I+IVW8TIWO2tmTQ+vhA3Yw, daniel-cYrQPVfZoowdnm+yROfE0A,
	haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w,
	robert.jarzmik-GANU6spQydw, corbet-T1hC0tSOHrs,
	nicolas.ferre-UWL1GkI3JZL3oGB3hsPCZA,
	alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
  Cc: linux-pwm-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Claudiu Beznea

Add documentation for PWM normal and complementary modes.

Signed-off-by: Claudiu Beznea <claudiu.beznea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [PATCH v2 12/16] pwm: atmel: add pwm capabilities
  2018-01-12 14:22 [PATCH v2 00/16] extend PWM framework to support PWM modes Claudiu Beznea
                   ` (7 preceding siblings ...)
       [not found] ` <1515766983-15151-1-git-send-email-claudiu.beznea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
@ 2018-01-12 14:22 ` Claudiu Beznea
  2018-01-12 14:23 ` [PATCH v2 14/16] pwm: add push-pull mode Claudiu Beznea
                   ` (2 subsequent siblings)
  11 siblings, 0 replies; 42+ messages in thread
From: Claudiu Beznea @ 2018-01-12 14:22 UTC (permalink / raw)
  To: thierry.reding, robh+dt, mark.rutland, linux, daniel,
	haojian.zhuang, robert.jarzmik, corbet, nicolas.ferre,
	alexandre.belloni
  Cc: linux-pwm, linux-kernel, linux-arm-kernel, linux-amlogic,
	linux-rockchip, linux-rpi-kernel, devicetree, Claudiu Beznea

Add pwm capabilities to Atmel PWM controllers.

Signed-off-by: Claudiu Beznea <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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [PATCH v2 13/16] drivers: pwm: core: add push-pull mode support
       [not found] ` <1515766983-15151-1-git-send-email-claudiu.beznea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
                     ` (3 preceding siblings ...)
  2018-01-12 14:22   ` [PATCH v2 11/16] pwm: add documentation for PWM modes Claudiu Beznea
@ 2018-01-12 14:23   ` Claudiu Beznea
  4 siblings, 0 replies; 42+ messages in thread
From: Claudiu Beznea @ 2018-01-12 14:23 UTC (permalink / raw)
  To: thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
	linux-I+IVW8TIWO2tmTQ+vhA3Yw, daniel-cYrQPVfZoowdnm+yROfE0A,
	haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w,
	robert.jarzmik-GANU6spQydw, corbet-T1hC0tSOHrs,
	nicolas.ferre-UWL1GkI3JZL3oGB3hsPCZA,
	alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
  Cc: linux-pwm-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Claudiu Beznea

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 <claudiu.beznea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [PATCH v2 14/16] pwm: add push-pull mode
  2018-01-12 14:22 [PATCH v2 00/16] extend PWM framework to support PWM modes Claudiu Beznea
                   ` (8 preceding siblings ...)
  2018-01-12 14:22 ` [PATCH v2 12/16] pwm: atmel: add pwm capabilities Claudiu Beznea
@ 2018-01-12 14:23 ` Claudiu Beznea
  2018-01-12 14:23 ` [PATCH v2 15/16] pwm: add documentation for pwm " Claudiu Beznea
  2018-01-12 14:23 ` [PATCH v2 16/16] pwm: atmel: add push-pull mode support Claudiu Beznea
  11 siblings, 0 replies; 42+ messages in thread
From: Claudiu Beznea @ 2018-01-12 14:23 UTC (permalink / raw)
  To: thierry.reding, robh+dt, mark.rutland, linux, daniel,
	haojian.zhuang, robert.jarzmik, corbet, nicolas.ferre,
	alexandre.belloni
  Cc: linux-pwm, linux-kernel, linux-arm-kernel, linux-amlogic,
	linux-rockchip, linux-rpi-kernel, devicetree, Claudiu Beznea

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

Signed-off-by: Claudiu Beznea <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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [PATCH v2 15/16] pwm: add documentation for pwm push-pull mode
  2018-01-12 14:22 [PATCH v2 00/16] extend PWM framework to support PWM modes Claudiu Beznea
                   ` (9 preceding siblings ...)
  2018-01-12 14:23 ` [PATCH v2 14/16] pwm: add push-pull mode Claudiu Beznea
@ 2018-01-12 14:23 ` Claudiu Beznea
       [not found]   ` <1515766983-15151-16-git-send-email-claudiu.beznea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
  2018-01-12 14:23 ` [PATCH v2 16/16] pwm: atmel: add push-pull mode support Claudiu Beznea
  11 siblings, 1 reply; 42+ messages in thread
From: Claudiu Beznea @ 2018-01-12 14:23 UTC (permalink / raw)
  To: thierry.reding, robh+dt, mark.rutland, linux, daniel,
	haojian.zhuang, robert.jarzmik, corbet, nicolas.ferre,
	alexandre.belloni
  Cc: linux-pwm, linux-kernel, linux-arm-kernel, linux-amlogic,
	linux-rockchip, linux-rpi-kernel, devicetree, Claudiu Beznea

Add documentation for PWM push-pull mode.

Signed-off-by: Claudiu Beznea <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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [PATCH v2 16/16] pwm: atmel: add push-pull mode support
  2018-01-12 14:22 [PATCH v2 00/16] extend PWM framework to support PWM modes Claudiu Beznea
                   ` (10 preceding siblings ...)
  2018-01-12 14:23 ` [PATCH v2 15/16] pwm: add documentation for pwm " Claudiu Beznea
@ 2018-01-12 14:23 ` Claudiu Beznea
  11 siblings, 0 replies; 42+ messages in thread
From: Claudiu Beznea @ 2018-01-12 14:23 UTC (permalink / raw)
  To: thierry.reding, robh+dt, mark.rutland, linux, daniel,
	haojian.zhuang, robert.jarzmik, corbet, nicolas.ferre,
	alexandre.belloni
  Cc: linux-pwm, linux-kernel, linux-arm-kernel, linux-amlogic,
	linux-rockchip, linux-rpi-kernel, devicetree, Claudiu Beznea

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

Signed-off-by: Claudiu Beznea <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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 03/16] pwm: cros-ec: update documentation regarding pwm-cells
  2018-01-12 14:22   ` [PATCH v2 03/16] pwm: cros-ec: update documentation regarding pwm-cells Claudiu Beznea
@ 2018-01-12 18:31     ` Brian Norris
  2018-01-15  9:01       ` Claudiu Beznea
  0 siblings, 1 reply; 42+ messages in thread
From: Brian Norris @ 2018-01-12 18:31 UTC (permalink / raw)
  To: Claudiu Beznea
  Cc: thierry.reding, robh+dt, mark.rutland, linux, daniel,
	haojian.zhuang, robert.jarzmik, corbet, nicolas.ferre,
	alexandre.belloni, linux-pwm, linux-kernel, linux-arm-kernel,
	linux-amlogic, linux-rockchip, linux-rpi-kernel, devicetree

On Fri, Jan 12, 2018 at 04:22:50PM +0200, Claudiu Beznea wrote:
> pwm-cells should be at least 2 to provide channel number and period value.

Nacked-by: Brian Norris <briannorris@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.)

> Cc: Brian Norris <briannorris@chromium.org>
> Signed-off-by: Claudiu Beznea <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.

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

Brian

>  
>  Example:
>  	cros-ec@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
> 

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 01/16] drivers: pwm: core: use a single of xlate function
       [not found]   ` <1515766983-15151-2-git-send-email-claudiu.beznea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
@ 2018-01-12 18:35     ` Brian Norris
       [not found]       ` <20180112183512.GB102880-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
  0 siblings, 1 reply; 42+ messages in thread
From: Brian Norris @ 2018-01-12 18:35 UTC (permalink / raw)
  To: Claudiu Beznea
  Cc: thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
	linux-I+IVW8TIWO2tmTQ+vhA3Yw, daniel-cYrQPVfZoowdnm+yROfE0A,
	haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w,
	robert.jarzmik-GANU6spQydw, corbet-T1hC0tSOHrs,
	nicolas.ferre-UWL1GkI3JZL3oGB3hsPCZA,
	alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
	linux-pwm-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Mike Dunn, Alexander Shiyan

Hi,

On Fri, Jan 12, 2018 at 04:22:48PM +0200, Claudiu Beznea wrote:
> 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).
> 
> Cc: Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Mike Dunn <mikedunn-kFrNdAxtuftBDgjK7y7TUQ@public.gmane.org>
> Cc: Brian Norris <briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Cc: Alexander Shiyan <shc_work-JGs/UdohzUI@public.gmane.org>
> Signed-off-by: Claudiu Beznea <claudiu.beznea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
> ---
> 
> 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 ---------------

For pwm-cros-ec.c:

Nacked-by: Brian Norris <briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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.

>  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-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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 01/16] drivers: pwm: core: use a single of xlate function
       [not found]       ` <20180112183512.GB102880-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
@ 2018-01-15  8:41         ` Claudiu Beznea
  2018-01-15 12:43           ` Claudiu Beznea
  2018-01-15 20:27           ` Andy Shevchenko
  0 siblings, 2 replies; 42+ messages in thread
From: Claudiu Beznea @ 2018-01-15  8:41 UTC (permalink / raw)
  To: Brian Norris
  Cc: thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
	linux-I+IVW8TIWO2tmTQ+vhA3Yw, daniel-cYrQPVfZoowdnm+yROfE0A,
	haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w,
	robert.jarzmik-GANU6spQydw, corbet-T1hC0tSOHrs,
	nicolas.ferre-UWL1GkI3JZL3oGB3hsPCZA,
	alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
	linux-pwm-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Mike Dunn, Alexander Shiyan

Hi Boris,

Thanks for your review. See below my answers.

On 12.01.2018 20:35, Brian Norris wrote:
> Hi,
> 
> On Fri, Jan 12, 2018 at 04:22:48PM +0200, Claudiu Beznea wrote:
>> 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).
>>
>> Cc: Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Cc: Mike Dunn <mikedunn-kFrNdAxtuftBDgjK7y7TUQ@public.gmane.org>
>> Cc: Brian Norris <briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> Cc: Alexander Shiyan <shc_work-JGs/UdohzUI@public.gmane.org>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
>> ---
>>
>> 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 ---------------
> 
> For pwm-cros-ec.c:
> 
> Nacked-by: Brian Norris <briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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.

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"
and located only:
arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
files) and changed the bindings in this series, as follows, patch 7 from this series:
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
then set pwm->args.period:
-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;
-}

The old flow is as follows:
of_pwm_get() -> cros_ec_pwm_xlate() { request chip and set constant period }

The new flow uses of_pwm_xlate():
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
> 
>>  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-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
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 03/16] pwm: cros-ec: update documentation regarding pwm-cells
  2018-01-12 18:31     ` Brian Norris
@ 2018-01-15  9:01       ` Claudiu Beznea
  2018-01-17  8:29         ` Claudiu Beznea
  0 siblings, 1 reply; 42+ messages in thread
From: Claudiu Beznea @ 2018-01-15  9:01 UTC (permalink / raw)
  To: Brian Norris
  Cc: thierry.reding, robh+dt, mark.rutland, linux, daniel,
	haojian.zhuang, robert.jarzmik, corbet, nicolas.ferre,
	alexandre.belloni, linux-pwm, linux-kernel, linux-arm-kernel,
	linux-amlogic, linux-rockchip, linux-rpi-kernel, devicetree



On 12.01.2018 20:31, Brian Norris wrote:
> On Fri, Jan 12, 2018 at 04:22:50PM +0200, Claudiu Beznea wrote:
>> pwm-cells should be at least 2 to provide channel number and period value.
> 
> Nacked-by: Brian Norris <briannorris@chromium.org>
> 
> 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,
> 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.
> 
>> Cc: Brian Norris <briannorris@chromium.org>
>> Signed-off-by: Claudiu Beznea <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.
> 
> Umm, "2 cells", but you use the singular "cell", and don't document what
> the second one is? That's nonsense.
> 
> Brian
> 
>>  
>>  Example:
>>  	cros-ec@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
>>
> 

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 01/16] drivers: pwm: core: use a single of xlate function
  2018-01-15  8:41         ` Claudiu Beznea
@ 2018-01-15 12:43           ` Claudiu Beznea
  2018-01-15 20:27           ` Andy Shevchenko
  1 sibling, 0 replies; 42+ messages in thread
From: Claudiu Beznea @ 2018-01-15 12:43 UTC (permalink / raw)
  To: Brian Norris
  Cc: mark.rutland, linux-pwm, linux-rpi-kernel, Mike Dunn,
	Alexander Shiyan, corbet, linux-kernel, linux, robh+dt,
	linux-rockchip, devicetree, thierry.reding, alexandre.belloni,
	haojian.zhuang, linux-amlogic, robert.jarzmik, daniel,
	linux-arm-kernel



On 15.01.2018 10:41, Claudiu Beznea wrote:
> Hi Boris,
s/Boris/Brian

> 
> Thanks for your review. See below my answers.
> 
> On 12.01.2018 20:35, Brian Norris wrote:
>> Hi,
>>
>> On Fri, Jan 12, 2018 at 04:22:48PM +0200, Claudiu Beznea wrote:
>>> 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).
>>>
>>> Cc: Thierry Reding <thierry.reding@gmail.com>
>>> Cc: Mike Dunn <mikedunn@newsguy.com>
>>> Cc: Brian Norris <briannorris@chromium.org>
>>> Cc: Alexander Shiyan <shc_work@mail.ru>
>>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>>> ---
>>>
>>> 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 ---------------
>>
>> For pwm-cros-ec.c:
>>
>> Nacked-by: Brian Norris <briannorris@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.
> 
> 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"
> and located only:
> arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
> arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
> files) and changed the bindings in this series, as follows, patch 7 from this series:
> 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
> then set pwm->args.period:
> -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;
> -}
> 
> The old flow is as follows:
> of_pwm_get() -> cros_ec_pwm_xlate() { request chip and set constant period }
> 
> The new flow uses of_pwm_xlate():
> 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
>>
>>>  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-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
>>
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 01/16] drivers: pwm: core: use a single of xlate function
  2018-01-15  8:41         ` Claudiu Beznea
  2018-01-15 12:43           ` Claudiu Beznea
@ 2018-01-15 20:27           ` Andy Shevchenko
       [not found]             ` <CAHp75VcaFDEQ1cBzeU2eBj_vf19ok6EWLC07SOTQLRH8BQSbzA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  1 sibling, 1 reply; 42+ messages in thread
From: Andy Shevchenko @ 2018-01-15 20:27 UTC (permalink / raw)
  To: Claudiu Beznea
  Cc: Brian Norris, Thierry Reding, Rob Herring, Mark Rutland,
	Russell King - ARM Linux, Daniel Mack, Haojian Zhuang,
	Robert Jarzmik, Jonathan Corbet, Nicolas Ferre,
	Alexandre Belloni, linux-pwm, Linux Kernel Mailing List,
	linux-arm Mailing List, linux-amlogic, linux-rockchip,
	linux-rpi-kernel, devicetree

On Mon, Jan 15, 2018 at 10:41 AM, Claudiu Beznea
<Claudiu.Beznea@microchip.com> wrote:

> For this driver, the PWM bindings were changed (I did a grep by "google,cros-ec-pwm"
> and located only:
> arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
> arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
> files) and changed the bindings in this series, as follows, patch 7 from this series:
> 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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 01/16] drivers: pwm: core: use a single of xlate function
       [not found]             ` <CAHp75VcaFDEQ1cBzeU2eBj_vf19ok6EWLC07SOTQLRH8BQSbzA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2018-01-16  8:24               ` Claudiu Beznea
  2018-01-17 23:14                 ` Brian Norris
  0 siblings, 1 reply; 42+ messages in thread
From: Claudiu Beznea @ 2018-01-16  8:24 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Brian Norris, Thierry Reding, Rob Herring, Mark Rutland,
	Russell King - ARM Linux, Daniel Mack, Haojian Zhuang,
	Robert Jarzmik, Jonathan Corbet, Nicolas Ferre,
	Alexandre Belloni, linux-pwm-u79uwXL29TY76Z2rM5mHXA,
	Linux Kernel Mailing List, linux-arm Mailing List,
	linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, devicetree



On 15.01.2018 22:27, Andy Shevchenko wrote:
> On Mon, Jan 15, 2018 at 10:41 AM, Claudiu Beznea
> <Claudiu.Beznea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org> wrote:
> 
>> For this driver, the PWM bindings were changed (I did a grep by "google,cros-ec-pwm"
>> and located only:
>> arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
>> arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
>> files) and changed the bindings in this series, as follows, patch 7 from this series:
>> 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.
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
> 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
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 01/16] drivers: pwm: core: use a single of xlate function
  2018-01-12 14:22 ` [PATCH v2 01/16] drivers: pwm: core: use a single of xlate function Claudiu Beznea
       [not found]   ` <1515766983-15151-2-git-send-email-claudiu.beznea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
@ 2018-01-16  9:07   ` Neil Armstrong
  2018-01-16  9:33     ` Claudiu Beznea
  1 sibling, 1 reply; 42+ messages in thread
From: Neil Armstrong @ 2018-01-16  9:07 UTC (permalink / raw)
  To: Claudiu Beznea, thierry.reding, robh+dt, mark.rutland, linux,
	daniel, haojian.zhuang, robert.jarzmik, corbet, nicolas.ferre,
	alexandre.belloni
  Cc: linux-pwm, Mike Dunn, Alexander Shiyan, devicetree, Brian Norris,
	linux-kernel, linux-rockchip, linux-rpi-kernel, linux-amlogic,
	linux-arm-kernel

On 12/01/2018 15:22, Claudiu Beznea wrote:
> 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).
> 
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Mike Dunn <mikedunn@newsguy.com>
> Cc: Brian Norris <briannorris@chromium.org>
> Cc: Alexander Shiyan <shc_work@mail.ru>
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
> 
> 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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 01/16] drivers: pwm: core: use a single of xlate function
  2018-01-16  9:07   ` Neil Armstrong
@ 2018-01-16  9:33     ` Claudiu Beznea
  0 siblings, 0 replies; 42+ messages in thread
From: Claudiu Beznea @ 2018-01-16  9:33 UTC (permalink / raw)
  To: Neil Armstrong, thierry.reding, robh+dt, mark.rutland, linux,
	daniel, haojian.zhuang, robert.jarzmik, corbet, nicolas.ferre,
	alexandre.belloni
  Cc: linux-pwm, Mike Dunn, Alexander Shiyan, devicetree, Brian Norris,
	linux-kernel, linux-rockchip, linux-rpi-kernel, linux-amlogic,
	linux-arm-kernel



On 16.01.2018 11:07, Neil Armstrong wrote:
> On 12/01/2018 15:22, Claudiu Beznea wrote:
>> 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).
>>
>> Cc: Thierry Reding <thierry.reding@gmail.com>
>> Cc: Mike Dunn <mikedunn@newsguy.com>
>> Cc: Brian Norris <briannorris@chromium.org>
>> Cc: Alexander Shiyan <shc_work@mail.ru>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>> ---
>>
>> 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.

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

> 
> Neil
> 

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 03/16] pwm: cros-ec: update documentation regarding pwm-cells
  2018-01-15  9:01       ` Claudiu Beznea
@ 2018-01-17  8:29         ` Claudiu Beznea
       [not found]           ` <c2078487-8cc6-429e-6c38-092d776c33aa-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
  0 siblings, 1 reply; 42+ messages in thread
From: Claudiu Beznea @ 2018-01-17  8:29 UTC (permalink / raw)
  To: Brian Norris
  Cc: mark.rutland, linux-pwm, linux-rpi-kernel, corbet, linux-kernel,
	linux, robh+dt, linux-rockchip, devicetree, thierry.reding,
	alexandre.belloni, haojian.zhuang, linux-amlogic, robert.jarzmik,
	daniel, linux-arm-kernel



On 15.01.2018 11:01, Claudiu Beznea wrote:
> 
> 
> On 12.01.2018 20:31, Brian Norris wrote:
>> On Fri, Jan 12, 2018 at 04:22:50PM +0200, Claudiu Beznea wrote:
>>> pwm-cells should be at least 2 to provide channel number and period value.
>>
>> Nacked-by: Brian Norris <briannorris@chromium.org>
>>
>> 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,
>> 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.
>>
>>> Cc: Brian Norris <briannorris@chromium.org>
>>> Signed-off-by: Claudiu Beznea <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.
>>
>> Umm, "2 cells", but you use the singular "cell", and don't document what
>> the second one is? That's nonsense.
I didn't saw this comment. The second cell is from the standard PWM binding
as all the other PWM drivers uses. e.g.:
pwms=<&controller PWM-channel PWM-period PWM-flags>

With these changes, if pwm-cells=1 then only PWM-channel will be parsed,
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
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.

Thanks,
Claudiu

>>
>> Brian
>>
>>>  
>>>  Example:
>>>  	cros-ec@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
>>>
>>
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 03/16] pwm: cros-ec: update documentation regarding pwm-cells
       [not found]           ` <c2078487-8cc6-429e-6c38-092d776c33aa-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
@ 2018-01-17 23:10             ` Brian Norris
  2018-01-18  9:18               ` Claudiu Beznea
  0 siblings, 1 reply; 42+ messages in thread
From: Brian Norris @ 2018-01-17 23:10 UTC (permalink / raw)
  To: Claudiu Beznea
  Cc: mark.rutland-5wv7dgnIgG8, linux-pwm-u79uwXL29TY76Z2rM5mHXA,
	linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	corbet-T1hC0tSOHrs, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-I+IVW8TIWO2tmTQ+vhA3Yw, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
	alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
	haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w,
	linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	robert.jarzmik-GANU6spQydw, daniel-cYrQPVfZoowdnm+yROfE0A,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r

On Wed, Jan 17, 2018 at 10:29:53AM +0200, Claudiu Beznea wrote:
> 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.

> 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.

> 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.

Brian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 01/16] drivers: pwm: core: use a single of xlate function
  2018-01-16  8:24               ` Claudiu Beznea
@ 2018-01-17 23:14                 ` Brian Norris
  2018-01-18  9:11                   ` Claudiu Beznea
  0 siblings, 1 reply; 42+ messages in thread
From: Brian Norris @ 2018-01-17 23:14 UTC (permalink / raw)
  To: Claudiu Beznea
  Cc: Andy Shevchenko, Thierry Reding, Rob Herring, Mark Rutland,
	Russell King - ARM Linux, Daniel Mack, Haojian Zhuang,
	Robert Jarzmik, Jonathan Corbet, Nicolas Ferre,
	Alexandre Belloni, linux-pwm, Linux Kernel Mailing List,
	linux-arm Mailing List, linux-amlogic, linux-rockchip,
	linux-rpi-kernel, devicetree

On Tue, Jan 16, 2018 at 10:24:59AM +0200, Claudiu Beznea wrote:
> 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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 01/16] drivers: pwm: core: use a single of xlate function
  2018-01-17 23:14                 ` Brian Norris
@ 2018-01-18  9:11                   ` Claudiu Beznea
  0 siblings, 0 replies; 42+ messages in thread
From: Claudiu Beznea @ 2018-01-18  9:11 UTC (permalink / raw)
  To: Brian Norris
  Cc: Andy Shevchenko, Thierry Reding, Rob Herring, Mark Rutland,
	Russell King - ARM Linux, Daniel Mack, Haojian Zhuang,
	Robert Jarzmik, Jonathan Corbet, Nicolas Ferre,
	Alexandre Belloni, linux-pwm, Linux Kernel Mailing List,
	linux-arm Mailing List, linux-amlogic, linux-rockchip,
	linux-rpi-kernel, devicetree



On 18.01.2018 01:14, Brian Norris wrote:
> On Tue, Jan 16, 2018 at 10:24:59AM +0200, Claudiu Beznea wrote:
>> 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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 03/16] pwm: cros-ec: update documentation regarding pwm-cells
  2018-01-17 23:10             ` Brian Norris
@ 2018-01-18  9:18               ` Claudiu Beznea
  0 siblings, 0 replies; 42+ messages in thread
From: Claudiu Beznea @ 2018-01-18  9:18 UTC (permalink / raw)
  To: Brian Norris
  Cc: mark.rutland, linux-pwm, linux-rpi-kernel, corbet, linux-kernel,
	linux, robh+dt, linux-rockchip, devicetree, thierry.reding,
	alexandre.belloni, haojian.zhuang, linux-amlogic, robert.jarzmik,
	daniel, linux-arm-kernel



On 18.01.2018 01:10, Brian Norris wrote:
> On Wed, Jan 17, 2018 at 10:29:53AM +0200, Claudiu Beznea wrote:
>> 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
> 

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 02/16] pwm: pxa: update documentation regarding pwm-cells
  2018-01-12 14:22 ` [PATCH v2 02/16] pwm: pxa: update documentation regarding pwm-cells Claudiu Beznea
@ 2018-01-19 22:30   ` Rob Herring
  2018-01-22  8:47     ` Claudiu Beznea
  0 siblings, 1 reply; 42+ messages in thread
From: Rob Herring @ 2018-01-19 22:30 UTC (permalink / raw)
  To: Claudiu Beznea
  Cc: thierry.reding, mark.rutland, linux, daniel, haojian.zhuang,
	robert.jarzmik, corbet, nicolas.ferre, alexandre.belloni,
	linux-pwm, linux-kernel, linux-arm-kernel, linux-amlogic,
	linux-rockchip, linux-rpi-kernel, devicetree, Mike Dunn

On Fri, Jan 12, 2018 at 04:22:49PM +0200, Claudiu Beznea wrote:
> pwm-cells should be at least 2 to provide channel number and period value.
> 
> Cc: Mike Dunn <mikedunn@newsguy.com>
> Signed-off-by: Claudiu Beznea <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.

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

Rob

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 10/16] pwm: Add PWM modes
  2018-01-12 14:22 ` [PATCH v2 10/16] pwm: Add " Claudiu Beznea
@ 2018-01-19 22:34   ` Rob Herring
  2018-01-22  8:54     ` Claudiu Beznea
  0 siblings, 1 reply; 42+ messages in thread
From: Rob Herring @ 2018-01-19 22:34 UTC (permalink / raw)
  To: Claudiu Beznea
  Cc: thierry.reding, mark.rutland, linux, daniel, haojian.zhuang,
	robert.jarzmik, corbet, nicolas.ferre, alexandre.belloni,
	linux-pwm, linux-kernel, linux-arm-kernel, linux-amlogic,
	linux-rockchip, linux-rpi-kernel, devicetree

On Fri, Jan 12, 2018 at 04:22:57PM +0200, Claudiu Beznea wrote:
> Define a macros for PWM modes to be used by device tree sources.
> 
> Signed-off-by: Claudiu Beznea <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)

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

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.

> +#define PWM_DTMODE_COMPLEMENTARY		(1 << 1)
> +
>  #endif
> -- 
> 2.7.4
> 

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 11/16] pwm: add documentation for PWM modes
  2018-01-12 14:22   ` [PATCH v2 11/16] pwm: add documentation for PWM modes Claudiu Beznea
@ 2018-01-19 22:39     ` Rob Herring
  2018-01-22  8:55       ` Claudiu Beznea
  0 siblings, 1 reply; 42+ messages in thread
From: Rob Herring @ 2018-01-19 22:39 UTC (permalink / raw)
  To: Claudiu Beznea
  Cc: thierry.reding, mark.rutland, linux, daniel, haojian.zhuang,
	robert.jarzmik, corbet, nicolas.ferre, alexandre.belloni,
	linux-pwm, linux-kernel, linux-arm-kernel, linux-amlogic,
	linux-rockchip, linux-rpi-kernel, devicetree

On Fri, Jan 12, 2018 at 04:22:58PM +0200, Claudiu Beznea wrote:
> Add documentation for PWM normal and complementary modes.

This and the previous patch can be combined.

> 
> Signed-off-by: Claudiu Beznea <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:

Based on the prior patches, "third cell" is probably to restrictive 
here and it should just say "in a flags 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
> 

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 15/16] pwm: add documentation for pwm push-pull mode
       [not found]   ` <1515766983-15151-16-git-send-email-claudiu.beznea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
@ 2018-01-19 22:41     ` Rob Herring
  0 siblings, 0 replies; 42+ messages in thread
From: Rob Herring @ 2018-01-19 22:41 UTC (permalink / raw)
  To: Claudiu Beznea
  Cc: thierry.reding-Re5JQEeQqe8AvxtiuMwx3w, mark.rutland-5wv7dgnIgG8,
	linux-I+IVW8TIWO2tmTQ+vhA3Yw, daniel-cYrQPVfZoowdnm+yROfE0A,
	haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w,
	robert.jarzmik-GANU6spQydw, corbet-T1hC0tSOHrs,
	nicolas.ferre-UWL1GkI3JZL3oGB3hsPCZA,
	alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
	linux-pwm-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA

On Fri, Jan 12, 2018 at 04:23:02PM +0200, Claudiu Beznea wrote:
> Add documentation for PWM push-pull mode.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
> ---
>  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 <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 02/16] pwm: pxa: update documentation regarding pwm-cells
  2018-01-19 22:30   ` Rob Herring
@ 2018-01-22  8:47     ` Claudiu Beznea
  0 siblings, 0 replies; 42+ messages in thread
From: Claudiu Beznea @ 2018-01-22  8:47 UTC (permalink / raw)
  To: Rob Herring
  Cc: thierry.reding-Re5JQEeQqe8AvxtiuMwx3w, mark.rutland-5wv7dgnIgG8,
	linux-I+IVW8TIWO2tmTQ+vhA3Yw, daniel-cYrQPVfZoowdnm+yROfE0A,
	haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w,
	robert.jarzmik-GANU6spQydw, corbet-T1hC0tSOHrs,
	nicolas.ferre-UWL1GkI3JZL3oGB3hsPCZA,
	alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
	linux-pwm-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Mike Dunn



On 20.01.2018 00:30, Rob Herring wrote:
> On Fri, Jan 12, 2018 at 04:22:49PM +0200, Claudiu Beznea wrote:
>> pwm-cells should be at least 2 to provide channel number and period value.
>>
>> Cc: Mike Dunn <mikedunn-kFrNdAxtuftBDgjK7y7TUQ@public.gmane.org>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
>> ---
>>  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.
> 
> 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
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 10/16] pwm: Add PWM modes
  2018-01-19 22:34   ` Rob Herring
@ 2018-01-22  8:54     ` Claudiu Beznea
       [not found]       ` <c5aeb8df-6aa8-cde4-9305-08777cac2f45-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
  0 siblings, 1 reply; 42+ messages in thread
From: Claudiu Beznea @ 2018-01-22  8:54 UTC (permalink / raw)
  To: Rob Herring
  Cc: mark.rutland-5wv7dgnIgG8, linux-pwm-u79uwXL29TY76Z2rM5mHXA,
	corbet-T1hC0tSOHrs, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-I+IVW8TIWO2tmTQ+vhA3Yw,
	haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w,
	nicolas.ferre-UWL1GkI3JZL3oGB3hsPCZA,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
	alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	robert.jarzmik-GANU6spQydw, daniel-cYrQPVfZoowdnm+yROfE0A,
	linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r



On 20.01.2018 00:34, Rob Herring wrote:
> On Fri, Jan 12, 2018 at 04:22:57PM +0200, Claudiu Beznea wrote:
>> Define a macros for PWM modes to be used by device tree sources.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
>> ---
>>  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

> 
>> +#define PWM_DTMODE_COMPLEMENTARY		(1 << 1)
>> +
>>  #endif
>> -- 
>> 2.7.4
>>
> 

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 11/16] pwm: add documentation for PWM modes
  2018-01-19 22:39     ` Rob Herring
@ 2018-01-22  8:55       ` Claudiu Beznea
  0 siblings, 0 replies; 42+ messages in thread
From: Claudiu Beznea @ 2018-01-22  8:55 UTC (permalink / raw)
  To: Rob Herring
  Cc: thierry.reding, mark.rutland, linux, daniel, haojian.zhuang,
	robert.jarzmik, corbet, nicolas.ferre, alexandre.belloni,
	linux-pwm, linux-kernel, linux-arm-kernel, linux-amlogic,
	linux-rockchip, linux-rpi-kernel, devicetree



On 20.01.2018 00:39, Rob Herring wrote:
> On Fri, Jan 12, 2018 at 04:22:58PM +0200, Claudiu Beznea wrote:
>> Add documentation for PWM normal and complementary modes.
> 
> This and the previous patch can be combined.
OK
> 
>>
>> Signed-off-by: Claudiu Beznea <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:
> 
> 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
> 
>>  - 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
>>
> 

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 10/16] pwm: Add PWM modes
       [not found]       ` <c5aeb8df-6aa8-cde4-9305-08777cac2f45-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
@ 2018-01-22 18:12         ` Rob Herring
       [not found]           ` <CAL_JsqJho2OrdnHwRPpYsbNB4RFTq5qSLA=36D2zy=Mi7B8XwQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 42+ messages in thread
From: Rob Herring @ 2018-01-22 18:12 UTC (permalink / raw)
  To: Claudiu Beznea
  Cc: Thierry Reding, Mark Rutland, Russell King, Daniel Mack,
	Haojian Zhuang, Robert Jarzmik, Jonathan Corbet, Nicolas Ferre,
	Alexandre Belloni, Linux PWM List,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	open list:ARM/Rockchip SoC...,
	moderated list:BROADCOM BCM2835 ARM ARCHITECTURE

On Mon, Jan 22, 2018 at 2:54 AM, Claudiu Beznea
<Claudiu.Beznea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org> wrote:
>
>
> On 20.01.2018 00:34, Rob Herring wrote:
>> On Fri, Jan 12, 2018 at 04:22:57PM +0200, Claudiu Beznea wrote:
>>> Define a macros for PWM modes to be used by device tree sources.
>>>
>>> Signed-off-by: Claudiu Beznea <claudiu.beznea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
>>> ---
>>>  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.

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

>> 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

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.

Also complementary mode could be accomplished with a single pwm output
and a board level inverter, right? How would that be handled when the
PWM driver doesn't support that mode?

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 10/16] pwm: Add PWM modes
       [not found]           ` <CAL_JsqJho2OrdnHwRPpYsbNB4RFTq5qSLA=36D2zy=Mi7B8XwQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2018-01-23 10:40             ` Claudiu Beznea
  2018-01-23 15:21               ` Rob Herring
  0 siblings, 1 reply; 42+ messages in thread
From: Claudiu Beznea @ 2018-01-23 10:40 UTC (permalink / raw)
  To: Rob Herring
  Cc: Thierry Reding, Mark Rutland, Russell King, Daniel Mack,
	Haojian Zhuang, Robert Jarzmik, Jonathan Corbet, Nicolas Ferre,
	Alexandre Belloni, Linux PWM List,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	open list:ARM/Rockchip SoC...,
	moderated list:BROADCOM BCM2835 ARM ARCHITECTURE



On 22.01.2018 20:12, Rob Herring wrote:
> On Mon, Jan 22, 2018 at 2:54 AM, Claudiu Beznea
> <Claudiu.Beznea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org> wrote:
>>
>>
>> On 20.01.2018 00:34, Rob Herring wrote:
>>> On Fri, Jan 12, 2018 at 04:22:57PM +0200, Claudiu Beznea wrote:
>>>> Define a macros for PWM modes to be used by device tree sources.
>>>>
>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
>>>> ---
>>>>  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.
> 
> 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.

> 
>>> 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
> 
> 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
in DT bindings, e.g. requested with these bindings:
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.
If the defined modes are:
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).
  
> 
> 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. 

Thank you,
Claudiu Beznea

How would that be handled when the
> PWM driver doesn't support that mode?
> 
> Rob
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 10/16] pwm: Add PWM modes
  2018-01-23 10:40             ` Claudiu Beznea
@ 2018-01-23 15:21               ` Rob Herring
  2018-01-23 16:55                 ` Claudiu Beznea
  0 siblings, 1 reply; 42+ messages in thread
From: Rob Herring @ 2018-01-23 15:21 UTC (permalink / raw)
  To: Claudiu Beznea
  Cc: Thierry Reding, Mark Rutland, Russell King, Daniel Mack,
	Haojian Zhuang, Robert Jarzmik, Jonathan Corbet, Nicolas Ferre,
	Alexandre Belloni, Linux PWM List, linux-kernel,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	linux-amlogic, open list:ARM/Rockchip SoC...,
	moderated list:BROADCOM BCM2835 ARM ARCHITECTURE

On Tue, Jan 23, 2018 at 4:40 AM, Claudiu Beznea
<Claudiu.Beznea@microchip.com> wrote:
>
>
> On 22.01.2018 20:12, Rob Herring wrote:
>> On Mon, Jan 22, 2018 at 2:54 AM, Claudiu Beznea
>> <Claudiu.Beznea@microchip.com> wrote:
>>>
>>>
>>> On 20.01.2018 00:34, Rob Herring wrote:
>>>> On Fri, Jan 12, 2018 at 04:22:57PM +0200, Claudiu Beznea wrote:
>>>>> Define a macros for PWM modes to be used by device tree sources.
>>>>>
>>>>> Signed-off-by: Claudiu Beznea <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)
>>>>
>>>> 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.
>>
>> 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. And we don't want to go
update all the existing users using 3 cells to use 4 cells just to
align.

If the mode needed to be set in the common case, then I might feel
differently about having a separate cell. But these modes seem like a
special case. How many PWM controllers actually support modes like
these?

>>>> 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
>>
>> 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
> in DT bindings, e.g. requested with these bindings:
> 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.
> If the defined modes are:
> 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. It would be
pretty hard to not support normal as that's just ignore the 2nd
signal.

>> 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. 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

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH v2 10/16] pwm: Add PWM modes
  2018-01-23 15:21               ` Rob Herring
@ 2018-01-23 16:55                 ` Claudiu Beznea
  0 siblings, 0 replies; 42+ messages in thread
From: Claudiu Beznea @ 2018-01-23 16:55 UTC (permalink / raw)
  To: Rob Herring
  Cc: Thierry Reding, Mark Rutland, Russell King, Daniel Mack,
	Haojian Zhuang, Robert Jarzmik, Jonathan Corbet, Nicolas Ferre,
	Alexandre Belloni, Linux PWM List, linux-kernel,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	linux-amlogic, open list:ARM/Rockchip SoC...,
	moderated list:BROADCOM BCM2835 ARM ARCHITECTURE



On 23.01.2018 17:21, Rob Herring wrote:
> On Tue, Jan 23, 2018 at 4:40 AM, Claudiu Beznea
> <Claudiu.Beznea@microchip.com> wrote:
>>
>>
>> On 22.01.2018 20:12, Rob Herring wrote:
>>> On Mon, Jan 22, 2018 at 2:54 AM, Claudiu Beznea
>>> <Claudiu.Beznea@microchip.com> wrote:
>>>>
>>>>
>>>> On 20.01.2018 00:34, Rob Herring wrote:
>>>>> On Fri, Jan 12, 2018 at 04:22:57PM +0200, Claudiu Beznea wrote:
>>>>>> Define a macros for PWM modes to be used by device tree sources.
>>>>>>
>>>>>> Signed-off-by: Claudiu Beznea <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)
>>>>>
>>>>> 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.
>>>
>>> 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

> 
>>>>> 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
>>>
>>> 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
>> in DT bindings, e.g. requested with these bindings:
>> 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.
>> If the defined modes are:
>> 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
> 

^ permalink raw reply	[flat|nested] 42+ messages in thread

end of thread, other threads:[~2018-01-23 16:55 UTC | newest]

Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-12 14:22 [PATCH v2 00/16] extend PWM framework to support PWM modes Claudiu Beznea
2018-01-12 14:22 ` [PATCH v2 01/16] drivers: pwm: core: use a single of xlate function Claudiu Beznea
     [not found]   ` <1515766983-15151-2-git-send-email-claudiu.beznea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
2018-01-12 18:35     ` Brian Norris
     [not found]       ` <20180112183512.GB102880-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2018-01-15  8:41         ` Claudiu Beznea
2018-01-15 12:43           ` Claudiu Beznea
2018-01-15 20:27           ` Andy Shevchenko
     [not found]             ` <CAHp75VcaFDEQ1cBzeU2eBj_vf19ok6EWLC07SOTQLRH8BQSbzA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2018-01-16  8:24               ` Claudiu Beznea
2018-01-17 23:14                 ` Brian Norris
2018-01-18  9:11                   ` Claudiu Beznea
2018-01-16  9:07   ` Neil Armstrong
2018-01-16  9:33     ` Claudiu Beznea
2018-01-12 14:22 ` [PATCH v2 02/16] pwm: pxa: update documentation regarding pwm-cells Claudiu Beznea
2018-01-19 22:30   ` Rob Herring
2018-01-22  8:47     ` Claudiu Beznea
2018-01-12 14:22 ` [PATCH v2 04/16] pwm: clps711x: " Claudiu Beznea
2018-01-12 14:22 ` [PATCH v2 05/16] ARM: dts: clps711x: update pwm-cells Claudiu Beznea
2018-01-12 14:22 ` [PATCH v2 07/16] arm64: dts: rockchip: " Claudiu Beznea
2018-01-12 14:22 ` [PATCH v2 08/16] drivers: pwm: core: extend PWM framework with PWM modes Claudiu Beznea
2018-01-12 14:22 ` [PATCH v2 10/16] pwm: Add " Claudiu Beznea
2018-01-19 22:34   ` Rob Herring
2018-01-22  8:54     ` Claudiu Beznea
     [not found]       ` <c5aeb8df-6aa8-cde4-9305-08777cac2f45-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
2018-01-22 18:12         ` Rob Herring
     [not found]           ` <CAL_JsqJho2OrdnHwRPpYsbNB4RFTq5qSLA=36D2zy=Mi7B8XwQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2018-01-23 10:40             ` Claudiu Beznea
2018-01-23 15:21               ` Rob Herring
2018-01-23 16:55                 ` Claudiu Beznea
     [not found] ` <1515766983-15151-1-git-send-email-claudiu.beznea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
2018-01-12 14:22   ` [PATCH v2 03/16] pwm: cros-ec: update documentation regarding pwm-cells Claudiu Beznea
2018-01-12 18:31     ` Brian Norris
2018-01-15  9:01       ` Claudiu Beznea
2018-01-17  8:29         ` Claudiu Beznea
     [not found]           ` <c2078487-8cc6-429e-6c38-092d776c33aa-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
2018-01-17 23:10             ` Brian Norris
2018-01-18  9:18               ` Claudiu Beznea
2018-01-12 14:22   ` [PATCH v2 06/16] ARM: dts: pxa: update pwm-cells Claudiu Beznea
2018-01-12 14:22   ` [PATCH v2 09/16] drivers: pwm: core: add PWM mode to pwm_config() Claudiu Beznea
2018-01-12 14:22   ` [PATCH v2 11/16] pwm: add documentation for PWM modes Claudiu Beznea
2018-01-19 22:39     ` Rob Herring
2018-01-22  8:55       ` Claudiu Beznea
2018-01-12 14:23   ` [PATCH v2 13/16] drivers: pwm: core: add push-pull mode support Claudiu Beznea
2018-01-12 14:22 ` [PATCH v2 12/16] pwm: atmel: add pwm capabilities Claudiu Beznea
2018-01-12 14:23 ` [PATCH v2 14/16] pwm: add push-pull mode Claudiu Beznea
2018-01-12 14:23 ` [PATCH v2 15/16] pwm: add documentation for pwm " Claudiu Beznea
     [not found]   ` <1515766983-15151-16-git-send-email-claudiu.beznea-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
2018-01-19 22:41     ` Rob Herring
2018-01-12 14:23 ` [PATCH v2 16/16] pwm: atmel: add push-pull mode support Claudiu Beznea

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).