[PATCH 0/2] Add Toshiba Visconti SoC PWM support

[PATCH 0/2] Add Toshiba Visconti SoC PWM support

[Nobuhiro Iwamatsu]

Hi,

This is the PWM driver for Toshiba's ARM SoC, Visconti[0].
This has not yet been included in Linux kernel, but patches have been posted [1]
and some patches have been applied [2].

Since this is a SoC driver, this cannot work by itself, but I have confirmed that it
works with the patch series sent as [1] with DT setting.

Best regards,
  Nobuhiro

[0]: https://toshiba.semicon-storage.com/ap-en/semiconductor/product/image-recognition-processors-visconti.html
[1]: http://lists.infradead.org/pipermail/linux-arm-kernel/2020-September/599678.html
[2]: http://lists.infradead.org/pipermail/linux-arm-kernel/2020-September/600578.html

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    |  48 ++++++
 drivers/pwm/Kconfig                           |   9 ++
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-visconti.c                    | 141 ++++++++++++++++++
 4 files changed, 199 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml
 create mode 100644 drivers/pwm/pwm-visconti.c

-- 
2.27.0


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

[PATCH 1/2] dt-bindings: pwm: Add bindings for Toshiba Visconti PWM Controller

[Nobuhiro Iwamatsu]

Add bindings for the Toshiba Visconti PWM Controller.

Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
---
 .../bindings/pwm/toshiba,pwm-visconti.yaml    | 48 +++++++++++++++++++
 1 file changed, 48 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..9145e9478b41
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml
@@ -0,0 +1,48 @@
+# SPDX-License-Identifier: GPL-2.0
+%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:
+      - enum:
+          - toshiba,pwm-tmpv7708
+      - const: toshiba,pwm-visconti
+
+  reg:
+    # base address and length of the registers block for the PWM.
+    maxItems: 1
+
+  '#pwm-cells':
+    # should be 2. See pwm.yaml in this directory for a description of
+    # the cells format.
+    const: 2
+
+required:
+  - compatible
+  - reg
+  - '#pwm-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        pwm: pwm@241c0000 {
+            compatible = "toshiba,pwm-tmpv7708", "toshiba,pwm-visconti";
+            reg = <0 0x241c0000 0 0x1000>;
+            pinctrl-names = "default";
+            pinctrl-0 = <&pwm_mux>;
+            #pwm-cells = <2>;
+        };
+    };
-- 
2.27.0


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

[PATCH 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support

[Nobuhiro Iwamatsu]

From: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>

Add a 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 | 141 +++++++++++++++++++++++++++++++++++++
 3 files changed, 151 insertions(+)
 create mode 100644 drivers/pwm/pwm-visconti.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index cb8d739067d2..f99d48f74c76 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -533,6 +533,15 @@ config PWM_TIEHRPWM
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-tiehrpwm.
 
+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_TWL
 	tristate "TWL4030/6030 PWM support"
 	depends on TWL4030_CORE
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index a59c710e98c7..ef6dc1af7c17 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -51,6 +51,7 @@ obj-$(CONFIG_PWM_SUN4I)		+= pwm-sun4i.o
 obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
 obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
 obj-$(CONFIG_PWM_TIEHRPWM)	+= pwm-tiehrpwm.o
+obj-$(CONFIG_PWM_VISCONTI)	+= pwm-visconti.o
 obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
 obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.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..601450111166
--- /dev/null
+++ b/drivers/pwm/pwm-visconti.c
@@ -0,0 +1,141 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Toshiba Visconti pulse-width-modulation controller driver
+ *
+ * Copyright (c) 2020 TOSHIBA CORPORATION
+ * Copyright (c) 2020 Toshiba Electronic Devices & Storage Corporation
+ * Copyright (c) 2020 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/pwm.h>
+#include <linux/platform_device.h>
+
+#define PWMC0_PWMACT BIT(5)
+
+#define REG_PCSR(ch) (0x400 + 4 * (ch))
+#define REG_PDUT(ch) (0x420 + 4 * (ch))
+#define REG_PWM0(ch) (0x440 + 4 * (ch))
+
+struct visconti_pwm_chip {
+	struct pwm_chip chip;
+	struct device *dev;
+	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, pwmc0;
+
+	dev_dbg(priv->dev, "%s: ch = %d en = %d p = 0x%llx d = 0x%llx\n", __func__,
+		pwm->hwpwm, state->enabled, state->period, state->duty_cycle);
+	if (state->enabled) {
+		period = state->period / 1000;
+		duty = state->duty_cycle / 1000;
+		if (period < 0x10000)
+			pwmc0 = 0;
+		else if (period < 0x20000)
+			pwmc0 = 1;
+		else if (period < 0x40000)
+			pwmc0 = 2;
+		else if (period < 0x80000)
+			pwmc0 = 3;
+		else
+			return -EINVAL;
+
+		if (pwmc0) {
+			period /= BIT(pwmc0);
+			duty /= BIT(pwmc0);
+		}
+
+		if (state->polarity == PWM_POLARITY_INVERSED)
+			pwmc0 |= PWMC0_PWMACT;
+
+		writel(pwmc0, priv->base + REG_PWM0(pwm->hwpwm));
+		writel(duty, priv->base + REG_PDUT(pwm->hwpwm));
+		writel(period, priv->base + REG_PCSR(pwm->hwpwm));
+	} else {
+		writel(0, priv->base + REG_PCSR(pwm->hwpwm));
+	}
+
+	return 0;
+}
+
+static const struct pwm_ops visconti_pwm_ops = {
+	.apply = visconti_pwm_apply,
+	.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->dev = &pdev->dev;
+
+	priv->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->base)) {
+		dev_err(dev, "unable to map I/O space\n");
+		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) {
+		dev_err(dev, "Cannot register visconti PWM: %d\n", ret);
+		return ret;
+	}
+
+	dev_info(&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,pwm-visconti", },
+	{ }
+};
+
+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("Toshiba");
+MODULE_ALIAS("platform:visconti-pwm");
-- 
2.27.0


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

