diff mbox series

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

Message ID 20200917223140.227542-3-nobuhiro1.iwamatsu@toshiba.co.jp (mailing list archive)
State New, archived
Headers show
Series Add Toshiba Visconti SoC PWM support | expand

Commit Message

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

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
Nobuhiro Iwamatsu Feb. 12, 2021, 8:30 a.m. UTC | #3 
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
>
Nobuhiro Iwamatsu Feb. 12, 2021, 8:40 a.m. UTC | #4 
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
diff mbox series

Patch

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