Linux-Watchdog Archive on lore.kernel.org
 help / color / Atom feed
From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Michael Walle <michael@walle.cc>
Cc: linux-gpio@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-hwmon@vger.kernel.org,
	linux-pwm@vger.kernel.org, linux-watchdog@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	Linus Walleij <linus.walleij@linaro.org>,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>,
	Rob Herring <robh+dt@kernel.org>,
	Jean Delvare <jdelvare@suse.com>,
	Guenter Roeck <linux@roeck-us.net>,
	Lee Jones <lee.jones@linaro.org>,
	Thierry Reding <thierry.reding@gmail.com>,
	Wim Van Sebroeck <wim@linux-watchdog.org>,
	Shawn Guo <shawnguo@kernel.org>, Li Yang <leoyang.li@nxp.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Jason Cooper <jason@lakedaemon.net>,
	Marc Zyngier <maz@kernel.org>, Mark Brown <broonie@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>, Pavel Machek <pavel@ucw.cz>
Subject: Re: [PATCH v6 06/13] pwm: add support for sl28cpld PWM controller
Date: Tue, 28 Jul 2020 09:43:52 +0200
Message-ID: <20200728074352.ckyfso5zwsfgupjx@pengutronix.de> (raw)
In-Reply-To: <20200725231834.25642-7-michael@walle.cc>