Re: [PATCH 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support

[Punit Agrawal]

Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> writes:

> From: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
>
> Add a 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 | 141 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 151 insertions(+)
>  create mode 100644 drivers/pwm/pwm-visconti.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index cb8d739067d2..f99d48f74c76 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -533,6 +533,15 @@ config PWM_TIEHRPWM
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-tiehrpwm.
>  
> +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.
> +

The entries in the file seem to be alphabetically sorted. Can you please
move this to the appropriate location.

>  config PWM_TWL
>  	tristate "TWL4030/6030 PWM support"
>  	depends on TWL4030_CORE
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index a59c710e98c7..ef6dc1af7c17 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -51,6 +51,7 @@ obj-$(CONFIG_PWM_SUN4I)		+= pwm-sun4i.o
>  obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
>  obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
>  obj-$(CONFIG_PWM_TIEHRPWM)	+= pwm-tiehrpwm.o
> +obj-$(CONFIG_PWM_VISCONTI)	+= pwm-visconti.o

Same comment as above.

>  obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
>  obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.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..601450111166
> --- /dev/null
> +++ b/drivers/pwm/pwm-visconti.c
> @@ -0,0 +1,141 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Toshiba Visconti pulse-width-modulation controller driver
> + *
> + * Copyright (c) 2020 TOSHIBA CORPORATION
> + * Copyright (c) 2020 Toshiba Electronic Devices & Storage Corporation
> + * Copyright (c) 2020 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/pwm.h>
> +#include <linux/platform_device.h>
> +
> +#define PWMC0_PWMACT BIT(5)
> +
> +#define REG_PCSR(ch) (0x400 + 4 * (ch))
> +#define REG_PDUT(ch) (0x420 + 4 * (ch))
> +#define REG_PWM0(ch) (0x440 + 4 * (ch))
> +
> +struct visconti_pwm_chip {
> +	struct pwm_chip chip;
> +	struct device *dev;

"dev" can be dropped from the structure if ..

> +	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, pwmc0;
> +
> +	dev_dbg(priv->dev, "%s: ch = %d en = %d p = 0x%llx d = 0x%llx\n", __func__,
> +		pwm->hwpwm, state->enabled, state->period, state->duty_cycle);

... the usage here is replaced by "chip->dev" instead of "priv->dev".

> +	if (state->enabled) {
> +		period = state->period / 1000;
> +		duty = state->duty_cycle / 1000;

Does it make sense to replace the constant 1000 here with the macro -
NSEC_PER_USEC?

Also, period and duty_cycle are defined as u64 in the pwm_state
structure. Is there any chance for the values to be truncated due to
them being u32 in the driver?

> +		if (period < 0x10000)
> +			pwmc0 = 0;
> +		else if (period < 0x20000)
> +			pwmc0 = 1;
> +		else if (period < 0x40000)
> +			pwmc0 = 2;
> +		else if (period < 0x80000)
> +			pwmc0 = 3;
> +		else
> +			return -EINVAL;
> +
> +		if (pwmc0) {
> +			period /= BIT(pwmc0);
> +			duty /= BIT(pwmc0);

It would be better to replace division with right-shift operator.

period >>= pwmc0;
duty >>= pwmc0;

> +		}
> +
> +		if (state->polarity == PWM_POLARITY_INVERSED)
> +			pwmc0 |= PWMC0_PWMACT;
> +
> +		writel(pwmc0, priv->base + REG_PWM0(pwm->hwpwm));
> +		writel(duty, priv->base + REG_PDUT(pwm->hwpwm));
> +		writel(period, priv->base + REG_PCSR(pwm->hwpwm));
> +	} else {
> +		writel(0, priv->base + REG_PCSR(pwm->hwpwm));
> +	}

One suggestion - the else condition can be handled first to reduce
indentation for the state->enabled condition,


        if (!state->enabled) {
           ...
           return 0;
        }


        <handle state enabled case>

But so far the driver is simple enough so I'll leave it upto you which
way you prefer.

> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops visconti_pwm_ops = {
> +	.apply = visconti_pwm_apply,
> +	.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->dev = &pdev->dev;
> +
> +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->base)) {
> +		dev_err(dev, "unable to map I/O space\n");
> +		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) {
> +		dev_err(dev, "Cannot register visconti PWM: %d\n", ret);
> +		return ret;
> +	}
> +
> +	dev_info(&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,pwm-visconti", },
> +	{ }
> +};
> +
> +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("Toshiba");

Please use the author name / email here.

> +MODULE_ALIAS("platform:visconti-pwm");

Thanks,
Punit

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

Re: [PATCH 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 6375 bytes --]

Hello,

On Fri, Sep 18, 2020 at 07:31:40AM +0900, Nobuhiro Iwamatsu wrote:
> diff --git a/drivers/pwm/pwm-visconti.c b/drivers/pwm/pwm-visconti.c
> new file mode 100644
> index 000000000000..601450111166
> --- /dev/null
> +++ b/drivers/pwm/pwm-visconti.c
> @@ -0,0 +1,141 @@
> +// SPDX-License-Identifier: GPL-2.0

The SPDX guys deprecated "GPL-2.0" as identifier and recommend
"GPL-2.0-only" instead. As in the kernel both are allowed I prefer the
latter.

> +/*
> + * Toshiba Visconti pulse-width-modulation controller driver
> + *
> + * Copyright (c) 2020 TOSHIBA CORPORATION
> + * Copyright (c) 2020 Toshiba Electronic Devices & Storage Corporation
> + * Copyright (c) 2020 Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
> + *
> + */

If there is a publically available manual, please add a link here.

> +#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 PWMC0_PWMACT BIT(5)
> +
> +#define REG_PCSR(ch) (0x400 + 4 * (ch))
> +#define REG_PDUT(ch) (0x420 + 4 * (ch))
> +#define REG_PWM0(ch) (0x440 + 4 * (ch))

Please us a prefix for the register defines. Also it would be great if
it would be obvious from the naming to which register the PWMACT bit
belongs.

> +struct visconti_pwm_chip {
> +	struct pwm_chip chip;
> +	struct device *dev;
> +	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, pwmc0;
> +
> +	dev_dbg(priv->dev, "%s: ch = %d en = %d p = 0x%llx d = 0x%llx\n", __func__,
> +		pwm->hwpwm, state->enabled, state->period, state->duty_cycle);
> +	if (state->enabled) {
> +		period = state->period / 1000;
> +		duty = state->duty_cycle / 1000;
> +		if (period < 0x10000)
> +			pwmc0 = 0;
> +		else if (period < 0x20000)
> +			pwmc0 = 1;
> +		else if (period < 0x40000)
> +			pwmc0 = 2;
> +		else if (period < 0x80000)
> +			pwmc0 = 3;
> +		else
> +			return -EINVAL;
> +
> +		if (pwmc0) {
> +			period /= BIT(pwmc0);
> +			duty /= BIT(pwmc0);
> +		}

You can drop the if and just make this:

	period <<= pwmc0;
	duty <<= pwmc0;

as this is a noop if pwmc0 is zero.

> +		if (state->polarity == PWM_POLARITY_INVERSED)
> +			pwmc0 |= PWMC0_PWMACT;
> +
> +		writel(pwmc0, priv->base + REG_PWM0(pwm->hwpwm));
> +		writel(duty, priv->base + REG_PDUT(pwm->hwpwm));
> +		writel(period, priv->base + REG_PCSR(pwm->hwpwm));

Some comments about the function of the hardware would be good.
Something like (I assume the optimal hardware here, please adapt to
reality):

	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 then the output drives the inactive state.

(I'm sure however this is wrong because you don't consider
state->polarity in the !state-enabled case.)

If your hardware doesn't behave as indicated please add a Limitations
paragraph at the beginning of the driver as is done for several other
drivers already describing the shortcomings.

> +	} else {
> +		writel(0, priv->base + REG_PCSR(pwm->hwpwm));
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops visconti_pwm_ops = {
> +	.apply = visconti_pwm_apply,

Please implement .get_state(). (And test it using PWM_DEBUG.)

> +	.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->dev = &pdev->dev;

You can better use

	priv->dev = dev;

here. (But I agree to the previous review that it makes little sense to
keep this member in struct visconti_pwm_chip.)

> +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->base)) {
> +		dev_err(dev, "unable to map I/O space\n");

devm_platform_ioremap_resource already emits an error message on failure,
so no need to add another.

> +		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) {
> +		dev_err(dev, "Cannot register visconti PWM: %d\n", ret);

Please use dev_err_probe here or %pe for the error code.

> +		return ret;
> +	}
> +
> +	dev_info(&pdev->dev, "visconti PWM registered\n");

Please degrade this to dev_dbg.

> +	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,pwm-visconti", },
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, visconti_pwm_of_match);

Please drop the empty line before MODULE_DEVICE_TABLE.

> +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);

