mbox series

[0/2] Add Toshiba Visconti SoC PWM support

Message ID 20200917223140.227542-1-nobuhiro1.iwamatsu@toshiba.co.jp
Headers show
Series Add Toshiba Visconti SoC PWM support | expand

Message

Nobuhiro Iwamatsu Sept. 17, 2020, 10:31 p.m. UTC
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

Comments

Punit Agrawal Sept. 21, 2020, 9 a.m. UTC | #1
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
Uwe Kleine-K├Ânig Sept. 22, 2020, 7:14 a.m. UTC | #2
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