linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/2] pwm: visconti: Add Toshiba Visconti SoC PWM support
@ 2021-04-09 23:08 Nobuhiro Iwamatsu
  2021-04-09 23:08 ` [PATCH v4 1/2] dt-bindings: pwm: Add bindings for Toshiba Visconti PWM Controller Nobuhiro Iwamatsu
  2021-04-09 23:08 ` [PATCH v4 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support Nobuhiro Iwamatsu
  0 siblings, 2 replies; 13+ messages in thread
From: Nobuhiro Iwamatsu @ 2021-04-09 23:08 UTC (permalink / raw)
  To: Rob Herring, Thierry Reding, Uwe Kleine-König, Lee Jones
  Cc: devicetree, linux-pwm, punit1.agrawal, yuji2.ishikawa,
	linux-arm-kernel, linux-kernel, Nobuhiro Iwamatsu

Hi,

This series is the PWM driver for Toshiba's ARM SoC, Visconti[0].
This provides DT binding documentation and device driver.

[0]: https://toshiba.semicon-storage.com/ap-en/semiconductor/product/image-recognition-processors-visconti.html

Updates:

  dt-bindings: pwm: Add bindings for Toshiba Visconti PWM Controller
    v3 -> v4:
      - No update.
    v2 -> v3:
      - Change compatible to toshiba,visconti-pwm
      - Change filename to toshiba,visconti-pwm.yaml.
      - Add Reviewed-by tag from Rob.
    v1 -> v2:
      - Change SPDX-License-Identifier to GPL-2.0-only OR BSD-2-Clause.
      - Set compatible toshiba,pwm-visconti only.
      - Drop unnecessary comments.

  pwm: visconti: Add Toshiba Visconti SoC PWM support
    v3 -> v4:
      - Sorted alphabetically include files.
      - Changed container_of to using static inline functions.
      - Dropped unnecessary dev_dbg().
      - Drop Initialization of chip.base.
      - Drop commnet "period too small".
      - Rebased for-next. 
    v2 -> v3:
      - Change compatible to toshiba,visconti-pwm.
      - Fix MODULE_ALIAS to platform:pwm-visconti, again.
      - Align continuation line to the opening parenthesis.
      - Rewrite the contents of visconti_pwm_apply() based on the contents suggested by Uwe.
    v1 -> v2:
      - Change SPDX-License-Identifier to GPL-2.0-only.
      - Add prefix for the register defines.
      - Drop struct device from struct visconti_pwm_chip.
      - Use '>>' instead of '/'.
      - Drop error message by devm_platform_ioremap_resource().
      - Use dev_err_probe instead of dev_err.
      - Change dev_info to dev_dbg.
      - Remove some empty lines.
      - Fix MODULE_ALIAS to platform:pwm-visconti.
      - Add .get_state() function.
      - Use the author name and email address to MODULE_AUTHOR.
      - Add more comment to function of the hardware.
      - Support .get_status() function.
      - Use NSEC_PER_USEC instead of 1000.
      - Alphabetically sorted for Makefile and Kconfig.
      - Added check for set value in visconti_pwm_apply().

Nobuhiro Iwamatsu (2):
  dt-bindings: pwm: Add bindings for Toshiba Visconti PWM Controller
  pwm: visconti: Add Toshiba Visconti SoC PWM support

 .../bindings/pwm/toshiba,pwm-visconti.yaml    |  43 ++++
 drivers/pwm/Kconfig                           |   9 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-visconti.c                    | 188 ++++++++++++++++++
 4 files changed, 241 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml
 create mode 100644 drivers/pwm/pwm-visconti.c

-- 
2.30.0.rc2


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

* [PATCH v4 1/2] dt-bindings: pwm: Add bindings for Toshiba Visconti PWM Controller
  2021-04-09 23:08 [PATCH v4 0/2] pwm: visconti: Add Toshiba Visconti SoC PWM support Nobuhiro Iwamatsu
@ 2021-04-09 23:08 ` Nobuhiro Iwamatsu
  2021-04-09 23:08 ` [PATCH v4 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support Nobuhiro Iwamatsu
  1 sibling, 0 replies; 13+ messages in thread
From: Nobuhiro Iwamatsu @ 2021-04-09 23:08 UTC (permalink / raw)
  To: Rob Herring, Thierry Reding, Uwe Kleine-König, Lee Jones
  Cc: devicetree, linux-pwm, punit1.agrawal, yuji2.ishikawa,
	linux-arm-kernel, linux-kernel, Nobuhiro Iwamatsu, Rob Herring

Add bindings for the Toshiba Visconti PWM Controller.

Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../bindings/pwm/toshiba,pwm-visconti.yaml    | 43 +++++++++++++++++++
 1 file changed, 43 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml

diff --git a/Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml b/Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml
new file mode 100644
index 000000000000..d350f5edfb67
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml
@@ -0,0 +1,43 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/toshiba,pwm-visconti.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Toshiba Visconti PWM Controller
+
+maintainers:
+  - Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
+
+properties:
+  compatible:
+    items:
+      - const: toshiba,visconti-pwm
+
+  reg:
+    maxItems: 1
+
+  '#pwm-cells':
+    const: 2
+
+required:
+  - compatible
+  - reg
+  - '#pwm-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        pwm: pwm@241c0000 {
+            compatible = "toshiba,visconti-pwm";
+            reg = <0 0x241c0000 0 0x1000>;
+            pinctrl-names = "default";
+            pinctrl-0 = <&pwm_mux>;
+            #pwm-cells = <2>;
+        };
+    };
-- 
2.30.0.rc2


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