The empty line before module_platform_driver is also unusual.

> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Toshiba");
> +MODULE_ALIAS("platform:visconti-pwm");

This is wrong; as the driver name is pwm-visconti this should be
MODULE_ALIAS("platform:pwm-visconti");

Best regards
Uwe

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

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH 1/2] dt-bindings: pwm: Add bindings for Toshiba Visconti PWM Controller

[Rob Herring]

On Fri, Sep 18, 2020 at 07:31:39AM +0900, Nobuhiro Iwamatsu wrote:
> Add bindings for the Toshiba Visconti PWM Controller.
> 
> Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
> ---
>  .../bindings/pwm/toshiba,pwm-visconti.yaml    | 48 +++++++++++++++++++
>  1 file changed, 48 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..9145e9478b41
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml
> @@ -0,0 +1,48 @@
> +# SPDX-License-Identifier: GPL-2.0

Dual license new bindings please.

(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:
> +      - enum:
> +          - toshiba,pwm-tmpv7708

The normal order is: vendor,soc-block

> +      - const: toshiba,pwm-visconti

Do you expect a lot of chips with the exact same version of the IP? If 
not drop. Future chips can always use toshiba,pwm-tmpv7708 as a 
fallback.

> +
> +  reg:
> +    # base address and length of the registers block for the PWM.

Drop. No need to describe common properties.

> +    maxItems: 1
> +
> +  '#pwm-cells':
> +    # should be 2. See pwm.yaml in this directory for a description of
> +    # the cells format.

Drop.

> +    const: 2
> +
> +required:
> +  - compatible
> +  - reg
> +  - '#pwm-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        pwm: pwm@241c0000 {
> +            compatible = "toshiba,pwm-tmpv7708", "toshiba,pwm-visconti";
> +            reg = <0 0x241c0000 0 0x1000>;
> +            pinctrl-names = "default";
> +            pinctrl-0 = <&pwm_mux>;
> +            #pwm-cells = <2>;
> +        };
> +    };
> -- 
> 2.27.0
> 

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

