* [PATCH v2 0/2] pwm: visconti: Add Toshiba Visconti SoC PWM support @ 2021-02-12 13:19 ` Nobuhiro Iwamatsu 0 siblings, 0 replies; 18+ messages in thread From: Nobuhiro Iwamatsu @ 2021-02-12 13:19 UTC (permalink / raw) To: Rob Herring, Thierry Reding, Uwe Kleine-König, Lee Jones Cc: devicetree, linux-pwm, punit1.agrawal, yuji2.ishikawa, linux-arm-kernel, linux-kernel, Nobuhiro Iwamatsu 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 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 v1 -> v2: - Change SPDX-License-Identifier to GPL-2.0-only. - Add prefix for the register defines. - Drop struct device from struct visconti_pwm_chip. - Use '>>' instead of '/'. - Drop error message by devm_platform_ioremap_resource(). - Use dev_err_probe instead of dev_err. - Change dev_info to dev_dbg. - Remove some empty lines. - Fix MODULE_ALIAS to platform:pwm-visconti. - Add .get_state() function. - Use the author name and email address to MODULE_AUTHOR. - Add more comment to function of the hardware. - Support .get_status() function. - Use NSEC_PER_USEC instead of 1000. - Alphabetically sorted for Makefile and Kconfig. - Added check for set value in visconti_pwm_apply(). Nobuhiro Iwamatsu (2): dt-bindings: pwm: Add bindings for Toshiba Visconti PWM Controller pwm: visconti: Add Toshiba Visconti SoC PWM support .../bindings/pwm/toshiba,pwm-visconti.yaml | 43 +++++ drivers/pwm/Kconfig | 9 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-visconti.c | 173 ++++++++++++++++++ 4 files changed, 226 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml create mode 100644 drivers/pwm/pwm-visconti.c -- 2.30.0.rc2 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 0/2] pwm: visconti: Add Toshiba Visconti SoC PWM support @ 2021-02-12 13:19 ` Nobuhiro Iwamatsu 0 siblings, 0 replies; 18+ messages in thread From: Nobuhiro Iwamatsu @ 2021-02-12 13:19 UTC (permalink / raw) To: Rob Herring, Thierry Reding, Uwe Kleine-König, Lee Jones Cc: devicetree, punit1.agrawal, linux-pwm, linux-kernel, Nobuhiro Iwamatsu, yuji2.ishikawa, linux-arm-kernel 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 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 v1 -> v2: - Change SPDX-License-Identifier to GPL-2.0-only. - Add prefix for the register defines. - Drop struct device from struct visconti_pwm_chip. - Use '>>' instead of '/'. - Drop error message by devm_platform_ioremap_resource(). - Use dev_err_probe instead of dev_err. - Change dev_info to dev_dbg. - Remove some empty lines. - Fix MODULE_ALIAS to platform:pwm-visconti. - Add .get_state() function. - Use the author name and email address to MODULE_AUTHOR. - Add more comment to function of the hardware. - Support .get_status() function. - Use NSEC_PER_USEC instead of 1000. - Alphabetically sorted for Makefile and Kconfig. - Added check for set value in visconti_pwm_apply(). Nobuhiro Iwamatsu (2): dt-bindings: pwm: Add bindings for Toshiba Visconti PWM Controller pwm: visconti: Add Toshiba Visconti SoC PWM support .../bindings/pwm/toshiba,pwm-visconti.yaml | 43 +++++ drivers/pwm/Kconfig | 9 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-visconti.c | 173 ++++++++++++++++++ 4 files changed, 226 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml create mode 100644 drivers/pwm/pwm-visconti.c -- 2.30.0.rc2 _______________________________________________ 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] 18+ messages in thread
* [PATCH v2 1/2] dt-bindings: pwm: Add bindings for Toshiba Visconti PWM Controller 2021-02-12 13:19 ` Nobuhiro Iwamatsu @ 2021-02-12 13:19 ` Nobuhiro Iwamatsu -1 siblings, 0 replies; 18+ messages in thread From: Nobuhiro Iwamatsu @ 2021-02-12 13:19 UTC (permalink / raw) To: Rob Herring, Thierry Reding, Uwe Kleine-König, Lee Jones Cc: devicetree, linux-pwm, punit1.agrawal, yuji2.ishikawa, linux-arm-kernel, linux-kernel, Nobuhiro Iwamatsu Add bindings for the Toshiba Visconti PWM Controller. Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> --- .../bindings/pwm/toshiba,pwm-visconti.yaml | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml diff --git a/Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml b/Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml new file mode 100644 index 000000000000..f11558cdc32f --- /dev/null +++ b/Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml @@ -0,0 +1,43 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pwm/toshiba,pwm-visconti.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Toshiba Visconti PWM Controller + +maintainers: + - Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> + +properties: + compatible: + items: + - const: toshiba,pwm-visconti + + 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,pwm-visconti"; + reg = <0 0x241c0000 0 0x1000>; + pinctrl-names = "default"; + pinctrl-0 = <&pwm_mux>; + #pwm-cells = <2>; + }; + }; -- 2.30.0.rc2 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 1/2] dt-bindings: pwm: Add bindings for Toshiba Visconti PWM Controller @ 2021-02-12 13:19 ` Nobuhiro Iwamatsu 0 siblings, 0 replies; 18+ messages in thread From: Nobuhiro Iwamatsu @ 2021-02-12 13:19 UTC (permalink / raw) To: Rob Herring, Thierry Reding, Uwe Kleine-König, Lee Jones Cc: devicetree, punit1.agrawal, linux-pwm, linux-kernel, Nobuhiro Iwamatsu, yuji2.ishikawa, linux-arm-kernel Add bindings for the Toshiba Visconti PWM Controller. Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> --- .../bindings/pwm/toshiba,pwm-visconti.yaml | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml diff --git a/Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml b/Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml new file mode 100644 index 000000000000..f11558cdc32f --- /dev/null +++ b/Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml @@ -0,0 +1,43 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pwm/toshiba,pwm-visconti.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Toshiba Visconti PWM Controller + +maintainers: + - Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> + +properties: + compatible: + items: + - const: toshiba,pwm-visconti + + 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,pwm-visconti"; + reg = <0 0x241c0000 0 0x1000>; + pinctrl-names = "default"; + pinctrl-0 = <&pwm_mux>; + #pwm-cells = <2>; + }; + }; -- 2.30.0.rc2 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: pwm: Add bindings for Toshiba Visconti PWM Controller 2021-02-12 13:19 ` Nobuhiro Iwamatsu @ 2021-03-05 20:22 ` Rob Herring -1 siblings, 0 replies; 18+ messages in thread From: Rob Herring @ 2021-03-05 20:22 UTC (permalink / raw) To: Nobuhiro Iwamatsu Cc: Thierry Reding, Uwe Kleine-König, Lee Jones, devicetree, linux-pwm, punit1.agrawal, yuji2.ishikawa, linux-arm-kernel, linux-kernel On Fri, Feb 12, 2021 at 10:19:09PM +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 | 43 +++++++++++++++++++ > 1 file changed, 43 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml > > diff --git a/Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml b/Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml > new file mode 100644 > index 000000000000..f11558cdc32f > --- /dev/null > +++ b/Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml > @@ -0,0 +1,43 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pwm/toshiba,pwm-visconti.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Toshiba Visconti PWM Controller > + > +maintainers: > + - Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> > + > +properties: > + compatible: > + items: > + - const: toshiba,pwm-visconti The 'normal' order is: toshiba,visconti-pwm Otherwise, Reviewed-by: Rob Herring <robh@kernel.org> > + > + 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,pwm-visconti"; > + reg = <0 0x241c0000 0 0x1000>; > + pinctrl-names = "default"; > + pinctrl-0 = <&pwm_mux>; > + #pwm-cells = <2>; > + }; > + }; > -- > 2.30.0.rc2 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: pwm: Add bindings for Toshiba Visconti PWM Controller @ 2021-03-05 20:22 ` Rob Herring 0 siblings, 0 replies; 18+ messages in thread From: Rob Herring @ 2021-03-05 20:22 UTC (permalink / raw) To: Nobuhiro Iwamatsu Cc: Thierry Reding, Uwe Kleine-König, Lee Jones, devicetree, linux-pwm, punit1.agrawal, yuji2.ishikawa, linux-arm-kernel, linux-kernel On Fri, Feb 12, 2021 at 10:19:09PM +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 | 43 +++++++++++++++++++ > 1 file changed, 43 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml > > diff --git a/Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml b/Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml > new file mode 100644 > index 000000000000..f11558cdc32f > --- /dev/null > +++ b/Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml > @@ -0,0 +1,43 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pwm/toshiba,pwm-visconti.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Toshiba Visconti PWM Controller > + > +maintainers: > + - Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> > + > +properties: > + compatible: > + items: > + - const: toshiba,pwm-visconti The 'normal' order is: toshiba,visconti-pwm Otherwise, Reviewed-by: Rob Herring <robh@kernel.org> > + > + 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,pwm-visconti"; > + reg = <0 0x241c0000 0 0x1000>; > + pinctrl-names = "default"; > + pinctrl-0 = <&pwm_mux>; > + #pwm-cells = <2>; > + }; > + }; > -- > 2.30.0.rc2 > _______________________________________________ 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] 18+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: pwm: Add bindings for Toshiba Visconti PWM Controller 2021-03-05 20:22 ` Rob Herring @ 2021-04-09 7:44 ` Nobuhiro Iwamatsu -1 siblings, 0 replies; 18+ messages in thread From: Nobuhiro Iwamatsu @ 2021-04-09 7:44 UTC (permalink / raw) To: Rob Herring Cc: Thierry Reding, Uwe Kleine-König, Lee Jones, devicetree, linux-pwm, punit1.agrawal, yuji2.ishikawa, linux-arm-kernel, linux-kernel Hi, On Fri, Mar 05, 2021 at 02:22:24PM -0600, Rob Herring wrote: > On Fri, Feb 12, 2021 at 10:19:09PM +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 | 43 +++++++++++++++++++ > > 1 file changed, 43 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml > > > > diff --git a/Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml b/Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml > > new file mode 100644 > > index 000000000000..f11558cdc32f > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml > > @@ -0,0 +1,43 @@ > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/pwm/toshiba,pwm-visconti.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Toshiba Visconti PWM Controller > > + > > +maintainers: > > + - Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> > > + > > +properties: > > + compatible: > > + items: > > + - const: toshiba,pwm-visconti > > The 'normal' order is: toshiba,visconti-pwm I see, I will change to it. > > Otherwise, > > Reviewed-by: Rob Herring <robh@kernel.org> > Thanks! Best regards, Nobuhiro ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: pwm: Add bindings for Toshiba Visconti PWM Controller @ 2021-04-09 7:44 ` Nobuhiro Iwamatsu 0 siblings, 0 replies; 18+ messages in thread From: Nobuhiro Iwamatsu @ 2021-04-09 7:44 UTC (permalink / raw) To: Rob Herring Cc: Thierry Reding, Uwe Kleine-König, Lee Jones, devicetree, linux-pwm, punit1.agrawal, yuji2.ishikawa, linux-arm-kernel, linux-kernel Hi, On Fri, Mar 05, 2021 at 02:22:24PM -0600, Rob Herring wrote: > On Fri, Feb 12, 2021 at 10:19:09PM +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 | 43 +++++++++++++++++++ > > 1 file changed, 43 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml > > > > diff --git a/Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml b/Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml > > new file mode 100644 > > index 000000000000..f11558cdc32f > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/pwm/toshiba,pwm-visconti.yaml > > @@ -0,0 +1,43 @@ > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/pwm/toshiba,pwm-visconti.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Toshiba Visconti PWM Controller > > + > > +maintainers: > > + - Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> > > + > > +properties: > > + compatible: > > + items: > > + - const: toshiba,pwm-visconti > > The 'normal' order is: toshiba,visconti-pwm I see, I will change to it. > > Otherwise, > > Reviewed-by: Rob Herring <robh@kernel.org> > Thanks! Best regards, Nobuhiro _______________________________________________ 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] 18+ messages in thread
* [PATCH v2 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support 2021-02-12 13:19 ` Nobuhiro Iwamatsu @ 2021-02-12 13:19 ` Nobuhiro Iwamatsu -1 siblings, 0 replies; 18+ messages in thread From: Nobuhiro Iwamatsu @ 2021-02-12 13:19 UTC (permalink / raw) To: Rob Herring, Thierry Reding, Uwe Kleine-König, Lee Jones Cc: devicetree, linux-pwm, punit1.agrawal, yuji2.ishikawa, linux-arm-kernel, linux-kernel, Nobuhiro Iwamatsu Add driver for the PWM controller on Toshiba Visconti ARM SoC. Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> --- drivers/pwm/Kconfig | 9 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-visconti.c | 173 +++++++++++++++++++++++++++++++++++++ 3 files changed, 183 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..2aa140f1ec04 --- /dev/null +++ b/drivers/pwm/pwm-visconti.c @@ -0,0 +1,173 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Toshiba Visconti pulse-width-modulation controller driver + * + * Copyright (c) 2020 TOSHIBA CORPORATION + * Copyright (c) 2020 Toshiba Electronic Devices & Storage Corporation + * + * Authors: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> + * + */ + +#include <linux/err.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/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) +#define PIPGM_PDUT_MAX 0xFFFF + +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, 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; + } + + period = state->period / NSEC_PER_USEC; + duty = state->duty_cycle / NSEC_PER_USEC; + 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 (duty > PIPGM_PDUT_MAX) + return -EINVAL; + + period >>= pwmc0; + duty >>= pwmc0; + + if (state->polarity == PWM_POLARITY_INVERSED) + pwmc0 |= PIPGM_PWMC_PWMACT; + + writel(pwmc0, priv->base + PIPGM_PWMC(pwm->hwpwm)); + writel(duty, 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,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("Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>"); +MODULE_ALIAS("platform:visconti-pwm"); -- 2.30.0.rc2 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support @ 2021-02-12 13:19 ` Nobuhiro Iwamatsu 0 siblings, 0 replies; 18+ messages in thread From: Nobuhiro Iwamatsu @ 2021-02-12 13:19 UTC (permalink / raw) To: Rob Herring, Thierry Reding, Uwe Kleine-König, Lee Jones Cc: devicetree, punit1.agrawal, linux-pwm, linux-kernel, Nobuhiro Iwamatsu, yuji2.ishikawa, linux-arm-kernel Add driver for the PWM controller on Toshiba Visconti ARM SoC. Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> --- drivers/pwm/Kconfig | 9 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-visconti.c | 173 +++++++++++++++++++++++++++++++++++++ 3 files changed, 183 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..2aa140f1ec04 --- /dev/null +++ b/drivers/pwm/pwm-visconti.c @@ -0,0 +1,173 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Toshiba Visconti pulse-width-modulation controller driver + * + * Copyright (c) 2020 TOSHIBA CORPORATION + * Copyright (c) 2020 Toshiba Electronic Devices & Storage Corporation + * + * Authors: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> + * + */ + +#include <linux/err.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/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) +#define PIPGM_PDUT_MAX 0xFFFF + +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, 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; + } + + period = state->period / NSEC_PER_USEC; + duty = state->duty_cycle / NSEC_PER_USEC; + 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 (duty > PIPGM_PDUT_MAX) + return -EINVAL; + + period >>= pwmc0; + duty >>= pwmc0; + + if (state->polarity == PWM_POLARITY_INVERSED) + pwmc0 |= PIPGM_PWMC_PWMACT; + + writel(pwmc0, priv->base + PIPGM_PWMC(pwm->hwpwm)); + writel(duty, 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,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("Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>"); +MODULE_ALIAS("platform:visconti-pwm"); -- 2.30.0.rc2 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support 2021-02-12 13:19 ` Nobuhiro Iwamatsu @ 2021-02-12 16:41 ` Uwe Kleine-König -1 siblings, 0 replies; 18+ messages in thread From: Uwe Kleine-König @ 2021-02-12 16:41 UTC (permalink / raw) To: Nobuhiro Iwamatsu Cc: Rob Herring, Thierry Reding, Lee Jones, devicetree, linux-pwm, punit1.agrawal, yuji2.ishikawa, linux-arm-kernel, linux-kernel, kernel [-- Attachment #1: Type: text/plain, Size: 7720 bytes --] Hello Nobuhiro, On Fri, Feb 12, 2021 at 10:19:10PM +0900, Nobuhiro Iwamatsu wrote: > Add driver for the PWM controller on Toshiba Visconti ARM SoC. > > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> > --- > drivers/pwm/Kconfig | 9 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-visconti.c | 173 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 183 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..2aa140f1ec04 > --- /dev/null > +++ b/drivers/pwm/pwm-visconti.c > @@ -0,0 +1,173 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Toshiba Visconti pulse-width-modulation controller driver > + * > + * Copyright (c) 2020 TOSHIBA CORPORATION > + * Copyright (c) 2020 Toshiba Electronic Devices & Storage Corporation > + * > + * Authors: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> > + * > + */ > + > +#include <linux/err.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/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) > +#define PIPGM_PDUT_MAX 0xFFFF > + > +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) Please align the continuation line to the opening parenthesis. > +{ > + struct visconti_pwm_chip *priv = to_visconti_chip(chip); > + u32 period, duty, 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. Did you just copy my optimal description or is your hardware really that nice? Do you know scripts/checkpatch.pl? I bet it will tell you to limit your lines to approx. 80 chars where sensible. > + */ > + if (!state->enabled) { > + writel(0, priv->base + PIPGM_PCSR(pwm->hwpwm)); > + return 0; > + } > + > + period = state->period / NSEC_PER_USEC; This becomes wrong if state->period > 1000 * 0xffffffff because you discard non-zero bits when reducing the size to u32. > + duty = state->duty_cycle / NSEC_PER_USEC; > + 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; This is equivalent to: pwmc0 = ilog2(period >> 16); if (pwmc0 > 3) return -EINVAL; > + if (duty > PIPGM_PDUT_MAX) > + return -EINVAL; I would expect that this check should only happen after duty is shifted below?! I think this cannot happen if you rely on the core to only give you states with duty_cycle <= period. > + period >>= pwmc0; > + duty >>= pwmc0; > + > + if (state->polarity == PWM_POLARITY_INVERSED) > + pwmc0 |= PIPGM_PWMC_PWMACT; > + > + writel(pwmc0, priv->base + PIPGM_PWMC(pwm->hwpwm)); > + writel(duty, priv->base + PIPGM_PDUT(pwm->hwpwm)); > + writel(period, priv->base + PIPGM_PCSR(pwm->hwpwm)); Please implement the following policy: Pick the biggest possible period not bigger than the requested period. With that pick the biggest possible duty cycle not bigger than the requested duty cycle. That means (assuming I understood your hardware correctly): u32 period, duty_cycle; /* * 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. */ pwmc0 = ilog2(period >> 16); BUG_ON(pwmc0 > 3); 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, 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) > +{ > +[...] > +} Looks good. > [...] > > +static struct platform_driver visconti_pwm_driver = { > + .driver = { > + .name = "pwm-visconti", > + .of_match_table = visconti_pwm_of_match, > + }, > + .probe = visconti_pwm_probe, > + .remove = visconti_pwm_remove, > +}; > +module_platform_driver(visconti_pwm_driver); > + > +MODULE_LICENSE("GPL v2"); > +MODULE_AUTHOR("Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>"); > +MODULE_ALIAS("platform:visconti-pwm"); This must match the .name field of the platform driver, so it must 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] 18+ messages in thread
* Re: [PATCH v2 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support @ 2021-02-12 16:41 ` Uwe Kleine-König 0 siblings, 0 replies; 18+ messages in thread From: Uwe Kleine-König @ 2021-02-12 16:41 UTC (permalink / raw) To: Nobuhiro Iwamatsu Cc: linux-pwm, punit1.agrawal, devicetree, yuji2.ishikawa, linux-kernel, Rob Herring, Thierry Reding, kernel, Lee Jones, linux-arm-kernel [-- Attachment #1.1: Type: text/plain, Size: 7720 bytes --] Hello Nobuhiro, On Fri, Feb 12, 2021 at 10:19:10PM +0900, Nobuhiro Iwamatsu wrote: > Add driver for the PWM controller on Toshiba Visconti ARM SoC. > > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> > --- > drivers/pwm/Kconfig | 9 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-visconti.c | 173 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 183 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..2aa140f1ec04 > --- /dev/null > +++ b/drivers/pwm/pwm-visconti.c > @@ -0,0 +1,173 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Toshiba Visconti pulse-width-modulation controller driver > + * > + * Copyright (c) 2020 TOSHIBA CORPORATION > + * Copyright (c) 2020 Toshiba Electronic Devices & Storage Corporation > + * > + * Authors: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> > + * > + */ > + > +#include <linux/err.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/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) > +#define PIPGM_PDUT_MAX 0xFFFF > + > +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) Please align the continuation line to the opening parenthesis. > +{ > + struct visconti_pwm_chip *priv = to_visconti_chip(chip); > + u32 period, duty, 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. Did you just copy my optimal description or is your hardware really that nice? Do you know scripts/checkpatch.pl? I bet it will tell you to limit your lines to approx. 80 chars where sensible. > + */ > + if (!state->enabled) { > + writel(0, priv->base + PIPGM_PCSR(pwm->hwpwm)); > + return 0; > + } > + > + period = state->period / NSEC_PER_USEC; This becomes wrong if state->period > 1000 * 0xffffffff because you discard non-zero bits when reducing the size to u32. > + duty = state->duty_cycle / NSEC_PER_USEC; > + 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; This is equivalent to: pwmc0 = ilog2(period >> 16); if (pwmc0 > 3) return -EINVAL; > + if (duty > PIPGM_PDUT_MAX) > + return -EINVAL; I would expect that this check should only happen after duty is shifted below?! I think this cannot happen if you rely on the core to only give you states with duty_cycle <= period. > + period >>= pwmc0; > + duty >>= pwmc0; > + > + if (state->polarity == PWM_POLARITY_INVERSED) > + pwmc0 |= PIPGM_PWMC_PWMACT; > + > + writel(pwmc0, priv->base + PIPGM_PWMC(pwm->hwpwm)); > + writel(duty, priv->base + PIPGM_PDUT(pwm->hwpwm)); > + writel(period, priv->base + PIPGM_PCSR(pwm->hwpwm)); Please implement the following policy: Pick the biggest possible period not bigger than the requested period. With that pick the biggest possible duty cycle not bigger than the requested duty cycle. That means (assuming I understood your hardware correctly): u32 period, duty_cycle; /* * 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. */ pwmc0 = ilog2(period >> 16); BUG_ON(pwmc0 > 3); 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, 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) > +{ > +[...] > +} Looks good. > [...] > > +static struct platform_driver visconti_pwm_driver = { > + .driver = { > + .name = "pwm-visconti", > + .of_match_table = visconti_pwm_of_match, > + }, > + .probe = visconti_pwm_probe, > + .remove = visconti_pwm_remove, > +}; > +module_platform_driver(visconti_pwm_driver); > + > +MODULE_LICENSE("GPL v2"); > +MODULE_AUTHOR("Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>"); > +MODULE_ALIAS("platform:visconti-pwm"); This must match the .name field of the platform driver, so it must 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support 2021-02-12 16:41 ` Uwe Kleine-König @ 2021-04-07 23:15 ` Nobuhiro Iwamatsu -1 siblings, 0 replies; 18+ messages in thread From: Nobuhiro Iwamatsu @ 2021-04-07 23:15 UTC (permalink / raw) To: Uwe Kleine-König Cc: Rob Herring, Thierry Reding, Lee Jones, devicetree, linux-pwm, punit1.agrawal, yuji2.ishikawa, linux-arm-kernel, linux-kernel, kernel Hi, Thanks for your review. On Fri, Feb 12, 2021 at 05:41:44PM +0100, Uwe Kleine-König wrote: > Hello Nobuhiro, > > On Fri, Feb 12, 2021 at 10:19:10PM +0900, Nobuhiro Iwamatsu wrote: > > Add driver for the PWM controller on Toshiba Visconti ARM SoC. > > > > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> > > --- > > drivers/pwm/Kconfig | 9 ++ > > drivers/pwm/Makefile | 1 + > > drivers/pwm/pwm-visconti.c | 173 +++++++++++++++++++++++++++++++++++++ > > 3 files changed, 183 insertions(+) > > create mode 100644 drivers/pwm/pwm-visconti.c > > <snip> > > diff --git a/drivers/pwm/pwm-visconti.c b/drivers/pwm/pwm-visconti.c > > new file mode 100644 > > index 000000000000..2aa140f1ec04 > > --- /dev/null > > +++ b/drivers/pwm/pwm-visconti.c > > @@ -0,0 +1,173 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * Toshiba Visconti pulse-width-modulation controller driver > > + * > > + * Copyright (c) 2020 TOSHIBA CORPORATION > > + * Copyright (c) 2020 Toshiba Electronic Devices & Storage Corporation > > + * > > + * Authors: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> > > + * > > + */ > > + > > +#include <linux/err.h> > > +#include <linux/io.h> > > +#include <linux/module.h> > > +#include <linux/of_device.h> > > +#include <linux/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) > > +#define PIPGM_PDUT_MAX 0xFFFF > > + > > +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) > > Please align the continuation line to the opening parenthesis. > I will fix this. > > +{ > > + struct visconti_pwm_chip *priv = to_visconti_chip(chip); > > + u32 period, duty, 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. > > Did you just copy my optimal description or is your hardware really that > nice? Yes, this hardware works as you wrote. And I added about the state if the sinnal when this hardware disabled. > > Do you know scripts/checkpatch.pl? I bet it will tell you to limit your > lines to approx. 80 chars where sensible. Yes, I know. I ran scripts/checkpatch.pl before send patch. I understand that the number of characters per line has been changed to 100 characters. Does the pwm driver recommend 80 characters? > > > + */ > > + if (!state->enabled) { > > + writel(0, priv->base + PIPGM_PCSR(pwm->hwpwm)); > > + return 0; > > + } > > + > > + period = state->period / NSEC_PER_USEC; > > This becomes wrong if state->period > 1000 * 0xffffffff because you > discard non-zero bits when reducing the size to u32. Your point is correct. I will fix this. > > > + duty = state->duty_cycle / NSEC_PER_USEC; > > + 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; > > This is equivalent to: > > pwmc0 = ilog2(period >> 16); > if (pwmc0 > 3) > return -EINVAL; > I see. And I noticed that there was a problem with the above code. I will use ilog2. > > + if (duty > PIPGM_PDUT_MAX) > > + return -EINVAL; > > I would expect that this check should only happen after duty is shifted > below?! I think this cannot happen if you rely on the core to only give > you states with duty_cycle <= period. I see. I will fix this. > > > + period >>= pwmc0; > > + duty >>= pwmc0; > > + > > + if (state->polarity == PWM_POLARITY_INVERSED) > > + pwmc0 |= PIPGM_PWMC_PWMACT; > > + > > + writel(pwmc0, priv->base + PIPGM_PWMC(pwm->hwpwm)); > > + writel(duty, priv->base + PIPGM_PDUT(pwm->hwpwm)); > > + writel(period, priv->base + PIPGM_PCSR(pwm->hwpwm)); > > Please implement the following policy: > > Pick the biggest possible period not bigger than the requested period. > With that pick the biggest possible duty cycle not bigger than the > requested duty cycle. That means (assuming I understood your hardware > correctly): > > u32 period, duty_cycle; > > /* > * 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. > */ > pwmc0 = ilog2(period >> 16); > BUG_ON(pwmc0 > 3); > > 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, priv->base + PIPGM_PDUT(pwm->hwpwm)); > writel(period, priv->base + PIPGM_PCSR(pwm->hwpwm)); > Thank you for your suggestion. I will reconsider based on this code. > > + return 0; > > +} > > + > > +static void visconti_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > > + struct pwm_state *state) > > +{ > > +[...] > > +} > > Looks good. > > > [...] > > > > +static struct platform_driver visconti_pwm_driver = { > > + .driver = { > > + .name = "pwm-visconti", > > + .of_match_table = visconti_pwm_of_match, > > + }, > > + .probe = visconti_pwm_probe, > > + .remove = visconti_pwm_remove, > > +}; > > +module_platform_driver(visconti_pwm_driver); > > + > > +MODULE_LICENSE("GPL v2"); > > +MODULE_AUTHOR("Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>"); > > +MODULE_ALIAS("platform:visconti-pwm"); > > This must match the .name field of the platform driver, so it must be > > MODULE_ALIAS("platform:pwm-visconti"); I forgot this mistake. I will fix. > > Best regards > Uwe Best regards, Nobuhiro ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support @ 2021-04-07 23:15 ` Nobuhiro Iwamatsu 0 siblings, 0 replies; 18+ messages in thread From: Nobuhiro Iwamatsu @ 2021-04-07 23:15 UTC (permalink / raw) To: Uwe Kleine-König Cc: Rob Herring, Thierry Reding, Lee Jones, devicetree, linux-pwm, punit1.agrawal, yuji2.ishikawa, linux-arm-kernel, linux-kernel, kernel Hi, Thanks for your review. On Fri, Feb 12, 2021 at 05:41:44PM +0100, Uwe Kleine-König wrote: > Hello Nobuhiro, > > On Fri, Feb 12, 2021 at 10:19:10PM +0900, Nobuhiro Iwamatsu wrote: > > Add driver for the PWM controller on Toshiba Visconti ARM SoC. > > > > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> > > --- > > drivers/pwm/Kconfig | 9 ++ > > drivers/pwm/Makefile | 1 + > > drivers/pwm/pwm-visconti.c | 173 +++++++++++++++++++++++++++++++++++++ > > 3 files changed, 183 insertions(+) > > create mode 100644 drivers/pwm/pwm-visconti.c > > <snip> > > diff --git a/drivers/pwm/pwm-visconti.c b/drivers/pwm/pwm-visconti.c > > new file mode 100644 > > index 000000000000..2aa140f1ec04 > > --- /dev/null > > +++ b/drivers/pwm/pwm-visconti.c > > @@ -0,0 +1,173 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * Toshiba Visconti pulse-width-modulation controller driver > > + * > > + * Copyright (c) 2020 TOSHIBA CORPORATION > > + * Copyright (c) 2020 Toshiba Electronic Devices & Storage Corporation > > + * > > + * Authors: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> > > + * > > + */ > > + > > +#include <linux/err.h> > > +#include <linux/io.h> > > +#include <linux/module.h> > > +#include <linux/of_device.h> > > +#include <linux/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) > > +#define PIPGM_PDUT_MAX 0xFFFF > > + > > +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) > > Please align the continuation line to the opening parenthesis. > I will fix this. > > +{ > > + struct visconti_pwm_chip *priv = to_visconti_chip(chip); > > + u32 period, duty, 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. > > Did you just copy my optimal description or is your hardware really that > nice? Yes, this hardware works as you wrote. And I added about the state if the sinnal when this hardware disabled. > > Do you know scripts/checkpatch.pl? I bet it will tell you to limit your > lines to approx. 80 chars where sensible. Yes, I know. I ran scripts/checkpatch.pl before send patch. I understand that the number of characters per line has been changed to 100 characters. Does the pwm driver recommend 80 characters? > > > + */ > > + if (!state->enabled) { > > + writel(0, priv->base + PIPGM_PCSR(pwm->hwpwm)); > > + return 0; > > + } > > + > > + period = state->period / NSEC_PER_USEC; > > This becomes wrong if state->period > 1000 * 0xffffffff because you > discard non-zero bits when reducing the size to u32. Your point is correct. I will fix this. > > > + duty = state->duty_cycle / NSEC_PER_USEC; > > + 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; > > This is equivalent to: > > pwmc0 = ilog2(period >> 16); > if (pwmc0 > 3) > return -EINVAL; > I see. And I noticed that there was a problem with the above code. I will use ilog2. > > + if (duty > PIPGM_PDUT_MAX) > > + return -EINVAL; > > I would expect that this check should only happen after duty is shifted > below?! I think this cannot happen if you rely on the core to only give > you states with duty_cycle <= period. I see. I will fix this. > > > + period >>= pwmc0; > > + duty >>= pwmc0; > > + > > + if (state->polarity == PWM_POLARITY_INVERSED) > > + pwmc0 |= PIPGM_PWMC_PWMACT; > > + > > + writel(pwmc0, priv->base + PIPGM_PWMC(pwm->hwpwm)); > > + writel(duty, priv->base + PIPGM_PDUT(pwm->hwpwm)); > > + writel(period, priv->base + PIPGM_PCSR(pwm->hwpwm)); > > Please implement the following policy: > > Pick the biggest possible period not bigger than the requested period. > With that pick the biggest possible duty cycle not bigger than the > requested duty cycle. That means (assuming I understood your hardware > correctly): > > u32 period, duty_cycle; > > /* > * 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. > */ > pwmc0 = ilog2(period >> 16); > BUG_ON(pwmc0 > 3); > > 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, priv->base + PIPGM_PDUT(pwm->hwpwm)); > writel(period, priv->base + PIPGM_PCSR(pwm->hwpwm)); > Thank you for your suggestion. I will reconsider based on this code. > > + return 0; > > +} > > + > > +static void visconti_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > > + struct pwm_state *state) > > +{ > > +[...] > > +} > > Looks good. > > > [...] > > > > +static struct platform_driver visconti_pwm_driver = { > > + .driver = { > > + .name = "pwm-visconti", > > + .of_match_table = visconti_pwm_of_match, > > + }, > > + .probe = visconti_pwm_probe, > > + .remove = visconti_pwm_remove, > > +}; > > +module_platform_driver(visconti_pwm_driver); > > + > > +MODULE_LICENSE("GPL v2"); > > +MODULE_AUTHOR("Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>"); > > +MODULE_ALIAS("platform:visconti-pwm"); > > This must match the .name field of the platform driver, so it must be > > MODULE_ALIAS("platform:pwm-visconti"); I forgot this mistake. I will fix. > > Best regards > Uwe Best regards, Nobuhiro _______________________________________________ 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] 18+ messages in thread
* Re: [PATCH v2 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support 2021-04-07 23:15 ` Nobuhiro Iwamatsu @ 2021-04-08 7:59 ` Uwe Kleine-König -1 siblings, 0 replies; 18+ messages in thread From: Uwe Kleine-König @ 2021-04-08 7:59 UTC (permalink / raw) To: Nobuhiro Iwamatsu Cc: linux-pwm, punit1.agrawal, devicetree, yuji2.ishikawa, linux-kernel, Rob Herring, Thierry Reding, kernel, Lee Jones, linux-arm-kernel [-- Attachment #1: Type: text/plain, Size: 1483 bytes --] Hello Nobuhiro, On Thu, Apr 08, 2021 at 08:15:48AM +0900, Nobuhiro Iwamatsu wrote: > > > + /* > > > + * 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. > > > > Did you just copy my optimal description or is your hardware really that > > nice? > > Yes, this hardware works as you wrote. > And I added about the state if the sinnal when this hardware disabled. > > > > > Do you know scripts/checkpatch.pl? I bet it will tell you to limit your > > lines to approx. 80 chars where sensible. > > Yes, I know. I ran scripts/checkpatch.pl before send patch. > I understand that the number of characters per line has been changed to > 100 characters. Does the pwm driver recommend 80 characters? For free-text comments I'd still recommend 80, yes. For code lines I'd be indeed more lax, as a line break in function calls reduces readability. 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] 18+ messages in thread
* Re: [PATCH v2 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support @ 2021-04-08 7:59 ` Uwe Kleine-König 0 siblings, 0 replies; 18+ messages in thread From: Uwe Kleine-König @ 2021-04-08 7:59 UTC (permalink / raw) To: Nobuhiro Iwamatsu Cc: linux-pwm, punit1.agrawal, devicetree, yuji2.ishikawa, linux-kernel, Rob Herring, Thierry Reding, kernel, Lee Jones, linux-arm-kernel [-- Attachment #1.1: Type: text/plain, Size: 1483 bytes --] Hello Nobuhiro, On Thu, Apr 08, 2021 at 08:15:48AM +0900, Nobuhiro Iwamatsu wrote: > > > + /* > > > + * 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. > > > > Did you just copy my optimal description or is your hardware really that > > nice? > > Yes, this hardware works as you wrote. > And I added about the state if the sinnal when this hardware disabled. > > > > > Do you know scripts/checkpatch.pl? I bet it will tell you to limit your > > lines to approx. 80 chars where sensible. > > Yes, I know. I ran scripts/checkpatch.pl before send patch. > I understand that the number of characters per line has been changed to > 100 characters. Does the pwm driver recommend 80 characters? For free-text comments I'd still recommend 80, yes. For code lines I'd be indeed more lax, as a line break in function calls reduces readability. 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support 2021-04-08 7:59 ` Uwe Kleine-König @ 2021-04-08 12:22 ` Thierry Reding -1 siblings, 0 replies; 18+ messages in thread From: Thierry Reding @ 2021-04-08 12:22 UTC (permalink / raw) To: Uwe Kleine-König Cc: Nobuhiro Iwamatsu, linux-pwm, punit1.agrawal, devicetree, yuji2.ishikawa, linux-kernel, Rob Herring, kernel, Lee Jones, linux-arm-kernel [-- Attachment #1: Type: text/plain, Size: 1644 bytes --] On Thu, Apr 08, 2021 at 09:59:20AM +0200, Uwe Kleine-König wrote: > Hello Nobuhiro, > > On Thu, Apr 08, 2021 at 08:15:48AM +0900, Nobuhiro Iwamatsu wrote: > > > > + /* > > > > + * 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. > > > > > > Did you just copy my optimal description or is your hardware really that > > > nice? > > > > Yes, this hardware works as you wrote. > > And I added about the state if the sinnal when this hardware disabled. > > > > > > > > Do you know scripts/checkpatch.pl? I bet it will tell you to limit your > > > lines to approx. 80 chars where sensible. > > > > Yes, I know. I ran scripts/checkpatch.pl before send patch. > > I understand that the number of characters per line has been changed to > > 100 characters. Does the pwm driver recommend 80 characters? > > For free-text comments I'd still recommend 80, yes. For code lines I'd > be indeed more lax, as a line break in function calls reduces readability. Let's not start making any special rules. It becomes impossible for anyone to keep track of those. If checkpatch doesn't complain for comments that exceed 80 characters, I will not reject based on that. Thierry [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support @ 2021-04-08 12:22 ` Thierry Reding 0 siblings, 0 replies; 18+ messages in thread From: Thierry Reding @ 2021-04-08 12:22 UTC (permalink / raw) To: Uwe Kleine-König Cc: Nobuhiro Iwamatsu, linux-pwm, punit1.agrawal, devicetree, yuji2.ishikawa, linux-kernel, Rob Herring, kernel, Lee Jones, linux-arm-kernel [-- Attachment #1.1: Type: text/plain, Size: 1644 bytes --] On Thu, Apr 08, 2021 at 09:59:20AM +0200, Uwe Kleine-König wrote: > Hello Nobuhiro, > > On Thu, Apr 08, 2021 at 08:15:48AM +0900, Nobuhiro Iwamatsu wrote: > > > > + /* > > > > + * 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. > > > > > > Did you just copy my optimal description or is your hardware really that > > > nice? > > > > Yes, this hardware works as you wrote. > > And I added about the state if the sinnal when this hardware disabled. > > > > > > > > Do you know scripts/checkpatch.pl? I bet it will tell you to limit your > > > lines to approx. 80 chars where sensible. > > > > Yes, I know. I ran scripts/checkpatch.pl before send patch. > > I understand that the number of characters per line has been changed to > > 100 characters. Does the pwm driver recommend 80 characters? > > For free-text comments I'd still recommend 80, yes. For code lines I'd > be indeed more lax, as a line break in function calls reduces readability. Let's not start making any special rules. It becomes impossible for anyone to keep track of those. If checkpatch doesn't complain for comments that exceed 80 characters, I will not reject based on that. Thierry [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2021-04-09 7:48 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-12 13:19 [PATCH v2 0/2] pwm: visconti: Add Toshiba Visconti SoC PWM support Nobuhiro Iwamatsu 2021-02-12 13:19 ` Nobuhiro Iwamatsu 2021-02-12 13:19 ` [PATCH v2 1/2] dt-bindings: pwm: Add bindings for Toshiba Visconti PWM Controller Nobuhiro Iwamatsu 2021-02-12 13:19 ` Nobuhiro Iwamatsu 2021-03-05 20:22 ` Rob Herring 2021-03-05 20:22 ` Rob Herring 2021-04-09 7:44 ` Nobuhiro Iwamatsu 2021-04-09 7:44 ` Nobuhiro Iwamatsu 2021-02-12 13:19 ` [PATCH v2 2/2] pwm: visconti: Add Toshiba Visconti SoC PWM support Nobuhiro Iwamatsu 2021-02-12 13:19 ` Nobuhiro Iwamatsu 2021-02-12 16:41 ` Uwe Kleine-König 2021-02-12 16:41 ` Uwe Kleine-König 2021-04-07 23:15 ` Nobuhiro Iwamatsu 2021-04-07 23:15 ` Nobuhiro Iwamatsu 2021-04-08 7:59 ` Uwe Kleine-König 2021-04-08 7:59 ` Uwe Kleine-König 2021-04-08 12:22 ` Thierry Reding 2021-04-08 12:22 ` Thierry Reding
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.