[-- Attachment #1: Type: text/plain, Size: 9941 bytes --]

Hello,

just a few minor issues left:

On Sun, Jul 26, 2020 at 01:18:27AM +0200, Michael Walle wrote:
> diff --git a/drivers/pwm/pwm-sl28cpld.c b/drivers/pwm/pwm-sl28cpld.c
> new file mode 100644
> index 000000000000..956fa09f3aba
> --- /dev/null
> +++ b/drivers/pwm/pwm-sl28cpld.c
> @@ -0,0 +1,223 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * sl28cpld PWM driver
> + *
> + * Copyright (c) 2020 Michael Walle <michael@walle.cc>
> + *
> + * There is no public datasheet available for this PWM core. But it is easy
> + * enough to be briefly explained. It consists of one 8-bit counter. The PWM
> + * supports four distinct frequencies by selecting when to reset the counter.
> + * With the prescaler setting you can select which bit of the counter is used
> + * to reset it. This implies that the higher the frequency the less remaining
> + * bits are available for the actual counter.
> + *
> + * Let cnt[7:0] be the counter, clocked at 32kHz:
> + * +-----------+--------+--------------+-----------+
> + * | prescaler |  reset | counter bits | frequency |
> + * +-----------+--------+--------------+-----------+
> + * |         0 | cnt[7] |     cnt[6:0] |     250Hz |
> + * |         1 | cnt[6] |     cnt[5:0] |     500Hz |
> + * |         2 | cnt[5] |     cnt[4:0] |      1kHz |
> + * |         3 | cnt[4] |     cnt[3:0] |      2kHz |
> + * +-----------+--------+--------------+-----------+

Very nice. I'd add a "period length" column, as this is what the PWM
core uses.

For your convenience (and as I created that table anyhow for further
checking of the formulas below):

 * +-----------+--------+--------------+-----------+--------+
 * | prescaler |  reset | counter bits | frequency | period |
 * |           |        |              |           | length |
 * +-----------+--------+--------------+-----------+--------+
 * |         0 | cnt[7] |     cnt[6:0] |     250Hz | 4000ns |
 * |         1 | cnt[6] |     cnt[5:0] |     500Hz | 2000ns |
 * |         2 | cnt[5] |     cnt[4:0] |      1kHz | 1000ns |
 * |         3 | cnt[4] |     cnt[3:0] |      2kHz |  500ns |
 * +-----------+--------+--------------+-----------+--------+

> + *
> + * Limitations:
> + * - The hardware cannot generate a 100% duty cycle if the prescaler is 0.
> + * - The hardware cannot atomically set the prescaler and the counter value,
> + *   which might lead to glitches and inconsistent states if a write fails.
> + * - The counter is not reset if you switch the prescaler which leads
> + *   to glitches, too.
> + * - The duty cycle will switch immediately and not after a complete cycle.
> + * - Depending on the actual implementation, disabling the PWM might have
> + *   side effects. For example, if the output pin is shared with a GPIO pin
> + *   it will automatically switch back to GPIO mode.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +
> +/*
> + * PWM timer block registers.
> + */
> +#define SL28CPLD_PWM_CTRL			0x00
> +#define   SL28CPLD_PWM_CTRL_ENABLE		BIT(7)
> +#define   SL28CPLD_PWM_CTRL_PRESCALER_MASK	GENMASK(1, 0)
> +#define SL28CPLD_PWM_CYCLE			0x01
> +#define   SL28CPLD_PWM_CYCLE_MAX		GENMASK(6, 0)
> +
> +#define SL28CPLD_PWM_CLK			32000 /* 32 kHz */
> +#define SL28CPLD_PWM_MAX_DUTY_CYCLE(prescaler)	(1 << (7 - (prescaler)))
> +#define SL28CPLD_PWM_PERIOD(prescaler) \
> +	(NSEC_PER_SEC / SL28CPLD_PWM_CLK * SL28CPLD_PWM_MAX_DUTY_CYCLE(prescaler))
> +
> +/*
> + * We calculate the duty cycle like this:
> + *   duty_cycle_ns = pwm_cycle_reg * max_period_ns / max_duty_cycle
> + *
> + * With
> + *   max_period_ns = (1 << 7 - prescaler) / pwm_clk * NSEC_PER_SEC
> + *   max_duty_cycle = 1 << (7 - prescaler)

If you don't need parenthesis in the max_period_ns around 7 - prescaler,
you don't need them either in the max_duty_cycle line.

> + * this then simplifies to:
> + *   duty_cycle_ns = pwm_cycle_reg / pwm_clk * NSEC_PER_SEC
> + */
> +#define SL28CPLD_PWM_TO_DUTY_CYCLE(reg) \
> +	(NSEC_PER_SEC / SL28CPLD_PWM_CLK * (reg))

For those who copy from your driver maybe add a comment like:

 * NSEC_PER_SEC / SL28CPLD_PWM_CLK is integer here, so we're not loosing
 * precision by doing the division first.

> +#define SL28CPLD_PWM_FROM_DUTY_CYCLE(duty_cycle) \
> +	(DIV_ROUND_DOWN_ULL((duty_cycle), NSEC_PER_SEC / SL28CPLD_PWM_CLK))
> +
> +struct sl28cpld_pwm {
> +	struct pwm_chip pwm_chip;
> +	struct regmap *regmap;
> +	u32 offset;
> +};
> +
> +static void sl28cpld_pwm_get_state(struct pwm_chip *chip,
> +				   struct pwm_device *pwm,
> +				   struct pwm_state *state)
> +{
> +	struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev);
> +	unsigned int reg;
> +	int prescaler;
> +
> +	regmap_read(priv->regmap, priv->offset + SL28CPLD_PWM_CTRL, &reg);

Would it make sense to hide this using e.g.:

	#define sl28cpkd_pwm_read(priv, reg, val)	regmap_read((priv)->regmap, (priv)->offset + (reg), val)

The line would then become:

	sl28cpkd_pwm_read(priv, SL28CPLD_PWM_CTRL, &reg);

which is a bit prettier. Up to you to decide. If you do it, please do
the same for write 

> +	state->enabled = reg & SL28CPLD_PWM_CTRL_ENABLE;
> +
> +	prescaler = FIELD_GET(SL28CPLD_PWM_CTRL_PRESCALER_MASK, reg);
> +	state->period = SL28CPLD_PWM_PERIOD(prescaler);
> +
> +	regmap_read(priv->regmap, priv->offset + SL28CPLD_PWM_CYCLE, &reg);
> +	state->duty_cycle = SL28CPLD_PWM_TO_DUTY_CYCLE(reg);
> +	state->polarity = PWM_POLARITY_NORMAL;
> +}
> +
> +static int sl28cpld_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			      const struct pwm_state *state)
> +{
> +	struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev);
> +	unsigned int cycle, prescaler;
> +	int ret;
> +	u8 ctrl;
> +
> +	/* Polarity inversion is not supported */
> +	if (state->polarity != PWM_POLARITY_NORMAL)
> +		return -EINVAL;
> +
> +	/*
> +	 * Calculate the prescaler. Pick the the biggest period that isn't
> +	 * bigger than the requested period.
> +	 */
> +	prescaler = DIV_ROUND_UP_ULL(SL28CPLD_PWM_PERIOD(0), state->period);
> +	prescaler = order_base_2(prescaler);
> +
> +	if (prescaler > field_max(SL28CPLD_PWM_CTRL_PRESCALER_MASK))
> +		return -ERANGE;