* [PATCH v4 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support
  2021-04-09 23:08 [PATCH v4 0/2] pwm: visconti: Add Toshiba Visconti SoC PWM support Nobuhiro Iwamatsu
  2021-04-09 23:08 ` [PATCH v4 1/2] dt-bindings: pwm: Add bindings for Toshiba Visconti PWM Controller Nobuhiro Iwamatsu
@ 2021-04-09 23:08 ` Nobuhiro Iwamatsu
  2021-04-10 13:53   ` Uwe Kleine-König
  2021-04-10 18:52   ` Uwe Kleine-König
  1 sibling, 2 replies; 13+ messages in thread
From: Nobuhiro Iwamatsu @ 2021-04-09 23:08 UTC (permalink / raw)
  To: Rob Herring, Thierry Reding, Uwe Kleine-König, Lee Jones
  Cc: devicetree, linux-pwm, punit1.agrawal, yuji2.ishikawa,
	linux-arm-kernel, linux-kernel, Nobuhiro Iwamatsu

Add driver for the PWM controller on Toshiba Visconti ARM SoC.

Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
---
 drivers/pwm/Kconfig        |   9 ++
 drivers/pwm/Makefile       |   1 +
 drivers/pwm/pwm-visconti.c | 188 +++++++++++++++++++++++++++++++++++++
 3 files changed, 198 insertions(+)
 create mode 100644 drivers/pwm/pwm-visconti.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 9a4f66ae8070..8ae68d6203fb 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -601,6 +601,15 @@ config PWM_TWL_LED
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-twl-led.
 
+config PWM_VISCONTI
+	tristate "Toshiba Visconti PWM support"
+	depends on ARCH_VISCONTI || COMPILE_TEST
+	help
+	  PWM Subsystem driver support for Toshiba Visconti SoCs.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-visconti.
+
 config PWM_VT8500
 	tristate "vt8500 PWM support"
 	depends on ARCH_VT8500 || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 6374d3b1d6f3..d43b1e17e8e1 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -56,4 +56,5 @@ obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
 obj-$(CONFIG_PWM_TIEHRPWM)	+= pwm-tiehrpwm.o
 obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
 obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.o
+obj-$(CONFIG_PWM_VISCONTI)	+= pwm-visconti.o
 obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
diff --git a/drivers/pwm/pwm-visconti.c b/drivers/pwm/pwm-visconti.c
new file mode 100644
index 000000000000..99d83f94ed86
--- /dev/null
+++ b/drivers/pwm/pwm-visconti.c
@@ -0,0 +1,188 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Toshiba Visconti pulse-width-modulation controller driver
+ *
+ * Copyright (c) 2020 TOSHIBA CORPORATION
+ * Copyright (c) 2020 Toshiba Electronic Devices & Storage Corporation
+ *
+ * Authors: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
+ *
+ */
+
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+
+#define PIPGM_PCSR(ch) (0x400 + 4 * (ch))
+#define PIPGM_PDUT(ch) (0x420 + 4 * (ch))
+#define PIPGM_PWMC(ch) (0x440 + 4 * (ch))
+
+#define PIPGM_PWMC_PWMACT		BIT(5)
+#define PIPGM_PWMC_CLK_MASK		GENMASK(1, 0)
+#define PIPGM_PWMC_POLARITY_MASK	GENMASK(5, 5)
+
+struct visconti_pwm_chip {
+	struct pwm_chip chip;
+	void __iomem *base;
+};
+
+static inline struct visconti_pwm_chip *to_visconti_chip(struct pwm_chip *chip)
+{
+	return container_of(chip, struct visconti_pwm_chip, chip);
+}
+
+static int visconti_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			      const struct pwm_state *state)
+{
+	struct visconti_pwm_chip *priv = to_visconti_chip(chip);
+	u32 period, duty_cycle, pwmc0;
+
+	/*
+	 * pwmc is a 2-bit divider for the input clock running at 1 MHz.
+	 * When the settings of the PWM are modified, the new values are shadowed in hardware until
+	 * the period register (PCSR) is written and the currently running period is completed. This
+	 * way the hardware switches atomically from the old setting to the new.
+	 * Also, disabling the hardware completes the currently running period and keeps the output
+	 * at low level at all times.
+	 */
+	if (!state->enabled) {
+		writel(0, priv->base + PIPGM_PCSR(pwm->hwpwm));
+		return 0;
+	}
+
+	/*
+	 * The biggest period the hardware can provide is
+	 *	(0xffff << 3) * 1000 ns
+	 * This value fits easily in an u32, so simplify the maths by
+	 * capping the values to 32 bit integers.
+	 */
+	if (state->period > (0xffff << 3) * 1000)
+		period = (0xffff << 3) * 1000;
+	else
+		period = state->period;
+
+	if (state->duty_cycle > period)
+		duty_cycle = period;
+	else
+		duty_cycle = state->duty_cycle;
+
+	/*
+	 * The input clock runs fixed at 1 MHz, so we have only
+	 * microsecond resolution and so can divide by
+	 * NSEC_PER_SEC / CLKFREQ = 1000 without loosing precision.
+	 */
+	period /= 1000;
+	duty_cycle /= 1000;
+
+	if (!period)
+		return -ERANGE;
+
+	/*
+	 * PWMC controls a divider that divides the input clk by a
+	 * power of two between 1 and 8. As a smaller divider yields
+	 * higher precision, pick the smallest possible one.
+	 */
+	if (period > 0xffff) {
+		pwmc0 = ilog2(period >> 16);
+		BUG_ON(pwmc0 > 3);
+	} else
+		pwmc0 = 0;
+
+	period >>= pwmc0;
+	duty_cycle >>= pwmc0;
+
+	if (state->polarity == PWM_POLARITY_INVERSED)
+		pwmc0 |= PIPGM_PWMC_PWMACT;
+	writel(pwmc0, priv->base + PIPGM_PWMC(pwm->hwpwm));
+	writel(duty_cycle, priv->base + PIPGM_PDUT(pwm->hwpwm));
+	writel(period, priv->base + PIPGM_PCSR(pwm->hwpwm));
+
+	return 0;
+}
+
+static void visconti_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+				   struct pwm_state *state)
+{
+	struct visconti_pwm_chip *priv = chip_to_priv(chip);
+	u32 period, duty, pwmc0, pwmc0_clk;
+
+	period = readl(priv->base + PIPGM_PCSR(pwm->hwpwm));
+	if (period)
+		state->enabled = true;
+	else
+		state->enabled = false;
+
+	duty = readl(priv->base + PIPGM_PDUT(pwm->hwpwm));
+	pwmc0 = readl(priv->base + PIPGM_PWMC(pwm->hwpwm));
+	pwmc0_clk = pwmc0 & PIPGM_PWMC_CLK_MASK;
+
+	state->period = (period << pwmc0_clk) * NSEC_PER_USEC;
+	state->duty_cycle = (duty << pwmc0_clk) * NSEC_PER_USEC;
+	if (pwmc0 & PIPGM_PWMC_POLARITY_MASK)
+		state->polarity = PWM_POLARITY_INVERSED;
+	else
+		state->polarity = PWM_POLARITY_NORMAL;
+}
+
+static const struct pwm_ops visconti_pwm_ops = {
+	.apply = visconti_pwm_apply,
+	.get_state = visconti_pwm_get_state,
+	.owner = THIS_MODULE,
+};
+
+static int visconti_pwm_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct visconti_pwm_chip *priv;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	platform_set_drvdata(pdev, priv);
+
+	priv->chip.dev = dev;
+	priv->chip.ops = &visconti_pwm_ops;
+	priv->chip.npwm = 4;
+
+	ret = pwmchip_add(&priv->chip);
+	if (ret < 0)
+		return dev_err_probe(&pdev->dev, ret, "Cannot register visconti PWM\n");
+
+	return 0;
+}
+
+static int visconti_pwm_remove(struct platform_device *pdev)
+{
+	struct visconti_pwm_chip *priv = platform_get_drvdata(pdev);
+
+	return pwmchip_remove(&priv->chip);
+}
+
+static const struct of_device_id visconti_pwm_of_match[] = {
+	{ .compatible = "toshiba,visconti-pwm", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, visconti_pwm_of_match);
+
+static struct platform_driver visconti_pwm_driver = {
+	.driver = {
+		.name = "pwm-visconti",
+		.of_match_table = visconti_pwm_of_match,
+	},
+	.probe = visconti_pwm_probe,
+	.remove = visconti_pwm_remove,
+};
+module_platform_driver(visconti_pwm_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>");
+MODULE_ALIAS("platform:pwm-visconti");
-- 
2.30.0.rc2


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

* Re: [PATCH v4 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support
  2021-04-09 23:08 ` [PATCH v4 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support Nobuhiro Iwamatsu
@ 2021-04-10 13:53   ` Uwe Kleine-König
  2021-04-12  2:55     ` Nobuhiro Iwamatsu
  2021-04-10 18:52   ` Uwe Kleine-König
  1 sibling, 1 reply; 13+ messages in thread
From: Uwe Kleine-König @ 2021-04-10 13:53 UTC (permalink / raw)
  To: Nobuhiro Iwamatsu
  Cc: Rob Herring, Thierry Reding, Lee Jones, devicetree, linux-pwm,
	punit1.agrawal, yuji2.ishikawa, linux-arm-kernel, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 4603 bytes --]

Hello,

just a few small details left to criticize ...

On Sat, Apr 10, 2021 at 08:08:37AM +0900, Nobuhiro Iwamatsu wrote:
> diff --git a/drivers/pwm/pwm-visconti.c b/drivers/pwm/pwm-visconti.c
> new file mode 100644
> index 000000000000..99d83f94ed86
> --- /dev/null
> +++ b/drivers/pwm/pwm-visconti.c
> @@ -0,0 +1,188 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Toshiba Visconti pulse-width-modulation controller driver
> + *
> + * Copyright (c) 2020 TOSHIBA CORPORATION
> + * Copyright (c) 2020 Toshiba Electronic Devices & Storage Corporation
> + *
> + * Authors: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
> + *
> + */
> +
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +
> +#define PIPGM_PCSR(ch) (0x400 + 4 * (ch))
> +#define PIPGM_PDUT(ch) (0x420 + 4 * (ch))
> +#define PIPGM_PWMC(ch) (0x440 + 4 * (ch))
> +
> +#define PIPGM_PWMC_PWMACT		BIT(5)
> +#define PIPGM_PWMC_CLK_MASK		GENMASK(1, 0)
> +#define PIPGM_PWMC_POLARITY_MASK	GENMASK(5, 5)
> +
> +struct visconti_pwm_chip {
> +	struct pwm_chip chip;
> +	void __iomem *base;
> +};
> +
> +static inline struct visconti_pwm_chip *to_visconti_chip(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct visconti_pwm_chip, chip);
> +}
> +
> +static int visconti_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			      const struct pwm_state *state)
> +{
> +	struct visconti_pwm_chip *priv = to_visconti_chip(chip);
> +	u32 period, duty_cycle, pwmc0;
> +
> +	/*
> +	 * pwmc is a 2-bit divider for the input clock running at 1 MHz.
> +	 * When the settings of the PWM are modified, the new values are shadowed in hardware until
> +	 * the period register (PCSR) is written and the currently running period is completed. This
> +	 * way the hardware switches atomically from the old setting to the new.
> +	 * Also, disabling the hardware completes the currently running period and keeps the output
> +	 * at low level at all times.

Can you please put a paragraph analogous to the one in pwm-sifive in the
same format. This simplified keeping an overview about the oddities of
the various supported chips.

> +	 */
> +	if (!state->enabled) {
> +		writel(0, priv->base + PIPGM_PCSR(pwm->hwpwm));
> +		return 0;
> +	}
> +
> [...]
> +
> +static void visconti_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +				   struct pwm_state *state)
> +{
> +	struct visconti_pwm_chip *priv = chip_to_priv(chip);
> +	u32 period, duty, pwmc0, pwmc0_clk;
> +
> +	period = readl(priv->base + PIPGM_PCSR(pwm->hwpwm));
> +	if (period)
> +		state->enabled = true;
> +	else
> +		state->enabled = false;

If PIPGM_PCSR is 0 the hardware is still active and setting a new
configuration completes the currently running period, right? Then I
think having always state->enabled = true is a better match.

> +	duty = readl(priv->base + PIPGM_PDUT(pwm->hwpwm));
> +	pwmc0 = readl(priv->base + PIPGM_PWMC(pwm->hwpwm));
> +	pwmc0_clk = pwmc0 & PIPGM_PWMC_CLK_MASK;
> +
> +	state->period = (period << pwmc0_clk) * NSEC_PER_USEC;
> +	state->duty_cycle = (duty << pwmc0_clk) * NSEC_PER_USEC;
> +	if (pwmc0 & PIPGM_PWMC_POLARITY_MASK)
> +		state->polarity = PWM_POLARITY_INVERSED;
> +	else
> +		state->polarity = PWM_POLARITY_NORMAL;
> +}
> +
> [...]
> +
> +static int visconti_pwm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct visconti_pwm_chip *priv;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	priv->chip.dev = dev;
> +	priv->chip.ops = &visconti_pwm_ops;
> +	priv->chip.npwm = 4;
> +
> +	ret = pwmchip_add(&priv->chip);
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret, "Cannot register visconti PWM\n");

