* [PATCH 0/2] Add Toshiba Visconti SoC PWM support @ 2020-09-17 22:31 Nobuhiro Iwamatsu 2020-09-17 22:31 ` [PATCH 1/2] dt-bindings: pwm: Add bindings for Toshiba Visconti PWM Controller Nobuhiro Iwamatsu 2020-09-17 22:31 ` [PATCH 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support Nobuhiro Iwamatsu 0 siblings, 2 replies; 9+ messages in thread From: Nobuhiro Iwamatsu @ 2020-09-17 22:31 UTC (permalink / raw) To: Rob Herring, Thierry Reding, Uwe Kleine-König, Lee Jones Cc: punit1.agrawal, yuji2.ishikawa, devicetree, linux-arm-kernel, linux-pwm, 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] dt-bindings: pwm: Add bindings for Toshiba Visconti PWM Controller 2020-09-17 22:31 [PATCH 0/2] Add Toshiba Visconti SoC PWM support Nobuhiro Iwamatsu @ 2020-09-17 22:31 ` Nobuhiro Iwamatsu 2020-09-23 20:37 ` Rob Herring 2020-09-17 22:31 ` [PATCH 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support Nobuhiro Iwamatsu 1 sibling, 1 reply; 9+ messages in thread From: Nobuhiro Iwamatsu @ 2020-09-17 22:31 UTC (permalink / raw) To: Rob Herring, Thierry Reding, Uwe Kleine-König, Lee Jones Cc: punit1.agrawal, yuji2.ishikawa, devicetree, linux-arm-kernel, linux-pwm, 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] dt-bindings: pwm: Add bindings for Toshiba Visconti PWM Controller 2020-09-17 22:31 ` [PATCH 1/2] dt-bindings: pwm: Add bindings for Toshiba Visconti PWM Controller Nobuhiro Iwamatsu @ 2020-09-23 20:37 ` Rob Herring 2021-02-12 6:44 ` Nobuhiro Iwamatsu 0 siblings, 1 reply; 9+ messages in thread From: Rob Herring @ 2020-09-23 20:37 UTC (permalink / raw) To: Nobuhiro Iwamatsu Cc: Thierry Reding, Uwe Kleine-König, Lee Jones, punit1.agrawal, yuji2.ishikawa, devicetree, linux-arm-kernel, linux-pwm 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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] dt-bindings: pwm: Add bindings for Toshiba Visconti PWM Controller 2020-09-23 20:37 ` Rob Herring @ 2021-02-12 6:44 ` Nobuhiro Iwamatsu 0 siblings, 0 replies; 9+ messages in thread From: Nobuhiro Iwamatsu @ 2021-02-12 6:44 UTC (permalink / raw) To: Rob Herring Cc: Thierry Reding, Uwe Kleine-König, Lee Jones, punit1.agrawal, yuji2.ishikawa, devicetree, linux-arm-kernel, linux-pwm 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support 2020-09-17 22:31 [PATCH 0/2] Add Toshiba Visconti SoC PWM support Nobuhiro Iwamatsu 2020-09-17 22:31 ` [PATCH 1/2] dt-bindings: pwm: Add bindings for Toshiba Visconti PWM Controller Nobuhiro Iwamatsu @ 2020-09-17 22:31 ` Nobuhiro Iwamatsu 2020-09-21 9:00 ` Punit Agrawal 2020-09-22 7:14 ` Uwe Kleine-König 1 sibling, 2 replies; 9+ messages in thread From: Nobuhiro Iwamatsu @ 2020-09-17 22:31 UTC (permalink / raw) To: Rob Herring, Thierry Reding, Uwe Kleine-König, Lee Jones Cc: punit1.agrawal, yuji2.ishikawa, devicetree, linux-arm-kernel, linux-pwm, Nobuhiro Iwamatsu, 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support 2020-09-17 22:31 ` [PATCH 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support Nobuhiro Iwamatsu @ 2020-09-21 9:00 ` Punit Agrawal 2021-02-12 8:30 ` Nobuhiro Iwamatsu 2020-09-22 7:14 ` Uwe Kleine-König 1 sibling, 1 reply; 9+ messages in thread From: Punit Agrawal @ 2020-09-21 9:00 UTC (permalink / raw) To: Nobuhiro Iwamatsu Cc: Rob Herring, Thierry Reding, Uwe Kleine-König, Lee Jones, linux-pwm, Nobuhiro Iwamatsu, devicetree, yuji2.ishikawa, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support 2020-09-21 9:00 ` Punit Agrawal @ 2021-02-12 8:30 ` Nobuhiro Iwamatsu 0 siblings, 0 replies; 9+ messages in thread From: Nobuhiro Iwamatsu @ 2021-02-12 8:30 UTC (permalink / raw) To: Punit Agrawal Cc: linux-pwm, Nobuhiro Iwamatsu, devicetree, yuji2.ishikawa, Rob Herring, Thierry Reding, Uwe Kleine-König, Lee Jones, linux-arm-kernel 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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support 2020-09-17 22:31 ` [PATCH 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support Nobuhiro Iwamatsu 2020-09-21 9:00 ` Punit Agrawal @ 2020-09-22 7:14 ` Uwe Kleine-König 2021-02-12 8:40 ` Nobuhiro Iwamatsu 1 sibling, 1 reply; 9+ messages in thread From: Uwe Kleine-König @ 2020-09-22 7:14 UTC (permalink / raw) To: Nobuhiro Iwamatsu Cc: Rob Herring, Thierry Reding, Lee Jones, punit1.agrawal, yuji2.ishikawa, devicetree, linux-arm-kernel, linux-pwm, Nobuhiro Iwamatsu [-- Attachment #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 #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support 2020-09-22 7:14 ` Uwe Kleine-König @ 2021-02-12 8:40 ` Nobuhiro Iwamatsu 0 siblings, 0 replies; 9+ messages in thread From: Nobuhiro Iwamatsu @ 2021-02-12 8:40 UTC (permalink / raw) To: Uwe Kleine-König Cc: devicetree, punit1.agrawal, Nobuhiro Iwamatsu, linux-pwm, Lee Jones, Rob Herring, Thierry Reding, yuji2.ishikawa, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-02-12 8:43 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-09-17 22:31 [PATCH 0/2] Add Toshiba Visconti SoC PWM support Nobuhiro Iwamatsu 2020-09-17 22:31 ` [PATCH 1/2] dt-bindings: pwm: Add bindings for Toshiba Visconti PWM Controller Nobuhiro Iwamatsu 2020-09-23 20:37 ` Rob Herring 2021-02-12 6:44 ` Nobuhiro Iwamatsu 2020-09-17 22:31 ` [PATCH 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support Nobuhiro Iwamatsu 2020-09-21 9:00 ` Punit Agrawal 2021-02-12 8:30 ` Nobuhiro Iwamatsu 2020-09-22 7:14 ` Uwe Kleine-König 2021-02-12 8:40 ` 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).