The calculation looks right.
Did you check the generated code? Maybe using an if or switch here is
more effective? (optional task for bonus points :-)

> +	ctrl = FIELD_PREP(SL28CPLD_PWM_CTRL_PRESCALER_MASK, prescaler);
> +	if (state->enabled)
> +		ctrl |= SL28CPLD_PWM_CTRL_ENABLE;
> +
> +	cycle = SL28CPLD_PWM_FROM_DUTY_CYCLE(state->duty_cycle);
> +	cycle = min_t(unsigned int, cycle, SL28CPLD_PWM_MAX_DUTY_CYCLE(prescaler));
> +
> +	/*
> +	 * Work around the hardware limitation. See also above. Trap 100% duty
> +	 * cycle if the prescaler is 0. Set prescaler to 1 instead. We don't
> +	 * care about the frequency because its "all-one" in either case.
> +	 *
> +	 * We don't need to check the actual prescaler setting, because only
> +	 * if the prescaler is 0 we can have this particular value.
> +	 */
> +	if (cycle == SL28CPLD_PWM_MAX_DUTY_CYCLE(0)) {
> +		ctrl &= ~SL28CPLD_PWM_CTRL_PRESCALER_MASK;
> +		ctrl |= FIELD_PREP(SL28CPLD_PWM_CTRL_PRESCALER_MASK, 1);
> +		cycle = SL28CPLD_PWM_MAX_DUTY_CYCLE(1);
> +	}
> +
> +	ret = regmap_write(priv->regmap, priv->offset + SL28CPLD_PWM_CTRL, ctrl);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(priv->regmap, priv->offset + SL28CPLD_PWM_CYCLE, (u8)cycle);

This cast isn't needed, is it?