Thierry told to have picked up my patch to add the function
devm_pwmchip_add. I just double checked and it didn't made it into his
for-next branch yet. When you respin this series please check if the
patch landed in the mean time and then make use of it.

> +	return 0;
> +}

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

* Re: [PATCH v4 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support
  2021-04-09 23:08 ` [PATCH v4 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support Nobuhiro Iwamatsu
  2021-04-10 13:53   ` Uwe Kleine-König
@ 2021-04-10 18:52   ` Uwe Kleine-König
  2021-04-12  3:04     ` Nobuhiro Iwamatsu
  1 sibling, 1 reply; 13+ messages in thread
From: Uwe Kleine-König @ 2021-04-10 18:52 UTC (permalink / raw)
  To: Nobuhiro Iwamatsu
  Cc: Rob Herring, Thierry Reding, Lee Jones, devicetree, linux-pwm,
	punit1.agrawal, yuji2.ishikawa, linux-arm-kernel, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 555 bytes --]

Hello,

one more comment:

On Sat, Apr 10, 2021 at 08:08:37AM +0900, Nobuhiro Iwamatsu wrote:
> +static inline struct visconti_pwm_chip *to_visconti_chip(struct pwm_chip *chip)

all functions but this one start have the common prefix "visconti_pwm_".
I like the concept of a common prefix and so you could rename this
function to visconti_pwm_from_chip or similar.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