Re: [PATCH 1/2] dt-bindings: pwm: Add bindings for Toshiba Visconti PWM Controller

[Nobuhiro Iwamatsu]

Hi,

Thank for your review.

On Wed, Sep 23, 2020 at 02:37:35PM -0600, Rob Herring wrote:
> On Fri, Sep 18, 2020 at 07:31:39AM +0900, Nobuhiro Iwamatsu wrote:
> > Add bindings for the Toshiba Visconti PWM Controller.
> > 
> > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
> > ---
> >  .../bindings/pwm/toshiba,pwm-visconti.yaml    | 48 +++++++++++++++++++
> >  1 file changed, 48 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..9145e9478b41
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml
> > @@ -0,0 +1,48 @@
> > +# SPDX-License-Identifier: GPL-2.0
> 
> Dual license new bindings please.
> 
> (GPL-2.0-only OR BSD-2-Clause)
> 

OK, I will chnage to dual license.

> > +%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:
> > +      - enum:
> > +          - toshiba,pwm-tmpv7708
> 
> The normal order is: vendor,soc-block
> 
> > +      - const: toshiba,pwm-visconti
> 
> Do you expect a lot of chips with the exact same version of the IP? If 
> not drop. Future chips can always use toshiba,pwm-tmpv7708 as a 
> fallback.