> +}
> +
> +static const struct pwm_ops sl28cpld_pwm_ops = {
> +	.apply = sl28cpld_pwm_apply,
> +	.get_state = sl28cpld_pwm_get_state,
> +	.owner = THIS_MODULE,
> +};
> +
> +static int sl28cpld_pwm_probe(struct platform_device *pdev)
> +{
> +	struct sl28cpld_pwm *priv;
> +	struct pwm_chip *chip;
> +	int ret;
> +
> +	if (!pdev->dev.parent)
> +		return -ENODEV;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +	if (!priv->regmap)

Error message here?

> +		return -ENODEV;
> +
> +	ret = device_property_read_u32(&pdev->dev, "reg", &priv->offset);
> +	if (ret) {
> +		dev_err(&pdev->dev, "no 'reg' property found (%pe)\n",
> +			ERR_PTR(ret));
> +		return -EINVAL;
> +	}
> +
> +	/* Initialize the pwm_chip structure */
> +	chip = &priv->pwm_chip;
> +	chip->dev = &pdev->dev;
> +	chip->ops = &sl28cpld_pwm_ops;
> +	chip->base = -1;
> +	chip->npwm = 1;
> +
> +	ret = pwmchip_add(&priv->pwm_chip);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to add PWM chip (%pe)",
> +			ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	return 0;
> +}
> +
> +static int sl28cpld_pwm_remove(struct platform_device *pdev)
> +{
> +	struct sl28cpld_pwm *priv = platform_get_drvdata(pdev);
> +
> +	return pwmchip_remove(&priv->pwm_chip);
> +}
> +
> +static const struct of_device_id sl28cpld_pwm_of_match[] = {
> +	{ .compatible = "kontron,sl28cpld-pwm" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, sl28cpld_pwm_of_match);
> +
> +static struct platform_driver sl28cpld_pwm_driver = {
> +	.probe = sl28cpld_pwm_probe,
> +	.remove	= sl28cpld_pwm_remove,
> +	.driver = {
> +		.name = "sl28cpld-pwm",
> +		.of_match_table = sl28cpld_pwm_of_match,
> +	},
> +};
> +module_platform_driver(sl28cpld_pwm_driver);
> +
> +MODULE_DESCRIPTION("sl28cpld PWM Driver");
> +MODULE_AUTHOR("Michael Walle <michael@walle.cc>");
> +MODULE_LICENSE("GPL");

Thanks
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 --]

  parent reply index

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-25 23:18 [PATCH v6 00/13] Add support for Kontron sl28cpld Michael Walle
2020-07-25 23:18 ` [PATCH v6 01/13] mfd: add simple regmap based I2C driver Michael Walle
2020-07-28  7:19   ` Lee Jones
2020-07-28  7:42     ` Michael Walle
2020-07-28  8:15       ` Lee Jones
2020-07-28  8:27         ` Michael Walle
2020-07-28  8:35           ` Lee Jones
2020-07-25 23:18 ` [PATCH v6 02/13] dt-bindings: mfd: Add bindings for sl28cpld Michael Walle
2020-07-28  7:24   ` Lee Jones
2020-07-28  7:57     ` Michael Walle
2020-07-28  8:27       ` Lee Jones
2020-07-28  8:38         ` Michael Walle
2020-07-28  8:56           ` Lee Jones
2020-07-28  9:06             ` Michael Walle
2020-07-28  9:20               ` Lee Jones
2020-07-28  9:39                 ` Michael Walle
2020-07-25 23:18 ` [PATCH v6 03/13] mfd: simple-mfd-i2c: add sl28cpld support Michael Walle
2020-07-28  7:25   ` Lee Jones
2020-07-25 23:18 ` [PATCH v6 04/13] irqchip: add sl28cpld interrupt controller support Michael Walle
2020-07-25 23:18 ` [PATCH v6 05/13] watchdog: add support for sl28cpld watchdog Michael Walle
2020-07-25 23:18 ` [PATCH v6 06/13] pwm: add support for sl28cpld PWM controller Michael Walle
2020-07-27  7:30   ` Thierry Reding
2020-07-28  7:43   ` Uwe Kleine-König [this message]
2020-07-28  8:21     ` Michael Walle
2020-07-28  9:47       ` Uwe Kleine-König
2020-07-28 10:51         ` Lee Jones
2020-07-25 23:18 ` [PATCH v6 07/13] gpio: add support for the sl28cpld GPIO controller Michael Walle
2020-07-26  9:16   ` Andy Shevchenko
2020-07-25 23:18 ` [PATCH v6 08/13] hwmon: add support for the sl28cpld hardware monitoring controller Michael Walle
2020-07-26  9:18   ` Andy Shevchenko
2020-07-25 23:18 ` [PATCH v6 09/13] arm64: dts: freescale: sl28: enable sl28cpld Michael Walle
2020-07-25 23:18 ` [PATCH v6 10/13] arm64: dts: freescale: sl28: map GPIOs to input events Michael Walle
2020-07-25 23:18 ` [PATCH v6 11/13] arm64: dts: freescale: sl28: enable LED support Michael Walle
2020-07-25 23:18 ` [PATCH v6 12/13] arm64: dts: freescale: sl28: enable fan support Michael Walle
2020-07-25 23:18 ` [PATCH v6 13/13] arm64: defconfig: enable the sl28cpld board management controller Michael Walle

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200728074352.ckyfso5zwsfgupjx@pengutronix.de \
    --to=u.kleine-koenig@pengutronix.de \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=bgolaszewski@baylibre.com \
    --cc=broonie@kernel.org \
    --cc=catalin.marinas@arm.com \
    --cc=devicetree@vger.kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=jason@lakedaemon.net \
    --cc=jdelvare@suse.com \
    --cc=lee.jones@linaro.org \
    --cc=leoyang.li@nxp.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-hwmon@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pwm@vger.kernel.org \
    --cc=linux-watchdog@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=maz@kernel.org \
    --cc=michael@walle.cc \
    --cc=pavel@ucw.cz \
    --cc=robh+dt@kernel.org \
    --cc=shawnguo@kernel.org \
    --cc=tglx@linutronix.de \
    --cc=thierry.reding@gmail.com \
    --cc=will@kernel.org \
    --cc=wim@linux-watchdog.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Watchdog Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-watchdog/0 linux-watchdog/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-watchdog linux-watchdog/ https://lore.kernel.org/linux-watchdog \
		linux-watchdog@vger.kernel.org
	public-inbox-index linux-watchdog

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-watchdog


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git