* Re: [PATCH v4 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support
  2021-04-10 13:53   ` Uwe Kleine-König
@ 2021-04-12  2:55     ` Nobuhiro Iwamatsu
  2021-04-12  7:02       ` Uwe Kleine-König
  0 siblings, 1 reply; 13+ messages in thread
From: Nobuhiro Iwamatsu @ 2021-04-12  2:55 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Rob Herring, Thierry Reding, Lee Jones, devicetree, linux-pwm,
	punit1.agrawal, yuji2.ishikawa, linux-arm-kernel, linux-kernel

Hi Uwe,

Thanks for your review.

On Sat, Apr 10, 2021 at 03:53:21PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> just a few small details left to criticize ...
> 
> On Sat, Apr 10, 2021 at 08:08:37AM +0900, Nobuhiro Iwamatsu wrote:
> > diff --git a/drivers/pwm/pwm-visconti.c b/drivers/pwm/pwm-visconti.c
> > new file mode 100644
> > index 000000000000..99d83f94ed86
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-visconti.c
> > @@ -0,0 +1,188 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Toshiba Visconti pulse-width-modulation controller driver
> > + *
> > + * Copyright (c) 2020 TOSHIBA CORPORATION
> > + * Copyright (c) 2020 Toshiba Electronic Devices & Storage Corporation
> > + *
> > + * Authors: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
> > + *
> > + */
> > +
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +
> > +#define PIPGM_PCSR(ch) (0x400 + 4 * (ch))
> > +#define PIPGM_PDUT(ch) (0x420 + 4 * (ch))
> > +#define PIPGM_PWMC(ch) (0x440 + 4 * (ch))
> > +
> > +#define PIPGM_PWMC_PWMACT		BIT(5)
> > +#define PIPGM_PWMC_CLK_MASK		GENMASK(1, 0)
> > +#define PIPGM_PWMC_POLARITY_MASK	GENMASK(5, 5)
> > +
> > +struct visconti_pwm_chip {
> > +	struct pwm_chip chip;
> > +	void __iomem *base;
> > +};
> > +
> > +static inline struct visconti_pwm_chip *to_visconti_chip(struct pwm_chip *chip)
> > +{
> > +	return container_of(chip, struct visconti_pwm_chip, chip);
> > +}
> > +
> > +static int visconti_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > +			      const struct pwm_state *state)
> > +{
> > +	struct visconti_pwm_chip *priv = to_visconti_chip(chip);
> > +	u32 period, duty_cycle, pwmc0;
> > +
> > +	/*
> > +	 * pwmc is a 2-bit divider for the input clock running at 1 MHz.
> > +	 * When the settings of the PWM are modified, the new values are shadowed in hardware until
> > +	 * the period register (PCSR) is written and the currently running period is completed. This
> > +	 * way the hardware switches atomically from the old setting to the new.
> > +	 * Also, disabling the hardware completes the currently running period and keeps the output
> > +	 * at low level at all times.
> 
> Can you please put a paragraph analogous to the one in pwm-sifive in the
> same format. This simplified keeping an overview about the oddities of
> the various supported chips.

OK, I will check pwm-sifive's, and add.

> 
> > +	 */
> > +	if (!state->enabled) {
> > +		writel(0, priv->base + PIPGM_PCSR(pwm->hwpwm));
> > +		return 0;
> > +	}
> > +
> > [...]
> > +
> > +static void visconti_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > +				   struct pwm_state *state)
> > +{
> > +	struct visconti_pwm_chip *priv = chip_to_priv(chip);
> > +	u32 period, duty, pwmc0, pwmc0_clk;
> > +
> > +	period = readl(priv->base + PIPGM_PCSR(pwm->hwpwm));
> > +	if (period)
> > +		state->enabled = true;
> > +	else
> > +		state->enabled = false;
> 
> If PIPGM_PCSR is 0 the hardware is still active and setting a new
> configuration completes the currently running period, right? Then I
> think having always state->enabled = true is a better match.
>
If PIPGM_PCSR is 0, the hardware is stopped. Even if the settings of
other registers are written, the values of other registers will not work
unless PIPGM_PCSR is written.

However, as a logic, if PIPGM_PCSR becomes 0, it is only
visconti_pwm_apply () in this driver,
so I think that this process should always be state-> enabled = true
as you pointed out.
I wil drop this, thanks.


> > +	duty = readl(priv->base + PIPGM_PDUT(pwm->hwpwm));
> > +	pwmc0 = readl(priv->base + PIPGM_PWMC(pwm->hwpwm));
> > +	pwmc0_clk = pwmc0 & PIPGM_PWMC_CLK_MASK;
> > +
> > +	state->period = (period << pwmc0_clk) * NSEC_PER_USEC;
> > +	state->duty_cycle = (duty << pwmc0_clk) * NSEC_PER_USEC;
> > +	if (pwmc0 & PIPGM_PWMC_POLARITY_MASK)
> > +		state->polarity = PWM_POLARITY_INVERSED;
> > +	else
> > +		state->polarity = PWM_POLARITY_NORMAL;
> > +}
> > +
> > [...]
> > +
> > +static int visconti_pwm_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct visconti_pwm_chip *priv;
> > +	int ret;
> > +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(priv->base))
> > +		return PTR_ERR(priv->base);
> > +
> > +	platform_set_drvdata(pdev, priv);
> > +
> > +	priv->chip.dev = dev;
> > +	priv->chip.ops = &visconti_pwm_ops;
> > +	priv->chip.npwm = 4;
> > +
> > +	ret = pwmchip_add(&priv->chip);
> > +	if (ret < 0)
> > +		return dev_err_probe(&pdev->dev, ret, "Cannot register visconti PWM\n");
> 
> Thierry told to have picked up my patch to add the function
> devm_pwmchip_add. I just double checked and it didn't made it into his
> for-next branch yet. When you respin this series please check if the
> patch landed in the mean time and then make use of it.
> 

OKay.

> > +	return 0;
> > +}
> 
> Best regards
> Uwe

Best regards,
  Nobuhiro


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

* Re: [PATCH v4 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support
  2021-04-10 18:52   ` Uwe Kleine-König
@ 2021-04-12  3:04     ` Nobuhiro Iwamatsu
  0 siblings, 0 replies; 13+ messages in thread
From: Nobuhiro Iwamatsu @ 2021-04-12  3:04 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Rob Herring, Thierry Reding, Lee Jones, devicetree, linux-pwm,
	punit1.agrawal, yuji2.ishikawa, linux-arm-kernel, linux-kernel

Hi,

On Sat, Apr 10, 2021 at 08:52:22PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> one more comment:
> 
> On Sat, Apr 10, 2021 at 08:08:37AM +0900, Nobuhiro Iwamatsu wrote:
> > +static inline struct visconti_pwm_chip *to_visconti_chip(struct pwm_chip *chip)
> 
> all functions but this one start have the common prefix "visconti_pwm_".
> I like the concept of a common prefix and so you could rename this
> function to visconti_pwm_from_chip or similar.

OK, I will change. 

> 
> Best regards
> Uwe
> 

Best regards,
  Nobuhiro

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

* Re: [PATCH v4 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support
  2021-04-12  2:55     ` Nobuhiro Iwamatsu
@ 2021-04-12  7:02       ` Uwe Kleine-König
  2021-04-16  8:07         ` Nobuhiro Iwamatsu
  0 siblings, 1 reply; 13+ messages in thread
From: Uwe Kleine-König @ 2021-04-12  7:02 UTC (permalink / raw)
  To: Nobuhiro Iwamatsu
  Cc: Rob Herring, Thierry Reding, Lee Jones, devicetree, linux-pwm,
	punit1.agrawal, yuji2.ishikawa, linux-arm-kernel, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 5339 bytes --]

