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 <[email protected]> > --- > 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 <[email protected]> > + * > + */ > + > +#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 <[email protected]>");
> +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/ |
signature.asc
Description: PGP signature