Currently it still supports only one IP. Therefore, "toshiba, pwm-visconti"
is enough for now. I will drop enum line..

> 
> > +
> > +  reg:
> > +    # base address and length of the registers block for the PWM.
> 
> Drop. No need to describe common properties.
> 

OK, I will drop this properties.

> > +    maxItems: 1
> > +
> > +  '#pwm-cells':
> > +    # should be 2. See pwm.yaml in this directory for a description of
> > +    # the cells format.
> 
> Drop.

OK, I will this comment lines.

> 
> > +    const: 2
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - '#pwm-cells'
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    soc {
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        pwm: pwm@241c0000 {
> > +            compatible = "toshiba,pwm-tmpv7708", "toshiba,pwm-visconti";
> > +            reg = <0 0x241c0000 0 0x1000>;
> > +            pinctrl-names = "default";
> > +            pinctrl-0 = <&pwm_mux>;
> > +            #pwm-cells = <2>;
> > +        };
> > +    };
> > -- 
> > 2.27.0
> > 
> 

Best regards,
  Nobuhiro


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

Re: [PATCH 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support

[Nobuhiro Iwamatsu]

Hi, 

Thanks for your review.

On Mon, Sep 21, 2020 at 06:00:26PM +0900, Punit Agrawal wrote:
> Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> writes:
> 
> > From: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
> >
> > Add a 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 | 141 +++++++++++++++++++++++++++++++++++++
> >  3 files changed, 151 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-visconti.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index cb8d739067d2..f99d48f74c76 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -533,6 +533,15 @@ config PWM_TIEHRPWM
> >  	  To compile this driver as a module, choose M here: the module
> >  	  will be called pwm-tiehrpwm.
> >  
> > +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.
> > +
> 
> The entries in the file seem to be alphabetically sorted. Can you please
> move this to the appropriate location.
> 

OK, I will this.

> >  config PWM_TWL
> >  	tristate "TWL4030/6030 PWM support"
> >  	depends on TWL4030_CORE
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index a59c710e98c7..ef6dc1af7c17 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -51,6 +51,7 @@ obj-$(CONFIG_PWM_SUN4I)		+= pwm-sun4i.o
> >  obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
> >  obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
> >  obj-$(CONFIG_PWM_TIEHRPWM)	+= pwm-tiehrpwm.o
> > +obj-$(CONFIG_PWM_VISCONTI)	+= pwm-visconti.o
> 
> Same comment as above.
> 
> >  obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
> >  obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.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..601450111166
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-visconti.c
> > @@ -0,0 +1,141 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Toshiba Visconti pulse-width-modulation controller driver
> > + *
> > + * Copyright (c) 2020 TOSHIBA CORPORATION
> > + * Copyright (c) 2020 Toshiba Electronic Devices & Storage Corporation
> > + * Copyright (c) 2020 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/pwm.h>
> > +#include <linux/platform_device.h>
> > +
> > +#define PWMC0_PWMACT BIT(5)
> > +
> > +#define REG_PCSR(ch) (0x400 + 4 * (ch))
> > +#define REG_PDUT(ch) (0x420 + 4 * (ch))
> > +#define REG_PWM0(ch) (0x440 + 4 * (ch))
> > +
> > +struct visconti_pwm_chip {
> > +	struct pwm_chip chip;
> > +	struct device *dev;
> 
> "dev" can be dropped from the structure if ..
> 

Indeed. I will drop this.

> > +	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, pwmc0;
> > +
> > +	dev_dbg(priv->dev, "%s: ch = %d en = %d p = 0x%llx d = 0x%llx\n", __func__,
> > +		pwm->hwpwm, state->enabled, state->period, state->duty_cycle);
> 
> ... the usage here is replaced by "chip->dev" instead of "priv->dev".
> 

I will fix this line.

> > +	if (state->enabled) {
> > +		period = state->period / 1000;
> > +		duty = state->duty_cycle / 1000;
> 
> Does it make sense to replace the constant 1000 here with the macro -
> NSEC_PER_USEC?


I see. I will change NSEC_PER_USEC instead of 1000.

> 
> Also, period and duty_cycle are defined as u64 in the pwm_state
> structure. Is there any chance for the values to be truncated due to
> them being u32 in the driver?

As you pointed out, the registers of this IP are 32bit.
Therefore, the value may be truncated. I will fix it.


> 
> > +		if (period < 0x10000)
> > +			pwmc0 = 0;
> > +		else if (period < 0x20000)
> > +			pwmc0 = 1;
> > +		else if (period < 0x40000)
> > +			pwmc0 = 2;
> > +		else if (period < 0x80000)
> > +			pwmc0 = 3;
> > +		else
> > +			return -EINVAL;
> > +
> > +		if (pwmc0) {
> > +			period /= BIT(pwmc0);
> > +			duty /= BIT(pwmc0);
> 
> It would be better to replace division with right-shift operator.
> 
> period >>= pwmc0;
> duty >>= pwmc0;

I will change to use shift.

> 
> > +		}
> > +
> > +		if (state->polarity == PWM_POLARITY_INVERSED)
> > +			pwmc0 |= PWMC0_PWMACT;
> > +
> > +		writel(pwmc0, priv->base + REG_PWM0(pwm->hwpwm));
> > +		writel(duty, priv->base + REG_PDUT(pwm->hwpwm));
> > +		writel(period, priv->base + REG_PCSR(pwm->hwpwm));
> > +	} else {
> > +		writel(0, priv->base + REG_PCSR(pwm->hwpwm));
> > +	}
> 
> One suggestion - the else condition can be handled first to reduce
> indentation for the state->enabled condition,
> 
> 
>         if (!state->enabled) {
>            ...
>            return 0;
>         }
> 
> 
>         <handle state enabled case>
> 
> But so far the driver is simple enough so I'll leave it upto you which
> way you prefer.


I see. 
I will change to your suggestion. 

> 
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct pwm_ops visconti_pwm_ops = {
> > +	.apply = visconti_pwm_apply,
> > +	.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->dev = &pdev->dev;
> > +
> > +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(priv->base)) {
> > +		dev_err(dev, "unable to map I/O space\n");
> > +		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) {
> > +		dev_err(dev, "Cannot register visconti PWM: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	dev_info(&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,pwm-visconti", },
> > +	{ }
> > +};
> > +
> > +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("Toshiba");
> 
> Please use the author name / email here.
> 

I see. I will change MODULE_AUTHOR to my name and email.

> > +MODULE_ALIAS("platform:visconti-pwm");
> 
> Thanks,
> Punit
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

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

Re: [PATCH 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support

[Nobuhiro Iwamatsu]

Hi,

Thanks for your review.

On Tue, Sep 22, 2020 at 09:14:09AM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Fri, Sep 18, 2020 at 07:31:40AM +0900, Nobuhiro Iwamatsu wrote:
> > diff --git a/drivers/pwm/pwm-visconti.c b/drivers/pwm/pwm-visconti.c
> > new file mode 100644
> > index 000000000000..601450111166
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-visconti.c
> > @@ -0,0 +1,141 @@
> > +// SPDX-License-Identifier: GPL-2.0
> 
> The SPDX guys deprecated "GPL-2.0" as identifier and recommend
> "GPL-2.0-only" instead. As in the kernel both are allowed I prefer the
> latter.
> 
I see. I will change to GPL-2.0-only.


> > +/*
> > + * Toshiba Visconti pulse-width-modulation controller driver
> > + *
> > + * Copyright (c) 2020 TOSHIBA CORPORATION
> > + * Copyright (c) 2020 Toshiba Electronic Devices & Storage Corporation
> > + * Copyright (c) 2020 Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
> > + *
> > + */
> 
> If there is a publically available manual, please add a link here.
> 

This device's manual is not open yet. If it is opened, I will add.


> > +#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 PWMC0_PWMACT BIT(5)
> > +
> > +#define REG_PCSR(ch) (0x400 + 4 * (ch))
> > +#define REG_PDUT(ch) (0x420 + 4 * (ch))
> > +#define REG_PWM0(ch) (0x440 + 4 * (ch))
> 
> Please us a prefix for the register defines. Also it would be great if
> it would be obvious from the naming to which register the PWMACT bit
> belongs.
> 

I will change this.

> > +struct visconti_pwm_chip {
> > +	struct pwm_chip chip;
> > +	struct device *dev;
> > +	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, pwmc0;
> > +
> > +	dev_dbg(priv->dev, "%s: ch = %d en = %d p = 0x%llx d = 0x%llx\n", __func__,
> > +		pwm->hwpwm, state->enabled, state->period, state->duty_cycle);
> > +	if (state->enabled) {
> > +		period = state->period / 1000;
> > +		duty = state->duty_cycle / 1000;
> > +		if (period < 0x10000)
> > +			pwmc0 = 0;
> > +		else if (period < 0x20000)
> > +			pwmc0 = 1;
> > +		else if (period < 0x40000)
> > +			pwmc0 = 2;
> > +		else if (period < 0x80000)
> > +			pwmc0 = 3;
> > +		else
> > +			return -EINVAL;
> > +
> > +		if (pwmc0) {
> > +			period /= BIT(pwmc0);
> > +			duty /= BIT(pwmc0);
> > +		}
> 
> You can drop the if and just make this:
> 
> 	period <<= pwmc0;
> 	duty <<= pwmc0;
> 
> as this is a noop if pwmc0 is zero.
> 

I will fix this.


> > +		if (state->polarity == PWM_POLARITY_INVERSED)
> > +			pwmc0 |= PWMC0_PWMACT;
> > +
> > +		writel(pwmc0, priv->base + REG_PWM0(pwm->hwpwm));
> > +		writel(duty, priv->base + REG_PDUT(pwm->hwpwm));
> > +		writel(period, priv->base + REG_PCSR(pwm->hwpwm));
> 
> Some comments about the function of the hardware would be good.
> Something like (I assume the optimal hardware here, please adapt to
> reality):
> 
> 	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 then the output drives the inactive state.
> 
> (I'm sure however this is wrong because you don't consider
> state->polarity in the !state-enabled case.)
> 
> If your hardware doesn't behave as indicated please add a Limitations
> paragraph at the beginning of the driver as is done for several other
> drivers already describing the shortcomings.
> 

OK, I will add a comment about IP restrictions as you pointed out.


> > +	} else {
> > +		writel(0, priv->base + REG_PCSR(pwm->hwpwm));
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct pwm_ops visconti_pwm_ops = {
> > +	.apply = visconti_pwm_apply,
> 
> Please implement .get_state(). (And test it using PWM_DEBUG.)
> 

OK, I will add get_state() function.

> > +	.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->dev = &pdev->dev;
> 
> You can better use
> 
> 	priv->dev = dev;
> 
> here. (But I agree to the previous review that it makes little sense to
> keep this member in struct visconti_pwm_chip.)
> 

OK, I will remove dev from visconti_pwm_chip.

> > +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(priv->base)) {
> > +		dev_err(dev, "unable to map I/O space\n");
> 
> devm_platform_ioremap_resource already emits an error message on failure,
> so no need to add another.

OK, I will drop error message.

> 
> > +		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) {
> > +		dev_err(dev, "Cannot register visconti PWM: %d\n", ret);
> 
> Please use dev_err_probe here or %pe for the error code.
> 


I will chakge to using  dev_err_probe.

> > +		return ret;
> > +	}
> > +
> > +	dev_info(&pdev->dev, "visconti PWM registered\n");
> 
> Please degrade this to dev_dbg.

I will change to dev_dbg.

> 
> > +	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,pwm-visconti", },
> > +	{ }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, visconti_pwm_of_match);
> 
> Please drop the empty line before MODULE_DEVICE_TABLE.
> 

OK, I will drop this.

> > +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);
> 
> The empty line before module_platform_driver is also unusual.
> 
OK, I will drop this.

> > +MODULE_LICENSE("GPL v2");
> > +MODULE_AUTHOR("Toshiba");
> > +MODULE_ALIAS("platform:visconti-pwm");
> 
> This is wrong; as the driver name is pwm-visconti this should be
> MODULE_ALIAS("platform:pwm-visconti");

OK, I will change to "platform:pwm-visconti".

> 
> Best regards
> Uwe
> 

Thanks!

Best regards,
  Nobuhiro

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



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


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