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
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
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,visconti-pwm.yaml | 43 ++++
drivers/pwm/Kconfig | 9 +
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-visconti.c | 193 ++++++++++++++++++
4 files changed, 246 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/toshiba,visconti-pwm.yaml
create mode 100644 drivers/pwm/pwm-visconti.c
--
2.30.0.rc2
Add driver for the PWM controller on Toshiba Visconti ARM SoC.
Signed-off-by: Nobuhiro Iwamatsu <[email protected]>
---
drivers/pwm/Kconfig | 9 ++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-visconti.c | 193 +++++++++++++++++++++++++++++++++++++
3 files changed, 203 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..ff4a5f5b0009
--- /dev/null
+++ b/drivers/pwm/pwm-visconti.c
@@ -0,0 +1,193 @@
+// 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 <[email protected]>
+ *
+ */
+
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/pwm.h>
+#include <linux/platform_device.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;
+};
+
+#define to_visconti_chip(chip) \
+ 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;
+
+ dev_dbg(chip->dev, "%s: ch = %d en = %d p = 0x%llx d = 0x%llx\n", __func__,
+ pwm->hwpwm, state->enabled, state->period, state->duty_cycle);
+
+ /*
+ * 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)
+ /* period too small */
+ 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 = to_visconti_chip(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.base = -1;
+ priv->chip.npwm = 4;
+
+ ret = pwmchip_add(&priv->chip);
+ if (ret < 0)
+ return dev_err_probe(&pdev->dev, ret, "Cannot register visconti PWM\n");
+
+ dev_dbg(&pdev->dev, "visconti PWM registered\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 <[email protected]>");
+MODULE_ALIAS("platform:pwm-visconti");
--
2.30.0.rc2
Add bindings for the Toshiba Visconti PWM Controller.
Signed-off-by: Nobuhiro Iwamatsu <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
.../bindings/pwm/toshiba,visconti-pwm.yaml | 43 +++++++++++++++++++
1 file changed, 43 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/toshiba,visconti-pwm.yaml
diff --git a/Documentation/devicetree/bindings/pwm/toshiba,visconti-pwm.yaml b/Documentation/devicetree/bindings/pwm/toshiba,visconti-pwm.yaml
new file mode 100644
index 000000000000..9e32e7dbb2f9
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/toshiba,visconti-pwm.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,visconti-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Toshiba Visconti PWM Controller
+
+maintainers:
+ - Nobuhiro Iwamatsu <[email protected]>
+
+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
On Fri, Apr 09, 2021 at 06:07:09PM +0900, Nobuhiro Iwamatsu wrote:
> Add driver for the PWM controller on Toshiba Visconti ARM SoC.
>
> Signed-off-by: Nobuhiro Iwamatsu <[email protected]>
> ---
> drivers/pwm/Kconfig | 9 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-visconti.c | 193 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 203 insertions(+)
> create mode 100644 drivers/pwm/pwm-visconti.c
Looks good, but I have a few minor comments, see below.
> 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..ff4a5f5b0009
> --- /dev/null
> +++ b/drivers/pwm/pwm-visconti.c
> @@ -0,0 +1,193 @@
> +// 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 <[email protected]>
> + *
> + */
> +
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/pwm.h>
> +#include <linux/platform_device.h>
Should be sorted alphabetically.
> +
> +#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;
> +};
> +
> +#define to_visconti_chip(chip) \
> + container_of(chip, struct visconti_pwm_chip, chip)
I prefer these to be static inline functions because that tends to give
better error messages than macros. Also, that's what's primarily used in
the PWM drivers, even if there are a couple of outliers.
I'll go fix those up.
> +
> +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;
> +
> + dev_dbg(chip->dev, "%s: ch = %d en = %d p = 0x%llx d = 0x%llx\n", __func__,
> + pwm->hwpwm, state->enabled, state->period, state->duty_cycle);
Don't the trace points work for you?
> +
> + /*
> + * 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)
> + /* period too small */
> + return -ERANGE;
Maybe braces around this so the two-line "block" doesn't look wrong,
even if it actually isn't. Or perhaps put the comment above the check
for the same effect.
Quite frankly, I'd just drop the comment because the code itself is
clear and the comment doesn't add anything.
> +
> + /*
> + * 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 = to_visconti_chip(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.base = -1;
There's no need for this anymore. The current PWM tree will always
assume base = -1.
> + priv->chip.npwm = 4;
> +
> + ret = pwmchip_add(&priv->chip);
> + if (ret < 0)
> + return dev_err_probe(&pdev->dev, ret, "Cannot register visconti PWM\n");
> +
> + dev_dbg(&pdev->dev, "visconti PWM registered\n");
Maybe not the best use of a debug message. There are better ways to
check if a device has successfully bound to a driver than relying on
debug messages.
> +
> + 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);
I think Uwe would prefer this to be done separately because he's working
towards removing the return value from pwmchip_remove() and if we start
ignoring it in new drivers that will make life easier going forward.
So this should just be:
pwmchip_remove(&priv->chip);
return 0;
Thierry
Hi Thierry,
Thanks for your review.
On Fri, Apr 09, 2021 at 03:20:12PM +0200, Thierry Reding wrote:
> On Fri, Apr 09, 2021 at 06:07:09PM +0900, Nobuhiro Iwamatsu wrote:
> > Add driver for the PWM controller on Toshiba Visconti ARM SoC.
> >
> > Signed-off-by: Nobuhiro Iwamatsu <[email protected]>
> > ---
> > drivers/pwm/Kconfig | 9 ++
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-visconti.c | 193 +++++++++++++++++++++++++++++++++++++
> > 3 files changed, 203 insertions(+)
> > create mode 100644 drivers/pwm/pwm-visconti.c
>
> Looks good, but I have a few minor comments, see below.
>
> > 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..ff4a5f5b0009
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-visconti.c
> > @@ -0,0 +1,193 @@
> > +// 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 <[email protected]>
> > + *
> > + */
> > +
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/platform_device.h>
>
> Should be sorted alphabetically.
>
I forgot it, I will fix.
> > +
> > +#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;
> > +};
> > +
> > +#define to_visconti_chip(chip) \
> > + container_of(chip, struct visconti_pwm_chip, chip)
>
> I prefer these to be static inline functions because that tends to give
> better error messages than macros. Also, that's what's primarily used in
> the PWM drivers, even if there are a couple of outliers.
>
> I'll go fix those up.
I see. I will change to use static inline functions..
>
> > +
> > +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;
> > +
> > + dev_dbg(chip->dev, "%s: ch = %d en = %d p = 0x%llx d = 0x%llx\n", __func__,
> > + pwm->hwpwm, state->enabled, state->period, state->duty_cycle);
>
> Don't the trace points work for you?
Yes, we can get this information by using the trace function. I will
drop this.
>
> > +
> > + /*
> > + * 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)
> > + /* period too small */
> > + return -ERANGE;
>
> Maybe braces around this so the two-line "block" doesn't look wrong,
> even if it actually isn't. Or perhaps put the comment above the check
> for the same effect.
I see, it's readability.
>
> Quite frankly, I'd just drop the comment because the code itself is
> clear and the comment doesn't add anything.
OK, I will drop this comment.
>
> > +
> > + /*
> > + * 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 = to_visconti_chip(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.base = -1;
>
> There's no need for this anymore. The current PWM tree will always
> assume base = -1.
I see. I will drop this.
>
> > + priv->chip.npwm = 4;
> > +
> > + ret = pwmchip_add(&priv->chip);
> > + if (ret < 0)
> > + return dev_err_probe(&pdev->dev, ret, "Cannot register visconti PWM\n");
> > +
> > + dev_dbg(&pdev->dev, "visconti PWM registered\n");
>
> Maybe not the best use of a debug message. There are better ways to
> check if a device has successfully bound to a driver than relying on
> debug messages.
I will drop this line. it says there are better way to check, but what is it?
>
> > +
> > + 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);
>
> I think Uwe would prefer this to be done separately because he's working
> towards removing the return value from pwmchip_remove() and if we start
> ignoring it in new drivers that will make life easier going forward.
>
> So this should just be:
>
> pwmchip_remove(&priv->chip);
>
> return 0;
I understand your suggestion.
However, it looks like the pwmchip_remove() hasn't been updated yet.
I will wait for the update of pwmchip_remove.
>
> Thierry
Best regards,
Nobuhiro
Hello,
On Sat, Apr 10, 2021 at 06:34:55AM +0900, Nobuhiro Iwamatsu wrote:
> > > +static int visconti_pwm_remove(struct platform_device *pdev)
> > > +{
> > > + struct visconti_pwm_chip *priv = platform_get_drvdata(pdev);
> > > +
> > > + return pwmchip_remove(&priv->chip);
> >
> > I think Uwe would prefer this to be done separately because he's working
> > towards removing the return value from pwmchip_remove() and if we start
> > ignoring it in new drivers that will make life easier going forward.
> >
> > So this should just be:
> >
> > pwmchip_remove(&priv->chip);
> >
> > return 0;
>
> I understand your suggestion.
> However, it looks like the pwmchip_remove() hasn't been updated yet.
> I will wait for the update of pwmchip_remove.
pwmchip_remove will always return 0 since b2c200e3f2fd which is in v5.3.
So Thierry's suggestion is safe and indeed welcome.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hi,
On Sat, Apr 10, 2021 at 03:56:58PM +0200, Uwe Kleine-K?nig wrote:
> Hello,
>
> On Sat, Apr 10, 2021 at 06:34:55AM +0900, Nobuhiro Iwamatsu wrote:
> > > > +static int visconti_pwm_remove(struct platform_device *pdev)
> > > > +{
> > > > + struct visconti_pwm_chip *priv = platform_get_drvdata(pdev);
> > > > +
> > > > + return pwmchip_remove(&priv->chip);
> > >
> > > I think Uwe would prefer this to be done separately because he's working
> > > towards removing the return value from pwmchip_remove() and if we start
> > > ignoring it in new drivers that will make life easier going forward.
> > >
> > > So this should just be:
> > >
> > > pwmchip_remove(&priv->chip);
> > >
> > > return 0;
> >
> > I understand your suggestion.
> > However, it looks like the pwmchip_remove() hasn't been updated yet.
> > I will wait for the update of pwmchip_remove.
>
> pwmchip_remove will always return 0 since b2c200e3f2fd which is in v5.3.
> So Thierry's suggestion is safe and indeed welcome.
Sorry, I was looking at a different function.
I will fix this.
Best regards,
Nobuhiro