On Mon, Apr 12, 2021 at 11:55:36AM +0900, Nobuhiro Iwamatsu wrote:
> Hi Uwe,
> 
> Thanks for your review.
> 
> On Sat, Apr 10, 2021 at 03:53:21PM +0200, Uwe Kleine-König wrote:
> > Hello,
> > 
> > just a few small details left to criticize ...
> > 
> > On Sat, Apr 10, 2021 at 08:08:37AM +0900, Nobuhiro Iwamatsu wrote:
> > > diff --git a/drivers/pwm/pwm-visconti.c b/drivers/pwm/pwm-visconti.c
> > > new file mode 100644
> > > index 000000000000..99d83f94ed86
> > > --- /dev/null
> > > +++ b/drivers/pwm/pwm-visconti.c
> > > @@ -0,0 +1,188 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Toshiba Visconti pulse-width-modulation controller driver
> > > + *
> > > + * Copyright (c) 2020 TOSHIBA CORPORATION
> > > + * Copyright (c) 2020 Toshiba Electronic Devices & Storage Corporation
> > > + *
> > > + * Authors: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
> > > + *
> > > + */
> > > +
> > > +#include <linux/err.h>
> > > +#include <linux/io.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/pwm.h>
> > > +
> > > +#define PIPGM_PCSR(ch) (0x400 + 4 * (ch))
> > > +#define PIPGM_PDUT(ch) (0x420 + 4 * (ch))
> > > +#define PIPGM_PWMC(ch) (0x440 + 4 * (ch))
> > > +
> > > +#define PIPGM_PWMC_PWMACT		BIT(5)
> > > +#define PIPGM_PWMC_CLK_MASK		GENMASK(1, 0)
> > > +#define PIPGM_PWMC_POLARITY_MASK	GENMASK(5, 5)
> > > +
> > > +struct visconti_pwm_chip {
> > > +	struct pwm_chip chip;
> > > +	void __iomem *base;
> > > +};
> > > +
> > > +static inline struct visconti_pwm_chip *to_visconti_chip(struct pwm_chip *chip)
> > > +{
> > > +	return container_of(chip, struct visconti_pwm_chip, chip);
> > > +}
> > > +
> > > +static int visconti_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > +			      const struct pwm_state *state)
> > > +{
> > > +	struct visconti_pwm_chip *priv = to_visconti_chip(chip);
> > > +	u32 period, duty_cycle, pwmc0;
> > > +
> > > +	/*
> > > +	 * pwmc is a 2-bit divider for the input clock running at 1 MHz.
> > > +	 * When the settings of the PWM are modified, the new values are shadowed in hardware until
> > > +	 * the period register (PCSR) is written and the currently running period is completed. This
> > > +	 * way the hardware switches atomically from the old setting to the new.
> > > +	 * Also, disabling the hardware completes the currently running period and keeps the output
> > > +	 * at low level at all times.
> > 
> > Can you please put a paragraph analogous to the one in pwm-sifive in the
> > same format. This simplified keeping an overview about the oddities of
> > the various supported chips.
> 
> OK, I will check pwm-sifive's, and add.
> 
> > 
> > > +	 */
> > > +	if (!state->enabled) {
> > > +		writel(0, priv->base + PIPGM_PCSR(pwm->hwpwm));
> > > +		return 0;
> > > +	}
> > > +
> > > [...]
> > > +
> > > +static void visconti_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > > +				   struct pwm_state *state)
> > > +{
> > > +	struct visconti_pwm_chip *priv = chip_to_priv(chip);
> > > +	u32 period, duty, pwmc0, pwmc0_clk;
> > > +
> > > +	period = readl(priv->base + PIPGM_PCSR(pwm->hwpwm));
> > > +	if (period)
> > > +		state->enabled = true;
> > > +	else
> > > +		state->enabled = false;
> > 
> > If PIPGM_PCSR is 0 the hardware is still active and setting a new
> > configuration completes the currently running period, right? Then I
> > think having always state->enabled = true is a better match.
>
> If PIPGM_PCSR is 0, the hardware is stopped. Even if the settings of
> other registers are written, the values of other registers will not work
> unless PIPGM_PCSR is written.

Correct me if I'm wrong, but how I understand it, PCSR is special as the
other registers are shadow until PCSR is written. (And that is
irrespective of which value is active in PCSR or what is written to
PCSR.)
 
> However, as a logic, if PIPGM_PCSR becomes 0, it is only
> visconti_pwm_apply () in this driver,
> so I think that this process should always be state-> enabled = true
> as you pointed out.
> I wil drop this, thanks.

For me the critical (and only) difference between "off" and
"duty cycle = 0" is that when a new configuration is to be applied. In
the "off" state a new period can (and should) start immediately, while
with "duty_cycle = 0" the rising edge should be delayed until the
currently running period is over.[1]

So the thing to do here (IMHO) is:

Iff with PIPGM_PCSR = 0 configuring a new setting (that is finalized
with writing a non-zero value to PIPGM_PCSR) completes the currently
running period, then always assume the PWM as enabled.

And so if the hardware is stopped and the counter is reset when 0 is
written to PIPGM_PCSR, model that as enabled = false.

Best regards
Uwe

[1] In practise this is more difficult because several PWMs don't
complete periods in general. But the hardware under discussion luckily
isn't one of these. And (worse) there are other hardware implementations
where off doesn't emit an inactive level.

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

* Re: [PATCH v4 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support
  2021-04-12  7:02       ` Uwe Kleine-König
@ 2021-04-16  8:07         ` Nobuhiro Iwamatsu
  2021-04-16  9:44           ` Uwe Kleine-König
  0 siblings, 1 reply; 13+ messages in thread
From: Nobuhiro Iwamatsu @ 2021-04-16  8:07 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Rob Herring, Thierry Reding, Lee Jones, devicetree, linux-pwm,
	punit1.agrawal, yuji2.ishikawa, linux-arm-kernel, linux-kernel

Hi,

Thanks for your review.

On Mon, Apr 12, 2021 at 09:02:32AM +0200, Uwe Kleine-König wrote:
> On Mon, Apr 12, 2021 at 11:55:36AM +0900, Nobuhiro Iwamatsu wrote:
> > Hi Uwe,
> > 
> > Thanks for your review.
> > 
> > On Sat, Apr 10, 2021 at 03:53:21PM +0200, Uwe Kleine-König wrote:
> > > Hello,
> > > 
> > > just a few small details left to criticize ...
> > > 
> > > On Sat, Apr 10, 2021 at 08:08:37AM +0900, Nobuhiro Iwamatsu wrote:
> > > > diff --git a/drivers/pwm/pwm-visconti.c b/drivers/pwm/pwm-visconti.c
> > > > new file mode 100644
> > > > index 000000000000..99d83f94ed86
> > > > --- /dev/null
> > > > +++ b/drivers/pwm/pwm-visconti.c
> > > > @@ -0,0 +1,188 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > +/*
> > > > + * Toshiba Visconti pulse-width-modulation controller driver
> > > > + *
> > > > + * Copyright (c) 2020 TOSHIBA CORPORATION
> > > > + * Copyright (c) 2020 Toshiba Electronic Devices & Storage Corporation
> > > > + *
> > > > + * Authors: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
> > > > + *
> > > > + */
> > > > +
> > > > +#include <linux/err.h>
> > > > +#include <linux/io.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/of_device.h>
> > > > +#include <linux/platform_device.h>
> > > > +#include <linux/pwm.h>
> > > > +
> > > > +#define PIPGM_PCSR(ch) (0x400 + 4 * (ch))
> > > > +#define PIPGM_PDUT(ch) (0x420 + 4 * (ch))
> > > > +#define PIPGM_PWMC(ch) (0x440 + 4 * (ch))
> > > > +
> > > > +#define PIPGM_PWMC_PWMACT		BIT(5)
> > > > +#define PIPGM_PWMC_CLK_MASK		GENMASK(1, 0)
> > > > +#define PIPGM_PWMC_POLARITY_MASK	GENMASK(5, 5)
> > > > +
> > > > +struct visconti_pwm_chip {
> > > > +	struct pwm_chip chip;
> > > > +	void __iomem *base;
> > > > +};
> > > > +
> > > > +static inline struct visconti_pwm_chip *to_visconti_chip(struct pwm_chip *chip)
> > > > +{
> > > > +	return container_of(chip, struct visconti_pwm_chip, chip);
> > > > +}
> > > > +
> > > > +static int visconti_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > +			      const struct pwm_state *state)
> > > > +{
> > > > +	struct visconti_pwm_chip *priv = to_visconti_chip(chip);
> > > > +	u32 period, duty_cycle, pwmc0;
> > > > +
> > > > +	/*
> > > > +	 * pwmc is a 2-bit divider for the input clock running at 1 MHz.
> > > > +	 * When the settings of the PWM are modified, the new values are shadowed in hardware until
> > > > +	 * the period register (PCSR) is written and the currently running period is completed. This
> > > > +	 * way the hardware switches atomically from the old setting to the new.
> > > > +	 * Also, disabling the hardware completes the currently running period and keeps the output
> > > > +	 * at low level at all times.
> > > 
> > > Can you please put a paragraph analogous to the one in pwm-sifive in the
> > > same format. This simplified keeping an overview about the oddities of
> > > the various supported chips.
> > 
> > OK, I will check pwm-sifive's, and add.

I will add the following :

 * Limitations:
 * - PIPGM_PWMC is a 2-bit divider (00: 1, 01: 2, 10: 4, 11: 8) for the input
 *   clock running at 1 MHz.
 * - When the settings of the PWM are modified, the new values are shadowed
 *   in hardware until the PIPGM_PCSR register is written and the currently
 *   running period is completed. This way the hardware switches atomically
 *   from the old setting to the new.
 * - Disabling the hardware completes the currently running period and keeps
 *   the output at low level at all times.


> > 
> > > 
> > > > +	 */
> > > > +	if (!state->enabled) {
> > > > +		writel(0, priv->base + PIPGM_PCSR(pwm->hwpwm));
> > > > +		return 0;
> > > > +	}
> > > > +
> > > > [...]
> > > > +
> > > > +static void visconti_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > +				   struct pwm_state *state)
> > > > +{
> > > > +	struct visconti_pwm_chip *priv = chip_to_priv(chip);
> > > > +	u32 period, duty, pwmc0, pwmc0_clk;
> > > > +
> > > > +	period = readl(priv->base + PIPGM_PCSR(pwm->hwpwm));
> > > > +	if (period)
> > > > +		state->enabled = true;
> > > > +	else
> > > > +		state->enabled = false;
> > > 
> > > If PIPGM_PCSR is 0 the hardware is still active and setting a new
> > > configuration completes the currently running period, right? Then I
> > > think having always state->enabled = true is a better match.
> >
> > If PIPGM_PCSR is 0, the hardware is stopped. Even if the settings of
> > other registers are written, the values of other registers will not work
> > unless PIPGM_PCSR is written.
> 
> Correct me if I'm wrong, but how I understand it, PCSR is special as the
> other registers are shadow until PCSR is written. (And that is
> irrespective of which value is active in PCSR or what is written to
> PCSR.)

This is correct.

>  
> > However, as a logic, if PIPGM_PCSR becomes 0, it is only
> > visconti_pwm_apply () in this driver,
> > so I think that this process should always be state-> enabled = true
> > as you pointed out.
> > I wil drop this, thanks.
> 
> For me the critical (and only) difference between "off" and
> "duty cycle = 0" is that when a new configuration is to be applied. In
> the "off" state a new period can (and should) start immediately, while
> with "duty_cycle = 0" the rising edge should be delayed until the
> currently running period is over.[1]
> 
> So the thing to do here (IMHO) is:
> 
> Iff with PIPGM_PCSR = 0 configuring a new setting (that is finalized
> with writing a non-zero value to PIPGM_PCSR) completes the currently
> running period, then always assume the PWM as enabled.

Yes, this device works that way.

> 
> And so if the hardware is stopped and the counter is reset when 0 is
> written to PIPGM_PCSR, model that as enabled = false.

I don't think the current this driver can handle the above.
As far as I can see your comment, I think I need to implement this,
but after writing 0 to PIPGM_PCSR, driver need to confirm that signal has
changed to low level and change state->enabled to false. 
But with this device has no way of knowing that the signal has changed
to low level.

> 
> Best regards
> Uwe
> 
> [1] In practise this is more difficult because several PWMs don't
> complete periods in general. But the hardware under discussion luckily
> isn't one of these. And (worse) there are other hardware implementations
> where off doesn't emit an inactive level.

I see.

Best regards,
  Nobuhiro

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

* Re: [PATCH v4 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support
  2021-04-16  8:07         ` Nobuhiro Iwamatsu
@ 2021-04-16  9:44           ` Uwe Kleine-König
  2021-04-16 12:15             ` Nobuhiro Iwamatsu
  0 siblings, 1 reply; 13+ messages in thread
From: Uwe Kleine-König @ 2021-04-16  9:44 UTC (permalink / raw)
  To: Nobuhiro Iwamatsu
  Cc: Rob Herring, Thierry Reding, Lee Jones, devicetree, linux-pwm,
	punit1.agrawal, yuji2.ishikawa, linux-arm-kernel, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 2129 bytes --]

Hello Nobuhiro,

On Fri, Apr 16, 2021 at 05:07:21PM +0900, Nobuhiro Iwamatsu wrote:
> On Mon, Apr 12, 2021 at 09:02:32AM +0200, Uwe Kleine-König wrote:
> > On Mon, Apr 12, 2021 at 11:55:36AM +0900, Nobuhiro Iwamatsu wrote:
> > > On Sat, Apr 10, 2021 at 03:53:21PM +0200, Uwe Kleine-König wrote:
> > > > Can you please put a paragraph analogous to the one in pwm-sifive in the
> > > > same format. This simplified keeping an overview about the oddities of
> > > > the various supported chips.
> > > 
> > > OK, I will check pwm-sifive's, and add.
> 
> I will add the following :
> 
>  * Limitations:
>  * - PIPGM_PWMC is a 2-bit divider (00: 1, 01: 2, 10: 4, 11: 8) for the input
>  *   clock running at 1 MHz.

I would strip that to:

 - Fixed input clock running at 1 MHz

>  * - When the settings of the PWM are modified, the new values are shadowed
>  *   in hardware until the PIPGM_PCSR register is written and the currently
>  *   running period is completed. This way the hardware switches atomically
>  *   from the old setting to the new.
>  * - Disabling the hardware completes the currently running period and keeps
>  *   the output at low level at all times.

This looks fine.
 
> > For me the critical (and only) difference between "off" and
> > "duty cycle = 0" is that when a new configuration is to be applied. In
> > the "off" state a new period can (and should) start immediately, while
> > with "duty_cycle = 0" the rising edge should be delayed until the
> > currently running period is over.[1]
> > 
> > So the thing to do here (IMHO) is:
> > 
> > Iff with PIPGM_PCSR = 0 configuring a new setting (that is finalized
> > with writing a non-zero value to PIPGM_PCSR) completes the currently
> > running period, then always assume the PWM as enabled.
> 
> Yes, this device works that way.

OK, then please use

	state->enabled = true

unconditionally in visconti_pwm_get_state().

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

* Re: [PATCH v4 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support
  2021-04-16  9:44           ` Uwe Kleine-König
@ 2021-04-16 12:15             ` Nobuhiro Iwamatsu
  2021-04-17 15:50               ` Uwe Kleine-König
  0 siblings, 1 reply; 13+ messages in thread
From: Nobuhiro Iwamatsu @ 2021-04-16 12:15 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Rob Herring, Thierry Reding, Lee Jones, devicetree, linux-pwm,
	punit1.agrawal, yuji2.ishikawa, linux-arm-kernel, linux-kernel

Hi Uwe,

Thanks for your comment.

On Fri, Apr 16, 2021 at 11:44:26AM +0200, Uwe Kleine-König wrote:
> Hello Nobuhiro,
> 
> On Fri, Apr 16, 2021 at 05:07:21PM +0900, Nobuhiro Iwamatsu wrote:
> > On Mon, Apr 12, 2021 at 09:02:32AM +0200, Uwe Kleine-König wrote:
> > > On Mon, Apr 12, 2021 at 11:55:36AM +0900, Nobuhiro Iwamatsu wrote:
> > > > On Sat, Apr 10, 2021 at 03:53:21PM +0200, Uwe Kleine-König wrote:
> > > > > Can you please put a paragraph analogous to the one in pwm-sifive in the
> > > > > same format. This simplified keeping an overview about the oddities of
> > > > > the various supported chips.
> > > > 
> > > > OK, I will check pwm-sifive's, and add.
> > 
> > I will add the following :
> > 
> >  * Limitations:
> >  * - PIPGM_PWMC is a 2-bit divider (00: 1, 01: 2, 10: 4, 11: 8) for the input
> >  *   clock running at 1 MHz.
> 
> I would strip that to:
> 
>  - Fixed input clock running at 1 MHz
> 

OK, I will update.

> >  * - When the settings of the PWM are modified, the new values are shadowed
> >  *   in hardware until the PIPGM_PCSR register is written and the currently
> >  *   running period is completed. This way the hardware switches atomically
> >  *   from the old setting to the new.
> >  * - Disabling the hardware completes the currently running period and keeps
> >  *   the output at low level at all times.
> 
> This looks fine.
>  
> > > For me the critical (and only) difference between "off" and
> > > "duty cycle = 0" is that when a new configuration is to be applied. In
> > > the "off" state a new period can (and should) start immediately, while
> > > with "duty_cycle = 0" the rising edge should be delayed until the
> > > currently running period is over.[1]
> > > 
> > > So the thing to do here (IMHO) is:
> > > 
> > > Iff with PIPGM_PCSR = 0 configuring a new setting (that is finalized
> > > with writing a non-zero value to PIPGM_PCSR) completes the currently
> > > running period, then always assume the PWM as enabled.
> > 
> > Yes, this device works that way.
> 
> OK, then please use
> 
> 	state->enabled = true
> 
> unconditionally in visconti_pwm_get_state().
> 

Please let me check.
If I unconditionally add 'state->enabled = true' to visconti_pwm_get_state(),
state->enabled is set to true because visconti_pwm_get_state() is called when
the device is created (this is when I write the device number to the export of
/sys/class/pwm/pwmchip0 ).
And since PIPGM_PCSR is 0 in this state, the pulse by PWM is not output.
However, I think this means that the device is working as this driver.
Is this correct?

> Best regards
> Uwe
> 

Best regards,
  Nobuhiro

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

* Re: [PATCH v4 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support
  2021-04-16 12:15             ` Nobuhiro Iwamatsu
@ 2021-04-17 15:50               ` Uwe Kleine-König
  2021-04-18 11:07                 ` Nobuhiro Iwamatsu
  0 siblings, 1 reply; 13+ messages in thread
From: Uwe Kleine-König @ 2021-04-17 15:50 UTC (permalink / raw)
  To: Nobuhiro Iwamatsu
  Cc: Rob Herring, Thierry Reding, Lee Jones, devicetree, linux-pwm,
	punit1.agrawal, yuji2.ishikawa, linux-arm-kernel, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1680 bytes --]

Hello Nobuhiro,

On Fri, Apr 16, 2021 at 09:15:23PM +0900, Nobuhiro Iwamatsu wrote:
> > > > For me the critical (and only) difference between "off" and
> > > > "duty cycle = 0" is that when a new configuration is to be applied. In
> > > > the "off" state a new period can (and should) start immediately, while
> > > > with "duty_cycle = 0" the rising edge should be delayed until the
> > > > currently running period is over.[1]
> > > > 
> > > > So the thing to do here (IMHO) is:
> > > > 
> > > > Iff with PIPGM_PCSR = 0 configuring a new setting (that is finalized
> > > > with writing a non-zero value to PIPGM_PCSR) completes the currently
> > > > running period, then always assume the PWM as enabled.
> > > 
> > > Yes, this device works that way.
> > 
> > OK, then please use
> > 
> > 	state->enabled = true
> > 
> > unconditionally in visconti_pwm_get_state().
> 
> Please let me check.
> If I unconditionally add 'state->enabled = true' to visconti_pwm_get_state(),
> state->enabled is set to true because visconti_pwm_get_state() is called when
> the device is created (this is when I write the device number to the export of
> /sys/class/pwm/pwmchip0 ).
> And since PIPGM_PCSR is 0 in this state, the pulse by PWM is not output.

A PWM that is currently configured with .enabled = true and .duty_cycle
= 0 doesn't have a pulse, so this is fine.

> However, I think this means that the device is working as this driver.

I don't understand this sentence.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

* Re: [PATCH v4 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support
  2021-04-17 15:50               ` Uwe Kleine-König
@ 2021-04-18 11:07                 ` Nobuhiro Iwamatsu
  0 siblings, 0 replies; 13+ messages in thread
From: Nobuhiro Iwamatsu @ 2021-04-18 11:07 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Rob Herring, Thierry Reding, Lee Jones, devicetree, linux-pwm,
	punit1.agrawal, yuji2.ishikawa, linux-arm-kernel, linux-kernel

Hi Uwe,

On Sat, Apr 17, 2021 at 05:50:09PM +0200, Uwe Kleine-König wrote:
> Hello Nobuhiro,
> 
> On Fri, Apr 16, 2021 at 09:15:23PM +0900, Nobuhiro Iwamatsu wrote:
> > > > > For me the critical (and only) difference between "off" and
> > > > > "duty cycle = 0" is that when a new configuration is to be applied. In
> > > > > the "off" state a new period can (and should) start immediately, while
> > > > > with "duty_cycle = 0" the rising edge should be delayed until the
> > > > > currently running period is over.[1]
> > > > > 
> > > > > So the thing to do here (IMHO) is:
> > > > > 
> > > > > Iff with PIPGM_PCSR = 0 configuring a new setting (that is finalized
> > > > > with writing a non-zero value to PIPGM_PCSR) completes the currently
> > > > > running period, then always assume the PWM as enabled.
> > > > 
> > > > Yes, this device works that way.
> > > 
> > > OK, then please use
> > > 
> > > 	state->enabled = true
> > > 
> > > unconditionally in visconti_pwm_get_state().
> > 
> > Please let me check.
> > If I unconditionally add 'state->enabled = true' to visconti_pwm_get_state(),
> > state->enabled is set to true because visconti_pwm_get_state() is called when
> > the device is created (this is when I write the device number to the export of
> > /sys/class/pwm/pwmchip0 ).
> > And since PIPGM_PCSR is 0 in this state, the pulse by PWM is not output.
> 
> A PWM that is currently configured with .enabled = true and .duty_cycle
> = 0 doesn't have a pulse, so this is fine.
> 

I understood, thanks.

> > However, I think this means that the device is working as this driver.
> 
> I don't understand this sentence.

The description of the struct pwm_state says "PWM enabled status".
I thought 'state-> enabled' would hold the working state of the hardware.

> 
> Best regards
> Uwe

Best regards,
  Nobuhiro

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

end of thread, other threads:[~2021-04-18 11:08 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-09 23:08 [PATCH v4 0/2] pwm: visconti: Add Toshiba Visconti SoC PWM support Nobuhiro Iwamatsu
2021-04-09 23:08 ` [PATCH v4 1/2] dt-bindings: pwm: Add bindings for Toshiba Visconti PWM Controller Nobuhiro Iwamatsu
2021-04-09 23:08 ` [PATCH v4 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support Nobuhiro Iwamatsu
2021-04-10 13:53   ` Uwe Kleine-König
2021-04-12  2:55     ` Nobuhiro Iwamatsu
2021-04-12  7:02       ` Uwe Kleine-König
2021-04-16  8:07         ` Nobuhiro Iwamatsu
2021-04-16  9:44           ` Uwe Kleine-König
2021-04-16 12:15             ` Nobuhiro Iwamatsu
2021-04-17 15:50               ` Uwe Kleine-König
2021-04-18 11:07                 ` Nobuhiro Iwamatsu
2021-04-10 18:52   ` Uwe Kleine-König
2021-04-12  3:04     ` Nobuhiro Iwamatsu

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